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

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

Jan