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

Backport toolkit container detection using systemd-detect-virt #11135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dmcilvaney
Copy link
Contributor

@dmcilvaney dmcilvaney commented Nov 19, 2024

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./LICENSES-AND-NOTICES/SPECS/data/licenses.json, ./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md, ./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

Backport of #11039. Removed the calls to logger.PrintMessageBox() and replaced with logger.Log.Warn().

Previous PR description follows:

There have been several issues with a mismatch between the build environment and the detected state by the toolkit. When running in docker, the chroots generally need to be configured externally with their mounts and re-used. This can be done via setting `CHROOT_DIR=/path/to/reusable/chroots`, and if the toolkit thinks it's in a container, it will switch modes.

See https://github.com/microsoft/azurelinux-tutorials/tree/main/build-in-container for more details.

Some builds are currently failing because what is ostensibly a container environment does not have /.dockerenv present.

In the opposite direction, there are also situations where WSL images (which should work fine as a normal build) are reporting as docker because they have a /.dockerenv file present.

systemd has a tool (systemd-detect-virt) which is designed to detect what sort of virtualization is being used to run the current environment. Instead of designing a new system to re-implement this behavior, we can just use this tool. We already have an implicit build dependency on systemd (we run the docker service etc.) so adding it as an explicit requirement shouldn't change anything.

Also, to help people self-diagnose, sanity check the configurations and warn the user:

  • If the CHROOT_DIR is set, but the tool thinks it's in a normal environment, print a warning. A fatal error will follow immediately after if this is actually broken.
  • If the systemd-detect-virt tool is not present, print a warning but fallback to the old behavior.

To validate this we will need to add a new testcase to the toolkit sanity test pipeline that ensures the chroots keep working.

Change Log
  • Prefer systemd-detect-virt over /.dockerenv for container detection
  • Print warnings if misconfiguration is detected
Does this affect the toolchain?

NO

Associated issues
Links to CVEs
Test Methodology
  • TBD

@microsoft-github-policy-service microsoft-github-policy-service bot added the main PR Destined for main label Nov 19, 2024
@dmcilvaney dmcilvaney added bug Something isn't working Tools labels Nov 19, 2024
@dmcilvaney dmcilvaney force-pushed the damcilva/2.0/tools/docker_detect_backport branch from 7f1f225 to 837efd7 Compare November 22, 2024 00:30
@dmcilvaney dmcilvaney force-pushed the damcilva/2.0/tools/docker_detect_backport branch from 837efd7 to 8fab474 Compare November 22, 2024 00:32
@dmcilvaney dmcilvaney marked this pull request as ready for review November 22, 2024 00:32
@dmcilvaney dmcilvaney requested a review from a team as a code owner November 22, 2024 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working main PR Destined for main Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant