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

[Jan Kiszka <jan.kiszka@siemens.com>]

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