Use pod namespace from backup when matching PVBs #3475

zubron · 2021-02-18T16:15:52Z

Please add a summary of your change

In #3051, we introduced an additional check to ensure that a PVB matched
a particular pod by checking both the name and the namespace of the pod.
This caused an issue when using a namespace mapping on restore. In the
case where a namespace mapping is being used, the check for whether a
PVB matches a particular pod will fail as the PVB was created for the
original pod namespace and is not aware of the new namespace mapping
being used. This resulted in PVRs not being created for pods that were
being restored into new namespaces. The restic init containers were
being created to wait on the volume restore, however this would cause
the restored pods to block indefinitely as they would be waiting for a
volume restore that was not scheduled.

To fix this, we use the original namespace of the pod from the backup to
match the PVB to the pod being restored, not the new namespace where
the pod is being restored into.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

Does your change fix a particular issue?

Fixes #3467

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required.
Updated the corresponding documentation in site/content/docs/main.

In vmware-tanzu#3051, we introduced an additional check to ensure that a PVB matched a particular pod by checking both the name and the namespace of the pod. This caused an issue when using a namespace mapping on restore. In the case where a namespace mapping is being used, the check for whether a PVB matches a particular pod will fail as the PVB was created for the original pod namespace and is not aware of the new namespace mapping being used. This resulted in PVRs not being created for pods that were being restored into new namespaces. The restic init containers were being created to wait on the volume restore, however this would cause the restored pods to block indefinitely as they would be waiting for a volume restore that was not scheduled. To fix this, we use the original namespace of the pod from the backup to match the PVB to the pod being restored, not the new namespace where the pod is being restored into. Fixes vmware-tanzu#3467. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

ashish-amarnath

LGTM!

ashish-amarnath · 2021-02-20T01:54:45Z

pkg/restore/restic_restore_action.go

+	var sourcePod corev1.Pod
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(input.ItemFromBackup.UnstructuredContent(), &sourcePod); err != nil {
+		return nil, errors.Wrap(err, "unable to convert source pod from runtime.Unstructured")
+	}
+


Just for reference, we could have also reverse looked up the RestoreItemActionExecuteInput.Restore.Spec.NamespaceMapping. But this is also fine.

I thought about doing that but there was actually an issue with that approach. At the point when this action's Execute function is called in restoreItem, the namespace mapping hasn't yet been applied to the object being restored. That doesn't happen until later in the function. This means that, under the current implementation, both pod and sourcePod have the same namespace when this is called. Rather than assuming they would be the same, I opted to look at the item from the backup. It seemed too risky for this change to change when the namespace mapping would be applied in restoreItem, but checking the pod from the backup also prevents this issue occurring again if when we do change when the mapping is applied.

I should add a comment though to explain why we can't rely on the namespace mapping from the restore spec. Thanks for reminding me about why I couldn't use that approach! 👍

@zubron Not sure I follow. The NamespaceMapping that I was referring to is a field on the restore object spec. This field is set using the value of the namespace-mappings flag that is passed at the CLI running velero restore create command.

Thanks for clarifying @zubron.
Just capturing our discussion here. Changes at line79-82 weren't necessary and we could have used pod.Namespace at https://github.com/vmware-tanzu/velero/pull/3475/files#diff-9f110ab28d31cd60e15e9ec43ad813f22bd8b9329eaf0ebe70c0b6590592b76dR100
and added unit tests to catch any future code reordering that may break. The changes at line79-82 is future proofing.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

nrb · 2021-02-22T18:27:03Z

Before reviewing, I think this has the potential for backporting so that v1.5.x users can benefit from the fix.

nrb

Thank you for the added test and the comment about why we can't do reverse lookup!

