Switch from finalizer to DeleteBackupRequest for deleting backups #383

ncdc · 2018-03-15T14:17:14Z

We ran into a lot of problems using a finalizer on the backup to allow
the Ark server to clean up all associated backup data when deleting a
backup.

Users also found it less than desirable that deleting the heptio-ark
namespace resulted in all the backup data being deleted.

This removes the finalizer and replaces it with an explicit
DeleteBackupRequest that is created as a means of requesting the
deletion of a backup and all its associated data. This is what ark backup delete does.

If you use kubectl to delete a backup or to delete the heptio-ark
namespace, this no longer deletes associated backups. Additionally, as
long as the heptio-ark namespace still exists, the Ark server's
BackupSyncController will continually sync backups into the heptio-ark
namespace from object storage.

TODO:

Modify ark backup describe to show DeleteBackupRequest status if relevant
Write one-shot code to run at server startup to remove our old backup finalizer if it's there
Update unit tests

Fixes #375
Fixes #376
Fixes #358

ncdc · 2018-03-15T14:17:41Z

@skriss @nrb I haven't updated tests yet - wanted to publish this now so you can start reviewing the logic in the delete and gc controllers.

ncdc · 2018-03-15T14:21:42Z

I also think it would be worth it to add some code that runs once each time the server starts, removing the ark GC finalizer from any backups that have it.

ncdc · 2018-03-15T14:33:52Z

