OCPBUGS-84368: add safe-to-evict annotation and remove tolerations to fix autoscaler scale-down and drain loop#8338
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds PodDisruptionBudget (PDB) support to the KAS connection checker component. A new helper function constructs PDB manifests with appropriate metadata. The reconciliation flow for the KAS connection checker is updated to annotate the Deployment pod template as safe to evict and then create or update a PodDisruptionBudget with MinAvailable=1, a selector matching KAS connection checker pods, and unhealthy pod eviction enabled. A new constant defines the cluster autoscaler safe-to-evict annotation key. Test coverage is expanded to validate PDB reconciliation behavior, including edge cases and error scenarios. Sequence DiagramsequenceDiagram
participant Reconciler as Reconciler<br/>(KAS Connection Checker)
participant K8sAPI as Kubernetes API
participant Deployment as Deployment<br/>Resource
participant PDB as PodDisruptionBudget<br/>Resource
Reconciler->>K8sAPI: Fetch existing Deployment
K8sAPI-->>Reconciler: Return Deployment
Reconciler->>Deployment: Annotate pod template<br/>(safe-to-evict)
Reconciler->>K8sAPI: Create/Update Deployment
K8sAPI-->>Reconciler: Deployment reconciled
Reconciler->>K8sAPI: Fetch existing PDB
K8sAPI-->>Reconciler: Return PDB (or none)
Reconciler->>PDB: Set minAvailable=1<br/>Set selector (app=KASConnectionChecker)<br/>Set unhealthyPodEvictionPolicy=AlwaysAllow
Reconciler->>K8sAPI: Create/Update PDB
K8sAPI-->>Reconciler: PDB reconciled
Reconciler-->>Reconciler: Return reconciliation result
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
67a200b to
238cc6a
Compare
openshift-ci
Bot
added
the
area/testing
![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 (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
2841-2855: Add an existing-PDB reconciliation case.These assertions only cover the create path. Since the controller now reconciles the PDB on every run, please add a case that seeds a
kas-connection-checkerPDB withmaxUnavailableand verifies reconcile rewrites it tominAvailable=1/AlwaysAllow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` around lines 2841 - 2855, Add a new test case that seeds an existing PodDisruptionBudget named by manifests.KASConnectionCheckerName in namespace manifests.KASConnectionCheckerNamespace with Spec.MaxUnavailable set (e.g., 1) and UnhealthyPodEvictionPolicy set to policyv1.DoNotEvict, then invoke the controller reconcile path used by other tests (the same reconcile helper used in resources_test.go) and assert the PDB is mutated: fetch the PDB via c.Get and verify pdb.Spec.MinAvailable equals intstr.FromInt32(1), pdb.Spec.UnhealthyPodEvictionPolicy equals policyv1.AlwaysAllow, and pdb.Spec.Selector still matches app=manifests.KASConnectionCheckerName to confirm reconciliation rewrote maxUnavailable to minAvailable and enforced AlwaysAllow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 1756-1764: The update closure passed to r.CreateOrUpdate for the
PodDisruptionBudget (pdb) should clear any existing pdb.Spec.MaxUnavailable
before assigning MinAvailable, because PodDisruptionBudgetSpec rejects objects
with both fields set; inside the anonymous func (the CreateOrUpdate callback
that mutates pdb), explicitly set pdb.Spec.MaxUnavailable = nil (or reset it)
prior to setting pdb.Spec.MinAvailable = ptr.To(intstr.FromInt32(1)) and
pdb.Spec.UnhealthyPodEvictionPolicy, referencing the pdb variable and the
CreateOrUpdate call that manages the kas-connection-checker PDB
(manifests.KASConnectionCheckerName).
---
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go`:
- Around line 2841-2855: Add a new test case that seeds an existing
PodDisruptionBudget named by manifests.KASConnectionCheckerName in namespace
manifests.KASConnectionCheckerNamespace with Spec.MaxUnavailable set (e.g., 1)
and UnhealthyPodEvictionPolicy set to policyv1.DoNotEvict, then invoke the
controller reconcile path used by other tests (the same reconcile helper used in
resources_test.go) and assert the PDB is mutated: fetch the PDB via c.Get and
verify pdb.Spec.MinAvailable equals intstr.FromInt32(1),
pdb.Spec.UnhealthyPodEvictionPolicy equals policyv1.AlwaysAllow, and
pdb.Spec.Selector still matches app=manifests.KASConnectionCheckerName to
confirm reconciliation rewrote maxUnavailable to minAvailable and enforced
AlwaysAllow.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dff10846-33f5-40b0-99c8-a423c96284f5
📒 Files selected for processing (4)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/kasconnectionchecker.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gotest/e2e/nodepool_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/nodepool_test.go
openshift-ci
Bot
added
the
area/hypershift-operator
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
784e934 to
cbad894
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8338 +/- ##
==========================================
+ Coverage 41.59% 41.69% +0.10%
==========================================
Files 758 758
Lines 93925 93943 +18
==========================================
+ Hits 39066 39173 +107
+ Misses 52113 52025 -88
+ Partials 2746 2745 -1
... and 4 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:
|
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
cbad894 to
8979ea8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
580-582: Clarify the top-level wrapped error message.This path now reconciles both Deployment and PDB, but the wrapper still says “deployment” only. A broader message will make failure triage clearer.
Proposed tweak
- errs = append(errs, fmt.Errorf("failed to reconcile KAS connection checker deployment: %w", err)) + errs = append(errs, fmt.Errorf("failed to reconcile KAS connection checker resources: %w", err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go` around lines 580 - 582, The current error wrapper in the call that invokes reconcileKASConnectionChecker uses "failed to reconcile KAS connection checker deployment" but that function reconciles both the Deployment and the PodDisruptionBudget; update the wrapped message to reflect both resources (e.g., "failed to reconcile KAS connection checker resources (deployment and PDB)" or similar) so failures from reconcileKASConnectionChecker are not misleading—change the fmt.Errorf wrapper string in the block that calls reconcileKASConnectionChecker(ctx, hcp, cliImage) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 580-582: The current error wrapper in the call that invokes
reconcileKASConnectionChecker uses "failed to reconcile KAS connection checker
deployment" but that function reconciles both the Deployment and the
PodDisruptionBudget; update the wrapped message to reflect both resources (e.g.,
"failed to reconcile KAS connection checker resources (deployment and PDB)" or
similar) so failures from reconcileKASConnectionChecker are not
misleading—change the fmt.Errorf wrapper string in the block that calls
reconcileKASConnectionChecker(ctx, hcp, cliImage) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4e5098fd-38b6-4599-929b-7eba3028cb17
📒 Files selected for processing (5)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/kasconnectionchecker.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gosupport/config/types.gotest/e2e/nodepool_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/nodepool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/kasconnectionchecker.go
|
/test e2e-conformance |
sdminonne
changed the title
openshift-ci-robot
added
jira/severity-important
|
@sdminonne: This pull request references Jira Issue OCPBUGS-84368, which is valid. The bug has been moved to the POST state. 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, sdminonne 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
the
approved
|
According to claude I'm rerunning |
|
/test e2e-conformance |
openshift-ci
Bot
added
the
needs-rebase
|
Thanks! few questions:
why is this annotation needed? does the autoscaler has any specific behaviour for pods with this priority class during draining?
where does autoscaler implement this? |
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
8979ea8 to
a1b9129
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aks-4-22 |
|
/test e2e-aws-4-22 |
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
/verified by me in local-dev |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aks |
|
/test e2e-aws |
|
/test e2e-aks |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aks |
|
/test e2e-aks |
openshift-ci
Bot
added
the
needs-rebase
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
3339fbb to
7ab35c9
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
the
needs-rebase
…y spread constraint Add the cluster-autoscaler safe-to-evict annotation to kas-connection-checker pods so PDBs do not block node scale-down. Add topology spread constraint to distribute pods across nodes. Fix stale error message and add missing toleration assertion. Gate the e2e spec test with CPOAtLeast for pre-4.23 HCCO. Co-Authored-By: Claude Opus 4.6 <[email protected]>
sdminonne
force-pushed
the
fix/kas-connection-checker-pdb-safe-to-evict
branch
from
7ab35c9 to
d32c325
Compare
|
I now have all the evidence needed. The job state is Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis job was not a real failure — it was aborted by the Prow trigger plugin before it could finish. All four image builds ( Root CauseThe Prow trigger plugin aborted this job run. This happens when:
The
The build log shows all image builds completed successfully:
The interrupt signal was received at 09:19:53Z while the final step — creating the release image This is not a PR code issue. The PR's code changes compiled and built into images without errors. Recommendations
Evidence
|
|
@sdminonne: 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. |
5 participants
Summary
Problem
The
kas-connection-checkerdeployment runs in thekube-systemnamespace without a PDB or safe-to-evict annotation. The cluster autoscaler treats pods inkube-systemwithout a PDB as system-critical pods that cannot be evicted during scale-down, blocking node draining on underutilized nodes. See also the autoscaler system drainability rule.Additionally, the deployment used a blanket
{Effect: NoSchedule, Operator: Exists}toleration that matched the cordon taint (node.kubernetes.io/unschedulable:NoSchedule), causing replacement pods to be scheduled back onto cordoned nodes during drain — creating a destructive evict-reschedule loop.Solution
Add
cluster-autoscaler.kubernetes.io/safe-to-evict: "true"annotation to the pod template. This bypasses the autoscaler's system drainability rule for kube-system pods, allowing nodes to be scaled down. The annotation is preferred over a PDB because it doesn't block scale-to-zero operations.Remove all custom tolerations. The previous blanket
NoScheduletoleration matched the cordon taint, causing the evict-reschedule loop described above. In a HyperShift hosted cluster there are no master nodes in the guest cluster, and the checker doesn't need to run on infra nodes, so noNoScheduletolerations are needed. Kubernetes provides defaultNoExecutetolerations (300s grace period) forunreachableandnot-readyvia theDefaultTolerationSecondsadmission controller.Changes
control-plane-operator/.../resources/resources.go: Addsafe-to-evictannotation to pod template; remove all custom tolerations (set tonil).control-plane-operator/.../resources/resources_test.go: Update tests to validate thesafe-to-evictannotation and assert no custom tolerations are set.Test plan
go test ./control-plane-operator/hostedclusterconfigoperator/controllers/resources/... -count=1passesFixes: https://redhat.atlassian.net/browse/OCPBUGS-84368
🤖 Generated with Claude Code