Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contrib: Deprecate torcx, ship containerd / docker as sysexts #1216

Merged
merged 40 commits into from
Oct 24, 2023

Conversation

t-lo
Copy link
Member

@t-lo t-lo commented Oct 5, 2023

This PR supersedes #982 which has been idling for a while. Main motivation of re-issuing the PR is to provide a branch directly on the scripts repo for Flatcar maintainers to collaborate on.

Features added

The PR includes a number of improvements to streamline the developer experience with sysexts. The improvements are:

  • build_sysext can now handle dependencies and create sysexts that depend on packages of other sysexts
  • build_image takes a command line option of the sysexts to include in the base OS, defaulting to containerd and docker. Motivation is to make it easier down the road to move base OS packages into sysexts, and to build leaner OS images.

Testing

Unrelated minor improvements

  • The PR also includes a few unrelated QoL improvements which were implemented along the way to ease development work and testing of this PR:
    • Add run_local_tests.sh to lower the bar of running tests locally for the latest image built
    • Make the update payload configurable for the qemu_update.sh test case. Formerly it was always fetched from bincache; fetch failures were silently ignored and lead to this test being skipped.
    • Add a new flag to run_sdk_container to not touch / update the version file (-U)
    • Fix detection of "SDK container image exists in local Docker registry and is up to date" in ci-automation. Formerly, this check always failed for nightly builds, leading to the SDK container being re-downloaded each time an SDK container was newly instanciated (e.g. after running docker container prune).

Dependencies (squashfs zstd support for Jenkins CI nodes)

[ALL DONE] Todo items


sudo "${SCRIPTS_DIR}/build_sysext" --board="${BOARD}" --image_builddir=${BUILD_DIR} --squashfs_base="${BUILD_DIR}/${image_sysext_base}" --manglefs_script="${SCRIPTS_DIR}/manglefs_containerd" containerd-flatcar app-containers/containerd
sudo install -m 0644 -D "${BUILD_DIR}/containerd-flatcar.raw" "${root_fs_dir}"/usr/share/flatcar/
sudo ln -sf /usr/share/flatcar/containerd-flatcar.raw "${root_fs_dir}"/etc/extensions/containerd-flatcar.raw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pothos does this work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't quite understand why we first create a docker sysext pretending containerd is installed via ${PORTAGE_CONFIGROOT}/etc/portage/profile/package.provided and then create a containerd sysext anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was done to have a split containerd and Docker sysext setup. This way users can disable Docker but keep our containerd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The pretending of containerd being installed is to build the Docker sysext without containerd in it.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean whether the symlinking to etc at this phase of the build will work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jepio to answer your question this works because of

# Backup the /etc contents to /usr/share/flatcar/etc to serve as
# source for creating missing files. Make sure that the preexisting
# /usr/share/flatcar/etc does not have any meaningful (non-empty)
# files, so we remove nothing important. There shouldn't be any
# symlinks either. Add "! -type d" to exclude directories as "stat"
# usually returns a size of a directory being 4096 or so.
if [[ $(sudo find "${root_fs_dir}/usr/share/flatcar/etc" -size +0 ! -type d 2>/dev/null | wc -l) -gt 0 ]]; then
die "Unexpected non-empty files in ${root_fs_dir}/usr/share/flatcar/etc"
fi
sudo rm -rf "${root_fs_dir}/usr/share/flatcar/etc"
sudo cp -a "${root_fs_dir}/etc" "${root_fs_dir}/usr/share/flatcar/etc"
.

Copy link
Member Author

@t-lo t-lo Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was done to have a split containerd and Docker sysext setup. This way users can disable Docker but keep our containerd.

Oh I understand the intent, but I'd argue that if we want stackable / dependent sysexts then we should make the packages shipped with each layer explicit, and re-use these when we build the next "upper" layer.

I've extended build_sysext.sh to cover this, and updated build_image_util.sh to use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks more complex than it needs to be. How about just mount the whole previous sysext when building the next one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sysexts do not contain package information since they only cover /usr (and/or /opt). Installed packages are registered in sub-directories below /var/db/pkg.
(I wish this was easier though!)

@@ -110,7 +110,7 @@ create_dev_container() {
# The remount services are provided by coreos-base/coreos-init
systemd_enable "${root_fs_dir}" "multi-user.target" "remount-usr.service"

finish_image "${image_name}" "${disk_layout}" "${root_fs_dir}" "${image_contents}" "${image_contents_wtd}"
DEVCONTAINER=1 finish_image "${image_name}" "${disk_layout}" "${root_fs_dir}" "${image_contents}" "${image_contents_wtd}"
Copy link
Member

@jepio jepio Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks unused

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the whole thing needs some cleaning up. This was added in a no-comment commit without justification, and the environment variable does not appear anywhere else in the code.

@t-lo
Copy link
Member Author

t-lo commented Oct 6, 2023

The PR in its current state:

  • can build OS images
  • does not build any torcx packages or artifacts
  • includes docker and containerd sysexts in OS image and update tarball, which have been manually tested and are working (qemu)

@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Build action triggered: https://github.com/flatcar/scripts/actions/runs/6688993401

@t-lo
Copy link
Member Author

t-lo commented Oct 10, 2023

Added SBOM, licenses, packages and file listing / disk usage inventory information for sysexts.

  • SLSA provenance is included with the sysexts.
  • SBOM and licenses JSON now include all packages of the final image, i.e. a combined list of base image and all base OS sysexts.
  • Packages lists, files list and detailed files list separate sections with files /packages lists for each sysext. The files and detailed files lists include tha sysext squashfs files on the base image.
  • Disk usage contains both final disk image usage as well as usage of each individual sysext squashfs.

Sample files attached - @krnowak @jepio @pothos could you please have a look and tell me if that works for you?

flatcar_production_image_contents.txt
flatcar_production_image_contents_wtd.txt
flatcar_production_image_disk_usage.txt
flatcar_production_image_licenses.json
flatcar_production_image_packages.txt
flatcar_production_image_sbom.json

@t-lo t-lo requested a review from jepio October 10, 2023 07:25
@t-lo t-lo force-pushed the contrib/torcx-deprecation-docker-sysext branch from 672f019 to 1ea4333 Compare October 10, 2023 11:21
@t-lo t-lo force-pushed the contrib/torcx-deprecation-docker-sysext branch from 1ea4333 to 0a2d1f9 Compare October 12, 2023 12:39
Copy link
Member

@pothos pothos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the name app-containers_docker.raw and app-containers_containerd.raw to docker-flatcar.raw and containerd-flatcar.raw. We have documented those for ages in
https://www.flatcar.org/docs/latest/container-runtimes/use-a-custom-docker-or-containerd-version/
and
https://www.flatcar.org/docs/latest/provisioning/sysext/#supplying-your-sysext-image-from-ignition
for a smooth upgrade experience which doesn't break user setups.

Also, app-containers is a gentooism that isn't really meaningful to users. While finally better than app-emulation we just shouldn't use Gentoo packaging categories for user-facing names. Also, we might want to add more things into the mix and have other Flatcar extensions as well, and the use of Gentoo packaging categories is not a good idea for consistency.

@pothos
Copy link
Member

pothos commented Oct 13, 2023

flatcar_production_image_contents.txt

The concatenation is not good for automated processing. We use this file for the image change reports and would have to split all these ### Sysext app-containers_docker.raw headers. Let's have separate files, instead of making our life harder.

@t-lo
Copy link
Member Author

t-lo commented Oct 13, 2023

flatcar_production_image_contents.txt

The concatenation is not good for automated processing. We use this file for the image change reports and would have to split all these ### Sysext app-containers_docker.raw headers. Let's have separate files, instead of making our life harder.

Listing sysext contents in these files in separate sections was an explicit ask. We additionally ship separate files per sysext, so processing everything should be rather simple: stop parsing flatcar_production_image_contents.txt at the first ^#.

@t-lo
Copy link
Member Author

t-lo commented Oct 13, 2023

Please change the name app-containers_docker.raw and app-containers_containerd.raw to docker-flatcar.raw and containerd-flatcar.raw. We have documented those for ages in https://www.flatcar.org/docs/latest/container-runtimes/use-a-custom-docker-or-containerd-version/ and https://www.flatcar.org/docs/latest/provisioning/sysext/#supplying-your-sysext-image-from-ignition for a smooth upgrade experience which doesn't break user setups.

Also, app-containers is a gentooism that isn't really meaningful to users. While finally better than app-emulation we just shouldn't use Gentoo packaging categories for user-facing names. Also, we might want to add more things into the mix and have other Flatcar extensions as well, and the use of Gentoo packaging categories is not a good idea for consistency.

Got it, will update. It's a bit sad because this convention will make it harder to generically split more apps from the base image into sysexts since we need to maintain app -> name relationships for each, while the current implementation guarantees unique names.

@krnowak
Copy link
Member

krnowak commented Oct 13, 2023

flatcar_production_image_contents.txt

The concatenation is not good for automated processing. We use this file for the image change reports and would have to split all these ### Sysext app-containers_docker.raw headers. Let's have separate files, instead of making our life harder.

Listing sysext contents in these files in separate sections was an explicit ask. We additionally ship separate files per sysext, so processing everything should be rather simple: stop parsing flatcar_production_image_contents.txt at the first ^#.

Or maybe even better - create a separate file, like flatcar_production_image_contents_kitchen_sink.txt, and leave other files as they were.

@t-lo
Copy link
Member Author

t-lo commented Oct 13, 2023

flatcar_production_image_contents.txt

The concatenation is not good for automated processing. We use this file for the image change reports and would have to split all these ### Sysext app-containers_docker.raw headers. Let's have separate files, instead of making our life harder.

Listing sysext contents in these files in separate sections was an explicit ask. We additionally ship separate files per sysext, so processing everything should be rather simple: stop parsing flatcar_production_image_contents.txt at the first ^#.

Or maybe even better - create a separate file, like flatcar_production_image_contents_kitchen_sink.txt, and leave other files as they were.

That's what I had initially, then the request came up to create merged lists. I'm not complaining though, the way you propose it is simpler and requires less code. Will update the PR.

@pothos
Copy link
Member

pothos commented Oct 13, 2023

Maybe the communication about the files got confusing because they serve different purposes and land on different places. For the license JSON it makes sense to have a merged view while for the contents txt files it rather doesn't because they are developer-focused. For the SBOM I think the merged view makes sense, too (even if it doesn't truly reflect what the user finds when opening the image, but I think that live SSH interaction for a scanner would also be a valid use case, so better list the files than not).

@t-lo t-lo force-pushed the contrib/torcx-deprecation-docker-sysext branch from 0a2d1f9 to 892ea57 Compare October 13, 2023 14:43
@t-lo t-lo requested review from pothos and krnowak October 13, 2023 14:47
@t-lo
Copy link
Member Author

t-lo commented Oct 13, 2023

Most notably I extended the format of --base-sysexts= in build_image to support sysext names independent of the packages included. The parameter currently defaults to containerd-flatcar:app-containers/containerd,docker-flatcar:app-containers/docker so we get a containerd sysext named containerd-flatcar.raw and a docker sysext named docker-flatcar.raw.

t-lo and others added 21 commits October 23, 2023 16:05
This change removes torcx libraries, references, and commandline options
from build automation scripts and from build_library/.

Containerd and docker are shipped via sysexts which are included in the
base image.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change refactors sysext builds during build_image and generalises
the code (no hard-coded containerd and docker anymore).

A command line option is added to build_image for sysexts to include in
the OS image. It defaults to containerd and docker but may be set to
arbitrary packages. The command line supports simple depenencies, i.e.
the "docker" sysext will re-use package information from the
"containerd" sysext and not include another containerd.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change refactors base OS sysext builds to use a separate build
script `build_library/sysext_prod_builder`, which is called from
`build_library/prod_image_util.sh` when `build_image` runs.

This allows for better separation of cleanup traps: prod image sysext
builds need its own trap / cleanup function for temporary build
directories and loopback mounts.

Prod sysext builds properly generate lincense and SBOM information, and
provide detailed file listings and disk space usage stats.

- SBOM / licenses JSON now include all packages of the
  final image, i.e. a combined list of base image and all base OS
  sysexts.
- Packages lists, files list and detailed files list include the sysext
  squashfs files for the base image, and separate sections with files /
  packages lists for each sysext.
- Disk usage contains both final disk image usage as well as usage of
  each individual sysext squashfs.
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change adds run_local_tests.sh, a script to run tests on local
builds. It's a comfort wrapper around ci-automation scripts and uses
the latest local build.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change makes QEMU_UPDATE_PAYLOAD configurable via
ci-automation/settings.env where it was hard-wired before.

The change also fixes fall-out in qemu_update.sh by ensuring a local tmp
directory is created before it is used by the test.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change bumps the image ref of the mantle container to
ghcr.io/flatcar/mantle:git-20a2f8ffee8c8a1a042b1da99f0f59312110f285.
This version includes 2 PRs (flatcar/mantle#465
and flatcar/mantle#466) which add support for
sysext docker / torcx removal in the OS image.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
This change adds a -U flag to run_sdk_container. If provided, the script
will not regenerate version.txt but instead use the existing file as-is.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Include PR flatcar/update_engine#30 to un-break
updates when torcx was removed in favour of sysext.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
- updated github actions for runc, containerd, and docker to not handle
  nonexistent ebuilds in app-torcx/ anymore
- removed spurious package_run_dependencies from build_image_util.sh
- build_sysext: generate pkginfo before mangle script runs
  use zstd for compression; add cli flag to select compression
- ci_automation_common.sh: remove spurious `/` from match string
- coreos, board-packages, bootengine: bump ebuild revisions
- kernel commonconfig: add squashfs zstd support

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Thank you Krzesimir!

Co-authored-by: Krzesimir Nowak <knowak@microsoft.com>
This change enables USE flags for all supported compression formats.
zstd specifically is required to build zstd sysexts.
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Turns out using ${var@Q} instead of ${var} ends up with paths like
/work/foo/'amd64'-usr/...
instead of
/work/foo/amd64-usr/...
which breaks the script. So we revert it.

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
Co-authored-by: Krzesimir Nowak <knowak@microsoft.com>
Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
@t-lo t-lo force-pushed the contrib/torcx-deprecation-docker-sysext branch from bc8ba62 to f81bbeb Compare October 23, 2023 14:05
@t-lo t-lo temporarily deployed to development October 23, 2023 14:06 — with GitHub Actions Inactive
@t-lo
Copy link
Member Author

t-lo commented Oct 23, 2023

Addressed Krzesimir's comments, rebased to latest main, restarted Ci tests.

@t-lo
Copy link
Member Author

t-lo commented Oct 24, 2023

  • Github CI is all green
  • Jenkins CI is all green
  • 2 LGTMs

==> Merging PR. Thank you so much @pothos and @krnowak for your help and support with improving this PR and getting it merged! Also big thanks to @tormath1 for help with the test suite and to @krishjainx for starting this work!

@t-lo t-lo merged commit ac811ab into main Oct 24, 2023
7 checks passed
@t-lo t-lo deleted the contrib/torcx-deprecation-docker-sysext branch October 24, 2023 07:33
@pothos
Copy link
Member

pothos commented Oct 30, 2023

Thanks, somehow I assumed the kernel config change would be part of last week's releases and also that we should have this here in Alpha. Eagerly waiting for the next Alpha then^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants