Improve PVC phase handling (criteo#70)

Now: - ignore PVC that are pending - raise a specific error if the phase is not expected Since Status is a subresource, it has to be updated directly in the tests
geobeau · Aug 12, 2024 · bf78b8e · bf78b8e
1 parent 1376a1d
commit bf78b8e
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 8 deletions.
diff --git a/internal/controller/applicationdisruptionbudget_controller_test.go b/internal/controller/applicationdisruptionbudget_controller_test.go
@@ -118,7 +118,7 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 					return createdADB.Status.WatchedNodes
 				}, timeout, interval).Should(Equal([]string{"node1"}))
 
-				By("Adding Pods")
+				By("Adding more Pods")
 				pod2 := newPod("podadb2", ADBNamespace, "node2", podLabels)
 				Expect(k8sClient.Create(ctx, &pod2)).Should(Succeed())
 
@@ -129,9 +129,10 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 					return createdADB.Status.WatchedNodes
 				}, timeout, interval).Should(Equal([]string{"node1", "node2"}))
 
-				By("Adding PVCs")
+				By("Adding a new PVC")
 				pvc3 := newPVC("pvc3", ADBNamespace, "node3-pv-local", podLabels)
-				Expect(k8sClient.Create(ctx, &pvc3)).Should(Succeed())
+				Expect(k8sClient.Create(ctx, pvc3.DeepCopy())).Should(Succeed())
+				Expect(k8sClient.Status().Update(ctx, pvc3.DeepCopy())).Should(Succeed())
 
 				By("checking the ApplicationDisruptionBudget updated the status")
 				Eventually(func() []string {
@@ -153,7 +154,8 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 				pod3 := newPod("podadb3", ADBNamespace, "", podLabels)
 				Expect(k8sClient.Create(ctx, &pod3)).Should(Succeed())
 				pvc3 := newPVC("pvc3", ADBNamespace, "node3-pv-local", podLabels)
-				Expect(k8sClient.Create(ctx, &pvc3)).Should(Succeed())
+				Expect(k8sClient.Create(ctx, pvc3.DeepCopy())).Should(Succeed())
+				Expect(k8sClient.Status().Update(ctx, pvc3.DeepCopy())).Should(Succeed())
 
 				By("creating a budget that accepts one disruption")
 				ndb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{
@@ -232,7 +234,8 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 
 				By("Adding PVC")
 				pvc := newPVC("pvc", ADBNamespace, "remote-pv", podLabels)
-				Expect(k8sClient.Create(ctx, &pvc)).Should(Succeed())
+				Expect(k8sClient.Create(ctx, pvc.DeepCopy())).Should(Succeed())
+				Expect(k8sClient.Status().Update(ctx, pvc.DeepCopy())).Should(Succeed())
 
 				By("creating a budget that accepts one disruption")
 				adb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{
@@ -265,6 +268,45 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 				Expect(adb.Status.WatchedNodes).Should(BeEmpty())
 			})
 		})
-	})
 
+		When("PVC is pending", func() {
+			It("is ignored by ADB", func() {
+				By("Adding PVC")
+				pvc := newPVC("pvc", ADBNamespace, "remote-pv", podLabels)
+				pvc.Status.Phase = corev1.ClaimPending
+				Expect(k8sClient.Create(ctx, pvc.DeepCopy())).Should(Succeed())
+				Expect(k8sClient.Status().Update(ctx, pvc.DeepCopy())).Should(Succeed())
+
+				By("creating a budget that accepts one disruption")
+				adb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{
+					TypeMeta: metav1.TypeMeta{
+						APIVersion: "nodedisruption.criteo.com/v1alpha1",
+						Kind:       "ApplicationDisruptionBudget",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      ADBname,
+						Namespace: ADBNamespace,
+					},
+					Spec: nodedisruptionv1alpha1.ApplicationDisruptionBudgetSpec{
+						PodSelector:    metav1.LabelSelector{MatchLabels: podLabels},
+						PVCSelector:    metav1.LabelSelector{MatchLabels: podLabels},
+						MaxDisruptions: 1,
+					},
+				}
+				Expect(k8sClient.Create(ctx, adb)).Should(Succeed())
+
+				By("checking the ApplicationDisruptionBudget updated the status and has seen no nodes")
+				Eventually(func() int {
+					err := k8sClient.Get(ctx, types.NamespacedName{
+						Namespace: ADBNamespace,
+						Name:      ADBname,
+					}, adb)
+					Expect(err).Should(Succeed())
+					return adb.Status.DisruptionsAllowed
+				}, timeout, interval).Should(Equal(1))
+
+				Expect(adb.Status.WatchedNodes).Should(BeEmpty())
+			})
+		})
+	})
 })
diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go
@@ -87,7 +87,9 @@ func newPVC(name, namespace, pvName string, labels map[string]string) corev1.Per
 			AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
 			Resources:   corev1.ResourceRequirements{Requests: resources},
 		},
-		Status: corev1.PersistentVolumeClaimStatus{},
+		Status: corev1.PersistentVolumeClaimStatus{
+			Phase: corev1.ClaimBound,
+		},
 	}
 }
 

diff --git a/pkg/resolver/resolver.go b/pkg/resolver/resolver.go
@@ -165,7 +165,19 @@ func (r *Resolver) GetNodesFromNamespacedPVCSelector(ctx context.Context, pvcSel
 	PVsToFetch := []string{}
 
 	for _, pvc := range PVCs.Items {
-		PVsToFetch = append(PVsToFetch, pvc.Spec.VolumeName)
+
+		logger.Info("PVC status", "pvc_name", pvc.Name, "pvc_phase", pvc.Status.Phase)
+		if pvc.Status.Phase == corev1.ClaimBound {
+			PVsToFetch = append(PVsToFetch, pvc.Spec.VolumeName)
+		} else if pvc.Status.Phase == corev1.ClaimLost {
+			return nodeSet, fmt.Errorf("PVC %s is marked lost. It is unsafe to grant disruption in this state", pvc.Name)
+		} else if pvc.Status.Phase == corev1.ClaimPending {
+			// Pending means that no nodes around bound yet so no need to fetch a non existing pv
+			logger.Info("Skipping PVC because it is pending", "pvc_name", pvc.Name, "pvc_phase", pvc.Status.Phase)
+			continue
+		} else {
+			return nodeSet, fmt.Errorf("PVC %s is unexpected phase (%s). It is unsafe to grant disruption in this state", pvc.Name, pvc.Status.Phase)
+		}
 	}
 
 	getOptions := []client.GetOption{}