* Use pod namespace from backup when matching PVBs In vmware-tanzu#3051, we introduced an additional check to ensure that a PVB matched a particular pod by checking both the name and the namespace of the pod. This caused an issue when using a namespace mapping on restore. In the case where a namespace mapping is being used, the check for whether a PVB matches a particular pod will fail as the PVB was created for the original pod namespace and is not aware of the new namespace mapping being used. This resulted in PVRs not being created for pods that were being restored into new namespaces. The restic init containers were being created to wait on the volume restore, however this would cause the restored pods to block indefinitely as they would be waiting for a volume restore that was not scheduled. To fix this, we use the original namespace of the pod from the backup to match the PVB to the pod being restored, not the new namespace where the pod is being restored into. Fixes vmware-tanzu#3467. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Explain why the namespace mapping can't be used Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Restore CAPI cluster objects in a better order Restoring CAPI workload clusters without this ordering caused the capi-controller-manager code to panic, resulting in an unhealthy cluster state. This can be worked around (https://community.pivotal.io/s/article/5000e00001pJyN41611954332537?language=en_US), but we provide the inclusion of these resources as a default in order to provide a better out-of-the-box experience. Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Add changelog Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Use pod namespace from backup when matching PVBs (#3475) * Use pod namespace from backup when matching PVBs In #3051, we introduced an additional check to ensure that a PVB matched a particular pod by checking both the name and the namespace of the pod. This caused an issue when using a namespace mapping on restore. In the case where a namespace mapping is being used, the check for whether a PVB matches a particular pod will fail as the PVB was created for the original pod namespace and is not aware of the new namespace mapping being used. This resulted in PVRs not being created for pods that were being restored into new namespaces. The restic init containers were being created to wait on the volume restore, however this would cause the restored pods to block indefinitely as they would be waiting for a volume restore that was not scheduled. To fix this, we use the original namespace of the pod from the backup to match the PVB to the pod being restored, not the new namespace where the pod is being restored into. Fixes #3467. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Explain why the namespace mapping can't be used Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Allow Dockerfiles to be configurable (#3634) For internal builds of Velero, we need to be able to specify an alternative Dockerfile which uses an alternative image registry to pull the base images from. This change adapts our Makefile such that both the main Dockerfile and build image Dockerfile can be overridden. We have some special handling for the build image to only build when the Dockerfile has changed. In this case, we check whether a custom Dockerfile has been provided, and always rebuild in that case. For custom build image Dockerfiles, use a fixed tag rather than the one based on commit SHA of the original file. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Combine CRD install verification into 1 job, and update k8s versions (#3448) * Validate CRDs against latest Kubernetes versions Add Kubernetes v1.19 and v1.20 series images, and consolidate the job into a single file to reduce repetition. Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Ignore job if the changes are only site/design Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Fix codespell error Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Cache Velero binary for reuse on workers This will cache the Velero binary based on the PR number and a SHA256 of the generated binary. This way, the runners testing each version of Kubernetes do not need to build it independently. Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Fix GitHub event access Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Wrap output path in quotes Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Move code checkout to build step Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Also cache go modules Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Fix syntax issues Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Download cached binary on each node Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Use cached go modules on main CI Signed-off-by: Nolan Brubaker <brubakern@vmware.com> * Add changelog for v1.5.4 Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> Co-authored-by: Nolan Brubaker <brubakern@vmware.com>

* Use pod namespace from backup when matching PVBs In vmware-tanzu#3051, we introduced an additional check to ensure that a PVB matched a particular pod by checking both the name and the namespace of the pod. This caused an issue when using a namespace mapping on restore. In the case where a namespace mapping is being used, the check for whether a PVB matches a particular pod will fail as the PVB was created for the original pod namespace and is not aware of the new namespace mapping being used. This resulted in PVRs not being created for pods that were being restored into new namespaces. The restic init containers were being created to wait on the volume restore, however this would cause the restored pods to block indefinitely as they would be waiting for a volume restore that was not scheduled. To fix this, we use the original namespace of the pod from the backup to match the PVB to the pod being restored, not the new namespace where the pod is being restored into. Fixes vmware-tanzu#3467. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Explain why the namespace mapping can't be used Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

github-actions bot added the has-unit-tests label Feb 18, 2021

zubron force-pushed the use-original-namespace-for-pvb-matching-3467 branch from 77b9ea1 to 77c2bd7 Compare February 18, 2021 16:18

github-actions bot added the has-changelog label Feb 18, 2021

zubron marked this pull request as ready for review February 18, 2021 16:20

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, jenting and nrb February 18, 2021 16:20

github-actions bot assigned zubron Feb 18, 2021

zubron added P1 - Important kind/release-blocker Must fix issues for the coming release (milestone) labels Feb 18, 2021

zubron added this to the v1.6.0 milestone Feb 18, 2021

ashish-amarnath previously approved these changes Feb 20, 2021

View reviewed changes

zubron dismissed ashish-amarnath’s stale review via dbd3d54 February 20, 2021 16:56

github-actions bot requested a review from ashish-amarnath February 20, 2021 16:57

Explain why the namespace mapping can't be used

0d56725

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

zubron force-pushed the use-original-namespace-for-pvb-matching-3467 branch from dbd3d54 to 0d56725 Compare February 20, 2021 16:57

nrb added the Backport candidate label Feb 22, 2021

nrb approved these changes Feb 22, 2021

View reviewed changes

ashish-amarnath approved these changes Feb 22, 2021

View reviewed changes

ashish-amarnath merged commit 0246a32 into vmware-tanzu:main Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pod namespace from backup when matching PVBs #3475

Use pod namespace from backup when matching PVBs #3475

zubron commented Feb 18, 2021 •

edited

Loading

ashish-amarnath left a comment

ashish-amarnath Feb 20, 2021

zubron Feb 20, 2021

zubron Feb 20, 2021

ashish-amarnath Feb 22, 2021

ashish-amarnath Feb 22, 2021

nrb commented Feb 22, 2021

nrb left a comment

Use pod namespace from backup when matching PVBs #3475

Use pod namespace from backup when matching PVBs #3475

Conversation

zubron commented Feb 18, 2021 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

ashish-amarnath left a comment

Choose a reason for hiding this comment

ashish-amarnath Feb 20, 2021

Choose a reason for hiding this comment

zubron Feb 20, 2021

Choose a reason for hiding this comment

zubron Feb 20, 2021

Choose a reason for hiding this comment

ashish-amarnath Feb 22, 2021

Choose a reason for hiding this comment

ashish-amarnath Feb 22, 2021

Choose a reason for hiding this comment

nrb commented Feb 22, 2021

nrb left a comment

Choose a reason for hiding this comment

zubron commented Feb 18, 2021 •

edited

Loading