Mount block PVC rw in clone source pods (#2944)

Some CSI drivers [0] don't allow mounting block ro, so they simply cannot perform host assisted clones. Host assisted clones are important as they are a last resort in some cases like switching over to a new storage provisioner. This may or may not have some performance implications (parallel cloning from single source) but we are choosing compatibility over performance in this case. [0] https://github.com/dell/csi-unity/blob/76cb78decd0c7e0ae3bded9668e7fbe9b39dd2ff/service/node.go#L420-L424 Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt · Oct 31, 2023 · 7c1d7db · 7c1d7db
1 parent aff18f0
commit 7c1d7db
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 2 deletions.
diff --git a/pkg/controller/clone-controller.go b/pkg/controller/clone-controller.go
@@ -625,7 +625,6 @@ func MakeCloneSourcePodSpec(sourceVolumeMode corev1.PersistentVolumeMode, image,
 					VolumeSource: corev1.VolumeSource{
 						PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
 							ClaimName: sourcePvcName,
-							ReadOnly:  sourceVolumeMode == corev1.PersistentVolumeBlock,
 						},
 					},
 				},

diff --git a/pkg/controller/clone-controller_test.go b/pkg/controller/clone-controller_test.go
@@ -178,6 +178,41 @@ var _ = Describe("Clone controller reconcile loop", func() {
 		}),
 	)
 
+	It("Should create new source pod if source PVC is in use by other host assisted source pod", func() {
+		testPvc := cc.CreatePvc("testPvc1", "default", map[string]string{
+			cc.AnnCloneRequest:   "default/source",
+			cc.AnnPodReady:       "true",
+			cc.AnnCloneToken:     "foobaz",
+			AnnUploadClientName:  "uploadclient",
+			cc.AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil)
+		sourcePvc := cc.CreatePvc("source", "default", map[string]string{}, nil)
+		pod := podUsingBlockPVC(sourcePvc, false)
+		pod.Name = "other-target-pvc-uid" + common.ClonerSourcePodNameSuffix
+		pod.Labels = map[string]string{
+			common.CDIComponentLabel: common.ClonerSourcePodName,
+		}
+		reconciler = createCloneReconciler(testPvc, sourcePvc, pod)
+		By("Setting up the match token")
+		reconciler.multiTokenValidator.ShortTokenValidator.(*cc.FakeValidator).Match = "foobaz"
+		reconciler.multiTokenValidator.ShortTokenValidator.(*cc.FakeValidator).Name = "source"
+		reconciler.multiTokenValidator.ShortTokenValidator.(*cc.FakeValidator).Namespace = "default"
+		reconciler.multiTokenValidator.ShortTokenValidator.(*cc.FakeValidator).Params["targetNamespace"] = "default"
+		reconciler.multiTokenValidator.ShortTokenValidator.(*cc.FakeValidator).Params["targetName"] = "testPvc1"
+		By("Verifying no source pod exists")
+		sourcePod, err := reconciler.findCloneSourcePod(testPvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sourcePod).To(BeNil())
+		result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}})
+		Expect(err).ToNot(HaveOccurred())
+		Expect(result.RequeueAfter).To(BeZero())
+		By("Verifying source pod gets created")
+		sourcePod, err = reconciler.findCloneSourcePod(testPvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sourcePod).ToNot(BeNil())
+		Expect(sourcePod.Name).ToNot(Equal(pod.Name))
+		reconciler = nil
+	})
+
 	DescribeTable("Should create new source pod if none exists, and target pod is marked ready and", func(sourceVolumeMode corev1.PersistentVolumeMode, podFunc func(*corev1.PersistentVolumeClaim) *corev1.Pod) {
 		testPvc := cc.CreatePvc("testPvc1", "default", map[string]string{
 			cc.AnnCloneRequest:   "default/source",
@@ -212,7 +247,7 @@ var _ = Describe("Clone controller reconcile loop", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(sourcePod).ToNot(BeNil())
 		if sourceVolumeMode == corev1.PersistentVolumeBlock {
-			Expect(sourcePod.Spec.Volumes[0].PersistentVolumeClaim.ReadOnly).To(BeTrue())
+			Expect(sourcePod.Spec.Volumes[0].PersistentVolumeClaim.ReadOnly).To(BeFalse())
 		} else {
 			Expect(sourcePod.Spec.Containers[0].VolumeMounts[0].ReadOnly).To(BeTrue())
 			Expect(sourcePod.Spec.Volumes[0].PersistentVolumeClaim.ReadOnly).To(BeFalse())
@@ -865,3 +900,35 @@ func createSourcePod(pvc *corev1.PersistentVolumeClaim, pvcUID string) *corev1.P
 
 	return pod
 }
+
+func podUsingBlockPVC(pvc *corev1.PersistentVolumeClaim, readOnly bool) *corev1.Pod {
+	return &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: pvc.Namespace,
+			Name:      pvc.Name + "-pod",
+		},
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					VolumeDevices: []corev1.VolumeDevice{
+						{
+							Name:       "v1",
+							DevicePath: common.WriteBlockPath,
+						},
+					},
+				},
+			},
+			Volumes: []corev1.Volume{
+				{
+					Name: "v1",
+					VolumeSource: corev1.VolumeSource{
+						PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+							ClaimName: pvc.Name,
+							ReadOnly:  readOnly,
+						},
+					},
+				},
+			},
+		},
+	}
+}
diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go
@@ -658,6 +658,12 @@ func GetPodsUsingPVCs(ctx context.Context, c client.Client, namespace string, na
 									onlyReadOnly = false
 								}
 							}
+							for _, vm := range c.VolumeDevices {
+								if vm.Name == volume.Name {
+									// Node level rw mount and container can't mount block device ro
+									onlyReadOnly = false
+								}
+							}
 						}
 						if onlyReadOnly {
 							// no rw mounts
@@ -667,6 +673,12 @@ func GetPodsUsingPVCs(ctx context.Context, c client.Client, namespace string, na
 						// all mounts must be ro
 						addPod = false
 					}
+					if strings.HasSuffix(pod.Name, common.ClonerSourcePodNameSuffix) && pod.Labels != nil &&
+						pod.Labels[common.CDIComponentLabel] == common.ClonerSourcePodName {
+						// Host assisted clone source pod only reads from source
+						// But some drivers disallow mounting a block PVC ReadOnly
+						addPod = false
+					}
 				}
 				if addPod {
 					pods = append(pods, pod)