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

Fixed hardcoded root partition device name sda2 #822

Merged
merged 1 commit into from
Apr 7, 2024
Merged

Conversation

nanjj
Copy link
Contributor

@nanjj nanjj commented Apr 2, 2024

The root partition may not be /dev/sda2, for example if set the boot disk as the second disk, the root partition device name will be /dev/sdb2. It may be /dev/vda2 if not using san disk. The fix figured out the root partition device UUID and set the UUID as the root, the root partition device UUID keeps no change.

Depends on lxc/distrobuilder#830

@nanjj nanjj force-pushed the main branch 2 times, most recently from d484e97 to a6af75a Compare April 2, 2024 02:07
@stgraber
Copy link
Member

stgraber commented Apr 2, 2024

Hmm, I like the idea behind the change but I'm not sure that the implementation is going to be safe. We frequently build multiple images in parallel on the same system which means that any kind of wildcard pattern around /dev/loop*p2 is going to be racy.

@nanjj
Copy link
Contributor Author

nanjj commented Apr 2, 2024

Hmm, I like the idea behind the change but I'm not sure that the implementation is going to be safe. We frequently build multiple images in parallel on the same system which means that any kind of wildcard pattern around /dev/loop*p2 is going to be racy.

The code is running in the build namespaces, you can check the distrobuilder code, only the free loop device used by this build being created in this build. So I think it's OK. Another idea is to pass environment variable say ROOT_PARTITION_DEVICE_NAME or ROOT_PARTITION_DEVICE_UUID to the POST SCRIPT but that may lead to distrobuilder code change.

@stgraber
Copy link
Member

stgraber commented Apr 2, 2024

Yeah, I'd actually prefer we change distrobuilder to expose env variables, that'd be far more reliable.

/dev is usually a devtmpfs or can get mounted as one by a package during the build process, devices aren't namespaced so loop devices will show up in all containers/namespaces that also use devtmpfs.

@nanjj
Copy link
Contributor Author

nanjj commented Apr 2, 2024

The code I mentioned here:
https://github.com/lxc/distrobuilder/blob/main/distrobuilder/vm.go#L164
So I do not think all loop devices in host will show up in all containers.

@nanjj nanjj force-pushed the main branch 2 times, most recently from 42fe395 to 3b382b0 Compare April 2, 2024 07:01
@stgraber
Copy link
Member

stgraber commented Apr 2, 2024

Note line 149, the logic you're pointing out is fallback logic for the images that do not have a full /dev already available. The code as it's written actually shows that distrobuilder was meant to support images building with a full /dev instance.

@nanjj
Copy link
Contributor Author

nanjj commented Apr 3, 2024

Note line 149, the logic you're pointing out is fallback logic for the images that do not have a full /dev already available. The code as it's written actually shows that distrobuilder was meant to support images building with a full /dev instance.

OK, I changed my mind, now the fix is based on PR lxc/distrobuilder#830

The root partition may not be /dev/sda2, for example if set the boot disk as the
second disk, the root partition device name will be /dev/sdb2. It may be
/dev/vda2 if not using san disk. The fix figured out the root partition device
UUID and set the UUID as the root, the root partition device UUID keeps no
change.

Depends on lxc/distrobuilder#830

Signed-off-by: JUN JIE NAN <[email protected]>
@stgraber
Copy link
Member

stgraber commented Apr 7, 2024

Going to wait a day before merging that one so we can be sure all the builders get the new distrobuilder first.

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

Successfully merging this pull request may close these issues.

2 participants