diff --git a/internal/controller/applicationdisruptionbudget_controller_test.go b/internal/controller/applicationdisruptionbudget_controller_test.go index 8153fb9..66fa16d 100644 --- 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 index 3133061..ed1ea1e 100644 --- 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 index c3cfdf4..a914b7c 100644 --- 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{}