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