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

Message ID 20210212085113.11013-2-silvano.cirujano-cuesta@siemens.com
State Superseded, archived
Headers show
Series Add support for containerized root filesystems | expand

Commit Message

Silvano Cirujano Cuesta Feb. 11, 2021, 10:51 p.m. UTC
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.

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

Comments

Jan Kiszka Feb. 12, 2021, 7:10 a.m. UTC | #1
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.

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

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

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

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

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

> +                ;;
> +            *)
> +                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 Cirujano Cuesta Feb. 12, 2021, 7:46 a.m. UTC | #2
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 :-)

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

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

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

>
>> +                ;;
>> +            *)
>> +                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
Silvano Cirujano Cuesta Feb. 12, 2021, 8:04 a.m. UTC | #3
On 12/02/2021 18:46, [ext] 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 :-)
>
>>> 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() {

Wouldn't it make sense to put the "containerize_rootfs" function in a separate class "container.bbclass" or "image-container-extension.bbclass" or similar and let "image.bbclass" inherit from it?

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

>>> +    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.
>
>>> +            *)
>>> +                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.
>
>>> +                    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.
>
>>> +                ;;
>>> +            *)
>>> +                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
>
Silvano
Jan Kiszka Feb. 12, 2021, 8:06 a.m. UTC | #4
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?

>>
>>> +            *)
>>> +                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 Cirujano Cuesta Feb. 12, 2021, 8:23 a.m. UTC | #5
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
Silvano Cirujano Cuesta Feb. 14, 2021, 11:46 p.m. UTC | #6
Wouldn't it make sense to put the "containerize_rootfs" function in a separate class and let "image.bbclass" inherit from it?

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
>
Jan Kiszka Feb. 15, 2021, 12:31 a.m. UTC | #7
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
>>
>

Patch

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'
+# 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"
+                ;;
+            *)
+                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}"
+    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
+                    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
+                ;;
+            *)
+                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"