PR#1199: GenericBuilder: Add a boolean 'succeeded' parameter to finalize - fm-orchestrator

fm-orchestrator

#1199 GenericBuilder: Add a boolean 'succeeded' parameter to finalize

Merged 5 years ago by mprahl. Opened 5 years ago by otaylor.

otaylor/fm-orchestrator builder-finalize-succeeded into master

GenericBuilder: Add a boolean 'succeeded' parameter to finalize

Owen W. Taylor • 5 years ago

da57146

module_build_service/builder/KojiModuleBuilder.py

file modified

+1 -1

		`@@ -1209,7 +1209,7 @@`
		`nvrs = set(kobo.rpmlib.make_nvr(rpm, force_epoch=True) for rpm in rpms)`
		`return list(nvrs)`

		`- def finalize(self):`
		`+ def finalize(self, succeeded=True):`
		`# Only import to koji CG if the module is "build" and not scratch.`
		`if (not self.module.scratch and`
		`self.config.koji_enable_content_generator and`

module_build_service/builder/MockModuleBuilder.py

file modified

+3 -3

		`@@ -542,11 +542,11 @@`
		`def repo_from_tag(cls, config, tag_name, arch):`
		`pass`

		`- def finalize(self):`
		`- # If the state is "done", run one last createrepo, to include`
		`+ def finalize(self, succeeded=True):`
		`+ # For successful builds, do one last createrepo, to include`
		`# the module metadata. We don't want to do this for failed builds,`
		`# since that makes it impossible to retry a build manually.`
		`- if self.module.state == models.BUILD_STATES["done"]:`
		`+ if succeeded:`
		`self._createrepo(include_module_yaml=True)`

		`@classmethod`

module_build_service/builder/base.py

file modified

+3 -2

		`@@ -262,12 +262,13 @@`
		`raise NotImplementedError()`

		`@abstractmethod`
		`- def finalize(self):`
		`+ def finalize(self, succeeded=True):`
		`"""`
		`+ :param succeeded: True if all module builds were successful`
		`:return: None`

		`This method is supposed to be called after all module builds are`
		`- successfully finished.`
		`+ finished.`

		`It could be utilized for various purposes such as cleaning or`
		`running additional build-system based operations on top of`

module_build_service/scheduler/handlers/modules.py

file modified

+1 -1

		`@@ -92,7 +92,7 @@`
		`session.add(component)`

		`# Tell the external buildsystem to wrap up`
		`- builder.finalize()`
		`+ builder.finalize(succeeded=False)`
		`else:`
		`# Do not overwrite state_reason set by Frontend if any.`
		`if not build.state_reason:`

module_build_service/scheduler/handlers/repos.py

file modified

+1 -1

		`@@ -151,7 +151,7 @@`
		`else:`
		`# Tell the external buildsystem to wrap up (CG import, createrepo, etc.)`
		`module_build.time_completed = datetime.utcnow()`
		`- builder.finalize()`
		`+ builder.finalize(succeeded=True)`

		`module_build.transition(config, state=models.BUILD_STATES['done'])`
		`session.commit()`

tests/test_build/test_build.py

file modified

+13 -2

		`@@ -103,6 +103,7 @@`

		`on_build_cb = None`
		`on_cancel_cb = None`
		`+ on_finalize_cb = None`
		`on_buildroot_add_artifacts_cb = None`
		`on_tag_artifacts_cb = None`

		`@@ -118,6 +119,7 @@`
		`FakeModuleBuilder.INSTANT_COMPLETE = False`
		`FakeModuleBuilder.on_build_cb = None`
		`FakeModuleBuilder.on_cancel_cb = None`
		`+ FakeModuleBuilder.on_finalize_cb = None`
		`FakeModuleBuilder.on_buildroot_add_artifacts_cb = None`
		`FakeModuleBuilder.on_tag_artifacts_cb = None`
		`FakeModuleBuilder.DEFAULT_GROUPS = None`
		`@@ -281,8 +283,9 @@`
		`component_build.nvr))`
		`return msgs`

		`- def finalize(self):`
		`- pass`
		`+ def finalize(self, succeeded=None):`
		`+ if FakeModuleBuilder.on_finalize_cb:`
		`+ FakeModuleBuilder.on_finalize_cb(self, succeeded)`


		`def cleanup_moksha():`
		`@@ -354,9 +357,13 @@`
		`tag_groups.append(set(['perl-Tangerine-1-1', 'perl-List-Compare-1-1']))`
		`tag_groups.append(set(['tangerine-1-1']))`

		`+ def on_finalize_cb(cls, succeeded):`
		`+ assert succeeded is True`
		`+`
		`def on_tag_artifacts_cb(cls, artifacts, dest_tag=True):`
		`assert tag_groups.pop(0) == set(artifacts)`

		`+ FakeModuleBuilder.on_finalize_cb = on_finalize_cb`
		`FakeModuleBuilder.on_tag_artifacts_cb = on_tag_artifacts_cb`

		`# Check that the components are added to buildroot after the batch`
		`@@ -505,11 +512,15 @@`
		`def on_cancel_cb(cls, task_id):`
		`cancelled_tasks.append(task_id)`

		`+ def on_finalize_cb(cls, succeeded):`
		`+ assert succeeded is False`
		`+`
		`# We do not want the builds to COMPLETE, but instead we want them`
		`# to be in the BULDING state after the FakeModuleBuilder.build().`
		`FakeModuleBuilder.BUILD_STATE = "BUILDING"`
		`FakeModuleBuilder.on_build_cb = on_build_cb`
		`FakeModuleBuilder.on_cancel_cb = on_cancel_cb`
		`+ FakeModuleBuilder.on_finalize_cb = on_finalize_cb`

		`msgs = []`
		`stop = module_build_service.scheduler.make_simple_stop_condition(db.session)`

tests/test_scheduler/test_repo_done.py

file modified

+2 -1

		`@@ -107,9 +107,10 @@`
		`module_build.time_completed = None`
		`db.session.commit()`

		`- def mocked_finalizer():`
		`+ def mocked_finalizer(succeeded=None):`
		`# Check that the time_completed is set in the time when`
		`# finalizer is called.`
		`+ assert succeeded is True`
		`module_build = module_build_service.models.ModuleBuild.query.get(2)`
		`assert module_build.time_completed is not None`

tests/test_utils/test_utils.py

file modified

+1 -1

		`@@ -866,7 +866,7 @@`
		`def repo_from_tag(self, config, tag_name, arch):`
		`pass`

		`- def finalize(self):`
		`+ def finalize(self, succeeded=True):`
		`pass`

otaylor commented 5 years ago

Previously MockModuleBuilder was checking the module state to see if
it should run a final createrepo, but since eafa930, finalize() is
called before changing the module state; add an explicit boolean to
GenericBuilder.finalize() to avoid worrying about ordering.

Ideally a test would be added to see that the createrepo gets done / doesn't get done appropriately, but that would require extensively extending the MockModuleBuilder tests.