Merge pull request #1611 from tesshuflower/restic_cache_volume_name_conflict

fix restic cache pvc name collisions for src & dst

Author: openshift-merge-bot[bot]

CHANGELOG.md

@@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Restic updated to v0.18.0
 - Syncthing updated to v1.29.3
 
+### Fixed
+
+- Fix restic cache PVC name collision if replicationsource and
+  replicationdestination have the same name and are in the same
+  namespace
+
 ## [v0.12.1]
 
 ### Security

controllers/mover/restic/mover.go

@@ -37,6 +37,7 @@ import (
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	volsyncv1alpha1 "github.com/backube/volsync/api/v1alpha1"
 	vserrors "github.com/backube/volsync/controllers/errors"
@@ -214,12 +215,51 @@ func (m *Mover) ensureCache(ctx context.Context,
 		return nil, err
 	}
 
+	err = m.migrationCleanupObsoleteCachePVC(ctx)
+	if err != nil {
+		return nil, err
+	}
+
 	// Allocate cache volume
-	cacheName := mover.VolSyncPrefix + m.owner.GetName() + "-cache"
+	dir := "src"
+	if !m.isSource {
+		dir = "dst"
+	}
+	cacheName := mover.VolSyncPrefix + dir + "-" + m.owner.GetName() + "-cache"
 	m.logger.Info("allocating cache volume", "PVC", cacheName, "isTemporary", isTemporary)
 	return cacheVh.EnsureNewPVC(ctx, m.logger, cacheName, isTemporary)
 }
 
+// Old names for the cache PVC were common between source & dest - our new name will include -src or -dst
+// to avoid collisions - migrate by cleanup old cache PVC if it exists
+// (see issue: https://github.com/backube/volsync/issues/1575)
+func (m *Mover) migrationCleanupObsoleteCachePVC(ctx context.Context) error {
+	obsCachePVC := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      mover.VolSyncPrefix + m.owner.GetName() + "-cache",
+			Namespace: m.owner.GetNamespace(),
+		},
+	}
+	err := m.client.Get(ctx, client.ObjectKeyFromObject(obsCachePVC), obsCachePVC)
+	if err == nil {
+		// the cache PVC (with old obsolete name) still exists, remove it but only if it is owned by this CR
+		isOwnedByUs, err := ctrlutil.HasOwnerReference(obsCachePVC.GetOwnerReferences(), m.owner, m.client.Scheme())
+		if err != nil {
+			return err
+		}
+		if !isOwnedByUs {
+			// Do not delete, not owned by this VolSync CR
+			return nil
+		}
+
+		m.logger.Info("deleting old cache volume, a new one will be created", "old cachePVC", obsCachePVC.GetName())
+		_ = m.client.Delete(ctx, obsCachePVC, client.PropagationPolicy(metav1.DeletePropagationBackground))
+		return nil
+	}
+
+	return client.IgnoreNotFound(err)
+}
+
 func (m *Mover) ensureSourcePVC(ctx context.Context) (*corev1.PersistentVolumeClaim, error) {
 	srcPVC := &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{

controllers/mover/restic/restic_test.go

@@ -40,11 +40,12 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/events"
 	"k8s.io/utils/ptr"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	volsyncv1alpha1 "github.com/backube/volsync/api/v1alpha1"
-	"github.com/backube/volsync/controllers/mover"
+	vsmover "github.com/backube/volsync/controllers/mover"
 	"github.com/backube/volsync/controllers/utils"
 )
 
@@ -239,7 +240,7 @@ var _ = Describe("Restic properly registers", func() {
 
 		It("is added to the mover catalog", func() {
 			found := false
-			for _, v := range mover.Catalog {
+			for _, v := range vsmover.Catalog {
 				if _, ok := v.(*Builder); ok {
 					found = true
 				}
@@ -624,6 +625,78 @@ var _ = Describe("Restic as a source", func() {
 					Expect(*cache.Spec.StorageClassName).To(Equal(cachesc))
 				})
 			})
+
+			Context("old cache name migration", func() {
+				var oldCache *corev1.PersistentVolumeClaim
+				When("The cache PVC with the old name exists", func() {
+					BeforeEach(func() {
+						sc := "some-storage"
+						oldCache = &corev1.PersistentVolumeClaim{
+							ObjectMeta: metav1.ObjectMeta{
+								Name:      vsmover.VolSyncPrefix + rs.GetName() + "-cache",
+								Namespace: rs.GetNamespace(),
+							},
+							Spec: corev1.PersistentVolumeClaimSpec{
+								AccessModes: []corev1.PersistentVolumeAccessMode{
+									corev1.ReadWriteOnce,
+								},
+								Resources: corev1.VolumeResourceRequirements{
+									Requests: corev1.ResourceList{
+										"storage": resource.MustParse("1Gi"),
+									},
+								},
+								StorageClassName: &sc,
+							},
+						}
+					})
+
+					JustBeforeEach(func() {
+						Expect(k8sClient.Create(ctx, oldCache)).To(Succeed())
+					})
+
+					When("The old cache pvc is not owned by this VolSync RS", func() {
+						It("Should not delete the old cache PVC and still create the new one", func() {
+							cache, err := mover.ensureCache(ctx, dataPVC, false)
+							Expect(err).ToNot(HaveOccurred())
+							Expect(cache.GetName()).To(ContainSubstring("-cache"))
+							Expect(cache.GetName()).To(ContainSubstring("-src-"))
+
+							// Check that old cache pvc is still there
+							Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(oldCache), oldCache)).To(Succeed())
+							Expect(oldCache.GetDeletionTimestamp().IsZero()).To(BeTrue()) // Not marked for deletion
+						})
+					})
+
+					When("The old cache PVC was owned by this VolSync RS", func() {
+						JustBeforeEach(func() {
+							// Need to do this after the RS has been created
+							Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(oldCache), oldCache)).To(Succeed())
+							Expect(ctrl.SetControllerReference(rs, oldCache, k8sClient.Scheme())).To(Succeed())
+							Expect(k8sClient.Update(ctx, oldCache)).To(Succeed())
+						})
+
+						It("Should delete the old cache pvc and still create the new one", func() {
+							cache, err := mover.ensureCache(ctx, dataPVC, false)
+							Expect(err).ToNot(HaveOccurred())
+							Expect(cache.GetName()).To(ContainSubstring("-cache"))
+							Expect(cache.GetName()).To(ContainSubstring("-src-"))
+
+							// Check that old cache pvc was deleted
+							err = k8sClient.Get(ctx, client.ObjectKeyFromObject(oldCache), oldCache)
+							if err == nil {
+								// Should be marked for deletion
+								Expect(oldCache.GetDeletionTimestamp().IsZero()).To(BeFalse())
+							} else {
+								Expect(kerrors.IsNotFound(err)).To(BeTrue())
+							}
+
+							// Should be able to reconcile again
+							_, err = mover.ensureCache(ctx, dataPVC, false)
+							Expect(err).ToNot(HaveOccurred())
+						})
+					})
+				})
+			})
 		})
 
 		Context("Source volume is handled properly", func() {
@@ -1829,6 +1902,32 @@ var _ = Describe("Restic as a destination", func() {
 			mover, _ = m.(*Mover)
 			Expect(mover).NotTo(BeNil())
 		})
+
+		Context("Restic cache is created correctly", func() {
+			var dataPVC *corev1.PersistentVolumeClaim
+			BeforeEach(func() {
+				am := corev1.ReadWriteMany
+				rd.Spec.Restic.AccessModes = []corev1.PersistentVolumeAccessMode{
+					am,
+				}
+				destVolCap := resource.MustParse("6Gi")
+				rd.Spec.Restic.Capacity = &destVolCap
+			})
+
+			JustBeforeEach(func() {
+				var err error
+				dataPVC, err = mover.ensureDestinationPVC(ctx)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("Should create old cache pvc", func() {
+				cache, err := mover.ensureCache(ctx, dataPVC, false)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(cache.GetName()).To(ContainSubstring("-cache"))
+				Expect(cache.GetName()).To(ContainSubstring("-dst-"))
+			})
+		})
+
 		When("no destination volume is supplied", func() {
 			var destVolCap resource.Quantity
 			var am corev1.PersistentVolumeAccessMode

test-e2e/test_restic_manual_normal.yml

@@ -224,7 +224,7 @@
       kubernetes.core.k8s_info:
         api_version: v1
         kind: PersistentVolumeClaim
-        name: volsync-restore-cache
+        name: volsync-dst-restore-cache
         namespace: "{{ namespace }}"
       register: cachepvcreloaded
 

test-e2e/test_restic_manual_normal_cleanup_pvcs.yml

@@ -207,7 +207,7 @@
       kubernetes.core.k8s_info:
         api_version: v1
         kind: PersistentVolumeClaim
-        name: volsync-restore-cache
+        name: volsync-dst-restore-cache
         namespace: "{{ namespace }}"
       register: cachepvc
       until: >
@@ -300,7 +300,7 @@
       kubernetes.core.k8s_info:
         api_version: v1
         kind: PersistentVolumeClaim
-        name: volsync-restore-cache
+        name: volsync-dst-restore-cache
         namespace: "{{ namespace }}"
       register: cachepvcreloaded
 

test-e2e/test_restic_manual_normal_longname.yml

@@ -225,7 +225,7 @@
       kubernetes.core.k8s_info:
         api_version: v1
         kind: PersistentVolumeClaim
-        name: volsync-restore-thisisavery-very-very-very-longnameevenlongerthan63chars2-cache
+        name: volsync-dst-restore-thisisavery-very-very-very-longnameevenlongerthan63chars2-cache
         namespace: "{{ namespace }}"
       register: cachepvcreloaded