Merge ~pelpsi/launchpad:duplicate_ci_jobs into launchpad:master

[Simone Pelosi]

Stages now depend on architecture

Stages now is a dictionary where each key corresponds
to the architecture and the value is the old stages array.
Thus we'll have number of build farm jobs equals to the
number of architectures.
If an arch is present in the pipeline but missing in previous
stages an error is raised.

LP: #1999591

[Colin Watson (cjwatson)]

Looks like a good start, but I'd definitely be more comfortable with a more detailed test that spelled out the expected output more explicitly, since some of the code doesn't look completely correct.

review: Needs Fixing

[Colin Watson (cjwatson)]

I see that you had to make quite extensive changes to account for one of my earlier comments asking if the structure of `stages` was right. For the record, it's totally fine to push back on me if I ask this sort of question and dealing with it seems to have massive fallout. Looking at this, I think we need to step back and think about it a bit more.

The problem with what we've ended up with here is that the format of the data structure in `CIBuild.stages` (as opposed to whatever intermediate objects we use in the process of constructing that) has changed, and I don't think that can be the right solution to this problem. That data structure is passed to the builder, so changing its format will almost certainly break the ability to run CI jobs on the Launchpad build farm.

The structure that ends up in `CIBuild.stages` is more or less a cleaned-up version of `pipeline` from https://lpci.readthedocs.io/en/latest/configuration.html. It's supposed to be a list of stages, each stage being a list of jobs, and each job being a tuple of (job_name, job_index). Within a stage, each job is executed (the specification allows for this to be in parallel, but in practice right now it's in series); all jobs in the stage are run, but the stage fails if any of those jobs fail. Each stage is executed in sequence, with subsequent stages only executing if previous stages succeeded. We need to preserve something compatible with this, as it's part of the build dispatch protocol; and we need to make sure that the structure continues to reflect what's in the `pipeline` configuration we've read, even if we're filtering it by series.

But there's another problem here. As mentioned above, the intended semantics are that jobs in later pipeline stages aren't run if earlier stages fail, and it's not really obvious how that works with multi-series pipelines given the current arrangements. Consider this hypothetical pipeline to build an artifact once and then test it on multiple series (which might make sense for artifacts like snaps that are generally supposed to be installable on multiple series, but that might have non-trivial interactions with the host system):

  pipeline:
    - build-snap
    - test-snap
  jobs:
    build-snap:
      series: focal
      architectures: [amd64, arm64]
      build-snaps: [snapcraft]
      run: snapcraft
    test-snap:
      matrix:
        - series: bionic
          architectures: [amd64]
        - series: focal
          architectures: [amd64, arm64]
        - series: jammy
          architectures: [amd64, arm64]
      run: ...

This needs to run `build-snap` on focal first, for each of amd64 and arm64; and then if and only if that succeeds, it needs to run `test-snap` on each of bionic (only on amd64, for the sake of a more interesting example), focal, and jammy. If we dispatch entirely separate jobs to the build farm that only contain the CI jobs for each of those releases, this isn't going to work as intended.

What I think `CIBuild.requestBuildsForRefs` needs to do here is to work out a list of build farm jobs it needs to request, each of which will run an appropriate subset of the pipeline depending on the architecture. E...

review: Needs Fixing

[Simone Pelosi (pelpsi)]

> I see that you had to make quite extensive changes to account for one of my
> earlier comments asking if the structure of `stages` was right. For the
> record, it's totally fine to push back on me if I ask this sort of question
> and dealing with it seems to have massive fallout. Looking at this, I think
> we need to step back and think about it a bit more.
>
> The problem with what we've ended up with here is that the format of the data
> structure in `CIBuild.stages` (as opposed to whatever intermediate objects we
> use in the process of constructing that) has changed, and I don't think that
> can be the right solution to this problem. That data structure is passed to
> the builder, so changing its format will almost certainly break the ability to
> run CI jobs on the Launchpad build farm.
>
> The structure that ends up in `CIBuild.stages` is more or less a cleaned-up
> version of `pipeline` from
> https://lpci.readthedocs.io/en/latest/configuration.html. It's supposed to be
> a list of stages, each stage being a list of jobs, and each job being a tuple
> of (job_name, job_index). Within a stage, each job is executed (the
> specification allows for this to be in parallel, but in practice right now
> it's in series); all jobs in the stage are run, but the stage fails if any of
> those jobs fail. Each stage is executed in sequence, with subsequent stages
> only executing if previous stages succeeded. We need to preserve something
> compatible with this, as it's part of the build dispatch protocol; and we need
> to make sure that the structure continues to reflect what's in the `pipeline`
> configuration we've read, even if we're filtering it by series.
>
> But there's another problem here. As mentioned above, the intended semantics
> are that jobs in later pipeline stages aren't run if earlier stages fail, and
> it's not really obvious how that works with multi-series pipelines given the
> current arrangements. Consider this hypothetical pipeline to build an
> artifact once and then test it on multiple series (which might make sense for
> artifacts like snaps that are generally supposed to be installable on multiple
> series, but that might have non-trivial interactions with the host system):
>
> pipeline:
> - build-snap
> - test-snap
> jobs:
> build-snap:
> series: focal
> architectures: [amd64, arm64]
> build-snaps: [snapcraft]
> run: snapcraft
> test-snap:
> matrix:
> - series: bionic
> architectures: [amd64]
> - series: focal
> architectures: [amd64, arm64]
> - series: jammy
> architectures: [amd64, arm64]
> run: ...
>
> This needs to run `build-snap` on focal first, for each of amd64 and arm64;
> and then if and only if that succeeds, it needs to run `test-snap` on each of
> bionic (only on amd64, for the sake of a more interesting example), focal, and
> jammy. If we dispatch entirely separate jobs to the build farm that only
> contain the CI jobs for each of those releases, this isn't going to work as
> intended.
>
> What I think `CIBuild.requestBuildsForRefs` needs to do here is to work out a
> list of build f...

[Colin Watson (cjwatson)]

review: Needs Fixing

[Simone Pelosi (pelpsi)]

Updated and commits squashed!

[Jürgen Gmach (jugmac00)]

For the future... when there are change requests, it is way easier to review the applied changes when you just push a new commit, and then do a squash after approval.

[Colin Watson (cjwatson)]

review: Approve

[Simone Pelosi (pelpsi)]

Squashed into one commit

diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 48e171a..fb29c2e 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -7,6 +7,7 @@ __all__ = [
     "CIBuild",
 ]
 
+from collections import defaultdict
 from copy import copy
 from datetime import timedelta, timezone
 from operator import itemgetter
@@ -22,6 +23,7 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.archivepublisher.debversion import Version
 from lp.buildmaster.enums import (
     BuildFarmJobType,
     BuildQueueStatus,
@@ -86,17 +88,34 @@ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
 def get_stages(configuration):
     """Extract the job stages for this configuration."""
-    stages = []
+    stages = defaultdict(list)
     if not configuration.pipeline:
         raise CannotBuild("No pipeline stages defined")
+    previous_job = ""
     for stage in configuration.pipeline:
-        jobs = []
         for job_name in stage:
+            jobs = defaultdict(list)
             if job_name not in configuration.jobs:
                 raise CannotBuild("No job definition for %r" % job_name)
             for i in range(len(configuration.jobs[job_name])):
-                jobs.append((job_name, i))
-        stages.append(jobs)
+                for arch in configuration.jobs[job_name][i]["architectures"]:
+                    # Making sure that the previous job is present
+                    # in the pipeline for a given arch.
+                    if previous_job != "":
+                        if (
+                            len(stages[arch]) == 0
+                            or previous_job not in stages[arch][-1][0]
+                        ):
+                            raise CannotBuild(
+                                f"Job {job_name} would run on {arch},"
+                                + f"but the previous job {previous_job}"
+                                + "in the same pipeline would not"
+                            )
+                    jobs[arch].append((job_name, i))
+
+            for arch, value in jobs.items():
+                stages[arch].append(value)
+            previous_job = job_name
     return stages
 
 
@@ -118,16 +137,25 @@ def determine_DASes_to_build(configuration, logger=None):
     # the .launchpad.yaml format doesn't currently support other
     # distributions (although nor does the Launchpad build farm).
     distribution = getUtility(ILaunchpadCelebrities).ubuntu
-    for series_name, architecture_names in architectures_by_series.items():
+
+    series_list = []
+    for series_name in architectures_by_series.keys():
         try:
             series = distribution[series_name]
+            series_list.append(series)
         except NotFoundError:
             if logger is not None:
                 logger.error("Unknown Ubuntu series name %s" % series_name)
             continue
+
+    if len(series_list) != 0:
+        latest_series = max(series_list, key=lambda x: Version(x.version))
         architectures = {
-            das.architecturetag: das for das in series.buildable_architectures
+            das.architecturetag: das
+            for das in latest_series.buildable_architectures
         }
+
+        architecture_names = architectures_by_series[latest_series.name]
         for architecture_name in architecture_names:
             try:
                 das = architectures[architecture_name]
@@ -135,7 +163,7 @@ def determine_DASes_to_build(configuration, logger=None):
                 if logger is not None:
                     logger.error(
                         "%s is not a buildable architecture name in "
-                        "Ubuntu %s" % (architecture_name, series_name)
+                        "Ubuntu %s" % (architecture_name, latest_series.name)
                     )
                 continue
             yield das
@@ -776,13 +804,14 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
                         e,
                     )
                 continue
+
             for das in determine_DASes_to_build(configuration, logger=logger):
                 self._tryToRequestBuild(
                     git_repository,
                     commit["sha1"],
                     configuration,
                     das,
-                    stages,
+                    stages[das.architecturetag],
                     logger,
                 )
 
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index a1f2360..32aa183 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -884,6 +884,65 @@ class TestCIBuildSet(TestCaseWithFactory):
         )
         self.assertEqual(("gpu",), build.builder_constraints)
 
