restic integration with Ark #508

skriss · 2018-05-18T22:19:15Z

Fixes #19

Reviewers: I wouldn't bother looking through individual commits, just the overall diff. I'll probably squash them all.

TODO:

update unit tests
update Makefile / associated build artifacts
finalize maintenance schedule
run backups/restores of multiple volumes within a pod concurrently

ncdc

Did a quick first pass. Will look in more detail at the restic repository manager and below tomorrow.

ncdc · 2018-05-22T01:03:21Z

cmd/restic-init-container/main.go

+			return false
+		}
+
+		fmt.Printf("Found %s", doneFile)


Do you need to break here?

no, since we need to make it through this loop for every child that's a directory. the Printf is for information only.

ncdc · 2018-05-22T01:04:34Z

cmd/restic-wrapper/main.go

+	os.Setenv("AZURE_ACCOUNT_NAME", os.Getenv("AZURE_STORAGE_ACCOUNT_ID"))
+	os.Setenv("AZURE_ACCOUNT_KEY", os.Getenv("AZURE_STORAGE_KEY"))
+
+	resticArgs := append([]string{"-p=" + passwordFile}, os.Args[1:]...)


If there are longhand forms for the flags, it might be clearer to use those.

ncdc · 2018-05-22T01:06:00Z

cmd/restic-wrapper/main.go

+	checkError(resticCmd.Run(), "ERROR running restic command")
+}
+
+func repoName(args []string) string {


doc/example pls

ncdc · 2018-05-22T01:06:35Z

cmd/restic-wrapper/main.go

+// otherwise.
+func checkError(err error, msg string) {
+	if err != nil {
+		fmt.Fprintf(os.Stderr, msg+": %s\n", err)


We usually %v errors

ncdc · 2018-05-22T01:07:21Z

cmd/restic-wrapper/main.go

+// checkError prints an error message and the error's text to stderr and
+// exits with a status code of 1 if the error is not nil, or is a no-op
+// otherwise.
+func checkError(err error, msg string) {


Since you're printing the msg and then the error, I think it would make sense to reverse the order of the arguments.

ncdc · 2018-05-22T01:29:20Z

pkg/restic/backupper_restorer.go

+	// for each volume to backup:
+	for _, volumeName := range volumesToBackup {
+		wg.Add(1)
+		go func(volumeName string) {


Could this be declared standalone instead of as an anonymous function?

ncdc · 2018-05-22T01:29:54Z

pkg/restic/backupper_restorer.go

+			cmd := backupCommand(br.metadataManager.RepoPrefix(), pod.Namespace, string(pod.UID), volumeDir, snapshotTags)
+
+			// exec it in the daemonset pod
+			if err := br.daemonSetExecutor.Exec(pod.Spec.NodeName, cmd, 10*time.Minute, log); err != nil {


10 minutes?

we probably need to make this tuneable.

I know we're switching to a CRD instead of using exec, but yes, we'll need this to be configurable from the start.

Either per backup or globally or both (use backup's but fallback to global if unset)

ncdc · 2018-05-22T01:31:58Z

pkg/restic/backupper_restorer.go

+			// save the snapshot's ID in the pod annotation
+			lock.Lock()
+			SetPodSnapshotAnnotation(pod, volumeName, snapshotID)
+			backupSnapshots = append(backupSnapshots, fmt.Sprintf("%s/%s", pod.Namespace, snapshotID))


Instead of using a lock, could we somehow use channel(s) to coordinate retrieving the snapshot id?

ncdc · 2018-05-22T01:36:21Z

pkg/restic/daemon_set_executor.go

+}
+
+func (dse *defaultDaemonSetExecutor) getDaemonSetPod(node string) (*corev1api.Pod, error) {
+	// TODO we may want to cache this list


We can start the k8s shared informer factory beforehand and use the pod informer

ncdc · 2018-05-22T01:40:05Z

pkg/restic/repository_manager.go

+		}
+
+		// strip the trailing '/' if it exists
+		if prefix[len(prefix)-1] == '/' {


strings.TrimSuffix

skriss · 2018-05-22T17:50:47Z

cmd/restic-wrapper/main.go

+	"k8s.io/client-go/rest"
+)
+
+func main() {


@ncdc I was considering trying to get rid of this binary. Since it's used in the daemonset, that would require the Ark server to create a temp creds file in the appropriate daemonset pod. Do you think that's a good idea?

skriss · 2018-05-23T17:12:27Z

@ncdc any issue with me squashing most/all of the commits down to 1?

ncdc · 2018-05-23T15:25:17Z

pkg/cmd/server/server.go

+	go func() {
+		interval := time.Hour
+
+		<-time.After(interval)


I don't think we need to have an initial one hour delay. If you wanted to be more confident that maintenance is run hourly (and especially not every time you start the server, if you're rapidly starting/quitting/restarting), we could have some CRD that keeps track of overall restic status, including the last maintenance time.

ncdc · 2018-05-23T15:27:56Z

pkg/cmd/server/server.go

@@ -314,6 +348,7 @@ var defaultResourcePriorities = []string{
 	"configmaps",
 	"serviceaccounts",
 	"limitranges",
+	"pods",


We should comment why the list is in this order: namespaces first because everything that isn't cluster-scoped lives in them, PVs because we need to get the volumes before PVCs or pods, ..., pods because we might be restoring restic backups, and we need to try to restore the pods before allowing the controllers (deployments, replicasets, statefulsets, etc) to spin up their own pods

ncdc · 2018-05-23T19:06:42Z

pkg/restic/backupper_restorer.go

+	}
+
+	// ensure a repo exists for the pod's namespace
+	exists, err := br.metadataManager.RepositoryExists(pod.Namespace)


Would it be worth it to combine RepositoryExists and InitRepo as EnsureRepo?

ncdc · 2018-05-23T19:31:40Z

pkg/backup/item_backupper.go

+			return errors.WithStack(err)
+		}
+
+		if err := ib.resticBackupper.BackupPodVolumes(ib.backup, pod, log); err != nil {


Since we've called restic.GetVolumesToBackup above, maybe we should pass that data into BackupPodVolumes so we don't have to do it twice?

ncdc · 2018-05-23T19:34:53Z

pkg/restic/backupper_restorer.go

+	RestorePodVolumes(restore *arkv1api.Restore, pod *corev1api.Pod, log logrus.FieldLogger) error
+}
+
+func (br *backupperRestorer) BackupPodVolumes(backup *arkv1api.Backup, pod *corev1api.Pod, log logrus.FieldLogger) error {


How would you feel about this function not mutating either the backup or the pod, and instead returning the appropriate information so that another function can deal with the mutations? It should make it easier to unit test where we call BackupPodVolumes (in item_backupper) - we'd be able to test the changes coming back from a mocked resticBackupper.

ncdc · 2018-05-23T20:05:08Z

pkg/restic/repository_manager.go

+}
+
+func (rm *repositoryManager) getAllRepos() ([]string, error) {
+	prefixes, err := rm.objectStore.ListCommonPrefixes(rm.bucket, "/")


If the resticLocation that came from the config was bucket/dir, that goes into rm.bucket - will that work here having both the bucket and the directory in a single string? Or do we need to call ListCommonPrefixes(bucketName, dir+"/")?

ncdc · 2018-05-23T20:11:27Z

pkg/backup/item_backupper.go

+			return errors.WithStack(err)
+		}
+
+		if err := ib.resticBackupper.BackupPodVolumes(ib.backup, pod, log); err != nil {


Note, this invocation ends up listing all the common prefixes (i.e. repos) in resticLocation in object storage (ensuring the restic repo exists for the namespace) for each pod. Are you concerned about how frequently we'll be running that same query for a given namespace? I know the cost is fairly minimal, but there is still a cost associated with listing from object storage. We may want something more efficient and/or a cache of the data.

ncdc · 2018-05-23T20:29:27Z

pkg/restic/repository_manager.go

+}
+
+func (rm *repositoryManager) InitRepo(name string) error {
+	resticCreds, err := rm.secretsClient.Get(credsSecret, metav1.GetOptions{})


The plan was to require an admin to precreate manually per namespace, wasn't it?

We should discuss the ui/ux. We could have something like

# generates an encryption key, creates a secret for it, maybe creates a new ResticRepo, ark server sees it's New and runs restic init? ark restic init --namespace foo # same as above, but uses a precreated encryption key instead of generating ark restic init --namespace foo --encryption-key /path/to/file

If we were to add a ResticRepo CRD, we could use that to manage its status (needs init, already done, etc). Not sure if we need this or how it would impact backup replication. Would probably have to be tied to a BackupTarget when we get there.

ncdc · 2018-05-23T20:40:00Z

pkg/restic/repository_manager.go

+	}
+
+	defer func() {
+		file.Close()


Need better error handling around close/remove errors.

ncdc · 2018-05-23T20:45:21Z

pkg/restore/restic_restore_action.go

+		initContainer.VolumeMounts = append(initContainer.VolumeMounts, mount)
+	}
+
+	if len(pod.Spec.InitContainers) == 0 || pod.Spec.InitContainers[0].Name != "restic-wait" {


If there are no init containers, list=initContainer

If the first init container isn't restic-wait, set list=initContainer, rest...

If the first init container is restic-wait, overwrite it with initContainer

?

skriss · 2018-05-25T04:29:37Z

@ncdc switched restores to using a CRD, and did some cleanup with the backup CRD/controller.

Still need to change backupper_restorer to not be global (and should be easy to split in half now).
Still need to support manual repo key creation.
Some lower-level cleanup to do as well.

ncdc · 2018-05-24T21:11:50Z

pkg/restic/backupper_restorer.go

+		case <-timeout:
+			errs = append(errs, errors.New("timed out waiting for all PodVolumeBackups to complete"))
+			break ForLoop
+		case res := <-br.backupResults[backup.UID]:


res, ok and check if !ok (channel closed)

ncdc · 2018-05-25T14:33:34Z

pkg/restic/backupper_restorer.go

+		},
+	)
+
+	// TODO do I need to use an actual context here?


Either a context or a stop channel passed in to the constructor. Use that single context/channel when running the informers and waiting for cache syncs.

ncdc · 2018-05-25T14:34:24Z

pkg/restic/backupper_restorer.go

+		br.metadataManager.RLock(pod.Namespace)
+		defer br.metadataManager.RUnlock(pod.Namespace)
+
+		// TODO should we return here, or continue with what we can?


As discussed briefly on slack, continue with what we can. We need a solid UX around error reporting (probably after this PR).

ncdc · 2018-05-25T14:36:34Z

pkg/restic/backupper_restorer.go

+	defer delete(br.backupResults, backup.UID)
+
+	for _, volumeName := range volumesToBackup {
+		br.metadataManager.RLock(pod.Namespace)


It's a bit scary needing to expose a lock in another component and lock/unlock it outside said component. Is there an alternative?

ncdc · 2018-05-25T15:12:07Z

pkg/restic/backupper_restorer.go

+				},
+			},
+			Labels: map[string]string{
+				"ark.heptio.com/backup": backup.Name,


Should include uid too. We have constants for backup name and uid labels that I added for delete backup requests: BackupNameLabel and BackupUIDLabel. They're in delete_backup_request.go but we should probably move them to a labels.go file.

ncdc · 2018-05-25T15:21:16Z

pkg/restic/backupper_restorer.go

+				"ns":         pod.Namespace,
+				"volume":     volumeName,
+			},
+			RepoPrefix: br.metadataManager.RepoPrefix(),


Let's discuss if we need this in the future (when we have backup targets)

ncdc · 2018-05-25T15:21:44Z

pkg/restic/backupper_restorer.go

+		br.metadataManager.RLock(pod.Namespace)
+		defer br.metadataManager.RUnlock(pod.Namespace)
+
+		// TODO should we return here, or continue with what we can?


ncdc · 2018-05-25T15:22:16Z

pkg/restic/backupper_restorer.go

+				},
+			},
+			Labels: map[string]string{
+				"ark.heptio.com/restore": restore.Name,


make it a constant in pkg/apis/... and also use uid

ncdc · 2018-05-25T15:25:40Z

pkg/apis/ark/v1/config.go

+	// Ark stores restic backups of pod volumes, specified either as "bucket" or
+	// "bucket/prefix". This bucket must be different than the `Bucket` field.
+	// Optional.
+	ResticLocation string `json:"resticLocation"`


Given that restic has its own set of supported storage backends, upon further reflection, I think we should split this out into a separate struct. Otherwise it implies that we/restic support all possible ObjectStores Ark supports (including any/all 3rd party plugins).

Let's have fields for provider name, bucket, prefix, and an open config map.

ncdc · 2018-05-27T17:24:54Z

pkg/restic/repository_manager.go

+
+	resultChan := make(chan *arkv1api.PodVolumeBackup)
+
+	informer := arkv1informers.NewFilteredPodVolumeBackupInformer(


We will probably want to have 1 informer running per backup, and this does 1 informer per backup+pod. We could add a new struct that would be per-backup, and create a new one at the beginning of a backup.

ncdc · 2018-05-27T17:27:26Z

pkg/restic/repository_manager.go

+		case res := <-resultChan:
+			switch res.Status.Phase {
+			case arkv1api.PodVolumeBackupPhaseCompleted:
+				SetPodSnapshotAnnotation(pod, res.Spec.Volume, res.Status.SnapshotID)


I still think I'd like to see this function return the pod/snapshot associations, and have another function modify that pod. It hopefully will make testing easier.

ncdc · 2018-05-27T17:27:37Z

pkg/restic/repository_manager.go

+			},
+		}
+
+		// TODO should we return here, or continue with what we can?


record the error & continue

ncdc · 2018-05-27T17:29:10Z

pkg/restic/repository_manager.go

+			UpdateFunc: func(_, obj interface{}) {
+				pvb := obj.(*arkv1api.PodVolumeBackup)
+
+				if pvb.Status.Phase == arkv1api.PodVolumeBackupPhaseCompleted || pvb.Status.Phase == arkv1api.PodVolumeBackupPhaseFailed {


Can this function be run in parallel for 2 pods for the same backup? We'll likely need to match PVBs for this backup's name/UID.

ncdc · 2018-05-27T17:29:29Z

pkg/restic/repository_manager.go

+			UpdateFunc: func(_, obj interface{}) {
+				pvr := obj.(*arkv1api.PodVolumeRestore)
+
+				if pvr.Status.Phase == arkv1api.PodVolumeRestorePhaseCompleted || pvr.Status.Phase == arkv1api.PodVolumeRestorePhaseFailed {


Can this function be run in parallel for 2 pods for the same restore? We'll likely need to match PVRs for this restore's name/UID.

ncdc · 2018-05-27T17:30:07Z

pkg/restic/repository_manager.go

+
+	resultChan := make(chan *arkv1api.PodVolumeRestore)
+
+	informer := arkv1informers.NewFilteredPodVolumeRestoreInformer(


Same comment as above re 1 informer per restore instead of 1 per restore+pod.

ncdc · 2018-06-04T15:52:13Z

Dockerfile-restic-init-container.alpine

@@ -0,0 +1,23 @@
+# Copyright 2017 the Heptio Ark contributors.


ncdc · 2018-06-04T15:53:44Z

cmd/restic-init-container/main.go

@@ -0,0 +1,61 @@
+package main


Add license header please

ncdc · 2018-06-04T15:54:59Z

examples/aws/10-deployment.yaml

@@ -29,11 +29,12 @@ spec:
      serviceAccountName: ark
      containers:
        - name: ark
-          image: gcr.io/heptio-images/ark:latest
+          image: gcr.io/steve-heptio/ark:restic


ncdc · 2018-06-04T15:55:17Z

examples/aws/10-deployment.yaml

          command:
            - /ark
          args:
            - server
+            - --log-level=debug


ncdc · 2018-06-04T15:55:54Z

examples/aws/20-restic-daemonset.yaml

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+---


Technically not needed - it's only used to separate between previous item and next item

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

pkg/restic/restorer.go

+	}
+
+	r.resultsLock.Lock()
+	r.results[resultsKey(pod.Namespace, pod.Name)] = make(chan *arkv1api.PodVolumeRestore)


Save channel as var and use below in for/select loop

ncdc · 2018-06-04T18:36:43Z

pkg/restic/restorer.go

+	repoManager *repositoryManager
+	informer    cache.SharedIndexInformer
+	results     map[string]chan *arkv1api.PodVolumeRestore
+	resultsLock sync.Mutex


Reorganize so everything locked by resultsLock is grouped below it

ncdc · 2018-06-04T18:36:55Z

pkg/restic/restorer.go

+		r.repoManager.repoLocker.Lock(pod.Namespace, false)
+		defer r.repoManager.repoLocker.Unlock(pod.Namespace, false)
+
+		volumeRestore := &arkv1api.PodVolumeRestore{


helper func

ncdc · 2018-06-04T18:37:07Z

pkg/restic/restorer.go

+	)
+
+	for volume, snapshot := range volumesToRestore {
+		r.repoManager.repoLocker.Lock(pod.Namespace, false)


lock once per namespace, outside of this func

ncdc · 2018-06-04T19:13:32Z

restic-container/Dockerfile

@@ -0,0 +1,26 @@
+# Copyright 2018 the Heptio Ark contributors.


This file is no longer used?

ncdc · 2018-06-04T19:14:31Z

Also need to remove the restic-init-container/restic-init-container binary executable from the branch

ncdc · 2018-06-04T19:58:09Z

Yes, fields passed in vs derived.

…

On Mon, Jun 4, 2018 at 3:38 PM, Steve Kriss ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/backup/item_backupper.go <#508 (comment)>: > - tarWriter tarWriter - resourceHooks []resourceHook - dynamicFactory client.DynamicFactory - discoveryHelper discovery.Helper - snapshotService cloudprovider.SnapshotService - + backup *api.Backup + namespaces *collections.IncludesExcludes + resources *collections.IncludesExcludes + backedUpItems map[itemKey]struct{} + actions []resolvedAction + tarWriter tarWriter + resourceHooks []resourceHook + dynamicFactory client.DynamicFactory + discoveryHelper discovery.Helper + snapshotService cloudprovider.SnapshotService itemHookHandler itemHookHandler Do you mean fields directly passed into the constructor? bc in this example, itemHookHandler and additionalItemBackupper are both set in the constructor, just derived rather than directly passed in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#508 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYnLWKfnr-TB82Nr6o73TqBfkiA-Oks5t5YzKgaJpZM4UFbLm> .

Signed-off-by: Steve Kriss <[email protected]>

skriss · 2018-06-04T21:56:21Z

@ncdc I believe I've addressed all your material comments, with a handful of minor things to do prior to 0.9. I also pulled your CLI commit into this PR. When you've had a chance to go through the latest code review commits, I'm going to squash this whole thing down to just 2 commits, one for vendor/ updates and one for the rest.

skriss · 2018-06-04T21:57:31Z

We'll need to update CI to use the new make all-containers and make all-push targets.

ncdc · 2018-06-05T16:31:36Z

pkg/server/generic_server.go

+	cancelFunc           context.CancelFunc
+}
+
+func NewGenericServer(


I am not sure this is helpful. There is not enough commonality between the daemonset command and the server command.

DS:

✅ sets controllers

❌ doesn't set preStartControllers

✅ sets postStartControllers

❌ doesn't set cleanup

Server:

❌ doesn't set controllers

✅ sets preStartControllers

✅ sets postStartControllers

✅ sets cleanup

If we decide to keep this, it would look better using functional options, so the constructor would be reduced to

func NewGenericServer(logger logrus.FieldLogger, options ...Option)

The common functionality used by both DS and Server is essentially:

handling shutdown signals

running controllers (although the registration is handled differently between DS and Server)

Pre/PostStartControllers and Cleanup are not used by both.

I don't think it's a burden to have 2 independent Run() implementations each invoke handleShutdownSignals.

We could consider creating a "controller executor" like this:

type ControllerRegistration struct { controller controller.Interface workers int } type ControllerExecutor struct { controllers []ControllerRegistration } func (ce *ControllerExecutor) Execute(ctx context.Context) { var wg sync.WaitGroup for _, c := range ce.controllers { wg.Add(1) go func(c ControllerRegistration) { c.controller.Run(ctx, c.workers) wg.Done() }(c) } }

Both the DS and Server structs could use this. I think I'd prefer an approach of this nature as opposed to the genericServer.

OK. I'll back this out.

skriss · 2018-06-06T04:29:26Z

Updated to just create common funcs for getting loggers, handling signals. Can do any other refactoring later.

ncdc · 2018-06-06T13:42:40Z

Thanks. Squash away.

Signed-off-by: Steve Kriss <[email protected]>

skriss · 2018-06-06T17:02:18Z

squashed and green!

skriss force-pushed the real-restic branch from b4a3e60 to 0facd5b Compare May 21, 2018 17:37

ncdc reviewed May 22, 2018

View reviewed changes

skriss commented May 22, 2018

View reviewed changes

ncdc reviewed May 23, 2018

View reviewed changes

skriss force-pushed the real-restic branch 3 times, most recently from dcd1f05 to 95b82e4 Compare May 25, 2018 04:24

ncdc reviewed May 25, 2018

View reviewed changes

skriss force-pushed the real-restic branch from fb3f8f0 to 3610670 Compare May 25, 2018 21:01

ncdc reviewed May 27, 2018

View reviewed changes

skriss force-pushed the real-restic branch 3 times, most recently from 1e92200 to 67cd96f Compare May 30, 2018 22:54

ncdc reviewed Jun 4, 2018

View reviewed changes

skriss force-pushed the real-restic branch 2 times, most recently from 363b3de to 64e0ca4 Compare June 4, 2018 21:45

add kube listers/informers to vendor/

c2c5b90

Signed-off-by: Steve Kriss <[email protected]>

skriss force-pushed the real-restic branch from 64e0ca4 to 090ece8 Compare June 4, 2018 21:53

skriss force-pushed the real-restic branch from ffd1520 to 83b1947 Compare June 5, 2018 00:44

ncdc reviewed Jun 5, 2018

View reviewed changes

skriss force-pushed the real-restic branch from 83b1947 to 55409c6 Compare June 6, 2018 04:27

add restic integration for doing pod volume backups/restores

50d4084

Signed-off-by: Steve Kriss <[email protected]>

skriss force-pushed the real-restic branch from 55409c6 to 50d4084 Compare June 6, 2018 16:49

skriss changed the title ~~[WIP] restic integration with Ark~~ restic integration with Ark Jun 6, 2018

ncdc merged commit ed2d7b4 into vmware-tanzu:master Jun 6, 2018

skriss deleted the real-restic branch June 8, 2018 22:11


		resultChan := make(chan *arkv1api.PodVolumeBackup)

		informer := arkv1informers.NewFilteredPodVolumeBackupInformer(

		@@ -0,0 +1,23 @@
		# Copyright 2017 the Heptio Ark contributors.

		@@ -0,0 +1,26 @@
		# Copyright 2018 the Heptio Ark contributors.

restic integration with Ark #508

restic integration with Ark #508

Conversation

skriss commented May 18, 2018 • 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

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 May 23, 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

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 May 25, 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

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

ncdc commented Jun 4, 2018

ncdc commented Jun 4, 2018 via email

skriss commented Jun 4, 2018

skriss commented Jun 4, 2018 • edited Loading

ncdc Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Jun 6, 2018

ncdc commented Jun 6, 2018

skriss commented Jun 6, 2018

skriss commented May 18, 2018 •

edited

Loading

skriss commented Jun 4, 2018 •

edited

Loading

ncdc Jun 5, 2018 •

edited

Loading