Merge ~pelpsi/launchpad:duplicate_ci_jobs into launchpad:master

Proposed by Simone Pelosi
Status: Merged
Approved by: Simone Pelosi
Approved revision: b1007d068d587970912f306dae6378dda7a1736f
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~pelpsi/launchpad:duplicate_ci_jobs
Merge into: launchpad:master
Diff against target: 383 lines (+290/-8)
2 files modified
lib/lp/code/model/cibuild.py (+37/-8)
lib/lp/code/model/tests/test_cibuild.py (+253/-0)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Review via email: mp+440534@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

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
Revision history for this message
Colin Watson (cjwatson) wrote :
Download full text (4.7 KiB)

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...

Read more...

review: Needs Fixing
Revision history for this message
Simone Pelosi (pelpsi) wrote :
Download full text (5.1 KiB)

> 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...

Read more...

Revision history for this message
Colin Watson (cjwatson) :
review: Needs Fixing
Revision history for this message
Simone Pelosi (pelpsi) wrote :

Updated and commits squashed!

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

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.

Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
Simone Pelosi (pelpsi) wrote :

Squashed into one commit

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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__ = [
7 "CIBuild",7 "CIBuild",
8]8]
99
10from collections import defaultdict
10from copy import copy11from copy import copy
11from datetime import timedelta, timezone12from datetime import timedelta, timezone
12from operator import itemgetter13from operator import itemgetter
@@ -22,6 +23,7 @@ from zope.security.proxy import removeSecurityProxy
2223
23from lp.app.errors import NotFoundError24from lp.app.errors import NotFoundError
24from lp.app.interfaces.launchpad import ILaunchpadCelebrities25from lp.app.interfaces.launchpad import ILaunchpadCelebrities
26from lp.archivepublisher.debversion import Version
25from lp.buildmaster.enums import (27from lp.buildmaster.enums import (
26 BuildFarmJobType,28 BuildFarmJobType,
27 BuildQueueStatus,29 BuildQueueStatus,
@@ -86,17 +88,34 @@ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
8688
87def get_stages(configuration):89def get_stages(configuration):
88 """Extract the job stages for this configuration."""90 """Extract the job stages for this configuration."""
89 stages = []91 stages = defaultdict(list)
90 if not configuration.pipeline:92 if not configuration.pipeline:
91 raise CannotBuild("No pipeline stages defined")93 raise CannotBuild("No pipeline stages defined")
94 previous_job = ""
92 for stage in configuration.pipeline:95 for stage in configuration.pipeline:
93 jobs = []
94 for job_name in stage:96 for job_name in stage:
97 jobs = defaultdict(list)
95 if job_name not in configuration.jobs:98 if job_name not in configuration.jobs:
96 raise CannotBuild("No job definition for %r" % job_name)99 raise CannotBuild("No job definition for %r" % job_name)
97 for i in range(len(configuration.jobs[job_name])):100 for i in range(len(configuration.jobs[job_name])):
98 jobs.append((job_name, i))101 for arch in configuration.jobs[job_name][i]["architectures"]:
99 stages.append(jobs)102 # Making sure that the previous job is present
103 # in the pipeline for a given arch.
104 if previous_job != "":
105 if (
106 len(stages[arch]) == 0
107 or previous_job not in stages[arch][-1][0]
108 ):
109 raise CannotBuild(
110 f"Job {job_name} would run on {arch},"
111 + f"but the previous job {previous_job}"
112 + "in the same pipeline would not"
113 )
114 jobs[arch].append((job_name, i))
115
116 for arch, value in jobs.items():
117 stages[arch].append(value)
118 previous_job = job_name
100 return stages119 return stages
101120
102121
@@ -118,16 +137,25 @@ def determine_DASes_to_build(configuration, logger=None):
118 # the .launchpad.yaml format doesn't currently support other137 # the .launchpad.yaml format doesn't currently support other
119 # distributions (although nor does the Launchpad build farm).138 # distributions (although nor does the Launchpad build farm).
120 distribution = getUtility(ILaunchpadCelebrities).ubuntu139 distribution = getUtility(ILaunchpadCelebrities).ubuntu
121 for series_name, architecture_names in architectures_by_series.items():140
141 series_list = []
142 for series_name in architectures_by_series.keys():
122 try:143 try:
123 series = distribution[series_name]144 series = distribution[series_name]
145 series_list.append(series)
124 except NotFoundError:146 except NotFoundError:
125 if logger is not None:147 if logger is not None:
126 logger.error("Unknown Ubuntu series name %s" % series_name)148 logger.error("Unknown Ubuntu series name %s" % series_name)
127 continue149 continue
150
151 if len(series_list) != 0:
152 latest_series = max(series_list, key=lambda x: Version(x.version))
128 architectures = {153 architectures = {
129 das.architecturetag: das for das in series.buildable_architectures154 das.architecturetag: das
155 for das in latest_series.buildable_architectures
130 }156 }
157
158 architecture_names = architectures_by_series[latest_series.name]
131 for architecture_name in architecture_names:159 for architecture_name in architecture_names:
132 try:160 try:
133 das = architectures[architecture_name]161 das = architectures[architecture_name]
@@ -135,7 +163,7 @@ def determine_DASes_to_build(configuration, logger=None):
135 if logger is not None:163 if logger is not None:
136 logger.error(164 logger.error(
137 "%s is not a buildable architecture name in "165 "%s is not a buildable architecture name in "
138 "Ubuntu %s" % (architecture_name, series_name)166 "Ubuntu %s" % (architecture_name, latest_series.name)
139 )167 )
140 continue168 continue
141 yield das169 yield das
@@ -776,13 +804,14 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
776 e,804 e,
777 )805 )
778 continue806 continue
807
779 for das in determine_DASes_to_build(configuration, logger=logger):808 for das in determine_DASes_to_build(configuration, logger=logger):
780 self._tryToRequestBuild(809 self._tryToRequestBuild(
781 git_repository,810 git_repository,
782 commit["sha1"],811 commit["sha1"],
783 configuration,812 configuration,
784 das,813 das,
785 stages,814 stages[das.architecturetag],
786 logger,815 logger,
787 )816 )
788817
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):
884 )884 )
885 self.assertEqual(("gpu",), build.builder_constraints)885 self.assertEqual(("gpu",), build.builder_constraints)
886886
887 def test_requestBuildsForRefs_missing_archs_previous_stages(self):
888 logger = BufferLogger()
889 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
890 series = self.factory.makeDistroSeries(
891 distribution=ubuntu,
892 name="focal",
893 )
894 self.factory.makeBuildableDistroArchSeries(
895 distroseries=series, architecturetag="amd64"
896 )
897 self.factory.makeBuildableDistroArchSeries(
898 distroseries=series, architecturetag="arm64"
899 )
900 configuration = dedent(
901 """\
902 pipeline:
903 - build
904 - lint
905 - test
906
907 jobs:
908 build:
909 matrix:
910 - series: focal
911 architectures: amd64
912 - series: bionic
913 architectures: arm64
914 - series: focal
915 architectures: arm64
916 run: pyproject-build
917 lint:
918 series: focal
919 architectures: arm64
920 run: echo hello world >output
921 test:
922 series: focal
923 architectures: amd64
924 run: echo hello world >output
925 """
926 ).encode()
927 repository = self.factory.makeGitRepository()
928 ref_paths = ["refs/heads/master"]
929 [ref] = self.factory.makeGitRefs(repository, ref_paths)
930 encoded_commit_json = {
931 "sha1": ref.commit_sha1,
932 "blobs": {".launchpad.yaml": configuration},
933 }
934 self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
935
936 getUtility(ICIBuildSet).requestBuildsForRefs(
937 repository, ref_paths, logger
938 )
939 self.assertEqual(
940 "ERROR Failed to request CI builds for %s: "
941 "Job test would run on amd64,but the previous job "
942 "lintin the same pipeline would not\n" % ref.commit_sha1,
943 logger.getLogBuffer(),
944 )
945
887 def test_requestBuildsForRefs_triggers_builds(self):946 def test_requestBuildsForRefs_triggers_builds(self):
888 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu947 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
889 series = self.factory.makeDistroSeries(948 series = self.factory.makeDistroSeries(
@@ -964,6 +1023,200 @@ class TestCIBuildSet(TestCaseWithFactory):
964 ),1023 ),
965 )1024 )
9661025
1026 def test_requestBuildsForRefs_multiple_architectures(self):
1027 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
1028 bionic = self.factory.makeDistroSeries(
1029 distribution=ubuntu,
1030 name="bionic",
1031 )
1032 focal = self.factory.makeDistroSeries(
1033 distribution=ubuntu,
1034 name="focal",
1035 )
1036 jammy = self.factory.makeDistroSeries(
1037 distribution=ubuntu,
1038 name="jammy",
1039 )
1040 for series in [bionic, focal, jammy]:
1041 self.factory.makeBuildableDistroArchSeries(
1042 distroseries=series, architecturetag="amd64"
1043 )
1044 self.factory.makeBuildableDistroArchSeries(
1045 distroseries=series, architecturetag="arm64"
1046 )
1047 configuration = dedent(
1048 """\
1049 pipeline:
1050 - build
1051 - test
1052
1053 jobs:
1054 build:
1055 series: focal
1056 architectures: [amd64, arm64]
1057 run: echo build
1058 test:
1059 matrix:
1060 - series: bionic
1061 architectures: [amd64]
1062 - series: focal
1063 architectures: [amd64, arm64]
1064 - series: jammy
1065 architectures: [amd64, arm64]
1066 run: echo test
1067 """
1068 ).encode()
1069 repository = self.factory.makeGitRepository()
1070 ref_paths = ["refs/heads/master"]
1071 [ref] = self.factory.makeGitRefs(repository, ref_paths)
1072 encoded_commit_json = {
1073 "sha1": ref.commit_sha1,
1074 "blobs": {".launchpad.yaml": configuration},
1075 }
1076 self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
1077
1078 getUtility(ICIBuildSet).requestBuildsForRefs(repository, ref_paths)
1079
1080 builds = getUtility(ICIBuildSet).findByGitRepository(repository)
1081 reports = list(
1082 getUtility(IRevisionStatusReportSet).findByRepository(repository)
1083 )
1084
1085 jammy_test_arm64 = None
1086 jammy_test_amd64 = None
1087
1088 self.assertEqual(7, len(reports))
1089
1090 for build in builds:
1091 self.assertEqual(ref.commit_sha1, build.commit_sha1)
1092
1093 if build.distro_arch_series.distroseries.name == "jammy":
1094 if build.distro_arch_series.architecturetag == "amd64":
1095 jammy_test_amd64 = build
1096 else:
1097 jammy_test_arm64 = build
1098
1099 self.assertIsNot(None, jammy_test_arm64)
1100 self.assertIsNot(None, jammy_test_amd64)
1101
1102 self.assertEqual(
1103 [[("build", 0)], [("test", 1), ("test", 2)]],
1104 jammy_test_arm64.stages,
1105 )
1106 self.assertEqual(
1107 [[("build", 0)], [("test", 0), ("test", 1), ("test", 2)]],
1108 jammy_test_amd64.stages,
1109 )
1110 self.assertThat(
1111 reports,
1112 MatchesSetwise(
1113 *(
1114 MatchesStructure.byEquality(
1115 creator=repository.owner,
1116 title=title,
1117 git_repository=repository,
1118 commit_sha1=ref.commit_sha1,
1119 ci_build=build,
1120 )
1121 for title, build in [
1122 # amd
1123 ("build:0", jammy_test_amd64),
1124 ("test:0", jammy_test_amd64),
1125 ("test:1", jammy_test_amd64),
1126 ("test:2", jammy_test_amd64),
1127 # arm
1128 ("build:0", jammy_test_arm64),
1129 ("test:1", jammy_test_arm64),
1130 ("test:2", jammy_test_arm64),
1131 ]
1132 )
1133 ),
1134 )
1135
1136 def test_requestBuildsForRefs_creates_correct_amount_of_builds(self):
1137 ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
1138 focal = self.factory.makeDistroSeries(
1139 distribution=ubuntu, name="focal", version="20.04"
1140 )
1141 jammy = self.factory.makeDistroSeries(
1142 distribution=ubuntu, name="jammy", version="22.04"
1143 )
1144 for series in [focal, jammy]:
1145 self.factory.makeBuildableDistroArchSeries(
1146 distroseries=series, architecturetag="amd64"
1147 )
1148 configuration = dedent(
1149 """\
1150 pipeline:
1151 - build
1152 - test
1153
1154 jobs:
1155 build:
1156 series: jammy
1157 architectures: amd64
1158 run: echo jammy
1159 test:
1160 series: focal
1161 architectures: amd64
1162 run: echo focal
1163 """
1164 ).encode()
1165 repository = self.factory.makeGitRepository()
1166 ref_paths = ["refs/heads/master"]
1167 [ref] = self.factory.makeGitRefs(repository, ref_paths)
1168 encoded_commit_json = {
1169 "sha1": ref.commit_sha1,
1170 "blobs": {".launchpad.yaml": configuration},
1171 }
1172 self.useFixture(GitHostingFixture(commits=[encoded_commit_json]))
1173
1174 getUtility(ICIBuildSet).requestBuildsForRefs(repository, ref_paths)
1175
1176 builds = list(getUtility(ICIBuildSet).findByGitRepository(repository))
1177 reports = list(
1178 getUtility(IRevisionStatusReportSet).findByRepository(repository)
1179 )
1180 self.assertEqual(2, len(reports))
1181
1182 self.assertEqual(1, len(builds))
1183
1184 jammy_build = builds[0]
1185
1186 self.assertEqual(
1187 "jammy", reports[0].ci_build.distro_arch_series.distroseries.name
1188 )
1189 self.assertEqual(
1190 "jammy", reports[1].ci_build.distro_arch_series.distroseries.name
1191 )
1192
1193 self.assertEqual([[("build", 0)], [("test", 0)]], jammy_build.stages)
1194
1195 # build:0 and test:0 are dispatched as part of the same build farm job
1196 # in order that test:0 can wait for build:0 to succeed.
1197 # The build farm job must have a single series for its outer container,
1198 # so we pick jammy.
1199 # `lpci` will create an inner jammy container for build:0 and
1200 # an inner focal container for test:0.
1201 self.assertThat(
1202 reports,
1203 MatchesSetwise(
1204 *(
1205 MatchesStructure.byEquality(
1206 creator=repository.owner,
1207 title=title,
1208 git_repository=repository,
1209 commit_sha1=ref.commit_sha1,
1210 ci_build=build,
1211 )
1212 for title, build in [
1213 ("build:0", jammy_build),
1214 ("test:0", jammy_build),
1215 ]
1216 )
1217 ),
1218 )
1219
967 def test_requestBuildsForRefs_no_commits_at_all(self):1220 def test_requestBuildsForRefs_no_commits_at_all(self):
968 repository = self.factory.makeGitRepository()1221 repository = self.factory.makeGitRepository()
969 ref_paths = ["refs/heads/master"]1222 ref_paths = ["refs/heads/master"]

Subscribers

People subscribed via source and target branches

to status/vote changes: