[PATCH] image: Run copy_boot_files after rootfs postprocessing

[PATCH] image: Run copy_boot_files after rootfs postprocessing

[Harald Seiler]

In some cases, postprocessing might trigger an update of the initramfs
which would not appear in the deployed version.  Fix this by running
copy_boot_files only after all postprocessing completed.

Signed-off-by: Harald Seiler <hws@denx.de>
---
 meta/classes/image.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 0150f2718573..21d634a8f34f 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -153,7 +153,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 after do_rootfs_postprocess
 
 python do_image_tools() {
     """Virtual task"""
-- 
2.25.4

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

[Henning Schild]

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.

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.

Henning

Am Thu, 25 Jun 2020 17:33:51 +0200
schrieb Harald Seiler <hws@denx.de>:

> In some cases, postprocessing might trigger an update of the initramfs
> which would not appear in the deployed version.  Fix this by running
> copy_boot_files only after all postprocessing completed.
> 
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>  meta/classes/image.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 0150f2718573..21d634a8f34f 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -153,7 +153,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 after
> do_rootfs_postprocess 
>  python do_image_tools() {
>      """Virtual task"""

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

[Jan Kiszka]

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.

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.
> 
> Henning
> 
> Am Thu, 25 Jun 2020 17:33:51 +0200
> schrieb Harald Seiler <hws@denx.de>:
> 
>> In some cases, postprocessing might trigger an update of the initramfs
>> which would not appear in the deployed version.  Fix this by running
>> copy_boot_files only after all postprocessing completed.
>>
>> Signed-off-by: Harald Seiler <hws@denx.de>
>> ---
>>   meta/classes/image.bbclass | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>> index 0150f2718573..21d634a8f34f 100644
>> --- a/meta/classes/image.bbclass
>> +++ b/meta/classes/image.bbclass
>> @@ -153,7 +153,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 after
>> do_rootfs_postprocess
>>   python do_image_tools() {
>>       """Virtual task"""
> 

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

[Harald Seiler]

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.

-- 
Harald

> > Henning
> > 
> > Am Thu, 25 Jun 2020 17:33:51 +0200
> > schrieb Harald Seiler <hws@denx.de>:
> > 
> > > In some cases, postprocessing might trigger an update of the initramfs
> > > which would not appear in the deployed version.  Fix this by running
> > > copy_boot_files only after all postprocessing completed.
> > > 
> > > Signed-off-by: Harald Seiler <hws@denx.de>
> > > ---
> > >   meta/classes/image.bbclass | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> > > index 0150f2718573..21d634a8f34f 100644
> > > --- a/meta/classes/image.bbclass
> > > +++ b/meta/classes/image.bbclass
> > > @@ -153,7 +153,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 after
> > > do_rootfs_postprocess
> > >   python do_image_tools() {
> > >       """Virtual task"""

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

[Henning Schild]

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.

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

ACK.

Henning

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

[Henning Schild]

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.

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?

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.
>  
> > 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!
> 
> ACK.
> 
> Henning
> 

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

[Henning Schild]

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

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.
> >    
> > > 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!
> > 
> > ACK.
> > 
> > Henning
> >   
> 

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

[Claudius Heine]

[-- Attachment #1.1: Type: text/plain, Size: 3025 bytes --]

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.

regards,
Claudius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Harald Seiler]

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

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

[Harald Seiler]

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.

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

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

[Jan Kiszka]

On 26.06.20 10:13, Harald Seiler wrote:
> 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.

That class is currently coupled to the use case of A/B partition 
selection via initramfs for the purpose of atomic system update. If we 
see use cases beyond, it would be straightforward to upstream. For the 
time being, the update integration will be evolved in isar-cip-core.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Henning Schild]

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

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

[Jan Kiszka]

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.

Jan

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

[Jan Kiszka]

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

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Henning Schild]

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

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

[Claudius Heine]

[-- Attachment #1.1: Type: text/plain, Size: 3141 bytes --]

Hi Jan,

On 26/06/2020 11.12, Jan Kiszka wrote:
>>>>> 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.

Not sure if I understand you correctly. Do you mean that minimization in
the rootfs postprocess should not be done and instead the things that
get removed there should just not be installed?

To be concrete, one thing that might be removed is 'apt' and 'dpkg'
itself. The only way how you can setup a root file system without it is
if you would install it using a package management from outside the root
file system.

While this would be the cleanest approach, and might even be supported
(See RootDir in apt.conf(5)), that would mean pretty massive changes to
Isar.

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

Right, copy_boot_files is in need for some rethinking.

regards,
Claudius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jan Kiszka]

On 29.06.20 11:04, Claudius Heine wrote:
> Hi Jan,
> 
> On 26/06/2020 11.12, Jan Kiszka wrote:
>>>>>> 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.
> 
> Not sure if I understand you correctly. Do you mean that minimization in
> the rootfs postprocess should not be done and instead the things that
> get removed there should just not be installed?
> 
> To be concrete, one thing that might be removed is 'apt' and 'dpkg'
> itself. The only way how you can setup a root file system without it is
> if you would install it using a package management from outside the root
> file system.
> 
> While this would be the cleanest approach, and might even be supported
> (See RootDir in apt.conf(5)), that would mean pretty massive changes to
> Isar.

If you can avoid installing, you do not need to remove things again for 
shrinking. That's what I meant here. Obviously, you can't avoid apt or 
dpkg in the first place, so that shrinking pattern would remain different.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Harald Seiler]

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

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

[Jan Kiszka]

On 29.06.20 14:22, Harald Seiler wrote:
> 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,

That is no issue: buildchroot-target and target rootfs are in line. If 
not, you have a different issue.

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

E.g. by generating the UUID at bitbake level and injecting it into the 
images - rather than doing that inside one of the images.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Henning Schild]

On Mon, 29 Jun 2020 14:41:29 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 29.06.20 14:22, Harald Seiler wrote:
> > 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,  
> 
> That is no issue: buildchroot-target and target rootfs are in line.
> If not, you have a different issue.
> 
> > 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 ...
> >   
> 
> E.g. by generating the UUID at bitbake level and injecting it into
> the images - rather than doing that inside one of the images.

I anyway do not get why that UUID is needed in the first place. Which
partition to take as root partition can be decided with a partition
label. You just have to be careful with swupdate, it destroy those
labels when flashing whole partitions.
Even if you decide to use an UUID, the bootloader should know that and
pass that arg.

 ... root=$(get_part_uuid(initrd-file)) ...

Might require improving bootloaders ;) or writing bootloader configs
instead of initrds in postrprocess ...

You can also put your static desired UUID in your wks-file and a
postinst of a customization-package. But that might be only for
recipes/layers that are not meant for sharing

Henning

> Jan
> 

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

[Jan Kiszka]

On 29.06.20 14:55, Henning Schild wrote:
> On Mon, 29 Jun 2020 14:41:29 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 29.06.20 14:22, Harald Seiler wrote:
>>> 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,
>>
>> That is no issue: buildchroot-target and target rootfs are in line.
>> If not, you have a different issue.
>>
>>> 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 ...
>>>    
>>
>> E.g. by generating the UUID at bitbake level and injecting it into
>> the images - rather than doing that inside one of the images.
> 
> I anyway do not get why that UUID is needed in the first place. Which
> partition to take as root partition can be decided with a partition
> label. You just have to be careful with swupdate, it destroy those
> labels when flashing whole partitions.

We need to identify the *filesystem* instance, not the partition, while 
booting. A new version of the filesystem may be in partition A or 
partition B, depending on how often the system was already updated.

I'm literally just now painting this slide for my talk tomorrow...

Jan

> Even if you decide to use an UUID, the bootloader should know that and
> pass that arg.
> 
>   ... root=$(get_part_uuid(initrd-file)) ...
> 
> Might require improving bootloaders ;) or writing bootloader configs
> instead of initrds in postrprocess ...
> 
> You can also put your static desired UUID in your wks-file and a
> postinst of a customization-package. But that might be only for
> recipes/layers that are not meant for sharing
> 
> Henning
> 
>> Jan
>>
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Claudius Heine]

[-- Attachment #1.1: Type: text/plain, Size: 5021 bytes --]

Hi Jan,

On 2020-06-29 14:41, Jan Kiszka wrote:
> On 29.06.20 14:22, Harald Seiler wrote:
>> 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,
> 
> That is no issue: buildchroot-target and target rootfs are in line. If
> not, you have a different issue.

I don't think you can rely on that. buildchroot-target is not image
specific, its architecture specific, so if you build multiple different
images in the same project (same TMPDIR) for the same debian
architecture the buildchroot-target will be reused for each on them and
possible contains additional packages from other image builds.

We should probably implement a similar mechanism as OE, where each
recipe gets their own build root fs in isar. Maybe using overlay, or at
least hardlinks should be possible.

regards,
Claudius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jan Kiszka]

On 25.06.20 17:33, Harald Seiler wrote:
> In some cases, postprocessing might trigger an update of the initramfs
> which would not appear in the deployed version.  Fix this by running
> copy_boot_files only after all postprocessing completed.
> 
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>  meta/classes/image.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 0150f2718573..21d634a8f34f 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -153,7 +153,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 after do_rootfs_postprocess
>  
>  python do_image_tools() {
>      """Virtual task"""
> 

There was a lengthy discussion after this, but I don't think the
conclusion was to hold back this patch, right? We are using it at least
in one case internally, so this should likely go in now.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

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

[Harald Seiler]

Hi Jan,

On Tue, 2020-10-13 at 12:19 +0200, Jan Kiszka wrote:
> On 25.06.20 17:33, Harald Seiler wrote:
> > In some cases, postprocessing might trigger an update of the initramfs
> > which would not appear in the deployed version.  Fix this by running
> > copy_boot_files only after all postprocessing completed.
> > 
> > Signed-off-by: Harald Seiler <hws@denx.de>
> > ---
> >  meta/classes/image.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> > index 0150f2718573..21d634a8f34f 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -153,7 +153,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 after do_rootfs_postprocess
> >  
> >  python do_image_tools() {
> >      """Virtual task"""
> > 
> 
> There was a lengthy discussion after this, but I don't think the
> conclusion was to hold back this patch, right? We are using it at least
> in one case internally, so this should likely go in now.

At least from my side we can drop this.  In the project where I needed
this it is now gone, because I switched to the alternate initramfs
mechanism over there.

Also, the arguments where this patch might break downstream use-cases do
hold some truth so I'd rather err on the side of caution and not go this
route.

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

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

[Jan Kiszka]

On 13.10.20 12:26, Harald Seiler wrote:
> Hi Jan,
> 
> On Tue, 2020-10-13 at 12:19 +0200, Jan Kiszka wrote:
>> On 25.06.20 17:33, Harald Seiler wrote:
>>> In some cases, postprocessing might trigger an update of the initramfs
>>> which would not appear in the deployed version.  Fix this by running
>>> copy_boot_files only after all postprocessing completed.
>>>
>>> Signed-off-by: Harald Seiler <hws@denx.de>
>>> ---
>>>  meta/classes/image.bbclass | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>>> index 0150f2718573..21d634a8f34f 100644
>>> --- a/meta/classes/image.bbclass
>>> +++ b/meta/classes/image.bbclass
>>> @@ -153,7 +153,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 after do_rootfs_postprocess
>>>  
>>>  python do_image_tools() {
>>>      """Virtual task"""
>>>
>>
>> There was a lengthy discussion after this, but I don't think the
>> conclusion was to hold back this patch, right? We are using it at least
>> in one case internally, so this should likely go in now.
> 
> At least from my side we can drop this.  In the project where I needed
> this it is now gone, because I switched to the alternate initramfs
> mechanism over there.

Seems that this change in the downstream project didn't hit its master yet.

> 
> Also, the arguments where this patch might break downstream use-cases do
> hold some truth so I'd rather err on the side of caution and not go this
> route.
> 

OK.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux