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

[Henning Schild <henning.schild@siemens.com>]

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.
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
>