Re: [PATCH v3 1/2] images: add support for container images

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

On 15.02.21 10:46, [ext] Silvano Cirujano Cuesta wrote:
> Wouldn't it make sense to put the "containerize_rootfs" function in a separate class and let "image.bbclass" inherit from it?
> 

Sounds reasonable to me, even if we end up with only one function in
that class, at least so far.

Jan

> The current structure that I have come up to seems weird to me, it isn't obvious that "containerize_rootfs" is meant to be reused.
> 
> (-) an additional class for a single function
> 
> (+) better structured code
> 
> Possibilities that seem to fit somehow:
> - specific class "container.bbclass",
> - specific class "image-container-extension.bbclass"
> - existing class already being inherited by "image.bbclass" ("rootfs.bbclass" -is it a rootfs feature?-, "image-postproc-extension.bbclass", "image-tools-extension.bbclass")
> and I cannot tell which one fits best.
> 
>   Silvano 
> 
> On 12/02/2021 19:23, [ext] Silvano Cirujano Cuesta wrote:
>> On 12/02/2021 19:06, Jan Kiszka wrote:
>>> On 12.02.21 18:46, Silvano Cirujano Cuesta wrote:
>>>> On 12/02/2021 18:10, Jan Kiszka wrote:
>>>>> On 12.02.21 09:51, [ext] Silvano Cirujano Cuesta wrote:
>>>>>> Add support for creation of container images with the build root
>>>>>> filesystems.
>>>>>>
>>>>>> Extend also task "populate_sdk" to support the creation of a container image
>>>>>> containing the SDK.
>>>>> Should be done in to steps: container-img.bbclass frirst, and then a
>>>>> patch to use it for the SDK as well.
>>>> Ok. There are some many different tastes WRT to big vs small commits :-)
>>> Rather /wrt logically separatable steps.
>>>
>>>>>> Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
>>>>>> ---
>>>>>>  meta/classes/container-img.bbclass       | 88 ++++++++++++++++++++++++
>>>>>>  meta/classes/image-sdk-extension.bbclass | 51 ++++++++++++--
>>>>>>  meta/classes/image.bbclass               |  1 +
>>>>>>  3 files changed, 133 insertions(+), 7 deletions(-)
>>>>>>  create mode 100644 meta/classes/container-img.bbclass
>>>>>>
>>>>>> diff --git a/meta/classes/container-img.bbclass b/meta/classes/container-img.bbclass
>>>>>> new file mode 100644
>>>>>> index 0000000..35c7bbc
>>>>>> --- /dev/null
>>>>>> +++ b/meta/classes/container-img.bbclass
>>>>>> @@ -0,0 +1,88 @@
>>>>>> +# This software is a part of ISAR.
>>>>>> +# Copyright (C) Siemens AG, 2021
>>>>>> +#
>>>>>> +# SPDX-License-Identifier: MIT
>>>>>> +#
>>>>>> +# This class provides the tasks 'containerize_rootfs' and 'containerize_sdk'
>>>>> Nope, it now only provides the former.
>>>> Yes, you're right, will fix it.
>>>>>> +# to create container images containing the target rootfs and the SDK
>>>>>> +# respectively.
>>>>>> +
>>>>>> +CONTAINER_FORMATS ?= "docker-archive"
>>>>>> +
>>>>>> +containerize_rootfs() {
>>>>>> +    local cmd="/bin/dash"
>>>>>> +    local empty_tag="empty"
>>>>>> +    local full_tag="latest"
>>>>>> +    local oci_img_dir="${WORKDIR}/oci-image"
>>>>>> +    local rootfs="$1"
>>>>>> +    local rootfs_id="$2"
>>>>>> +    local container_formats="$3"
>>>>>> +
>>>>>> +    # prepare OCI container image skeleton
>>>>>> +    bbdebug 1 "prepare OCI container image skeleton"
>>>>>> +    rm -rf "${oci_img_dir}"
>>>>>> +    sudo umoci init --layout "${oci_img_dir}"
>>>>>> +    sudo umoci new --image "${oci_img_dir}:${empty_tag}"
>>>>>> +    sudo umoci config --image "${oci_img_dir}:${empty_tag}" \
>>>>>> +        --config.cmd="${cmd}"
>>>>>> +    sudo umoci unpack --image "${oci_img_dir}:${empty_tag}" \
>>>>>> +        "${oci_img_dir}_unpacked"
>>>>>> +
>>>>>> +    # add root filesystem as the flesh of the skeleton
>>>>>> +    sudo cp -a "${rootfs}"/* "${oci_img_dir}_unpacked/rootfs/"
>>>>>> +
>>>>>> +    # pack container image
>>>>>> +    bbdebug 1 "pack container image"
>>>>>> +    sudo umoci repack --image "${oci_img_dir}:${full_tag}" \
>>>>>> +        "${oci_img_dir}_unpacked"
>>>>>> +    sudo umoci remove --image "${oci_img_dir}:${empty_tag}"
>>>>>> +    sudo rm -rf "${oci_img_dir}_unpacked"
>>>>>> +
>>>>>> +    # no root needed anymore
>>>>>> +    sudo chown --recursive $(id -u):$(id -g) "${oci_img_dir}"
>>>>>> +
>>>>>> +    # convert the OCI container image to the desired format
>>>>>> +    image_name="isar-${rootfs_id}"
>>>>>> +    for image_type in ${CONTAINER_FORMATS} ; do
>>>>>> +        image_archive="${DEPLOY_DIR_IMAGE}/${rootfs_id}-${image_type}.tar"
>>>>>> +        bbdebug 1 "Creating container image type: ${image_type}"
>>>>>> +        case "${image_type}" in
>>>>>> +            "docker-archive" | "oci-archive")
>>>>>> +                if [ "${image_type}" = "oci-archive" ] ; then
>>>>>> +                    target="${image_type}:${image_archive}:latest"
>>>>>> +                else
>>>>>> +                    target="${image_type}:${image_archive}:${image_name}:latest"
>>>>>> +                fi
>>>>>> +                rm -f "${image_archive}" "${image_archive}.xz"
>>>>>> +                bbdebug 2 "Converting OCI image to ${image_type}"
>>>>>> +                skopeo --insecure-policy copy \
>>>>>> +                    "oci:${oci_img_dir}:${full_tag}" "${target}"
>>>>>> +                bbdebug 2 "Compressing image"
>>>>>> +                xz -T0 "${image_archive}"
>>>>>> +                ;;
>>>>>> +            "oci")
>>>>>> +                tar --create --xz --directory "${oci_img_dir}" \
>>>>>> +                    --file "${image_archive}.xz" .
>>>>>> +                ;;
>>>>>> +            "docker-daemon" | "containers-storage")
>>>>>> +                skopeo --insecure-policy copy \
>>>>>> +                    "oci:${oci_img_dir}:${full_tag}" \
>>>>>> +                    "${image_type}:${image_name}:latest"
>>>>>> +                ;;
>>>>> Missing check for "Am I in a container?", like in the SDK. Maybe move
>>>>> that test here and share.
>>>> Not needed, since the usage of IMAGE_TYPE is fixing already to container type.
>>>>
>>>> In the case of the SDK the same task is provides the non-containerized format tar-xz.
>>>>
>>> I cannot follow: What is the difference between
>>> CONTAINER_FORMATS="docker-daemon" and SDK_FORMATS="docker-daemon" when
>>> running inside a kas build container? Both do not work, do they?
>> I misunderstood what you meant.
>>
>> But I got it now, and that's what I meant with the messed up case section.
>>
>> In the next version the "Am I a container?" is in the function, no need to do it twice.
>>
>>>>>> +            *)
>>>>>> +                die "Unsupported format for containerize_rootfs: ${image_type}"
>>>>>> +                ;;
>>>>>> +        esac
>>>>>> +    done
>>>>>> +}
>>>>>> +
>>>>>> +do_container_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>>>>>> +do_container_image[vardeps] += "CONTAINER_FORMATS"
>>>>>> +do_container_image(){
>>>>>> +    rootfs_id="${DISTRO}-${DISTRO_ARCH}"
>>>>>> +
>>>>>> +    bbnote "Generate container image in these formats: ${CONTAINER_FORMATS}"
>>>>> Probabably more "bbdebug"? Unsure. But we aren't using bbnote in the
>>>>> core so far. Nor bbdebug, though.
>>>> At least bbdebug is IMO needed for debbuging if goes wrong.
>>>>
>>>> BTW I'm using bbdebug a lot in the containerize_rootfs section because I've missed those kind of traces much too often when trying to debug some issues on ISAR recipes.
>>>>
>>>> Perhaps we should have more debug verbosity in the logs to ease debugging...
>>>>
>>>>>> +    containerize_rootfs "${IMAGE_ROOTFS}" "${rootfs_id}" "${CONTAINER_FORMATS}"
>>>>>> +}
>>>>>> +
>>>>>> +addtask container_image before do_image after do_image_tools
>>>>>> diff --git a/meta/classes/image-sdk-extension.bbclass b/meta/classes/image-sdk-extension.bbclass
>>>>>> index a8c708a..63138da 100644
>>>>>> --- a/meta/classes/image-sdk-extension.bbclass
>>>>>> +++ b/meta/classes/image-sdk-extension.bbclass
>>>>>> @@ -6,11 +6,25 @@
>>>>>>  # This class extends the image.bbclass to supply the creation of a sdk
>>>>>>  
>>>>>>  SDK_INCLUDE_ISAR_APT ?= "0"
>>>>>> +SDK_FORMATS ?= "tar-xz"
>>>>>> +
>>>>>> +sdk_tar_xz() {
>>>>>> +    # Copy mount_chroot.sh for convenience
>>>>>> +    sudo cp ${SCRIPTSDIR}/mount_chroot.sh ${SDKCHROOT_DIR}
>>>>>> +
>>>>>> +    # Create SDK archive
>>>>>> +    cd -P ${SDKCHROOT_DIR}/..
>>>>>> +    sudo tar --transform="s|^rootfs|sdk-${DISTRO}-${DISTRO_ARCH}|" \
>>>>>> +        -c rootfs | xz -T0 > ${DEPLOY_DIR_IMAGE}/sdk-${DISTRO}-${DISTRO_ARCH}.tar.xz
>>>>>> +    bbnote "SDK rootfs available in ${DEPLOY_DIR_IMAGE}/sdk-${DISTRO}-${DISTRO_ARCH}.tar.xz"
>>>>>> +}
>>>>>>  
>>>>>>  do_populate_sdk[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>>>>>>  do_populate_sdk[depends] = "sdkchroot:do_build"
>>>>>> -do_populate_sdk[vardeps] += "SDK_INCLUDE_ISAR_APT"
>>>>>> +do_populate_sdk[vardeps] += "SDK_INCLUDE_ISAR_APT SDK_FORMATS"
>>>>>>  do_populate_sdk() {
>>>>>> +    local sdk_container_formats=""
>>>>>> +
>>>>>>      if [ "${SDK_INCLUDE_ISAR_APT}" = "1" ]; then
>>>>>>          # Copy isar-apt with deployed Isar packages
>>>>>>          sudo cp -Trpfx ${REPO_ISAR_DIR}/${DISTRO} ${SDKCHROOT_DIR}/isar-apt
>>>>>> @@ -48,12 +62,35 @@ do_populate_sdk() {
>>>>>>          done
>>>>>>      done
>>>>>>  
>>>>>> -    # Copy mount_chroot.sh for convenience
>>>>>> -    sudo cp ${SCRIPTSDIR}/mount_chroot.sh ${SDKCHROOT_DIR}
>>>>>> +    # separate SDK formats: TAR and container formats
>>>>>> +    for sdk_format in ${SDK_FORMATS} ; do
>>>>>> +        case ${sdk_format} in
>>>>>> +            "tar-xz")
>>>>>> +                sdk_tar_xz
>>>>>> +                ;;
>>>>>> +            "docker-archive" | "oci" | "oci-archive")
>>>>>> +                if [ -z "${sdk_container_formats}" ] ; then
>>>>> Unneeded, just use the else part unconditionally.
>>>> The else part alone adds a heading whitespace. It's being ignored in containerize_rootfs, but it's still messing up some outputs.
>>>>
>>>> Not really useless, but not important (in fact that was my 1st version). I can change it in the next patch series version that I need anyway.
>>>>
>>> Looks like cosmetics, not functional issues.
>>>
>>> But if you dislike the leading whitespaces in the debug logs, make it
>>> trailing (prepend rather than append).
>>>
>>>>>> +                    sdk_container_formats="${sdk_format}"
>>>>>> +                else
>>>>>> +                    sdk_container_formats="${sdk_container_formats} ${sdk_format}"
>>>>>> +                fi
>>>>>> +                ;;
>>>>>> +            "docker-daemon" | "containers-storage")
>>>>>> +                if [ -f /.dockerenv ] || [ -f /run/.containerenv ] ; then
>>>>>> +                    die "Adding the SDK container image to a container runtime (${sdk_format}) not supported if running from a container (e.g. 'kas-container')"
>>>>>> +                fi
>>>>> See above, should likely go into containerize_rootfs().
>>>> Right, will fix it.
>>>>
>>>> In fact this case section is really messed up, I have to clean it up completely.
>>>>
>>> OK, seems we are again on the same page.
>>>
>>>>>> +                ;;
>>>>>> +            *)
>>>>>> +                die "unsupported SDK format specified: ${sdk_format}"
>>>>>> +                ;;
>>>>>> +        esac
>>>>>> +    done
>>>>>>  
>>>>>> -    # Create SDK archive
>>>>>> -    cd -P ${SDKCHROOT_DIR}/..
>>>>>> -    sudo tar --transform="s|^rootfs|sdk-${DISTRO}-${DISTRO_ARCH}|" \
>>>>>> -        -c rootfs | xz -T0 > ${DEPLOY_DIR_IMAGE}/sdk-${DISTRO}-${DISTRO_ARCH}.tar.xz
>>>>>> +    # generate the SDK in all the desired container formats
>>>>>> +    if [ -n "${sdk_container_formats}" ] ; then
>>>>>> +        bbnote "Generating SDK container in ${sdk_container_formats} format"
>>>>>> +        containerize_rootfs "${SDKCHROOT_DIR}" "sdk-${DISTRO}-${DISTRO_ARCH}" "${sdk_container_formats}"
>>>>>> +    fi
>>>>>>  }
>>>>>> +
>>>>>>  addtask populate_sdk after do_rootfs
>>>>>> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
>>>>>> index eddc444..7fb7b7e 100644
>>>>>> --- a/meta/classes/image.bbclass
>>>>>> +++ b/meta/classes/image.bbclass
>>>>>> @@ -76,6 +76,7 @@ inherit image-tools-extension
>>>>>>  inherit image-postproc-extension
>>>>>>  inherit image-locales-extension
>>>>>>  inherit image-account-extension
>>>>>> +inherit container-img
>>>>>>  
>>>>>>  # Extra space for rootfs in MB
>>>>>>  ROOTFS_EXTRA ?= "64"
>>>>>>
>>>>> Jan
>>>> Silvano
>>>>
>>> Jan
>> Silvano
>>
> 

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