test, e2e-upgrade: run per-example cleanup even when upgrade gates fail - #1573
Conversation
kubevirt-prow
Bot
added
do-not-merge/work-in-progress
|
Hi @Copilot. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
mkowalski
left a comment
mkowalski
left a comment
There was a problem hiding this comment.
Disposition: REQUEST_CHANGES
The test-isolation direction is correct, and increasing the slow bond budgets is reasonable, but the current cleanup implementation can still delete the only cleanup request while leaving dirty host state. Two independent reproducers confirmed the blocking failure modes.
Required changes
-
Register cleanup before applying the example policy
kubectl applycan create the NNCP before returning an error.- Register
DeferCleanupimmediately beforecreateUpgradeCasePolicy. - The cleanup callback should use bounded polling to determine whether the policy exists.
NotFoundis a successful no-op; transient API errors should retry for a meaningful duration.
-
Build and submit one atomic cleanup desired state
- Merge
ExampleSpec.CleanupStateand allIfaceNames(state: absent) into onenmstate.State. - Submit one NNCP update, not one complete desired-state replacement per interface.
- Merge
-
Verify the cleanup generation, not stale NNCP conditions
- Capture the NNCP generation returned by the cleanup update.
- List matching NNCEs by
nmstate.io/policy=<policy-name>. - Require all matching enactments to have
status.policyGeneration == cleanup generationandAvailable=Truebefore deleting the NNCP. - A plain
kubectl wait nncp ... Availableis insufficient because NNCP conditions have noobservedGenerationand can still reflect the previous spec.
-
Do not delete on failed restoration
- If cleanup update/convergence cannot be verified, leave the NNCP containing the cleanup state so a recovering handler can continue reconciliation.
- Abort the suite (
AbortSuite) to prevent subsequent specs from running on contaminated nodes. - Delete the NNCP only after cleanup generation convergence is proven.
-
Make deletion/API polling resilient
- A transient GET after DELETE must not terminate the wait immediately; continue polling until NotFound or timeout while retaining the last error for diagnostics.
- If existence cannot be determined after bounded retries, abort the suite rather than silently proceeding.
-
Handle intentionally skipped examples
Skipoccurs before policy creation. Do not reinstall operators or wait for a non-existent policy in teardown for these specs.
-
Repository requirements
- Remove/fold the unsigned
Initial plancommit: current DCO failure reports commit69c261fmissingSigned-off-by. - Mark ready for review when complete (
do-not-merge/work-in-progressis currently present). - This is test-only; use
/release-note-none(or add the repository-expected release-note treatment) to cleardo-not-merge/release-note-label-needed.
- Remove/fold the unsigned
Verification gates
go test ./test/e2e/upgrade -run '^$'go vet ./test/e2e/upgrade- repo golangci-lint for
./test/e2e/upgrade/... /test pull-kubernetes-nmstate-e2e-upgrade-k8son the exact final commit- Confirm the run finishes under the Prow
E2E_TEST_TIMEOUT=1h, reports the four intentional skips, and has no downstreameth1 ... attached to controller bond0cascade.
Review evidence
- Cleanup-delete reproducer: cleanup failure followed by deferred deletion left
bond0present and visible to the next spec. - Existence-check reproducer: a transient API GET failed before cleanup registration; policy and host state both remained.
- Source confirms NNCP deletion only removes its enactment and does not revert node network state.
Generated by /code-review:deep-review. This review was generated using AI. Please verify before acting on it.
|
|
||
| // Deleting the policy is deferred so it also happens if the cleanup | ||
| // configuration below does not converge in time. | ||
| DeferCleanup(func() { |
There was a problem hiding this comment.
BLOCKING (runtime-reproduced): do not defer unconditional NNCP deletion. If createUpgradeCaseCleanupPolicy fails, this callback still deletes the NNCP. Deleting an NNCP does not revert host networking; the controller only removes the enactment, so bond0 can remain while the only cleanup reconciliation request disappears. A focused Ginkgo reproducer observed cleanup-fails policy=true bond0=true then deferred-delete policy=false bond0=true, and the next spec still saw bond0=true. Delete only after restoration for the cleanup generation is proven. If restoration fails, keep the cleanup NNCP and abort the suite so contaminated state cannot reach another example.
There was a problem hiding this comment.
Fixed in 54dd577. Deletion is no longer deferred unconditionally: cleanupUpgradeCase deletes the NNCP only after waitForPolicyGenerationApplied proves every matching enactment applied the cleanup generation with Available=True. On any failure (compose, update, convergence or delete) the NNCP is left holding the cleanup desired state so a recovering handler keeps reconciling it, and AbortSuite stops the remaining specs so contaminated nodes can't reach another example. deletePolicy also retries the DELETE and keeps polling for NotFound instead of failing on a transient GET.
| } | ||
|
|
||
| kubectlAndCheck("wait", "nncp", example.PolicyName, "--for", "condition=Available", "--timeout", "3m") | ||
| kubectlAndCheck("wait", "nncp", example.PolicyName, "--for", "condition=Available", "--timeout", PolicyAvailableTimeout) |
There was a problem hiding this comment.
BLOCKING: this wait can accept stale success, and the interface loop can lose cleanup operations. Updating spec.desiredState preserves the old NNCP Available=True; NNCP conditions have no observedGeneration, so kubectl wait may return before cleanup is applied. Also, each IfaceNames iteration replaces the entire desired state; workqueue coalescing can reconcile only the last generation (e.g. remove ovs0 but leave br1). Build one cleanup desired state containing all absent interfaces, update once, capture the resulting NNCP generation, then wait until every matching NNCE reports status.policyGeneration == generation and Available=True before deleting the policy.
There was a problem hiding this comment.
Fixed in 54dd577. cleanupDesiredState now merges ExampleSpec.CleanupState with every IfaceNames entry (state: absent) into a single nmstate state, submitted as one updatePolicyDesiredState call, so workqueue coalescing can no longer drop an interface (verified the composed state for all examples, e.g. ovs-bridge-iface yields br1 + ovs0 absent in one state, and dns keeps its dns-resolver block).
The kubectl wait nncp ... Available gate is gone from the cleanup path. waitForPolicyGenerationApplied captures the generation returned by the update, lists NNCEs by nmstate.io/policy=<policy> and requires every one of them to report status.policyGeneration == <generation> and Available=True. If no enactment matches the policy (no matching node) it falls back to the NNCP Available condition. The NNCP is deleted only after that.
| knmstateReporter.ReportAfterEach(specReport) | ||
| }) | ||
|
|
||
| func policyExists(name string) bool { |
There was a problem hiding this comment.
BLOCKING (runtime-reproduced): a one-shot/transient Get can bypass all cleanup. policyExists fails before DeferCleanup is registered, so a short API timeout leaves both the policy and dirty host state. Register cleanup before kubectl apply (apply may create the NNCP before returning an error), then determine existence with meaningful bounded polling (seconds, not retry.DefaultRetry's ~40ms). Treat NotFound as a successful no-op and retry transient read failures.
There was a problem hiding this comment.
Fixed in f4b1f5b/54dd577. DeferCleanup(cleanupUpgradeCase) is now registered immediately before createUpgradeCasePolicy, so an apply that creates the NNCP and then fails is still cleaned up. Existence is determined by policyExists, which polls up to APIRetryTimeout (30s at ReadInterval), treats NotFound as a successful no-op and retries transient reads; if existence still can't be determined the suite is aborted instead of silently skipping cleanup.
Ordering is preserved: I verified with a scratch Ginkgo suite that a DeferCleanup registered in the It runs after all AfterEach nodes at that level and still runs when one of them fails.
|
@copilot Please address all blocking findings in review #1573 (review), update this PR branch, and run the listed local verification gates. In particular, make cleanup atomic and generation-aware, retain the cleanup NNCP + abort the suite when restoration cannot be proven, fix transient API polling, fold/remove the unsigned plan commit, apply This message was generated using AI. Please verify before acting on it. |
Addressed in
On the repo requirements: Verified locally: |
All blocking findings are implemented on the branch (latest commit also removes the last stale-condition path):
Verification gates run locally on the final commit: Two items need maintainer action: I can't run |
|
/release-note-none |
kubevirt-prow
Bot
added
release-note-none
mkowalski
left a comment
mkowalski
left a comment
There was a problem hiding this comment.
Second-pass disposition: REQUEST_CHANGES
Copilot addressed the original deep-review blockers in substance:
- cleanup is registered before apply;
- transient API reads/deletes have meaningful bounded retry;
- cleanup desired state is atomic across all interfaces;
- deletion occurs only after attempted verification;
- failed cleanup retains the NNCP and aborts the suite;
- skipped/unapplied examples do not run the upgrade teardown.
Local compilation, vet, and golangci-lint also pass on 502baeb3c.
Two blocking generation-verification gaps remain:
-
Capture the expected NNCE set before the cleanup update.
- List enactments by
nmstate.io/policy=<name>before updating the NNCP. - Store the exact expected enactment names.
- If the expected set is empty, no enactment applied the example state; cleanup can be a no-op/delete. Do not infer “no matching nodes” from a transient empty post-update list.
- List enactments by
-
Require the exact cleanup generation on the complete enactment set.
- After the single cleanup update, wait until every expected NNCE is present, has
status.policyGeneration == cleanupGeneration, andAvailable=True. - Do not succeed when only a subset is returned.
- Also reject any actual policy-labeled enactment that is stale/unavailable so newly appearing enactments cannot be ignored.
- Remove the NNCP heartbeat/wall-clock fallback (
updatedAt,policyIsAvailableSince); generation-aware NNCE status is the server-observable token.
- After the single cleanup update, wait until every expected NNCE is present, has
Repository/CI blockers also remain:
- Lowercase required
dcostill fails because unsigned commit69c261fremains. The uppercase GitHub DCO check passing does not satisfy Prow. This needs a maintainer history rewrite/drop or recreation/squash path. - PR remains draft with
do-not-merge/work-in-progressanddo-not-merge/release-note-label-needed. - No Prow presubmits have run on final head. After the code update, a collaborator must apply
/release-note-none, mark ready, and run/test allplus/test pull-kubernetes-nmstate-e2e-upgrade-k8s.
Runtime acceptance remains: exact final head must finish the upgrade suite under E2E_TEST_TIMEOUT=1h, retain the four intentional skips, and show no downstream eth1 ... attached to controller bond0 cascade.
This review was generated using AI. Please verify before acting on it.
| if err != nil { | ||
| return err | ||
| } | ||
| if len(enactments.Items) == 0 { |
There was a problem hiding this comment.
BLOCKING: zero NNCEs is not equivalent to “no matching node,” and the fallback is unsafe. An empty list can be transient (handler restart/list race) or mean matching nodes have not created NNCEs yet. A genuinely unmatched policy reports Ignored/NoMatchingNode, not Available=True, while a policy can also be Available with NotReady nodes ignored. The wall-clock heartbeat fallback (updatedAt) is also race-prone: an old-generation heartbeat can occur after the client timestamp but before the successful Update. Remove this fallback. Snapshot the policy-labeled NNCE names before the cleanup update; if the snapshot is empty, no node has an enactment that could have applied the original state, so cleanup can be treated as a no-op/delete. Otherwise require those expected NNCEs to remain present and reach the cleanup generation + Available=True.
There was a problem hiding this comment.
Fixed in 3aaeaff/3b2b397. The heartbeat fallback is gone: policyIsAvailableSince is removed and waitForPolicyGenerationApplied no longer takes a wall-clock token. cleanupUpgradeCase now snapshots the policy-labeled NNCE names with policyEnactmentNames (bounded retry on transient list errors) before the cleanup update. An empty snapshot means no enactment could have applied the example state, so the update/wait is skipped and cleanup is just the delete. A non-empty snapshot is what convergence is verified against, so a transient empty list afterwards is a failure, not a success — and waitForPolicyGenerationApplied itself errors out if it is ever asked to verify with nothing expected and nothing listed.
| // observedGeneration and can report the previous desired state. | ||
| return policyIsAvailableSince(name, updatedAt) | ||
| } | ||
| for i := range enactments.Items { |
There was a problem hiding this comment.
BLOCKING: a partial enactment list passes. The loop validates only objects currently returned. With two matching nodes, one cleanup-generation Available NNCE plus one missing NNCE returns success, after which the policy is deleted while the missing node may retain the example configuration. Capture the expected enactment names before updating. During convergence require every expected name; also validate every actual enactment is at the cleanup generation and Available (a newly appearing enactment must not be ignored). If an expected NNCE disappears or an unexpected one remains stale, retain the cleanup NNCP and abort rather than delete.
There was a problem hiding this comment.
Fixed in 3aaeaff. The expected enactment names are captured before the update and passed into waitForPolicyGenerationApplied. Each poll now requires every actual policy-labeled enactment to be at the cleanup generation with Available=True (so a newly appearing enactment can't be ignored) and then requires every expected name to be among those converged, so a partial list — one converged NNCE plus one missing — keeps failing instead of passing. If that doesn't converge within PolicyAvailableTimeout the NNCP is retained with the cleanup desired state and the suite is aborted; the delete only happens after convergence.
| // update was issued at. | ||
| func updatePolicyDesiredState(name string, desiredState shared.State) (int64, time.Time, error) { | ||
| generation := int64(0) | ||
| updatedAt := time.Now() |
There was a problem hiding this comment.
The updatedAt wall-clock token is no longer needed once cleanup verification is based on an expected NNCE set and status.policyGeneration. Capturing it before GET/Update retries permits an old-generation heartbeat during retries to look “fresh,” and handler/client clocks and timestamp precision need not align. Return only the successful update generation.
There was a problem hiding this comment.
Removed in 3aaeaff. updatePolicyDesiredState returns (int64, error) again — only the generation of the successful update — and policyIsAvailableSince is deleted. Verification is now purely status.policyGeneration + Available=True over the expected NNCE set.
|
@copilot Please address the two remaining blocking generation-verification findings in #1573 (review). Snapshot the exact policy-labeled NNCE names before the cleanup update, require the complete expected set at the cleanup generation and Available, reject stale/unexpected enactments, and remove the wall-clock/NNCP-status fallback. Run the local gates and update the branch. Please also report anything that still requires maintainer action separately (DCO history rewrite, release-note label, ready state, Prow trigger). This message was generated using AI. Please verify before acting on it. |
All three items are addressed in the latest commits:
Both items addressed:
|
|
/test pull-kubernetes-nmstate-e2e-upgrade-k8s Final runtime validation requested on settled head Comment posted with AI assistance. Please verify before acting on it. Assisted-By: github-copilot/gpt-5.6-sol |
Current readiness on
|
|
/test pull-kubernetes-nmstate-e2e-handler-k8s |
|
/ok-to-test |
|
/retest |
|
/test pull-kubernetes-nmstate-e2e-upgrade-k8s |
|
/lgtm I will squash what need to be squash and add @mkowalski as DCO. |
kubevirt-prow
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qinqon 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 |
kubevirt-prow
Bot
added
the
approved
f3d2dcc to
2e096be
Compare
kubevirt-prow
Bot
added
dco-signoff: yes
Always clean up each upgrade example, even when applying the policy or upgrading the operator fails. Apply cleanup atomically and verify the expected enactments observed the new policy generation before deleting the policy. Fail policy waits immediately on terminal enactment errors, retain reporter diagnostics in CI artifacts, and initialize the reporter with handler nodes. Work around the incompatible copy-mac-from field only in the published v0.87.0 bond fixture. Signed-off-by: Mat Kowalski <mko@redhat.com> Assisted-By: GPT-5.6-Sol <noreply@github.com>
2e096be to
434c0de
Compare
|
/ok-to-test |
|
/retest unrelated 63.6M 6 4080k 0 0 6300 0 2:56:38 0:11:03 2:45:35 0
6 63.6M 6 4080k 0 0 6291 0 2:56:53 0:11:04 2:45:49 0
6 63.6M 6 4080k 0 0 6282 0 2:57:08 0:11:05 2:46:03 0
6 63.6M 6 4080k 0 0 6272 0 2:57:25 0:11:06 2:46:19 0
6 63.6M 6 4080k 0 0 6263 0 2:57:40 0:11:07 2:46:33 0
6 63.6M 6 4080k 0 0 6256 0 2:57:52 0:11:07 2:46:45 0
6 63.6M 6 4080k 0 0 6256 0 2:57:52 0:11:07 2:46:45 0
curl: (18) transfer closed with 62590620 bytes remaining to read
gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
Error: building at STEP "RUN ./build/install-go.sh ${GO_VERSION}": while running runtime: exit status 2
make: *** [Makefile:220: push-handler] Error 2
+ EXIT_VALUE=2 |
|
/hold cancel |
kubevirt-prow
Bot
added
lgtm
|
/retest |
pull-kubernetes-nmstate-e2e-upgrade-k8shad a test-isolation cascade: when thebondexample failed an availability gate, teardown was skipped, leavingbond0enslavingeth1/eth2. Later examples that configureeth1then failed withInterface eth1 cannot have IP enabled as it is attached to controller bond0.Test isolation
Cleanup is registered with
DeferCleanupin theItbody before policy creation. It runs after the upgradeAfterEacheven when that node fails, so a failed apply or post-upgrade gate cannot skip restoration.cleanupUpgradeCase:Available=True;Ignored=True/NoMatchingNodeis positively confirmed.Intentionally skipped examples do not install the new operator or attempt cleanup for a policy that was never created.
v0.87.0 bond compatibility
Exact-head Prow run
2090437894639456256proved that the v0.87.0 handler/nmstate fails its publishedbond.yamlwithInvalidArgument: Failed to find interface eth1 for copy-mac-from of iface bond0wheneth1is enslaved in the same transaction.hack/prepare-e2e-test-upgrade.shapplies a checked compatibility rewrite only when the previous release is exactly v0.87.0: it requirescopy-mac-fromto be present, then removes only that key from the downloaded upgrade-test copy. Current documentation is unchanged, and later releases retain normalcopy-mac-fromupgrade coverage.Availability diagnostics and timeouts
waitForPolicyAvailablereplaces blindkubectl waitcalls. TerminalFailing=True/FailedToConfigureNNCEs stop polling immediately and include the actionable NNCE message;Retryingremains non-terminal.Failure artifacts
The upgrade suite discovers nmstate handler nodes before initializing
KubernetesNMStateReporter. CI teardown uploads${E2E_LOGS}/handler/*alongside operator output, preserving per-spec NNCE state, NetworkManager logs, journals, and device state for failures that occur before later operator reinstalls.Test-only change; no product code touched. A failure in one example can no longer contaminate later examples.
This description was generated with AI assistance. Please verify before acting on it.
Assisted-By: github-copilot/gpt-5.6-sol