[pve-devel] [pve-zsync] Discussion/Feedback: Extending pve-zsync to handle intervening snapshots

[Wolfgang Link]

Hello,

I think that's a good idea.

However, I am not sure whether the additional flag described in point 1 is necessary,
since old jobs already have an independent dest dataset.
So the new logic should not conflict it.

I think it would be great to rewrite the "snapshot_get" 
and "snapshot_exists" functions to fit this use case,
In my opinion it makes little sense to copy the function 
and only change dest and source and give the function a new name.

Regards,
Wolfgang

> On 06/15/2020 9:50 PM Bruce Wainer <brwainer at gmail.com> wrote:
> 
>  
> Hello,
> 
> I am considering making an improvement to pve-zsync and submitting it as a
> patch, but I would like some feedback before doing so.
> 
> Problem:
> pve-zsync can't be used multiple times between the same source and
> destination pool. For example, these two commands are allowed:
> pve-zsync create --source 192.168.10.18:101 --name 101-daily --dest
> 192.168.10.14:HDDs/replica-daily
> pve-zsync create --source 192.168.10.18:101 --name 101-weekly --dest
> 192.168.10.14:HDDs/replica-weekly
> But these two are not:
> pve-zsync create --source 192.168.10.18:101 --name 101-daily --dest
> 192.168.10.14:HDDs/replica
> pve-zsync create --source 192.168.10.18:101 --name 101-weekly --dest
> 192.168.10.14:HDDs/replica
> 
> The first set of commands work together because the destination volumes are
> different. However, this means that the destination host will have
> duplicate copies of the source. This is not ideal. The second set of
> commands will not work because the last_snap used for the zfs send/recv
> only takes into account the current job.
> 
> Proposal
> Flip the source and destination in snapshot_get() and snapshot_exists(),
> because if the last snapshot on the destination doesn't exist on the
> source, the sync is going to fail anyway (ZFS will error with "cannot
> receive new filesystem stream: destination has snapshots").
> 1. Add a new command option "use_last_dest_snapshot" (add to help, to cron
> generation, etc). If this is set, the new logic will be used.
> 2. Add "sub snapshot_get_dest" which is similar to snapshot_get() except it
> performs its checks on the destination, not the source. Also, last_snap
> would be the last snapshot of all, not just the last one related to the
> current job.
> 3. Add "sub snapshot_exists_source" which is the same as snapshot_exists()
> except it performs its checks on the source, not the destination. I would
> also improve the error message if the snapshot doesn't exist on the source.
> 4. In "sub send_image", if " use_last_dest_snapshot " is set then the new
> functions would be called, otherwise the old functions would be called.
> 
> This will be my first time submitting changes to a project, but I believe I
> am capable of writing code that meets your code standards. I am aware of
> the style guide and will submit the CLA.
> 
> Please let me know what you think about my proposed changes.
> 
> Thank you,
> Bruce Wainer
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel