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

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

Hi Ryan,

first of all thank you for your answer and sorry for the long reply:

> [...]
> A two-drive RAID5 is RAID1 with different metadata format. Why
> not just create a RAID1 with your two disks? IIUC, you can
> already convert RAID1 to RAID5.
>
> I believe users who do understand they are creating RAID5 in
> degraded mode and that they can grow their RAID5 later also
> know they can convert a RAID1 into RAID5 as well.
>
> The expansion of an array is out-of-scope for the installer where
> as producing non-degraded in the least confusing configurations
> is very much in-scope.
One important point first: A two component RAID5 is absolutely NOT a degraded array.
Since this is really an important point, let me detail this further:
With two components you have two possibilities to create a RAID5:
 a) two component RAID5 (non-degraded)
 b) three component RAID5 (degraded)

There are several differences between both configurations:
The capacity of b) is double the capacity of a).
On the other hand if you lose one drive, in a) case your data is still existing - b) already lost it.
Also, you will have a performance impact on b) since the RAID has to reconstruct (CPU!) (due to degraded state)
the data blocks which would be put to the third component device.
Regarding the data arrangement on the component devices, it would look like the following:
a)
0 1 <-- component
A P1(=A)
P2(=B) B
C P3(=C)
....

b)
0 1 2(missing) <-- component
A B P1(=A^B)
C P2(=C^D) D
P3(=E^F) E F
....

So I am _not_ asking for the degraded array feature which is for sure an "expert"/corner case feature.

Let me hijack your "semi-educated" user standpoint:
A user with the following requirements:
 - Availability (survives one component failure)
 - Expandability (Can add space on demand)

will stumble quite easily over the "grow" feature of mdadm for RAID5. Thus he might - after minor research - select two disk RAID5 happily if the installer allows. On the other hand - if the installer only allows RAID1, it is not so easy to find/imagine that this actually also satisfies his expandability requirement.

>> 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?).

> The 'missing' keyword is interesting. This does allow creating
> an array of larger number of disks without all being present.
> I'm still finding this to be a dubious "feature" for the initial
> installer.
> [...]
As mentioned above, I did not want to propose the creation of degraded arrays.
I was wondering, why you implement the checks and error messages in curtin instead of just issuing the mdadm command and in case of an error propagating back the error messages from mdadm (which are often quite explanatory).

>> 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").

> For min-drives on RAID5 it's not related. For RAID10 my
> understanding is that the default layout for RAID10 would not
> give you the optimal single thread perf the linux-raid thread
> I found[1] indicated that f2 would be best (but this may be dated).
>
> 1. https://marc.info/?l=linux-raid&m=141051176728630&w=2
You are absolutely right and this is still valid information.
Still, I would propose to handle this in a subsequent PR to not intermix a "bugfix" with new feature.

> A big portion of what curtin is providing is a language in which
> to describe your storage configuration. Subiquity is a consumer
> of this language and in-short is a user-facing tool to help
> construct curtin's storage configuration files.
>
> Generally curtin delegates behavior choices to the constructor
> of the configuration. That is as Michael pointed out, curtin
> shouldn't second guess that someone wants to construct a two-disk
> RAID5 array etc. I generally agree with that perspective.
>
> The other end of this is that without guidance users end up
> constructing things that don't actually work or are sub-optimal
> and the blame falls to the installer for letting users footgun
> their server. Subiquity goes through great efforts to prevent
> users from constructing configurations which result in an
> unbootable server even if curtin is more than happy to do so.
>
> If we are to enable these special cases then we must provide
> configuration language sufficient to describe what is possible
> rather than only the special cases and be explicit about it.
>
> For example, the 2-disk raid5 array config could look like:
>
> - type: raid
> level: 5
> raid_devices: 3
> devices:
> - /dev/sda
> - /dev/sdb

As written above, this would be a degraded raid which I did not want to propose.
Thus the raid_devices are for my needs redundant (and in my case = 2).
I also mentioned in the subiquity bug report that the impact for the "unintentional"
two disk RAID5 user is not too severe compared to a RAID1. For details, please have a look there.

> And the RAID10 array with specific layout could be
>
> - type: raid
> level: 10
> raid_devices: 2
> devices:
> - /dev/sda
> - /dev/sdb
> layout: f2
Same here, raid_devices would be unnecessary unless you want to support it.

> However, we'd like to sanity check these configurations, for
> example
>
> - type: raid
> level: 5
> raid_devices: 5
> devices:
> - /dev/sdb
> - /dev/sdc
>
> Will fail at runtime (as I showed above) but this is a poor
> experience when curtin's block schema can and should validate
> the configuration and reject it indicating that RAID5 can only
> allow one missing device.
Not sure if it is an option for curtin to run the command "as-is":
 mdadm --create -l 5 -n 5 /dev/md0 /dev/sdb /dev/sdc
and pass back the (quite descriptive) mdadm error message "You haven't given enough devices (real or missing) to create this array".

> Likewise, adding layout support will need additional checks
> and validations so that the install experience isn't painful
> and the language we use to describe what we want isn't ambiguous
> allowing curtin to incrementally add feature/function over time.
Again, if it is an option for you, you can just pass everything to mdadm an receive a good explanation if something went wrong. This way you avoid duplicating the logic and error handling of mdadm.
$ mdadm --create -l 5 -n 2 -p argl /dev/md0 /dev/sdb /dev/sdc
mdadm: layout argl not understood for raid5.

Apart from that, the "layout" keyword would be really nice.
As a first step, this could be only valid for RAID10, since the layout of the other levels are probably not too interesting (at least I do not see a strong argument why anyone would favor right-symmetric over left-symmetric or the like).

Best regards,
 Thimo

« Back to merge proposal