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

🐛 Use namespace and name to match PVB to Pod restore #3051

Merged
merged 1 commit into from
Nov 10, 2020
Merged

🐛 Use namespace and name to match PVB to Pod restore #3051

merged 1 commit into from
Nov 10, 2020

Conversation

ashish-amarnath
Copy link
Member

@ashish-amarnath ashish-amarnath commented Nov 2, 2020 •
edited by nrb
Loading

Signed-off-by: Ashish Amarnath [email protected]

Fixes: #3027
Fixes: #2691
Fixes: #2956

@zubron Thanks for the debugging and repro example.

@github-actions github-actions bot requested review from carlisia, nrb and zubron November 2, 2020 20:08
@ashish-amarnath ashish-amarnath mentioned this pull request Nov 2, 2020
Closed
@ashish-amarnath
Copy link
Member Author

Here is the sample workload, from Bridget, that gives us a repro https://gist.github.com/zubron/dce63c10b5aea026f988b8a14d2934c5#file-app-yaml

zubron
zubron suggested changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @ashish-amarnath! Just a couple of minor suggestions in test names but is otherwise good. Also looks like some other restore tests need to be updated following this change.

pkg/restic/common_test.go Outdated Show resolved Hide resolved
pkg/restic/common_test.go Outdated Show resolved Hide resolved
@zubron
Copy link
Contributor

zubron commented Nov 2, 2020

This will also fix #2691 and #2956, I looked at those two issues again and they also have the same problem of using the same pod name across multiple namespaces.

@nrb
Copy link
Contributor

nrb commented Nov 2, 2020

Could I get a summary of what the issue was? Were PVBs being assigned to the wrong pod?

@zubron
Copy link
Contributor

zubron commented Nov 2, 2020

@nrb I wrote up my findings for this issue in these two comments: #3027 (comment) and #3027 (comment)

@github-actions github-actions bot requested review from nrb and zubron November 2, 2020 23:21
@ashish-amarnath
Copy link
Member Author

To summarize the bug here.
Velero uses only the pod's name to find its corresponding podvolumebackups. When there are pods with the same name but in different namespaces, the wrong podvolume backup is matched with the wrong pod.

The fix here is to use both the pod name and its namespace to find the podvolumebackups that pertain to the pod.

Looking at the CI failures now.

zubron
zubron approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks!

@nrb nrb merged commit dc6762a into vmware-tanzu:main Nov 10, 2020
@georgettica georgettica mentioned this pull request Dec 23, 2020
Closed
georgettica pushed a commit to georgettica/velero that referenced this pull request Dec 23, 2020
@ashish-amarnath @georgettica
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
9ab54f9 
Signed-off-by: Ashish Amarnath <[email protected]>
zubron pushed a commit that referenced this pull request Jan 14, 2021
@ashish-amarnath @zubron
🐛 Use namespace and name to match PVB to Pod restore (#3051)
cd26cd0 
Signed-off-by: Ashish Amarnath <[email protected]>
georgettica pushed a commit to georgettica/velero that referenced this pull request Jan 26, 2021
@ashish-amarnath @georgettica
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
4cab0d4 
Signed-off-by: Ashish Amarnath <[email protected]>
vadasambar pushed a commit to vadasambar/velero that referenced this pull request Feb 3, 2021
@ashish-amarnath
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
cfd737d 
Signed-off-by: Ashish Amarnath <[email protected]>
zubron added a commit to zubron/velero that referenced this pull request Feb 18, 2021
@zubron
Use pod namespce from backup when matching PVBs
3597d93 
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 <[email protected]>
zubron added a commit to zubron/velero that referenced this pull request Feb 18, 2021
@zubron
Use pod namespace from backup when matching PVBs
77b9ea1 
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 <[email protected]>
@zubron zubron mentioned this pull request Feb 18, 2021
Merged
3 tasks
zubron added a commit to zubron/velero that referenced this pull request Feb 18, 2021
@zubron
Use pod namespace from backup when matching PVBs
77c2bd7 
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 <[email protected]>
ashish-amarnath pushed a commit that referenced this pull request Feb 22, 2021
@zubron
Use pod namespace from backup when matching PVBs (#3475)
0246a32 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
zubron added a commit to zubron/velero that referenced this pull request Mar 31, 2021
@zubron
Use pod namespace from backup when matching PVBs (vmware-tanzu#3475)
72841d1 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
zubron added a commit to zubron/velero that referenced this pull request Mar 31, 2021
@zubron
Use pod namespace from backup when matching PVBs (vmware-tanzu#3475)
7242498 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
carlisia pushed a commit that referenced this pull request Apr 1, 2021
@zubron @nrb
Add cherry-pick commits and changelog for v1.5.4 (#3651)
525705b 
* 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 <[email protected]>

* Add changelog

Signed-off-by: Nolan Brubaker <[email protected]>

* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Ignore job if the changes are only site/design

Signed-off-by: Nolan Brubaker <[email protected]>

* Fix codespell error

Signed-off-by: Nolan Brubaker <[email protected]>

* 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 <[email protected]>

* Fix GitHub event access

Signed-off-by: Nolan Brubaker <[email protected]>

* Wrap output path in quotes

Signed-off-by: Nolan Brubaker <[email protected]>

* Move code checkout to build step

Signed-off-by: Nolan Brubaker <[email protected]>

* Also cache go modules

Signed-off-by: Nolan Brubaker <[email protected]>

* Fix syntax issues

Signed-off-by: Nolan Brubaker <[email protected]>

* Download cached binary on each node

Signed-off-by: Nolan Brubaker <[email protected]>

* Use cached go modules on main CI

Signed-off-by: Nolan Brubaker <[email protected]>

* Add changelog for v1.5.4

Signed-off-by: Bridget McErlean <[email protected]>

Co-authored-by: Nolan Brubaker <[email protected]>
dharmab pushed a commit to dharmab/velero that referenced this pull request May 25, 2021
@ashish-amarnath @dharmab
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
3e9d4b0 
Signed-off-by: Ashish Amarnath <[email protected]>
dharmab pushed a commit to dharmab/velero that referenced this pull request May 25, 2021
@zubron @dharmab
Use pod namespace from backup when matching PVBs (vmware-tanzu#3475)
ce2a1fa 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jun 29, 2021
@ashish-amarnath @ywk253100
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
a4e7926 
Signed-off-by: Ashish Amarnath <[email protected]>
ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jun 29, 2021
@zubron @ywk253100
Use pod namespace from backup when matching PVBs (vmware-tanzu#3475)
4a9c26c 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
@ashish-amarnath @gyaozhou
🐛 Use namespace and name to match PVB to Pod restore (vmware-tanzu#3051)
d1b8708 
Signed-off-by: Ashish Amarnath <[email protected]>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
@zubron @gyaozhou
Use pod namespace from backup when matching PVBs (vmware-tanzu#3475)
8b0c69d 
* 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 <[email protected]>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlisia carlisia carlisia left review comments

@zubron zubron zubron approved these changes

@nrb nrb nrb approved these changes

Assignees

@ashish-amarnath ashish-amarnath

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ashish-amarnath @zubron @nrb @carlisia