Skip to content

OCPBUGS-98464: handle stale DaemonSet in GlobalPullSecret test #8990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jparrill wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −20
Open

OCPBUGS-98464: handle stale DaemonSet in GlobalPullSecret test #8990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 36 additions & 20 deletions test/e2e/util/globalps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ const (
)

// CreateKubeletConfigVerifierDaemonSet creates a DaemonSet that verifies the config.json file
// on all nodes of the cluster, comparing it with the cluster's pull secret
// 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.
Comment on lines +29 to +30

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crclient.Client, dsImage string) error {
// Get the cluster's pull secret for comparison
pullSecret := &corev1.Secret{}
Expand All @@ -50,6 +51,17 @@ func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crcli
}
}

// Clean up any stale DaemonSet left by a previous failed run

@clebs clebs Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ironcladlou ironcladlou Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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)
}

Comment on lines +54 to +64

@coderabbitai coderabbitai Bot Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.go

Repository: 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.

@ironcladlou ironcladlou Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 using Get
  • If the DaemonSet already exists, return a descriptive error causing the test to fail immediately (via the g.Expect(err).NotTo(HaveOccurred()) call in VerifyKubeletConfigWithDaemonSet) instead of deleting and recreating it
  • Propagate any non-NotFound Get error as a failure too

Failed to handle agent chat message. Please try again.

daemonSet := &appsv1.DaemonSet{
ObjectMeta: metav1.ObjectMeta{
Name: KubeletConfigVerifierDaemonSetName,
Expand Down Expand Up @@ -205,6 +217,29 @@ func CreateKubeletConfigVerifierDaemonSet(ctx context.Context, guestClient crcli
func VerifyKubeletConfigWithDaemonSet(t *testing.T, ctx context.Context, guestClient crclient.Client, dsImage string, expectedNodeCount int32) {
g := NewWithT(t)

// Register cleanup first so it runs even if the test fails or times out
t.Cleanup(func() {
t.Log("Cleaning up kubelet config verifier resources")
ds := &appsv1.DaemonSet{
ObjectMeta: metav1.ObjectMeta{
Name: KubeletConfigVerifierDaemonSetName,
Namespace: KubeletConfigVerifierNamespace,
},
}
if err := guestClient.Delete(ctx, ds); err != nil && !apierrors.IsNotFound(err) {
t.Logf("Warning: failed to clean up DaemonSet: %v", err)
}
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "pull-secret",
Namespace: KubeletConfigVerifierNamespace,
},
}
if err := guestClient.Delete(ctx, secret); err != nil && !apierrors.IsNotFound(err) {
t.Logf("Warning: failed to clean up pull secret: %v", err)
}
})

// Create the DaemonSet
t.Log("Creating kubelet config verifier DaemonSet")
err := CreateKubeletConfigVerifierDaemonSet(ctx, guestClient, dsImage)
Expand All @@ -218,23 +253,4 @@ func VerifyKubeletConfigWithDaemonSet(t *testing.T, ctx context.Context, guestCl
konnectivityDS := hccomanifests.KonnectivityAgentDaemonSet()
g.Expect(waitForDaemonSetReady(t, ctx, guestClient, konnectivityDS.Name, konnectivityDS.Namespace, expectedNodeCount)).To(Succeed())
g.Expect(waitForDaemonSetReady(t, ctx, guestClient, KubeletConfigVerifierDaemonSetName, KubeletConfigVerifierNamespace, expectedNodeCount)).To(Succeed())

// Clean up the DaemonSet after verification
t.Log("Cleaning up kubelet config verifier DaemonSet")
daemonSet := &appsv1.DaemonSet{
ObjectMeta: metav1.ObjectMeta{
Name: KubeletConfigVerifierDaemonSetName,
Namespace: KubeletConfigVerifierNamespace,
},
}
g.Expect(guestClient.Delete(ctx, daemonSet)).To(Succeed())

// Clean up pull-secret secret in kube-system namespace
pullSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "pull-secret",
Namespace: KubeletConfigVerifierNamespace,
},
}
g.Expect(guestClient.Delete(ctx, pullSecret)).To(Succeed())
}