Skip to content

Fix longrunning suite for node test cases #31339

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
ngopalak-redhat wants to merge 3 commits into
base: main
Choose a base branch
from
+1,023 −1,024
Open

Fix longrunning suite for node test cases #31339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d63693
Fix longrunning test case Exec timeout
ngopalak-redhat Jun 25, 2026
fc7d1ba
Fix longrunning test case Exec timeout
ngopalak-redhat Jun 30, 2026
d8722c1
Fix longrunning test case Exec timeout
ngopalak-redhat Jul 1, 2026
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
55 changes: 42 additions & 13 deletions test/extended/imagepolicy/imagepolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ func updateImageConfig(oc *exutil.CLI, allowedRegistries []string) {
return err
})
o.Expect(err).NotTo(o.HaveOccurred(), "error updating image config")
WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func cleanupImageConfig(oc *exutil.CLI) error {
Expand All @@ -238,8 +237,7 @@ func cleanupImageConfig(oc *exutil.CLI) error {
return err
})
o.Expect(err).NotTo(o.HaveOccurred(), "error cleaning up image config")
WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
return nil
}

Expand Down Expand Up @@ -285,8 +283,7 @@ func createClusterImagePolicy(oc *exutil.CLI, policy configv1.ClusterImagePolicy
_, err := oc.AdminConfigClient().ConfigV1().ClusterImagePolicies().Create(context.TODO(), &policy, metav1.CreateOptions{})
o.Expect(err).NotTo(o.HaveOccurred())

WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func deleteClusterImagePolicy(oc *exutil.CLI, policyName string) error {
Expand All @@ -296,8 +293,7 @@ func deleteClusterImagePolicy(oc *exutil.CLI, policyName string) error {
if err := oc.AdminConfigClient().ConfigV1().ClusterImagePolicies().Delete(context.TODO(), policyName, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
return fmt.Errorf("failed to delete cluster image policy %s: %v", policyName, err)
}
WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
return nil
}

Expand All @@ -312,8 +308,7 @@ func createImagePolicy(oc *exutil.CLI, policy configv1.ImagePolicy, namespace st

// Wait until each pool's Spec.Configuration.Name changes from the initial value
// and the pool reports Updated=true
WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func deleteImagePolicy(oc *exutil.CLI, policyName string, namespace string) error {
Expand All @@ -323,8 +318,7 @@ func deleteImagePolicy(oc *exutil.CLI, policyName string, namespace string) erro
if err := oc.AdminConfigClient().ConfigV1().ImagePolicies(namespace).Delete(context.TODO(), policyName, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
return fmt.Errorf("failed to delete image policy %s in namespace %s: %v", policyName, namespace, err)
}
WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
return nil
}

Expand Down Expand Up @@ -707,7 +701,42 @@ func WaitForMCPConfigSpecChangeAndUpdated(oc *exutil.CLI, pool string, initialSp
return false
}
return machineconfighelper.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated)
}, 20*time.Minute, 10*time.Second).Should(o.BeTrue())
}, 15*time.Minute, 10*time.Second).Should(o.BeTrue())
}

func WaitForMCPsConfigSpecChangeAndUpdated(oc *exutil.CLI, workerInitialSpec, masterInitialSpec string) {
e2e.Logf("Waiting for worker and master pools to complete")
clientSet, err := machineconfigclient.NewForConfig(oc.KubeFramework().ClientConfig())
o.Expect(err).NotTo(o.HaveOccurred())

o.Eventually(func() bool {
workerMCP, err := clientSet.MachineconfigurationV1().MachineConfigPools().Get(context.TODO(), "worker", metav1.GetOptions{})
if err != nil {
return false
}
masterMCP, err := clientSet.MachineconfigurationV1().MachineConfigPools().Get(context.TODO(), "master", metav1.GetOptions{})
if err != nil {
return false
}

workerReady := workerMCP.Status.Configuration.Name != workerInitialSpec &&
workerMCP.Spec.Configuration.Name == workerMCP.Status.Configuration.Name &&
machineconfighelper.IsMachineConfigPoolConditionTrue(workerMCP.Status.Conditions, mcfgv1.MachineConfigPoolUpdated)

masterReady := masterMCP.Status.Configuration.Name != masterInitialSpec &&
masterMCP.Spec.Configuration.Name == masterMCP.Status.Configuration.Name &&
machineconfighelper.IsMachineConfigPoolConditionTrue(masterMCP.Status.Conditions, mcfgv1.MachineConfigPoolUpdated)

if !workerReady {
e2e.Logf("Worker MCP not ready yet")
}
if !masterReady {
e2e.Logf("Master MCP not ready yet")
}

return workerReady && masterReady
}, 15*time.Minute, 10*time.Second).Should(o.BeTrue())
e2e.Logf("Both worker and master pools completed successfully")
}

