Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-compile end-to-end gpu driver validation #91

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

shivakunv
Copy link
Contributor

No description provided.

@shivakunv shivakunv self-assigned this Aug 22, 2024
@shivakunv shivakunv force-pushed the e2etestdriver branch 8 times, most recently from 3fe0f83 to 9c84eaf Compare August 22, 2024 15:24
@shivakunv shivakunv requested a review from cdesiniotis August 22, 2024 15:31
@shivakunv shivakunv marked this pull request as ready for review August 22, 2024 15:31
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
vgpu/src/go.mod Outdated Show resolved Hide resolved
vgpu/src/go.mod Outdated Show resolved Hide resolved
vgpu/src/go.sum Outdated Show resolved Hide resolved
@shivakunv shivakunv force-pushed the e2etestdriver branch 13 times, most recently from 03e0cc4 to 2563c6f Compare August 23, 2024 17:01
@shivakunv shivakunv force-pushed the e2etestdriver branch 8 times, most recently from bbcbf64 to f3aba06 Compare September 9, 2024 06:01
@shivakunv
Copy link
Contributor Author

@cdesiniotis PTAL

.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
tests/scripts/findkernelversion.sh Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@cdesiniotis cdesiniotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added our comments during a peer-review.

For a follow-up, let's investigate how we can prevent the gitlab-master pipelines from publishing tags when the e2e tests fail. One idea is to push the tags initially to a staging location on ghcr.io, and only after the tests pass do we copy them over to a different location. The internal pipelines would be configured to pull the tags from the latter location.

.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
tests/ci-run-e2e.sh Outdated Show resolved Hide resolved
@shivakunv shivakunv force-pushed the e2etestdriver branch 12 times, most recently from ed8ced1 to 55ef518 Compare September 10, 2024 16:20
Copy link
Contributor

@cdesiniotis cdesiniotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shivakunv thanks for the patience and work on this. I left a few more questions / comments. None of them are blocking and I am fine to address some cleanup / improvements in follow-ups. I definitely want to make the Github Action steps more readable / maintainable. It will also help us in the future when we want to start building precompiled images for other distributions, i.e. ubuntu24.04.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
driver_branch=$(echo "$driver_branch_json" | jq -r '.[]')

kernel_versions=()
for kernel_flavor in $kernel_flavors; do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay if we do this in a follow-up, but I would like to clean this up a bit for readability. We should consider abstracting away as much of this as possible in a script that we can call from here. For example, we can simplify this entire code block to just:

source ./tests/scripts/ci-precompiled-helpers.sh
KERNEL_VERSIONS=$(get_kernel_versions_to_test $BASE_TARGET $DRIVER_BRANCHES $KERNEL_FLAVORS)
if [ -z "$KERNEL_VERSIONS" ]; then
  exit
fi
# Convert array to JSON format and assign
echo "[]" > $GITHUB_WORKSPACE/matrix_values.json
printf '%s\n' "${KERNEL_VERSIONS[@]}" | jq -R . | jq -s . > $GITHUB_WORKSPACE/matrix_values.json
echo "matrix_values=$(cat $GITHUB_WORKSPACE/matrix_values.json | jq -c .)" >> $GITHUB_OUTPUT
echo "matrix_values_not_empty=1" >> $GITHUB_OUTPUT

We should make this extensible, so in the future we can reuse this script for different distributions, e.g. ubuntu24.04.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted, TODO added

tests/local.sh Outdated Show resolved Hide resolved
export DRIVER_BRANCH="${3}"
export DIST="${4}"

export REGCTL_VERSION=v0.4.7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the latest regctl version?

Not blocking -- we may want to consider adding a separate step in any GH action that requires regctl. That way we can define / maintain the version of regctl at the top-level where the GH actions are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted, TODO added

tests/ci-run-e2e.sh Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/precompiled.yaml Outdated Show resolved Hide resolved
tests/ci-run-e2e.sh Show resolved Hide resolved
@shivakunv
Copy link
Contributor Author

@cdesiniotis
Thanks for reviewing . updated the comments

@shivakunv shivakunv merged commit 3e44e3b into main Sep 12, 2024
9 checks passed
@shivakunv shivakunv deleted the e2etestdriver branch December 9, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants