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

restore: rename PV when remapping a namespace if PV exists in-cluster #1779

Merged
merged 7 commits into from
Aug 27, 2019
Merged

restore: rename PV when remapping a namespace if PV exists in-cluster #1779

merged 7 commits into from
Aug 27, 2019

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Aug 20, 2019 •
edited
Loading

Signed-off-by: Steve Kriss [email protected]

closes #192

TODO:

  • add unit test cases (positive cases & ensure adequate negative cases)
  • think about possible ways to simplify this code

prydonius
prydonius reviewed Aug 20, 2019
View reviewed changes
pkg/restore/restore.go Outdated Show resolved Hide resolved
pkg/restore/restore.go Outdated Show resolved Hide resolved
@skriss skriss changed the title rename PV during restore when cloning a namespace if PV already exists in-cluster restore: rename PV when remapping a namespace if PV exists in-cluster Aug 21, 2019
@skriss
Copy link
Contributor Author

skriss commented Aug 21, 2019

Addressed comments so far + added test cases

@skriss
changelog
dc3890d 
Signed-off-by: Steve Kriss <[email protected]>
@skriss skriss marked this pull request as ready for review August 23, 2019 16:22
@skriss skriss requested review from carlisia and nrb August 23, 2019 16:22
carlisia
carlisia reviewed Aug 23, 2019
View reviewed changes
pkg/restore/restore.go Outdated Show resolved Hide resolved
carlisia
carlisia reviewed Aug 23, 2019
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

Some suggestions.

@skriss
address review feedback
0d0749a 
Signed-off-by: Steve Kriss <[email protected]>
@skriss
Copy link
Contributor Author

skriss commented Aug 27, 2019

Addressed comments. Ref. #1779 (comment) -- I did it that way originally (using the explicit if...else) because I thought it was clearer, but I've updated it per your comment. Fine with whichever is clearer.

@carlisia
Copy link
Contributor

It was clearer, this is clear enough I think. Looking..

@carlisia
Copy link
Contributor

carlisia commented Aug 27, 2019 •
edited
Loading

Hm... if you don't see this as an improvement, please leave it alone, this might be way too much thinking. Here's what was twisting my mind the first time around, and here's how this all can be improved (maybe), although in the end... potatoes/potatos.

image

