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

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

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.
And i think that Claudius "made up cleaning step" might actually be
somewhere out there. So we are talking about an interface change
without a use-case ... strong reject!

Henning