[RFC] image: Reorder do_copy_boot_files task

There might be cases where in there are some initramfs changes in postprocess.
For example, via the distro config script.

In such a scenario we would have an outdated initramfs file in deploy directory.
Certain downstream Wic plugins directly consume the image from deploy directory.
It then uses the outdated initramfs for creating the wic image.

Copy boot files after rootfs postprocess but before finalizing the
rootfs.

Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
---
 RECIPE-API-CHANGELOG.md    | 7 ++++++-
 meta/classes/image.bbclass | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

Hi All,

On Fri, Feb 18, 2022 at 3:24 PM Vijai Kumar K
<Vijaikumar_Kanagarajan@mentor.com> wrote:
>
> There might be cases where in there are some initramfs changes in postprocess.
> For example, via the distro config script.
>
> In such a scenario we would have an outdated initramfs file in deploy directory.
> Certain downstream Wic plugins directly consume the image from deploy directory.
> It then uses the outdated initramfs for creating the wic image.
>
> Copy boot files after rootfs postprocess but before finalizing the
> rootfs.

Any comments on this?

Thanks,
Vijai Kumar K

>
> Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> ---
>  RECIPE-API-CHANGELOG.md    | 7 ++++++-
>  meta/classes/image.bbclass | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> index cad15a8..ef53b1a 100644
> --- a/RECIPE-API-CHANGELOG.md
> +++ b/RECIPE-API-CHANGELOG.md
> @@ -342,4 +342,9 @@ The bitbake variable defines the respective environment variable which is availa
>  When cross compiling, `cross` is added to the `DEB_BUILD_PROFILES` environment variable.
>  Please note, that manually exported versions of the variables are overwritten.
>
> -For a list of well-known Debian build profiles and common practices, we refer to Debian's BuildProfileSpec.
> \ No newline at end of file
> +For a list of well-known Debian build profiles and common practices, we refer to Debian's BuildProfileSpec.
> +
> +### Move do_copy_boot_files task after do_rootfs_postprocess
> +
> +The boot-files(kernel, initrd, dtbs) are now shipped to tmp/deploy/images after
> +do_rootfs_postprocess task and before do_rootfs_finalize task.
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 6d77243..d70a93b 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -170,7 +170,7 @@ do_copy_boot_files() {
>          cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
>      done
>  }
> -addtask copy_boot_files before do_rootfs_postprocess after do_rootfs_install
> +addtask copy_boot_files before do_rootfs_finalize after do_rootfs_postprocess
>
>  python do_image_tools() {
>      """Virtual task"""
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "isar-users" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/isar-users/20220218095428.1767282-1-Vijaikumar_Kanagarajan%40mentor.com.

Am Fri, 18 Feb 2022 15:24:28 +0530
schrieb Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>:

> There might be cases where in there are some initramfs changes in
> postprocess. For example, via the distro config script.

Postprocessing is a hack reserved for very few special cases, most of
which should read and not write.

That smells like you are abusing postprocess, and whatever you do there
might also break kernel updates with "apt-get".

Please give us the example, i bet it can be moved out of postprocess
into a much more sustainable place (like a package).

With no good example, NACK from me.

> In such a scenario we would have an outdated initramfs file in deploy
> directory. Certain downstream Wic plugins directly consume the image
> from deploy directory. It then uses the outdated initramfs for
> creating the wic image.

Maybe those downstream wic plugins deserve fixing as well, take the
initrd and kernel out of root/boot.

For image type wic there should in fact be no reason to copy the boot
files to deploy. That is something for ext4 and others going to "qemu
--kernel ..."
While those files are there for any image i would not suggest using
them. We have lava setups where we convert a wic into
kernel+initrd+nfsrootfs+cmdline, all taken only out of that wic. No
side channels. I guess one could also look into ".wic.img.p1" but the
boot files have no place as output for wic images, let alone as input
for wic.

regards,
Henning

> Copy boot files after rootfs postprocess but before finalizing the
> rootfs.
> 
> Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> ---
>  RECIPE-API-CHANGELOG.md    | 7 ++++++-
>  meta/classes/image.bbclass | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> index cad15a8..ef53b1a 100644
> --- a/RECIPE-API-CHANGELOG.md
> +++ b/RECIPE-API-CHANGELOG.md
> @@ -342,4 +342,9 @@ The bitbake variable defines the respective
> environment variable which is availa When cross compiling, `cross` is
> added to the `DEB_BUILD_PROFILES` environment variable. Please note,
> that manually exported versions of the variables are overwritten. 
> -For a list of well-known Debian build profiles and common practices,
> we refer to Debian's BuildProfileSpec. \ No newline at end of file
> +For a list of well-known Debian build profiles and common practices,
> we refer to Debian's BuildProfileSpec. +
> +### Move do_copy_boot_files task after do_rootfs_postprocess
> +
> +The boot-files(kernel, initrd, dtbs) are now shipped to
> tmp/deploy/images after +do_rootfs_postprocess task and before
> do_rootfs_finalize task. diff --git a/meta/classes/image.bbclass
> b/meta/classes/image.bbclass index 6d77243..d70a93b 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -170,7 +170,7 @@ do_copy_boot_files() {
>          cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
>      done
>  }
> -addtask copy_boot_files before do_rootfs_postprocess after
> do_rootfs_install +addtask copy_boot_files before do_rootfs_finalize
> after do_rootfs_postprocess 
>  python do_image_tools() {
>      """Virtual task"""

Hi Henning,

On Fri, Feb 25, 2022 at 2:01 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Fri, 18 Feb 2022 15:24:28 +0530
> schrieb Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>:
>
> > There might be cases where in there are some initramfs changes in
> > postprocess. For example, via the distro config script.
>
> Postprocessing is a hack reserved for very few special cases, most of
> which should read and not write.
>
> That smells like you are abusing postprocess, and whatever you do there
> might also break kernel updates with "apt-get".
>
> Please give us the example, i bet it can be moved out of postprocess
> into a much more sustainable place (like a package).
>
> With no good example, NACK from me.

Thank you for the quick review comments. Let me try to provide more information.

Postprocess abuse has always been a concern, we have ways to inject
stuff and we also have undocumented rules on
what should or should not be done as part of certain tasks.
We could capture it. Happy to do that.

The end user accidentally steps over this and something breaks.

The original issue was that plymouth theme was changed at a later stage.
The issue was fixed by having it not done as part of postprocess.

We have an option to pass a custom distro configuration script. It is
run as part of postprocess[1].
At best the user could run plymouth-set-default-theme[2] as part of
this script that changes the initramfs during postprocess.

We could not completely rule out modifications in postprocess. And the
rootfs is not complete till rootfs_finalize where we remove the
ability to chroot.
Changes could even then be injected, but that does not happen that
often and the user needs to create a task or append one to do that.
Not as simple as a variable append.

We should take the images from a finalized rootfs, and then deploy it,
so that the images in both deploy and rootfs are the same no matter
whether we use it or not.

Despite the fact that what should or should not be done in rootfs
postprocess, It only makes sense to copy the files to deploy directory
after finalizing the rootfs.

[1]https://github.com/ilbers/isar/blob/ffdd1b0ce026d21c8b62c06c926d205aad3078b6/meta/classes/image-postproc-extension.bbclass#L36
[2]https://manpages.debian.org/buster/plymouth/plymouth-set-default-theme.1

>
> > In such a scenario we would have an outdated initramfs file in deploy
> > directory. Certain downstream Wic plugins directly consume the image
> > from deploy directory. It then uses the outdated initramfs for
> > creating the wic image.
>
> Maybe those downstream wic plugins deserve fixing as well, take the
> initrd and kernel out of root/boot.
>
> For image type wic there should in fact be no reason to copy the boot
> files to deploy. That is something for ext4 and others going to "qemu
> --kernel ..."

Maybe, but ideally deploy should have the final kernel and initrd
shipped, no matter whether its a wic image or ext4img.
We could have a scenario where we build multiple image types, both
ends up with different initramfs because we used the file from
different places(IMAGE_ROOTFS/DEPLOY_DIR)

I hope this gives some justification to the patch?

Thanks,
Vijai Kumar K

> While those files are there for any image i would not suggest using
> them. We have lava setups where we convert a wic into
> kernel+initrd+nfsrootfs+cmdline, all taken only out of that wic. No
> side channels. I guess one could also look into ".wic.img.p1" but the
> boot files have no place as output for wic images, let alone as input
> for wic.
>
> regards,
> Henning
>
> > Copy boot files after rootfs postprocess but before finalizing the
> > rootfs.
> >
> > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > ---
> >  RECIPE-API-CHANGELOG.md    | 7 ++++++-
> >  meta/classes/image.bbclass | 2 +-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > index cad15a8..ef53b1a 100644
> > --- a/RECIPE-API-CHANGELOG.md
> > +++ b/RECIPE-API-CHANGELOG.md
> > @@ -342,4 +342,9 @@ The bitbake variable defines the respective
> > environment variable which is availa When cross compiling, `cross` is
> > added to the `DEB_BUILD_PROFILES` environment variable. Please note,
> > that manually exported versions of the variables are overwritten.
> > -For a list of well-known Debian build profiles and common practices,
> > we refer to Debian's BuildProfileSpec. \ No newline at end of file
> > +For a list of well-known Debian build profiles and common practices,
> > we refer to Debian's BuildProfileSpec. +
> > +### Move do_copy_boot_files task after do_rootfs_postprocess
> > +
> > +The boot-files(kernel, initrd, dtbs) are now shipped to
> > tmp/deploy/images after +do_rootfs_postprocess task and before
> > do_rootfs_finalize task. diff --git a/meta/classes/image.bbclass
> > b/meta/classes/image.bbclass index 6d77243..d70a93b 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -170,7 +170,7 @@ do_copy_boot_files() {
> >          cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
> >      done
> >  }
> > -addtask copy_boot_files before do_rootfs_postprocess after
> > do_rootfs_install +addtask copy_boot_files before do_rootfs_finalize
> > after do_rootfs_postprocess
> >  python do_image_tools() {
> >      """Virtual task"""
>
> --
> You received this message because you are subscribed to the Google Groups "isar-users" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/isar-users/20220225093137.54487e31%40md1za8fc.ad001.siemens.net.

Am Fri, 25 Feb 2022 16:09:02 +0530
schrieb vijai kumar <vijaikumar.kanagarajan@gmail.com>:

> Hi Henning,
> 
> On Fri, Feb 25, 2022 at 2:01 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Fri, 18 Feb 2022 15:24:28 +0530
> > schrieb Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>:
> >  
> > > There might be cases where in there are some initramfs changes in
> > > postprocess. For example, via the distro config script.  
> >
> > Postprocessing is a hack reserved for very few special cases, most
> > of which should read and not write.
> >
> > That smells like you are abusing postprocess, and whatever you do
> > there might also break kernel updates with "apt-get".
> >
> > Please give us the example, i bet it can be moved out of postprocess
> > into a much more sustainable place (like a package).
> >
> > With no good example, NACK from me.  
> 
> Thank you for the quick review comments. Let me try to provide more
> information.
> 
> Postprocess abuse has always been a concern, we have ways to inject
> stuff and we also have undocumented rules on
> what should or should not be done as part of certain tasks.
> We could capture it. Happy to do that.
> 
> The end user accidentally steps over this and something breaks.
> 
> The original issue was that plymouth theme was changed at a later
> stage. The issue was fixed by having it not done as part of
> postprocess.

Sound like a feature that helped you fix a problem in a better way, and
not postprocess.

> We have an option to pass a custom distro configuration script. It is
> run as part of postprocess[1].
> At best the user could run plymouth-set-default-theme[2] as part of
> this script that changes the initramfs during postprocess.

These config scripts deserve to go away entirely, and postproc deserves
to run on ro-mounted rootfs per default. IMHO ...

This would make very clear to anyone that messing around there is not
an interface to extend or hook into.

> We could not completely rule out modifications in postprocess. And the
> rootfs is not complete till rootfs_finalize where we remove the
> ability to chroot.
> Changes could even then be injected, but that does not happen that
> often and the user needs to create a task or append one to do that.
> Not as simple as a variable append.

Isar can rule things out by more strict enforcement and reducing abuse
potential. Any downstream can only rule it out with code reviews and
dedication to avoiding hacks.

> We should take the images from a finalized rootfs, and then deploy it,
> so that the images in both deploy and rootfs are the same no matter
> whether we use it or not.

if you mean the kernel and initrd image, yes, or not store a copy at
all when wic is the only imagetype
Which means your downstream wic plugin needs fixing for sure.

> Despite the fact that what should or should not be done in rootfs
> postprocess, It only makes sense to copy the files to deploy directory
> after finalizing the rootfs.
> 
> [1]https://github.com/ilbers/isar/blob/ffdd1b0ce026d21c8b62c06c926d205aad3078b6/meta/classes/image-postproc-extension.bbclass#L36
> [2]https://manpages.debian.org/buster/plymouth/plymouth-set-default-theme.1

I have no clue how that 2 works internally, but let me guess that it
writes a config file or snippet somewhere in etc. So all you need is a
packages which depends on plymouth and which either places some files
or just calls the tool in postinst while not containing anything really.
Or you drop config bits before installing plymouth and its postinst
will take care.

> > > In such a scenario we would have an outdated initramfs file in
> > > deploy directory. Certain downstream Wic plugins directly consume
> > > the image from deploy directory. It then uses the outdated
> > > initramfs for creating the wic image.  
> >
> > Maybe those downstream wic plugins deserve fixing as well, take the
> > initrd and kernel out of root/boot.
> >
> > For image type wic there should in fact be no reason to copy the
> > boot files to deploy. That is something for ext4 and others going
> > to "qemu --kernel ..."  
> 
> Maybe, but ideally deploy should have the final kernel and initrd
> shipped, no matter whether its a wic image or ext4img.
> We could have a scenario where we build multiple image types, both
> ends up with different initramfs because we used the file from
> different places(IMAGE_ROOTFS/DEPLOY_DIR)

If you do not postprocess mess around, they will always be the same.

And i would even assume that they could very well differ depedning on
image type (at least initrd) but also kernel.

> I hope this gives some justification to the patch?

Afraid not, it caters for more hacks when applied. And it not being
applied helped you find a proper solution for your plymouth problem.

Or maybe i still do not get it.

I am tempted to write a patch that will warn if any file has a more
recent mtime/ctime than "end of install". I guess the only one should
be "/etc/os-release" in plain Isar. Maybe that would be a good hack
indicator instead. If you agree maybe you want to write such a patch.

Henning

> Thanks,
> Vijai Kumar K
> 
> > While those files are there for any image i would not suggest using
> > them. We have lava setups where we convert a wic into
> > kernel+initrd+nfsrootfs+cmdline, all taken only out of that wic. No
> > side channels. I guess one could also look into ".wic.img.p1" but
> > the boot files have no place as output for wic images, let alone as
> > input for wic.
> >
> > regards,
> > Henning
> >  
> > > Copy boot files after rootfs postprocess but before finalizing the
> > > rootfs.
> > >
> > > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > > ---
> > >  RECIPE-API-CHANGELOG.md    | 7 ++++++-
> > >  meta/classes/image.bbclass | 2 +-
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > > index cad15a8..ef53b1a 100644
> > > --- a/RECIPE-API-CHANGELOG.md
> > > +++ b/RECIPE-API-CHANGELOG.md
> > > @@ -342,4 +342,9 @@ The bitbake variable defines the respective
> > > environment variable which is availa When cross compiling,
> > > `cross` is added to the `DEB_BUILD_PROFILES` environment
> > > variable. Please note, that manually exported versions of the
> > > variables are overwritten. -For a list of well-known Debian build
> > > profiles and common practices, we refer to Debian's
> > > BuildProfileSpec. \ No newline at end of file +For a list of
> > > well-known Debian build profiles and common practices, we refer
> > > to Debian's BuildProfileSpec. + +### Move do_copy_boot_files task
> > > after do_rootfs_postprocess +
> > > +The boot-files(kernel, initrd, dtbs) are now shipped to
> > > tmp/deploy/images after +do_rootfs_postprocess task and before
> > > do_rootfs_finalize task. diff --git a/meta/classes/image.bbclass
> > > b/meta/classes/image.bbclass index 6d77243..d70a93b 100644
> > > --- a/meta/classes/image.bbclass
> > > +++ b/meta/classes/image.bbclass
> > > @@ -170,7 +170,7 @@ do_copy_boot_files() {
> > >          cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
> > >      done
> > >  }
> > > -addtask copy_boot_files before do_rootfs_postprocess after
> > > do_rootfs_install +addtask copy_boot_files before
> > > do_rootfs_finalize after do_rootfs_postprocess
> > >  python do_image_tools() {
> > >      """Virtual task"""  
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "isar-users" group. To unsubscribe from this group and stop
> > receiving emails from it, send an email to
> > isar-users+unsubscribe@googlegroups.com. To view this discussion on
> > the web visit
> > https://groups.google.com/d/msgid/isar-users/20220225093137.54487e31%40md1za8fc.ad001.siemens.net.
> >