Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mount block PVC rw in clone source pods #2944

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Mount block PVC rw in clone source pods #2944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion pkg/controller/clone-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,6 @@ func MakeCloneSourcePodSpec(sourceVolumeMode corev1.PersistentVolumeMode, image,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: sourcePvcName,
ReadOnly: sourceVolumeMode == corev1.PersistentVolumeBlock,
},
},
},
Expand Down
69 changes: 68 additions & 1 deletion pkg/controller/clone-controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test

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",
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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,
},
},
},
},
},
}
}
12 changes: 12 additions & 0 deletions pkg/controller/common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,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
}
}
Comment on lines +606 to +611
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhenriks @awels I think this check was always broken for pods with volumeDevices

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. This will only cover cases where block PVCs are in the pod spec read/write but not referred to by a container.

}
if onlyReadOnly {
// no rw mounts
Expand All @@ -612,6 +618,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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pod.Labels != nil is not required. pod.Labels[common.CDIComponentLabel] == common.ClonerSourcePodName can still be executed when pod.Labels is nil

// Host assisted clone source pod only reads from source
// But some drivers disallow mounting a block PVC ReadOnly
addPod = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several consecutive conditions lead to the same result (false addPod), would it be possible to clean-up this a little so we have it more condenssed? Maybe something like a shouldAddPod function? Not a must but I think this code is getting hard to follow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the whole function can be refactored but I prefer to stay on the safe side since this is going to backport all the way to 0.55

}
}
if addPod {
pods = append(pods, pod)
Expand Down