osbuilder: allow rootfs builds w/o git or version file deps #9825

ms-mahuber · 2024-06-11T21:27:09Z

This allows to call tooling like the rootfs builder outside of a git environment. Calculating the VERSION_COMMIT field without warnings/errors depends on the presence of a VERSION file and on a git environment. We set the VERSION_COMMIT field to 'unknown' if the version cannot be properly calculated. This not only prevents errors in the Makefile but also prevents from rootfs.sh being called with an empty -o parameter, thus the distro parameter from being parsed incorrectly.
The change also makes a rust version check optional for cases where we want to build the rootfs w/o building the agent and thus w/o the rust dependency

Fixes: 9824

fidencio

A few comments, @ms-mahuber.

fidencio · 2024-06-12T21:14:19Z

tools/osbuilder/Makefile

@@ -23,9 +23,9 @@ TARGET_IMAGE          := $(IMAGES_BUILD_DEST)/kata-containers.img
 TARGET_INITRD         := $(IMAGES_BUILD_DEST)/kata-containers-initrd.img

 VERSION_FILE   := ./VERSION
-VERSION        := $(shell grep -v ^\# $(VERSION_FILE))
+VERSION        := $(if $(shell test -f $(VERSION_FILE)),$(shell grep -v ^\# $(VERSION_FILE)),"unknown")


What would be the case where the VERSION file is not existent / not present?

Our releases do include such file, and any tarball generated on GitHub would as well.

I am not opinionated here. Once scenario can be where such release is being used to create packages (RPMs). For instance, a package that builds and ships the Kata host side components, or a minimal package that ships the relevant tooling/scripting (from osbuilder) to compose the guest components (rootfs, ...).

When creating such packages, my thinking is that one would merely want to include the minimal set of required files to compose the UVM.

Now, the VERSION file (which is symlinked to various places) seems to be less useful for some scenarios - for instance in the case where the UVM/rootfs is being assembled based on pre-built components (no build steps are executed, merely composing the UVM).

Looking at the code, the only real use of the VERSION file is within create_summary_file when we want to compose the rootfs based on pre-built components. Not everyone using kata-containers may be really interested in this summary file.

This is why I would propose to not 'hard depend' on this VERSION file and allow it to be inaccessible.

No emotional attachment here. If people think otherwise that's fair. :)

Note: Another topic may be in the future whether to include a summary file in the UVM at all (buzzword build reproducibility). :) Maybe an idea could be to produce this file alongside the rootfs artifact or make it an optional step.

For instance, a package that builds and ships the Kata host side components, or a minimal package that ships the relevant tooling/scripting (from osbuilder) to compose the guest components (rootfs, ...).

Oh, I see. In this case we already installed such a tool in the system, and it'd still depend on the VERSION file to be placed somewhere, otherwise it'd bail.

I am not against having an "unknown" version here, as long as you're fine on having this on your side as well.

Ideally the release process should do better, and actually just have a version set there that the tool could use, without depending on the VERSION file, but this is orthogonal to your PR.

I have consistently aligned the read-in of the VERSION variable across all Makefiles.

Note: may make sense to move this to a common .mk file in a subsequent PR.

fidencio · 2024-06-12T21:17:02Z

tools/osbuilder/Makefile

 COMMIT_NO      := $(shell git rev-parse HEAD 2> /dev/null || true)
-COMMIT         := $(if $(shell git status --porcelain --untracked-files=no),${COMMIT_NO}-dirty,${COMMIT_NO})
+COMMIT         := $(if $(shell git status --porcelain --untracked-files=no 2> /dev/null),${COMMIT_NO}-dirty,${COMMIT_NO})


For those 2, could we actually check whether it's a git repo as the first thing? Even if we end up calling a helper shell script for doing that.

I believe there is even two aspects here when we think of more production-like environments that would build the UVM components:

the source code may not be a git repository

git may not even be available

So, I guess one may want to check for both and that would be in utils.mk?

So, let's break this down.

Source code is a git repository

git is available, as a git clone ... happened

Source code is NOT a git repository

git is available, but cannot be used to get the commit as this is not a git repo

git is not available

So, IMHO, it having git installed or not is really relevant if the source code is not a git repo. And if it's a git repo, we know git is there as it was cloned from somewhere.

With that said, I'd just check whether the source code is a git repo or not. And yes, I think the check could be done on utils.mk.

I have realized the same pattern exists in various other Makefiles and aligned the calls consistently. In a subsequent change one could de-duplicate those calls across makefiles.

fidencio · 2024-06-12T21:22:16Z

tools/osbuilder/rootfs-builder/rootfs.sh

+	# this check seems duplicate to the check in setup_rootfs, and we should
+	# also only need this check when we need to build the agent (see setup_rootfs)
+	if [ -z "${AGENT_SOURCE_BIN}" ] ; then
+		# need to detect rustc's version too?
+		detect_rust_version ||
+			die "Could not detect the required rust version for AGENT_VERSION='${AGENT_VERSION:-main}'."
+
+		echo "Required rust version: $RUST_VERSION"
+	fi


I'd like to see this in a different commit, but still as part of the same PR.

But ... :-) ... can we try to simply remove this whole block? I think it's not needed. And if it's needed, we should also check for AGENT_TARBALL. But I truly believe it can be removed.

To the best of my knowledge, this should not be needed. Happy to remove if nobody knows otherwise and if testing passes.

Yay, let's remove it.

I think we should remove this check for now indeed as there is no reason to check twice.

But: consistent early checks for the case when we need to build the agent may actually be reasonable. If you look at the relevant part in setup_rootfs(), the check looks a bit different, plus for instance a check for libseccomp is being executed (also, note that the .cargo related line is executed twice, unsure if that is required). Looks like that code can generally be improved.

Maybe in a subsequent PR one can think of adding a function that checks (and maybe installs) all the requirements early in order to fail early resp. more gracefully? Not sure what the intentions here are and whether it's worth the time. Thoughts? :)

fidencio · 2024-06-12T21:22:54Z

tools/osbuilder/scripts/lib.sh

@@ -181,7 +181,8 @@ create_summary_file()
 	[ "$AGENT_INIT" = yes ] && agent="${init}"

 	local -r agentdir="${script_dir}/../../../"
-	local -r agent_version=$(cat ${agentdir}/VERSION)
+	local agent_version=$(cat ${agentdir}/VERSION 2> /dev/null)
+	[ -z "$agent_version" ] && agent_version="unknown"


Okay, but then again, when the VERSION file wouldn't be present?

See answer above.

We set the VERSION variable consistently across Makefiles to 'unknown' if the file is empty or not present. We also use git commands consistently for calculating the COMMIT, COMMIT_NO variables, not erroring out when building outside of a git repository. In create_summary_file we also account for a missing/empty VERSION file. This makes e.g. the UVM build process in an environment where we build outside of git with a minimal/reduced set of files smoother. Signed-off-by: Manuel Huber <mahuber@microsoft.com>

This call is only necessary when building the agent during rootfs composition. In this case, the relevant call is carried out later, see setup_rootfs. Note: without properly gating this call in build_rootfs_distro, additional dependencies would be introduced when only intending to compose the rootfs w/o building the agent, e.g., in a more production-grade build environment. Signed-off-by: Manuel Huber <mahuber@microsoft.com>

ms-mahuber · 2024-06-13T22:59:19Z

A few comments, @ms-mahuber.

I've left a few new comments and split the change into two. I hope for the scope/intention of this PR/issue my changes make sense.

fidencio

lgtm, assuming green tests!

Thanks @ms-mahuber!

ms-mahuber mentioned this pull request Jun 11, 2024

osbuilder: allow rootfs builds w/o git or version file deps #9824

Open

katacontainersbot added the size/small Small and simple task label Jun 11, 2024

ms-mahuber force-pushed the mahuber/main branch from 1602d86 to 3d6c9ad Compare June 12, 2024 16:32

fidencio reviewed Jun 12, 2024

View reviewed changes

ms-mahuber added 2 commits June 13, 2024 22:46

ms-mahuber force-pushed the mahuber/main branch from 3d6c9ad to 4210673 Compare June 13, 2024 22:54

fidencio approved these changes Jun 20, 2024

View reviewed changes

fidencio added the ok-to-test label Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osbuilder: allow rootfs builds w/o git or version file deps #9825

osbuilder: allow rootfs builds w/o git or version file deps #9825

ms-mahuber commented Jun 11, 2024

fidencio left a comment

fidencio Jun 12, 2024

ms-mahuber Jun 12, 2024

fidencio Jun 13, 2024

ms-mahuber Jun 13, 2024 •

edited

fidencio Jun 12, 2024

ms-mahuber Jun 12, 2024

fidencio Jun 13, 2024

ms-mahuber Jun 13, 2024

fidencio Jun 12, 2024

ms-mahuber Jun 12, 2024

fidencio Jun 13, 2024

ms-mahuber Jun 13, 2024 •

edited

fidencio Jun 12, 2024

ms-mahuber Jun 12, 2024

ms-mahuber commented Jun 13, 2024

fidencio left a comment

osbuilder: allow rootfs builds w/o git or version file deps #9825

Are you sure you want to change the base?

osbuilder: allow rootfs builds w/o git or version file deps #9825

Conversation

ms-mahuber commented Jun 11, 2024

fidencio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-mahuber Jun 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-mahuber Jun 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-mahuber commented Jun 13, 2024

fidencio left a comment

Choose a reason for hiding this comment

ms-mahuber Jun 13, 2024 •

edited

ms-mahuber Jun 13, 2024 •

edited