fix

cybozu-go · Mar 8, 2024 · 663513c · 663513c
1 parent 5612e01
commit 663513c
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 47 deletions.
diff --git a/e2e/suite_test.go b/e2e/suite_test.go
@@ -33,14 +33,13 @@ var (
 )
 
 const (
-	pvcName            = "rbd-pvc"
-	pvcName2           = "rbd-pvc-2"
-	nonExistentPvcName = "non-existent-pvc"
-	poolName           = "replicapool"
-	rbdPVCBackupName   = "rbdpvcbackup-test"
-	rbdPVCBackupName2  = "rbdpvcbackup-test-2"
-	rbdPVCBackupName3  = "rbdpvcbackup-test-3"
-	namespace          = "rook-ceph"
+	pvcName           = "rbd-pvc"
+	pvcName2          = "rbd-pvc-2"
+	poolName          = "replicapool"
+	rbdPVCBackupName  = "rbdpvcbackup-test"
+	rbdPVCBackupName2 = "rbdpvcbackup-test-2"
+	rbdPVCBackupName3 = "rbdpvcbackup-test-3"
+	namespace         = "rook-ceph"
 )
 
 func execAtLocal(cmd string, input []byte, args ...string) ([]byte, []byte, error) {
@@ -417,28 +416,4 @@ var _ = Describe("rbd backup system", func() {
 			return fmt.Errorf("RBDPVCBackup resource %s still exists. stdout: %s", rbdPVCBackupName, stdout)
 		}).Should(Succeed())
 	})
-
-	It("should not succeed RBDPVCBackup creation if specified non-existent PVC name", func() {
-		By("Creating an RBDPVCBackup specified the non-existent PVC name")
-		manifest := fmt.Sprintf(testRBDPVCBackupTemplate, rbdPVCBackupName, rbdPVCBackupName, namespace, nonExistentPvcName)
-		_, _, err := kubectlWithInput([]byte(manifest), "apply", "-f", "-")
-		Expect(err).NotTo(HaveOccurred())
-
-		By("Waiting for the conditions of the RBDPVCBackup resource to become \"Failed\"")
-		Eventually(func() error {
-			stdout, stderr, err := kubectl("-n", namespace, "get", "rbdpvcbackup", rbdPVCBackupName, "-ojson")
-			if err != nil {
-				return fmt.Errorf("get rbdpvcbackup %s failed. stderr: %s, err: %w", rbdPVCBackupName, string(stderr), err)
-			}
-			var backup backupv1.RBDPVCBackup
-			err = yaml.Unmarshal(stdout, &backup)
-			if err != nil {
-				return err
-			}
-			if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsFailed {
-				return fmt.Errorf("conditions of RBDPVCBackup is not \"Failed\" yet. status.condigions: %s", backup.Status.Conditions)
-			}
-			return nil
-		}).Should(Succeed())
-	})
 })
diff --git a/internal/controller/rbdpvcbackup_controller.go b/internal/controller/rbdpvcbackup_controller.go
@@ -131,10 +131,6 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	err = r.Get(ctx, types.NamespacedName{Namespace: pvcNamespace, Name: pvcName}, &pvc)
 	if err != nil {
 		logger.Error("failed to get PVC", "namespace", pvcNamespace, "name", pvcName, "error", err)
-		err2 := r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsFailed)
-		if err2 != nil {
-			return ctrl.Result{}, err2
-		}
 		return ctrl.Result{}, err
 	}
 	if pvc.Status.Phase != corev1.ClaimBound {
@@ -143,10 +139,6 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 			return ctrl.Result{Requeue: true}, nil
 		} else {
 			logger.Error("failed to bound PVC", "status.phase", pvc.Status.Phase)
-			err = r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsFailed)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
 			return ctrl.Result{}, fmt.Errorf("failed to bound PVC (status.phase: %s)", pvc.Status.Phase)
 		}
 
@@ -157,10 +149,6 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	err = r.Get(ctx, types.NamespacedName{Namespace: req.NamespacedName.Namespace, Name: pvName}, &pv)
 	if err != nil {
 		logger.Error("failed to get PV", "namespace", req.NamespacedName.Namespace, "name", pvName, "error", err)
-		err2 := r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsFailed)
-		if err2 != nil {
-			return ctrl.Result{}, err2
-		}
 		return ctrl.Result{}, err
 	}
 
@@ -231,6 +219,10 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		out, err := executeCommand(command, nil)
 		if err != nil {
 			logger.Info("failed to run `rbd snap ls`", "poolName", poolName, "imageName", imageName, "error", err)
+			err2 := r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsFailed)
+			if err2 != nil {
+				return ctrl.Result{}, err2
+			}
 			return ctrl.Result{Requeue: true}, nil
 		}
 		var snapshots []Snapshot
@@ -252,6 +244,10 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		}
 		if !existSnapshot {
 			logger.Info("snapshot not exists", "snapshotName", backup.Name)
+			err2 := r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsFailed)
+			if err2 != nil {
+				return ctrl.Result{}, err2
+			}
 			return ctrl.Result{Requeue: true}, nil
 		}
 	}

diff --git a/internal/controller/rbdpvcbackup_controller_test.go b/internal/controller/rbdpvcbackup_controller_test.go
@@ -357,7 +357,7 @@ var _ = Describe("RBDPVCBackup controller", func() {
 		}).Should(Succeed())
 	})
 
-	It("should be \"Failed\" conditions in RBDPVCBackup resource if the status.phase of the PVC is in the lost state from the beginning", func() {
+	It("should not be \"Bound\" conditions in RBDPVCBackup resource if the status.phase of the PVC is in the lost state from the beginning", func() {
 		ctx := context.Background()
 		ns := createNamespace()
 
@@ -434,8 +434,42 @@ var _ = Describe("RBDPVCBackup controller", func() {
 				return err
 			}
 
-			if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsFailed {
-				return fmt.Errorf("status.conditions does not set \"Failed\" yet (status.conditions: %s)", backup.Status.Conditions)
+			if backup.Status.Conditions == backupv1.RBDPVCBackupConditionsBound {
+				return fmt.Errorf("status.conditions should not be \"Bound\"")
+			}
+
+			return nil
+		}).Should(Succeed())
+	})
+
+	It("should not be \"Bound\" conditions in RBDPVCBackup resource if specified non-existent PVC name", func() {
+		ctx := context.Background()
+		ns := createNamespace()
+
+		backup := backupv1.RBDPVCBackup{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "backup",
+				Namespace: ns,
+			},
+			Spec: backupv1.RBDPVCBackupSpec{
+				PVC: "non-existent-pvc",
+			},
+		}
+		err := k8sClient.Create(ctx, &backup)
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(func() error {
+			namespacedName := types.NamespacedName{
+				Namespace: ns,
+				Name:      backup.Name,
+			}
+			err = k8sClient.Get(ctx, namespacedName, &backup)
+			if err != nil {
+				return err
+			}
+
+			if backup.Status.Conditions == backupv1.RBDPVCBackupConditionsBound {
+				return fmt.Errorf("status.conditions should not be \"Bound\"")
 			}
 
 			return nil