OCPCLOUD-3436: use AlwaysAllow UnhealthyPodEvictionPolicy option in PDBs#457
OCPCLOUD-3436: use AlwaysAllow UnhealthyPodEvictionPolicy option in PDBs#457atiratree wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@atiratree: This pull request references OCPCLOUD-3436 which is a valid jira issue. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughgetPDB now sets Spec.UnhealthyPodEvictionPolicy to policy/v1's AlwaysAllow using a pointer. Tests were updated: the podDisruptionBudget test helper defaults the field, and a table test asserts applyPodDisruptionBudget reports a modification when an existing PDB has a nil eviction policy but the desired PDB has a non-nil value. ChangesPodDisruptionBudget UnhealthyPodEvictionPolicy
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ 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 NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/resourceapply/resourceapply_test.go`:
- Around line 859-869: The test currently sets the desired PDB's
UnhealthyPodEvictionPolicy to nil and expects an update, but the PR intends to
verify reconciling from nil -> AlwaysAllow; update the table entry so that
applyPodDisruptionBudgetArguments.inputFn returns a PDB with
Spec.UnhealthyPodEvictionPolicy set to
policyv1.UnhealthyPodEvictionPolicyType("AlwaysAllow") (or the helper that
produces that), and existingFn returns a PDB with
Spec.UnhealthyPodEvictionPolicy == nil (use podDisruptionBudget for existing and
modify inputFn accordingly), keeping expectModified: true and leaving
applyPodDisruptionBudgetArguments and podDisruptionBudget references to locate
the change.
🪄 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: 6b8fc36b-42f6-4c8e-b472-dd49b1a097db
📒 Files selected for processing (2)
pkg/cloud/common/resources.gopkg/controllers/resourceapply/resourceapply_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controllers/resourceapply/resourceapply_test.go (1)
859-869:⚠️ Potential issue | 🟠 MajorTest the
nil -> AlwaysAllowdirection instead of the reverse.This entry still sets the desired PDB to
niland the existing one toAlwaysAllow, so it validates the opposite transition from the one this PR is meant to guarantee. Please swap the fixtures so the desired object carriesAlwaysAllowand the existing object isnil.🔁 Suggested fix
Entry("When there is a mismatch of UnhealthyPodEvictionPolicy it is updated", applyPodDisruptionBudgetArguments{ - inputFn: func(namespace string) *policyv1.PodDisruptionBudget { - pdb := podDisruptionBudget(namespace) - pdb.Spec.UnhealthyPodEvictionPolicy = nil - return pdb - }, - existingFn: podDisruptionBudget, + inputFn: podDisruptionBudget, + existingFn: func(namespace string) *policyv1.PodDisruptionBudget { + pdb := podDisruptionBudget(namespace) + pdb.Spec.UnhealthyPodEvictionPolicy = nil + return pdb + }, expectModified: true, }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/resourceapply/resourceapply_test.go` around lines 859 - 869, The test entry in resourceapply_test.go currently validates the reverse transition by setting inputFn to return a PDB with UnhealthyPodEvictionPolicy = nil and existingFn to return podDisruptionBudget (which is AlwaysAllow); swap those fixtures so the desired (inputFn) PDB carries AlwaysAllow and the existingFn returns nil. Update the applyPodDisruptionBudgetArguments entry (inputFn, existingFn) for this case so inputFn builds the PDB with Spec.UnhealthyPodEvictionPolicy = pointer to policyv1.UnhealthyPodEvictionPolicyType("AlwaysAllow") and existingFn returns podDisruptionBudget with Spec.UnhealthyPodEvictionPolicy = nil, keeping expectModified: true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controllers/resourceapply/resourceapply_test.go`:
- Around line 859-869: The test entry in resourceapply_test.go currently
validates the reverse transition by setting inputFn to return a PDB with
UnhealthyPodEvictionPolicy = nil and existingFn to return podDisruptionBudget
(which is AlwaysAllow); swap those fixtures so the desired (inputFn) PDB carries
AlwaysAllow and the existingFn returns nil. Update the
applyPodDisruptionBudgetArguments entry (inputFn, existingFn) for this case so
inputFn builds the PDB with Spec.UnhealthyPodEvictionPolicy = pointer to
policyv1.UnhealthyPodEvictionPolicyType("AlwaysAllow") and existingFn returns
podDisruptionBudget with Spec.UnhealthyPodEvictionPolicy = nil, keeping
expectModified: true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b4cef41e-c0b0-4371-92ab-c3fba1d61924
📒 Files selected for processing (2)
pkg/cloud/common/resources.gopkg/controllers/resourceapply/resourceapply_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloud/common/resources.go
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/controllers/resourceapply/resourceapply_test.go`:
- Line 861: A goimports formatting error affects the test file near the table
entry using inputFn: podDisruptionBudget; run goimports (or gofmt then
goimports) on that file to fix imports and formatting, then stage the updated
file so the linter passes — ensure imports are grouped/ordered and any unused
imports removed before committing.
🪄 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: 400ff8c1-7409-4db9-bf9c-0cedba2d0566
📒 Files selected for processing (2)
pkg/cloud/common/resources.gopkg/controllers/resourceapply/resourceapply_test.go
|
@atiratree: The following test 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. |
2 participants
Allow eviction of unhealthy (not ready) pods even if there are no disruptions
allowed on a PodDisruptionBudget. This can help to drain/maintain a node and
recover without a manual intervention when multiple instances of nodes or pods
are misbehaving.
Conventions change: openshift/enhancements#1995
Summary by CodeRabbit
Bug Fixes
Tests