Restore progress reporting #3125

pranavgaikwad · 2020-12-02T20:43:06Z

This PR introduces progress reporting for Velero Restore.

Closes #21

The changes in this PR follow the design doc : https://github.com/vmware-tanzu/velero/blob/main/design/restore-progress.md

Summary of changes :

pkg/apis/velero/v1/restore.go : Introduces new field RestoreProgress in RestoreStatus
config/crd/bases/velero.io_restores.yaml & config/crd/crds/crds.go: Updates Restore CRD to include above field.
pkg/cmd/server/server.go : Passes velero client when creating a new Restore. The client is needed to send updates to RestoreStatus during an ongoing restore
pkg/restore/restore.go: Modifies restore flow as per https://github.com/vmware-tanzu/velero/blob/main/design/restore-progress.md#modifications-to-restorego.
pkg/cmd/util/output/restore_describer.go: Updates velero describe command to show restore progress information
pkg/restore/restore_test.go: Adds missing velero client in one of the Restore tests
pkg/util/kube/utils.go: Updates EnsureNamespaceExistsAndIsReady method to return an additional boolean to indicate whether a new Namespace was created or not. If new ns created, it will be added to list of restored items.
pkg/util/kube/utils_test.go: Updates test to verify value of the new boolean

jenting

Could we add the restore progress reporting at velero restore describe, same as the backup progress report at velero backup describe, something like:
```
Total items to be restored from:
Items restored from:
```
I create a backup resource, at backup reporting, the total items are 106. However, when I restore from the backup resource, the total times are 22, why? 🤔

pranavgaikwad · 2021-01-13T20:45:08Z

@jenting Thanks for pointing that out. I will take a look, my first guess is that maybe those resources were skipped during restore. I will confirm though.

pranavgaikwad · 2021-01-22T19:26:18Z

@jenting In my environment, I didn't see a huge difference in the number of items backed up vs restored:

apiVersion: velero.io/v1
kind: Backup
metadata:
  [...]
  labels:
    velero.io/storage-location: default
  name: bkp-1
  namespace: velero
  [...]
spec:
  [...]
  includedNamespaces:
  - test-bkp
  - test-bkp-2
status:
  [...]
  phase: Completed
  progress:
    itemsBackedUp: 389
    totalItems: 389
  startTimestamp: "2021-01-22T19:13:06Z"

apiVersion: velero.io/v1
kind: Restore
metadata:
  name: bkp-1-20210122141607
  namespace: velero
  [...]
spec:
  backupName: bkp-1
  excludedResources:
  - nodes
  - events
  - events.events.k8s.io
  - backups.velero.io
  - restores.velero.io
  - resticrepositories.velero.io
  hooks: {}
  includedNamespaces:
  - '*'
status:
  completionTimestamp: "2021-01-22T19:16:29Z"
  phase: Completed
  progress:
    itemsRestored: 387
    totalItems: 387
  startTimestamp: "2021-01-22T19:16:08Z"

The difference of 2 items in Backup and Restore comes from the fact that this PR doesn't include namespaces in the count of items restored. There is a way to update this function to return the number of ns it creates so that ns'es can be included in the count of items. I will look into that.

I'd be interested to know what kind of resources you tried to restore when you saw this mismatch in numbers.

jenting · 2021-01-26T14:12:32Z

I forgot the original test steps but I could provide a new one.

Steps:

Create a new namespace called nginx
Install nginx kubectl apply -f https://k8s.io/examples/application/deployment.yaml
Backup namespace nginx velero backup create backup-nginx --include-namespaces=nginx
Describe backup velero backup describe backup-nginx --details

Total items to be backed up:  31
Items backed up:              31

Resource List:
  apps/v1/Deployment:
    - nginx/nginx-deployment
  apps/v1/ReplicaSet:
    - nginx/nginx-deployment-66b6c48dd5
  v1/Event:
    - nginx/nginx-deployment-66b6c48dd5-9l4n9.165dccc49aae8161
    - nginx/nginx-deployment-66b6c48dd5-9l4n9.165dccc4b5fe1f95
    - nginx/nginx-deployment-66b6c48dd5-9l4n9.165dccc4bac83caf
    - nginx/nginx-deployment-66b6c48dd5-9l4n9.165dccc4c2609ca9
    - nginx/nginx-deployment-66b6c48dd5-9l4n9.165dcccc5f50a571
    - nginx/nginx-deployment-66b6c48dd5-czxj5.165dccd10a7cede5
    - nginx/nginx-deployment-66b6c48dd5-czxj5.165dccd126e35f36
    - nginx/nginx-deployment-66b6c48dd5-czxj5.165dccd13023360e
    - nginx/nginx-deployment-66b6c48dd5-czxj5.165dccd13700b6cf
    - nginx/nginx-deployment-66b6c48dd5-kwwmn.165dccd1083e8e3f
    - nginx/nginx-deployment-66b6c48dd5-kwwmn.165dccd12510c30b
    - nginx/nginx-deployment-66b6c48dd5-kwwmn.165dccd130b5c888
    - nginx/nginx-deployment-66b6c48dd5-kwwmn.165dccd136ed2aa2
    - nginx/nginx-deployment-66b6c48dd5-ph7tc.165dccc49ab2bd34
    - nginx/nginx-deployment-66b6c48dd5-ph7tc.165dccc4b9fc9cf2
    - nginx/nginx-deployment-66b6c48dd5-ph7tc.165dccc4c1536718
    - nginx/nginx-deployment-66b6c48dd5-ph7tc.165dccc4c8c0fc9a
    - nginx/nginx-deployment-66b6c48dd5-ph7tc.165dcccc5ec2d1b3
    - nginx/nginx-deployment-66b6c48dd5.165dccc49802b570
    - nginx/nginx-deployment-66b6c48dd5.165dccc4992f8858
    - nginx/nginx-deployment-66b6c48dd5.165dccd107609102
    - nginx/nginx-deployment-66b6c48dd5.165dccd10865dc6d
    - nginx/nginx-deployment.165dccc4964f2922
    - nginx/nginx-deployment.165dccd10652f80c
  v1/Namespace:
    - nginx
  v1/Pod:
    - nginx/nginx-deployment-66b6c48dd5-czxj5
    - nginx/nginx-deployment-66b6c48dd5-kwwmn
  v1/Secret:
    - nginx/default-token-qwsgw
  v1/ServiceAccount:
    - nginx/default

Delete namespace nginx
Restore from backup backup-nginx, velero restore create restore-nginx --from-backup=backup-nginx --include-namespaces=nginx
Check size kubectl get restores restore-nginx -o yaml

spec:
  backupName: backup-nginx
  excludedResources:
  - nodes
  - events
  - events.events.k8s.io
  - backups.velero.io
  - restores.velero.io
  - resticrepositories.velero.io
  hooks: {}
  includedNamespaces:
  - nginx
status:
  completionTimestamp: "2021-01-26T14:06:10Z"
  phase: Completed
  progress:
    itemsRestored: 6
    totalItems: 6
  startTimestamp: "2021-01-26T14:06:09Z"

pranavgaikwad · 2021-01-26T17:06:42Z

@jenting As the events are excluded from the restore, 24 instances of the v1/Event resource present in the backup were not restored and hence the difference in the number. Please also note that I am working on a way to include namespaces in the count.

jenting · 2021-01-27T01:35:40Z

@jenting As the events are excluded from the restore, 24 instances of the v1/Event resource present in the backup were not restored and hence the difference in the number. Please also note that I am working on a way to include namespaces in the count.

Cool, thank you for the explanation.

I'd love that the velero backup describe displays the counts on total items to be restored and current restored items.

nrb

I think I understand how this is all working, but I'd like some more comments, as the restore code can be a little tricky :)

Overall I think it makes sense, though, and I appreciate your efforts here!

And finally, it needs a rebase before we'll be able to merge.

pkg/restore/restore.go

nrb · 2021-02-03T20:31:23Z

pkg/restore/restore.go

+	return
+}
+
+func (ctx *restoreContext) getSelectedRestoreableItems(resource, targetNamespace, originalNamespace string, items []string) (res restoreableResource, warnings Result, errs Result) {


If I'm reading this code correctly, getOrderedResourceCollection is very similar to the previous restore loop in that it's applying the selection criteria like namespace, and includes/excludes. However, the change here is that this function, getSelectedRestoreableItems then groups those resources by namespace and applies the Kubernetes label selection. It replaces some of the functionality of the restoreResource function, which should be removed since it's no longer relevant with this PR, from what I can tell.

Do I have that right? If so please add a comment to clarify that's what this function is doing, so we can keep track of it.

Also, based on this code it looks like we're unmarshaling twice.

@nrb that is correct. We are unmarshling twice. Unmarshling is needed at first to apply selection criteria, and then finally to actually restore the object. Another alternative was to keep entire object in-memory until it's restored which isnt very attractive.

and your'e correct about the duplicate code from restoreResource function. thanks for pointing that out. will clean it up

Another alternative was to keep entire object in-memory until it's restored which isnt very attractive.

Right, that is the trade we'd have to make. I honestly think we will end up doing this at some point, though, as we'd like to form a graph of objects. For now I think this will work and we can revisit the unmarshalling mechanisms in the future.

changelogs/unreleased/3125-pranavgaikwad

pranavgaikwad · 2021-02-05T19:58:34Z

@jenting This commit addresses your feedback about velero describe restore command: 35f8e27

pranavgaikwad · 2021-02-05T22:00:51Z

@nrb when you have a moment, request you to take a look at new code I added to include namespaces in the total count. That change updates EnsureNamespaceExistsAndIsReady() util method to return whether it creates a new ns or not. If a new ns is created, it is added to the list of restored items.

nrb · 2021-02-09T21:23:19Z

@pranavgaikwad Looks like there was a conflict with the colorized output PR that's affecting this.

pranavgaikwad · 2021-02-10T23:15:18Z

@pranavgaikwad Looks like there was a conflict with the colorized output PR that's affecting this.

Sure will rebase again.

pkg/restore/restore.go

jenting · 2021-02-17T02:18:59Z

pkg/restore/restore.go

+				if namespace != "" && !existingNamespaces.Has(selectedItem.targetNamespace) {
+					logger := ctx.log.WithField("namespace", namespace)
+					ns := getNamespace(logger, archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", namespace), selectedItem.targetNamespace)
+					if _, nsCreated, err := kube.EnsureNamespaceExistsAndIsReady(ns, ctx.namespaceClient, ctx.resourceTerminatingTimeout); err != nil {
+						errs.AddVeleroError(err)
+						continue
+					} else {
+						// add the newly created namespace to the list of restored items
+						if nsCreated {
+							itemKey := velero.ResourceIdentifier{
+								GroupResource: kuberesource.Namespaces,
+								Namespace:     ns.Namespace,
+								Name:          ns.Name,
+							}
+							ctx.restoredItems[itemKey] = struct{}{}
+						}
+					}


This block code looks similar to

velero/pkg/restore/restore.go

Lines 794 to 811 in b527a8a

// if the namespace scoped resource should be restored, ensure that the namespace into

// which the resource is being restored into exists.

// This is the *remapped* namespace that we are ensuring exists.

nsToEnsure := getNamespace(ctx.log, archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", obj.GetNamespace()), namespace)

if _, nsCreated, err := kube.EnsureNamespaceExistsAndIsReady(nsToEnsure, ctx.namespaceClient, ctx.resourceTerminatingTimeout); err != nil {

errs.AddVeleroError(err)

return warnings, errs

} else {

// add the newly created namespace to the list of restored items

if nsCreated {

itemKey := velero.ResourceIdentifier{

GroupResource: kuberesource.Namespaces,

Namespace: nsToEnsure.Namespace,

Name: nsToEnsure.Name,

}

ctx.restoredItems[itemKey] = struct{}{}

}

}

And the code logic would run twice. I'm not sure what's the difference between these two or could we merge them into one?

@jenting This duplication is required because restoreItem() can be called for additional items returned by plugins. The recursive call in that case only triggers restoreItem() function alone and not the rest of the previous logic.

jenting

LGTM

jenting · 2021-02-19T00:18:39Z

@pranavgaikwad I just realized there is a conflict file. Please resolve it if you have time. 🙇

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

carlisia · 2021-02-25T02:01:05Z

I'll be reviewing this tomorrow.

jenting

LGTM, thank you @pranavgaikwad

nrb

🎉

* restore progress reporting Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * add restore statistics to describe restore Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * address feedback, include namespaces in the count Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

rishi-anand · 2021-06-08T04:27:59Z

@pranavgaikwad, @jenting, @nrb, @carlisia I have seen this issue again in my cluster which is on v1.6.0

I have restored in same cluster with preserveNodePort: true and velero was throwing error msg about nodePort already in use. Later, I changed preserveNodePort: false but then restore crd says all items restored but phase is InProgress from a very long time. I tried restarting velero pod but that didn't fixed the issue.

apiVersion: velero.io/v1
kind: Restore
metadata:
  creationTimestamp: "2021-06-08T01:57:44Z"
  name: backup-1
  namespace: velero
  resourceVersion: "13816289"
  selfLink: /apis/velero.io/v1/namespaces/velero/restores/backup-1
  uid: c8cc70b3-4b7a-4bde-aadb-da5856aeb259
spec:
  backupName: backup-1
  excludedResources:
  - nodes
  - events
  - events.events.k8s.io
  - backups.velero.io
  - restores.velero.io
  - resticrepositories.velero.io
  hooks: {}
  includeClusterResources: true
  preserveNodePorts: true
  restorePVs: true
status:
  phase: InProgress
  progress:
    itemsRestored: 1037
    totalItems: 1037
  startTimestamp: "2021-06-08T01:57:44Z"

* restore progress reporting Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * add restore statistics to describe restore Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com> * address feedback, include namespaces in the count Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

pranavgaikwad force-pushed the restore-progress branch 4 times, most recently from 2d764c0 to 0aeba45 Compare December 16, 2020 23:07

pranavgaikwad marked this pull request as ready for review December 16, 2020 23:09

pranavgaikwad changed the title ~~WIP - Restore progress reporting~~ Restore progress reporting Dec 16, 2020

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, jenting, nrb and zubron December 16, 2020 23:09

github-actions bot assigned pranavgaikwad Dec 16, 2020

jenting suggested changes Jan 13, 2021

View reviewed changes

pranavgaikwad force-pushed the restore-progress branch from 0aeba45 to b30819d Compare January 22, 2021 17:45

github-actions bot requested a review from jenting January 22, 2021 17:45

nrb added kind/release-blocker Must fix issues for the coming release (milestone) P2 - Long-term important labels Feb 3, 2021

nrb suggested changes Feb 3, 2021

View reviewed changes

pranavgaikwad force-pushed the restore-progress branch from b30819d to c09035c Compare February 5, 2021 15:23

github-actions bot requested a review from nrb February 5, 2021 15:24

pranavgaikwad force-pushed the restore-progress branch 2 times, most recently from 0401254 to a5b5a18 Compare February 5, 2021 19:56

pranavgaikwad force-pushed the restore-progress branch from a5b5a18 to 7db5a2d Compare February 9, 2021 14:29

github-actions bot requested a review from nrb February 9, 2021 14:29

nrb added this to the v1.6.0 milestone Feb 10, 2021

pranavgaikwad force-pushed the restore-progress branch from 7db5a2d to b527a8a Compare February 12, 2021 14:35

github-actions bot added has-changelog has-unit-tests labels Feb 12, 2021

jenting reviewed Feb 17, 2021

View reviewed changes

jenting previously approved these changes Feb 18, 2021

View reviewed changes

pranavgaikwad added 3 commits February 22, 2021 16:54

restore progress reporting

47baa56

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

add restore statistics to describe restore

fee7375

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

address feedback, include namespaces in the count

b4b7890

Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>

pranavgaikwad dismissed jenting’s stale review via b4b7890 February 22, 2021 21:58

pranavgaikwad force-pushed the restore-progress branch from b527a8a to b4b7890 Compare February 22, 2021 21:58

github-actions bot requested a review from jenting February 22, 2021 21:58

dsu-igeek added this to In progress in v1.6.0 via automation Feb 22, 2021

jenting approved these changes Mar 3, 2021

View reviewed changes

nrb approved these changes Mar 4, 2021

View reviewed changes

v1.6.0 automation moved this from In progress to Reviewer approved Mar 4, 2021

nrb merged commit c46fe71 into vmware-tanzu:main Mar 4, 2021

v1.6.0 automation moved this from Reviewer approved to Done Mar 4, 2021

This was referenced Mar 12, 2021

API groups e2e tests remove controllers #3564

Merged

Restore progress reporting bug fix #3583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore progress reporting #3125

Restore progress reporting #3125

pranavgaikwad commented Dec 2, 2020 •

edited

jenting left a comment

pranavgaikwad commented Jan 13, 2021

pranavgaikwad commented Jan 22, 2021 •

edited

jenting commented Jan 26, 2021 •

edited

pranavgaikwad commented Jan 26, 2021 •

edited

jenting commented Jan 27, 2021

nrb left a comment

nrb Feb 3, 2021

pranavgaikwad Feb 5, 2021

pranavgaikwad Feb 5, 2021

nrb Feb 5, 2021

pranavgaikwad commented Feb 5, 2021

pranavgaikwad commented Feb 5, 2021

nrb commented Feb 9, 2021

pranavgaikwad commented Feb 10, 2021

jenting Feb 17, 2021

pranavgaikwad Feb 18, 2021

jenting left a comment

jenting commented Feb 19, 2021

carlisia commented Feb 25, 2021

jenting left a comment

nrb left a comment

rishi-anand commented Jun 8, 2021

	// if the namespace scoped resource should be restored, ensure that the namespace into
	// which the resource is being restored into exists.
	// This is the remapped namespace that we are ensuring exists.
	nsToEnsure := getNamespace(ctx.log, archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", obj.GetNamespace()), namespace)
	if _, nsCreated, err := kube.EnsureNamespaceExistsAndIsReady(nsToEnsure, ctx.namespaceClient, ctx.resourceTerminatingTimeout); err != nil {
	errs.AddVeleroError(err)
	return warnings, errs
	} else {
	// add the newly created namespace to the list of restored items
	if nsCreated {
	itemKey := velero.ResourceIdentifier{
	GroupResource: kuberesource.Namespaces,
	Namespace: nsToEnsure.Namespace,
	Name: nsToEnsure.Name,
	}
	ctx.restoredItems[itemKey] = struct{}{}
	}
	}

Restore progress reporting #3125

Restore progress reporting #3125

Conversation

pranavgaikwad commented Dec 2, 2020 • edited

jenting left a comment

Choose a reason for hiding this comment

pranavgaikwad commented Jan 13, 2021

pranavgaikwad commented Jan 22, 2021 • edited

jenting commented Jan 26, 2021 • edited

pranavgaikwad commented Jan 26, 2021 • edited

jenting commented Jan 27, 2021

nrb left a comment

Choose a reason for hiding this comment

nrb Feb 3, 2021

Choose a reason for hiding this comment

pranavgaikwad Feb 5, 2021

Choose a reason for hiding this comment

pranavgaikwad Feb 5, 2021

Choose a reason for hiding this comment

nrb Feb 5, 2021

Choose a reason for hiding this comment

pranavgaikwad commented Feb 5, 2021

pranavgaikwad commented Feb 5, 2021

nrb commented Feb 9, 2021

pranavgaikwad commented Feb 10, 2021

jenting Feb 17, 2021

Choose a reason for hiding this comment

pranavgaikwad Feb 18, 2021

Choose a reason for hiding this comment

jenting left a comment

Choose a reason for hiding this comment

jenting commented Feb 19, 2021

carlisia commented Feb 25, 2021

jenting left a comment

Choose a reason for hiding this comment

nrb left a comment

Choose a reason for hiding this comment

rishi-anand commented Jun 8, 2021

pranavgaikwad commented Dec 2, 2020 •

edited

pranavgaikwad commented Jan 22, 2021 •

edited

jenting commented Jan 26, 2021 •

edited

pranavgaikwad commented Jan 26, 2021 •

edited