Re: [PATCH] expand-on-first-boot: wait for udev to create symlink

["Schaffner, Tobias" <tobias.schaffner@siemens.com>]

On 08.12.22 18:39, Schild, Henning (T CED SES-DE) wrote:
> Am Thu, 8 Dec 2022 17:55:42 +0100
> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> 
>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
>>
>> systemd-growfs depends on a symlink to the partition of the filesystem
>> that should be resized. This symlink is created by udev in
>> /dev/block/.
>>
>> If this symlink is not yet created for example because systemd-udev is
>> not up yet systemd-growfs will fail.
>>
>> We could use Require and After to depend on the systemd-udev service
>> but this could again create a race condition if udev is up but not
>> fast enough after the partx -u.
>>
>> Resolve the symlinks in /dev/block/ periodically until the symlink
>> appears before running systemd-growfs.
>> ---
>>   .../files/expand-last-partition.sh               | 16
>> +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git
>> a/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
>> b/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
>> index 57055cc..7ebb3e5 100755 ---
>> a/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
>> +++
>> b/meta/recipes-support/expand-on-first-boot/files/expand-last-partition.sh
>> @@ -38,7 +38,8 @@ if [ "$DISK_SIZE" -lt "$MINIMAL_SIZE" ]; then exit
>> 0 fi -LAST_PART="$(sfdisk -d "${BOOT_DEV}" 2>/dev/null | tail -1 |
>> cut -d ' ' -f 1)" +LAST_PART_NAME="$(lsblk -l -o NAME "${BOOT_DEV}" |
>> tail -1)" +LAST_PART="/dev/${LAST_PART_NAME}"
>>   
>>   # Transform the partition table as follows:
>>   #
>> @@ -79,6 +80,19 @@ if [ ! -d "${MOUNT_POINT}" ]; then
>>   	exit 1
>>   fi
>>   
>> +START_TIME="$(date +%s)"
>> +
>> +# Wait for udev to create the symlink to the partition in
> 
> Newer systemd versions will not need that anymore. Maybe write down
> which version that would be and starting from which debian distro we
> can drop that. Who knows when that udev stuff changes and the symlinks
> will never appear ... We should only wait for them in systemd versions
> that use them.
> 
> If we already have a version out there ... like bookworm? We can
> already implement it without the wait.

Yes you are right. I will add a check and skip the loop from V252 on.

>> /dev/block/ as +# systemd-growfs depends on it
>> +while ! readlink /dev/block/* | grep -q "${LAST_PART_NAME}"; do
> 
> I would make that much stricter to not be tricked by partial matches
> 
> sda4 vs sda42
> 
> readlink -f /dev/block/* | grep -e -q "^${LAST_PART}$"

I get a relative path from readlink. So I will have to skip the ^ but
to use match the end of the line is a good idea. -e is not needed for
start and end line matches. ;)

>> +    sleep 0.1
> 
> I wonder if there is anything we could do here. Maybe "udevadm trigger"
> and depend on udev after all. Because the 5 is a nasty guess ...


Nothing that I am aware of. The main reason for the issues that we have
seen was that the udev service was not up yet. We could maybe check if it
is up first and after that only allow a small period. But then again how
long do we wait for the service to be up and running.

>> +    CURRENT_TIME="$(date +%s)"
>> +    if [ $(( CURRENT_TIME - START_TIME )) -gt "5" ]; then
> 
> we can simply i++ > 50, no need to call date
> 
> so maybe
> 
> err=1
> for i in $(seq 0 50); do
> 	if readlink -f /dev/block/* | grep -e -q "^${LAST_PART}$"; then
> 		err=0
> 		break
> 	fi
> 	sleep 0.1
> done
> 
> if $err ...
>> +        echo "Could not find symlink to last part in /dev/block/."
>> +        exit 1
>> +    fi
>> +done
>> +
>>   mount "${LAST_PART}" "${MOUNT_POINT}"
>>   /lib/systemd/systemd-growfs "${MOUNT_POINT}"
> 
> we could also loop over calling this until it goes "0" or we reach a
> retry counter, that way we magically handle new systemd versions that
> do not need symlinks and do not implement anything with symlinks.


That's a cool idea. As systemd-growfs returns 1 in all error cases we will
have to parse stderr to determine if we ran in the error case that we want
to handle. I will give it a try.


> Henning
> 
>>   umount "${MOUNT_POINT}"
>