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

Add channel program for taking recursive snapshots based on property #9050

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

clinta
Copy link
Contributor

@clinta clinta commented Jul 17, 2019

Motivation and Context

This is a contribution of a channel program that takes recursive snapshots as suggested by @kpande in #8443 .

Description

Creates the directory contrib/zcp where useful community maintained channel programs can kept and starts with a channel program to satisfy the request in #8443.

How Has This Been Tested?

Successfully ran the channel program.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@clinta clinta marked this pull request as ready for review July 17, 2019 17:36
@ahrens ahrens requested a review from shartse July 17, 2019 17:42
@ahrens ahrens added the Status: Code Review Needed Ready for review and testing label Jul 17, 2019
@clinta clinta force-pushed the contrib_zcp branch 2 times, most recently from 8f7b59f to e4a87e4 Compare July 17, 2019 20:40
@clinta
Copy link
Contributor Author

clinta commented Jul 17, 2019

Added the ability to specify the property as an optional argument
Usage: zfs program <pool> autosnap.lua [noop] [<property>] <snapshot>

@clinta
Copy link
Contributor Author

clinta commented Jul 17, 2019

The <pool> parameter is required by zfs program, but the channel program itself operates on whatever dataset you specify, just as if you did zfs snapshot.

So zfs program tank autosnap.lua tank/foo/bar@backup only creates tank/foo/bar@backup and @backup snapshots of any child datasets of tank/foo/bar. It does not touch anything else in the tank pool.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This is great, I think having useful example channel programs in contrib will be very helpful.

Would it be worthwhile to consider going one step father and include these examples in the packages? If so, where would be the best place to install them by default? Are there other existing scripts we should consider adding at the same time?

Either way, we'll need to make sure these new files are tracked by the build system. You'll need to add a Makefile.am to the new zcp directory, until we determine where these should be installed it can be very simple, just one line should work.

nodist_zcp_SCRIPTS = autosnap.lua

And add the new directory to SUBDIRS and DIST_SUBDIRS in contrib/Makefile.am, and lastly reference the new Makefile in the top-level configure.ac. Like so.

diff --git a/configure.ac b/configure.ac
index 9e4f8ff11830..ea8dc872a755 100644
--- a/configure.ac
+++ b/configure.ac
@@ -133,6 +133,7 @@ AC_CONFIG_FILES([
        contrib/initramfs/hooks/Makefile
        contrib/initramfs/scripts/Makefile
        contrib/initramfs/scripts/local-top/Makefile
+       contrib/zcp/Makefile
        contrib/pyzfs/Makefile
        contrib/pyzfs/setup.py
        module/Makefile
diff --git a/contrib/Makefile.am b/contrib/Makefile.am
index 81926a83ee69..9a82f82ee387 100644
--- a/contrib/Makefile.am
+++ b/contrib/Makefile.am
@@ -1,2 +1,2 @@
-SUBDIRS = bash_completion.d dracut initramfs pyzfs
-DIST_SUBDIRS = bash_completion.d dracut initramfs pyzfs
+SUBDIRS = bash_completion.d dracut initramfs pyzfs zcp
+DIST_SUBDIRS = bash_completion.d dracut initramfs pyzfs zcp

end

noop = false
-- 'noop' is a workaround for issue #9056
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to include the proposed fix, adding the + prefix to optstring from #9056 (comment) so we wouldn't need this workaround. Applying it only to zfs program for now should minimize any potential compatibility problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should that be included here, or should that fix be it's own PR?

@behlendorf behlendorf requested a review from sdimitro July 18, 2019 00:31
@clinta clinta force-pushed the contrib_zcp branch 2 times, most recently from 3e53c4c to dbcf31f Compare July 18, 2019 01:39
@clinta
Copy link
Contributor Author

clinta commented Jul 18, 2019

@behlendorf thanks to your comments on #9056 I was able to remove the workaround and just document that the program needs to be called with a -- between the name of the script and the parameters.

This makes the argument processing quite a bit simpler and no longer position dependent.

The usage is now zfs program <pool> autosnap.lua -- [-n] [-p <property>] <snapshot>

@clinta
Copy link
Contributor Author

clinta commented Jul 18, 2019

Still struggling to get this to build. Looks like there's something more is need in contrib/zcp/Makefile.am.

The TEST builds are failing with make[5]: *** No rule to make target 'autosnap.lua', needed by 'all-am'. Stop.

@behlendorf
Copy link
Contributor

@clinta sorry about the Makefile.am, for now let's keep it super simple. This should work.

-zcpdir = $(datarootdir)/zcp
-nodist_zcp_SCRIPTS = autosnap.lua
+EXTRA_DIST = autosnap.lua

@Fabian-Gruenbichler
Copy link
Contributor

Fabian-Gruenbichler commented Jul 19, 2019 via email

@codecov
Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #9050 into master will decrease coverage by 12.15%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9050       +/-   ##
===========================================
- Coverage   78.67%   66.51%   -12.16%     
===========================================
  Files         400      325       -75     
  Lines      121009   103708    -17301     
===========================================
- Hits        95199    68982    -26217     
- Misses      25810    34726     +8916
Flag Coverage Δ
#kernel ?
#user 66.51% <ø> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43a8536...4c5667d. Read the comment docs.

Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Signed-off-by: Clint Armstrong <[email protected]>
@clinta
Copy link
Contributor Author

clinta commented Jul 19, 2019

I'm looking at some of the zcp tests and I think I should be able to add testing for this program.

@behlendorf
Copy link
Contributor

Let's go ahead and add this to contrib/zcp for the moment, then we can follow up as needed with changes to potentially install it and add some test coverage.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 30, 2019
@behlendorf behlendorf merged commit f489458 into openzfs:master Jul 30, 2019
allanjude pushed a commit to allanjude/zfs that referenced this pull request Aug 12, 2019
Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Reviewed-by: Kash Pande <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Clint Armstrong <[email protected]>
Closes openzfs#8443
Closes openzfs#9050
allanjude pushed a commit to allanjude/zfs that referenced this pull request Oct 13, 2019
Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Reviewed-by: Kash Pande <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Clint Armstrong <[email protected]>
Closes openzfs#8443
Closes openzfs#9050
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 24, 2019
Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Reviewed-by: Kash Pande <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Clint Armstrong <[email protected]>
Closes openzfs#8443
Closes openzfs#9050
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Reviewed-by: Kash Pande <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Clint Armstrong <[email protected]>
Closes openzfs#8443
Closes openzfs#9050
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Channel programs that many users find useful should be included with zfs
in the /contrib directory. This is the first of these contributions. A
channel program to recursively take snapshots of datasets with the
property com.sun:auto-snapshot=true.

Reviewed-by: Kash Pande <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Clint Armstrong <[email protected]>
Closes #8443
Closes #9050
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants