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

[Harald Seiler <hws@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 4377 bytes --]

Hello Claudius,

On Fri, 2020-06-26 at 09:17 +0200, 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 can see the use-case you are describing though I think mine (or rather
the one for IMAGE_UUID) is just as valid.  Both are things that need to
happen after the rootfs has been created but before the imager is invoked.
The difference is that one needs to happen before copy_boot_files() and
one needs to happen after.  I see two solutions:

- Either we split postprocessing into 'early' (before boot files are
  deployed) and 'late' (after boot files have been deployed).  This has
  the disadvantage of further convoluting the task chain and isn't really
  pretty in my opinion.
- The cleanup 'postprocessing' is treated special and a different solution
  is created for it; one that runs after deploying the boot files.  E.g.
  a ROOTFS_CLEANUP_FILES variable which is filled with files and
  directories to be removed before invoking the imager.

Maybe you have another idea for this?

-- 
Harald

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-62  Fax: +49-8142-66989-80   Email: hws@denx.de

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 850 bytes --]