Adjust Trainer v2 precondition to check JobSet CRD #2951
Conversation
WalkthroughAdds a JobSet CRD presence check to the trainer controller's pre-condition validation that returns a StopError with status.JobSetCRDMissingMessage when absent. Introduces JobSetv1alpha2 GVK, a status constant, e2e helpers and a test that validate resource creation, plus unit tests for CRD present/missing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/controller/components/trainer/trainer_controller_actions.go (1)
22-25: JobSet CRD precondition wiring looks correct; minor error message nitThe control flow (operator check first, then CRD check with
cluster.HasCRDandStopErroron missing CRD) is sound and matches the PR intent; no functional issues spotted.The formatted error string is slightly awkward though:
return odherrors.NewStopError("failed to check %s CRDs version: %w", gvk.JobSetv1alpha2, err)Consider tightening it to “CRD version”:
- return odherrors.NewStopError("failed to check %s CRDs version: %w", gvk.JobSetv1alpha2, err) + return odherrors.NewStopError("failed to check %s CRD version: %w", gvk.JobSetv1alpha2, err)Also applies to: 38-45
internal/controller/components/trainer/trainer_controller_actions_test.go (1)
9-18: New JobSet CRD precondition tests are solid; optional setup deduplicationThe two new tests exercise the intended scenarios well:
TestCheckPreConditions_Managed_JobSetCRDNotInstalled: validates that, with the JobSet operator present but no CRD,checkPreConditionsfails and surfacesJobSetCRDMissingMessage.TestCheckPreConditions_Managed_JobSetCRDInstalled: sets up a fake scheme, a JobSet CRD with the expected stored version, and an OperatorCondition, then confirmscheckPreConditionssucceeds.This gives good coverage of the newly introduced CRD check while keeping the existing “operator not installed” case intact.
If you want to tidy things up further, you could extract a small helper to build the
ReconciliationRequest(trainer instance + conditions manager) to reduce the repeated boilerplate across the three tests, but that’s purely optional.Also applies to: 44-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/controller/components/trainer/trainer_controller_actions.go(2 hunks)internal/controller/components/trainer/trainer_controller_actions_test.go(2 hunks)internal/controller/status/status.go(1 hunks)pkg/cluster/gvk/gvk.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/components/trainer/trainer_controller_actions.go (4)
pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (1)
JobSetv1alpha2(641-645)pkg/controller/actions/errors/errors.go (1)
NewStopError(20-24)internal/controller/status/status.go (1)
JobSetCRDMissingMessage(183-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: kube-linter
- GitHub Check: golangci-lint
🔇 Additional comments (2)
internal/controller/status/status.go (1)
181-184: JobSet CRD missing status message is consistent and clearThe new
JobSetCRDMissingMessagefits well with the existing status-message conventions and clearly distinguishes the “CRD missing” case from “operator not installed”. No issues from a correctness or UX standpoint.pkg/cluster/gvk/gvk.go (1)
641-645: JobSetv1alpha2 GVK definition matches expected CRD identity
Group,Version, andKindforJobSetv1alpha2line up with the CRD name used in tests (jobsets.jobset.x-k8s.io) and whatHasCRDexpects. This should integrate cleanly with other GVK constants.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2951 +/- ##
==========================================
+ Coverage 47.06% 47.08% +0.01%
==========================================
Files 148 148
Lines 10443 10448 +5
==========================================
+ Hits 4915 4919 +4
Misses 4981 4981
- Partials 547 548 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/controller/components/trainer/trainer_controller_actions.go(2 hunks)internal/controller/components/trainer/trainer_controller_actions_test.go(2 hunks)internal/controller/status/status.go(1 hunks)pkg/cluster/gvk/gvk.go(1 hunks)tests/e2e/creation_test.go(2 hunks)tests/e2e/helper_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/status/status.go
- internal/controller/components/trainer/trainer_controller_actions.go
- internal/controller/components/trainer/trainer_controller_actions_test.go
- pkg/cluster/gvk/gvk.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/creation_test.go (3)
tests/e2e/resource_options_test.go (3)
WithObjectToCreate(121-133)WithCondition(262-266)WithCustomErrorMsg(287-291)tests/e2e/helper_test.go (1)
CreateJobSetOperator(406-420)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)
🔇 Additional comments (1)
tests/e2e/helper_test.go (1)
405-420: LGTM with verification needed.The helper function correctly constructs an unstructured JobSetOperator resource following the same pattern as other helper functions in this file (e.g.,
CreateHardwareProfile). The structure is appropriate for e2e test resource creation.However, please verify the apiVersion and resource configuration as noted in the review comment for
creation_test.go.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CFSNM The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Description
Trainer v2 controller requires JobSet CRD to be available to start properly.
Adding JobSet CRD check to trainer component preconditions to properly indicate JobSet CRD unavailability in DSC status.
How Has This Been Tested?
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
Summary by CodeRabbit
New Features
Tests
Misc
✏️ Tip: You can customize this high-level summary in your review settings.