When I first saw the first if statement (deleted part), the `if shouldRenamePV { shouldRestoreSnapshot = true} required a lot of thinking, because of the two "shoulds" being true.

Now what I would suggest: invert the original logic, so in the !shouldRenamePV statement, add an else and do the shouldRestoreSnapshot = true in there. Now for me at least it's easy to glance and understand: IF NOT renaming the PV, then check if it should restore the snapshot; (else) if renaming, then restore snapshot no matter what;

@carlisia
Copy link
Contributor

Made some fixes to my comment.

@skriss
Copy link
Contributor Author

skriss commented Aug 27, 2019

👍 that seems reasonable to me!

@skriss
address review feedback
cff42f5 
Signed-off-by: Steve Kriss <[email protected]>
@carlisia
Copy link
Contributor

This looks soooo much better to my brain!

carlisia
carlisia approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

👍

prydonius
prydonius approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

lgtm

@prydonius prydonius merged commit 60f9898 into vmware-tanzu:master Aug 27, 2019
@skriss skriss deleted the clone-pv branch August 28, 2019 14:41
jessestuart added a commit to jessestuart/velero that referenced this pull request Aug 28, 2019
@jessestuart
Merge remote-tracking branch 'upstream/master' into jesse/20190828_merge
36a4fa4 
* upstream/master: (118 commits)
  restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779)
  when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807)
  Unit tests for restic restore (vmware-tanzu#1747)
  Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808)
  create backups from schedules using velero create backup (vmware-tanzu#1734)
  remove calls to restic check before/after prune (vmware-tanzu#1794)
  Propose adding feature flags to velero
  restic backup and restore progress proposal (vmware-tanzu#1765)
  allow custom restic repo prefix to be specified in BSL config
  error if restic repo identifier can't be determined
  update nokogiri dep for website
  update links on website home page for latest release (vmware-tanzu#1789)
  Velero 1.1 blog post
  v1.1.0 changelog
  fix error formatting
  Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)"
  allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)
  v1.1.0 docs
  Add the prefix to BSL config map so that object stores can use it when initializing (vmware-tanzu#1767)
  add stable/velero to helm commands
  ...
jessestuart added a commit to jessestuart/velero that referenced this pull request Aug 28, 2019
@jessestuart
Merge remote-tracking branch 'upstream/master' into jesse/20190828_merge
9cde4ba 
* upstream/master: (118 commits)
  restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779)
  when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807)
  Unit tests for restic restore (vmware-tanzu#1747)
  Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808)
  create backups from schedules using velero create backup (vmware-tanzu#1734)
  remove calls to restic check before/after prune (vmware-tanzu#1794)
  Propose adding feature flags to velero
  restic backup and restore progress proposal (vmware-tanzu#1765)
  allow custom restic repo prefix to be specified in BSL config
  error if restic repo identifier can't be determined
  update nokogiri dep for website
  update links on website home page for latest release (vmware-tanzu#1789)
  Velero 1.1 blog post
  v1.1.0 changelog
  fix error formatting
  Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)"
  allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)
  v1.1.0 docs
  Add the prefix to BSL config map so that object stores can use it when initializing (vmware-tanzu#1767)
  add stable/velero to helm commands
  ...
jessestuart added a commit to jessestuart/velero that referenced this pull request Aug 28, 2019
@jessestuart
Merge remote-tracking branch 'upstream/master' into jesse/20190828_merge
bebc2df 
* upstream/master: (118 commits)
  restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779)
  when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807)
  Unit tests for restic restore (vmware-tanzu#1747)
  Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808)
  create backups from schedules using velero create backup (vmware-tanzu#1734)
  remove calls to restic check before/after prune (vmware-tanzu#1794)
  Propose adding feature flags to velero
  restic backup and restore progress proposal (vmware-tanzu#1765)
  allow custom restic repo prefix to be specified in BSL config
  error if restic repo identifier can't be determined
  update nokogiri dep for website
  update links on website home page for latest release (vmware-tanzu#1789)
  Velero 1.1 blog post
  v1.1.0 changelog
  fix error formatting
  Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)"
  allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)
  v1.1.0 docs
  Add the prefix to BSL config map so that object stores can use it when initializing (vmware-tanzu#1767)
  add stable/velero to helm commands
  ...
jessestuart added a commit to jessestuart/velero that referenced this pull request Aug 28, 2019
@jessestuart
Merge branch 'jesse/20190828_merge'
fbb3d26 
* jesse/20190828_merge: (511 commits)
  fix(ci): Update arm32 target.
  feat(ci): Auto-build restic-restore-helper image in CI.
  restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779)
  when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807)
  Unit tests for restic restore (vmware-tanzu#1747)
  Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808)
  create backups from schedules using velero create backup (vmware-tanzu#1734)
  remove calls to restic check before/after prune (vmware-tanzu#1794)
  Propose adding feature flags to velero
  restic backup and restore progress proposal (vmware-tanzu#1765)
  allow custom restic repo prefix to be specified in BSL config
  error if restic repo identifier can't be determined
  update nokogiri dep for website
  update links on website home page for latest release (vmware-tanzu#1789)
  Velero 1.1 blog post
  v1.1.0 changelog
  fix error formatting
  Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)"
  allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)
  v1.1.0 docs
  ...
@nrb nrb mentioned this pull request Dec 16, 2019
Closed
@sseago sseago mentioned this pull request Oct 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlisia carlisia carlisia approved these changes

@prydonius prydonius prydonius approved these changes

@nrb nrb Awaiting requested review from nrb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support restoring a PV into the same cluster
3 participants
@skriss @carlisia @prydonius