Mount block PVC rw in clone source pods #2944
Conversation
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-bot
added
release-note
| for _, vm := range c.VolumeDevices { | ||
| if vm.Name == volume.Name { | ||
| // Node level rw mount and container can't mount block device ro | ||
| onlyReadOnly = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Kind of. This will only cover cases where block PVCs are in the pod spec read/write but not referred to by a container.
|
/test pull-containerized-data-importer-e2e-nfs |
1 similar comment
|
/test pull-containerized-data-importer-e2e-nfs |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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() { | |||
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awels The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
/hold |
kubevirt-bot
added
the
do-not-merge/hold
| @@ -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 { | |||
There was a problem hiding this comment.
pod.Labels != nil is not required. pod.Labels[common.CDIComponentLabel] == common.ClonerSourcePodName can still be executed when pod.Labels is nil
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
|
/cherrypick release-v1.58 |
|
@akalenyu: new pull request created: #2956 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherrypick release-v1.57 |
|
@akalenyu: new pull request created: #2957 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherrypick release-v1.56 |
|
@akalenyu: new pull request created: #2958 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
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
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
https://issues.redhat.com/browse/CNV-33859
Special notes for your reviewer:
Release note: