-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
job snapshots prefix for replication filtering #459
base: master
Are you sure you want to change the base?
job snapshots prefix for replication filtering #459
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Captured some of my comments from the thread in #403 so I don't forget them if we chose to go with the approach implemented in this PR.
zfs/versions.go
Outdated
// Workaround when using JobSnapPrefix because bookmarks are not following | ||
// the snapshotting prefix but they have fixed prefix: | ||
// replicationCursorBookmarkNamePrefix = "zrepl_CURSOR" | ||
if len(o.ShortnamePrefix) > 0 && len(o.Types) == 0 && v.Type == Bookmark && strings.HasPrefix(v.Name, "zrepl_CURSOR") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just allow all bookmarks, probably rename ShortnamePrefix
to SnapPrefix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks
Snapshotting SnapshottingEnum `yaml:"snapshotting"` | ||
Filesystems FilesystemsFilter `yaml:"filesystems"` | ||
Send *SendOptions `yaml:"send,fromdefaults,optional"` | ||
JobSnapPrefix string `yaml:"replication_snapshots_prefix,optional"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs coverage in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some docs, I hope I did it right and also it is enough, feel free to change anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got some errors with the circleci checks.
It looks better now but the test-go-on-latest-go-release check is still failing, no idea why yet, any help is welcome:)
endpoint/endpoint.go
Outdated
fsvs, err := zfs.ZFSListFilesystemVersions(ctx, lp, zfs.ListFilesystemVersionsOptions{}) | ||
|
||
fsvs, err := zfs.ZFSListFilesystemVersions(ctx, lp, zfs.ListFilesystemVersionsOptions{ | ||
ShortnamePrefix: s.config.JobSnapPrefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have something more flexible than just prefix matching?
See my ideas in #403 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure there is space for improvements, I’ll think about it but I can’t promise anything in a short time.
any update on this? |
This PR will allow job snapshots prefix for replication filtering.
Added optional parameter replication_snapshots_prefix that will tell zrepl to replicate only snapshots with the prefix provided, it is configurable only for push and source jobs and will be filtering the snapshots from the sender side. If not used zrepl will keep working as it used to replicating every snapshot created in the filesystem.
zrepl.yml syntax example:
replication_snapshots_prefix: “zrepl_”
…
Should address #403
Please give it a try and see if it helps