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
Zfs action ordering #1730
Zfs action ordering #1730
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.
the safe_loads make me twitch, otherwise ok
c3ed6f6
to
a258f20
Compare
@jpnurmi - when this PR is merged I believe guided installs with the 'ZFS' flag are ready |
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.
Hi, thanks for cleaning that up, much nicer now.
I have some questions but don't need to review again once you've answered them (unless you want me to!)
@@ -1711,6 +1756,12 @@ def _all(self, **kw): | |||
def all_mounts(self): | |||
return self._all(type='mount') | |||
|
|||
def all_mountlikes(self): |
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 almost wonder if you should change all_mounts to do this. It would break the server tui in a more obvious way than the slightly subtle way leaving it alone does :-)
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.
So I setup dryrun on this branch and sent v2/guided capability == 'ZFS' and went into manual partitioning, and it exposes all sorts of fun. I think I want to ponder this all in a future branch.
|
||
@property | ||
def canmount(self): | ||
return get_canmount(self.properties, False) |
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.
The previous version sure looked like it had different defaults for canmount for zfs and zpool objects. Want to check that detail? (I certainly don't know what is correct here!)
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.
Intentional! I have found it helpful to think of a ZPool as implicitly creating a ZFS object, and the fs_properties on the zpool should be equivalent to the properties on a zfs.
Then I made sure the above matched curtin's default behavior, and I think we should revisit that, as curtin defaults canmount='off' while zfsutils-linux defaults to canmount='on', for both a zfs dataset and the implicit zfs dataset created by the zpool.
So for right now I think this part is correct, and we should ponder https://git.launchpad.net/curtin/tree/curtin/block/zfs.py#n19 .
handle that also.""" | ||
if properties is None: | ||
return default | ||
vals = { |
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.
That looks like a good use case for python's match construct. Our focal's CI still runs on python 3.8, right?
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.
It does. I'm going to reluctantly decline at this point, as I do think running the integration tests on focal has some value.
Address action ordering with zpool/zfs objects - these objects are 'Mountlike' objects and may represent a viable mountpoint, which affects rootfs detection and the required ordering of curtin storage actions.