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 #830

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.

@stgraber
Copy link
Member

stgraber commented Apr 2, 2024

Same comment as on lxc/lxc-ci#822, I'd prefer that some race-free method be used as otherwise multiple image builds on the same system may make this logic racy.

@nanjj
Copy link
Contributor Author

nanjj commented Apr 2, 2024

Same comment as on lxc/lxc-ci#822, I'd prefer that some race-free method be used as otherwise multiple image builds on the same system may make this logic racy.

I changed my mind, do as you prefered.

@nanjj nanjj force-pushed the main branch 5 times, most recently from 0518c5b to 8613f0e Compare April 2, 2024 06:58
nanjj added a commit to nanjj/lxc-ci that referenced this pull request 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

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

stgraber commented Apr 2, 2024

Great to see!

Could you split your PR into two commits? One adding the new env variables to distrobuilder and then the second updating the example .yaml to use it?

nanjj added a commit to nanjj/distrobuilder that referenced this pull request Apr 3, 2024
nanjj added a commit to nanjj/distrobuilder that referenced this pull request Apr 3, 2024
}

c.global.ctx = context.WithValue(c.global.ctx, shared.ContextKeyEnviron,
[]string{fmt.Sprintf("ROOTFS_DEVICE_UUID=%s", rootfsDevUUID)})
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably go for DISTROBUILDR_ROOTFS_UUID instead so that it's clear that this is a distrobuilder thing and we avoid potential conflicts with other env variables (as unlikely as that may be).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

Signed-off-by: JUN JIE NAN <[email protected]>
nanjj added a commit to nanjj/distrobuilder that referenced this pull request Apr 7, 2024
nanjj added a commit to nanjj/lxc-ci that referenced this pull request Apr 7, 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

Signed-off-by: JUN JIE NAN <[email protected]>
@stgraber stgraber merged commit 2420fea into lxc:main Apr 7, 2024
8 checks passed
tomponline added a commit to canonical/lxd-imagebuilder that referenced this pull request Aug 6, 2024
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