Allows explicit include/exclude of namespaces on restores #59

jrnt30 · 2017-08-28T01:20:42Z

Fixes #22

Introduces similar Include/Exclude declaration on the Restore
resource and cli flags
Removed the Namespace attribute
Fixed a few small issues with the backup validation

Signed-off-by: Justin Nauman justin.r.nauman@gmail.com

heptibot · 2017-08-28T01:20:43Z

Can one of the admins verify this patch?

ncdc · 2017-08-29T11:46:16Z

ok to test

skriss

Thanks for this PR, @jrnt30! I'd like to get @ncdc's opinion on whether it's worth retaining support for the Namespaces field/flag at all -- we are still in alpha so some breaking changes are to be expected at this stage. In the meantime, I reviewed and added a few minor comments.

skriss · 2017-08-29T20:36:09Z

pkg/apis/ark/v1/restore.go

+	IncludedNamespaces []string `json:"includedNamespaces"`
+
+	// ExcludedNamespaces contains a list of namespaces that are not
+	// included in the backup.


replace "backup" with "restore"

skriss · 2017-08-29T20:36:28Z

pkg/cmd/cli/restore/create.go

 		NamespaceMappings: flag.NewMap().WithEntryDelimiter(",").WithKeyValueDelimiter(":"),
 		RestoreVolumes:    flag.NewOptionalBool(nil),
 	}
 }

 func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) {
-	flags.Var(&o.Labels, "labels", "labels to apply to the restore")
-	flags.Var(&o.Namespaces, "namespaces", "comma-separated list of namespaces to restore")
+	flags.Var(&o.IncludeNamespaces, "include-namespaces", "namespaces to include in the backup (use '*' for all namespaces)")


replace "backup" with "restore"

skriss · 2017-08-29T20:36:35Z

pkg/cmd/cli/restore/create.go

-	flags.Var(&o.Labels, "labels", "labels to apply to the restore")
-	flags.Var(&o.Namespaces, "namespaces", "comma-separated list of namespaces to restore")
+	flags.Var(&o.IncludeNamespaces, "include-namespaces", "namespaces to include in the backup (use '*' for all namespaces)")
+	flags.Var(&o.ExcludeNamespaces, "exclude-namespaces", "namespaces to exclude from the backup")


replace "backup" with "restore"

skriss · 2017-08-29T20:39:06Z

pkg/controller/restore_controller.go

@@ -278,6 +285,10 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str
 		validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.")
 	}



You should validate the the included/excluded namespaces are a valid set of includes/excludes. See backup controller

skriss · 2017-08-29T20:49:04Z

pkg/controller/restore_controller.go

@@ -278,6 +285,10 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str
 		validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.")
 	}

+	if len(itm.Spec.Namespaces) > 0 && len(itm.Spec.IncludedNamespaces) > 0 {
+		validationErrors = append(validationErrors, "Namespace and ItemNamespaces can not both be defined on the backup spec.")


"Namespaces and IncludedNamespaces cannot both be defined on the restore spec."

jrnt30 · 2017-08-30T02:02:53Z

Thanks for the feedback, I was curious about that as well. The one thing that I hadn't initially thought of is that anyone that has Ark installed then would need to upgrade.

I have seen project, specifically Helm, that have a tight coupling with Server vs. Client version. While it's somewhat frustrating at times, I do see how it would significantly reduce the complexity over time until the API reaches a point of stability.

jrnt30 · 2017-08-31T02:35:43Z

So having a bit more time to look at this properly my tests are failing due to some of the complexities around the handling of the migration of Namespace and the new namespace validation piece. I will wait to hear if we need to support the retro attribute and if not just rework this a bit.

Also as a note, the Jenkins auth doesn't allow viewing of the job currently even with the Github OAuth authorized.

ncdc · 2017-09-05T15:32:25Z

I'm fine removing the old flag. We'll make sure to include the required UX change in the release notes/changelog. Everyone ok with that?

skriss · 2017-09-05T16:11:03Z

Works for me.

ncdc · 2017-09-05T16:13:56Z

@jrnt30 please go ahead and remove the old flag - thanks!

- Introduces similar Include/Exclude declaration on the Restore resource and cli flags - Kept support for legacy Namespaces attribute until it could be deprecating. Defining both IncludeNamespaces and Namespaces results in a validation error and the Restore will not be processed (shouldn't be able to occur) Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>

- Adding in additional test to ensure *Namespaces attributes don't directly conflict logically with one another - Additional PR changes around naming/typos Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>

- Per discussion, there is no reason to deal with the complexity of backwards compatibility with the Namespace attribute on the Restore domain. - Also noticed there was an error on the validation of the BackupController where the message would actually just be the index of the error and not the contents of the message itself. Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>

skriss · 2017-09-06T00:12:49Z

pkg/controller/backup_controller.go

@@ -292,11 +292,11 @@ func cloneBackup(in interface{}) (*api.Backup, error) {
 func (controller *backupController) getValidationErrors(itm *api.Backup) []string {
 	var validationErrors []string

-	for err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedResources, itm.Spec.ExcludedResources) {
+	for _, err := range collections.ValidateIncludesExcludes(itm.Spec.IncludedResources, itm.Spec.ExcludedResources) {


@jrnt30 thanks for catching!

skriss · 2017-09-06T00:13:11Z

updates LGTM, thanks @jrnt30 !

skriss · 2017-09-06T00:13:47Z

@ncdc pls review/merge if you approve.

Update Dockerfile.ubi to use pinned restic

skriss suggested changes Aug 29, 2017

View reviewed changes

skriss reviewed Aug 29, 2017

View reviewed changes

jrnt30 changed the title ~~Allows explicit include/exclude of namespaces on restores~~ WIP: Allows explicit include/exclude of namespaces on restores Aug 31, 2017

jrnt30 added 3 commits September 5, 2017 17:17

Additional Validation on Include/Exclude

b50a046

- Adding in additional test to ensure *Namespaces attributes don't directly conflict logically with one another - Additional PR changes around naming/typos Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>

jrnt30 changed the title ~~WIP: Allows explicit include/exclude of namespaces on restores~~ Allows explicit include/exclude of namespaces on restores Sep 5, 2017

skriss reviewed Sep 6, 2017

View reviewed changes

skriss approved these changes Sep 6, 2017

View reviewed changes

ncdc added this to the v0.4.0 milestone Sep 6, 2017

ncdc merged commit 305e7fc into vmware-tanzu:master Sep 6, 2017

dymurray pushed a commit to dymurray/velero that referenced this pull request Jun 24, 2020

Merge pull request vmware-tanzu#59 from dymurray/fixubi

0759514

Update Dockerfile.ubi to use pinned restic

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows explicit include/exclude of namespaces on restores #59

Allows explicit include/exclude of namespaces on restores #59

jrnt30 commented Aug 28, 2017 •

edited

Loading

heptibot commented Aug 28, 2017

ncdc commented Aug 29, 2017

skriss left a comment

skriss Aug 29, 2017

skriss Aug 29, 2017

skriss Aug 29, 2017

skriss Aug 29, 2017

skriss Aug 29, 2017

jrnt30 commented Aug 30, 2017

jrnt30 commented Aug 31, 2017

ncdc commented Sep 5, 2017

skriss commented Sep 5, 2017

ncdc commented Sep 5, 2017

skriss Sep 6, 2017

skriss commented Sep 6, 2017

skriss commented Sep 6, 2017

		@@ -278,6 +285,10 @@ func (controller restoreController) getValidationErrors(itm api.Restore) []str
		validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.")
		}

Allows explicit include/exclude of namespaces on restores #59

Allows explicit include/exclude of namespaces on restores #59

Conversation

jrnt30 commented Aug 28, 2017 • edited Loading

heptibot commented Aug 28, 2017

ncdc commented Aug 29, 2017

skriss left a comment

Choose a reason for hiding this comment

skriss Aug 29, 2017

Choose a reason for hiding this comment

skriss Aug 29, 2017

Choose a reason for hiding this comment

skriss Aug 29, 2017

Choose a reason for hiding this comment

skriss Aug 29, 2017

Choose a reason for hiding this comment

skriss Aug 29, 2017

Choose a reason for hiding this comment

jrnt30 commented Aug 30, 2017

jrnt30 commented Aug 31, 2017

ncdc commented Sep 5, 2017

skriss commented Sep 5, 2017

ncdc commented Sep 5, 2017

skriss Sep 6, 2017

Choose a reason for hiding this comment

skriss commented Sep 6, 2017

skriss commented Sep 6, 2017

jrnt30 commented Aug 28, 2017 •

edited

Loading