From 328894a534b8d452b23ce0a2e61df700b68acc93 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 16:45:13 +0100 Subject: [PATCH 01/11] move test_licences from test_dependencies.py to test_licences.py `test_dependencies.py` only contained a single test, which tested the licences of the dependencies. It only ran on Intel. Thus, it was a style check in all but name. Move it to `test_licences.py`, which is where it belongs. Signed-off-by: Patrick Roy --- .../build/test_dependencies.py | 42 ------------------- .../integration_tests/style/test_licenses.py | 28 +++++++++++++ 2 files changed, 28 insertions(+), 42 deletions(-) delete mode 100644 tests/integration_tests/build/test_dependencies.py diff --git a/tests/integration_tests/build/test_dependencies.py b/tests/integration_tests/build/test_dependencies.py deleted file mode 100644 index 3e2bcca8c20..00000000000 --- a/tests/integration_tests/build/test_dependencies.py +++ /dev/null @@ -1,42 +0,0 @@ -# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 -"""Enforces controls over dependencies.""" - -import os - -import pytest - -from host_tools import proc -from host_tools.cargo_build import cargo - -pytestmark = pytest.mark.skipif( - "Intel" not in proc.proc_type(), reason="test only runs on Intel" -) - - -def test_licenses(): - """Ensure license compatibility for Firecracker. - - For a list of currently allowed licenses checkout deny.toml in - the root directory. - """ - toml_file = os.path.normpath( - os.path.join(os.path.dirname(os.path.realpath(__file__)), "../../../Cargo.toml") - ) - - _, stdout, stderr = cargo( - "deny", f"--manifest-path {toml_file} check licenses bans" - ) - assert "licenses ok" in stdout - - # "cargo deny" should deny licenses by default but for some reason copyleft is allowed - # by it and if we add a dependency which has copyleft licenses "cargo deny" won't report - # it unless it is explicitly told to do so from the deny.toml. - # Our current deny.toml seems to cover all the cases we need but, - # if there is an exception like copyleft (where we don't want and don't deny - # in deny.toml and is allowed by cardo deny), we don't want to be left in the dark. - # For such cases check "cargo deny" output, make sure that there are no warnings reported - # related to the license and take appropriate actions i.e. either add them to allow list - # or remove them if they are incompatible with our licenses. - license_res = [line for line in stderr.split("\n") if "license" in line] - assert not license_res diff --git a/tests/integration_tests/style/test_licenses.py b/tests/integration_tests/style/test_licenses.py index 0633ac4e104..16d59fe6205 100644 --- a/tests/integration_tests/style/test_licenses.py +++ b/tests/integration_tests/style/test_licenses.py @@ -5,6 +5,8 @@ import datetime from framework import utils_repo +from framework.defs import FC_WORKSPACE_DIR +from host_tools.cargo_build import cargo AMAZON_COPYRIGHT_YEARS = range(2018, datetime.datetime.now().year + 1) AMAZON_COPYRIGHT = ( @@ -116,5 +118,31 @@ def test_for_valid_licenses(): assert not error_msg, f"Files {error_msg} have invalid licenses" +def test_dependency_licenses(): + """Ensure license compatibility for Firecracker. + + For a list of currently allowed licenses checkout deny.toml in + the root directory. + """ + toml_file = FC_WORKSPACE_DIR / "Cargo.toml" + + _, stdout, stderr = cargo( + "deny", f"--manifest-path {toml_file} check licenses bans" + ) + assert "licenses ok" in stdout + + # "cargo deny" should deny licenses by default but for some reason copyleft is allowed + # by it and if we add a dependency which has copyleft licenses "cargo deny" won't report + # it unless it is explicitly told to do so from the deny.toml. + # Our current deny.toml seems to cover all the cases we need but, + # if there is an exception like copyleft (where we don't want and don't deny + # in deny.toml and is allowed by cardo deny), we don't want to be left in the dark. + # For such cases check "cargo deny" output, make sure that there are no warnings reported + # related to the license and take appropriate actions i.e. either add them to allow list + # or remove them if they are incompatible with our licenses. + license_res = [line for line in stderr.split("\n") if "license" in line] + assert not license_res + + if __name__ == "__main__": test_for_valid_licenses() From e9f6f7d85245ad2954715a3936ef165940eceffd Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 16:46:21 +0100 Subject: [PATCH 02/11] test: remove dead code from test_licences.py The `return True` was flagged by IntelliJ as unreachable (and I concur), the main function was probably to allow running the test without pytest, but we have separate mechanism for this these days (`./tools/devtool checkstyle`). Signed-off-by: Patrick Roy --- tests/integration_tests/style/test_licenses.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/integration_tests/style/test_licenses.py b/tests/integration_tests/style/test_licenses.py index 16d59fe6205..c95181850f7 100644 --- a/tests/integration_tests/style/test_licenses.py +++ b/tests/integration_tests/style/test_licenses.py @@ -98,7 +98,6 @@ def _validate_license(filename): or has_intel_copyright or has_rivos_copyright ) - return True def test_for_valid_licenses(): @@ -142,7 +141,3 @@ def test_dependency_licenses(): # or remove them if they are incompatible with our licenses. license_res = [line for line in stderr.split("\n") if "license" in line] assert not license_res - - -if __name__ == "__main__": - test_for_valid_licenses() From 175fe7859e48d54bea1167cefa94bbe92ba0b7e1 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 16:48:09 +0100 Subject: [PATCH 03/11] test: move `test_binary_{size,static_linking}`.py to functional tests The idea here is that these tests depend on the compilation step, and thus test a production binary. This smells "integration" to me. The actual reason for moving this however is so that we can have the `build` group no longer depend on the shared compilation step. Signed-off-by: Patrick Roy --- tests/integration_tests/{build => functional}/test_binary_size.py | 0 .../{build => functional}/test_binary_static_linking.py | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/integration_tests/{build => functional}/test_binary_size.py (100%) rename tests/integration_tests/{build => functional}/test_binary_static_linking.py (100%) diff --git a/tests/integration_tests/build/test_binary_size.py b/tests/integration_tests/functional/test_binary_size.py similarity index 100% rename from tests/integration_tests/build/test_binary_size.py rename to tests/integration_tests/functional/test_binary_size.py diff --git a/tests/integration_tests/build/test_binary_static_linking.py b/tests/integration_tests/functional/test_binary_static_linking.py similarity index 100% rename from tests/integration_tests/build/test_binary_static_linking.py rename to tests/integration_tests/functional/test_binary_static_linking.py From bd1391ffe2afd3142ee32bc79b24d3283a86031a Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 16:56:55 +0100 Subject: [PATCH 04/11] buildkite: eliminate `initial_steps` The point of these is to run in parallel with the compilation steps, and not block the wait step if they fail. However, since we use `depends_on: null` for these, it doesn't matter whether they are initial or not, so just add them via `add_step` with `decorate=False`, as we do for normal steps. Signed-off-by: Patrick Roy --- .buildkite/common.py | 14 +------------- .buildkite/pipeline_pr.py | 15 +++++++++------ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index c33acee9b49..3ced475bc15 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -233,7 +233,7 @@ class BKPipeline: parser = COMMON_PARSER - def __init__(self, initial_steps=None, with_build_step=True, **kwargs): + def __init__(self, with_build_step=True, **kwargs): self.steps = [] self.args = args = self.parser.parse_args() # Retry one time if agent was lost. This can happen if we terminate the @@ -263,18 +263,6 @@ def __init__(self, initial_steps=None, with_build_step=True, **kwargs): else: self.shared_build = None - # If we run initial_steps before the "wait" step above, then a failure of the initial steps - # would result in the build not progressing past the "wait" step (as buildkite only proceeds past a wait step - # if everything before it passed). Thus put the initial steps after the "wait" step, but set `"depends_on": null` - # to start running them immediately (e.g. without waiting for the "wait" step to unblock). - # - # See also https://buildkite.com/docs/pipelines/dependencies#explicit-dependencies-in-uploaded-steps - if initial_steps: - for step in initial_steps: - step["depends_on"] = None - - self.steps += initial_steps - def add_step(self, step, decorate=True): """ Add a step to the pipeline. diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index 42ef986b0b5..2a9740ccd91 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -21,15 +21,18 @@ pipeline = BKPipeline( priority=DEFAULT_PRIORITY, timeout_in_minutes=45, - initial_steps=[ - { - "command": "./tools/devtool -y checkstyle", - "label": "ðŸŠķ Style", - }, - ], with_build_step=not DOC_ONLY_CHANGE, ) +pipeline.add_step( + { + "command": "./tools/devtool -y checkstyle", + "label": "ðŸŠķ Style", + "depends_on": None, + }, + decorate=False, +) + # run sanity build of devtool if Dockerfile is changed if any(x.parent.name == "devctr" for x in changed_files): pipeline.build_group_per_arch( From 4a5fddea4655ca7ee357eb44828d36f0e19a5c3c Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 17:06:32 +0100 Subject: [PATCH 05/11] buildkite: Do not block build group on compilation No tests in the build group use the pre-compiled binaries, as these tests only run clippy, compute code coverage, and run unittests. So just start running these immediately. Signed-off-by: Patrick Roy --- .buildkite/pipeline_pr.py | 1 + 1 file changed, 1 insertion(+) diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index 2a9740ccd91..e971b87da54 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -72,6 +72,7 @@ pipeline.build_group( "ðŸ“Ķ Build", pipeline.devtool_test(pytest_opts="integration_tests/build/"), + decorate=False, ) pipeline.build_group( From 9553c0ab5327975d1ac13d9d73ed7677bbd5b444 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 17:22:02 +0100 Subject: [PATCH 06/11] refactor: buildkite: store architecture per instance Store whether a given instances is x86 or arm Signed-off-by: Patrick Roy --- .buildkite/common.py | 18 +++++++++--------- .buildkite/pipeline_cpu_template.py | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index 3ced475bc15..38aa17a293f 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -13,14 +13,14 @@ import subprocess from pathlib import Path -DEFAULT_INSTANCES = [ - "c5n.metal", # Intel Skylake - "m5n.metal", # Intel Cascade Lake - "m6i.metal", # Intel Icelake - "m6a.metal", # AMD Milan - "m6g.metal", # Graviton2 - "m7g.metal", # Graviton3 -] +DEFAULT_INSTANCES = { + "c5n.metal": "x86_64", # Intel Skylake + "m5n.metal": "x86_64", # Intel Cascade Lake + "m6i.metal": "x86_64", # Intel Icelake + "m6a.metal": "x86_64", # AMD Milan + "m6g.metal": "aarch64", # Graviton2 + "m7g.metal": "aarch64", # Graviton3 +} DEFAULT_PLATFORMS = [ ("al2", "linux_5.10"), @@ -146,7 +146,7 @@ def __call__(self, parser, namespace, value, option_string=None): "--instances", required=False, nargs="+", - default=DEFAULT_INSTANCES, + default=DEFAULT_INSTANCES.keys(), ) COMMON_PARSER.add_argument( "--platforms", diff --git a/.buildkite/pipeline_cpu_template.py b/.buildkite/pipeline_cpu_template.py index 81497a51e3c..2a182ed6e4b 100755 --- a/.buildkite/pipeline_cpu_template.py +++ b/.buildkite/pipeline_cpu_template.py @@ -34,7 +34,7 @@ class BkStep(str, Enum): "tools/devtool -y test --no-build -- -m no_block_pr integration_tests/functional/test_cpu_template_helper.py -k test_guest_cpu_config_change", ], BkStep.LABEL: "🖐ïļ fingerprint", - "instances": DEFAULT_INSTANCES, + "instances": DEFAULT_INSTANCES.keys(), "platforms": DEFAULT_PLATFORMS, }, "cpuid_wrmsr": { From 2f08edbe0d2bb7f74471fc05e437671292803f3e Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 17:23:00 +0100 Subject: [PATCH 07/11] refactor: buildkite: Use `build_group_per_arch` for shared build Reduces code duplication slightly Signed-off-by: Patrick Roy --- .buildkite/common.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index 38aa17a293f..3b09183ec7f 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -258,8 +258,8 @@ def __init__(self, with_build_step=True, **kwargs): # Build sharing if with_build_step: build_cmds, self.shared_build = shared_build() - step_build = group("🏗ïļ Build", build_cmds, **self.per_arch) - self.steps += [step_build, "wait"] + self.build_group_per_arch("🏗ïļ Build", build_cmds, decorate=False) + self.steps += ["wait"] else: self.shared_build = None @@ -311,8 +311,9 @@ def build_group_per_arch(self, *args, **kwargs): """ Build a group, parametrizing over the architectures only. """ + decorate = kwargs.pop("decorate", True) combined = overlay_dict(self.per_arch, kwargs) - return self.add_step(group(*args, **combined)) + return self.add_step(group(*args, **combined), decorate=decorate) def to_dict(self): """Render the pipeline as a dictionary.""" From 71ffdbe3d900c052847abaeff5e81f26c2039bbc Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 17:27:07 +0100 Subject: [PATCH 08/11] buildkite: generate keys for per-arch steps These will allow us to have x86 tests only block on x86 compilation, and arm tests only on arm compilation. Signed-off-by: Patrick Roy --- .buildkite/common.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index 3b09183ec7f..71dc2e0422b 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -258,7 +258,9 @@ def __init__(self, with_build_step=True, **kwargs): # Build sharing if with_build_step: build_cmds, self.shared_build = shared_build() - self.build_group_per_arch("🏗ïļ Build", build_cmds, decorate=False) + self.build_group_per_arch( + "🏗ïļ Build", build_cmds, decorate=False, key_prefix="build" + ) self.steps += ["wait"] else: self.shared_build = None @@ -307,13 +309,20 @@ def build_group(self, *args, **kwargs): combined = overlay_dict(self.per_instance, kwargs) return self.add_step(group(*args, **combined), decorate=decorate) - def build_group_per_arch(self, *args, **kwargs): + def build_group_per_arch(self, label, *args, **kwargs): """ Build a group, parametrizing over the architectures only. """ decorate = kwargs.pop("decorate", True) + key_prefix = kwargs.pop("key_prefix", None) combined = overlay_dict(self.per_arch, kwargs) - return self.add_step(group(*args, **combined), decorate=decorate) + grp = group(label, *args, **combined) + if key_prefix: + for step in grp["steps"]: + step["key"] = ( + key_prefix + "_" + DEFAULT_INSTANCES[step["agents"]["instance"]] + ) + return self.add_step(grp, decorate=decorate) def to_dict(self): """Render the pipeline as a dictionary.""" From 1dfcab7b771daae702787bc050b638349524ffca Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 17:28:25 +0100 Subject: [PATCH 09/11] buildkite: Replace wait step with explicit dependencies This way, if the x86 build is faster than the ARM build, the x86 tests can start running while the ARM ones are still blocked. Signed-off-by: Patrick Roy --- .buildkite/common.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index 71dc2e0422b..732928d4c84 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -261,7 +261,6 @@ def __init__(self, with_build_step=True, **kwargs): self.build_group_per_arch( "🏗ïļ Build", build_cmds, decorate=False, key_prefix="build" ) - self.steps += ["wait"] else: self.shared_build = None @@ -297,6 +296,10 @@ def _adapt_group(self, group): for step in group["steps"]: step["command"] = prepend + step["command"] + if self.shared_build is not None: + step["depends_on"] = ( + "build_" + DEFAULT_INSTANCES[step["agents"]["instance"]] + ) return group def build_group(self, *args, **kwargs): From dd1af79687d4b549634d757205c12c50d6d2d0ca Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 16 Sep 2024 11:17:15 +0100 Subject: [PATCH 10/11] refactor: buildkite: rename `decorate` to `depends_on_build` Name more clearly indicate what the parameter does (adding `depends_on` steps for the shared compilation, and adapting the step commands with an artifact download). Signed-off-by: Patrick Roy --- .buildkite/common.py | 18 ++++++++++-------- .buildkite/pipeline_pr.py | 6 +++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/.buildkite/common.py b/.buildkite/common.py index 732928d4c84..b2496914ed4 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -259,21 +259,21 @@ def __init__(self, with_build_step=True, **kwargs): if with_build_step: build_cmds, self.shared_build = shared_build() self.build_group_per_arch( - "🏗ïļ Build", build_cmds, decorate=False, key_prefix="build" + "🏗ïļ Build", build_cmds, depends_on_build=False, key_prefix="build" ) else: self.shared_build = None - def add_step(self, step, decorate=True): + def add_step(self, step, depends_on_build=True): """ Add a step to the pipeline. https://buildkite.com/docs/pipelines/step-reference :param step: a Buildkite step - :param decorate: inject needed commands for sharing builds + :param depends_on_build: inject needed commands for sharing builds """ - if decorate and isinstance(step, dict): + if depends_on_build and isinstance(step, dict): step = self._adapt_group(step) self.steps.append(step) return step @@ -308,15 +308,17 @@ def build_group(self, *args, **kwargs): https://buildkite.com/docs/pipelines/group-step """ - decorate = kwargs.pop("decorate", True) + depends_on_build = kwargs.pop("depends_on_build", True) combined = overlay_dict(self.per_instance, kwargs) - return self.add_step(group(*args, **combined), decorate=decorate) + return self.add_step( + group(*args, **combined), depends_on_build=depends_on_build + ) def build_group_per_arch(self, label, *args, **kwargs): """ Build a group, parametrizing over the architectures only. """ - decorate = kwargs.pop("decorate", True) + depends_on_build = kwargs.pop("depends_on_build", True) key_prefix = kwargs.pop("key_prefix", None) combined = overlay_dict(self.per_arch, kwargs) grp = group(label, *args, **combined) @@ -325,7 +327,7 @@ def build_group_per_arch(self, label, *args, **kwargs): step["key"] = ( key_prefix + "_" + DEFAULT_INSTANCES[step["agents"]["instance"]] ) - return self.add_step(grp, decorate=decorate) + return self.add_step(grp, depends_on_build=depends_on_build) def to_dict(self): """Render the pipeline as a dictionary.""" diff --git a/.buildkite/pipeline_pr.py b/.buildkite/pipeline_pr.py index e971b87da54..552ad45b4ed 100755 --- a/.buildkite/pipeline_pr.py +++ b/.buildkite/pipeline_pr.py @@ -30,7 +30,7 @@ "label": "ðŸŠķ Style", "depends_on": None, }, - decorate=False, + depends_on_build=False, ) # run sanity build of devtool if Dockerfile is changed @@ -61,7 +61,7 @@ platforms=[("al2", "linux_5.10")], timeout_in_minutes=300, **DEFAULTS_PERF, - decorate=False, + depends_on_build=False, ) # modify Kani steps' label for step in kani_grp["steps"]: @@ -72,7 +72,7 @@ pipeline.build_group( "ðŸ“Ķ Build", pipeline.devtool_test(pytest_opts="integration_tests/build/"), - decorate=False, + depends_on_build=False, ) pipeline.build_group( From 353e8af185acdee755e5bf2db8115166917a064d Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 16 Sep 2024 11:20:03 +0100 Subject: [PATCH 11/11] buildkite: add comment about kwargs consumed by build_group_per_arch All other args are passed down as `**kwargs` to `group`, so call out which ones aren't. Signed-off-by: Patrick Roy --- .buildkite/common.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.buildkite/common.py b/.buildkite/common.py index b2496914ed4..7119b79ee76 100644 --- a/.buildkite/common.py +++ b/.buildkite/common.py @@ -317,6 +317,10 @@ def build_group(self, *args, **kwargs): def build_group_per_arch(self, label, *args, **kwargs): """ Build a group, parametrizing over the architectures only. + + kwargs consumed by this method and not passed down to `group`: + - `depends_on_build` (default: `True`): Whether the steps in this group depend on the artifacts from the shared compilation steps + - `key_prefix`: If set, causes the generated steps to have a "key" field set to f"{key_prefix}_{$ARCH}". """ depends_on_build = kwargs.pop("depends_on_build", True) key_prefix = kwargs.pop("key_prefix", None)