Code review comment for ~mwhudson/curtin:raid-min-levels

Revision history for this message
Thimo E (thimoe) wrote :

Hi Ryan,
I am the author of the PR (https://github.com/canonical/subiquity/pull/929) as well as the originating bug report (https://bugs.launchpad.net/subiquity/+bug/1880023).
Thanks for considering the changes. A few comments from my side:
I think this change is first of all a fix for a bug. The bug is that the code and the comment above state these values are "minimum number of devices for a given raid level". The original numbers are incorrect in this regard (see https://github.com/canonical/subiquity/pull/929 for pointers to the relevant information sources).

Also there are valid use-cases for both reductions. For (linux) RAID10 it is obviously single stream read speed and for RAID5 the main reason is expandability.

Regarding your RAID5 comment: a RAID5 with two components is not missing parity. The parity is just coincidentally the data itself which means the data on the device is arranged (parity distribution on equal data will not have an effect) like a RAID1. Nevertheless the metadata is RAID5 and also the upgrade path using "grow" is more obvious (RAID5 two component to RAID5 three component). Of course it is possible to do the intermediate step (RAID1 two components to RAID5 two components) before.

As a side-note, "mdadm" complains if the creation is missing some devices and it is not explicitly told "missing", so I am generally wondering why it is necessary to repeat the check in the installer (due to missing dry-run feature of mdadm?).

Regarding the layout support, I think this is a much appreciated feature but it is not a dependency to fix the current issue.
Since we are at the layout topic: I am not sure if there is much interest for the layout feature beyond RAID10. Thus it might also be an option to have two(/three) different RAID10 modes like RAID10near and RAID10far. The main question then would be if it is safe to assume the number of copies to be 2 (from mdadm man page: "2 is normal, 3 can be useful").

BR

« Back to merge proposal