Add support for generating incremental streams when pushing volumes #657
Conversation
…ssary for the other new functional test, as well as for other reasons).
… to the new snapshot.
…re-use elsewhere. Also change the behavior slightly so that the results are sorted by creation time rather than name. Previously we considered encoding the creation time in the snapshot name itself but we have not actually done this yet. The creation property that zfs itself maintains lets us get snapshots in a useful order. The incremental send code wants to use the most recent snapshot - so having them ordered by creation time helps.
Also add a generic interface test for the extremely boring case - which happens to be the only case the other implementation handles.
…t should have before. Also fix the test to catch this.
| results = [] | ||
| for snapshot in snapshots: | ||
| try: | ||
| results.append(SnapshotName.from_bytes(snapshot.name)) |
There was a problem hiding this comment.
There seems to be some inversion of abstraction going on here. File an issue.
| ``some`` and ``others`` If no ``Snapshot`` appears in both, ``None`` is | ||
| returned. | ||
| """ | ||
| in_others = set(others).__contains__ |
There was a problem hiding this comment.
I'm sorry @exarkun I didn't understand this 😕 Can you add an explanatory comment. Why not just:
others = set(others)
...
if snapshot in others:
| @@ -165,8 +201,15 @@ def get_path(self): | |||
| return self._mountpoint | |||
|
|
|||
| @contextmanager | |||
| def reader(self): | |||
| """Send zfs stream of contents.""" | |||
| def reader(self, remote_snapshots=None): | |||
There was a problem hiding this comment.
It's not part of this branch, but I find the reader / writer names a little confusing. I associate reader with the receiving end and writer with the sender. Do you think sender and receiver might be better names? For another ticket perhaps.
There was a problem hiding this comment.
I find it confusing too. When we switch to having a protocol I think these will probably just go away and we won't have to worry about the naming.
| # snapshot will be able to exclude the data written above. | ||
| # Ultimately it would be better to have an API the purpose of which | ||
| # is explicitly to take a snapshot and to use that here instead of | ||
| # relying on `reader` to do this. |
|
Thanks @exarkun LGTM too. I left a few additional inline comments. Feel free to ignore any that don't make sense. |
…n-46 Add support for generating incremental streams when pushing volumes.
This partially addresses #46 - but does not completely resolve it.
Added here is support for a new parameter to
Filesystem.readerto specifying which snapshots the receiver has. The implementation uses this to generate an incremental stream if possible. Also added isIFilesystem.snapshotswhich can be used to get a value to pass for this parameter.Work to glue this together with the actual push implementation will come in a follow-up.