Automatically manage restic repos #606

skriss · 2018-06-25T16:37:29Z

Fixes #577

Builds on #570, ignore first commit.

Replaces ark restic repo init with automatic repository initialization, using a single static non-secret restic key.

nrb · 2018-06-25T21:43:01Z

pkg/restic/repository_ensurer.go

+			return nil, errors.New("restic repository is not ready")
+		}
+		return repo, nil
+	}


Might add a comment here that the rest of the function is for the apierrors.IsNotFound(err) == true case.

nrb · 2018-06-26T15:09:50Z

pkg/restic/restorer.go

@@ -85,7 +84,7 @@ func (r *restorer) RestorePodVolumes(restore *arkv1api.Restore, pod *corev1api.P
 		return nil
 	}

-	repo, err := getReadyRepo(r.repoLister, restore.Namespace, pod.Namespace)
+	repo, err := r.repoEnsurer.EnsureRepo(r.ctx, restore.Namespace, pod.Namespace)


Would we ever end up creating a repo here? I'm thinking that even if we do, the data and buckets are in an invalid state already.

This has the potential to create the restic repo. We shouldn't be doing that as part of a restore.

How else would you handle the case where a user installs Ark in a fresh cluster and wants to restore an existing backup from another cluster with restic snapshots?

My bad, I wasn't thinking about the restore half of the equation (that's what happens when I try to wrap up a code review at the end of the day after a long meeting!).

nrb · 2018-06-26T15:17:27Z

pkg/restic/backupper.go

 func (b *backupper) BackupPodVolumes(backup *arkv1api.Backup, pod *corev1api.Pod, log logrus.FieldLogger) (map[string]string, []error) {
 	// get volumes to backup from pod's annotations
 	volumesToBackup := GetVolumesToBackup(pod)
 	if len(volumesToBackup) == 0 {
 		return nil, nil
 	}

-	repo, err := getReadyRepo(b.repoLister, backup.Namespace, pod.Namespace)
+	repo, err := b.repoEnsurer.EnsureRepo(b.ctx, backup.Namespace, pod.Namespace)
 	if err != nil {
 		return nil, []error{err}
 	}


This function is executed in the daemonset, correct?

I ran into an issue yesterday that was partially my fault, but probably warrants a log message. If the daemonset exists, but no pods in it are ready, the restic backup process will still proceed and then hang waiting for the daemonset pod to finish working. In my testing, it waited for over 5 minutes and couldn't be stopped with ctrl-c, only a kill -9.

I can probably replicate if needed to walk through this.

ncdc

I made it through most of the PR but there's some files at the end I didn't have time for yet before EOD. But I wanted to submit now so you can see my comments on the repositoryEnsurer.

ncdc · 2018-06-26T18:31:36Z

pkg/cmd/server/server.go

+	// warn if restic daemonset does not exist
+	if _, err := s.kubeClient.AppsV1().DaemonSets(s.namespace).Get(restic.DaemonSet, metav1.GetOptions{}); apierrors.IsNotFound(err) {
+		s.logger.Warn("Ark restic daemonset not found; restic backups/restores will not work until it's created")
+	}


Do we want to log a warning if we got some other error?

ncdc · 2018-06-26T18:34:51Z

pkg/controller/backup_deletion_controller.go

+		return []error{err}
+	}
+
+	ctx, cancelFunc := context.WithTimeout(context.Background(), time.Minute)


Let's extract the time.Minute as a const

ncdc · 2018-06-26T18:49:09Z

pkg/restic/repository_ensurer.go

+	}
+}
+
+func (r *repositoryEnsurer) onReady(name string) chan *arkv1api.ResticRepository {


Would probably call this something else like newReadyChan

ncdc · 2018-06-26T18:51:45Z

pkg/restic/repository_keys.go

@@ -11,26 +29,27 @@ import (
 const (
 	CredentialsSecretName = "ark-restic-credentials"
 	CredentialsKey        = "ark-restic-credentials"


We should change the name of this key inside the secret, don't you think?

ncdc · 2018-06-26T18:52:10Z

pkg/restic/repository_keys.go

@@ -11,26 +29,27 @@ import (
 const (
 	CredentialsSecretName = "ark-restic-credentials"
 	CredentialsKey        = "ark-restic-credentials"
+	EncryptionKey         = "static-passw0rd"


Does this need to be exported?

ncdc · 2018-06-26T18:53:25Z

pkg/restic/repository_keys.go

 	secret := &corev1api.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
 			Name:      CredentialsSecretName,
 		},
 		Type: corev1api.SecretTypeOpaque,
 		Data: map[string][]byte{
-			CredentialsKey: data,
+			CredentialsKey: []byte(EncryptionKey),
 		},
 	}

 	_, err := secretClient.Secrets(namespace).Create(secret)


Let's do a Get() first to avoid more of #281

ncdc · 2018-06-26T21:08:26Z

pkg/restic/repository_manager.go

+		return errors.New("timed out waiting for cache to sync")
+	}
+
+	repo, err := rm.repoEnsurer.EnsureRepo(ctx, rm.namespace, snapshot.Repo)


This has the potential to create the restic repo. We shouldn't be doing that as part of deletions/GC.

same question re: spinning up a new cluster, then having GC process backups that were synced in from an existing bucket.

Yup, my bad

ncdc · 2018-06-26T21:08:44Z

pkg/restic/restorer.go

@@ -85,7 +84,7 @@ func (r *restorer) RestorePodVolumes(restore *arkv1api.Restore, pod *corev1api.P
 		return nil
 	}

-	repo, err := getReadyRepo(r.repoLister, restore.Namespace, pod.Namespace)
+	repo, err := r.repoEnsurer.EnsureRepo(r.ctx, restore.Namespace, pod.Namespace)


This has the potential to create the restic repo. We shouldn't be doing that as part of a restore.

ncdc · 2018-06-26T21:18:07Z

pkg/restic/repository_ensurer.go

+	readyChans     map[string]chan *arkv1api.ResticRepository
+}
+
+func newRepositoryEnsurer(repoInformer arkv1informers.ResticRepositoryInformer, repoClient arkv1client.ResticRepositoriesGetter) *repositoryEnsurer {


I would recommend changing the logic so we create a new one of these per *restic.backupper. Then we don't need a lock or multiple channels and can instead just have a single channel.

I'm even tempted to say we can do away with the informer+channel. Move the initializeRepo functionality from *resticRepositoryController directly into the *restic.Backupper or this repositoryEnsurer. Then it's a lot simpler to implement EnsureRepo:

Try to get the repo.

If it exists & is ready, done.

If it exists & isn't ready, wait (?) or call Check(). Return result of wait/check

If it doesn't exist, try to create it

If create succeeds, run the logic in initializeRepo

If you want to avoid having multiple channels, we'd need to have one of these per call to BackupPodVolumes in the backupper, since a backup can have >1 namespace/repo. Not sure exactly what that would look like, since I don't think we'd want to be creating a new informer each time, but we can't be adding/removing event handlers while an existing one is running.

I'm not sure I like the idea of moving the repo init logic into the backupper or ensurer, but I think we need to talk it through in person.

Why don't you like what I did?

Let me take a look at it again with fresh eyes. I had to draw out the flow and the relationship between the components (ensurer vs repo controller). I was trying to find a way to have a single component responsible for the creation and initialization of the repo, instead of having that split across 2 (the ensurer that creates the ResticRepository, and the controller that initializes it in object storage and sets the phase to Ready).

ncdc · 2018-06-27T15:07:52Z

@skriss does this look accurate for the EnsureRepo flow?

skriss · 2018-06-27T15:52:53Z

Looks right to me

ncdc · 2018-06-27T16:38:47Z

Backup flow:

ncdc · 2018-06-27T16:43:31Z

Delete snapshot flow (from backup deletion controller):

ncdc · 2018-06-27T17:33:47Z

Daemonset pod backup flow:

ncdc · 2018-06-27T18:17:43Z

Restore overview:

ncdc · 2018-06-27T18:30:58Z

DaemonSet pod restore flow:

skriss · 2018-06-27T20:32:46Z

Addressed items discussed in new commits; will do squashing post-review.

ncdc · 2018-06-27T20:47:24Z

LGTM

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

skriss · 2018-06-27T20:51:39Z

squashed.

skriss added this to the v0.9.0 milestone Jun 25, 2018

skriss requested review from ncdc and nrb June 25, 2018 16:37

nrb reviewed Jun 25, 2018

View reviewed changes

skriss force-pushed the restic-auto-manage-repos branch from 8b5bb78 to 2847d25 Compare June 25, 2018 22:55

ncdc mentioned this pull request Jun 26, 2018

How to retrieve and restore restic backup #611

Closed

nrb reviewed Jun 26, 2018

View reviewed changes

ncdc reviewed Jun 26, 2018

View reviewed changes

skriss force-pushed the restic-auto-manage-repos branch from c8cb556 to c44a16a Compare June 27, 2018 20:30

replace ark restic repo init cmd with automatic initialization of repos

22e8f23

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

skriss force-pushed the restic-auto-manage-repos branch from c44a16a to 22e8f23 Compare June 27, 2018 20:51

ncdc merged commit 51298f8 into vmware-tanzu:master Jun 27, 2018

skriss deleted the restic-auto-manage-repos branch June 27, 2018 21:21

skriss mentioned this pull request Jul 3, 2018

The "backup.ark.heptio.com/backup-volumes" setting for restic is unclear #568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically manage restic repos #606

Automatically manage restic repos #606

skriss commented Jun 25, 2018 •

edited

Loading

nrb Jun 25, 2018

skriss Jun 25, 2018

nrb Jun 26, 2018

ncdc Jun 26, 2018

skriss Jun 26, 2018 •

edited

Loading

ncdc Jun 27, 2018

nrb Jun 26, 2018

ncdc left a comment

ncdc Jun 26, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

skriss Jun 26, 2018

ncdc Jun 27, 2018

ncdc Jun 26, 2018

ncdc Jun 26, 2018

skriss Jun 26, 2018

ncdc Jun 27, 2018

ncdc commented Jun 27, 2018

skriss commented Jun 27, 2018

ncdc commented Jun 27, 2018 •

edited

Loading

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

skriss commented Jun 27, 2018 •

edited

Loading

ncdc commented Jun 27, 2018

skriss commented Jun 27, 2018

Automatically manage restic repos #606

Automatically manage restic repos #606

Conversation

skriss commented Jun 25, 2018 • edited Loading

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 Jun 26, 2018 • edited Loading

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 Jun 27, 2018

skriss commented Jun 27, 2018

ncdc commented Jun 27, 2018 • edited Loading

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

ncdc commented Jun 27, 2018

skriss commented Jun 27, 2018 • edited Loading

ncdc commented Jun 27, 2018

skriss commented Jun 27, 2018

skriss commented Jun 25, 2018 •

edited

Loading

skriss Jun 26, 2018 •

edited

Loading

ncdc commented Jun 27, 2018 •

edited

Loading

skriss commented Jun 27, 2018 •

edited

Loading