Bug 1887526: only include pvc in volume mounts at stage restore of a pod #97
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| exclude = append(exclude, volume.Name) | ||
| continue | ||
| } | ||
| pvcVolumes = append(pvcVolumes, volume) |
There was a problem hiding this comment.
We also need to filter this to exclude "move" and "snapshot copy" volumes from the pod, since these are excluded from stage restore, and the pod won't come up if it's missing volumes. What's not obvious is how to do that here.
@dymurray do you have any insight here? When creating stage pods in the controller, we can use the MigPlan selection values. We don't have access to the PVC annotations since those are in the backup, not in the cluster. Do we need to annotate application pods with a list of PVC exclusions? Basically when we add the stage labels to application pods on backup, in GetApplicationPodsWithStageLabels, when we find a PVC that should be excluded from the stage pod because it's a move or snapshot copy, we'll add its name to a PVC exclude list annotation, and the plugin here could make use of that. Is there a cleaner way to do this?
There was a problem hiding this comment.
Basically when we add the stage labels to application pods on backup, in GetApplicationPodsWithStageLabels, when we find a PVC that should be excluded from the stage pod because it's a move or snapshot copy, we'll add its name to a PVC exclude list annotation, and the plugin here could make use of that.
I've been trying to thinnk of a better way to do this and I haven't yet. This seems to be the cleanest way to make this work.
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
velero-plugins/pod/restore.go
Outdated
| continue | ||
| } | ||
| if len(pod.Labels[common.ExcludePVCPodLabel]) > 0 { | ||
| if strings.Contains(pod.Labels[common.ExcludePVCPodLabel], volume.Name) { |
There was a problem hiding this comment.
We need to split the Exclude label out into a slice and compare slice entries. In the simple case, "foo,bar,baz" does contain "foo" so we'll exclude foo, "foo1,foo2" will contain "foo" so foo would be erroneously skipped.
velero-plugins/pod/restore.go
Outdated
| @@ -113,9 +113,55 @@ func (p *RestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (*v | |||
| destStagePodImage := input.Restore.Annotations[common.StagePodImageAnnotation] | |||
| if len(pod.Labels[common.IncludedInStageBackupLabel]) > 0 && len(destStagePodImage) > 0 { | |||
| p.Log.Infof("[pod-restore] swapping stage pod images for pod %s", pod.Name) | |||
| pvcVolumes := []corev1API.Volume{} | |||
| exclude := []string{} | |||
| moveVolumes := []string{} | |||
There was a problem hiding this comment.
nit: not a merge blocker, but a minor comment. It's slightly confusing to call this "moveVolumes" since it also includes snapshot copy volumes. "exclude" is already in use, though. Maybe "excludePVC" or "excludeAnnotation" or something like that?
There was a problem hiding this comment.
+1, removed excude as it was not being used
|
LGTM |
JaydipGabani
changed the title
|
@JaydipGabani: All pull requests linked via external trackers have merged: Bugzilla bug 1887526 has been moved to the MODIFIED state. 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. |
This PR trim downs volumes and volume mounts to only PVCs while restoring pods in stage migration. Needed for the PR
Changes include:
sleepto make the application pod look like stage pod