feat(cpo): use HO image for CPO on 4.20+ clusters #7446
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughChanges update container build files to include control-plane-operator and control-plane-pki-operator binaries with symlink configuration and new metadata label. The image selection logic adds CPO binary detection with caching to check HO image availability before consulting release payload. Tests validate the updated precedence logic across multiple scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
do-not-merge/needs-area
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee 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 |
openshift-ci
bot
added
area/hypershift-operator
|
/test verify |
|
/test e2e-aws-4-20 |
|
@muraee: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @support/util/util.go:
- Line 686: The version check using "if version.Major >= 4 && version.Minor >=
20 && cpoBinaryExistsInHOImage()" is incorrect for majors >4 and for majors <4;
update the condition in util.go to correctly test "version >= 4.20" by either
constructing a semver threshold (e.g., create minVersionForHOImage :=
semver.Version{Major:4, Minor:20, Patch:0} and use
version.GTE(minVersionForHOImage)) or use an equivalent comparison like "if
version.Major > 4 || (version.Major == 4 && version.Minor >= 20) &&
cpoBinaryExistsInHOImage()", keeping the check against
cpoBinaryExistsInHOImage() unchanged and referencing the existing version
variable and cpoBinaryExistsInHOImage() call.
- Around line 633-647: The package-level cpoBinaryExists is racy; replace the
manual nil-check/write in cpoBinaryExistsInHOImage with a thread-safe
initialization using sync.Once (add a package-level sync.Once, e.g.
cpoBinaryOnce) or a sync.Mutex; call cpoBinaryOnce.Do(func(){ stat
os.Stat(cpoBinaryPath) and set cpoBinaryExists = &exists }) inside
cpoBinaryExistsInHOImage and then return *cpoBinaryExists so reads/writes are
synchronized and the value is computed exactly once.
🧹 Nitpick comments (2)
support/util/util_test.go (2)
1128-1134: Test setup directly mutates package-level state.The test sets
cpoBinaryExistsdirectly (line 1133), which works with the current pointer-based caching but would break ifsync.Onceis adopted per the suggestion inutil.go. Consider abstracting the binary existence check via a function variable or interface to improve testability.♻️ Suggestion for improved testability
In
util.go, use a function variable that can be replaced in tests:// cpoBinaryExistsFunc is the function used to check CPO binary existence. // It can be replaced in tests. var cpoBinaryExistsFunc = cpoBinaryExistsInHOImageThen in tests:
cpoBinaryExistsFunc = func() bool { return tc.cpoBinaryExists } defer func() { cpoBinaryExistsFunc = cpoBinaryExistsInHOImage }()
1045-1126: Good test coverage, but consider adding a test case for major version > 4.The test cases comprehensively cover 4.x versions, but given the version comparison bug noted in
util.go, adding a test case for version 5.0+ would help catch regressions and validate the fix.{ name: "When version is 5.0 and CPO binary exists it should use HO image", version: "5.0.0", payloadHasHypershift: true, cpoBinaryExists: true, expectedImage: hoImage, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
Containerfile.operatorDockerfilesupport/util/util.gosupport/util/util_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
Containerfile.operatorsupport/util/util.goDockerfilesupport/util/util_test.go
🧬 Code graph analysis (1)
support/util/util_test.go (3)
support/releaseinfo/releaseinfo.go (1)
ReleaseImage(39-42)api/hypershift/v1beta1/hostedcluster_types.go (1)
ControlPlaneOperatorImageAnnotation(59-59)support/util/util.go (1)
GetControlPlaneOperatorImage(663-698)
⏰ 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). (5)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-gomaxprocs-webhook-on-pull-request
🔇 Additional comments (6)
Dockerfile (2)
7-29: LGTM! Build stage and binary packaging changes are well-structured.The additions correctly build and package the control-plane-operator and control-plane-pki-operator binaries, with appropriate symlinks for the multi-call binary pattern where ignition-server, konnectivity-socks5-proxy, availability-prober, and token-minter all resolve to control-plane-operator.
45-45: LGTM! New capability label added.The label correctly signals that this image supports the kas-custom-kubeconfig feature.
Containerfile.operator (2)
7-29: LGTM! Changes mirror Dockerfile appropriately.The build stage and binary packaging changes are consistent with the Dockerfile, ensuring both container build paths produce equivalent images.
54-54: LGTM! Label added consistently with Dockerfile.support/util/util.go (1)
649-661: LGTM! Documentation accurately reflects the updated precedence logic.The docstring clearly explains the five-level precedence hierarchy for CPO image resolution.
support/util/util_test.go (1)
1016-1036: LGTM! Clean fake release provider implementation.The
testReleaseProvidercorrectly constructs aReleaseImagewith the version in the ImageStream name and component images in tags.
|
/test e2e-aws |
|
/cc @rtheis |
|
/test e2e-aws |
|
@muraee: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
For cluster versions 4.20 and above, use the HyperShift Operator image directly for the Control Plane Operator instead of extracting it from the OCP release payload. This enables: - Faster feature delivery for CPO (ships with HO releases) - Simplified hotfix process (single HO image bump fixes all clusters) - Consistent deployment model between managed and self-managed The change includes a safety check that verifies the control-plane-operator binary exists in the HO image before using it. This ensures backward compatibility with older HO images that don't include the CPO binary - they will continue to use the release payload CPO. Dockerfiles are updated to: - Build and include control-plane-operator and control-plane-pki-operator - Add symlinks for ignition-server, konnectivity-socks5-proxy, etc. - Add missing CPO feature discovery labels 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
muraee
force-pushed
the
use-ho-image-for-cpo-420
branch
from
bac4dde to
1141357
Compare
2 participants
Summary
For cluster versions 4.20 and above, use the HyperShift Operator image directly for the Control Plane Operator instead of extracting it from the OCP release payload.
Benefits
Changes
support/util/util.go: ModifiedGetControlPlaneOperatorImage()to use HO image for 4.20+ if the CPO binary existssupport/util/util_test.go: Added comprehensive unit tests for the new behaviorDockerfile&Containerfile.operator:control-plane-operatorandcontrol-plane-pki-operatorbinariesignition-server,konnectivity-socks5-proxy,availability-prober,token-minterio.openshift.hypershift.control-plane-operator-supports-kas-custom-kubeconfig=trueSafety Mechanism
The code includes a safety check that verifies
/usr/bin/control-plane-operatorexists in the HO image before using it. This ensures:Behavior Matrix
Test plan
GetControlPlaneOperatorImage🤖 Generated with Claude Code