curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+464246@xxxxxxxxxxxxxxxxxx>]

Review: Approve

I think this is fine and probably helps. I think it's a bit overbroad probably but maybe that's not a real problem considering how much overbroad stuff there is in curtin currently...

Diff comments:

> diff --git a/curtin/block/zfs.py b/curtin/block/zfs.py
> index ccdddbb..81ba1e1 100644
> --- a/curtin/block/zfs.py
> +++ b/curtin/block/zfs.py
> @@ -89,6 +90,7 @@ class ZPoolEncryption:
>          zfs_create(
>              self.poolname, "keystore", {"encryption": "off"}, keystore_size,
>          )
> +        udevadm_settle()

I don't know for sure (obviously with racy things like this) but I suspect this might actually be the real fix here: afaict the /dev/zvol/{self.poolname}/keystore node is created by udev. Would it be cleaner if zfs_create called udevadm_settle(exists=$path) before returning? (It might also be nice if it returned the path too?)

>  
>          with ExitStack() as es:
>              for vdev in self.vdevs:
> @@ -97,13 +99,19 @@ class ZPoolEncryption:
>              # cryptsetup format and open this keystore
>              keystore_volume = f"/dev/zvol/{self.poolname}/keystore"
>              cmd = ["cryptsetup", "luksFormat", keystore_volume, self.keyfile]
> -            util.subp(cmd)
> +
> +            # strace has shown that udevd does indeed probe these keystores

FWIW the issue we see with https://systemd.io/BLOCK_DEVICE_LOCKING/ type stuff AIUI is that udev accessing the _parent_ block device causes the _partition_ or child nodes to disappear briefly. I don't think that's what's happening here? If it is you would need to be locking the parent block device but I don't think there is one for zfs?

That said I doubt any of this hurts!

> +            with util.FlockEx(keystore_volume):
> +                util.subp(cmd, capture=True)
> +
> +            udevadm_settle()
> +
>              dm_name = f"keystore-{self.poolname}"
>              cmd = [
>                  "cryptsetup", "open", "--type", "luks", keystore_volume,
>                  dm_name, "--key-file", self.keyfile,
>              ]
> -            util.subp(cmd)
> +            util.subp(cmd, capture=True)

Should this udevadm_settle(exists="f"/dev/mapper/{dm_name}") too? Although device mapper stuff integrates somewhat with udev so maybe not...

>  
>          with ExitStack() as es:
>              # format as ext4, mount it, move the previously-generated systemkey


-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/464246
Your team curtin developers is subscribed to branch curtin:master.