Re: [PATCH] image: Run copy_boot_files after rootfs postprocessing

[Jan Kiszka <jan.kiszka@siemens.com>]

On 29.06.20 14:55, Henning Schild wrote:
> On Mon, 29 Jun 2020 14:41:29 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 29.06.20 14:22, Harald Seiler wrote:
>>> Hi Jan,
>>>
>>> On Fri, 2020-06-26 at 11:12 +0200, Jan Kiszka wrote:
>>>> On 26.06.20 09:17, Claudius Heine wrote:
>>>>> Hi Harald,
>>>>>
>>>>> On 2020-06-25 19:24, Harald Seiler wrote:
>>>>>> Hello Henning,
>>>>>>
>>>>>> On Thu, 2020-06-25 at 19:02 +0200, Jan Kiszka wrote:
>>>>>>> On 25.06.20 18:48, [ext] Henning Schild wrote:
>>>>>>>> Hi Harald,
>>>>>>>>
>>>>>>>> can you elaborate on those cases? The postprocessing is hacky,
>>>>>>>> if the problem is coming from your layer you should probably
>>>>>>>> keep this patch in you layer.
>>>>>>>
>>>>>>> Basically do_generate_image_uuid from
>>>>>>> https://lore.kernel.org/cip-dev/20200625141015.31719-4-Quirin.Gylstorff@siemens.com/T/#u,
>>>>>>> just modeled as post-processing hook, rather than a task.
>>>>>>
>>>>>> For reference, this is the exact code:
>>>>>>
>>>>>>        ROOTFS_POSTPROCESS_COMMAND =+
>>>>>> "image_postprocess_generate_uuid"
>>>>>> image_postprocess_generate_uuid() { sudo sed -i
>>>>>> '/^IMAGE_UUID=.*/d' '${IMAGE_ROOTFS}/etc/os-release' echo
>>>>>> "IMAGE_UUID=\"${IMAGE_UUID}\"" | \ sudo tee -a
>>>>>> '${IMAGE_ROOTFS}/etc/os-release'
>>>>>>
>>>>>>            sudo -E chroot '${ROOTFSDIR}' \
>>>>>>                update-initramfs -u
>>>>>>        }
>>>>>>   
>>>>>>> Jan
>>>>>>>   
>>>>>>>> Maybe you can point out an issue in isar itself, or explain
>>>>>>>> how you got into this situation? We can then see if your
>>>>>>>> change is generic enough for upstream. You could also provide
>>>>>>>> the error-case from your layer as an upstream feature, if that
>>>>>>>> is generic enough.
>>>>>>
>>>>>> I think this patch addresses an issue in isar itself.  There is
>>>>>> no reason for copy_boot_files() to run before the postprocessing
>>>>>> does.  I've checked through the git history and the reason this
>>>>>> relationship was introduced was a bigger refactor of the task
>>>>>> dependency chain.  It does not seem to be intentionally this way
>>>>>> from what I can tell.
>>>>>>
>>>>>> The other way around makes more sense, in my opinion.  As stated
>>>>>> in the commit message, postprocessing might do an update to the
>>>>>> initramfs (as seen above) and this change needs to be reflected
>>>>>> in the deployed initramfs as well, instead of silently only
>>>>>> living in the version that is part of the rootfs.
>>>>>>
>>>>>> I also checked all existing postprocessing commands and did not
>>>>>> see any that assume to be run after the boot files have been
>>>>>> deployed.
>>>>>
>>>>> Its been a while when I implemented this, but I also thought of
>>>>> the scenario where someone would like to 'minimize' a image via
>>>>> the root fs postprocessing by deleting everything that is not
>>>>> needed, and that could possible include the kernel + initramfs,
>>>>> if those are stored somewhere else outside the root file system.
>>>>> So the idea was, IIRC, to move the kernel and initrd to the
>>>>> deploy dir, out of harms way, before postprocessing does its
>>>>> rootfs manipulation.
>>>>>
>>>>> So by ordering the copy_boot_files behind the root fs post
>>>>> processing, you might break other layers that rely on this
>>>>> ordering and have such 'minimization' procedures, that remove the
>>>>> kernel package and specific files.
>>>>>
>>>>> We don't have such 'minimization' stuff in upstream isar, since it
>>>>> pretty much breaks apt and dpkg, but if you do image based
>>>>> update, you might not care.
>>>>
>>>> I think the problem with this pattern is elsewhere: We should not
>>>> install stuff on the rootfs in the first place that shall not end
>>>> up in the rootfs. That this copy_boot_files thing depends on the
>>>> installation on the rootfs is actually a bug. It should use the
>>>> chroot for its work, like the imager does (for the bootloader
>>>> e.g.).
>>>
>>> For kernel and DTB I am totally on your side but I'm not sure how
>>> you would want to do this for the initramfs.  I think the initramfs
>>> should definitely be generated from the 'real' rootfs because
>>> otherwise you might get packages from chroot 'polluting' it in
>>> unexpected ways.  Also,
>>
>> That is no issue: buildchroot-target and target rootfs are in line.
>> If not, you have a different issue.
>>
>>> considering the IMAGE_UUID use-case in particular, how would you
>>> get the ID from the rootfs into the chroot for the hook to pick it
>>> up?  Not sure whether there is a clean solution for this ...
>>>    
>>
>> E.g. by generating the UUID at bitbake level and injecting it into
>> the images - rather than doing that inside one of the images.
> 
> I anyway do not get why that UUID is needed in the first place. Which
> partition to take as root partition can be decided with a partition
> label. You just have to be careful with swupdate, it destroy those
> labels when flashing whole partitions.

We need to identify the *filesystem* instance, not the partition, while 
booting. A new version of the filesystem may be in partition A or 
partition B, depending on how often the system was already updated.

I'm literally just now painting this slide for my talk tomorrow...

Jan

> Even if you decide to use an UUID, the bootloader should know that and
> pass that arg.
> 
>   ... root=$(get_part_uuid(initrd-file)) ...
> 
> Might require improving bootloaders ;) or writing bootloader configs
> instead of initrds in postrprocess ...
> 
> You can also put your static desired UUID in your wks-file and a
> postinst of a customization-package. But that might be only for
> recipes/layers that are not meant for sharing
> 
> Henning
> 
>> Jan
>>
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux