From 7f2fa15180823ec7b83cca7578d8b576e45916fb Mon Sep 17 00:00:00 2001 From: "d.amsallem" Date: Thu, 23 Jul 2026 11:13:59 +0200 Subject: [PATCH] nodedisruption: reject instead of pending when preparing past deadline * When an app was in "preparing" state and the retryable ND reach the deadline, we would never call the next step to the app since deadline is reached (setting "AnyFailed" at true). * It was making the ND to be never deleted, because it would never reached a final state (granted or rejected). * Here the goals is to force reject (like before the revamp of the API) if the deadline is reached. * NB: /terminate is out of the state-machine, so it will be called at deletion of the ND, even if the deadline is reached ! --- .../controller/nodedisruption_controller.go | 5 +- .../nodedisruption_controller_test.go | 133 ++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/internal/controller/nodedisruption_controller.go b/internal/controller/nodedisruption_controller.go index 9b291c5..4bf4d23 100644 --- a/internal/controller/nodedisruption_controller.go +++ b/internal/controller/nodedisruption_controller.go @@ -325,7 +325,10 @@ func (ndr *SingleNodeDisruptionReconciler) tryTransitionToGranted(ctx context.Co logger.Info("Disruption rejected", "status", status) } } - if anyPreparing >= 0 { + // Even if application are preparing themselve for disruption, if the deadline has been reached + // We need to force Reject the maintenance for Tacamo to delete the ND and triggers the /terminate endpoint. + // Otherwise if an application keep staying in preparing stage, ND will never end and never be deleted. + if anyPreparing >= 0 && !ndr.NodeDisruption.Spec.Retry.IsAfterDeadline() { state = nodedisruptionv1alpha1.Pending } else { state = nodedisruptionv1alpha1.Rejected diff --git a/internal/controller/nodedisruption_controller_test.go b/internal/controller/nodedisruption_controller_test.go index 3044b77..738ebbc 100644 --- a/internal/controller/nodedisruption_controller_test.go +++ b/internal/controller/nodedisruption_controller_test.go @@ -860,6 +860,139 @@ var _ = Describe("NodeDisruption controller", func() { }) }) + When("an application is preparing and the deadline is reached", func() { + It("rejects the node disruption instead of keeping it pending", func() { + mockBasePath := "/api/v2" + + By("Starting an http server where prepare succeeds but the app never becomes ready") + prepareCalledCnt := 0 + readyCalledCnt := 0 + terminateCalledCnt := 0 + hookFn := func(w http.ResponseWriter, req *http.Request) { + switch req.URL.Path { + case mockBasePath + "/prepare": + prepareCalledCnt++ + w.WriteHeader(http.StatusOK) + case mockBasePath + "/ready": + readyCalledCnt++ + // Application is still preparing: ask the controller to retry later. + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusTooEarly) + case mockBasePath + "/terminate": + terminateCalledCnt++ + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusOK) + } + } + mockServer := startDummyHTTPServer(hookFn) + defer mockServer.Close() + + By("creating a budget that accepts one disruption with a v2 hook") + adb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "nodedisruption.criteo.com/v1alpha1", + Kind: "ApplicationDisruptionBudget", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Spec: nodedisruptionv1alpha1.ApplicationDisruptionBudgetSpec{ + PodSelector: metav1.LabelSelector{MatchLabels: podLabels}, + MaxDisruptions: 1, + HookV2BasePath: nodedisruptionv1alpha1.HookSpec{ + URL: mockServer.URL + mockBasePath, + }, + }, + } + Expect(k8sClient.Create(ctx, adb)).Should(Succeed()) + + By("checking the ApplicationDisruptionBudget is synchronized") + ADBLookupKey := types.NamespacedName{Name: "test", Namespace: "default"} + createdADB := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{} + Eventually(func() []string { + err := k8sClient.Get(ctx, ADBLookupKey, createdADB) + Expect(err).Should(Succeed()) + return createdADB.Status.WatchedNodes + }, timeout, interval).Should(Equal([]string{"node1"})) + + By("creating a new NodeDisruption with retry enabled and a deadline in the future") + disruption := &nodedisruptionv1alpha1.NodeDisruption{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "nodedisruption.criteo.com/v1alpha1", + Kind: "NodeDisruption", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: NDName, + Namespace: NDNamespace, + }, + Spec: nodedisruptionv1alpha1.NodeDisruptionSpec{ + NodeSelector: metav1.LabelSelector{MatchLabels: nodeLabels1}, + Type: "maintenance", + Retry: nodedisruptionv1alpha1.RetrySpec{ + Enabled: true, + // Deadline is 1m in the future + Deadline: metav1.Time{Time: time.Now().Add(time.Minute)}, + }, + }, + } + Expect(k8sClient.Create(ctx, disruption.DeepCopy())).Should(Succeed()) + + By("staying Pending while the application is preparing and the deadline is not reached") + createdDisruption := &nodedisruptionv1alpha1.NodeDisruption{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, NDLookupKey, createdDisruption)).Should(Succeed()) + g.Expect(createdDisruption.Status.State).To(Equal(nodedisruptionv1alpha1.Pending)) + g.Expect(createdDisruption.Status.NextRetryDate.IsZero()).To(BeFalse()) + g.Expect(createdDisruption.Status.DisruptedDisruptionBudgets).To(HaveLen(1)) + g.Expect(createdDisruption.Status.DisruptedDisruptionBudgets[0].Preparing).To(BeTrue()) + g.Expect(createdDisruption.Status.DisruptedDisruptionBudgets[0].Ok).To(BeFalse()) + }, timeout, interval).Should(Succeed()) + Expect(prepareCalledCnt).Should(BeNumerically(">=", 1)) + + By("polling the ready hook on retry while the app stays not-ready and the disruption stays Pending") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, NDLookupKey, createdDisruption)).Should(Succeed()) + g.Expect(readyCalledCnt).To(BeNumerically(">=", 1)) + g.Expect(createdDisruption.Status.State).To(Equal(nodedisruptionv1alpha1.Pending)) + }, timeout, interval).Should(Succeed()) + + By("forcing the deadline to be reached while the application is still preparing") + Expect(k8sClient.Get(ctx, NDLookupKey, createdDisruption)).Should(Succeed()) + createdDisruption.Spec.Retry.Deadline = metav1.Time{Time: time.Now().Add(-time.Minute)} + Expect(k8sClient.Update(ctx, createdDisruption.DeepCopy())).Should(Succeed()) + + By("switching to Rejected instead of staying Pending forever") + Eventually(func() nodedisruptionv1alpha1.NodeDisruptionState { + err := k8sClient.Get(ctx, NDLookupKey, createdDisruption) + if err != nil { + panic("should be able to get") + } + return createdDisruption.Status.State + }, timeout, interval).Should(Equal(nodedisruptionv1alpha1.Rejected)) + + By("keeping the budget marked as preparing so the app can still be terminated") + Expect(createdDisruption.Status.DisruptedDisruptionBudgets).Should(ContainElement(And( + HaveField("Reference.Kind", "ApplicationDisruptionBudget"), + HaveField("Preparing", true), + ))) + + By("still calling the terminate hook on deletion even though the deadline was reached") + Expect(terminateCalledCnt).Should(Equal(0)) + Expect(k8sClient.Delete(ctx, createdDisruption.DeepCopy())).Should(Succeed()) + Eventually(func() int { + return terminateCalledCnt + }, timeout, interval).Should(BeNumerically(">=", 1)) + + By("removing the NodeDisruption after terminate propagation") + Eventually(func() bool { + err := k8sClient.Get(ctx, NDLookupKey, createdDisruption) + return errorsk8s.IsNotFound(err) + }, timeout, interval).Should(BeTrue()) + }) + }) + When("there is an ADB that does not support the node disruption type", func() { It("ignores the budget and grants the disruption", func() { By("creating a budget that rejects everything but only supports maintenance1 type")