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

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

Am Fri, 26 Jun 2020 10:44:25 +0200
schrieb Jan Kiszka <jan.kiszka@siemens.com>:

> On 26.06.20 10:26, Henning Schild wrote:
> > Am Fri, 26 Jun 2020 10:13:36 +0200
> > schrieb Harald Seiler <hws@denx.de>:
> >   
> >> Hello Henning,
> >>
> >> On Thu, 2020-06-25 at 21:27 +0200, Henning Schild wrote:  
> >>> Am Thu, 25 Jun 2020 21:23:54 +0200
> >>> schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> >>>      
> >>>> Am Thu, 25 Jun 2020 20:43:07 +0200
> >>>> schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> >>>>      
> >>>>> Am Thu, 25 Jun 2020 19:24:30 +0200
> >>>>> schrieb Harald Seiler <hws@denx.de>:
> >>>>>        
> >>>>>> 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
> >>>>>>      }  
> >>>>>
> >>>>> If /etc/os-release goes into the initrd we have the issue with
> >>>>> image_postprocess_mark in isar, a valid reason for a merge.  
> >>
> >> It does not.  The reason /etc/os-release and the initramfs become
> >> related here is because the IMAGE_UUID from the rootfs needs to be
> >> added into the initramfs so it can later identify the rootfs it
> >> belongs to (when multiple copies exist).  This is done by a
> >> separate initramfs hook installed as a package.
> >>  
> >>>> But that is not the case. You re-create the initrd in your layer
> >>>> so it might just be your problem! How about we discuss the
> >>>> IMAGE_UUID upstream together with the reordering?  
> >>>
> >>> Or do we need the "update-initramfs -u" for
> >>> image_postprocess_mark? 
> >>
> >> No, as described above, this is only necessary for IMAGE_UUID
> >> because there, a hook copies the IMAGE_UUID into the initramfs.
> >> The rest of /etc/os-release is irrelevant for initrd (AFAIK).
> >>  
> >>> If so that should be done as a patch in this queue, you will get
> >>> your reordering for IMAGE_UUID in your layer if you decide to keep
> >>> that downstream.  
> >>
> >> I don't know what the plan is; whether IMAGE_UUID should be
> >> upstreamed at some point.  Jan, you're probably the person to ask
> >> here?  From my side, I think this would be a useful feature to
> >> have. 
> >>> Henning
> >>>      
> >>>> Henning
> >>>>      
> >>>>>>> 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.  
> >>>>>
> >>>>> Ok, sounds fair.  
> >>
> >> In the other thread, Claudius has shed some light on the design
> >> decisions that let to this.  But let's keep that discussion over
> >> there.
> >>  
> >>>>>> 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.  
> >>>>>
> >>>>> I just worried about people abusing postprocess for changes that
> >>>>> should be packages instead. Thanks for going into detail!  
> >>
> >> If you have a better approach than what I came up with, please do
> >> tell! I decided on postprocessing for a few reasons:
> >>
> >> - The IMAGE_UUID is an 'image feature' as in, the image recipe
> >> should decide what its ID is (an image inheriting `image_uuid` can
> >> set the ID string).  So pushing that into a package is difficult.
> >> - As this only adds a value to /etc/os-release instead of deploying
> >> the whole file, a package approach would need to work via postinst
> >> which seems like a hack to me.
> >> - The previous approach from Quirin was to add a separate task for
> >> adding the UUID.  This task can then be placed in the dependency
> >> chain where I have now moved the postprocessing but this feels like
> >> duplicated work when a postprocessing task exists as a feature
> >> already.  
> > 
> > I did not read this since nobody proposed the UUID-feature. Upstream
> > should not care about all of this. There are two different potential
> > changes in some layers that favor one or the other order.
> > But there is no reason for Isar to change the order. A patch
> > without a use-case needs to be rejected.  
> 
> That is obviously wrong. Upstream should care about anything
> reasonable that is brought up from active downstream users.

Sure. But ideally with a use-case or a test-case that shows the issue
that is addressed. I appreciate the upstream work and am trying to
squeeze out more ;).
Not like anything gets merged here anyway ...

Just ran a grep on a popular layer of not so active users, would not be
surprised if they had an issue with this reordering. Added people to Cc
and will not look into it.

Henning

> Jan