pkg/backup/delete_helpers.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func NewDeleteBackupRequest(name string) *v1.DeleteBackupRequest {


TODO: godoc

ncdc · 2018-03-15T14:35:41Z

pkg/controller/backup_deletion_controller.go

+
+	deleteBackupRequestInformer.Informer().AddEventHandler(
+		cache.ResourceEventHandlerFuncs{
+			AddFunc: c.enqueue,


TODO: need an UpdateFunc (e.g. request is created and modified before the controller sees it, status is still ""/New)

ncdc · 2018-03-15T14:37:10Z

pkg/controller/gc_controller.go

 		},
+		syncPeriod,


TODO: switch to resyncFunc in genericController

ncdc · 2018-03-15T14:38:09Z

pkg/controller/generic_controller.go

+	name             string
+	queue            workqueue.RateLimitingInterface
+	logger           logrus.FieldLogger
+	syncHandler      func(key string) error


TODO: add resyncFunc

nrb · 2018-03-15T15:51:00Z

pkg/controller/backup_deletion_controller.go

+	}
+
+	// Try to delete restores
+	log.Info("Removing restores")


IMO we should remove restores when explicitly deleting a backup, but I have seen others mention that retaining restores as a matter of record would be beneficial. The restores would then be cleaned up via the GC controller. I think that's also a reasonable approach, wdyt?

I don't think this should be an option, though, we should only do one or the other.

I'm seeing now that the GC controller uses the BackupDeleteRequests to do a delete, so I suppose the 2nd approach would require shuffling some logic around. I'm fine with it as is.

skriss

initial pass

skriss · 2018-03-15T15:21:49Z

Gopkg.toml

@@ -31,6 +31,11 @@ required = [
  "k8s.io/code-generator/cmd/informer-gen",
 ]

+[prune]


skriss · 2018-03-15T15:30:06Z

pkg/apis/ark/v1/delete_backup_request.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// DeleteBackupRequestList is a list of DeleteBackupRequests.
+type DeleteBackupRequestList struct {


question - why isn't there a code-generator for the *List types?

Probably not really worth the effort

skriss · 2018-03-15T15:35:13Z

pkg/backup/delete_helpers.go

+	return &v1.DeleteBackupRequest{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: name + "-",
+			Labels: map[string]string{


Is the backup name stored in a label in addition to the spec just for ease of filtering?

Yes, so we can delete all requests for backup x

skriss · 2018-03-15T15:38:46Z

pkg/cmd/cli/backup/delete.go

 }

 func (o *DeleteOptions) Validate(c *cobra.Command, args []string, f client.Factory) error {
 	if len(args) != 1 {
 		return errors.New("you must specify only one argument, the backup's name")
 	}

-	kubeClient, err := f.KubeClient()


do we want to validate that the specified backup exists here?

skriss · 2018-03-15T15:48:45Z

pkg/controller/backup_deletion_controller.go

+func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) error {
+	log := c.logger.WithFields(logrus.Fields{
+		"namespace": req.Namespace,
+		"name":      req.Spec.BackupName,


should the name field be req.Spec.Name? Maybe add that and the backup name as fields?

skriss · 2018-03-15T16:08:51Z

pkg/controller/generic_controller.go

+
+	}()
+
+	c.logger.Info("Starting %s", c.name)


would probably be useful to add the controller name as a field to the c.logger

skriss · 2018-03-15T16:17:15Z

README.md

@@ -135,28 +135,6 @@ ark restore describe <RESTORE_NAME>

 For more information, see [the debugging information][18].

-### Clean up


maybe leave this section, and specify that you can just delete the NS, and note that backups/snapshots will not be deleted

Ok. I'll make it clearer that this is optional and only if you want to remove stuff.

skriss · 2018-03-15T16:17:52Z

docs/azure-config.md

@@ -115,7 +115,7 @@ Now you need to create a Secret that contains all the seven environment variable

 ```bash
 kubectl create secret generic cloud-credentials \
-    --namespace <ARK_SERVER_NAMESPACE> \
+    --namespace <ARK_SERVER_ARK_NAMESPACE> \


ARK_NAMESPACE

skriss · 2018-03-15T16:19:28Z

docs/ibm-config.md

@@ -47,7 +47,7 @@ Create a Secret. In the directory of the credentials file you just created, run:

 ```bash
 kubectl create secret generic cloud-credentials \
-    --namespace <ARK_SERVER_NAMESPACE> \
+    --namespace <ARK_SERVER_ARK_NAMESPACE> \


ARK_NAMESPACE

skriss · 2018-03-15T16:22:36Z

docs/namespace.md


-To run the server in another namespace, you edit the relevant files, changing `heptio-ark-server` to
+To run the server in another namespace, you edit the relevant files, changing `heptio-ark` to


One thing not explicitly called out here is that the cloud-credentials secret also needs to be created in the alternate namespace. It's templatized in each cloud-provider's walkthrough, but maybe worth just referencing it here too.

skriss · 2018-03-15T16:30:25Z

pkg/controller/backup_deletion_controller.go

+			r.Status.Phase = v1.DeleteBackupRequestPhaseProcessed
+			r.Status.Errors = []string{"unable to delete backup because it includes PV snapshots and Ark is not configured with a PersistentVolumeProvider"}
+		})
+		if err != nil {


return err

skriss · 2018-03-16T17:19:19Z

Some observations from testing:

all of the normal-case functionality looks good (removal of finalizer from old backups, ark backup delete, garbage-collection)
the warning message when running ark backup delete foo without the --confirm flag is misleading (references force-deletion, orphaning cloud resources)
in Azure, if I manually delete a snapshot before deleting a backup that references it, Ark doesn't error and deletes the backup (I think the outcome is OK, but I would've expected an error. It may be that the Azure API doesn't return an error if you try to delete a non-existent snapshot?)
you were going to add a check for non-existent backups in the command code, looks like that's not there yet

when running ark backup delete for a non-existent backup, the Ark server log has this error:

time="2018-03-16T16:57:58Z" level=error msg="Error in syncHandler, re-adding item to queue" controller=backup-deletion error="error patching DeleteBackupRequest: Object 'Kind' is missing in '{\"object\":{\"apiVersion\":\"ark.heptio.com/v1\",\"kind\":\"DeleteBackupRequest\",\"metadata\":{\"clusterName\":\"\",\"creationTimestamp\":\"2018-03-16T16:57:58Z\",\"deletionGracePeriodSeconds\":null,\"deletionTimestamp\":null,\"generateName\":\"nginx-\",\"generation\":0,\"labels\":{\"ark.heptio.com/backup-name\":\"nginx\"},\"name\":\"nginx-3hp7g\",\"namespace\":\"heptio-ark\",\"resourceVersion\":\"1049234\",\"selfLink\":\"\",\"uid\":\"2ddbb8b4-293b-11e8-bf7d-0a58ac1f100c\"},\"spec\":{\"backupName\":\"nginx\"},\"status\":{\"errors\":[\"backup not found\"],\"phase\":\"Processed\"}}}'" error.file="backup_deletion_controller.go:259" error.function="(*backupDeletionController).patchDeleteBackupRequest" key=heptio-ark/nginx-3hp7g logSource="pkg/controller/generic_controller.go:136"

if I take a backup with a snapshot, then remove the persistentVolumeProvider from config, then try to delete the backup, the failed delete request doesn't show up in ark backup describe -- looks like this is because we only show the associated requests if the backup is in phase Deleting, which the backup is not in this case
if I try to delete a non-existent backup foo, then later I actually create a backup foo, when I go to delete backup foo I now see the failed backup requests from prior to the backup existing in ark backup describe. Maybe they should be linked by UID rather than name?

ncdc · 2018-03-16T18:25:18Z

the warning message when running ark backup delete foo without the --confirm flag is misleading (references force-deletion, orphaning cloud resources)

Fixed

in Azure, if I manually delete a snapshot before deleting a backup that references it, Ark doesn't error and deletes the backup (I think the outcome is OK, but I would've expected an error. It may be that the Azure API doesn't return an error if you try to delete a non-existent snapshot?)

I'm not sure an error is appropriate here. Your intent is to delete a backup and all its associated data. If we can't find some of the associated data, that's not really an issue, is it? I would expect it to be a problem if you wanted to restore, of course.

Maybe we need something as part of the backup sync controller that periodically verifies the integrity of a backup's associated data?

you were going to add a check for non-existent backups in the command code, looks like that's not there yet

That's pushed up now.

when running ark backup delete for a non-existent backup, the Ark server log has this error:

I manually created a DeleteBackupRequest with what should be the same as what ark backup delete does (since I now have the check for nonexistent backups), and I can't reproduce the patch error. Would you have some time to try to reproduce & debug?

if I take a backup with a snapshot, then remove the persistentVolumeProvider from config, then try to delete the backup, the failed delete request doesn't show up in ark backup describe -- looks like this is because we only show the associated requests if the backup is in phase Deleting, which the backup is not in this case

So maybe always show if at least 1 has errors?

if I try to delete a non-existent backup foo, then later I actually create a backup foo, when I go to delete backup foo I now see the failed backup requests from prior to the backup existing in ark backup describe. Maybe they should be linked by UID rather than name?

I'll link on both name and uid.

This brings up something I haven't addressed yet - there's no expiration on DeleteBackupRequests nor any good way to clean them up (other than successfully deleting a backup).

skriss · 2018-03-16T19:07:21Z

in Azure, if I manually delete a snapshot before deleting a backup that references it, Ark doesn't error and deletes the backup (I think the outcome is OK, but I would've expected an error. It may be that the Azure API doesn't return an error if you try to delete a non-existent snapshot?)

I'm not sure an error is appropriate here. Your intent is to delete a backup and all its associated data. If we can't find some of the associated data, that's not really an issue, is it? I would expect it to be a problem if you wanted to restore, of course.

Yeah, I think it's fine as-is.

Maybe we need something as part of the backup sync controller that periodically verifies the integrity of a backup's associated data?

Would be a nice feature to add, but not needed as part of this PR.

when running ark backup delete for a non-existent backup, the Ark server log has this error:

I manually created a DeleteBackupRequest with what should be the same as what ark backup delete does (since I now have the check for nonexistent backups), and I can't reproduce the patch error. Would you have some time to try to reproduce & debug?

Yep, I'll look into it

if I take a backup with a snapshot, then remove the persistentVolumeProvider from config, then try to delete the backup, the failed delete request doesn't show up in ark backup describe -- looks like this is because we only show the associated requests if the backup is in phase Deleting, which the backup is not in this case

So maybe always show if at least 1 has errors?

I think that makes sense. Makes more sense to me than putting the backup in Deleting.

This brings up something I haven't addressed yet - there's no expiration on DeleteBackupRequests nor any good way to clean them up (other than successfully deleting a backup).

Yeah - probably not urgent but would be a good idea to GC old ones, esp. those that aren't associated with any existing backup.

skriss · 2018-03-27T19:19:22Z

I can no longer produce the patch error - attempted deletion of a nonexistent backup is blocked by the CLI, and applying the resource directly works fine.

ncdc · 2018-04-04T20:25:18Z

@skriss added deletion of processed delete backup requests > 24h

- Add pruning settings to Gopkg.toml - Update vendoring deps doc to point to dep installation instructions and to use dep instead of hack/dep-save.sh - Remove hack/dep-save.sh Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Now that we've configured pruning for dep, this removes all unused packages, all non-go files, and all tests from the vendor directory. NOTE: due to a change in dep, it preserves anything that looks like a license file. We'll be pulling in a few files we weren't previously using - mostly license files. It's easier to just go with what dep does than to try to exclude them after the fact. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Use a custom builder image to do all of Ark's builds. This image now contains k8s.io/code-generator for code generation. Enable docker in travis to use the builder image. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

We ran into a lot of problems using a finalizer on the backup to allow the Ark server to clean up all associated backup data when deleting a backup. Users also found it less than desirable that deleting the heptio-ark namespace resulted in all the backup data being deleted. This removes the finalizer and replaces it with an explicit DeleteBackupRequest that is created as a means of requesting the deletion of a backup and all its associated data. This is what `ark backup delete` does. If you use kubectl to delete a backup or to delete the heptio-ark namespace, this no longer deletes associated backups. Additionally, as long as the heptio-ark namespace still exists, the Ark server's BackupSyncController will continually sync backups into the heptio-ark namespace from object storage. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss · 2018-04-05T16:43:22Z

Latest commit LGTM

Always request DeleteBackupRequests for a given backup so we can show failed deletion attempts if you try to delete a backup that has PV snapshots when Ark doesn't have a persistentVolumeProvider configured. When creating a DeleteBackupRequest, include a label for the UID so we can match based on name and UID when associated DeleteBackupRequests with a given backup. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Make sure a DeleteBackupRequest has its Spec.BackupName filled in. If not, record an error in the status and mark the request as processed. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss · 2018-04-05T19:19:37Z

last 2 also LGTM

When the BackupDeletionController processes a request, set the request's backup-name and backup-uid labels if they aren't currently set. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss · 2018-04-05T20:14:05Z

2 things found testing:

deleting a backup while it's in progress is allowed. this could lead to snapshots getting orphaned if it's deleted after the snapshot is triggered but before it's recorded in the backup obj (I was able to make this happen). It's certainly an edge case, and blocking deletion of In Progress backups may not be desirable (e.g. if they get stuck there for some reason), so not sure if it makes sense to attempt to prevent or not.
I tested on GKE this time, and if I manually deleted a snapshot prior to ark backup deleteing the backup linked to the snapshot, I now get an error with the deletion because the GCP API returns a not found error from the DeleteSnapshot call. Per prior comments, this goes through without error on Azure (I think the Azure API doesn't return an error in this scenario). Haven't tried on AWS yet. It would be nice to have consistent behavior across clouds if possible.

Otherwise, all LGTM!

skriss · 2018-04-05T23:42:24Z

As a followup to the previous comment, the scenario in the 2nd bullet also errors on AWS.

ncdc · 2018-04-06T14:03:08Z

deleting a backup while it's in progress is allowed. this could lead to snapshots getting orphaned if it's deleted after the snapshot is triggered but before it's recorded in the backup obj (I was able to make this happen). It's certainly an edge case, and blocking deletion of In Progress backups may not be desirable (e.g. if they get stuck there for some reason), so not sure if it makes sense to attempt to prevent or not.

The problem is knowing if an InProgress backup is truly in progress, or stuck. Maybe it's running, or maybe the Ark server was restarted in the middle of the backup.

We could add an in-memory list/map/set of in progress backups and disallow deleting those.

I tested on GKE this time, and if I manually deleted a snapshot prior to ark backup deleteing the backup linked to the snapshot, I now get an error with the deletion because the GCP API returns a not found error from the DeleteSnapshot call. Per prior comments, this goes through without error on Azure (I think the Azure API doesn't return an error in this scenario). Haven't tried on AWS yet. It would be nice to have consistent behavior across clouds if possible.

This requires that we change the protobuf messages to return better errors, so we can identify "not found" properly. I'd like to defer fixing this until we make the plugin/protobuf changes. That ok with you?

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc · 2018-04-06T17:09:15Z

Latest commit keeps track of in-progress backups in memory and doesn't allow deletion of them.

skriss · 2018-04-06T19:18:37Z

OK, latest LGTM

ncdc added Bug p0 - Hair On Fire labels Mar 15, 2018

ncdc commented Mar 15, 2018

View reviewed changes

nrb reviewed Mar 15, 2018

View reviewed changes

skriss suggested changes Mar 15, 2018

View reviewed changes

skriss reviewed Mar 15, 2018

View reviewed changes

ncdc force-pushed the delete-backup-request branch 2 times, most recently from f06cb35 to 1fe9dea Compare March 15, 2018 20:18

ncdc mentioned this pull request Mar 16, 2018

Convert GC controller to use a work queue #358

Closed

ncdc force-pushed the delete-backup-request branch from 1fe9dea to ea74e5d Compare March 16, 2018 18:02

ncdc force-pushed the delete-backup-request branch 3 times, most recently from 3960c93 to 54dabd3 Compare April 4, 2018 20:20

ncdc added 9 commits April 4, 2018 16:44

Update dep info

4d8fb90

- Add pruning settings to Gopkg.toml - Update vendoring deps doc to point to dep installation instructions and to use dep instead of hack/dep-save.sh - Remove hack/dep-save.sh Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Use a custom builder image

1455d67

Use a custom builder image to do all of Ark's builds. This image now contains k8s.io/code-generator for code generation. Enable docker in travis to use the builder image. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Remove k8s.io/code-generator dependency

b0a2a0b

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Switch back to heptio-ark for the server

1545406

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Add DeleteBackupRequest CRD to 00-prereqs.yaml

dd9b9f3

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Expire processed deleted backup requests > 24hr

1ae492d

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Regen docs

a4d5061

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc force-pushed the delete-backup-request branch from 54dabd3 to a4d5061 Compare April 5, 2018 15:19

ncdc added 2 commits April 5, 2018 14:21

Require DeleteBackupRequest.Spec.BackupName

ef57a44

Make sure a DeleteBackupRequest has its Spec.BackupName filled in. If not, record an error in the status and mark the request as processed. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc changed the title ~~[WIP] Switch from finalizer to DeleteBackupRequest for deleting backups~~ Switch from finalizer to DeleteBackupRequest for deleting backups Apr 5, 2018

ncdc added this to the v0.8.0 milestone Apr 5, 2018

Set DeleteBackupRequest labels if missing

644a75e

When the BackupDeletionController processes a request, set the request's backup-name and backup-uid labels if they aren't currently set. Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

Don't allow deletion of in-progress backups

3f2e222

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss approved these changes Apr 6, 2018

View reviewed changes

skriss merged commit 25d46a7 into vmware-tanzu:master Apr 6, 2018

ncdc deleted the delete-backup-request branch April 6, 2018 19:24

ncdc mentioned this pull request Apr 18, 2018

ark plugin commands need to operate against the server's namespace #340

Closed

This pull request was closed.

		@@ -135,28 +135,6 @@ ark restore describe <RESTORE_NAME>

		For more information, see [the debugging information][18].

		### Clean up


		To run the server in another namespace, you edit the relevant files, changing `heptio-ark-server` to
		To run the server in another namespace, you edit the relevant files, changing `heptio-ark` to

Switch from finalizer to DeleteBackupRequest for deleting backups #383

Switch from finalizer to DeleteBackupRequest for deleting backups #383

Conversation

ncdc commented Mar 15, 2018 • edited Loading

ncdc commented Mar 15, 2018

ncdc commented Mar 15, 2018

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

skriss 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

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

skriss commented Mar 16, 2018 • edited Loading

ncdc commented Mar 16, 2018

skriss commented Mar 16, 2018

skriss commented Mar 27, 2018

ncdc commented Apr 4, 2018

skriss commented Apr 5, 2018

skriss commented Apr 5, 2018

skriss commented Apr 5, 2018 • edited Loading

skriss commented Apr 5, 2018

ncdc commented Apr 6, 2018

ncdc commented Apr 6, 2018

skriss commented Apr 6, 2018

ncdc commented Mar 15, 2018 •

edited

Loading

skriss commented Mar 16, 2018 •

edited

Loading

skriss commented Apr 5, 2018 •

edited

Loading