SPLAT-2854: Fixing issue related to unintended VAP interactions#1518
SPLAT-2854: Fixing issue related to unintended VAP interactions#1518vr4manta wants to merge 1 commit into
Conversation
|
@vr4manta: This pull request references SPLAT-2854 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughAdds a reusable helper for the ChangesVAP match policy configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/e2e/vsphere/failure_domain_vap.go (2)
236-236: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a bounded, cancellable context for the suite.
context.Background()cannot expire or be canceled, so a blocked Kubernetes request can stall this serial suite. Use a per-spec context with cancellation or a deadline.As per path instructions, Go code must use
context.Contextfor cancellation and timeouts.🤖 Prompt for 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. In `@test/e2e/vsphere/failure_domain_vap.go` at line 236, Replace context.Background() in the suite setup with a per-spec context.Context that has cancellation or a deadline, and ensure the cancel function is invoked during spec cleanup. Propagate this bounded context through the suite’s Kubernetes operations so blocked requests can terminate without changing the serial execution behavior.Source: Path instructions
281-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the deployed policies use
Equivalentmatching.These checks only verify existence, and every update below uses
config.openshift.io/v1, so the suite still passes ifMatchPolicyreverts to its default. Assert each returned VAP'sSpec.MatchConstraints.MatchPolicy, or add equivalent constructor assertions inpkg/webhooks/vap_test.go.🤖 Prompt for 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. In `@test/e2e/vsphere/failure_domain_vap.go` around lines 281 - 304, Update the VAP deployment test in the “should have three VAPs and three bindings deployed” case to retain each returned ValidatingAdmissionPolicy and assert its Spec.MatchConstraints.MatchPolicy is Equivalent. Apply this to the Machine, CPMS, and MachineSet policies while preserving the existing existence checks; binding assertions do not need this validation.
🤖 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 `@test/e2e/vsphere/failure_domain_vap.go`:
- Around line 140-158: Update the failure-domain selection logic around the
firstMatch fallback to return false when no CPMS-only failure domain can be
isolated. Remove the fallback that returns a worker-shared failure domain, so
downstream rejection continues to identify ControlPlaneMachineSet as the failure
reason.
- Around line 500-521: Update the failure-domain selection logic around the fd
candidate loop to also exclude domains referenced by existing MachineSets.
Inspect each MachineSet’s failure-domain region and zone labels, mark matching
candidates as used, and only assign fd when no Machine, CPMS, or MachineSet
references it.
- Around line 379-390: Register a DeferCleanup restoration before the
Infrastructure mutation in the failure-domain test, before the Update call in
the removal flow. Ensure the cleanup re-fetches the current Infrastructure and
restores infra.Spec.PlatformSpec.VSphere.FailureDomains even if the subsequent
GET or restore update fails, matching the existing mutation-test cleanup
pattern; then retain the explicit restoration path as appropriate.
- Around line 491-498: Update the CPMS lookup in the failure-domain selection
flow to treat only an apierrors.IsNotFound(cpmsErr) result as an absent CPMS;
for every other non-nil error, assert or fail immediately instead of continuing
as though no CPMS exists. Preserve the existing FailureDomains processing when
the lookup succeeds.
- Around line 426-431: Update the MachineSet cleanup in DeferCleanup to wait
until the deleted testMS is confirmed absent. After issuing Delete, poll the
same MachineSets resource using the existing context and testMS.Name until the
Get result returns apierrors.IsNotFound; preserve warning logs for unexpected
deletion or polling errors.
- Around line 70-72: Remove the single-failure-domain shortcut in the
failure-domain lookup logic that returns fds[0] when len(fds) == 1. Let the
existing region/zone label matching determine whether the Machine VAP applies,
so unlabeled single-FD Machines are not treated as VAP references.
---
Nitpick comments:
In `@test/e2e/vsphere/failure_domain_vap.go`:
- Line 236: Replace context.Background() in the suite setup with a per-spec
context.Context that has cancellation or a deadline, and ensure the cancel
function is invoked during spec cleanup. Propagate this bounded context through
the suite’s Kubernetes operations so blocked requests can terminate without
changing the serial execution behavior.
- Around line 281-304: Update the VAP deployment test in the “should have three
VAPs and three bindings deployed” case to retain each returned
ValidatingAdmissionPolicy and assert its Spec.MatchConstraints.MatchPolicy is
Equivalent. Apply this to the Machine, CPMS, and MachineSet policies while
preserving the existing existence checks; binding assertions do not need this
validation.
🪄 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: a3a488a1-ed58-4124-8ce9-77fe7b798e54
📒 Files selected for processing (2)
pkg/webhooks/vap.gotest/e2e/vsphere/failure_domain_vap.go
bf1b8d2 to
556cc26
Compare
Detects spurious VAP updates caused by nil vs server-defaulted field diffs in resourceapply. Asserts resourceVersion stability across 6 MAO sync cycles — fails before openshift/machine-api-operator#1518, passes after. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/webhooks/vap.go (1)
44-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
MatchPolicy: Equivalent.The constructors now depend on this setting, but the existing tests in
pkg/webhooks/vap_test.godo not assert it. Add an assertion for all three policies so a future refactor cannot silently revert to exact matching.Also applies to: 72-74, 183-185, 290-292
🤖 Prompt for 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. In `@pkg/webhooks/vap.go` around lines 44 - 47, Add regression assertions in pkg/webhooks/vap_test.go for all three policy constructors, verifying each sets MatchPolicy to Equivalent. Cover the constructors associated with the referenced locations and preserve the existing test structure while ensuring a future change cannot revert them to exact matching.
🤖 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.
Nitpick comments:
In `@pkg/webhooks/vap.go`:
- Around line 44-47: Add regression assertions in pkg/webhooks/vap_test.go for
all three policy constructors, verifying each sets MatchPolicy to Equivalent.
Cover the constructors associated with the referenced locations and preserve the
existing test structure while ensuring a future change cannot revert them to
exact matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 982a7a0a-e133-4c2f-9be9-ebea1f38e4a2
📒 Files selected for processing (1)
pkg/webhooks/vap.go
|
/verified by @jcpowermac via integration testing. https://github.com/jcpowermac/testing-day2-vcenter/blob/main/docs/tests/N-SEQ-08.md |
openshift-ci-robot
added
the
verified
|
@jcpowermac: 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/test ? |
|
/test e2e-vsphere-ovn e2e-vsphere-ovn-multi-vcenter |
|
/retest |
|
/lgtm |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
@nrb: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb 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
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@vr4manta: 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. |
4 participants
SPLAT-2854
Changes
Summary by CodeRabbit