OCPBUGS-88531: Propagate restart-date annotation to CNO operand pod templates - #3030
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-88531, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughAdds HostedControlPlane restart-date parsing and render-time propagation so ChangesHyperShift restart-date propagation
Sequence Diagram(s)sequenceDiagram
participant OperconfigController
participant ParseHostedControlPlane
participant SetRestartDateAnnotation
participant RenderedObjects
OperconfigController->>ParseHostedControlPlane: read HostedControlPlane restart date
OperconfigController->>SetRestartDateAnnotation: annotate rendered objects when restart date is set
SetRestartDateAnnotation->>RenderedObjects: update pod-template annotations
OperconfigController->>OperconfigController: mark RenderError degraded on annotation failure
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
![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
🤖 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 `@pkg/hypershift/hypershift_test.go`:
- Around line 154-155: The test code is discarding error returns by using blank
identifiers (_) instead of capturing them, which can mask test failures and
produce misleading passes. Identify all instances in the test file where
function calls return errors that are being ignored (such as calls to
unstructured.NestedStringMap and similar utility functions), capture the
returned error values instead of discarding them with _, and add appropriate
assertions or error handling using the test assertion framework (e.g.,
g.Expect(err).NotTo(HaveOccurred())) to verify these operations succeed as
expected during test setup and assertions.
In `@pkg/hypershift/hypershift.go`:
- Line 291: The call to unstructured.NestedStringMap is discarding the error
return value (the third return value) by using a blank identifier, which means
any errors from malformed annotations are silently ignored. Instead of ignoring
the error, capture it as a named variable in the assignment to
unstructured.NestedStringMap, check if the error is non-nil, and handle it
appropriately before proceeding to mutate the pod-template annotations. This
ensures that rendering defects in the annotations are surfaced rather than
silently hidden.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37272bb8-3d49-4c7b-9530-3c9966bb18b4
📒 Files selected for processing (3)
pkg/controller/operconfig/operconfig_controller.gopkg/hypershift/hypershift.gopkg/hypershift/hypershift_test.go
|
/jira refresh |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-bug
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-88531, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-88531, which is valid. 3 validation(s) were run on this bug
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. |
|
/retest-required |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
/retest |
|
|
||
| restartDate, _, err := unstructured.NestedString(hcp.UnstructuredContent(), "metadata", "annotations", RestartDateAnnotation) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to extract restart date annotation: %v", err) |
There was a problem hiding this comment.
Nit: It would be better to use %w to preserve the error chain but since all other calls in this file use %v let's keep it.
|
/lgtm |
kyrtapz
left a comment
kyrtapz
left a comment
There was a problem hiding this comment.
When the hypershift.openshift.io/restart-date annotation is set on a HostedControlPlane CR, CNO-managed operands like cloud-network-config-controller were not being restarted. This happened because CNO uses server-side apply with deterministic manifests — applying the same manifest twice is a no-op, so operands don't roll out unless something in the pod template changes.
While the change is mostly I don't get the reasoning from the PR description.
CNO never propagated this annotation to its operand pod templates, so there was nothing to trigger a rollout. This has nothing to do with SSA.
|
|
||
| // SetRestartDateAnnotation sets the restart-date annotation on pod templates of all | ||
| // Deployment, DaemonSet, and StatefulSet objects to trigger a rolling restart. | ||
| func SetRestartDateAnnotation(objs []*unstructured.Unstructured, restartDate string) error { |
There was a problem hiding this comment.
This will cause a rollout of ALL CNO managed Deployments, DaemonSets, and StatefulSets, even if they run only in the hosted cluster.
I was sure that this annotation only affects the control-plane components in hypershift, is that not the case?
There was a problem hiding this comment.
Good catch — SetRestartDateAnnotation currently operates on all rendered objects from Render(), which includes guest-cluster DaemonSets (ovnkube-node, multus, etc.), not just the HCP-namespace control-plane Deployments. The original CPO restart logic only targeted multus-admission-controller, network-node-identity, and ovnkube-control-plane in the HCP namespace.
I'll scope this by filtering on the HCP namespace so only control-plane objects are annotated.
| r.status.UnsetProgressing(statusmanager.OperatorRender) | ||
| } | ||
|
|
||
| if hcp := bootstrapResult.Infra.HostedControlPlane; hcp != nil && hcp.RestartDate != "" { |
There was a problem hiding this comment.
When the annotation is removed there will be another rollout, is this the expected behavior?
There was a problem hiding this comment.
Yes, removing the annotation would cause another rollout since the pod template spec changes (annotation removed). This is the expected behavior — removing the annotation is an explicit operator action, and the resulting rollout restores the clean state. I'll document this in the PR description.
|
/retest-required |
|
/retest-required |
|
/hold Revision ab7d755 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
openshift-ci
Bot
added
the
needs-rebase
|
/hold cancel |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-upgrade-ipsec, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw, ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec, ci/prow/e2e-ovn-ipsec-step-registry 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. |
openshift-ci
Bot
removed
the
do-not-merge/hold
…emplates When the hypershift.openshift.io/restart-date annotation is set on a HostedControlPlane, CNO now reads it and injects it into the pod template annotations of all rendered Deployments, DaemonSets, and StatefulSets. This triggers Kubernetes rolling restarts for all CNO operands, including cloud-network-config-controller which was previously missed. The fix follows the existing pattern of reading HCP annotations in ParseHostedControlPlane (like priority-class) and modifying rendered objects post-Render (like setOVNObjectAnnotation). This is the correct architectural location for this logic since CNO owns these operands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
lgtm
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
@kyrtapz could I get lgtm again please when you have a moment? I rebased. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, kyrtapz, mgencur 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 |
|
/override ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-upgrade-ipsec, ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec, ci/prow/e2e-ovn-ipsec-step-registry 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. |
|
@bryan-cox: 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. |
fc89599
into
openshift:master
|
@bryan-cox: Jira Issue OCPBUGS-88531: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-88531 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
4 participants
Summary
When the
hypershift.openshift.io/restart-dateannotation is set on a HostedControlPlane CR, CNO-managed operands likecloud-network-config-controllerwere not being restarted. CNO never propagated this annotation to its operand pod templates, so there was nothing to trigger a rollout.Previously, the CPO (control-plane-operator) handled restart-date propagation by directly patching individual deployments (multus-admission-controller, network-node-identity, ovnkube-control-plane), but this approach missed cloud-network-config-controller entirely — which is the bug reported in OCPBUGS-88531.
This PR fixes the issue by having CNO propagate the annotation itself:
hypershift.openshift.io/restart-dateannotation from the HostedControlPlane CR inParseHostedControlPlane()SetRestartDateAnnotation()helper that injects the restart-date into pod template annotations of rendered Deployments, DaemonSets, and StatefulSets within the HCP namespace only (guest-cluster workloads are not affected)Render()returns, gated on HyperShift mode and a non-empty restart-date valueThe companion HyperShift PR (#8751) removes the redundant CPO-side restart logic, since CNO now handles it directly.
Annotation removal behavior
If the
restart-dateannotation is removed from the HostedControlPlane, CNO will render manifests without the annotation on pod templates. This changes the pod template spec, triggering another rollout that restores the pods to a clean state (no restart-date annotation). This is expected behavior — removing the annotation is an explicit operator action.Test plan
ParseHostedControlPlanewith restart-date annotationSetRestartDateAnnotationcovering Deployments, DaemonSets, StatefulSets, non-apps objects, namespace filtering, annotation preservation, and multi-object handlinggo build ./...passesgo test ./pkg/... ./cmd/...passesgo vetpasseshypershift.openshift.io/restart-dateannotation on a HostedCluster and verified all 4 CNO operands restart (cloud-network-config-controller, multus-admission-controller, network-node-identity, ovnkube-control-plane). Full test verification report.Fixes: https://issues.redhat.com/browse/OCPBUGS-88531