Skip to content

refactor: update Kueue management state checks and messaging#52

Merged
lburgazzoli merged 1 commit intoopendatahub-io:mainfrom
andyatmiami:feat/prohibit-kueue-unmanaged
Mar 19, 2026
Merged

refactor: update Kueue management state checks and messaging#52
lburgazzoli merged 1 commit intoopendatahub-io:mainfrom
andyatmiami:feat/prohibit-kueue-unmanaged

Conversation

@andyatmiami
Copy link
Copy Markdown
Contributor

@andyatmiami andyatmiami commented Mar 19, 2026

Description

  • Deregistered the Kueue OperatorInstalledCheck in command.go, with a note to re-enable it in a future release that supports the Unmanaged state.
  • Enhanced the ManagementStateCheck to validate that only the Removed state is supported for Kueue before upgrading to RHOAI 3.3.1, updating the remediation messages accordingly.
  • Renamed test functions to reflect the new logic regarding management states, ensuring clarity in test outcomes.
  • Updated the DataIntegrityCheck to focus on the Unmanaged state, aligning checks with the new management state requirements.

This refactor improves clarity and correctness in Kueue management state validations, ensuring users are informed about the necessary migration steps for upgrades.

How Has This Been Tested?

Kueue Unmanaged

  1. Create a 2.25.3 RHOAI instance
  2. Create sample data that includes "invariant checks" we want to guard against
  3. ➜ odh-cli/ git:(feat/agressive-kueue-gating) $ ./bin/kubectl-odh lint --target-version 3.3 --verbose

Look for prohibited severity in output

ℹ️ For Unmanaged - we are still running both the managementState check as well as the data-integrity check for Kueue... as the managementState is a point-in-time strategy that will be relaxed in the future - while the data-integrity check will continue to be required... so I felt it necessary to still continue to run the data-integrity check in this case

TOP LEVEL BANNER / SUMMARY TABLE

image

VERBOSE OUTPUT TABLE

image

RESULT OUTPUT

image

Kueue Managed

  1. Leverage existing BU cluster ( the "big" / "bad" one I use frequently for regression testing)
    • https://console-openshift-console.apps.prod.rhoai.rh-aiservices-bu.com
  2. ➜ odh-cli/ git:(feat/agressive-kueue-gating) $ ./bin/kubectl-odh lint --target-version 3.3 --verbose --debug

Look for prohibited severity in output

SUMMARY TABLE

image

Kueue Removed (and/or not present)

  1. Create a 2.25.3 RHOAI instance *without Kueue
  2. Create simple kueue-less sample data
  3. ➜ odh-cli/ git:(feat/agressive-kueue-gating) $ ./bin/kubectl-odh lint --target-version 3.3 --verbose

SUMMARY TABLE

image

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • Bug Fixes

    • Updated Kueue management-state compatibility rules for RHOAI 3.x: Both "Managed" and "Unmanaged" states are now marked as prohibited, with updated error messaging.
  • Chores

    • Deregistered the Kueue Operator installed check; scheduled for re-enablement in a future release.
    • Refined validation logic for Kueue state detection.

- Deregistered the Kueue OperatorInstalledCheck in command.go, with a note to re-enable it in a future release that supports the Unmanaged state.
- Enhanced the ManagementStateCheck to validate that only the Removed state is supported for Kueue before upgrading to RHOAI 3.3.1, updating the remediation messages accordingly.
- Renamed test functions to reflect the new logic regarding management states, ensuring clarity in test outcomes.
- Updated the DataIntegrityCheck to focus on the Unmanaged state, aligning checks with the new management state requirements.

This refactor improves clarity and correctness in Kueue management state validations, ensuring users are informed about the necessary migration steps for upgrades.

Signed-off-by: Andy Stoneberg <astonebe@redhat.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 19, 2026

📝 Walkthrough

Walkthrough

This PR modifies Kueue management-state validation logic for RHOAI 3.x. Both ManagementStateManaged and ManagementStateUnmanaged states are now marked as prohibited with fixed message constants and ImpactProhibited, reversing prior conditional compatibility handling. The helper function IsKueueActive is renamed to IsKueueUnmanaged with inverted semantics (now returns true only for Unmanaged state). The OperatorInstalledCheck is deregistered from the command registry with a deferred registration note for version 3.3.x. Test cases and fixtures are updated to reflect new prohibition semantics.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes


