launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28327
[Merge] ~cjwatson/launchpad:cibuild-results-in-database into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:cibuild-results-in-database into launchpad:master with ~cjwatson/launchpad:cibuild-stages-in-database as a prerequisite.
Commit message:
Update CIBuild results in buildd-manager
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/419196
We previously handled all updates to the results of `CIBuild`s in archiveuploader. The main problem with this approach is that without some other rearrangements it only does anything for successful builds, but we want to update the corresponding `RevisionStatusReport` rows in unsuccessful cases too. It also means that report updates are delayed for some time until the serialized upload task completes, which isn't ideal.
Move some of this work back to `storeLogFromWorker` so that report results are updated in buildd-manager. We still defer uploading artifacts to the librarian to archiveuploader, though we now store the `jobs` dictionary returned by the worker in the database rather than marshalling it via a `jobs.json` file on disk.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:cibuild-results-in-database into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index a09a3d7..73ec417 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -1063,6 +1063,7 @@ public.processor = SELECT
public.product = SELECT
public.productseries = SELECT
public.publisherconfig = SELECT
+public.revisionstatusreport = SELECT, INSERT, UPDATE
public.section = SELECT
public.seriessourcepackagebranch = SELECT
public.snap = SELECT
diff --git a/lib/lp/archiveuploader/ciupload.py b/lib/lp/archiveuploader/ciupload.py
index 7cbd698..99a9d6d 100644
--- a/lib/lp/archiveuploader/ciupload.py
+++ b/lib/lp/archiveuploader/ciupload.py
@@ -7,15 +7,10 @@ __all__ = [
"CIUpload",
]
-import json
import os
-from zope.component import getUtility
-
from lp.archiveuploader.utils import UploadError
from lp.buildmaster.enums import BuildStatus
-from lp.code.enums import RevisionStatusResult
-from lp.code.interfaces.revisionstatus import IRevisionStatusReportSet
class CIUpload:
@@ -34,11 +29,7 @@ class CIUpload:
"""Process this upload, loading it into the database."""
self.logger.debug("Beginning processing.")
- jobs_path = os.path.join(self.upload_path, "jobs.json")
- try:
- with open(jobs_path) as jobs_file:
- jobs = json.load(jobs_file)
- except FileNotFoundError:
+ if not build.results:
raise UploadError("Build did not run any jobs.")
# collect all artifacts
@@ -57,49 +48,27 @@ class CIUpload:
dirpath, filename
))
- for job_name in jobs:
- report = getUtility(IRevisionStatusReportSet).getByCIBuildAndTitle(
- build, job_name)
- if not report:
- # The report should normally exist, since
- # lp.code.model.cibuild.CIBuildSet._tryToRequestBuild
- # creates report rows for the jobs it expects to run, but
- # for robustness it's a good idea to ensure its existence
- # here.
- report = getUtility(IRevisionStatusReportSet).new(
- creator=build.git_repository.owner,
- title=job_name,
- git_repository=build.git_repository,
- commit_sha1=build.commit_sha1,
- ci_build=build,
- )
+ for job_id in build.results:
+ report = build.getOrCreateRevisionStatusReport(job_id)
# attach log file
- log_file = os.path.join(self.upload_path, job_name + ".log")
+ log_file = os.path.join(self.upload_path, job_id + ".log")
try:
with open(log_file, mode="rb") as f:
report.setLog(f.read())
except FileNotFoundError as e:
raise UploadError(
"log file `%s` for job `%s` not found" % (
- e.filename, job_name)
+ e.filename, job_id)
) from e
# attach artifacts
- for file_path in artifacts[job_name]:
+ for file_path in artifacts[job_id]:
with open(file_path, mode="rb") as f:
report.attach(
name=os.path.basename(file_path), data=f.read()
)
- # set status
- try:
- result = RevisionStatusResult.items[jobs[job_name]["result"]]
- except KeyError as e:
- raise UploadError(
- "Invalid RevisionStatusResult `%s`" % e.args[0]) from e
- report.transitionToNewResult(result)
-
self.logger.debug("Updating %s" % build.title)
build.updateStatus(BuildStatus.FULLYBUILT)
diff --git a/lib/lp/archiveuploader/tests/test_ciupload.py b/lib/lp/archiveuploader/tests/test_ciupload.py
index 5cc81d9..853cabb 100644
--- a/lib/lp/archiveuploader/tests/test_ciupload.py
+++ b/lib/lp/archiveuploader/tests/test_ciupload.py
@@ -3,11 +3,11 @@
"""Test uploads of CIBuilds."""
-import json
import os
from storm.store import Store
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from lp.archiveuploader.tests.test_uploadprocessor import (
TestUploadProcessorBase,
@@ -22,7 +22,7 @@ from lp.code.interfaces.revisionstatus import IRevisionStatusReportSet
from lp.services.osutils import write_file
-class TestCIUBuildUploads(TestUploadProcessorBase):
+class TestCIBuildUploads(TestUploadProcessorBase):
"""End-to-end tests of CIBuild uploads."""
def setUp(self):
@@ -37,9 +37,11 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
)
def test_requires_completed_CI_job(self):
- """If no jobs run, no `jobs.json` will be created.
+ """If no jobs run, no results will be saved.
- This results in an `UploadError` / rejected upload."""
+ This results in an `UploadError` / rejected upload.
+ """
+ os.makedirs(os.path.join(self.incoming_folder, "test"))
handler = UploadHandler.forProcessor(
self.uploadprocessor,
self.incoming_folder,
@@ -54,16 +56,10 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
)
def test_requires_log_file(self):
- # create "jobs.json"
- path = os.path.join(self.incoming_folder, "test", "jobs.json")
- content = {
- 'build:0':
- {
- 'result': 'SUCCEEDED',
- }
+ removeSecurityProxy(self.build).results = {
+ 'build:0': {'result': 'SUCCEEDED'},
}
- write_file(path, json.dumps(content).encode("utf-8"))
-
+ os.makedirs(os.path.join(self.incoming_folder, "test"))
handler = UploadHandler.forProcessor(
self.uploadprocessor,
self.incoming_folder,
@@ -79,16 +75,12 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
)
def test_triggers_store_upload_for_completed_ci_builds(self):
- # create "jobs.json"
- path = os.path.join(self.incoming_folder, "test", "jobs.json")
- content = {
- 'build:0':
- {
- 'log': 'test_file_hash',
- 'result': 'SUCCEEDED',
- }
+ removeSecurityProxy(self.build).results = {
+ 'build:0': {
+ 'log': 'test_file_hash',
+ 'result': 'SUCCEEDED',
+ },
}
- write_file(path, json.dumps(content).encode("utf-8"))
# create log file
path = os.path.join(self.incoming_folder, "test", "build:0.log")
@@ -107,11 +99,7 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
content = b"abc"
write_file(path, content)
- revision_status_report = self.factory.makeRevisionStatusReport(
- title="build:0",
- ci_build=self.build,
- )
- Store.of(revision_status_report).flush()
+ report = self.build.getOrCreateRevisionStatusReport("build:0")
handler = UploadHandler.forProcessor(
self.uploadprocessor,
@@ -125,9 +113,7 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
self.assertEqual(UploadStatusEnum.ACCEPTED, result)
self.assertEqual(BuildStatus.FULLYBUILT, self.build.status)
- artifact_urls = getUtility(
- IRevisionStatusReportSet
- ).getByCIBuildAndTitle(self.build, "build:0").getArtifactURLs(
+ artifact_urls = report.getArtifactURLs(
RevisionStatusArtifactType.BINARY
)
self.assertEqual(
@@ -135,66 +121,13 @@ class TestCIUBuildUploads(TestUploadProcessorBase):
{url.rsplit("/")[-1] for url in artifact_urls}
)
- def test_requires_valid_result_status(self):
- # create "jobs.json"
- path = os.path.join(self.incoming_folder, "test", "jobs.json")
- content = {
- 'build:0':
- {
- 'result': 'MADE_UP_STATUS', # this is an invalid result
- }
- }
- write_file(path, json.dumps(content).encode("utf-8"))
-
- # create log file
- path = os.path.join(self.incoming_folder, "test", "build:0.log")
- content = "some log content"
- write_file(path, content.encode("utf-8"))
-
- # create artifact
- path = os.path.join(
- self.incoming_folder, "test", "build:0", "ci.whl")
- content = b"abc"
- write_file(path, content)
-
- # create artifact in a sub-directory
- path = os.path.join(
- self.incoming_folder, "test", "build:0", "sub", "test.whl")
- content = b"abc"
- write_file(path, content)
-
- revision_status_report = self.factory.makeRevisionStatusReport(
- title="build:0",
- ci_build=self.build,
- )
- Store.of(revision_status_report).flush()
-
- handler = UploadHandler.forProcessor(
- self.uploadprocessor,
- self.incoming_folder,
- "test",
- self.build,
- )
-
- result = handler.processCIResult(self.log)
-
- # we explicitly provided an invalid result status
- # which causes a rejected upload
- self.assertEqual(
- UploadStatusEnum.REJECTED, result
- )
-
def test_creates_revision_status_report_if_not_present(self):
- # create "jobs.json"
- path = os.path.join(self.incoming_folder, "test", "jobs.json")
- content = {
- 'build:0':
- {
- 'log': 'test_file_hash',
- 'result': 'SUCCEEDED',
- }
+ removeSecurityProxy(self.build).results = {
+ 'build:0': {
+ 'log': 'test_file_hash',
+ 'result': 'SUCCEEDED',
+ },
}
- write_file(path, json.dumps(content).encode("utf-8"))
# create log file
path = os.path.join(self.incoming_folder, "test", "build:0.log")
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index dc49901..50ec99f 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -241,10 +241,9 @@ class BuildFarmJobBehaviourBase:
return d
@defer.inlineCallbacks
- def storeLogFromWorker(self, build_queue=None):
+ def storeLogFromWorker(self, worker_status):
"""See `IBuildFarmJob`."""
- lfa_id = yield self.getLogFromWorker(
- build_queue or self.build.buildqueue_record)
+ lfa_id = yield self.getLogFromWorker(self.build.buildqueue_record)
self.build.setLog(lfa_id)
transaction.commit()
@@ -306,7 +305,7 @@ class BuildFarmJobBehaviourBase:
self.build.buildqueue_record.builder.name, status))
build_status = None
if status == 'OK':
- yield self.storeLogFromWorker()
+ yield self.storeLogFromWorker(worker_status)
# handleSuccess will sometimes perform write operations
# outside the database transaction, so a failure between
# here and the commit can cause duplicated results. For
@@ -314,7 +313,7 @@ class BuildFarmJobBehaviourBase:
# queue twice if notify() crashes.
build_status = yield self.handleSuccess(worker_status, logger)
elif status in fail_status_map:
- yield self.storeLogFromWorker()
+ yield self.storeLogFromWorker(worker_status)
build_status = fail_status_map[status]
else:
raise BuildDaemonError(
diff --git a/lib/lp/code/interfaces/cibuild.py b/lib/lp/code/interfaces/cibuild.py
index 68fa19f..9859722 100644
--- a/lib/lp/code/interfaces/cibuild.py
+++ b/lib/lp/code/interfaces/cibuild.py
@@ -20,6 +20,7 @@ from lazr.restful.fields import Reference
from zope.schema import (
Bool,
Datetime,
+ Dict,
Int,
List,
TextLine,
@@ -119,6 +120,11 @@ class ICIBuildView(IPackageBuildView):
stages = List(
title=_("A list of stages in this build's configured pipeline."))
+ results = Dict(
+ title=_(
+ "A mapping from job IDs to result tokens, retrieved from the "
+ "builder."))
+
def getConfiguration(logger=None):
"""Fetch a CI build's .launchpad.yaml from code hosting, if possible.
@@ -134,6 +140,15 @@ class ICIBuildView(IPackageBuildView):
cannot be parsed.
"""
+ def getOrCreateRevisionStatusReport(job_id):
+ """Get the `IRevisionStatusReport` for a given job in this build.
+
+ Create the report if necessary.
+
+ :param job_id: A job ID, in the form "JOB_NAME:JOB_INDEX".
+ :return: An `IRevisionStatusReport`.
+ """
+
def getFileByName(filename):
"""Return the corresponding `ILibraryFileAlias` in this context.
diff --git a/lib/lp/code/interfaces/revisionstatus.py b/lib/lp/code/interfaces/revisionstatus.py
index 8dcb2e4..b36c832 100644
--- a/lib/lp/code/interfaces/revisionstatus.py
+++ b/lib/lp/code/interfaces/revisionstatus.py
@@ -205,9 +205,9 @@ class IRevisionStatusReport(IRevisionStatusReportView,
class IRevisionStatusReportSet(Interface):
"""The set of all revision status reports."""
- def new(creator, title, git_repository, commit_sha1, date_created=None,
+ def new(creator, title, git_repository, commit_sha1,
url=None, result_summary=None, result=None, date_started=None,
- date_finished=None, log=None):
+ date_finished=None, log=None, ci_build=None):
"""Return a new revision status report.
:param title: A text string.
@@ -215,13 +215,13 @@ class IRevisionStatusReportSet(Interface):
is being created.
:param commit_sha1: The sha1 of the commit for which the report
is being created.
- :param date_created: The date when the report is being created.
:param url: External URL to view result of report.
:param result_summary: A short summary of the result.
:param result: The result of the check job for this revision.
:param date_started: DateTime that report was started.
:param date_finished: DateTime that report was completed.
:param log: Stores the content of the artifact for this report.
+ :param ci_build: The `ICIBuild` that produced this report, if any.
"""
def getByID(id):
diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 33d8255..912d31f 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -387,6 +387,37 @@ class CIBuild(PackageBuildMixin, StormBase):
return []
return self._jobs.get("stages", [])
+ @property
+ def results(self):
+ """See `ICIBuild`."""
+ if self._jobs is None:
+ return {}
+ return self._jobs.get("results", {})
+
+ @results.setter
+ def results(self, results):
+ """See `ICIBuild`."""
+ if self._jobs is None:
+ self._jobs = {}
+ self._jobs["results"] = results
+
+ def getOrCreateRevisionStatusReport(self, job_id):
+ """See `ICIBuild`."""
+ report = getUtility(IRevisionStatusReportSet).getByCIBuildAndTitle(
+ self, job_id)
+ if report is None:
+ # The report should normally exist, since
+ # lp.code.model.cibuild.CIBuildSet._tryToRequestBuild creates
+ # report rows for the jobs it expects to run, but for robustness
+ # it's a good idea to ensure its existence here.
+ report = getUtility(IRevisionStatusReportSet).new(
+ creator=self.git_repository.owner,
+ title=job_id,
+ git_repository=self.git_repository,
+ commit_sha1=self.commit_sha1,
+ ci_build=self)
+ return report
+
def getFileByName(self, filename):
"""See `ICIBuild`."""
if filename.endswith(".txt.gz"):
@@ -487,17 +518,12 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
# lpbuildd.ci._make_job_id in launchpad-buildd;
# lp.archiveuploader.ciupload looks for this report and attaches
# artifacts to it.
- rsr_set = getUtility(IRevisionStatusReportSet)
for stage in stages:
for job_name, i in stage:
# XXX cjwatson 2022-03-17: It would be better if we
# could set some kind of meaningful description as well.
- rsr_set.new(
- creator=git_repository.owner,
- title="%s:%s" % (job_name, i),
- git_repository=git_repository,
- commit_sha1=commit_sha1,
- ci_build=build)
+ build.getOrCreateRevisionStatusReport(
+ "%s:%s" % (job_name, i))
except CIBuildAlreadyRequested:
pass
except Exception as e:
diff --git a/lib/lp/code/model/cibuildbehaviour.py b/lib/lp/code/model/cibuildbehaviour.py
index 29c91cf..35fe231 100644
--- a/lib/lp/code/model/cibuildbehaviour.py
+++ b/lib/lp/code/model/cibuildbehaviour.py
@@ -7,9 +7,6 @@ __all__ = [
"CIBuildBehaviour",
]
-import json
-import os
-
from twisted.internet import defer
from zope.component import adapter
from zope.interface import implementer
@@ -24,6 +21,7 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
from lp.buildmaster.model.buildfarmjobbehaviour import (
BuildFarmJobBehaviourBase,
)
+from lp.code.enums import RevisionStatusResult
from lp.code.interfaces.cibuild import ICIBuild
from lp.soyuz.adapters.archivedependencies import (
get_sources_list_for_building,
@@ -87,12 +85,14 @@ class CIBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
# We have no interesting checks to perform here.
@defer.inlineCallbacks
- def _downloadFiles(self, worker_status, upload_path, logger):
- # In addition to downloading everything from the filemap, we need to
- # save the "jobs" field in order to reliably map files to individual
- # CI jobs.
+ def storeLogFromWorker(self, worker_status):
if "jobs" in worker_status:
- jobs_path = os.path.join(upload_path, "jobs.json")
- with open(jobs_path, "w") as jobs_file:
- json.dump(worker_status["jobs"], jobs_file)
- yield super()._downloadFiles(worker_status, upload_path, logger)
+ # Save the "jobs" field so that the uploader can reliably map
+ # files to individual CI jobs.
+ removeSecurityProxy(self.build).results = worker_status["jobs"]
+ # Update status reports for each individual job.
+ for job_id, job_status in worker_status["jobs"].items():
+ report = self.build.getOrCreateRevisionStatusReport(job_id)
+ report.transitionToNewResult(
+ RevisionStatusResult.items[job_status["result"]])
+ yield super().storeLogFromWorker(worker_status)
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index d44a2fd..d4cc4ec 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -347,6 +347,24 @@ class TestCIBuild(TestCaseWithFactory):
hosting_fixture.getBlob.result = invalid_result
self.assertRaises(CannotParseConfiguration, build.getConfiguration)
+ def test_getOrCreateRevisionStatusReport_present(self):
+ build = self.factory.makeCIBuild()
+ report = self.factory.makeRevisionStatusReport(
+ title="build:0", ci_build=build)
+ self.assertEqual(
+ report, build.getOrCreateRevisionStatusReport("build:0"))
+
+ def test_getOrCreateRevisionStatusReport_absent(self):
+ build = self.factory.makeCIBuild()
+ self.assertThat(
+ build.getOrCreateRevisionStatusReport("build:0"),
+ MatchesStructure.byEquality(
+ creator=build.git_repository.owner,
+ title="build:0",
+ git_repository=build.git_repository,
+ commit_sha1=build.commit_sha1,
+ ci_build=build))
+
class TestCIBuildSet(TestCaseWithFactory):
diff --git a/lib/lp/code/model/tests/test_cibuildbehaviour.py b/lib/lp/code/model/tests/test_cibuildbehaviour.py
index 190622a..505d4b5 100644
--- a/lib/lp/code/model/tests/test_cibuildbehaviour.py
+++ b/lib/lp/code/model/tests/test_cibuildbehaviour.py
@@ -5,7 +5,6 @@
import base64
from datetime import datetime
-import json
import os.path
import re
import time
@@ -59,6 +58,7 @@ from lp.buildmaster.tests.test_buildfarmjobbehaviour import (
TestHandleStatusMixin,
TestVerifySuccessfulBuildMixin,
)
+from lp.code.enums import RevisionStatusResult
from lp.code.model.cibuildbehaviour import CIBuildBehaviour
from lp.services.config import config
from lp.services.log.logger import (
@@ -396,19 +396,46 @@ class TestHandleStatusForCIBuild(
@defer.inlineCallbacks
def test_handleStatus_WAITING_OK_with_jobs(self):
# If the worker status includes a "jobs" item, then we additionally
- # dump that to jobs.json.
+ # save that as the build's results and update its reports.
with dbuser(config.builddmaster.dbuser):
yield self.behaviour.handleStatus(
self.build.buildqueue_record,
{"builder_status": "BuilderStatus.WAITING",
"build_status": "BuildStatus.OK",
"filemap": {"build:0.log": "test_file_hash"},
- "jobs": {"build:0": {"log": "test_file_hash"}}})
- jobs_path = os.path.join(
- self.upload_root, "incoming",
- self.behaviour.getUploadDirLeaf(self.build.build_cookie),
- str(self.build.archive.id), self.build.distribution.name,
- "jobs.json")
- with open(jobs_path) as jobs_file:
- self.assertEqual(
- {"build:0": {"log": "test_file_hash"}}, json.load(jobs_file))
+ "jobs": {
+ "build:0": {
+ "log": "test_file_hash",
+ "result": "SUCCEEDED",
+ },
+ }})
+ self.assertEqual(
+ {"build:0": {"log": "test_file_hash", "result": "SUCCEEDED"}},
+ self.build.results)
+ self.assertEqual(
+ RevisionStatusResult.SUCCEEDED,
+ self.build.getOrCreateRevisionStatusReport("build:0").result)
+
+ @defer.inlineCallbacks
+ def test_handleStatus_WAITING_PACKAGEFAIL_with_jobs(self):
+ # If the worker status includes a "jobs" item, then we additionally
+ # save that as the build's results and update its reports, even if
+ # the build failed.
+ with dbuser(config.builddmaster.dbuser):
+ yield self.behaviour.handleStatus(
+ self.build.buildqueue_record,
+ {"builder_status": "BuilderStatus.WAITING",
+ "build_status": "BuildStatus.PACKAGEFAIL",
+ "filemap": {"build:0.log": "test_file_hash"},
+ "jobs": {
+ "build:0": {
+ "log": "test_file_hash",
+ "result": "FAILED",
+ },
+ }})
+ self.assertEqual(
+ {"build:0": {"log": "test_file_hash", "result": "FAILED"}},
+ self.build.results)
+ self.assertEqual(
+ RevisionStatusResult.FAILED,
+ self.build.getOrCreateRevisionStatusReport("build:0").result)
