install: add target_root_path for RootSetup
#1752
Conversation
HuijingHei
force-pushed
the
target-bootupd
branch
6 times, most recently
from
01ff672 to
0830023
Compare
|
Build the patch based on Check the If I reboot, seems it still booted into old rhel9.6, how can I check if the installation is successful?
|
|
This is a complex topic. A first thing here is that This issue also relates to #820 I think at the current time it's expected that
That is strange; what does |
|
One issue also related to this is that some of the install integration tests are still mostly orchestrated via the GHA I think we should likely switch them over to tmt. Although it gets subtle as reproducing the Edge starting state via tmt will require some work, but we can do that as a followup. |
Actually on ostree OS, it only removes
Run After reboot, boot into the old rhel9.6-edge, find directory |
You're right! And that's one of the issues here - this system is ostree but not bootupd, and that's one of the problem roots - I think in this case we'll just not change the bootloader state at all which is not expected. I think basically if we detect ostree but not bootupd, then we should proceed by wiping out the complete ESP as before alongside everything in Now...there is a related but distinct thing here which is that if we detect bootupd, we should also forcibly do a |
HuijingHei
force-pushed
the
target-bootupd
branch
3 times, most recently
from
25cf806 to
4c8db70
Compare
Sorry that this is out of my knowledge, what I see is from clean_boot_directories() before install_to_filesystem_impl(), any pointer for this? Thank you!
LGTM, have no idea where I should add this.
Should the new entries config and kernel file are created after running |
Yep that's the right place
In the logic where we have
Yes the boot loader entries are written by the install (and upgrade) process; those should stay the same as is today. What's special cased right now is ostree-vs-non-ostree in the |
The bootupctl update is a little different as it currently reads path form
With @jbtrystram 's help, do testing with change, cleanup ESP files and preserve like After reboot, boot into the old rhel9.6-edge, directory I think what you said for the comment makes sense, but I have no idea how to fix this, any pointer is much appreciated. Copy the comment here: |
HuijingHei
force-pushed
the
target-bootupd
branch
from
13f3c22 to
41935bb
Compare
|
Check more and find that the new The workaround is remove After booted, check that Edit:
This explains why the mount shows as |
|
Based on the above comment, one workaround is
Edit: |
Get pointer from bootc-dev#1752 (comment) On Ostree OS, should empty the complete ESP and everything in `/boot` but preserve `/boot/loader`. Signed-off-by: Huijing Hei <[email protected]> Co-worked by: Jean-Baptiste Trystram <[email protected]>
HuijingHei
force-pushed
the
target-bootupd
branch
2 times, most recently
from
5e2338b to
e5a6ba4
Compare
HuijingHei
changed the title
root_path for RootSetuptarget_root_path for RootSetup
|
Do some testing based on PR, Before reboot: After reboot and check |
HuijingHei
force-pushed
the
target-bootupd
branch
from
e5a6ba4 to
0e61edd
Compare
HuijingHei
force-pushed
the
target-bootupd
branch
from
b313845 to
914710a
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks for the hard work on this! There's so many subtleties here. I think this looks right, but I would like to spend some time looking more carefully after break too.
| # Build test container image for testing on coreos with SKIP_CONFIGS=1, | ||
| # without configs and no curl container image | ||
| build-testimage-coreos PATH: | ||
| @just build-from-package {{PATH}} |
There was a problem hiding this comment.
Ah it looks like this is going to conflict with #1864
But it's ok if this goes in first I can rebase
| # adjust: | ||
| # - when: VARIANT_ID != coreos | ||
| # enabled: false | ||
| # because: this needs to start an ostree OS firstly |
There was a problem hiding this comment.
Hum. But what does "ostree OS" mean here that's different from the other tests which start via bcvk libvirt run?
Doesn't this relate more to just the /boot partition setup?
If it's about having a separate /boot on an ostree based OS then note we should also be able to easily replicate this with Anaconda-based installs too (or bootc-image-builder).
There was a problem hiding this comment.
I think this is more related to esp partition and a separate /boot on existing ostree OS, like edge and FCOS/RHCOS.
Maybe I should use coreos here, as it is testing on FCOS. In this case, just use bcvk libvirt run to start fedora-coreos:testing-devel and then do testing.
I can try later if can use Anaconda, thank you for your pointer!
HuijingHei
force-pushed
the
target-bootupd
branch
2 times, most recently
from
fecf28b to
e2f17b4
Compare
|
Restarted some CI jobs here |
Head branch was pushed to by a user without write access
HuijingHei
force-pushed
the
target-bootupd
branch
from
e2f17b4 to
b48299c
Compare
|
Just trigger the tests. |
HuijingHei
force-pushed
the
target-bootupd
branch
from
d518357 to
b6bebb5
Compare
Get pointer from Colin's comment bootc-dev#1752 (comment) - Empty the complete ESP - On ostree OS, empty `/boot` but preserve `/boot/loader` - On none ostree OS, the loader is directory that needs to be removed. Signed-off-by: Huijing Hei <[email protected]>
HuijingHei
force-pushed
the
target-bootupd
branch
from
b6bebb5 to
b101f69
Compare
|
Rebase the main branch to trigger the test. I have no idea why test Edit: |
HuijingHei
force-pushed
the
target-bootupd
branch
2 times, most recently
from
a8d77ff to
e2f5e56
Compare
Get pointer from Colin's comment bootc-dev#1752 (comment) - Empty the complete ESP - On ostree OS, empty `/boot` but preserve `/boot/loader` - On none ostree OS, the loader is directory that needs to be removed. Signed-off-by: Huijing Hei <[email protected]>
When running `install to-filesystem` on ostree OS, should use `target_root_path` for bootupctl to install bootloader. Signed-off-by: Huijing Hei <[email protected]>
Fix what we did in bootc-dev@92d9d38 Signed-off-by: Huijing Hei <[email protected]>
On FCOS, esp is not mounted after booted, need to find esp and mount before cleaning, or `/boot/efi` will be removed. Signed-off-by: Huijing Hei <[email protected]>
To workaround bootc-dev/bcvk#174, will build `bootc-integration-coreos` container firstly and save it to `bootc.tar`, then load it to install. Signed-off-by: Huijing Hei <[email protected]>
HuijingHei
force-pushed
the
target-bootupd
branch
2 times, most recently
from
2ac1aa6 to
d00f8d1
Compare
|
Hold on, try to find out what is actually block test install bootc image on fedora-coreos. Edit: From logs, after reboot, ssh failed to connect the guest. But can not reproduce locally, add |
To workaround the issue that failed to ssh after reboot, can not reproduce locally. Signed-off-by: Huijing Hei <[email protected]>
HuijingHei
force-pushed
the
target-bootupd
branch
from
c5e0f26 to
bab8f98
Compare
| memory: 4096 | ||
| disk: 20 | ||
| ssh-option: | ||
| - "PubkeyAcceptedAlgorithms=+ssh-rsa" |
There was a problem hiding this comment.
Interesting; this seems most likely to relate to changes in Fedora 43 though I don't understand why it'd only appear with FCOS.
| /// Directory file descriptor for the above physical root. | ||
| pub(crate) physical_root: Dir, | ||
| /// Target root path /target. | ||
| pub(crate) target_root_path: Option<Utf8PathBuf>, |
There was a problem hiding this comment.
In general, I would like if we could go to having most things be file descriptor relative; it's unfortunate to need to add a new absolute path here.
But I understand that the bootupd install is using it today.
What I would find helpful is to understand a bit more strictly the meaing of "target" here. I think we're effectively using it to be "what the user provided as an install target" right?
And for ostree/bootc systems, that's always the mounted deployment root conventionally today when they say -v /:/target.
But that's just conventional - what would happen if they passed -v /sysroot:/target say?
The more I think about this the more I feel what we should do is detect when the passed root filesystem refers to either a mounted deployment root or the physical root, and automatically canonicalize internally to "physical root" and "deployment root" (which for non-ostree systems are the same thing), and then discard the user provided input path.
There was a problem hiding this comment.
What I would find helpful is to understand a bit more strictly the meaing of "target" here. I think we're effectively using it to be "what the user provided as an install target" right?
And for ostree/bootc systems, that's always the mounted deployment root conventionally today when they say -v /:/target.
But that's just conventional - what would happen if they passed -v /sysroot:/target say?
The more I think about this the more I feel what we should do is detect when the passed root filesystem refers to either a mounted deployment root or the physical root, and automatically canonicalize internally to "physical root" and "deployment root" (which for non-ostree systems are the same thing), and then discard the user provided input path.
This sounds better and more sane, will try this later.
There was a problem hiding this comment.
what would happen if they passed -v /sysroot:/target say?
We still need /boot/efi (or separate /boot) to update the bootloader, if we pass -v /sysroot:/target, then how could we get the "physical root" like / (to get /boot/efi)?
| // Get a file descriptor for the root path | ||
| let rootfs_fd = { | ||
| // It will be /target/sysroot on ostree OS, or will be /target | ||
| let rootfs_fd = if is_already_ostree { |
There was a problem hiding this comment.
While I know you're just changing existing code, I think it'd be cleaner to fold this conditional with the one above.
I had to read this whole logic a few times to notice that a key bit here is we're mutating the user input fsopts.root_path = possible_physical_root just above, and that's an important aspect.
| // Find the UUID of /boot because we need it for GRUB. | ||
| let boot_uuid = if boot_is_mount { | ||
| let boot_path = fsopts.root_path.join(BOOT); | ||
| let boot_path = target_root_path.join(BOOT); |
There was a problem hiding this comment.
This seems to be a key change here. I guess we needed this because in the ostree case we'd mutated fsopts.root_path to refer to the physical root, and when /boot is actually a separate mount point that was broken on ostree setups?
But this relates to what I mean above - we could instead just know to look at the deployment's /boot explicitly.
There was a problem hiding this comment.
This seems to be a key change here. I guess we needed this because in the ostree case we'd mutated
fsopts.root_pathto refer to the physical root, and when/bootis actually a separate mount point that was broken on ostree setups?
I am a little confused, do you mean "physical root" is /target, and "deployment root" is /target/sysroot?
From following log (copy from bug), we passed /target/sysroot, but we should pass /target.
> bootupctl backend install --write-uuid -vvvv --update-firmware --auto --device /dev/vda /target/sysroot
...
[DEBUG bootupd::efi] Mounted at "/target/sysroot/boot"
...
Could not find /boot/efi/EFI when installing "redhat/grub.cfg"
And I do not understand /sysroot/boot and /boot correctly, are they the same?
There was a problem hiding this comment.
For non-bootc/ostree setups, "physical root == root", so the user always passes -v /:/target - in other words /target inside there is always the filesystem root.
For bootc/ostree setups, they are not the same. What I am saying though is we should support being passed either thing (/sysroot or /) for /target and automatically do the right thing, and resolve the right roots.
And I do not understand /sysroot/boot and /boot correctly, are they the same?
Typically we only mount the separate boot partition in the target root, not the physical root.
There was a problem hiding this comment.
Understood, thank you for the detail info.
For bootc/ostree setups, they are not the same. What I am saying though is we should support being passed either thing (/sysroot or /) for /target and automatically do the right thing, and resolve the right roots.
Try to catch your meaning, for bootc/ostree setups, if passed -v /sysroot:/target, my concern is how to get ESP part, but looks can get /target part, then get its parent disk, then get separate boot part if there is (maybe need to mount it to /target/boot?), and ESP.
podman run --rm -it --privileged -v /dev:/dev -v /var/lib/containers:/var/lib/containers -v /sysroot:/target \
--pid=host --security-opt label=type:unconfined_t \
quay.io/fedora/fedora-bootc:rawhide bash
bash-5.3# cd /target
bash-5.3# findmnt .
TARGET SOURCE FSTYPE OPTIONS
/target /dev/vda4 xfs rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,prjquota
There was a problem hiding this comment.
Will try this later.
Get pointer from Colin's comment #1752 (comment) - Empty the complete ESP - On ostree OS, empty `/boot` but preserve `/boot/loader` - On none ostree OS, the loader is directory that needs to be removed. Signed-off-by: Huijing Hei <[email protected]>
2 participants
/boot&/boot/efitarget_root_pathtobootupctlto install bootloader./bootclean_boot_directories()