Actionable Issues

1. Breaking API Semantic Change Without Deprecation
Function IsKueueActive in support.go is renamed to IsKueueUnmanaged with inverted semantics. While the summary indicates this is internal-only, verify no external imports or public API contracts depend on the original function name and behavior. If this function is part of a public package interface, a deprecation wrapper should precede removal.

2. Policy Inversion Requires Validation Across Integration Points
The change from conditionally allowing Unmanaged to prohibiting both Managed and Unmanaged is a breaking behavior shift. Confirm all affected check invocations (CanApply calls) have been audited. The workloads/kueue/check.go predicate change from IsKueueActive to IsKueueUnmanaged inverts the applicability logic—ensure downstream impact assessment of workloads that previously passed validation.

3. Error Handling Addition for Unexpected Management State
The new fmt.Errorf(...) return for unexpected req.ManagementState values in kueue.go is operationally sound. However, verify all callers of the affected function handle the error case explicitly rather than silently ignoring it, as prior logic may have assumed no error path.

4. Deregistration Without Clear Sunset Path
The OperatorInstalledCheck deregistration via commented-out code in command.go leaves ambiguity. Document the expected re-registration trigger version (3.3.x mentioned in doc comment) and removal deadline. Without versioned cleanup, commented code can become technical debt.

5. Test Fixture Bulk Update Risk
Changing all test fixtures from "Managed" to "Unmanaged" across 19 lines in check_test.go without individual test-case justification creates difficulty in auditing test intent per scenario. Verify each updated test fixture actually requires the "Unmanaged" state for its validation logic rather than being a blanket substitution.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately describes the primary change: updating Kueue management state checks and messaging throughout the codebase.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

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.

@andyatmiami andyatmiami marked this pull request as ready for review March 19, 2026 19:18
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pkg/lint/checks/components/kueue/kueue.go (1)

22-24: TODO for version parameterization is tracked.

Hardcoded "3.3.1" in messages is acknowledged by the deferred TODO. Consider creating a follow-up issue to avoid stale version references in future releases.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/checks/components/kueue/kueue.go` around lines 22 - 24, The two
user-facing constants msgManagedProhibited and msgUnmanagedProhibited hardcode
the version "3.3.1"; update them to reference a parameterized version instead
(using ComponentRequest.TargetVersion or a helper that formats the target
version) so messages reflect the actual target version at runtime; change the
string construction so it inserts the target version value into the message text
for both msgManagedProhibited and msgUnmanagedProhibited and ensure any callers
of those constants use the runtime-formatted message rather than the static
constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/lint/checks/components/kueue/kueue.go`:
- Around line 22-24: The two user-facing constants msgManagedProhibited and
msgUnmanagedProhibited hardcode the version "3.3.1"; update them to reference a
parameterized version instead (using ComponentRequest.TargetVersion or a helper
that formats the target version) so messages reflect the actual target version
at runtime; change the string construction so it inserts the target version
value into the message text for both msgManagedProhibited and
msgUnmanagedProhibited and ensure any callers of those constants use the
runtime-formatted message rather than the static constants.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: d351b4ad-f64e-48d1-8689-febff05feba5

📥 Commits

Reviewing files that changed from the base of the PR and between 95906e8 and 7089d45.

📒 Files selected for processing (7)
  • pkg/lint/checks/components/kueue/kueue.go
  • pkg/lint/checks/components/kueue/kueue_operator_installed.go
  • pkg/lint/checks/components/kueue/kueue_test.go
  • pkg/lint/checks/workloads/kueue/check.go
  • pkg/lint/checks/workloads/kueue/check_test.go
  • pkg/lint/checks/workloads/kueue/support.go
  • pkg/lint/command.go

Copy link
Copy Markdown
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm

Reading through the changes, as user i can understand that unmanaged and managed, managementState of kueue are prohibited and upgrade cant be performed.
👍

@lburgazzoli lburgazzoli merged commit 74f26e9 into opendatahub-io:main Mar 19, 2026
5 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.

3 participants