Restructure backup format for resource prioritization #132

dgoodwin · 2017-10-12T19:29:25Z

Restructure backups for resource prioritization.

Previously the directory structure separated resources depending on
whether or not they were cluster or namespace scoped. All cluster
resources were restored first, then all namespace resources. Priority
did not apply across both and you could not order any namespace
resources before any cluster resources.

This restructure sorts first on resource type.

resources/serviceaccounts/namespaces/ns1.json
resources/nodes/cluster/node1.json

This will break old backups as the format is no longer consistent as
announced on the Google group.

heptibot · 2017-10-12T19:29:26Z

Can one of the admins verify this patch?

dgoodwin · 2017-10-13T14:59:18Z

@ncdc I think this is ready to be looked at now.

ncdc · 2017-10-13T16:05:01Z

@dgoodwin thanks, I'll start reviewing shortly. When we're ready to merge, you'll need to make sure your commit(s) have signoff (git commit --signoff) as per https://github.com/heptio/ark/blob/master/CONTRIBUTING.md.

ncdc · 2017-10-13T16:06:44Z

ok to test

ncdc · 2017-10-13T16:39:13Z

pkg/backup/backup.go

@@ -473,9 +473,9 @@ func (ib *realItemBackupper) backupItem(ctx *backupContext, item map[string]inte

 	var filePath string
 	if namespace != "" {
-		filePath = strings.Join([]string{api.NamespaceScopedDir, namespace, groupResource.String(), name + ".json"}, "/")
+		filePath = strings.Join([]string{api.ResourcesDir, groupResource.String(), api.NamespaceScopedDir, namespace, name + ".json"}, "/")


filepath.Join

ncdc · 2017-10-13T16:39:20Z

pkg/backup/backup.go

 	} else {
-		filePath = strings.Join([]string{api.ClusterScopedDir, groupResource.String(), name + ".json"}, "/")
+		filePath = strings.Join([]string{api.ResourcesDir, groupResource.String(), api.ClusterScopedDir, name + ".json"}, "/")


filepath.Join

ncdc · 2017-10-13T16:41:02Z

pkg/restore/restore.go


-	nses, err := ctx.fileSystem.ReadDir(namespacesPath)
+	resourceDirs, err := ctx.fileSystem.ReadDir(resourcesDir)


Do you think there's any performance advantage one way or another between reading the resources dir up front like this, and just calling DirExists on each resource from ctx.prioritizedResources?

I can't come up with a theory one way or the other, would have to test but in context of restore my guess would be not significant at this point. (backup might be another story as I assume people might be running that a lot more)

If worthwhile let me know and I'll test next week.

ncdc · 2017-10-13T16:41:08Z

pkg/restore/restore.go

-		if !namespaceFilter.ShouldInclude(ns.Name()) {
-			ctx.infof("Skipping namespace %s", ns.Name())
-			continue
+		resourcePath := path.Join(resourcesDir, rscDir.Name())


filepath.Join

ncdc · 2017-10-13T16:41:11Z

pkg/restore/restore.go

-			continue
+		resourcePath := path.Join(resourcesDir, rscDir.Name())
+
+		clusterSubDir := path.Join(resourcePath, api.ClusterScopedDir)


filepath.Join

ncdc · 2017-10-13T16:42:12Z

pkg/restore/restore.go

+		if clusterSubDirExists {
+			w, e := ctx.restoreResource(resource.String(), "", clusterSubDir)
+			merge(&warnings, &w)
+			merge(&errors, &e)


continue?

Must be no such thing as a resource that could be cluster or namespace scoped. Will add that in.

ncdc · 2017-10-13T16:42:31Z

pkg/restore/restore.go

+				if !nsDir.IsDir() {
+					continue
+				}
+				nsPath := path.Join(nsSubDir, nsDir.Name())


filepath.Join

ncdc · 2017-10-13T16:42:34Z

pkg/restore/restore.go

+			merge(&errors, &e)
+		}
+
+		nsSubDir := path.Join(resourcePath, api.NamespaceScopedDir)


filepath.Join

ncdc · 2017-10-13T16:43:33Z

pkg/restore/restore.go

+				nsName := nsDir.Name()
+
+				// fetch mapped NS name
+				// TODO: what is a mapped ns name? for renames?


This is when you do ark restore create --namespace-mappings foo:bar,hello:goodbye,.... Any resources in foo will be restored to bar instead, etc.

Thanks, I think we might need to make use of this at some point.

ncdc · 2017-10-13T16:44:42Z

pkg/restore/restore.go

+				}
+				if _, err := kube.EnsureNamespaceExists(ns, ctx.namespaceClient); err != nil {
+					addArkError(&errors, err)
+					return warnings, errors


Shouldn't this continue the nsDirs loop on line 318 to try additional namespaces?

Whoah, yeah that's a bad artifact from the refactor, good catch. Changing to continue.

dgoodwin · 2017-10-13T19:19:51Z

Updated and added signoff.

dgoodwin · 2017-10-13T19:20:54Z

One potential point of consideration, should I try to detect and warn about old backup tar.gzs?

Probably a case for backup.tar.gz versioning as well here.

ncdc · 2017-10-13T19:23:15Z

Let's do that in a follow-up, at least the bit about writing some metadata to the backup tarball (and let's make sure we add an issue for that). I'm less concerned right now about detecting the old backup format and bailing out early, although that's probably easy to do, once we add in the metadata.

I am in the process of restructuring pkg/backup so it'll be easier if we do this after my exec hooks land.

dgoodwin · 2017-10-13T19:25:14Z

Works for me.

skriss · 2017-10-17T02:54:21Z

pkg/restore/restore.go

-	exists, err = ctx.fileSystem.DirExists(namespacesPath)
+	// Make sure the top level "resources" dir exists:
+	resourcesDir := filepath.Join(dir, api.ResourcesDir)
+	_, err := ctx.fileSystem.DirExists(resourcesDir)


the bool should be checked, since in the normal case, if the dir doesn't exist, this will return (false, nil).

Nice catch.

skriss · 2017-10-17T02:58:35Z

pkg/restore/restore.go

+			addArkError(&errors, err)
+			return warnings, errors
+		}
+		if nsSubDirExists {


nit: maybe flip this conditional with a continue to reduce nesting for the main block of logic?

skriss · 2017-10-17T02:59:53Z

pkg/restore/restore.go

+					continue
+				}
+
+				nsName := nsDir.Name()


may as well move this up a few lines and eliminate a few more calls to nsDir.Name()

skriss · 2017-10-17T03:01:10Z

pkg/restore/restore.go

-func (ctx *context) restoreNamespace(nsName, nsPath string) (api.RestoreResult, api.RestoreResult) {
+// restoreResource restores the specified cluster or namespace scoped resource. If namespace is
+// empty we are restoring a cluster level resource, otherwise into the specified namespace.
+func (ctx *context) restoreResource(resource string, namespace string, resourcePath string) (api.RestoreResult, api.RestoreResult) {


restoreResource(resource, namespace, resourcePath string)

skriss · 2017-10-17T03:10:22Z

This doc also needs to be updated

Previously the directory structure separated resources depending on whether or not they were cluster or namespace scoped. All cluster resources were restored first, then all namespace resources. Priority did not apply across both and you could not order any namespace resources before any cluster resources. This restructure sorts firstly on resource type. resources/serviceaccounts/namespaces/ns1.json resources/nodes/cluster/node1.json This will break old backups as the format is no longer consistent as announced on the Google group. Signed-off-by: Devan Goodwin <dgoodwin@redhat.com>

dgoodwin · 2017-10-17T11:52:09Z

Comments addressed I think!

ncdc · 2017-10-17T18:17:57Z

Thanks again @dgoodwin!!!

dgoodwin force-pushed the ordering branch 3 times, most recently from 7fcd531 to 575db48 Compare October 13, 2017 14:57

dgoodwin changed the title ~~WIP: Resource Ordering~~ Restructure backup format for resource prioritization Oct 13, 2017

ncdc suggested changes Oct 13, 2017

View reviewed changes

dgoodwin force-pushed the ordering branch from 575db48 to 9db8588 Compare October 13, 2017 19:18

skriss reviewed Oct 17, 2017

View reviewed changes

dgoodwin force-pushed the ordering branch from 9db8588 to 98decbf Compare October 17, 2017 11:48

dgoodwin force-pushed the ordering branch from 98decbf to ed0194c Compare October 17, 2017 11:49

ncdc added this to the v0.5.0 milestone Oct 17, 2017

ncdc added enhancement labels Oct 17, 2017

ncdc approved these changes Oct 17, 2017

View reviewed changes

ncdc merged commit bb5088f into vmware-tanzu:master Oct 17, 2017

ncdc mentioned this pull request Oct 17, 2017

Support restore priority for some namespace resources over some cluster resources. #111

Closed

dgoodwin deleted the ordering branch October 17, 2017 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure backup format for resource prioritization #132

Restructure backup format for resource prioritization #132

dgoodwin commented Oct 12, 2017 •

edited

Loading

heptibot commented Oct 12, 2017

dgoodwin commented Oct 13, 2017

ncdc commented Oct 13, 2017 •

edited

Loading

ncdc commented Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

dgoodwin Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

dgoodwin Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

ncdc Oct 13, 2017

dgoodwin Oct 13, 2017

ncdc Oct 13, 2017

dgoodwin Oct 13, 2017

dgoodwin commented Oct 13, 2017

dgoodwin commented Oct 13, 2017

ncdc commented Oct 13, 2017

dgoodwin commented Oct 13, 2017

skriss Oct 17, 2017

dgoodwin Oct 17, 2017

skriss Oct 17, 2017

skriss Oct 17, 2017

skriss Oct 17, 2017

skriss commented Oct 17, 2017

dgoodwin commented Oct 17, 2017

ncdc commented Oct 17, 2017


		nses, err := ctx.fileSystem.ReadDir(namespacesPath)
		resourceDirs, err := ctx.fileSystem.ReadDir(resourcesDir)

Restructure backup format for resource prioritization #132

Restructure backup format for resource prioritization #132

Conversation

dgoodwin commented Oct 12, 2017 • edited Loading

heptibot commented Oct 12, 2017

dgoodwin commented Oct 13, 2017

ncdc commented Oct 13, 2017 • edited Loading

ncdc commented Oct 13, 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

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

dgoodwin commented Oct 13, 2017

dgoodwin commented Oct 13, 2017

ncdc commented Oct 13, 2017

dgoodwin commented Oct 13, 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 Oct 17, 2017

dgoodwin commented Oct 17, 2017

ncdc commented Oct 17, 2017

dgoodwin commented Oct 12, 2017 •

edited

Loading

ncdc commented Oct 13, 2017 •

edited

Loading