From f43000a299ef426c4005f04c2cdbfa22c359a20a Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Mon, 20 Jan 2025 02:05:32 +0000 Subject: [PATCH 1/2] test: simply call Gomega's Equal to check if two PV(C)s are equal Signed-off-by: Ryotaro Banno --- .../mantlerestore_controller_test.go | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/internal/controller/mantlerestore_controller_test.go b/internal/controller/mantlerestore_controller_test.go index 4bb86abe..8d4e6e8f 100644 --- a/internal/controller/mantlerestore_controller_test.go +++ b/internal/controller/mantlerestore_controller_test.go @@ -177,11 +177,7 @@ func (test *mantleRestoreControllerUnitTest) testCreateRestoringPV() { err = k8sClient.Get(ctx, client.ObjectKey{Name: fmt.Sprintf("mr-%s-%s", test.tenantNamespace, restore.Name)}, &pv2) Expect(err).NotTo(HaveOccurred()) - pv1Bin, err := pv1.Marshal() - Expect(err).NotTo(HaveOccurred()) - pv2Bin, err := pv2.Marshal() - Expect(err).NotTo(HaveOccurred()) - Expect(pv1Bin).To(Equal(pv2Bin)) + Expect(pv1).To(Equal(pv2)) }) It("should return an error, if the PV already exists and having different RestoredBy annotation", func(ctx SpecContext) { @@ -196,11 +192,7 @@ func (test *mantleRestoreControllerUnitTest) testCreateRestoringPV() { err = k8sClient.Get(ctx, client.ObjectKey{Name: fmt.Sprintf("mr-%s-%s", test.tenantNamespace, restore.Name)}, &pv3) Expect(err).NotTo(HaveOccurred()) - pv1Bin, err := pv1.Marshal() - Expect(err).NotTo(HaveOccurred()) - pv3Bin, err := pv3.Marshal() - Expect(err).NotTo(HaveOccurred()) - Expect(pv1Bin).To(Equal(pv3Bin)) + Expect(pv1).To(Equal(pv3)) }) } @@ -241,11 +233,7 @@ func (test *mantleRestoreControllerUnitTest) testCreateRestoringPVC() { err = k8sClient.Get(ctx, client.ObjectKey{Name: restore.Name, Namespace: test.tenantNamespace}, &pvc2) Expect(err).NotTo(HaveOccurred()) - pvc1Bin, err := pvc1.Marshal() - Expect(err).NotTo(HaveOccurred()) - pvc2Bin, err := pvc2.Marshal() - Expect(err).NotTo(HaveOccurred()) - Expect(pvc1Bin).To(Equal(pvc2Bin)) + Expect(pvc1).To(Equal(pvc2)) }) It("should return an error, if the PVC already exists and having different RestoredBy annotation", func(ctx SpecContext) { @@ -260,11 +248,7 @@ func (test *mantleRestoreControllerUnitTest) testCreateRestoringPVC() { err = k8sClient.Get(ctx, client.ObjectKey{Name: restore.Name, Namespace: test.tenantNamespace}, &pvc3) Expect(err).NotTo(HaveOccurred()) - pvc1Bin, err := pvc1.Marshal() - Expect(err).NotTo(HaveOccurred()) - pvc3Bin, err := pvc3.Marshal() - Expect(err).NotTo(HaveOccurred()) - Expect(pvc1Bin).To(Equal(pvc3Bin)) + Expect(pvc1).To(Equal(pvc3)) }) } From 70c0f1af4ccd0c20f6cb0f3b8e7b8eda36055a64 Mon Sep 17 00:00:00 2001 From: Ryotaro Banno Date: Mon, 20 Jan 2025 02:06:43 +0000 Subject: [PATCH 2/2] restore: don't try to overwrite existing PVCs Signed-off-by: Ryotaro Banno --- .../controller/mantlerestore_controller.go | 9 +++- .../mantlerestore_controller_test.go | 42 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/internal/controller/mantlerestore_controller.go b/internal/controller/mantlerestore_controller.go index cad5a0f7..1527b0e2 100644 --- a/internal/controller/mantlerestore_controller.go +++ b/internal/controller/mantlerestore_controller.go @@ -311,8 +311,13 @@ func (r *MantleRestoreReconciler) createOrUpdateRestoringPVC(ctx context.Context if pvc.Annotations == nil { pvc.Annotations = map[string]string{} } - if annot, ok := pvc.Annotations[PVCAnnotationRestoredBy]; ok && annot != restoredBy { - return fmt.Errorf("the existing PVC is having different MantleRestore UID: %s, %s", + annotatedRestoredBy, ok := pvc.Annotations[PVCAnnotationRestoredBy] + if !pvc.CreationTimestamp.IsZero() && !ok { + return fmt.Errorf("the existing PVC was not created by any mantle-controller: %s/%s", + pvcNamespace, pvcName) + } + if ok && annotatedRestoredBy != restoredBy { + return fmt.Errorf("the existing PVC was created by different mantle-controller: %s, %s", pvcName, pvc.Annotations[PVCAnnotationRestoredBy]) } pvc.Annotations[PVCAnnotationRestoredBy] = restoredBy diff --git a/internal/controller/mantlerestore_controller_test.go b/internal/controller/mantlerestore_controller_test.go index 8d4e6e8f..72eb6079 100644 --- a/internal/controller/mantlerestore_controller_test.go +++ b/internal/controller/mantlerestore_controller_test.go @@ -204,6 +204,48 @@ func (test *mantleRestoreControllerUnitTest) testCreateRestoringPVC() { restore = test.restoreResource() }) + It("should return an error if the PVC already exists and has no RestoredBy annotation", func(ctx SpecContext) { + err := k8sClient.Create(ctx, &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: restore.Name, Namespace: test.tenantNamespace}, + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, + Resources: corev1.VolumeResourceRequirements{ + Requests: test.srcPVC.Spec.Resources.Requests, + }, + StorageClassName: test.srcPVC.Spec.StorageClassName, + + // We intentionally leave volumeName unset here. Setting it + // would cause createOrUpdate to fail since volumeName is + // immutable. Our goal is to test that createOrUpdate isn't + // applied to this PVC, so we keep it empty. + }, + }) + Expect(err).NotTo(HaveOccurred()) + + var pvc corev1.PersistentVolumeClaim + err = k8sClient.Get(ctx, client.ObjectKey{Name: restore.Name, Namespace: test.tenantNamespace}, &pvc) + Expect(err).NotTo(HaveOccurred()) + + err = test.reconciler.createOrUpdateRestoringPVC(ctx, restore, test.backup) + Expect(err).To(HaveOccurred()) + + By("PVC should not be updated") + var updatedPVC corev1.PersistentVolumeClaim + err = k8sClient.Get(ctx, client.ObjectKey{Name: restore.Name, Namespace: test.tenantNamespace}, &updatedPVC) + Expect(err).NotTo(HaveOccurred()) + + Expect(pvc).To(Equal(updatedPVC)) + + // delete pvc0 for the following tests + pvc.ObjectMeta.Finalizers = []string{} + err = k8sClient.Update(ctx, &pvc) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Delete(ctx, &pvc) + Expect(err).NotTo(HaveOccurred()) + err = k8sClient.Get(ctx, client.ObjectKey{Name: restore.Name, Namespace: test.tenantNamespace}, &updatedPVC) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) + It("should create a correct PVC", func(ctx SpecContext) { err := test.reconciler.createOrUpdateRestoringPVC(ctx, restore, test.backup) Expect(err).NotTo(HaveOccurred())