func isDisconnectedCluster(oc *exutil.CLI) bool {
Expand Down
9 changes: 3 additions & 6 deletions test/extended/node/criocredentialprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,7 @@ func updateCRIOCredentialProviderConfig(oc *exutil.CLI, matchImages []string, ex
return
}

imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
imagepolicy.WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func getWorkerNodes(oc *exutil.CLI) ([]corev1.Node, error) {
Expand Down Expand Up @@ -289,8 +288,7 @@ func createIDMSResources(oc *exutil.CLI) {

e2e.Logf("Created ImageDigestMirrorSet %q", idms.Name)

imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
imagepolicy.WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func cleanupIDMSResources(oc *exutil.CLI) {
Expand All @@ -302,8 +300,7 @@ func cleanupIDMSResources(oc *exutil.CLI) {

e2e.Logf("Deleted ImageDigestMirrorSet %q", "digest-mirror")

imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, workerPool, initialWorkerSpec)
imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, masterPool, initialMasterSpec)
imagepolicy.WaitForMCPsConfigSpecChangeAndUpdated(oc, initialWorkerSpec, initialMasterSpec)
}

func createNamespaceRBAC(f *e2e.Framework, namespace string) {
Expand Down
8 changes: 2 additions & 6 deletions test/extended/node/dra/example/example_dra.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
admissionapi "k8s.io/pod-security-admission/api"

dracommon "github.com/openshift/origin/test/extended/node/dra/common"
nodeutils "github.com/openshift/origin/test/extended/node"
exutil "github.com/openshift/origin/test/extended/util"
"github.com/openshift/origin/test/extended/util/image"
)
Expand All @@ -31,7 +32,7 @@ var (
prerequisitesError error
)

var _ = g.Describe("[sig-scheduling][Feature:DRA-Example][Suite:openshift/dra-example][Serial][Skipped:Disconnected]", func() {
var _ = g.Describe("[sig-scheduling][Feature:DRA-Example][Suite:openshift/dra-example][Serial][Skipped:Disconnected]", nodeutils.SkipOnMicroShift, func() {
defer g.GinkgoRecover()

oc := exutil.NewCLIWithPodSecurityLevel("dra-example", admissionapi.LevelPrivileged)
Expand All @@ -43,11 +44,6 @@ var _ = g.Describe("[sig-scheduling][Feature:DRA-Example][Suite:openshift/dra-ex
)

g.BeforeEach(func(ctx context.Context) {
isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient())
o.Expect(err).NotTo(o.HaveOccurred())
if isMicroShift {
g.Skip("Skipping DRA example driver tests on MicroShift cluster")
}

validator = NewDeviceValidator(oc.KubeFramework())
builder = dracommon.NewResourceBuilder(oc.Namespace(), dracommon.DriverConfig{
Expand Down
11 changes: 3 additions & 8 deletions test/extended/node/image_volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ var _ = describeImageVolumeTests(imageVolumeTestConfig{

// describeImageVolumeTests generates a full test suite for image or artifact volumes.
func describeImageVolumeTests(config imageVolumeTestConfig) bool {
return g.Describe("[sig-node] [FeatureGate:ImageVolume] "+config.describeLabel, func() {
return g.Describe("[sig-node] [FeatureGate:ImageVolume] "+config.describeLabel, SkipOnMicroShift, func() {
defer g.GinkgoRecover()

f := framework.NewDefaultFramework(config.frameworkName)
Expand All @@ -66,13 +66,8 @@ func describeImageVolumeTests(config imageVolumeTestConfig) bool {
podName = config.frameworkName + "-test"
)

g.BeforeEach(func() {
// Microshift doesn't inherit OCP feature gates, and ImageVolume won't work either
isMicroshift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient())
o.Expect(err).NotTo(o.HaveOccurred())
if isMicroshift {
g.Skip("Not supported on Microshift")
}
g.BeforeEach(func(ctx context.Context) {
EnsureNodesReady(ctx, oc)
})

g.It("should succeed with pod and pull policy of Always", func(ctx context.Context) {
Expand Down
41 changes: 8 additions & 33 deletions test/extended/node/kubelet_secret_pulled_images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const (
credVerifyPublicImage = internalRegistryPrefix + "/openshift/tools:latest"
)

var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptive][OCPFeatureGate:KubeletEnsureSecretPulledImages][Serial]", g.Ordered, func() {
var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptive][OCPFeatureGate:KubeletEnsureSecretPulledImages][Serial]", g.Ordered, SkipOnMicroShift, func() {
defer g.GinkgoRecover()

var (
Expand All @@ -46,12 +46,6 @@ var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptiv
// Setup: import a private image into the internal registry so all tests
// can use it without hardcoded credentials or external accounts.
g.BeforeAll(func() {
// Skip on MicroShift clusters
isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient())
o.Expect(err).NotTo(o.HaveOccurred())
if isMicroShift {
g.Skip("Skipping test on MicroShift cluster")
}

if !exutil.IsNoUpgradeFeatureSet(oc) {
g.Skip("requires TechPreviewNoUpgrade or CustomNoUpgrade feature set")
Expand Down Expand Up @@ -205,8 +199,8 @@ var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptiv
credVerifyCreateSecret(ctx, oc, ns, "pull-secret", pullSecret)

g.DeferCleanup(func() {
_ = deleteKC(oc, kcName)
_ = waitForMCP(ctx, mcClient, "worker", 30*time.Minute)
cleanupCtx := context.Background()
_ = CleanupKubeletConfig(cleanupCtx, mcClient, kcName, "worker")
})
Comment on lines 201 to 204

@coderabbitai coderabbitai Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Don't swallow rollback failures in this ordered cleanup.

This DeferCleanup drops both the deleteKC and waitForMCP errors, so Case 4 can leave the worker pool mid-rollout and still pass cleanup. In an g.Ordered suite, that leaks mutated cluster state into later cases.

Suggested fix 
 		g.DeferCleanup(func() {
-			_ = deleteKC(oc, kcName)
-			_ = waitForMCP(ctx, mcClient, "worker", 15*time.Minute)
+			if err := deleteKC(oc, kcName); err != nil {
+				framework.Failf("cleanup: failed to delete KubeletConfig %s: %v", kcName, err)
+			}
+			if err := waitForMCP(ctx, mcClient, "worker", 15*time.Minute); err != nil {
+				framework.Failf("cleanup: worker MCP did not recover after deleting %s: %v", kcName, err)
+			}
 		})

As per path instructions, **/*.go: Never ignore error returns.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
g.DeferCleanup(func() {
_ = deleteKC(oc, kcName)
_ = waitForMCP(ctx, mcClient, "worker", 30*time.Minute)
_ = waitForMCP(ctx, mcClient, "worker", 15*time.Minute)
})
g.DeferCleanup(func() {
if err := deleteKC(oc, kcName); err != nil {
framework.Failf("cleanup: failed to delete KubeletConfig %s: %v", kcName, err)
}
if err := waitForMCP(ctx, mcClient, "worker", 15*time.Minute); err != nil {
framework.Failf("cleanup: worker MCP did not recover after deleting %s: %v", kcName, err)
}
})
🤖 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/extended/node/kubelet_secret_pulled_images.go` around lines 207 - 210,
The ordered cleanup in the kubelet secret pulled images test is swallowing
rollback failures because both deleteKC and waitForMCP errors are ignored.
Update the DeferCleanup registered in the Case 4 setup to handle and surface
errors from deleteKC and waitForMCP instead of discarding them, so the g.Ordered
suite does not continue with a partially rolled back worker pool. Use the
existing cleanup block near the Case 4 setup to keep the cluster state
consistent for later cases.

Source: Path instructions


g.By("Pre-caching private image on the node with a valid secret")
Expand All @@ -215,7 +209,7 @@ var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptiv
g.By("Applying NeverVerify policy and waiting for MCO rollout")
credVerifyApplyPolicy(ctx, mcClient, kcName, `{"imagePullCredentialsVerificationPolicy":"NeverVerify"}`)
credVerifyWaitForMCPUpdating(ctx, mcClient, "worker")
err = waitForMCP(ctx, mcClient, "worker", 30*time.Minute)
err = WaitForMCP(ctx, mcClient, "worker", 15*time.Minute)
o.Expect(err).NotTo(o.HaveOccurred())

g.By("Verifying NeverVerify policy allows pod without secret to use cached image")
Expand All @@ -224,7 +218,7 @@ var _ = g.Describe("[sig-node][Suite:openshift/disruptive-longrunning][Disruptiv
g.By("Switching to AlwaysVerify policy and waiting for MCO rollout")
credVerifyApplyPolicy(ctx, mcClient, kcName, `{"imagePullCredentialsVerificationPolicy":"AlwaysVerify"}`)
credVerifyWaitForMCPUpdating(ctx, mcClient, "worker")
err = waitForMCP(ctx, mcClient, "worker", 30*time.Minute)
err = WaitForMCP(ctx, mcClient, "worker", 15*time.Minute)
o.Expect(err).NotTo(o.HaveOccurred())

// This pod also re-caches the image after MCO rollout since pull records are cleared
Expand Down Expand Up @@ -411,34 +405,15 @@ func credVerifyApplyPolicy(ctx context.Context, mcClient *mcclient.Clientset, na
},
}

existing, err := mcClient.MachineconfigurationV1().KubeletConfigs().Get(ctx, name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
_, err = mcClient.MachineconfigurationV1().KubeletConfigs().Create(ctx, kc, metav1.CreateOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
return
}
o.Expect(err).NotTo(o.HaveOccurred())
existing.Spec = kc.Spec
_, err = mcClient.MachineconfigurationV1().KubeletConfigs().Update(ctx, existing, metav1.UpdateOptions{})
_, err := CreateOrUpdateKubeletConfig(ctx, mcClient, kc)
o.Expect(err).NotTo(o.HaveOccurred())
}

// credVerifyWaitForMCPUpdating waits for the MCP to start updating, avoiding a race
// where waitForMCP returns immediately before the MCO picks up a KubeletConfig change.
func credVerifyWaitForMCPUpdating(ctx context.Context, mcClient *mcclient.Clientset, poolName string) {
o.Eventually(func() bool {
mcp, err := mcClient.MachineconfigurationV1().MachineConfigPools().Get(ctx, poolName, metav1.GetOptions{})
if err != nil {
e2e.Logf("Error getting MCP %s: %v", poolName, err)
return false
}
for _, condition := range mcp.Status.Conditions {
if condition.Type == "Updating" && condition.Status == corev1.ConditionTrue {
return true
}
}
return false
}, 2*time.Minute, 10*time.Second).Should(o.BeTrue(), fmt.Sprintf("MCP %s should start updating", poolName))
err := WaitForMCPUpdating(ctx, mcClient, poolName, 2*time.Minute)
o.Expect(err).NotTo(o.HaveOccurred(), "MCP %s should start updating", poolName)
}

func credVerifyPod(namespace, name, image, nodeName string, pullPolicy corev1.PullPolicy, secretName ...string) *corev1.Pod {
Expand Down
Loading