Gate DRA reconciler on Openshift >= 4.21#1842
Conversation
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughIntroduces a shared ChangesVersion discovery and DRA gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Manager as cmd/manager/main.go
participant VersionPkg as internal/version
participant API as Kubernetes API Server
participant DRAController as DRA Reconciler
Manager->>VersionPkg: DiscoverOCPVersion(restCfg)
VersionPkg->>API: ServerVersion()
API-->>VersionPkg: GitVersion string
VersionPkg->>VersionPkg: ParseOCPVersion(gitVersion)
VersionPkg-->>Manager: OCPVersion{Major, Minor}
alt version >= MinOCPMajorForDRA/Minor
Manager->>DRAController: setup DRA reconciler
else version too old
Manager->>Manager: log skip message
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The DRA reconciler was registered unconditionally, which would crash the entire operator on clusters older than 4.21 because the resource.k8s.io/v1 DeviceClass API does not exist there. Introduce an internal/version package with OCPVersion, DiscoverOCPVersion, and an AtLeast method to centralise version discovery and comparison. The manager now checks the cluster version at startup and only registers the DRA controller when the minimum version is met. The webhook package is updated to use the shared version package instead of its own local types. Also fixes a pre-existing bug where /run was missing from the allowed hostPath prefixes error message.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/version/version.go (2)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment typo: "kv" should refer to the receiver
ov.✏️ Proposed fix
-// AtLeast returns true if kv is greater than or equal to the given major.minor version. +// AtLeast returns true if ov is greater than or equal to the given major.minor version. func (ov OCPVersion) AtLeast(major, minor int) bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/version/version.go` around lines 36 - 39, The doc comment for OCPVersion.AtLeast has a typo and should refer to the receiver ov instead of kv. Update the comment on AtLeast so it accurately describes ov as the version being compared, keeping the rest of the method unchanged.
42-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
%winstead of%vfor error wrapping.
DiscoverOCPVersionwraps errors with%v, discarding the underlying error forerrors.Is/errors.Aschecks, whileParseOCPVersionbelow uses%w. Aligning both improves error-chain consistency.♻️ Proposed fix
discClient, err := discovery.NewDiscoveryClientForConfig(cfg) if err != nil { - return OCPVersion{}, fmt.Errorf("failed to create discovery client: %v", err) + return OCPVersion{}, fmt.Errorf("failed to create discovery client: %w", err) } serverVersion, err := discClient.ServerVersion() if err != nil { - return OCPVersion{}, fmt.Errorf("failed to query OpenShift server version: %v", err) + return OCPVersion{}, fmt.Errorf("failed to query OpenShift server version: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/version/version.go` around lines 42 - 54, DiscoverOCPVersion is formatting wrapped errors with %v, which drops the original error chain. Update the error returns in DiscoverOCPVersion to use %w for both the discovery.NewDiscoveryClientForConfig and discClient.ServerVersion failures so they behave consistently with ParseOCPVersion and preserve errors.Is/errors.As compatibility.internal/version/version_test.go (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDescribe block name mismatches tested function.
Says "ParseKubeVersion" but exercises
ParseOCPVersion.✏️ Proposed fix
-var _ = Describe("ParseKubeVersion", func() { +var _ = Describe("ParseOCPVersion", func() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/version/version_test.go` at line 46, The Describe block name is misleading because it says ParseKubeVersion while the tests actually cover ParseOCPVersion. Update the Describe string in the version_test.go suite to match the real target function name, and keep the surrounding ParseOCPVersion assertions aligned with that renamed block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/manager/main.go`:
- Around line 130-139: Version discovery in main startup is currently fatal for
the whole manager, so a discovery/RBAC/API error can block every controller
instead of only DRA setup. Update the startup flow around
version.DiscoverOCPVersion and the subsequent cmd.FatalError handling to treat
discovery failures as non-fatal: log a warning and continue registering the
manager and other controllers. Then gate the DRA reconciler setup on both a
successful discovery result and the version check, using the existing ocpVersion
/ ocpVersionErr path in main.go.
---
Nitpick comments:
In `@internal/version/version_test.go`:
- Line 46: The Describe block name is misleading because it says
ParseKubeVersion while the tests actually cover ParseOCPVersion. Update the
Describe string in the version_test.go suite to match the real target function
name, and keep the surrounding ParseOCPVersion assertions aligned with that
renamed block.
In `@internal/version/version.go`:
- Around line 36-39: The doc comment for OCPVersion.AtLeast has a typo and
should refer to the receiver ov instead of kv. Update the comment on AtLeast so
it accurately describes ov as the version being compared, keeping the rest of
the method unchanged.
- Around line 42-54: DiscoverOCPVersion is formatting wrapped errors with %v,
which drops the original error chain. Update the error returns in
DiscoverOCPVersion to use %w for both the discovery.NewDiscoveryClientForConfig
and discClient.ServerVersion failures so they behave consistently with
ParseOCPVersion and preserve errors.Is/errors.As compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c40e53e6-8959-4ab7-81a3-b04e258cbb98
📒 Files selected for processing (7)
cmd/manager/main.gocmd/webhook-server/main.gointernal/version/version.gointernal/version/version_test.gointernal/webhook/hub/managedclustermodule_webhook.gointernal/webhook/module.gointernal/webhook/module_test.go
| restCfg := ctrl.GetConfigOrDie() | ||
|
|
||
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), options) | ||
| ocpVersion, err := version.DiscoverOCPVersion(restCfg) | ||
| if err != nil { | ||
| cmd.FatalError(setupLogger, err, "unable to discover Openshift version") | ||
| } | ||
|
|
||
| setupLogger.Info("Detected Openshift version", "major", ocpVersion.Major, "minor", ocpVersion.Minor) | ||
|
|
||
| mgr, err := ctrl.NewManager(restCfg, options) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Version-discovery failure crashes the entire manager, not just DRA setup.
version.DiscoverOCPVersion is called before the manager is created, and any error (not only "version too old") triggers cmd.FatalError. This means a transient discovery-client error, RBAC hiccup, or unreachable API server prevents all controllers from starting — even though only the DRA reconciler actually needs this information. The stated goal of this change is to avoid crashing the operator on unsupported clusters, but this introduces a new, broader crash surface. There's also no timeout/context passed into the discovery call, so a slow/unreachable API server could hang startup indefinitely rather than fail fast.
Consider treating discovery failure as non-fatal: log a warning and skip DRA reconciler setup (similar to the "insufficient version" branch), so the rest of the controllers still register.
💡 Suggested approach
- ocpVersion, err := version.DiscoverOCPVersion(restCfg)
- if err != nil {
- cmd.FatalError(setupLogger, err, "unable to discover Openshift version")
- }
-
- setupLogger.Info("Detected Openshift version", "major", ocpVersion.Major, "minor", ocpVersion.Minor)
+ ocpVersion, ocpVersionErr := version.DiscoverOCPVersion(restCfg)
+ if ocpVersionErr != nil {
+ setupLogger.Error(ocpVersionErr, "unable to discover OpenShift version; DRA controller will be disabled")
+ } else {
+ setupLogger.Info("Detected OpenShift version", "major", ocpVersion.Major, "minor", ocpVersion.Minor)
+ }Then guard the DRA setup on ocpVersionErr == nil && ocpVersion.AtLeast(...).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| restCfg := ctrl.GetConfigOrDie() | |
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), options) | |
| ocpVersion, err := version.DiscoverOCPVersion(restCfg) | |
| if err != nil { | |
| cmd.FatalError(setupLogger, err, "unable to discover Openshift version") | |
| } | |
| setupLogger.Info("Detected Openshift version", "major", ocpVersion.Major, "minor", ocpVersion.Minor) | |
| mgr, err := ctrl.NewManager(restCfg, options) | |
| restCfg := ctrl.GetConfigOrDie() | |
| ocpVersion, ocpVersionErr := version.DiscoverOCPVersion(restCfg) | |
| if ocpVersionErr != nil { | |
| setupLogger.Error(ocpVersionErr, "unable to discover OpenShift version; DRA controller will be disabled") | |
| } else { | |
| setupLogger.Info("Detected OpenShift version", "major", ocpVersion.Major, "minor", ocpVersion.Minor) | |
| } | |
| mgr, err := ctrl.NewManager(restCfg, options) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/manager/main.go` around lines 130 - 139, Version discovery in main
startup is currently fatal for the whole manager, so a discovery/RBAC/API error
can block every controller instead of only DRA setup. Update the startup flow
around version.DiscoverOCPVersion and the subsequent cmd.FatalError handling to
treat discovery failures as non-fatal: log a warning and continue registering
the manager and other controllers. Then gate the DRA reconciler setup on both a
successful discovery result and the version check, using the existing ocpVersion
/ ocpVersionErr path in main.go.
TomerNewman
force-pushed
the
fix/gate-dra-reconciler-on-kube-version
branch
from
902d1e2 to
8590213
Compare
|
/lgtm |
openshift-merge-bot
Bot
merged commit fbf4c8f
into
rh-ecosystem-edge:main
3 participants
The DRA reconciler was registered unconditionally, which would crash the entire operator on clusters older than 4.21 because the resource.k8s.io/v1 DeviceClass API does not exist there.
Introduce an internal/version package with OCPVersion, DiscoverOCPVersion, and an AtLeast method to centralise version discovery and comparison. The manager now checks the cluster version at startup and only registers the DRA controller when the minimum version is met. The webhook package is updated to use the shared version package instead of its own local types.
Also fixes a pre-existing bug where /run was missing from the allowed hostPath prefixes error message.
/cc @ybettan @yevgeny-shnaidman
/assign @yevgeny-shnaidman @ybettan
fix #1841
Summary by CodeRabbit
New Features
Bug Fixes
/runas an allowed path.