Skip to content
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

Initial MDRAID support #277

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Initial MDRAID support #277

wants to merge 4 commits into from

Conversation

Harvie
Copy link
Contributor

@Harvie Harvie commented Dec 20, 2024

This allows to create level 1 MDRAID with 1 device. Can be empty or prepopulated with data image.
Includes unit test using mdadm to check generated image.

Eg.:

image mdraid-md.img {
  mdraid {
    level = 1
  }
  partition data {
    image = "mdraid-ext4.img"
  }
}

It might sound stupid to create single device raid, but it actualy fits my reallife usecase where i pre-generate such raid when making image and user then can very easily add more devices during runtime when needed later. For example like this:

mdadm --grow /dev/md23 --raid-disks=2 --force
maddm /dev/md23 --add /dev/sdb1

Although it should be simple to pre-generate images for all raid members in genimage. It shouldn't be much harder than generating multiple images with the same RAID UUID and few metadata changes. But i haven't researched that so far.

Also see #191

@Harvie Harvie force-pushed the master branch 3 times, most recently from e3f0cc8 to 3d315c9 Compare December 20, 2024 08:56
@Harvie
Copy link
Contributor Author

Harvie commented Dec 20, 2024

I have no idea why the test fails here. It says file mdraid.config is missing, but it's obviously there.
Also mdraid tests are passing at my PC when i do make check-TESTS:

image

@michaelolbrich
Copy link
Member

https://github.com/pengutronix/genimage/actions/runs/12428468239/job/34700133730?pr=277#step:6:1000

The test run make distcheck which runs the test from the generated tarball. You forgot to add the test config to EXTRA_DIST so it's missing.

Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
@Harvie
Copy link
Contributor Author

Harvie commented Dec 20, 2024

Seems to be fixed now.

image-mdraid.c Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image-mdraid.c Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image-mdraid.c Outdated Show resolved Hide resolved
image_info(image, "MDRAID partition has to be called 'data' instead of '%s'\n", part->name);
} else {
if(img_in) {
image_error(image, "MDRAID cannot contain more than one data partition!\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will never be more than one, right? In that case, take a look at how image-android-sparse.c does this:
Add an single option and create the partition internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was thinking that in future i might use this to reference to other member drives of the same RAID array. in that case there will be other "partitions" in the config.

exec_test_set_prereq mdadm
test_expect_success mdadm "mdraid" "
run_genimage_root mdraid.config test.mdraid &&
mdadm --examine images/test.mdraid &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save the output here in a file, and just grep the file below.

Hmm, or better use test_cmp to compare the output with a file. With fixed uuids and timestamp, the output should be reproducible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can do that, but i didn't really wanted to be dependend on exact MDADM version... Hence checking individual fields in the output rather than comparing whole thing, so it keeps working in case they add field or something...

@Harvie Harvie force-pushed the master branch 3 times, most recently from 0f935d6 to 02d0983 Compare December 20, 2024 13:29
Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
@Harvie
Copy link
Contributor Author

Harvie commented Dec 21, 2024

Happy holidays.
@michaelolbrich I think i've resolved most of your objections, but i think i could really use your guidance about the multi-image thing. While i am not gonna implement generating multi device raid now in the first mdraid PR, i think it's bound to happen in the future and i would like to prepare the codebase for it, so we don't have to change config syntax in the future. What do you think would be the correct way of doing that?

It would kinda make sense for the image-mdraid.c to actualy generate two (or more) images, all being part of that RAID1 mirror. But on the other hand i don't think this would allow genimage to resolve dependencies correctly. Maybe i should manualy create other image structs and add them to list in mdraid_setup() ? would that be enough for image-hd to find them and trigger the build of all of them when needed? what would you suggest?

@Harvie
Copy link
Contributor Author

Harvie commented Dec 21, 2024

One approach might be specifing all the output images of that array in single mdraid image node.

But maybe i should just do something like this instead:

image raid1-a.img {
  mdraid {
    level = 1
    devices = 2
  }
  partition data {
    image = "mdraid-ext4.img"
  }
}

image raid1-b.img {
  mdraid {
    master = raid1-a.img    #most of the config is gonna be inherited from master
    position = 2      #this is 2nd device in the array described by master image
  }
}

and then B image can lookup all the configuration from A image config node... but still this does not guarantee that B will be only generated after A. (UUIDs and other details of the A image need to be decided before generating B).

Is there way to enforce dependency?

@Harvie
Copy link
Contributor Author

Harvie commented Dec 22, 2024

OK, i did binary diff of superblocks of two disks belonging to same RAID1 and highlighted the important parts:

image

Some things i am confused about:

  • Dev numbers 0 and 2 (why skipped 1? maybe the first disk should be 1, not 0?)
  • Why is there 3rd role with 0x0100 ???? Maybe the system is really confused about device ids starting at 0

Update: oh, the active role is simply number of the disk in the array (not sure why, because we already have DEV_NUMBER)

[harvie@anemophobia mdadm]$ grep DISK_ROLE md_p.h 
#define MD_DISK_ROLE_SPARE	0xffff
#define MD_DISK_ROLE_FAULTY	0xfffe
#define MD_DISK_ROLE_JOURNAL	0xfffd
#define MD_DISK_ROLE_MAX	0xff00 /* max value of regular disk role */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants