Improve deadline exceed message

Deadline exceeded is what the status of a node disruption will show once a Node Disruption is fully rejected. Most of the time, we reach the deadline because some budgets are preventing from reaching the granted state. In the previous version, we lost the context and had to look at logs. Now we keep the previous status and only append a reason. This way we know why the last attempt was rejected.
geobeau · Jun 7, 2024 · 9468fd4 · 9468fd4
1 parent aaf4722
commit 9468fd4
Show file tree

Hide file tree

Showing 2 changed files with 111 additions and 1 deletion.
diff --git a/internal/controller/nodedisruption_controller.go b/internal/controller/nodedisruption_controller.go
@@ -281,6 +281,19 @@ func (ndr *SingleNodeDisruptionReconciler) generateRejectedStatus(reason string)
 	}
 }
 
+// appendRejectedStatus add a new status on top of the existing ones
+func (ndr *SingleNodeDisruptionReconciler) appendRejectedStatus(reason string) []nodedisruptionv1alpha1.DisruptedBudgetStatus {
+	return append(ndr.NodeDisruption.Status.DisruptedDisruptionBudgets, nodedisruptionv1alpha1.DisruptedBudgetStatus{
+		Reference: nodedisruptionv1alpha1.NamespacedName{
+			Namespace: ndr.NodeDisruption.Namespace,
+			Name:      ndr.NodeDisruption.Name,
+			Kind:      ndr.NodeDisruption.Kind,
+		},
+		Reason: reason,
+		Ok:     false,
+	})
+}
+
 // ValidateOverlappingDisruption checks that the current disruption doesn't overlap and existing one
 func (ndr *SingleNodeDisruptionReconciler) ValidateOverlappingDisruption(ctx context.Context, disruptedNodes resolver.NodeSet) (anyFailed bool, statuses []nodedisruptionv1alpha1.DisruptedBudgetStatus, err error) {
 	allDisruptions := &nodedisruptionv1alpha1.NodeDisruptionList{}
@@ -322,7 +335,8 @@ func (ndr *SingleNodeDisruptionReconciler) ValidateWithInternalConstraints(ctx c
 	}
 
 	if ndr.NodeDisruption.Spec.Retry.IsAfterDeadline() {
-		return true, ndr.generateRejectedStatus("Deadline exceeded"), nil
+		// Conserve the statuses of the previous iteration to conserve the reason of rejection
+		return true, ndr.appendRejectedStatus("Deadline exceeded"), nil
 	}
 
 	return false, statuses, nil

diff --git a/internal/controller/nodedisruption_controller_test.go b/internal/controller/nodedisruption_controller_test.go
@@ -472,6 +472,102 @@ var _ = Describe("NodeDisruption controller", func() {
 					}, timeout, interval).Should(Equal(nodedisruptionv1alpha1.Rejected))
 				})
 			})
+
+			When("a node disruption's deadline is elapsed", func() {
+				It("It keeps the previous status", func() {
+					By("creating a budget that rejects everything")
+					ndb := &nodedisruptionv1alpha1.NodeDisruptionBudget{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "nodedisruption.criteo.com/v1alpha1",
+							Kind:       "NodeDisruptionBudget",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test",
+						},
+						Spec: nodedisruptionv1alpha1.NodeDisruptionBudgetSpec{
+							NodeSelector:      metav1.LabelSelector{MatchLabels: nodeLabels1},
+							MaxDisruptedNodes: 0,
+						},
+					}
+					Expect(k8sClient.Create(ctx, ndb)).Should(Succeed())
+
+					By("creating a new NodeDisruption with retry enabled and an far deadline")
+					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},
+							Retry: nodedisruptionv1alpha1.RetrySpec{
+								Enabled: true,
+								// Deadline is 1m in future
+								Deadline: metav1.Time{Time: time.Now().Add(time.Minute)},
+							},
+						},
+					}
+					Expect(k8sClient.Create(ctx, disruption.DeepCopy())).Should(Succeed())
+
+					By("trying at least one and filling the status field")
+					Eventually(func() bool {
+						err := k8sClient.Get(ctx, NDLookupKey, disruption)
+						if err != nil {
+							panic("should be able to get")
+						}
+						return !disruption.Status.NextRetryDate.IsZero()
+					}, timeout, interval).Should(BeTrue())
+
+					By("properly filling the status")
+					expectedStatus := []nodedisruptionv1alpha1.DisruptedBudgetStatus{{
+						Reference: nodedisruptionv1alpha1.NamespacedName{
+							Namespace: "",
+							Name:      "test",
+							Kind:      "NodeDisruptionBudget",
+						},
+						Reason: "No more disruption allowed",
+						Ok:     false,
+					}}
+					Expect(disruption.Status.DisruptedDisruptionBudgets).Should(Equal(expectedStatus))
+
+					By("forcing deadline exceeding")
+					disruption.Spec.Retry.Deadline = metav1.Time{Time: time.Now().Add(-time.Minute)}
+					Expect(k8sClient.Update(ctx, disruption.DeepCopy())).Should(Succeed())
+
+					By("waiting for rejection")
+					Eventually(func() nodedisruptionv1alpha1.NodeDisruptionState {
+						err := k8sClient.Get(ctx, NDLookupKey, disruption)
+						if err != nil {
+							panic("should be able to get")
+						}
+						return disruption.Status.State
+					}, timeout, interval).Should(Equal(nodedisruptionv1alpha1.Rejected))
+
+					By("keeping previous status")
+					expectedStatus = []nodedisruptionv1alpha1.DisruptedBudgetStatus{{
+						Reference: nodedisruptionv1alpha1.NamespacedName{
+							Namespace: "",
+							Name:      "test",
+							Kind:      "NodeDisruptionBudget",
+						},
+						Reason: "No more disruption allowed",
+						Ok:     false,
+					}, {
+						Reference: nodedisruptionv1alpha1.NamespacedName{
+							Namespace: "",
+							Name:      "test-nodedisruption",
+							Kind:      "NodeDisruption",
+						},
+						Reason: "Deadline exceeded",
+						Ok:     false,
+					}}
+					Expect(disruption.Status.DisruptedDisruptionBudgets).Should(Equal(expectedStatus))
+				})
+			})
+
 		})
 
 		Context("If a NodeDisruption's nodeSelector does not match any node", func() {