raid: add assemble option #1826

lehmanju · 2024-03-04T17:01:34Z

Adds option to assemble raid devices before creating new ones. Allows for persistent raid configurations without wiping existing data.

Haven't tested this yet, but my main goal is to provide an option to reassemble existing raids when reprovisioning. Looking forward to feedback.

adds option to assemble raid devices before creating new ones. allows for persistent raid configurations without wiping existing data.

lehmanju · 2024-03-15T20:57:04Z

Corresponding issue #579

prestist · 2024-03-26T19:28:00Z

@lehmanju thank you for working on this!

Firstly have you gotten a chance to test this?

prestist

Again, thank you for working on this, it looks sane from my understanding, it is missing some translation bits, and a few nits here and there. lmk.

prestist · 2024-03-26T19:30:24Z

config/doc/ignition.yaml

@@ -168,6 +168,8 @@ root:
              desc: the number of spares (if applicable) in the array.
            - name: options
              desc: any additional options to be passed to mdadm.
+            - name: assemble
+              desc: try to assemble raid array from the list of devices before creating it. Defaults to false.


Hmm, wdyt "Attempts to assemble the RAID array from the list of devices before creating it. Defaults to false."

prestist · 2024-03-26T19:40:45Z

internal/exec/stages/disks/raid.go

+				"assembling %q", md.Name,
+			); err != nil {
+				s.Logger.Info("mdadm assemble failed: %v", err)
+			} else {


I think we might be able to simplify some of this nesting wdyt?

prestist · 2024-03-26T19:41:43Z

internal/exec/stages/disks/raid.go

+			}
+
+			if _, err := s.Logger.LogCmd(
+				exec.Command(distro.MdadmCmd(), args...),


it might help readablity if we make this command a var

aka cmd:= exec.Command(distro.MdadmCmd(), args...)

not super opinionated on this, just a passing thought.

prestist · 2024-03-26T19:42:37Z

internal/exec/stages/disks/raid.go

@@ -74,6 +74,9 @@ func (s stage) createRaids(config types.Config) error {
 				if err := s.waitOnDevices([]string{devName}, "raids"); err != nil {
 					s.Logger.Info("mdadm assemble failed: %v", err)
 				} else {
+					if err := s.waitOnDevices([]string{devName}, "raids"); err != nil {


Could we add a comment for this, or explain it a bit in the commit message?

prestist · 2024-03-26T19:45:41Z

config/v3_5_experimental/schema/ignition.json

@@ -236,6 +236,9 @@
              "items": {
                "type": "string"
              }
+            },
+            "assemble": {


Since you added a struct field you need to add a translation for it in translations, this is why CI is failing.

prestist · 2024-03-26T19:46:24Z

config/v3_5_experimental/types/schema.go

-	Name    string       `json:"name"`
-	Options []RaidOption `json:"options,omitempty"`
-	Spares  *int         `json:"spares,omitempty"`
+	Assemble *bool        `json:"assemble,omitempty"`


Since you added a struct field you need to add a translation for it in translations, this is why CI is failing.

lehmanju · 2024-03-29T20:36:33Z

Had no time to test this yet. Was also looking for a rough opinion on behavior first. Because "assemble" would just try to assemble any previous raids, even if they do not match the current (possibly changed) configuration.

prestist · 2024-04-04T19:07:51Z

@lehmanju it might make sense to bring this up in the fcos meeting, we have one weekly at 16:30 UTC.

jlebon · 2024-04-05T17:37:18Z

This is doing assembly, but not checking that the assembled RAID matches the specification, right?

I think also the semantics should match all the other objects with reuse semantics: a field named e.g. wipeRaid which default to false (allows reuse, errors if it doesn't match spec) but if set to true always wipes it first.

lehmanju · 2024-04-06T07:23:37Z

This is doing assembly, but not checking that the assembled RAID matches the specification, right?

Correct.

The problem I recall is that additional options for raid creation are passed directly to mdadm. Ideally, its not possible to pass random options but to have separate fields for them. Then checking whether the specification matches is easier. But even then it might only be possible after assemble, since raid properties are not visible up front.

I wanted to implement a minimal, non breaking change, hence a simple assemble option.

jlebon · 2024-04-12T19:50:16Z

This is doing assembly, but not checking that the assembled RAID matches the specification, right?

Correct.

The problem I recall is that additional options for raid creation are passed directly to mdadm. Ideally, its not possible to pass random options but to have separate fields for them. Then checking whether the specification matches is easier. But even then it might only be possible after assemble, since raid properties are not visible up front.

I wanted to implement a minimal, non breaking change, hence a simple assemble option.

You're right that with the options key it becomes impossible to verify everything. I think that's OK though. Compare to the luks implementation; we only check the bits we do spec out before considering it reusable. We should document the limitations of our reuse logic.

For raid, we could check level, devices, and spares.

Note that mdadm --examine can be used to analyze a RAID member without activating the device. That'll give info on the RAID level. The number of devices and spares can be deduced too, but it's a bit awkward. It seems fine too to activate it to make analyzing easier. (And then disassemble it if it doesn't match.)

I think also the semantics should match all the other objects with reuse semantics: a field named e.g. wipeRaid which default to false (allows reuse, errors if it doesn't match spec) but if set to true always wipes it first.

One thing I realized is that the problem with defaulting to false is that it's a behavioural change. We would have to default to true to keep the same behaviour, which is OK though a bit awkward being that all the other wipe* options default to false.

lehmanju · 2024-04-15T18:13:49Z

I just checked the current behavior: All raids are automatically assembled when found. No additional configuration is required; FCOS does this by default.

Therefore, adding the option wipeRaid is probably the best solution. If set to false, we check whether it matches and do nothing further. If set to true, we create a new raid as previously.

jlebon · 2024-04-15T21:09:33Z

I just checked the current behavior: All raids are automatically assembled when found. No additional configuration is required; FCOS does this by default.

Was this in the real root or in the initramfs? AFAIK, the mdraid udev rules in the initramfs will purposely not auto-assemble any array except those specified by rd.md.uuid.

lehmanju · 2024-05-03T04:58:36Z

It was the real root, in the initramfs RAID is not assembled.

As it turns out, I sadly don't find the time to continue working on this. I'll have to close this PR.

jlebon · 2024-05-03T13:15:19Z

@lehmanju That's fair. Nonetheless, thanks for the discussion. I think it will help anyone who has cycles to pick this up again in the future.

lehmanju added 2 commits March 4, 2024 17:59

raid: add assemble option

5bf0bb7

adds option to assemble raid devices before creating new ones. allows for persistent raid configurations without wiping existing data.

raid: add wait

34c5429

prestist reviewed Mar 26, 2024

View reviewed changes

lehmanju closed this May 3, 2024

jlebon mentioned this pull request May 3, 2024

Add Ignition options to create partition or RAID volume only if missing #579

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raid: add assemble option #1826

raid: add assemble option #1826

lehmanju commented Mar 4, 2024

lehmanju commented Mar 15, 2024

prestist commented Mar 26, 2024

prestist left a comment •

edited

Loading

prestist Mar 26, 2024

prestist Mar 26, 2024

prestist Mar 26, 2024

prestist Mar 26, 2024

prestist Mar 26, 2024

prestist Mar 26, 2024 •

edited

Loading

prestist Mar 26, 2024

lehmanju commented Mar 29, 2024

prestist commented Apr 4, 2024

jlebon commented Apr 5, 2024

lehmanju commented Apr 6, 2024

jlebon commented Apr 12, 2024

lehmanju commented Apr 15, 2024

jlebon commented Apr 15, 2024

lehmanju commented May 3, 2024

jlebon commented May 3, 2024

raid: add assemble option #1826

raid: add assemble option #1826

Conversation

lehmanju commented Mar 4, 2024

lehmanju commented Mar 15, 2024

prestist commented Mar 26, 2024

prestist left a comment • edited Loading

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

prestist Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

prestist Mar 26, 2024

Choose a reason for hiding this comment

lehmanju commented Mar 29, 2024

prestist commented Apr 4, 2024

jlebon commented Apr 5, 2024

lehmanju commented Apr 6, 2024

jlebon commented Apr 12, 2024

lehmanju commented Apr 15, 2024

jlebon commented Apr 15, 2024

lehmanju commented May 3, 2024

jlebon commented May 3, 2024

prestist left a comment •

edited

Loading

prestist Mar 26, 2024 •

edited

Loading