Skip to content

Conversation

@karthikvetrivel
Copy link
Member

@karthikvetrivel karthikvetrivel commented Oct 14, 2025

This PR is the second step for migrating GPU Operator e2e tests from bash to Go/Ginkgo.

Overview

This PR introduces a comprehensive set of helper utilities that leverage the GPU operator's generated Go clientsets (api/versioned).

Note: This PR contains only the helper function infrastructure. Actual test implementations using these helpers will be added in a follow-up PR.

Changes

1. Helper Infrastructure (tests/e2e/helpers/)

Created comprehensive helper utilities:

  • operator.go - Helm client for deploying GPU Operator
  • clusterpolicy.go - ClusterPolicy CRUD + component toggles (DCGM, GFD, etc.)
  • nvidiadriver.go - NVIDIADriver CR operations (cluster-scoped)
  • daemonset.go - DaemonSet queries and readiness checks
  • node.go - Node labeling operations
  • workload.go - GPU workload deployment and verification
  • pod.go - Pod operations and namespace management

Structure:

  • Flattened directory: moved operator/helm.gooperator.go, kubernetes/pod.gopod.go
  • Updated imports in gpu_operator_test.go

2. Constants (tests/e2e/constants.go)

Added shared constants:

  • DefaultPollingInterval - Standard 5s interval for wait operations
  • UpgradeDoneState - Driver upgrade completion state

3. Updated Existing Tests

Modified gpu_operator_test.go to use new helper package structure:

  • operator.Clienthelpers.OperatorClient
  • k8stest.Clienthelpers.PodClient
  • Improved error handling for pod log retrieval

Next Steps

  • Step 3: Migrate bash test scenarios to Go using these helpers
  • Step 4: Update CI/CD pipelines to run Go-based e2e tests

@karthikvetrivel karthikvetrivel force-pushed the refactor/e2e-migration branch 2 times, most recently from 0313d88 to 1b4d24e Compare October 15, 2025 18:31
Copy link
Contributor

@rajathagasthya rajathagasthya left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing the comments!

@karthikvetrivel
Copy link
Member Author

Moving this to draft until I add tests to keep the github.com/NVIDIA/gpu-operator dep (by referencing it in the e2e/tests code).

@karthikvetrivel karthikvetrivel marked this pull request as draft October 17, 2025 21:09
@karthikvetrivel karthikvetrivel force-pushed the refactor/e2e-migration branch 4 times, most recently from 99feca6 to 10ad3e0 Compare October 20, 2025 04:16
@karthikvetrivel karthikvetrivel marked this pull request as ready for review October 20, 2025 12:50
@rajathagasthya
Copy link
Contributor

@karthikvetrivel Could you update helm.sh/helm/v3 dependency to address Dependabot alerts in https://github.com/NVIDIA/gpu-operator/security/dependabot? go get helm.sh/helm/v3@latest should work.

@karthikvetrivel
Copy link
Member Author

@rajathagasthya This would require updating the main module's Kubernetes dependencies from v0.33.2 to v0.34.0. Should we still do it?

@rajathagasthya
Copy link
Contributor

@tariq1890 updated it recently in #1805, so we should be able to contain these changes to just the e2e module.

@karthikvetrivel
Copy link
Member Author

@rajathagasthya Fixed and amended. Thanks!

@coderabbitai
Copy link

coderabbitai bot commented Oct 23, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

Thanks @rajathagasthya PR LGTM, I'll leave final approval to @tariq1890 and @cdesiniotis

@karthikvetrivel
Copy link
Member Author

@cdesiniotis @tariq1890 Bumping this PR again for final review when you guys have the chance.

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.

Thanks @karthikvetrivel! I left a few comments, most are not blockers.

Comment on lines +75 to +76
if daemonSet.Status.NumberReady == daemonSet.Status.DesiredNumberScheduled &&
daemonSet.Status.NumberReady > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an observation -- we check for daemonset readiness in several places. Most notably:

func isDaemonSetReady(name string, n ClusterPolicyController) gpuv1.State {

func (s *stateSkel) isDaemonSetReady(uds *unstructured.Unstructured, reqLogger logr.Logger) (bool, error) {

It may be in our best interest to align all these implementations at some point and reuse the same helper (if it makes sense). This is obviously out of scope for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense and agreed. It's a good item to track for future refactoring.

Comment on lines +115 to +117
if !strings.Contains(logs, "NVIDIA") && !strings.Contains(logs, "GPU") {
return fmt.Errorf("pod logs do not contain evidence of GPU access")
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 with this for now, but we probably want to improve upon this in future iterations. E.g. this method could exec into the main container and invoke nvidia-smi itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I added a note for this. Once I add the tests for these helpers, I will look into implementing this.

@karthikvetrivel karthikvetrivel dismissed ArangoGutierrez’s stale review November 19, 2025 18:46

Received LGTM, closing this review.

@karthikvetrivel karthikvetrivel merged commit e8061d2 into NVIDIA:main Nov 19, 2025
16 checks passed
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.

5 participants