Hi Coiby,
On Tue, 2 Aug 2022 15:54:11 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
Hi Philipp,
On Fri, Jul 29, 2022 at 05:33:57PM +0200, Philipp Rudo wrote:
>Hi Coiby,
>
>this series doesn't seem to be merged yet. So let me add a few
>comments/questions ;-)
Thanks for reviewing the patch!
my pleasure.
>
>On Fri, 13 May 2022 14:08:08 +0800
>Coiby Xu <coxu(a)redhat.com> wrote:
>
>> Fedora 33 and 34 Cloud Base Images have only partition with the
> ^
> one (?)
Nice catch!
>
>> following directory structure,
>> .
>> ├── bin -> usr/bin
>> ├── boot
>> ├── dev
>> ├── etc
>> ├── home
>> ├── root
>>
>> By comparison, Fedora 35, 36 and 37 Cloud Base Images have multiple
>> partitions. The root partition which is the last partition has the
>> following directory,
>> .
>> ├── home
>> └── root
>> ├── bin -> usr/bin
>> ├── boot
>> ├── dev
>> ├── etc
>> ├── home
>> ├── root
>
>Just for my understanding, in this schema /root/home is the home
>directory for root and /home for everybody else. Is that correct?
/root/root is the home directory for root and /root/home is for
everybody else. And /home seems to be unused.
Thanks for the explanation! It's weird that there is this unused
/home... but well it's not our problem ;-)
>
>> and the 2nd partition is the boot partition.
>>
>> This patch address the above changes by mounting {LAST_PARTITION}/root as
>> to TEMP_ROOT and mount FIRST_PARTITION to TEMP_ROOT/boot. So the test
>> image can be built successfully.
>
>That's a little bit confusing to me. When the boot partition is the 2nd
>partition why is the variable called FIRST_PARTITION?
Sorry for the confusion! FIRST_PARTITION should be SECOND_PARTITION.
>
>>
>> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
>> ---
>> tests/scripts/image-init-lib.sh | 29 ++++++++++++++++++++++++++---
>> 1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/scripts/image-init-lib.sh b/tests/scripts/image-init-lib.sh
>> index 0a1524b..5995004 100644
>> --- a/tests/scripts/image-init-lib.sh
>> +++ b/tests/scripts/image-init-lib.sh
>> @@ -23,7 +23,7 @@ is_mounted()
>> clean_up()
>> {
>> for _mnt in ${MNTS[@]}; do
>> - is_mounted $_mnt && $SUDO umount -f $_mnt
>> + is_mounted $_mnt && $SUDO umount -f -R $_mnt
>> done
>>
>> for _dev in ${DEVS[@]}; do
>> @@ -81,6 +81,21 @@ get_mountable_dev() {
>> fi
>> }
>>
>> +# get the separate boot partition
>> +# return the 2nd partition as boot partition
>> +get_mount_boot() {
>> + local dev=$1 parts
>> +
>> + $SUDO partprobe $dev && sync
>> + parts="$(ls -1 ${dev}p*)"
>> + if [ -n "$parts" ]; then
>> + if [ $(echo "$parts" | wc -l) -gt 1 ]; then
>> + echo "$parts" | head -2 | tail -1
>> + fi
>> + fi
>
>Hmmm... difficult...
>
>Can we really rely on ls providing the right order here? If so why not
>simply do 'ls -1 ${dev}p2' ?
Yes, I should have simply used "ls -1 ${dev}". However your suggestion
of using lsblk inspires me to find a more reliable method which also
works for RHEL cloud base image,
if [[ $(lsblk -f ${dev}p2 -n -o LABEL 2> /dev/null) == boot ]]
echo ${dev}p2
fi
Originally I refrained from using LABEL as the way I understand it it
is more a scratchpad for users so they can remember which partition
contains what. So especially on systems with a custom partition scheme
there is no guarantee that the LABEL on the boot partition contains
'boot'. That's why I thought MOUNTPOINT or PARTLABEL are better choices
(although when thinking about it PARTLABEL probably has the same
problem).
Anyway, on second thought this script is only run in a very specific
environment and isn't shipped to customers. So we don't have to be
generic but can take some shortcuts. So using LABEL should be fine.
But I'm not sure if the suggestion you made above makes sense.
Originally the function was used to return the boot partition of a
device (which happens to be the second partition). Now you are verifying
that the second partition is the boot partition.
Anyway when you are using the LABEL you can also use findfs to get the
partition. I don't know what findfs does when there are multiple
partitions with the same LABEL so I think it is better to pipe the
output to grep. I.e. this should work
findfs LABEL=boot 2> /dev/null | grep $dev
Thanks
Philipp
>
>Would it make sense to switch to something more powerful like lsblk?
>For example we could use
>
> lsblk -p -r -n -o NAME,MOUNTPOINT ${dev} | grep $root/boot
Btw, the above command doesn't work for the cloud image case,
$ lsblk -p -r -n -o NAME,MOUNTPOINT /dev/nbd2
/dev/nbd2
/dev/nbd2p1
/dev/nbd2p2
/dev/nbd2p3
/dev/nbd2p4
/dev/nbd2p5
>
>or
> lsblk -p -r -n -o NAME,PARTLABEL ${dev} | grep efi
So is the above command.
>
>(what for non-efi systems, e.g. s390?).
Currently, Fedora doesn't have s390 cloud image so we simply skip this
concern now.
>
>Finally, you can drop the two if-blocks by simply piping to sed. For
>example your original version could be rewritten to
>
> ls -1 ${dev}p* | sed -n 2p
>
>which prints the second line or "" if it doesn't exist.
>
>Thanks
>Philipp
>
>> +}
>> +
>> +
>> prepare_loop() {
>> [ -n "$(lsmod | grep "^loop")" ] && return
>>
>> @@ -133,7 +148,7 @@ image_lock()
>> # Mount a device, will umount it automatially when shell exits
>> mount_image() {
>> local image=$1 fmt
>> - local dev mnt mnt_dev
>> + local dev mnt mnt_dev boot root
>>
>> # Lock the image just in case user run this script in parrel
>> image_lock $image
>> @@ -166,12 +181,20 @@ mount_image() {
>>
>> $SUDO mount $mnt_dev $mnt
>> [ $? -ne 0 ] && perror_exit "failed to mount device
'$mnt_dev'"
>> + boot=$(get_mount_boot "$dev")
>> + if [[ -n "$boot" ]]; then
>> + root=$(get_image_mount_root $image)
>> + $SUDO mount $boot $root/boot
>> + [ $? -ne 0 ] && perror_exit "failed to mount the bootable
partition for device '$mnt_dev'"
>> + fi
>> }
>>
>> get_image_mount_root() {
>> local image=$1
>> local root=${MNTS[$image]}
>>
>> + # Starting from Fedora 36, the root node is /root/root of the last partition
>> + [ -d "$root/root/root" ] && root=$root/root
>> echo $root
>>
>> if [ -z "$root" ]; then
>> @@ -213,7 +236,7 @@ run_in_image() {
>>
>> inst_in_image() {
>> local image=$1 src=$2 dst=$3
>> - local root=${MNTS[$image]}
>> + local root=$(get_image_mount_root $1)
>>
>> $SUDO cp $src $root/$dst
>> }
>