+    def test_requestBuildsForRefs_missing_archs_previous_stages(self):
+        logger = BufferLogger()
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        series = self.factory.makeDistroSeries(
+            distribution=ubuntu,
+            name="focal",
+        )
+        self.factory.makeBuildableDistroArchSeries(
+            distroseries=series, architecturetag="amd64"
+        )
+        self.factory.makeBuildableDistroArchSeries(
+            distroseries=series, architecturetag="arm64"
+        )
+        configuration = dedent(
+            """\
+            pipeline:
+                - build
+                - lint
+                - test
+
+            jobs:
+                build:
+                    matrix:
+                        - series: focal
+                          architectures: amd64
+                        - series: bionic
+                          architectures: arm64
+                        - series: focal
+                          architectures: arm64
+                    run: pyproject-build
+                lint:
+                    series: focal
+                    architectures: arm64
+                    run: echo hello world >output
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: echo hello world >output
+            """
+        ).encode()
+        repository = self.factory.makeGitRepository()
+        ref_paths = ["refs/heads/master"]
+        [ref] = self.factory.makeGitRefs(repository, ref_paths)
+        encoded_commit_json = {
+            "sha1": ref.commit_sha1,
+            "blobs": {".launchpad.yaml": configuration},
+        }
+        self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
+
+        getUtility(ICIBuildSet).requestBuildsForRefs(
+            repository, ref_paths, logger
+        )
+        self.assertEqual(
+            "ERROR Failed to request CI builds for %s: "
+            "Job test would run on amd64,but the previous job "
+            "lintin the same pipeline would not\n" % ref.commit_sha1,
+            logger.getLogBuffer(),
+        )
+
     def test_requestBuildsForRefs_triggers_builds(self):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         series = self.factory.makeDistroSeries(
@@ -964,6 +1023,200 @@ class TestCIBuildSet(TestCaseWithFactory):
             ),
         )
 
+    def test_requestBuildsForRefs_multiple_architectures(self):
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        bionic = self.factory.makeDistroSeries(
+            distribution=ubuntu,
+            name="bionic",
+        )
+        focal = self.factory.makeDistroSeries(
+            distribution=ubuntu,
+            name="focal",
+        )
+        jammy = self.factory.makeDistroSeries(
+            distribution=ubuntu,
+            name="jammy",
+        )
+        for series in [bionic, focal, jammy]:
+            self.factory.makeBuildableDistroArchSeries(
+                distroseries=series, architecturetag="amd64"
+            )
+            self.factory.makeBuildableDistroArchSeries(
+                distroseries=series, architecturetag="arm64"
+            )
+        configuration = dedent(
+            """\
+            pipeline:
+                - build
+                - test
+
+            jobs:
+                build:
+                    series: focal
+                    architectures: [amd64, arm64]
+                    run: echo build
+                test:
+                    matrix:
+                        - series: bionic
+                          architectures: [amd64]
+                        - series: focal
+                          architectures: [amd64, arm64]
+                        - series: jammy
+                          architectures: [amd64, arm64]
+                    run: echo test
+            """
+        ).encode()
+        repository = self.factory.makeGitRepository()
+        ref_paths = ["refs/heads/master"]
+        [ref] = self.factory.makeGitRefs(repository, ref_paths)
+        encoded_commit_json = {
+            "sha1": ref.commit_sha1,
+            "blobs": {".launchpad.yaml": configuration},
+        }
+        self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
+
+        getUtility(ICIBuildSet).requestBuildsForRefs(repository, ref_paths)
+
+        builds = getUtility(ICIBuildSet).findByGitRepository(repository)
+        reports = list(
+            getUtility(IRevisionStatusReportSet).findByRepository(repository)
+        )
+
+        jammy_test_arm64 = None
+        jammy_test_amd64 = None
+
+        self.assertEqual(7, len(reports))
+
+        for build in builds:
+            self.assertEqual(ref.commit_sha1, build.commit_sha1)
+
+            if build.distro_arch_series.distroseries.name == "jammy":
+                if build.distro_arch_series.architecturetag == "amd64":
+                    jammy_test_amd64 = build
+                else:
+                    jammy_test_arm64 = build
+
+        self.assertIsNot(None, jammy_test_arm64)
+        self.assertIsNot(None, jammy_test_amd64)
+
+        self.assertEqual(
+            [[("build", 0)], [("test", 1), ("test", 2)]],
+            jammy_test_arm64.stages,
+        )
+        self.assertEqual(
+            [[("build", 0)], [("test", 0), ("test", 1), ("test", 2)]],
+            jammy_test_amd64.stages,
+        )
+        self.assertThat(
+            reports,
+            MatchesSetwise(
+                *(
+                    MatchesStructure.byEquality(
+                        creator=repository.owner,
+                        title=title,
+                        git_repository=repository,
+                        commit_sha1=ref.commit_sha1,
+                        ci_build=build,
+                    )
+                    for title, build in [
+                        # amd
+                        ("build:0", jammy_test_amd64),
+                        ("test:0", jammy_test_amd64),
+                        ("test:1", jammy_test_amd64),
+                        ("test:2", jammy_test_amd64),
+                        # arm
+                        ("build:0", jammy_test_arm64),
+                        ("test:1", jammy_test_arm64),
+                        ("test:2", jammy_test_arm64),
+                    ]
+                )
+            ),
+        )
+
+    def test_requestBuildsForRefs_creates_correct_amount_of_builds(self):
+        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        focal = self.factory.makeDistroSeries(
+            distribution=ubuntu, name="focal", version="20.04"
+        )
+        jammy = self.factory.makeDistroSeries(
+            distribution=ubuntu, name="jammy", version="22.04"
+        )
+        for series in [focal, jammy]:
+            self.factory.makeBuildableDistroArchSeries(
+                distroseries=series, architecturetag="amd64"
+            )
+        configuration = dedent(
+            """\
+            pipeline:
+                - build
+                - test
+
+            jobs:
+                build:
+                    series: jammy
+                    architectures: amd64
+                    run: echo jammy
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: echo focal
+            """
+        ).encode()
+        repository = self.factory.makeGitRepository()
+        ref_paths = ["refs/heads/master"]
+        [ref] = self.factory.makeGitRefs(repository, ref_paths)
+        encoded_commit_json = {
+            "sha1": ref.commit_sha1,
+            "blobs": {".launchpad.yaml": configuration},
+        }
+        self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
+
+        getUtility(ICIBuildSet).requestBuildsForRefs(repository, ref_paths)
+
+        builds = list(getUtility(ICIBuildSet).findByGitRepository(repository))
+        reports = list(
+            getUtility(IRevisionStatusReportSet).findByRepository(repository)
+        )
+        self.assertEqual(2, len(reports))
+
+        self.assertEqual(1, len(builds))
+
+        jammy_build = builds[0]
+
+        self.assertEqual(
+            "jammy", reports[0].ci_build.distro_arch_series.distroseries.name
+        )
+        self.assertEqual(
+            "jammy", reports[1].ci_build.distro_arch_series.distroseries.name
+        )
+
+        self.assertEqual([[("build", 0)], [("test", 0)]], jammy_build.stages)
+
+        # build:0 and test:0 are dispatched as part of the same build farm job
+        # in order that test:0 can wait for build:0 to succeed.
+        # The build farm job must have a single series for its outer container,
+        # so we pick jammy.
+        # `lpci` will create an inner jammy container for build:0 and
+        # an inner focal container for test:0.
+        self.assertThat(
+            reports,
+            MatchesSetwise(
+                *(
+                    MatchesStructure.byEquality(
+                        creator=repository.owner,
+                        title=title,
+                        git_repository=repository,
+                        commit_sha1=ref.commit_sha1,
+                        ci_build=build,
+                    )
+                    for title, build in [
+                        ("build:0", jammy_build),
+                        ("test:0", jammy_build),
+                    ]
+                )
+            ),
+        )
+
     def test_requestBuildsForRefs_no_commits_at_all(self):
         repository = self.factory.makeGitRepository()
         ref_paths = ["refs/heads/master"]