feat(remote-restore): enabling restore in different namespace #72

mynktl · 2020-04-22T15:57:38Z

Changes:

Enabling remote restore in different namespace.
When restore, for cstor volume, is performed for remote-backup then velero-plugin find the velero relevant restore resource and get the namespace mapping and creates a pvc in that namespace.
plugin find the relevant restore from the sorted list(creationTimestamp in decreasing order) of restore resource using following criteria:
- restore is in in-progress state AND backup for that restore matches with the backup name from snapshotID

fixes: #67

Signed-off-by: mayank mayank.patel@mayadata.io

mynktl · 2020-04-22T15:58:40Z

Refactoring will be taken in next PR.

Signed-off-by: mayank <mayank.patel@mayadata.io>

mynktl · 2020-04-24T17:44:30Z

Test output:

?   	github.com/openebs/velero-plugin/pkg/cstor	[no test files]
?   	github.com/openebs/velero-plugin/pkg/snapshot	[no test files]
?   	github.com/openebs/velero-plugin/pkg/velero	[no test files]
?   	github.com/openebs/velero-plugin/tests/app	[no test files]
?   	github.com/openebs/velero-plugin/tests/config	[no test files]
?   	github.com/openebs/velero-plugin/tests/k8s	[no test files]
?   	github.com/openebs/velero-plugin/tests/openebs	[no test files]
=== RUN   TestVELERO
Running Suite: Velero integration test suite
============================================
Random Seed: 1587749220
Will run 7 of 7 specs

• [SLOW TEST:45.227 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Non-scheduled Backup
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:77
    Backup Test 1
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:78
------------------------------
Waiting for schedule umsbxctz completion completed Backup:1/3
Waiting for schedule umsbxctz completion completed Backup:1/3
Waiting for schedule umsbxctz completion completed Backup:1/3
Waiting for schedule umsbxctz completion completed Backup:2/3
Waiting for schedule umsbxctz completion completed Backup:2/3
Waiting for schedule umsbxctz completion completed Backup:2/3
Waiting for schedule umsbxctz completion completed Backup:3/3
• [SLOW TEST:250.831 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Scheduled Backup
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:103
    Backup Test 1
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:104
------------------------------
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-ce80653f-8650-11ea-b467-d89c67d5e4a1..
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-ce80653f-8650-11ea-b467-d89c67d5e4a1..
• [SLOW TEST:95.206 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Restore Test
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:128
    Restore from non-scheduled backup
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:137
------------------------------
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-c85708ed-8651-11ea-b467-d89c67d5e4a1..
Waiting for restore vtbrtinz-umsbxctz-20200424173016 completion..
• [SLOW TEST:115.160 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Restore Test
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:128
    Restore from scheduled backup without base-backup
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:162
------------------------------
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-ec27745a-8651-11ea-b467-d89c67d5e4a1..
• [SLOW TEST:80.525 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Restore Test
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:128
    Restore from scheduled backup using base-backup
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:172
------------------------------
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-45dd509f-8652-11ea-b467-d89c67d5e4a1..
• [SLOW TEST:60.152 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Restore Test in different namespace
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:209
    Restore from non-scheduled backup to different Namespace
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:218
------------------------------
Waiting for cleanup of deployment openebs/openebs.io/persistent-volume=pvc-69bcb63a-8652-11ea-b467-d89c67d5e4a1..
• [SLOW TEST:85.594 seconds]
Backup/Restore Test
/home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:76
  Restore Test in different namespace
  /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:209
    Restore from scheduled backup to different Namespace
    /home/mayank/go/src/github.com/openebs/velero-plugin/tests/sanity/backup_test.go:240
------------------------------

Ran 7 of 7 Specs in 772.827 seconds
SUCCESS! -- 7 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestVELERO (772.83s)
PASS
ok  	github.com/openebs/velero-plugin/tests/sanity	772.842s
?   	github.com/openebs/velero-plugin/tests/velero	[no test files]
?   	github.com/openebs/velero-plugin/velero-blockstore-cstor	[no test files]

Signed-off-by: mayank <mayank.patel@mayadata.io>

README.md

kmova · 2020-04-27T15:00:44Z

pkg/cstor/cstor.go

-	s := strings.Split(snapshotID, "-velero-bkp-")
-	volumeID := s[0]
-	snapName := s[1]
+	volumeID, snapName := getInfoFromSnapshotID(snapshotID)

 	snapType := "cloud"


should this be cloud or remote?

kmova · 2020-04-28T04:47:33Z

pkg/velero/velero.go

@@ -0,0 +1,32 @@
+package velero


Can you add another task to verify copyright checks, something to the effect of https://github.com/openebs/maya/blob/master/buildscripts/travis-build.sh#L41

done. I'll address it in separate PR.

kmova · 2020-04-28T04:53:33Z

pkg/velero/restore.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// GetRestoreNamespace return the namespace mapping for the given namespace


The comment can be modified to make it clear that target namespace is returned by this function.

GetRestoreNamespace return the target namespace for restoring from the backup (bkpName). If namespace mapping was provided to the restore command then target namespace is returned. If namespace mapping was not provided to the restore command, source namespace (ns) will be returned.

kmova · 2020-04-28T04:56:10Z

pkg/velero/restore.go

+		return "", errors.Wrapf(err, "failed to get list of restore")
+	}
+
+	sort.Sort(sort.Reverse(RestoreByCreationTimestamp(list.Items)))


Can you comment above this line to indicate why sort was used?

This logic seems to work because only one restore is active on a given backup name. Is this true.

Also is it not possible to pass the restore id or something to this function. If not, specify in the comment that velero doesn't send the restore id, so we have to depend on the backup name and the order of invocation. And at any given time there will ideally be only one RestorePhaseInProgress for a given backup name.

ok. I'll add the comment similar to PR description.

kmova · 2020-04-28T05:10:56Z

The changes look good. But I have few questions, which may not be related to this PR. Can you point me to a doc link that answers the following:

(a) Can the namespace-mapping be only provided to remote restores?
(b) Can the user create local snapshot and clone of a volume either in the same namespace as source pvc or a different namespace?

Also a suggestion for future reviews: going through the code, I see the flow for restore involves creating PVC and PV (or at least returning those specs to velero to go ahead and create them). Can you include in the PR details a link to the high-level steps during restore in this repository? Maybe a .md or a wiki page. This will help the reviewer to get into the context faster - especially when reviewing the code after a long time or to first-time reviewers.

Signed-off-by: mayank <mayank.patel@mayadata.io>

mynktl · 2020-04-29T07:42:47Z

@kmova
I've addressed the review comments, PTAL.

The changes look good. But I have few questions, which may not be related to this PR. Can you point me to a doc link that answers the following:

https://github.com/openebs/velero-plugin/blob/master/README.md

Also a suggestion for future reviews: going through the code, I see the flow for restore involves creating PVC and PV (or at least returning those specs to velero to go ahead and create them). Can you include in the PR details a link to the high-level steps during restore in this repository? Maybe a .md or a wiki page. This will help the reviewer to get into the context faster - especially when reviewing the code after a long time or to first-time reviewers.

https://docs.google.com/document/d/1-4WsM0AjLORb3lTCUUGyYOY_LNdTOATFesi7kTAr7SA/edit
This doc is internal for plugin core team. This should help new reviewer to understand the project.

kmova · 2020-04-29T08:53:03Z

The above doc link helps @mynktl if we can convert that into a design doc on GitHub/wiki and make sure we update it as new features are added will be really helpful!

mynktl requested review from kmova and vishnuitta April 22, 2020 15:57

mynktl added Enhancement issue/pr is a feature request On Hold PR doesn't have sufficient changes, all the scenarios are not covered or changes are requested labels Apr 22, 2020

Enabling remote restore in different namespace

bed99ce

Signed-off-by: mayank <mayank.patel@mayadata.io>

mynktl force-pushed the remote-ns branch from 8e2c945 to dc4a256 Compare April 24, 2020 14:32

mynktl removed the On Hold PR doesn't have sufficient changes, all the scenarios are not covered or changes are requested label Apr 24, 2020

mynktl added this to the 1.10 milestone Apr 24, 2020

mynktl added this to Pre-commits and Designs - Due: Apr 30 2020 in 1.10 Release Tracker - Due May 15th. Apr 24, 2020

mynktl force-pushed the remote-ns branch from 9173cd5 to 941686b Compare April 24, 2020 18:02

mayank added 2 commits April 25, 2020 00:18

updating README for remote restore in different namespace

4eee506

Signed-off-by: mayank <mayank.patel@mayadata.io>

adding changelog

890d7a6

Signed-off-by: mayank <mayank.patel@mayadata.io>

mynktl force-pushed the remote-ns branch 3 times, most recently from 84dd514 to 7c0dc23 Compare April 24, 2020 20:58

BDD for restore of remote backup in different namespace

711e955

Signed-off-by: mayank <mayank.patel@mayadata.io>

mynktl force-pushed the remote-ns branch from 7c0dc23 to 711e955 Compare April 24, 2020 22:01

fixing sanity test about restore in different ns

1b408bc

Signed-off-by: mayank <mayank.patel@mayadata.io>

kmova reviewed Apr 27, 2020

View reviewed changes

README.md Show resolved Hide resolved

kmova reviewed Apr 27, 2020

View reviewed changes

kmova reviewed Apr 28, 2020

View reviewed changes

addressing review comments

ebf265d

Signed-off-by: mayank <mayank.patel@mayadata.io>

kmova approved these changes Apr 29, 2020

View reviewed changes

kmova merged commit e52ad84 into openebs:master Apr 29, 2020

1.10 Release Tracker - Due May 15th. automation moved this from Pre-commits and Designs - Due: Apr 30 2020 to Done Apr 29, 2020

mynktl deleted the remote-ns branch May 5, 2020 14:38

shashank855 mentioned this pull request May 12, 2020

Restore of remote backup in new namespace openebs-archive/e2e-tests#351

Closed

This was referenced May 13, 2020

docs(changelog): changelog for v1.10.0-RC1 #88

Closed

docs(changelog): changelog for v1.10.0 #89

Merged

cherry-pick(docs,changelog): changelog for v1.10.0 #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remote-restore): enabling restore in different namespace #72

feat(remote-restore): enabling restore in different namespace #72

mynktl commented Apr 22, 2020 •

edited

mynktl commented Apr 22, 2020

mynktl commented Apr 24, 2020

kmova Apr 27, 2020

mynktl Apr 29, 2020

kmova Apr 28, 2020

mynktl Apr 29, 2020

kmova Apr 28, 2020

mynktl Apr 29, 2020

kmova Apr 28, 2020

mynktl Apr 29, 2020

kmova commented Apr 28, 2020

mynktl commented Apr 29, 2020 •

edited

kmova commented Apr 29, 2020

feat(remote-restore): enabling restore in different namespace #72

feat(remote-restore): enabling restore in different namespace #72

Conversation

mynktl commented Apr 22, 2020 • edited

mynktl commented Apr 22, 2020

mynktl commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmova commented Apr 28, 2020

mynktl commented Apr 29, 2020 • edited

kmova commented Apr 29, 2020

mynktl commented Apr 22, 2020 •

edited

mynktl commented Apr 29, 2020 •

edited