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

[Harald Seiler <hws@denx.de>]

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

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