CNTRLPLANE-993: feat(API): Add controlPlaneUpToDate condition#6300
CNTRLPLANE-993: feat(API): Add controlPlaneUpToDate condition#6300enxebre wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
do-not-merge/work-in-progress
openshift-ci
bot
added
area/api
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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/documentation
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
from
89ceb1c to
93c7d44
Compare
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
2 times, most recently
from
f50b3d8 to
25453b2
Compare
|
/lgtm |
typeid
left a comment
typeid
left a comment
There was a problem hiding this comment.
Added some very minor nits on the comment grammar, logic wise it looks good to me though. Thanks for the heads up on this :)
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
from
25453b2 to
3ab02fa
Compare
openshift-ci
bot
added
area/testing
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
2 times, most recently
from
5e97854 to
5ed926d
Compare
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
![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
♻️ Duplicate comments (3)
api/hypershift/v1beta1/hostedcluster_conditions.go (1)
23-29: Tighten wording; add an explicit semantics note.Small doc polish and a clarifying sentence that the Status reflects version alignment only (availability/rollout carried in Message). Mirrors the current implementation behavior.
Apply this diff:
- // ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version. - // This is a useful semantic to make decisions decoupled from the ability of components running on the data plane to roll out during an upgrade to a specific version. - // For example, acknowledge that none of the components running management side are vulnerable to a specific CVE. - // For example, don't block HostedCluster upgrades when there's no data plane compute. - // Until https://issues.redhat.com/browse/OCPSTRAT-1454 is fixed, this condition has limited value, as the versions of OVN Control plane components are still dictated by the ability of the data plane to rollout. + // ControlPlaneUpToDate indicates whether the control-plane components running on the management side are up to date with the desired release version. + // This is useful for decisions decoupled from the data-plane’s ability to roll out during an upgrade to a specific version. + // For example, acknowledge that management-side components are not vulnerable to a specific CVE. + // For example, do not block HostedCluster upgrades when there is no data-plane compute. + // Note: the condition Status reflects version alignment only; component availability and rollout details are reported in the Message for context. + // Until https://issues.redhat.com/browse/OCPSTRAT-1454 is resolved, this condition has limited value because OVN control‑plane component versions are still dictated by data‑plane rollout ability.control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
3495-3561: Fix “” message when all components are up to date; minor cleanups
- Message becomes the literal string “” when there are no errors. Emit AllIsWell instead.
- Precompute expectedVersion to avoid repeated calls.
- Nit: use lowerCamelCase for rolloutCompleteCondition.
Apply:
func (r *HostedControlPlaneReconciler) reconcileControlPlaneUpToDateCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, releaseImage *releaseinfo.ReleaseImage) error { var componentErrors []error @@ // Check if all components version matches HCP spec release version. // Capture any errors for RolloutCompleteCondition and AvailableCondition whether version matches or not. reason := hyperv1.AsExpectedReason controlPlaneUpToDateStatus := metav1.ConditionTrue + expectedVersion := releaseImage.Version() for _, component := range componentList.Items { - if component.Status.Version != releaseImage.Version() { - componentErrors = append(componentErrors, fmt.Errorf("component %q version %q does not match expected version %s", component.Name, component.Status.Version, releaseImage.Version())) + if component.Status.Version != expectedVersion { + componentErrors = append(componentErrors, fmt.Errorf("component %q version %q does not match expected version %s", component.Name, component.Status.Version, expectedVersion)) controlPlaneUpToDateStatus = metav1.ConditionFalse reason = "VersionMismatch" } availableCondition := meta.FindStatusCondition(component.Status.Conditions, string(hyperv1.ControlPlaneComponentAvailable)) @@ - RolloutCompleteCondition := meta.FindStatusCondition(component.Status.Conditions, string(hyperv1.ControlPlaneComponentRolloutComplete)) - if RolloutCompleteCondition == nil { + rolloutCompleteCondition := meta.FindStatusCondition(component.Status.Conditions, string(hyperv1.ControlPlaneComponentRolloutComplete)) + if rolloutCompleteCondition == nil { componentErrors = append(componentErrors, fmt.Errorf("component %s rollout is not complete. Reason: Unknown", component.Name)) } - if RolloutCompleteCondition != nil && RolloutCompleteCondition.Status != metav1.ConditionTrue { - componentErrors = append(componentErrors, fmt.Errorf("component %s rollout is not complete. Reason: %s, Message: %s", component.Name, RolloutCompleteCondition.Reason, RolloutCompleteCondition.Message)) + if rolloutCompleteCondition != nil && rolloutCompleteCondition.Status != metav1.ConditionTrue { + componentErrors = append(componentErrors, fmt.Errorf("component %s rollout is not complete. Reason: %s, Message: %s", component.Name, rolloutCompleteCondition.Reason, rolloutCompleteCondition.Message)) } } + agg := utilerrors.NewAggregate(componentErrors) + msg := hyperv1.AllIsWellMessage + if agg != nil { + msg = agg.Error() + } + upToDateCondition := metav1.Condition{ Type: string(hyperv1.ControlPlaneUpToDate), Status: controlPlaneUpToDateStatus, Reason: reason, - Message: fmt.Sprint(utilerrors.NewAggregate(componentErrors)), + Message: msg, ObservedGeneration: hcp.Generation, } meta.SetStatusCondition(&hcp.Status.Conditions, upToDateCondition) return nil }docs/content/reference/api.md (1)
4700-4706: Tighten wording, fix style, add behavior note; update Go doc comment and re-run make api-docs
- Use “control plane”/“data plane” consistently in lower case.
- Prefer “running on the management side.”
- Use the verb form “roll out.”
- Replace “none of the components” with “no components.”
- Add an explicit note that the condition reflects version alignment only; availability/rollout do not affect True/False (only the message), per design.
Apply in source (not this generated file), then regenerate:
-<td><p>ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version. -This is a useful semantic to make decisions decoupled from the ability of components running on the data plane to roll out during an upgrade to a specific version. -For example, acknowledge that none of the components running management side are vulnerable to a specific CVE. -For example, don’t block HostedCluster upgrades when there’s no data plane compute. -Until <a href="https://issues.redhat.com/browse/OCPSTRAT-1454">https://issues.redhat.com/browse/OCPSTRAT-1454</a> is fixed, this condition has limited value, as the versions of OVN Control plane components are still dictated by the ability of the data plane to rollout.</p> +<td><p>ControlPlaneUpToDate indicates whether the control plane components running on the management side are up to date with the desired release version. +This condition enables decisions decoupled from the data plane’s ability to roll out during an upgrade to a specific version. +For example, acknowledge that no components running on the management side are vulnerable to a specific CVE. +For example, don’t block HostedCluster upgrades when there is no data plane compute. +Until <a href="https://issues.redhat.com/browse/OCPSTRAT-1454">https://issues.redhat.com/browse/OCPSTRAT-1454</a> is fixed, this condition has limited value because the versions of OVN control plane components are still dictated by the data plane’s ability to roll out. +Note: this condition reflects only version alignment; availability/rollout issues may appear in the message but do not affect the condition status.</p>Note: Update the Go doc comment that generates this block and run
make api-docs; don’t hand-edit this file. Also runmake verify-codespellfor markdown.
🧹 Nitpick comments (3)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go (1)
23-29: Keep vendor docs in sync with API and clarify semantics.Mirror the same wording/semantics note here to avoid drift between api/ and vendor/.
Apply this diff:
- // ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version. - // This is a useful semantic to make decisions decoupled from the ability of components running on the data plane to roll out during an upgrade to a specific version. - // For example, acknowledge that none of the components running management side are vulnerable to a specific CVE. - // For example, don't block HostedCluster upgrades when there's no data plane compute. - // Until https://issues.redhat.com/browse/OCPSTRAT-1454 is fixed, this condition has limited value, as the versions of OVN Control plane components are still dictated by the ability of the data plane to rollout. + // ControlPlaneUpToDate indicates whether the control-plane components running on the management side are up to date with the desired release version. + // This is useful for decisions decoupled from the data-plane’s ability to roll out during an upgrade to a specific version. + // For example, acknowledge that management-side components are not vulnerable to a specific CVE. + // For example, do not block HostedCluster upgrades when there is no data-plane compute. + // Note: the condition Status reflects version alignment only; component availability and rollout details are reported in the Message for context. + // Until https://issues.redhat.com/browse/OCPSTRAT-1454 is resolved, this condition has limited value because OVN control‑plane component versions are still dictated by data‑plane rollout ability.hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
803-823: Avoid mutating HCP conditions by pointer aliasing.You assign the pointer returned by meta.FindStatusCondition to condition, then mutate ObservedGeneration. That mutates the HCP object in-memory (even if not persisted) and risks subtle side effects during this reconcile. Copy before modifying.
Apply this diff:
- if hcp != nil { - hcpCondition := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ControlPlaneUpToDate)) - if hcpCondition != nil { - condition = hcpCondition - } else { - condition.Message = "Condition not found in the HCP" - } - } + if hcp != nil { + if src := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ControlPlaneUpToDate)); src != nil { + // Copy to avoid mutating the HCP condition instance. + copied := *src + condition = &copied + } else { + condition.Message = "Condition not found in the HCP" + } + }control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
331-332: Watch on ControlPlaneComponent added — consider filtering to reduce reconcile noiseOptional: add a predicate to only enqueue when Status.Version or relevant conditions change to avoid excessive reconciles on metadata-only updates.
Apply via builder predicate on this watch (e.g., custom predicate.Funcs on Update/Delete).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
api/hypershift/v1beta1/hostedcluster_conditions.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go(1 hunks)docs/content/reference/api.md(1 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)support/conditions/conditions.go(1 hunks)test/e2e/util/util.go(1 hunks)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- support/conditions/conditions.go
- test/e2e/util/util.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
🧰 Additional context used
📓 Path-based instructions (9)
**/!(*.pb).go
📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)
**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.govendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
Use
make lint-fixafter writing Go code to automatically fix most linting issuesFollow the rules defined in @100-go-mistakes.mdc for Go code
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.govendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go
hypershift-operator/controllers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Place operator controller implementations under hypershift-operator/controllers/
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
{hypershift-operator,control-plane-operator}/controllers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
{hypershift-operator,control-plane-operator}/controllers/**/*.go: Controller code should follow controller-runtime patterns with proper error handling and requeuing
Use controller-runtime structured logging in controllers
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
{hypershift-operator,control-plane-operator}/controllers/**
📄 CodeRabbit inference engine (AGENTS.md)
Place platform-specific implementations within their respective controller subdirectories to keep platform logic isolated
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
After modifying API types in the api/ package, run
make apito regenerate APIs and CRDs
Files:
api/hypershift/v1beta1/hostedcluster_conditions.go
api/**
📄 CodeRabbit inference engine (AGENTS.md)
api/**: API definitions and CRDs must reside under the api/ directory
Prefer API version v1beta1; use feature gates for experimental functionality
Generate CRDs via controller-gen using the OpenShift-specific tooling for this repository
Files:
api/hypershift/v1beta1/hostedcluster_conditions.go
control-plane-operator/controllers/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Place control-plane controller implementations under control-plane-operator/controllers/
Files:
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
For markdown files, use
make verify-codespellto catch spelling errors
Files:
docs/content/reference/api.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: enxebre
PR: openshift/hypershift#6300
File: control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go:3494-3550
Timestamp: 2025-09-04T09:50:08.095Z
Learning: The ControlPlaneUpToDate condition in Hypershift is specifically designed to track version alignment between control plane components and the desired release version. Availability and rollout status checks are captured for informational purposes in the message but do not affect the True/False status of the condition itself. This separation of concerns allows tracking version correctness independently from operational readiness.
📚 Learning: 2025-09-04T09:50:08.095Z
Learnt from: enxebre
PR: openshift/hypershift#6300
File: control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go:3494-3550
Timestamp: 2025-09-04T09:50:08.095Z
Learning: The ControlPlaneUpToDate condition in Hypershift is specifically designed to track version alignment between control plane components and the desired release version. Availability and rollout status checks are captured for informational purposes in the message but do not affect the True/False status of the condition itself. This separation of concerns allows tracking version correctness independently from operational readiness.
Applied to files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.goapi/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.govendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.godocs/content/reference/api.md
📚 Learning: 2025-08-29T18:49:17.570Z
Learnt from: CR
PR: openshift/hypershift#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T18:49:17.570Z
Learning: Applies to {hypershift-operator,control-plane-operator}/controllers/**/*.go : Controller code should follow controller-runtime patterns with proper error handling and requeuing
Applied to files:
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
🧬 Code graph analysis (2)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (2)
api/hypershift/v1beta1/hostedcluster_conditions.go (2)
ControlPlaneUpToDate(28-28)StatusUnknownReason(209-209)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go (2)
ControlPlaneUpToDate(28-28)StatusUnknownReason(209-209)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (3)
api/hypershift/v1beta1/controlplanecomponent_types.go (4)
ControlPlaneComponent(87-98)ControlPlaneComponentList(102-111)ControlPlaneComponentAvailable(20-20)ControlPlaneComponentRolloutComplete(22-22)support/releaseinfo/releaseinfo.go (1)
ReleaseImage(39-42)api/hypershift/v1beta1/hostedcluster_conditions.go (2)
ControlPlaneUpToDate(28-28)AsExpectedReason(210-210)
⏰ 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). (2)
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
🔇 Additional comments (3)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go (1)
23-29: Vendor and API definitions in sync. VerifiedControlPlaneUpToDateappears exactly once in bothapi/…/hostedcluster_conditions.goandvendor/…/hostedcluster_conditions.gowith no duplicates.control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (2)
781-783: LGTM: UpToDate condition reconciliation call placed correctlyInvocation after release image lookup and before the status Patch is appropriate.
3523-3550: Semantics match intent: version alignment drives Status; availability/rollout are informationalThis aligns with the documented design for ControlPlaneUpToDate. No change needed.
enxebre
changed the title
|
/test verify |
enxebre
changed the title
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
from
eb81f58 to
cd86662
Compare
enxebre
changed the title
|
/uncc rtheis |
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/content/reference/api.md (1)
4700-4706: Fix wording, hyphenation, and document Status semantics for ControlPlaneUpToDate (update Go doc comments and re-run make api-docs)Generated text has style/grammar issues (“management side”, “rollout”), and it’s missing the key contract: Status reflects version alignment only. Please update the source Go doc comments for the condition (not this generated file), then run make api-docs.
-</tr><tr><td><p>"ControlPlaneUpToDate"</p></td> -<td><p>ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version. -This is a useful semantic to make decisions decoupled from the ability of components running on the data plane to rollout during an upgrade to a specific version. -E.g. Acknowledge that none of the components running management side are vulnerable to a specific CVE. -E.g. Don’t block HC upgrades when there’s no data plane compute. -Until <a href="https://issues.redhat.com/browse/OCPSTRAT-1454">https://issues.redhat.com/browse/OCPSTRAT-1454</a> is fixed, this condition has limited value, as the versions of OVN Control plane components are still dictated by the ability of the data plane to rollout.</p> +</tr><tr><td><p>"ControlPlaneUpToDate"</p></td> +<td><p>ControlPlaneUpToDate indicates whether the control plane components running on the management side are up to date with the desired release version. +This condition enables decisions that are decoupled from the data plane’s ability to roll out during an upgrade to a specific version. +For example, acknowledge that no components running on the management side are vulnerable to a specific CVE. +For example, don’t block HostedCluster upgrades when there is no data-plane compute. +Note: the Status reflects only version alignment; availability and rollout details may be included in the message for context but do not affect the True/False value. +Until <a href="https://issues.redhat.com/browse/OCPSTRAT-1454">https://issues.redhat.com/browse/OCPSTRAT-1454</a> is fixed, this condition has limited value because the versions of OVN control plane components are still dictated by the data plane’s ability to roll out.</p>Also run: make verify-codespell for the regenerated docs.
🧹 Nitpick comments (2)
support/conditions/conditions.go (2)
33-35: Add a brief clarifying comment to avoid conflating “up to date” with availability.Apply:
- hyperv1.ClusterVersionProgressing: metav1.ConditionFalse, - hyperv1.ControlPlaneUpToDate: metav1.ConditionTrue, + hyperv1.ClusterVersionProgressing: metav1.ConditionFalse, + // ControlPlaneUpToDate reflects version alignment of management-side control plane components only; + // availability/rollout are orthogonal and reflected elsewhere. + hyperv1.ControlPlaneUpToDate: metav1.ConditionTrue,
39-41: Typo in comment (“tooday”).Apply:
- // TODO: hyperv1.ValidOIDCConfiguration should really be platform-agnostic, but tooday it's not + // TODO: hyperv1.ValidOIDCConfiguration should really be platform-agnostic, but today it's not
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
api/hypershift/v1beta1/hostedcluster_conditions.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go(1 hunks)docs/content/reference/api.md(1 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(1 hunks)support/conditions/conditions.go(1 hunks)test/e2e/util/util.go(1 hunks)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- api/hypershift/v1beta1/hostedcluster_conditions.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- test/e2e/util/util.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.pb).go
📄 CodeRabbit inference engine (.cursor/rules/100-go-mistakes.mdc)
**/!(*.pb).go: Avoid variable shadowing
Do not over-nest control flow (e.g., nested if or for blocks)
Avoid init() functions unless absolutely necessary
Keep functions small and focused
Prefer composition over inheritance (via embedding)
Use the functional options pattern for constructors where flexibility is needed
Avoid defining interfaces until you need them
Do not return interfaces from constructors or public APIs
Define interfaces on the consumer side, not the producer side
Keep interfaces small and focused (generally 1–2 methods)
Avoid embedding pointer types unless necessary
Don’t overuse getters/setters — prefer public fields when it makes sense
Use value receivers when the method doesn't mutate state or require pointer semantics
Do not use util, common, or similarly vague package names
Avoid package name collisions by using clear, unique names
Do not expose unnecessary symbols (keep exported API minimal)
Distinguish between nil and empty slices
Avoid memory leaks from slicing large arrays
Always check the capacity when copying or appending slices
Preallocate slice capacity when size is known ahead of time
Always initialize maps before use
Check existence with the two-value assignment (val, ok := m[key])
Be aware that ranging over a map is in random order
Always check errors — don’t ignore them
Wrap errors with context when rethrowing
Avoid panics except in truly unrecoverable cases
Use errors.Is and errors.As for error comparison in Go 1.20+
Always defer cancel() when using context.WithCancel
Do not leak goroutines — ensure they exit cleanly
Avoid data races — use mutexes or channels appropriately
Never close a channel from the receiving side
Keep imports grouped and ordered: stdlib, external, internal
Avoid magic numbers — use named constants
Prefer explicit over implicit — especially in exported APIs
Only use generics when they simplify code or add real flexibility
Avoid over-engineering with type parameters
Be cautious with constraint complexity — keep...
Files:
support/conditions/conditions.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
Use
make lint-fixafter writing Go code to automatically fix most linting issuesFollow the rules defined in @100-go-mistakes.mdc for Go code
Files:
support/conditions/conditions.go
support/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Define common, shared interfaces and utilities under support/ packages
Files:
support/conditions/conditions.go
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/code-formatting.mdc)
For markdown files, use
make verify-codespellto catch spelling errors
Files:
docs/content/reference/api.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: enxebre
PR: openshift/hypershift#6300
File: control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go:3494-3550
Timestamp: 2025-09-04T09:50:08.095Z
Learning: The ControlPlaneUpToDate condition in Hypershift is specifically designed to track version alignment between control plane components and the desired release version. Availability and rollout status checks are captured for informational purposes in the message but do not affect the True/False status of the condition itself. This separation of concerns allows tracking version correctness independently from operational readiness.
📚 Learning: 2025-09-04T09:50:08.095Z
Learnt from: enxebre
PR: openshift/hypershift#6300
File: control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go:3494-3550
Timestamp: 2025-09-04T09:50:08.095Z
Learning: The ControlPlaneUpToDate condition in Hypershift is specifically designed to track version alignment between control plane components and the desired release version. Availability and rollout status checks are captured for informational purposes in the message but do not affect the True/False status of the condition itself. This separation of concerns allows tracking version correctness independently from operational readiness.
Applied to files:
support/conditions/conditions.godocs/content/reference/api.md
🧬 Code graph analysis (1)
support/conditions/conditions.go (2)
api/hypershift/v1beta1/hostedcluster_conditions.go (1)
ControlPlaneUpToDate(28-28)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.go (1)
ControlPlaneUpToDate(28-28)
⏰ 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). (2)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
🔇 Additional comments (1)
support/conditions/conditions.go (1)
34-35: Cross-version tolerance confirmedTests in
test/e2e/util/util.goexplicitly deleteControlPlaneUpToDatefor older releases (<4.20), and the condition is defined in both the in-repo API and vendor.
ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version. This is a useful semantic to make decissions decoupled from the ability of components running on the data plane to rollout during an upgrade to a specific version. E.g. Acknowledge that none of the components running management side is vunerable to a specific CVE. E.g. Don't block subsequent HC upgrades when there's no data plane compute. Until https://issues.redhat.com/browse/OCPSTRAT-1454 is fixed, this condition has limited value, since OVN Control plane components will be still dictated by the ability of the data plane to rollout.
enxebre
force-pushed
the
controlplaneuptodate-condition
branch
from
cd86662 to
f2fd2c8
Compare
|
@enxebre: This pull request references CNTRLPLANE-993 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aks |
|
/lgtm |
|
/uncc @jparrill |
|
/verify help |
|
/verified help |
|
@enxebre: The 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@enxebre: The following tests failed, say
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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
closing in favour of openshift/enhancements#1950 |
9 participants
What this PR does / why we need it:
ControlPlaneUpToDate indicates whether the control plane components running management side are up to date with the desired release version.
This is a useful semantic to make decissions decoupled from the ability of components running on the data plane to rollout during an upgrade to a specific version.
E.g. Acknowledge that none of the components running management side is vunerable to a specific CVE.
E.g. Don't block subsequent HC upgrades when there's no data plane compute.
Until https://issues.redhat.com/browse/OCPSTRAT-1454 is fixed, this condition has limited value, since OVN Control plane components will be still dictated by the ability of the data plane to rollout.
Will add unit/e2e coverage once agreed upon the approach.
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores