add delete backup cmd using finalizer and simplify GC process #252

skriss · 2017-12-15T01:03:38Z

Signed-off-by: Steve Kriss steve@heptio.com

Fixes #212
Fixes #77

@ncdc PTAL and provide feedback before I fix unit tests. I've manually tested successfully.

ncdc

First pass. Need to do another pass to review the actual gc changes

ncdc · 2017-12-15T14:58:25Z

pkg/cmd/cli/delete/delete.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2017 Heptio Inc.


project ark contribs

ncdc · 2017-12-15T14:58:57Z

pkg/controller/backup_controller.go

@@ -229,6 +229,9 @@ func (controller *backupController) processBackup(key string) error {
 	// set backup version
 	backup.Status.Version = backupVersion

+	// add GC finalizer
+	backup.Finalizers = append(backup.Finalizers, gcFinalizer)


Only add if it doesn't exist

ncdc · 2017-12-15T14:59:31Z

pkg/controller/gc_controller.go

-var _ Interface = &gcController{}
+	backupInformer.Informer().AddEventHandler(
+		cache.ResourceEventHandlerFuncs{
+			UpdateFunc: func(old interface{}, new interface{}) {


Let's make this a real function so we can unit test it

Since new is a reserved keyword, it might be better not to use it. Looking at the kube source, I've seen

func(oldObj, newObj interface{})

func(old, cur interface{})

I think I prefer the first form.

Also since you're not using old, you can just do func(_, newObj interface{})

ncdc · 2017-12-15T15:10:32Z

pkg/controller/gc_controller.go

+				}
+				log.Debugf("Backup has finalizers %s", backup.Finalizers)
+
+				for x, finalizer := range backup.Finalizers {


I'd extract this to a function that just returns a bool. No need for idx.

And then maybe make another helper that returns a new slice w/the finalizer removed. See e.g. https://github.com/kubernetes/kubernetes/blob/722cb7a758d4678e7ed5c4f09ded99d6bc5b0b4c/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/helpers.go#L109-L118

ncdc · 2017-12-15T15:13:12Z

pkg/controller/gc_controller.go

+					return
+				}
+
+				// no more finalizers: we don't need to issue another delete


Shouldn't this be "There are other finalizers, so we won't issue a delete as other controllers still need to process" along with a len >= 1 check?

Not 100% sure on how this is supposed to work. I'm going off of https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/#finalizers which says:

...Each controller then removes its finalizer from the list and issues the delete request again. This request only deletes the object if the list of finalizers is now empty, meaning all finalizers are done.

However, the behavior I was seeing was that if I issued a delete after removing the last/only finalizer, it would fail with object not found; the object appeared to be deleted once the last finalizer had been removed.

ncdc

Note: CRD finalizers require kube 1.7.5 or newer.

ncdc · 2017-12-20T00:28:26Z

pkg/cmd/cli/delete/delete.go

+	backupCommand.Aliases = []string{"backups"}
+
+	c.AddCommand(
+		backupCommand,


Wire in schedules and restores too? Or in a follow-up?

ncdc · 2017-12-20T00:38:46Z

pkg/controller/gc_controller.go

+		log.WithError(err).Error("Error marshaling finalizers patch")
+	}
+
+	upd, err := c.backupClient.Backups(api.DefaultNamespace).Patch(backup.Name, types.MergePatchType, patchBytes)


use backup.Namespace instead of api.DefaultNamespace so we don't have to come back later and change it (to support an arbitrary namespace for ark)

We might want to pull in client-go's retry package and use retry.RetryOnConflict. Or we could just stop here on conflict and let the next run of the loop try again.

ncdc · 2017-12-20T00:39:19Z

pkg/controller/gc_controller.go

-	c.processBackups()
-}
+	now := c.clock.Now()
+	c.logger.Info("Garbage-collecting backups that have expired as of now")


I think we can remove "that have expired as of now" 😄

ncdc · 2017-12-20T00:40:08Z

pkg/controller/gc_controller.go

+	for _, backup := range backups {
+		log := c.logger.WithField("backup", kube.NamespaceAndName(backup))
+		if backup.Status.Expiration.Time.After(now) {
+			log.Info("Backup has not expired yet, skipping")


Do you think we need this at the info level still?

ncdc · 2017-12-20T00:40:29Z

pkg/controller/gc_controller.go

+		// since backups have a finalizer, this will actually have the effect of setting a deletionTimestamp and calling
+		// an update. The update will be handled by this controller and will result in a deletion of the obj storage
+		// files and the API object.
+		if err := c.backupClient.Backups(api.DefaultNamespace).Delete(backup.Name, &metav1.DeleteOptions{}); err != nil {


backup.Namespace

ncdc · 2017-12-20T15:32:52Z

pkg/controller/gc_controller.go

+		return
+	}
+
+	// no more finalizers: we don't need to issue another delete


Talked to api machinery: no need to issue additional deletes. The only action we need to perform is the patch to remove our finalizer.

ncdc · 2017-12-20T15:41:16Z

pkg/controller/gc_controller.go

+	}
+
+	log.Infof("Garbage-collecting backup")
+	if err := c.garbageCollect(backup, log); err != nil {


If this call errors once, it's entirely possible (likely even?) that it will continue to error the next time we try it for the same backup. It might be worth considering switching to a rate limited queue (like our other controllers), or keeping track of how many times we've tried to delete (via annotations) and stopping after some fixed limit.

Do you think this is PR-blocking?

ncdc · 2017-12-20T15:46:01Z

pkg/controller/gc_controller_test.go

+			expectedObjectStorageDeletions: sets.NewString("backup-1"),
+		},
+		{
+			name: "if snapshot deletion fails, don't delete kube backup and return error",


We aren't deleting the kube backup from garbageCollect any more

ncdc · 2017-12-20T15:46:46Z

pkg/controller/gc_controller_test.go

 		snapshots                      sets.String
 		backupFiles                    sets.String
 		backupMetadataFiles            sets.String
 		restores                       []*api.Restore
+		nilSnapshotService             bool
+		expectedErr                    bool
 		expectedRestoreDeletes         []string
 		expectedBackupDelete           string


not used any more by this test

ncdc · 2017-12-20T15:48:50Z

pkg/controller/gc_controller_test.go

 		snapshots                      sets.String
 		backupFiles                    sets.String
 		backupMetadataFiles            sets.String
 		restores                       []*api.Restore
+		nilSnapshotService             bool
+		expectedErr                    bool
 		expectedRestoreDeletes         []string
 		expectedBackupDelete           string
 		expectedSnapshots              sets.String
 		expectedObjectStorageDeletions sets.String
 	}{
 		{


We may just want to convert this to a few test cases:

nil snapshot server, backup has snapshots, return error (ensure that no DeleteSnapshot, DeleteBackupDir, restore Delete calls are issued (handled by mock.AssertExpectations etc))

happy path

do as much as you can (some DeleteSnapshot errors & successes, DeleteBackupDir error & success, some restore Delete errors & successes) and return error(s) encountered

ncdc · 2017-12-20T16:14:14Z

Need unit test coverage on handleFinalizer

ncdc · 2017-12-20T19:41:50Z

pkg/controller/gc_controller.go

-	upd, err := c.backupClient.Backups(api.DefaultNamespace).Patch(backup.Name, types.MergePatchType, patchBytes)
-	if err != nil {
+	if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		_, err = c.backupClient.Backups(backup.Namespace).Patch(backup.Name, types.MergePatchType, patchBytes)


If we're going to use RetryOnConflict, we have to regenerate the patchMap in the anonymous func, based on an updated copy of the backup (either from the lister, or using a real client to fetch the data from the apiserver). Otherwise we'll keep sending the same patch and getting the same conflict each time. That's why I was suggesting it might be easier to just let the next iteration handle it.

skriss · 2017-12-20T23:17:27Z

addressed everything except the rate-limited queue comment.

ncdc · 2017-12-21T15:03:25Z

This also fixes #77, doesn't it?

ncdc · 2017-12-21T15:18:17Z

pkg/controller/gc_controller.go

+	}
+	log.Debugf("Backup has finalizers %s", backup.Finalizers)
+
+	// only process the GC finalizer when it's the first item


I went through the kube code and can't find any place where it only acts on a finalizer if it's the first element in the slice. Let's go back to proceeding if the slice contains our finalizer, regardless of position. Sorry for the flip-flop.

np, easy to switch back.

ncdc · 2017-12-21T15:21:49Z

pkg/controller/gc_controller.go

-			continue
+			restoreLog.Info("Deleting restore referencing backup")
+			if err := c.restoreClient.Restores(restore.Namespace).Delete(restore.Name, &metav1.DeleteOptions{}); err != nil {
+				restoreLog.WithError(errors.WithStack(err)).Error("Error deleting restore")


Do you think this should just be an error so we consider the GC of the backup a failure?

eh, in my opinion no -- I'm not too worried about orphaned restores (since they don't cost any cloud resources), plus that would have the result of leaving the backup API object around, despite its tarball/etc. potentially having been deleted, which could make a user believe it's still restorable.

skriss · 2017-12-21T18:15:41Z

Re: #77 , it doesn't address it directly but does mean that Ark won't orphan files in obj storage. However, if a user manually deletes a backup API obj, or manually adds files to obj storage, Ark won't be able to clean them up.

Actually, if a user manually deletes an API obj, it'll get re-synced so that case is fine. So the only case we don't handle is files manually added to the bucket outside of Ark. I don't think we need to handle that.

ncdc · 2017-12-21T18:25:25Z

So the only case we don't handle is files manually added to the bucket outside of Ark

I'm pretty sure BackupService.DeleteBackupDir deletes everything

skriss · 2017-12-21T18:34:12Z

everything within a backup dir, yes. I think we can consider #77 addressed and if some other related issue pops up we can create a new one.

Signed-off-by: Steve Kriss <steve@heptio.com>

nrb · 2017-12-21T21:07:38Z

Probably worth making a release note for the version, though I don't think we currently have a process for doing so til release time.

ncdc · 2017-12-22T18:12:32Z

pkg/cmd/cli/backup/delete.go

+			cmd.CheckError(err)
+
+			if !serverVersion.AtLeast(controller.MinVersionForDelete) {
+				cmd.CheckError(fmt.Errorf("this command requires the Kubernetes server version to be at least %s", controller.MinVersionForDelete))


Do you want to do errors.Errorf for consistency with the rest of the code base? (Apparently we're still using it in pkg/restore/restore.go when adding warnings & errors to the restore result.)

ncdc · 2017-12-22T18:23:34Z

1 minor q

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss · 2017-12-22T18:27:18Z

sure, updated.

skriss · 2018-01-02T19:23:35Z

@ncdc did you have anything further here? this is ready to merge from my perspective.

ncdc · 2018-01-02T19:25:59Z

Nope, merging

…ore-priority Add vsb to restore priority

skriss requested review from ncdc and nrb December 15, 2017 01:03

ncdc reviewed Dec 15, 2017

View reviewed changes

skriss force-pushed the delete-backup branch 4 times, most recently from 4e95706 to 9ccb14e Compare December 15, 2017 21:16

skriss changed the title ~~[WIP] add delete backup cmd using finalizer and simplify GC process~~ add delete backup cmd using finalizer and simplify GC process Dec 18, 2017

skriss added the enhancement label Dec 18, 2017

skriss added this to the v0.7.0 milestone Dec 18, 2017

skriss mentioned this pull request Dec 19, 2017

BackupController: do as much as possible #250

Merged

ncdc suggested changes Dec 20, 2017

View reviewed changes

ncdc reviewed Dec 20, 2017

View reviewed changes

skriss force-pushed the delete-backup branch from 2361337 to af71115 Compare December 20, 2017 23:09

ncdc suggested changes Dec 21, 2017

View reviewed changes

skriss force-pushed the delete-backup branch from af71115 to 54eb048 Compare December 21, 2017 18:22

skriss added 2 commits December 21, 2017 10:47

add delete backup cmd using finalizer and simplify GC process

f512379

Signed-off-by: Steve Kriss <steve@heptio.com>

include restore & schedule under ark delete

8e5feec

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss force-pushed the delete-backup branch from 54eb048 to 8e5feec Compare December 21, 2017 18:49

ncdc reviewed Dec 22, 2017

View reviewed changes

disable GC and backup deletion if Kubernetes is less than v1.7.5

1c97478

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss force-pushed the delete-backup branch from bf25e54 to 1c97478 Compare December 22, 2017 18:25

ncdc approved these changes Dec 22, 2017

View reviewed changes

ncdc merged commit 6b0b637 into vmware-tanzu:master Jan 2, 2018

skriss deleted the delete-backup branch January 2, 2018 19:27

shubham-pampattiwar pushed a commit to shubham-pampattiwar/velero that referenced this pull request Apr 6, 2023

Merge pull request vmware-tanzu#252 from shubham-pampattiwar/add-rest…

e27e606

…ore-priority Add vsb to restore priority

add delete backup cmd using finalizer and simplify GC process #252

add delete backup cmd using finalizer and simplify GC process #252

Conversation

skriss commented Dec 15, 2017 • edited Loading

ncdc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc left a 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

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

ncdc commented Dec 20, 2017

Choose a reason for hiding this comment

skriss commented Dec 20, 2017

ncdc commented Dec 21, 2017

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

skriss commented Dec 21, 2017 • edited Loading

ncdc commented Dec 21, 2017

skriss commented Dec 21, 2017

nrb commented Dec 21, 2017

Choose a reason for hiding this comment

ncdc commented Dec 22, 2017

skriss commented Dec 22, 2017

skriss commented Jan 2, 2018

ncdc commented Jan 2, 2018

skriss commented Dec 15, 2017 •

edited

Loading

skriss commented Dec 21, 2017 •

edited

Loading