From 6359a7d0fef14acb7cdc3e19774ff6a7c50b3cab Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Wed, 21 Oct 2026 13:41:40 -0400 Subject: [PATCH 1/3] test(operator): cover the config-update reset clearing executors HandleConfigUpdates calls deleteConfigUpdateExecutors for every node it resets, but no spec exercised that wiring: dropping or moving the call would let a reset node finish on a Job still running the old ConfigMap without failing a test. Seed an unfinished config Job on a reset node and on a node the reset leaves alone, and assert only the first is cleared. Part of #776 Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- .../controller/skyhook_controller_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/operator/internal/controller/skyhook_controller_test.go b/operator/internal/controller/skyhook_controller_test.go index fa9c78d6..ef73538c 100644 --- a/operator/internal/controller/skyhook_controller_test.go +++ b/operator/internal/controller/skyhook_controller_test.go @@ -2991,6 +2991,59 @@ var _ = Describe("skyhook controller tests", func() { }) }) + // Dropping or moving the deleteConfigUpdateExecutors call would let a reset node finish on a + // Job still running the old ConfigMap, and no other spec would notice. + It("deletes an unfinished config Job on a reset node and leaves one on a node it does not reset", func() { + clusterState, skyhook, pkg, oldCM, newCM := seedConfigReset("cfg-reset-jobs", map[string]v1alpha1.NodeState{ + // the erroring node opens the gate, and is the one the reset clears + "cfg-reset-jobs-erroring": pkgState(v1alpha1.StateErroring, v1alpha1.StageConfig), + "cfg-reset-jobs-apply": pkgState(v1alpha1.StateInProgress, v1alpha1.StageApply), + }) + + seedJob := func(nodeName string) *batchv1.Job { + raw, err := json.Marshal(PackageSkyhook{PackageRef: pkg.PackageRef, Stage: v1alpha1.StageConfig}) + Expect(err).ToNot(HaveOccurred()) + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg-reset-job-" + nodeName, + Namespace: opts.Namespace, + Annotations: map[string]string{packageAnnotationKey: string(raw)}, + Labels: map[string]string{ + fmt.Sprintf("%s/name", v1alpha1.METADATA_PREFIX): skyhook.GetSkyhook().Name, + fmt.Sprintf("%s/package", v1alpha1.METADATA_PREFIX): fmt.Sprintf("%s-%s", pkg.Name, pkg.Version), + jobNodeLabel: nodeLabelValue(nodeName), + }, + }, + Spec: batchv1.JobSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + Containers: []corev1.Container{{Name: "c", Image: "busybox"}}, + }}}, + } + Expect(k8sClient.Create(ctx, job)).To(Succeed()) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, job) }) + Eventually(func() error { + return operator.Get(ctx, client.ObjectKeyFromObject(job), &batchv1.Job{}) + }).Should(Succeed()) + return job + } + resetJob := seedJob("cfg-reset-jobs-erroring") + keptJob := seedJob("cfg-reset-jobs-apply") + + updated, _, err := operator.HandleConfigUpdates(ctx, clusterState, skyhook, pkg, oldCM, newCM) + Expect(err).ToNot(HaveOccurred()) + Expect(updated).To(BeTrue()) + + // envtest runs no garbage collector, so a foreground delete leaves the Job with a + // deletionTimestamp rather than removing it. + deleting := func(job *batchv1.Job) bool { + var got batchv1.Job + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(job), &got) + return apierrors.IsNotFound(err) || (err == nil && got.DeletionTimestamp != nil) + } + Expect(deleting(resetJob)).To(BeTrue(), "the reset node's unfinished config Job is cleared") + Expect(deleting(keptJob)).To(BeFalse(), "a node the reset leaves alone keeps its Job") + }) + It("keeps the gate closed when no node is complete or erroring at config or later", func() { clusterState, skyhook, pkg, oldCM, newCM := seedConfigReset("cfg-reset-closed", map[string]v1alpha1.NodeState{ "cfg-reset-closed-apply": pkgState(v1alpha1.StateErroring, v1alpha1.StageApply), From a2d4e73674c8f6f357bb6694f4d4567d4f4cfeee Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Thu, 22 Oct 2026 13:40:34 -0400 Subject: [PATCH 2/3] test(controller): cover a non-erroring reset node in the config-reset Job cleanup spec Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- .../internal/controller/skyhook_controller_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/operator/internal/controller/skyhook_controller_test.go b/operator/internal/controller/skyhook_controller_test.go index ef73538c..bec88e2c 100644 --- a/operator/internal/controller/skyhook_controller_test.go +++ b/operator/internal/controller/skyhook_controller_test.go @@ -2993,11 +2993,13 @@ var _ = Describe("skyhook controller tests", func() { // Dropping or moving the deleteConfigUpdateExecutors call would let a reset node finish on a // Job still running the old ConfigMap, and no other spec would notice. - It("deletes an unfinished config Job on a reset node and leaves one on a node it does not reset", func() { + It("deletes an unfinished config Job on every reset node and leaves one on a node it does not reset", func() { clusterState, skyhook, pkg, oldCM, newCM := seedConfigReset("cfg-reset-jobs", map[string]v1alpha1.NodeState{ // the erroring node opens the gate, and is the one the reset clears "cfg-reset-jobs-erroring": pkgState(v1alpha1.StateErroring, v1alpha1.StageConfig), - "cfg-reset-jobs-apply": pkgState(v1alpha1.StateInProgress, v1alpha1.StageApply), + // resettable without erroring, so cleanup must not depend on the gate-opening node + "cfg-reset-jobs-interrupt": pkgState(v1alpha1.StateInProgress, v1alpha1.StageInterrupt), + "cfg-reset-jobs-apply": pkgState(v1alpha1.StateInProgress, v1alpha1.StageApply), }) seedJob := func(nodeName string) *batchv1.Job { @@ -3027,6 +3029,7 @@ var _ = Describe("skyhook controller tests", func() { return job } resetJob := seedJob("cfg-reset-jobs-erroring") + interruptJob := seedJob("cfg-reset-jobs-interrupt") keptJob := seedJob("cfg-reset-jobs-apply") updated, _, err := operator.HandleConfigUpdates(ctx, clusterState, skyhook, pkg, oldCM, newCM) @@ -3040,7 +3043,8 @@ var _ = Describe("skyhook controller tests", func() { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(job), &got) return apierrors.IsNotFound(err) || (err == nil && got.DeletionTimestamp != nil) } - Expect(deleting(resetJob)).To(BeTrue(), "the reset node's unfinished config Job is cleared") + Expect(deleting(resetJob)).To(BeTrue(), "the erroring reset node's unfinished config Job is cleared") + Expect(deleting(interruptJob)).To(BeTrue(), "a non-erroring reset node's unfinished config Job is cleared") Expect(deleting(keptJob)).To(BeFalse(), "a node the reset leaves alone keeps its Job") }) From 8d266f05c332a48477dba6d0fb4e3d7b40916478 Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Fri, 23 Oct 2026 13:02:37 -0400 Subject: [PATCH 3/3] test(controller): say that the interrupt node is reset too in the config-reset Job spec Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- operator/internal/controller/skyhook_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator/internal/controller/skyhook_controller_test.go b/operator/internal/controller/skyhook_controller_test.go index bec88e2c..777eabda 100644 --- a/operator/internal/controller/skyhook_controller_test.go +++ b/operator/internal/controller/skyhook_controller_test.go @@ -2995,7 +2995,7 @@ var _ = Describe("skyhook controller tests", func() { // Job still running the old ConfigMap, and no other spec would notice. It("deletes an unfinished config Job on every reset node and leaves one on a node it does not reset", func() { clusterState, skyhook, pkg, oldCM, newCM := seedConfigReset("cfg-reset-jobs", map[string]v1alpha1.NodeState{ - // the erroring node opens the gate, and is the one the reset clears + // the erroring node opens the gate; it and the interrupt node are both reset "cfg-reset-jobs-erroring": pkgState(v1alpha1.StateErroring, v1alpha1.StageConfig), // resettable without erroring, so cleanup must not depend on the gate-opening node "cfg-reset-jobs-interrupt": pkgState(v1alpha1.StateInProgress, v1alpha1.StageInterrupt),