Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support both pre and post backup hooks #243

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

ncdc
Copy link
Contributor

@ncdc ncdc commented Dec 12, 2017

Deprecate backup "hooks" in favor of "pre" hooks:

  • In the Backup, spec.hooks.resources.hooks becomes spec.hooks.resources.pre. The former is still supported, but deprecated.
  • The supported pod annotations hook.backup.ark.heptio.com/* become pre.hook.backup.ark.heptio.com/*. The former is still supported, but deprecated.

Add support for "post" hooks.

Update documentation.

Fixes #244

@ncdc
Copy link
Contributor Author

ncdc commented Jan 4, 2018

Odd that GitHub closed this, since all I did was force-push to my branch in my fork. Reopening.

@ncdc
Copy link
Contributor Author

ncdc commented Jan 4, 2018

@skriss @nrb code ready for review. Once you're happy with it, I'll update the docs.

@@ -0,0 +1,102 @@
/*
Copyright 2017 Heptio Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably update the copyright year now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I originally wrote this in 2017 for KubeCon.

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, just one nit on a comment. Didn't review the unit test changes in detail yet but looks good to continue with docs.

// These constants represent the relative priorities for resources in the core API group. We want to
// ensure that we process pods, then pvcs, then pvs, then anything else. This ensures that when a
// pod is backed up, we can perform a pre hook, then process pvcs and pvs (including taking a
// snapshotService), then perform a post hook on the pod.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshotService -> snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay autocomplete. Thanks for catching.

Signed-off-by: Andy Goldstein <[email protected]>
@ncdc
Copy link
Contributor Author

ncdc commented Jan 5, 2018

Now w/updated docs.

@ncdc ncdc changed the title WIP pre/post hooks Support both pre and post backup hooks Jan 5, 2018
hooks:
# Same content as pre below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you intend to fill this in or just leave it as a ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref, unless you want it filled in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm fine as-is.

# An array of hooks to run after all custom actions and additional items have been
# processed. Currently only "exec" hooks are supported.
post:
# Same content as pre above.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you intend to fill this in or just leave it as a ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref, unless you want it filled in?

itself, and in the Backup spec.
when that pod is being backed up.

Ark versions prior to v0.7.0 only support hooks that execute prior to any custom action processing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be helpful to add an end-to-end outline of the steps in the backup process: first run pre hooks, then run custom actions one by one, backing up any additional items returned before moving on to the next custom action, then run post hooks, then add the item to the tarball. This would help users see the overall process and exactly where hooks fit in, which might be helpful if they're not intimately familiar with how the process proceeds.

I wonder if longer-term we should try to write out full-length docs describing in some detail how the backup and restore operations proceed. That might be helpful, but not necessary for now. cc @Bradamant3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, but let's do it in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@skriss
Copy link
Contributor

skriss commented Jan 5, 2018

lgtm, any more todos @nrb @ncdc?

@ncdc
Copy link
Contributor Author

ncdc commented Jan 5, 2018

None here. LMK when to squash the code review commit.

@skriss
Copy link
Contributor

skriss commented Jan 5, 2018

No squashing needed here, looks good to merge

@ncdc
Copy link
Contributor Author

ncdc commented Jan 5, 2018

Yeah, had my PRs mixed up

@skriss skriss merged commit 4264abd into vmware-tanzu:master Jan 5, 2018
@ncdc ncdc deleted the pre-post-hooks branch March 5, 2018 15:58
github-actions bot pushed a commit to kaovilai/velero that referenced this pull request Mar 15, 2023
Add `make -f Makefile.prow ci` that can be run in prow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants