OCPBUGS-98464: handle stale DaemonSet in GlobalPullSecret test#8990
OCPBUGS-98464: handle stale DaemonSet in GlobalPullSecret test#8990jparrill wants to merge 1 commit into
Conversation
The kubelet-config-verifier DaemonSet was created with a raw Create call that failed with 409 AlreadyExists when a previous test run left it behind (e.g., after a timeout before cleanup). The cleanup also used g.Expect which could fail the test during teardown, preventing resource removal. Delete any stale DaemonSet before creating a new one, and move cleanup to t.Cleanup so it always runs regardless of test outcome. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
/area ci |
📝 WalkthroughWalkthroughThe kubelet config verifier now deletes an existing DaemonSet before creating a new one, ignoring not-found errors and reporting other deletion failures. Resource cleanup is registered through 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@jparrill: The label(s) 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 kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
area/testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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 |
|
/area ci-tooling |
openshift-ci
Bot
added
approved
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-98464, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/util/globalps.go`:
- Around line 29-30: Update the pull-secret setup flow in the verifier creation
logic to reconcile or replace the existing kube-system/pull-secret before
creating the DaemonSet. Ensure stale credentials from prior runs are refreshed
from the current cluster pull secret, rather than accepting an existing Secret
unchanged, while preserving the subsequent node-file comparison behavior.
- Around line 54-64: Update the stale DaemonSet cleanup using the existing
`staleDS` and `guestClient` flow so deletion waits for confirmed completion:
after `Delete` succeeds, poll `Get` until it returns `NotFound`, propagating
other errors and honoring the context before proceeding to create the
replacement.
🪄 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: 467ef7e9-22e8-4b4c-86fe-9c50dc753ee6
📒 Files selected for processing (1)
test/e2e/util/globalps.go
| // on all nodes of the cluster, comparing it with the cluster's pull secret. | ||
| // Stale resources from a previous failed run are cleaned up before creation. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Refresh the replicated pull-secret before creating the verifier.
The new comment claims stale resources are cleaned before creation, but an existing kube-system/pull-secret is still accepted unchanged. A previous interrupted run can leave stale data, causing the verifier to compare the node file against old credentials and produce false failures or passes. Reconcile or replace the Secret before creating the DaemonSet.
🤖 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/util/globalps.go` around lines 29 - 30, Update the pull-secret setup
flow in the verifier creation logic to reconcile or replace the existing
kube-system/pull-secret before creating the DaemonSet. Ensure stale credentials
from prior runs are refreshed from the current cluster pull secret, rather than
accepting an existing Secret unchanged, while preserving the subsequent
node-file comparison behavior.
| // Clean up any stale DaemonSet left by a previous failed run | ||
| staleDS := &appsv1.DaemonSet{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: KubeletConfigVerifierDaemonSetName, | ||
| Namespace: KubeletConfigVerifierNamespace, | ||
| }, | ||
| } | ||
| if err := guestClient.Delete(ctx, staleDS); err != nil && !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("failed to delete stale DaemonSet: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline test/e2e/util/globalps.go --view expanded || true
echo '--- relevant lines 1-280 ---'
nl -ba test/e2e/util/globalps.go | sed -n '1,280p'
echo '--- search for daemonset name and wait/retry logic ---'
rg -n "KubeletConfigVerifierDaemonSetName|Delete\\(|AlreadyExists|NotFound|WaitFor|poll|Eventually|Create\\(" test/e2e/util/globalps.go test/e2e -g '!**/vendor/**'Repository: openshift/hypershift
Length of output: 742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant lines 1-280 ---'
sed -n '1,280p' test/e2e/util/globalps.go | cat -n
echo '--- focused search for waiting/retry logic ---'
rg -n "AlreadyExists|NotFound|WaitFor|Eventually|Poll|Create\\(|Delete\\(" test/e2e/util/globalps.goRepository: openshift/hypershift
Length of output: 11853
Wait for the DaemonSet to be fully deleted before recreating it. Delete only requests removal; if a previous run left kubelet-config-verifier terminating, the subsequent Create can still fail with AlreadyExists. Poll Get until NotFound before creating the replacement.
🤖 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/util/globalps.go` around lines 54 - 64, Update the stale DaemonSet
cleanup using the existing `staleDS` and `guestClient` flow so deletion waits
for confirmed completion: after `Delete` succeeds, poll `Get` until it returns
`NotFound`, propagating other errors and honoring the context before proceeding
to create the replacement.
There was a problem hiding this comment.
The robot is right about this; I would suggest we make a guard / precondition to fail the test immediately if the DaemonSet unexpectedly exists per #8990 (comment)
There was a problem hiding this comment.
@ironcladlou, agreed — that's a cleaner approach than delete-and-poll. Since cleanup is now guaranteed via t.Cleanup, an existing DaemonSet at creation time signals a real problem (e.g., a previous run's cleanup failed), so failing fast with a clear error is more useful than silently deleting and racing a recreate.
I'll plan the change to replace the stale-delete logic in CreateKubeletConfigVerifierDaemonSet with a Get-based precondition that errors out immediately if the DaemonSet already exists.
- Replace the "delete stale DaemonSet" block in
CreateKubeletConfigVerifierDaemonSet(test/e2e/util/globalps.go) with a precondition check usingGet - If the DaemonSet already exists, return a descriptive error causing the test to fail immediately (via the
g.Expect(err).NotTo(HaveOccurred())call inVerifyKubeletConfigWithDaemonSet) instead of deleting and recreating it - Propagate any non-NotFound
Geterror as a failure too
Failed to handle agent chat message. Please try again.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8990 +/- ##
=======================================
Coverage 43.79% 43.79%
=======================================
Files 772 772
Lines 96037 96037
=======================================
Hits 42061 42061
Misses 51061 51061
Partials 2915 2915
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| // Clean up any stale DaemonSet left by a previous failed run |
There was a problem hiding this comment.
Is it really needed to have the deletion in 2 places? IIUC the cleanup below always runs so this should not be necessary.
Are there cases in which the cleanup could not run?
Are the cases in which the cleanup could fail but this deletion succeed?
There was a problem hiding this comment.
If the test can't tolerate the leaked/existing DaemonSet, and we have what should be a reliable cleanup function in place (below), what if instead of the duplicated cleanup logic here we added an additional precondition guard which fails the test immediately if the unexpected DaemonSet already exists in order to send a clear signal that we've got a cleanup bug?
|
Despite the feedback above I think if the PR's current state w/ the primary deletion lifecycle change makes the test more reliable we should merge it ASAP and incorporate the feedback into a followup |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
Now I have all the evidence needed. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe Key evidence of the rolling update race:
Why 2/3: The hosted cluster Why PR #8990 does not fix this: The PR addresses a different issue — it handles stale The test ran the Recommendations
Evidence
|
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/retest-required |
|
@jparrill: 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
kubelet-config-verifierDaemonSet before creating a new one inCreateKubeletConfigVerifierDaemonSett.Cleanup()inVerifyKubeletConfigWithDaemonSetso it runs even when the test fails or times outFixes
Root Cause
The
EnsureGlobalPullSecrettest creates akubelet-config-verifierDaemonSet to verify pull secret propagation. Two issues caused intermittent 409 errors and cascading failures:No stale resource handling:
CreateKubeletConfigVerifierDaemonSetused a rawclient.Create()with noAlreadyExistshandling. If a previous test run left the DaemonSet behind (e.g., after a timeout), the next run got a 409 and failed.Cleanup dependent on test success: Resource cleanup ran inline after the readiness wait using
g.Expect().To(Succeed()). If the readiness wait timed out, cleanup never executed — leaving stale resources for the next run.Changes
t.Cleanup()— always runsg.Expect(fails the test on cleanup error)t.Logfwarning (non-fatal)Test plan
e2e-aws-ovnperiodic should no longer hit 409 errors on thekubelet-config-verifierDaemonSet🤖 Generated with Claude Code
Summary by CodeRabbit