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

distro/rhel84: use a random uuid for XFS partition #1131

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

ondrejbudai
Copy link
Member

@ondrejbudai ondrejbudai commented Dec 8, 2020

Imagine this situation: You have a RHEL system booted from an image produced
by osbuild-composer. On this system, you want to use osbuild-composer to
create another image of RHEL.

However, there's currently something funny with partitions:

All RHEL images built by osbuild-composer contain a root xfs partition. The
interesting bit is that they all share the same xfs partition UUID. This might
sound like a good thing for reproducibility but it has a quirk.

The issue appears when osbuild runs the qemu assembler: it needs to mount all
partitions of the future image to copy the OS tree into it.

Imagine that osbuild-composer is running on a system booted from an image
produced by osbuild-composer. This means that its root xfs partition has this
uuid:

efe8afea-c0a8-45dc-8e6e-499279f6fa5d

When osbuild-composer builds an image on this system, it runs osbuild that
runs the qemu assembler at some point. As I said previously, it will mount
all partitions of the future image. That means that it will also try to
mount the root xfs partition with this uuid:

efe8afea-c0a8-45dc-8e6e-499279f6fa5d

Do you remember this one? Yeah, it's the same one as before. However, the xfs
kernel driver doesn't like that. It contains a global table[1] of all xfs
partitions that forbids to mount 2 xfs partitions with the same uuid.

I mean... uuids are meant to be unique, right?

This commit changes the way we build RHEL 8.4 images: Each one now has a
unique uuid. It's now literally a unique universally unique identifier. haha

[1]: https://github.com/torvalds/linux/blob/a349e4c659609fd20e4beea89e5c4a4038e33a95/fs/xfs/xfs_mount.c#L51

@jkozol
Copy link
Contributor

jkozol commented Dec 8, 2020

Using a "unique universally unique identifier" makes sense to me. I think you will also need to update the image tests. I believe they will fail since the uuid in the manifests will be different than the uuid generated by the tests.

@teg
Copy link
Member

teg commented Dec 11, 2020

I support this! However, note that these things are still not actually universally unique, as the produced images will be copied and deployed repeatedly, so if someone were to somehow mount one of these images inside themselves we'd have the same problem (but that's always the case).

@ondrejbudai ondrejbudai force-pushed the uuid branch 2 times, most recently from d5d2357 to 27c5cdd Compare December 14, 2020 22:01
@ondrejbudai ondrejbudai marked this pull request as draft December 14, 2020 22:02
@ondrejbudai
Copy link
Member Author

This is actually harder than I originally thought. The code is more or less done now but there are no commit messages and the code is not commented. I pushed this just to trigger this CI. I'm marking this as a draft and will finish the documentation part of this PR tomorrow.

require.JSONEqf cannot handle diffs of such a big entity as a manifest is.
It just prints an empty string.

This commit unmarshalls the manifests instead and then uses the cmp library
to make a very nice and readable diff.

Signed-off-by: Ondřej Budai <[email protected]>
Previously, the partition table definition was defined inside an assembler.
This has an issue though: The partitions and filesystems are needed at several
other places, e.g. grub2 stage and fstab stage. As the partition table was
basically hardcoded, this didn't matter - we could just use constants
in these stages. Not ideal but it worked.

This commit changes the behaviour: A partition table is firstly created and
then it's passed to the assembler function where complete assembler options
are created out of it.

To make this change as small as possible, osbuild.QEMUAssemblerOptions type
is used to encode the partition table for now.

Signed-off-by: Ondřej Budai <[email protected]>
Using osbuild.QEMUAssemblerOptions to encode a partition table was weird.
This commit introduces a disk package that contains data types for defining
partition tables. Also, there's a handy function to convert the abstact
partition table to osbuild.QEMUAssemblerOptions.

Signed-off-by: Ondřej Budai <[email protected]>
Now that we have an abstract partition table definition, we can use it to
generate org.osbuild.fstab stage options.

This is extremely nice because it removes magic contains.

Signed-off-by: Ondřej Budai <[email protected]>
Let's use the root partition UUID from the partition table instead of
hardcoding the value.

Signed-off-by: Ondřej Budai <[email protected]>
Imagine this situation: You have a RHEL system booted from an image produced
by osbuild-composer. On this system, you want to use osbuild-composer to
create another image of RHEL.

However, there's currently something funny with partitions:

All RHEL images built by osbuild-composer contain a root xfs partition. The
interesting bit is that they all share the same xfs partition UUID. This might
sound like a good thing for reproducibility but it has a quirk.

The issue appears when osbuild runs the qemu assembler: it needs to mount all
partitions of the future image to copy the OS tree into it.

Imagine that osbuild-composer is running on a system booted from an imaged
produced by osbuild-composer. This means that its root xfs partition has this
uuid:

efe8afea-c0a8-45dc-8e6e-499279f6fa5d

When osbuild-composer builds an image on this system, it runs osbuild that
runs the qemu assembler at some point. As I said previously, it will mount
all partitions of the future image. That means that it will also try to
mount the root xfs partition with this uuid:

efe8afea-c0a8-45dc-8e6e-499279f6fa5d

Do you remember this one? Yeah, it's the same one as before. However, the xfs
kernel driver doesn't like that. It contains a global table[1] of all xfs
partitions that forbids to mount 2 xfs partitions with the same uuid.

I mean... uuids are meant to be unique, right?

This commit changes the way we build RHEL 8.4 images: Each one now has a
unique uuid. It's now literally a unique universally unique identifier. haha

[1]: https://github.com/torvalds/linux/blob/a349e4c659609fd20e4beea89e5c4a4038e33a95/fs/xfs/xfs_mount.c#L51
@ondrejbudai
Copy link
Member Author

This now ready to be reviewed.

A big issue with a unique UUID for root partition was that we hardcoded the UUID at several places around rhel84/distro.go. If we want to use a randomly generated one, we would need to pass it around somehow.

I decided to not implement any ugly hacks and do it in the sanest way I could imagine: The rhel84.pipeline() method now firstly generates a partition table and then the root FS UUID is taken from it. Also, the partition table is used to generate fstab (this is imho pretty cool).

This has two pretty nice benefits:

  1. We define the partition table in one place. No magic constants all around the code.

  2. If we ever want to implement partition table customization, this is a very good first step.

@ondrejbudai ondrejbudai marked this pull request as ready for review December 15, 2020 11:57
Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

Very nice work. Pulling out the partition table like this makes perfect sense.

@ondrejbudai
Copy link
Member Author

RHEL 8 koji-osbuild build failed because we use an old snapshot, see osbuild/koji-osbuild#54
Fedora 32 tests failed because of #1135

None of these issues are connected with this PR, let's merge this.

@ondrejbudai ondrejbudai merged commit 973639d into osbuild:main Dec 15, 2020
@ondrejbudai ondrejbudai deleted the uuid branch December 15, 2020 15:50
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.

3 participants