OCPBUGS-88531: Remove CPO-side restart logic for CNO operands - #8751
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-88531, which is valid. 3 validation(s) were run on this bug
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. |
📝 WalkthroughWalkthroughIn 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8751 +/- ##
==========================================
+ Coverage 41.75% 41.86% +0.11%
==========================================
Files 758 759 +1
Lines 93981 94019 +38
==========================================
+ Hits 39240 39362 +122
+ Misses 51988 51902 -86
- Partials 2753 2755 +2
... and 13 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
923dc8e to
5682e1f
Compare
|
/hold Dropped some comments. Thanks! Holding until companion PR from cluster-network-operator#3030 got merged. Feel free to remove it when that happens. |
| } | ||
|
|
||
| func (r *HostedControlPlaneReconciler) cleanupClusterNetworkOperatorResources(ctx context.Context, hcp *hyperv1.HostedControlPlane, hasRouteCap bool) error { | ||
| if restartAnnotation, ok := hcp.Annotations[hyperv1.RestartDateAnnotation]; ok { |
There was a problem hiding this comment.
This function has zero callers after your PR — the three call sites you removed were its only consumers. Worth deleting it here to avoid leaving dead exported code in the package. Same for MultusAdmissionControllerDeployment, NetworkNodeIdentityDeployment, OVNKubeControlPlaneDeployment and their constants in manifests/cno.go — all orphaned now.
Happy to help with a follow-up if you prefer to keep this PR minimal, but since it's all in the same ownership boundary it fits naturally here.
There was a problem hiding this comment.
Isn't it still called in
?There was a problem hiding this comment.
I was going to comment the same - let's remove the function now because it's unused.
Isn't it still called in
The function SetRestartAnnotationAndPatch was only called in this cleanupClusterNetworkOperatorResources.
See:
ᐅ grep -Ri SetRestartAnnotationAndPatch
control-plane-operator/controllers/hostedcontrolplane/v2/cno/component.go:func SetRestartAnnotationAndPatch(ctx context.Context, crclient client.Client, dep *appsv1.Deployment, restartAnnotation string) error {
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go: if err := cnov2.SetRestartAnnotationAndPatch(ctx, r.Client, multusDeployment, restartAnnotation); err != nil {
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go: if err := cnov2.SetRestartAnnotationAndPatch(ctx, r.Client, networkNodeIdentityDeployment, restartAnnotation); err != nil {
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go: // CNO manages overall ovnkube-control-plane deployment. CPO manages restarts. Note that cnov2.SetRestartAnnotationAndPatch just returns err == nil if the deployment isn't found (so if OVN isn't being used)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go: if err := cnov2.SetRestartAnnotationAndPatch(ctx, r.Client, ovnKubeControlPlaneDeployment, restartAnnotation); err != nil {
There was a problem hiding this comment.
Done — removed SetRestartAnnotationAndPatch and the orphaned manifest helpers (MultusAdmissionControllerDeployment, NetworkNodeIdentityDeployment, OVNKubeControlPlaneDeployment) along with their unused constants and imports.
CNO now handles restart-date annotation propagation to all its operands (multus-admission-controller, network-node-identity, ovnkube-control-plane, cloud-network-config-controller) directly via the fix in CNO PR openshift#3030. The CPO-side restart logic for these deployments is now redundant and can be removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5682e1f to
407530a
Compare
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 3
|
|
/hold cancel CNO PR merged |
|
/retest |
|
Now I have a clear picture. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 10 test failures are pre-existing flaky infrastructure issues unrelated to the PR changes. PR #8751 only removes CPO-side restart logic for CNO operands (multus-admission-controller, network-node-identity, ovnkube-control-plane) from three files in the control-plane-operator. None of the failures involve these components. The 4 root leaf failures are: (1) an Root CauseThe 10 reported failures decompose into 4 independent, unrelated leaf failures plus 6 cascaded parent test failures: Failure 1: TestKarpenterUpgradeControlPlane/EnsureNoCrashingPods
Failure 2: TestCreateClusterRequestServingIsolation/ValidateHostedCluster
Failure 3: TestNodePool/HostedCluster0/Main/TestKMSRootVolumeEncryption
Failure 4: TestNodePool/HostedCluster0/Main/TestNodePoolDay2Tags
Cascaded failures (6): TestKarpenterUpgradeControlPlane, TestKarpenterUpgradeControlPlane/ValidateHostedCluster, TestCreateClusterRequestServingIsolation, TestNodePool/HostedCluster0/Main, TestNodePool/HostedCluster0, and TestNodePool all fail because their children failed. The HostedCluster0 teardown condition check ( None of these failures are related to PR #8751, which only removes the Recommendations
Evidence
|
|
/verified by e2e |
|
/test e2e-aws |
|
@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. |
|
/label acknowledge-critical-fixes-only |
|
@bryan-cox: 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. |
|
@bryan-cox: Jira Issue OCPBUGS-88531 is in an unrecognized state (Verified) and will not be moved to the MODIFIED state. 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. |
Summary
The
cleanupClusterNetworkOperatorResourcesfunction previously contained logic to readhypershift.openshift.io/restart-datefrom the HCP and patch it onto CNO-managed deployments. CNO reads the annotation from HCP itself and sets it as a pod template annotation on all rendered workloads, so the CPO-side logic is unnecessary duplication.Test plan
Tested and verified. Test verification report here - https://bryan-cox.github.io/architectural-artifact-sharing/test-verification-report-ocpbugs-84239/index.html.
Summary by CodeRabbit
ovnkube-sbdbRoute when applicable, and deleting theovnkube-master-externalandovnkube-master-internalServices.