PR#1397: newRepo is not called on every batch - fm-orchestrator

fm-orchestrator

#1397 newRepo is not called on every batch

Closed 4 years ago by mprahl. Opened 4 years ago by mcurlej.

mcurlej/fm-orchestrator buildorder into master

newRepo is not called on every batch

Martin Curlej • 4 years ago

6001748

module_build_service/utils/reuse.py

file modified

+6 -4

		`@@ -158,6 +158,7 @@`
		`# [(component, component_to_reuse), ...]`
		`component_pairs = []`

		`+ all_components_reusable = True`
		`# Find out if we can reuse all components and cache component and`
		`# component to reuse pairs.`
		`for c in module.component_builds:`
lucarval commented 4 years ago Don't we need to iterate by batch? The bit later on that advances the module.batch to the highest reusable component.batch would have unintended consequences otherwise.
		`@@ -171,10 +172,11 @@`
		`mmd=mmd,`
		`old_mmd=old_mmd,`
		`)`
		`- if not component_to_reuse:`
		`- return False`
		`+ if all_components_reusable and not component_to_reuse:`
		`+ all_components_reusable = False`
cqi commented 4 years ago Does this change the original function behavior? The original behavior is once no reusable component is found, function returns False immediately. With this change, function will continue going through the `module.component_builds`.
mprahl commented 4 years ago @cqi is correct. You don't want to keep reusing components after a component is encountered that can't be reused. If you do, then components in the same batch or later as the component that couldn't be reused will be in the buildroot, which is not desirable.

		`- component_pairs.append((c, component_to_reuse))`
		`+ if component_to_reuse:`
		`+ component_pairs.append((c, component_to_reuse))`

		`# Stores components we will tag to buildroot and final tag.`
		`components_to_tag = []`
		`@@ -193,7 +195,7 @@`
		`builder.buildroot_add_artifacts(components_to_tag, install=False)`
		`builder.tag_artifacts(components_to_tag, dest_tag=True)`

		`- return True`
		`+ return all_components_reusable`


		`def get_reusable_components(db_session, module, component_names, previous_module_build=None):`

tests/test_build/test_build.py

file modified

+192

		`@@ -1059,6 +1059,198 @@`
		`@pytest.mark.usefixtures("reuse_component_init_data")`
		`@patch("module_build_service.auth.get_user", return_value=user)`
		`@patch("module_build_service.scm.SCM")`
		`+ def test_submit_build_run_new_repo_once(`
		`+ self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc, db_session`
		`+ ):`
		`+ """`
		`+ Tests how many times the newRepo is run with change-and-after strategy when we`
		`+ have to rebuild only the 3rd batch and reuse the second`
		`+ """`
		`+`
		`+ # make the component in the last batch un-reusable`
		`+ db_session.query(models.ComponentBuild).filter_by(`
		`+ id=7).update({"ref": "1fbed359411a1baa08d4a88e0d12d426fbf8f602c"})`
		`+ db_session.commit()`
		`+`
		`+ # need to fake a running build task for the module`
		`+ def on_get_task_info_cb(cls, task_id):`
		`+ # new repo task_id`
		`+ if int(task_id) == 123:`
		`+ return {"state": koji.TASK_STATES["FREE"]}`
		`+ else:`
		`+ return {"state": koji.TASK_STATES["CLOSED"]}`
		`+`
		`+ FakeModuleBuilder.on_get_task_info_cb = on_get_task_info_cb`
		`+`
		`+ # we need to fake the message on _sent_repo_done, so we can measure how`
		`+ # many times was newRepo called`
		`+ mock_koji_repo_change = Mock()`
		`+ mock_msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message",`
		`+ repo_tag="module-testmodule-master-20170219191323-c40c156c-build")`
		`+`
		`+ mock_koji_repo_change.return_value = mock_msg`
		`+`
		`+ @patch("module_build_service.messaging.KojiRepoChange", mock_koji_repo_change)`
		`+ def _send_repo_done(self):`
		`+ msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message", repo_tag=self.tag_name + "-build")`
		`+ module_build_service.scheduler.consumer.work_queue_put(msg)`
		`+`
		`+ FakeModuleBuilder._send_repo_done = _send_repo_done`
		`+`
		`+ with models.make_db_session(conf) as scheduler_db_session:`
		`+ self.run_scheduler(scheduler_db_session, msgs=[MBSModule("local module build", 3, 1)])`
		`+`
		`+ reused_component_ids = {`
		`+ "module-build-macros": None,`
		`+ "perl-Tangerine": 1,`
		`+ "perl-List-Compare": 2,`
		`+ }`
		`+`
		`+ assert mock_koji_repo_change.call_count == 1`
		`+`
		`+ for build in models.ModuleBuild.get_by_id(db_session, 3).component_builds:`
		`+ assert build.state == koji.BUILD_STATES["COMPLETE"]`
		`+ assert build.module_build.state in [`
		`+ models.BUILD_STATES["done"],`
		`+ models.BUILD_STATES["ready"],`
		`+ ]`
		`+ if build.package in ["tangerine", "module-build-macros"]:`
		`+ assert not build.reused_component_id`
		`+ else:`
		`+ assert build.reused_component_id == reused_component_ids[build.package]`
		`+`
		`+ @pytest.mark.usefixtures("reuse_component_init_data")`
		`+ @patch("module_build_service.auth.get_user", return_value=user)`
		`+ @patch("module_build_service.scm.SCM")`
		`+ def test_submit_build_run_new_repo_three_times(`
		`+ self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc, db_session`
		`+ ):`
		`+ """`
		`+ Tests how many times the newRepo is run with change-and-after strategy when we`
		`+ have to rebuild one component from the 2nd batch and reuse the 3rd.`
		`+ """`
		`+ db_session.query(models.ComponentBuild).filter_by(`
		`+ id=5).update({"ref": "1fbed359411a1baa08d4a88e0d12d426fbf8f602c"})`
		`+ db_session.commit()`
		`+`
		`+ def on_get_task_info_cb(cls, task_id):`
		`+ if cls.module_str.batch == 1:`
		`+ return {"state": koji.TASK_STATES["FREE"]}`
		`+ elif cls.module_str.batch == 2:`
		`+ return {"state": koji.TASK_STATES["CLOSED"]}`
		`+ else:`
		`+ return {"state": koji.TASK_STATES["FREE"]}`
		`+`
		`+ FakeModuleBuilder.on_get_task_info_cb = on_get_task_info_cb`
		`+`
		`+ # we need to fake the message on _sent_repo_done, so we can measure how`
		`+ # many times was newRepo called`
		`+ mock_koji_repo_change = Mock()`
		`+ mock_msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message",`
		`+ repo_tag="module-testmodule-master-20170219191323-c40c156c-build")`
		`+`
		`+ mock_koji_repo_change.return_value = mock_msg`
		`+`
		`+ @patch("module_build_service.messaging.KojiRepoChange", mock_koji_repo_change)`
		`+ def _send_repo_done(self):`
		`+ msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message", repo_tag=self.tag_name + "-build")`
		`+ module_build_service.scheduler.consumer.work_queue_put(msg)`
		`+`
		`+ FakeModuleBuilder._send_repo_done = _send_repo_done`
		`+`
		`+ with models.make_db_session(conf) as scheduler_db_session:`
		`+ self.run_scheduler(scheduler_db_session, msgs=[MBSModule("local module build", 3, 1)])`
		`+`
		`+ reused_component_ids = {`
		`+ "module-build-macros": None,`
		`+ "perl-Tangerine": 1,`
		`+ "perl-List-Compare": 2,`
		`+ "tangerine": 3,`
		`+ }`
		`+`
		`+ assert mock_koji_repo_change.call_count == 3`
		`+`
		`+ for build in models.ModuleBuild.get_by_id(db_session, 3).component_builds:`
		`+ assert build.state == koji.BUILD_STATES["COMPLETE"]`
		`+ assert build.module_build.state in [`
		`+ models.BUILD_STATES["done"],`
		`+ models.BUILD_STATES["ready"],`
		`+ ]`
		`+ if build.package in ["module-build-macros", "perl-Tangerine", "tangerine"]:`
		`+ assert not build.reused_component_id`
		`+ else:`
		`+ assert build.reused_component_id == reused_component_ids[build.package]`
		`+`
		`+ @pytest.mark.usefixtures("reuse_component_init_data")`
		`+ @patch("module_build_service.auth.get_user", return_value=user)`
		`+ @patch("module_build_service.scm.SCM")`
		`+ def test_submit_build_run_new_repo_two_times(`
		`+ self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc, db_session`
		`+ ):`
		`+ """`
		`+ Tests how many times the newRepo is run with only-changed strategy when we`
		`+ have to rebuild one component from the 2nd batch and reuse the 3rd.`
		`+ """`
		`+ db_session.query(models.ModuleBuild).filter_by(id=3).update(`
		`+ {"rebuild_strategy": "only-changed"}`
		`+ )`
		`+ db_session.query(models.ComponentBuild).filter_by(`
		`+ id=5).update({"ref": "1fbed359411a1baa08d4a88e0d12d426fbf8f602c"})`
		`+ db_session.commit()`
		`+`
		`+ def on_get_task_info_cb(cls, task_id):`
		`+ if cls.module_str.batch in [1, 2]:`
		`+ return {"state": koji.TASK_STATES["FREE"]}`
		`+ else:`
		`+ return {"state": koji.TASK_STATES["CLOSED"]}`
		`+`
		`+ FakeModuleBuilder.on_get_task_info_cb = on_get_task_info_cb`
		`+`
		`+ # we need to fake the message on _sent_repo_done, so we can measure how`
		`+ # many times was newRepo called`
		`+ mock_koji_repo_change = Mock()`
		`+ mock_msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message",`
		`+ repo_tag="module-testmodule-master-20170219191323-c40c156c-build")`
		`+`
		`+ mock_koji_repo_change.return_value = mock_msg`
		`+`
		`+ @patch("module_build_service.messaging.KojiRepoChange", mock_koji_repo_change)`
		`+ def _send_repo_done(self):`
		`+ msg = module_build_service.messaging.KojiRepoChange(`
		`+ msg_id="a faked internal message", repo_tag=self.tag_name + "-build")`
		`+ module_build_service.scheduler.consumer.work_queue_put(msg)`
		`+`
		`+ FakeModuleBuilder._send_repo_done = _send_repo_done`
		`+`
		`+ with models.make_db_session(conf) as scheduler_db_session:`
		`+ self.run_scheduler(scheduler_db_session, msgs=[MBSModule("local module build", 3, 1)])`
		`+`
		`+ reused_component_ids = {`
		`+ "perl-List-Compare": 2,`
		`+ "tangerine": 3,`
		`+ }`
		`+`
		`+ assert mock_koji_repo_change.call_count == 1`
		`+`
		`+ for build in models.ModuleBuild.get_by_id(db_session, 3).component_builds:`
		`+ assert build.state == koji.BUILD_STATES["COMPLETE"]`
		`+ assert build.module_build.state in [`
		`+ models.BUILD_STATES["done"],`
		`+ models.BUILD_STATES["ready"],`
		`+ ]`
		`+ if build.package in ["module-build-macros", "perl-Tangerine"]:`
		`+ assert not build.reused_component_id`
		`+ else:`
		`+ assert build.reused_component_id == reused_component_ids[build.package]`
		`+`
		`+ @pytest.mark.usefixtures("reuse_component_init_data")`
		`+ @patch("module_build_service.auth.get_user", return_value=user)`
		`+ @patch("module_build_service.scm.SCM")`
		`def test_submit_build_reuse_all_without_build_macros(`
		`self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc, db_session`
		`):`

tests/test_utils/test_utils.py

file modified

+27

		`@@ -290,6 +290,33 @@`
		`assert username_arg == username`
		`rmtree(module_dir)`

		`+ def test_reuse_some_components(self, db_session):`
		`+ """`
		`+ Test, if not all components are reused and if the reusable components get tagged`
		`+ """`
		`+`
		`+ mock_builder = mock.Mock()`
		`+ module = db_session.query(models.ModuleBuild)\`
		`+ .filter_by(name="testmodule")\`
		`+ .order_by(models.ModuleBuild.id.desc())\`
		`+ .first()`
		`+ db_session.query(models.ComponentBuild).filter_by(`
		`+ id=module.component_builds[2].id).update({"ref": "fake_changed_ref"})`
		`+ db_session.commit()`
		`+ result = module_build_service.utils.attempt_to_reuse_all_components(`
		`+ mock_builder, db_session, module)`
		`+`
		`+ buildroot_args = mock_builder.buildroot_add_artifacts.call_args[0][0]`
		`+ tag_artifacts_args = mock_builder.tag_artifacts.call_args[0][0]`
		`+`
		`+ assert len(buildroot_args) == 2`
		`+ assert len(tag_artifacts_args) == 2`
		`+`
		`+ assert module.component_builds[2].nvr not in buildroot_args`
		`+ assert module.component_builds[2].nvr not in tag_artifacts_args`
		`+`
		`+ assert not result`
		`+`

		`class TestUtils:`
		`def setup_method(self, test_method):`

mcurlej commented 4 years ago

The old behaviour called newRepo on every batch. Now it will be called only on batches
which have something to rebuild. Batches which will reuse all its components will not
trigger newRepo.

Ticket-ID: FACTORY-3346

Signed-off-by: Martin Curlej mcurlej@redhat.com

mcurlej commented 4 years ago

@jkaluza @mprahl pls can you take a look?

mikeb commented 4 years ago

Build #378 failed (commit: 6001748).
Rebase or make new commits to rebuild.

mcurlej commented 4 years ago

@jkaluza @mprahl it seems the tests/test_build/test_build.py::TestBuild::test_submit_build_resume test is hanging. The funny bit is when i run only the test it will pass. Any ideas?

lucarval commented on line 8 of module_build_service/utils/reuse.py 4 years ago

Don't we need to iterate by batch? The bit later on that advances the module.batch to the highest reusable component.batch would have unintended consequences otherwise.

cqi commented 4 years ago

it seems the tests/test_build/test_build.py::TestBuild::test_submit_build_resume test is hanging. The funny bit is when i run only the test it will pass. Any ideas?

Not sure if it is helpful, but you could try to run test case TestBuild and output fedmsg-hub logs and see if there is something wrong.

cqi commented on line 16 of module_build_service/utils/reuse.py 4 years ago

Does this change the original function behavior? The original behavior is once no reusable component is found, function returns False immediately. With this change, function will continue going through the module.component_builds.

mprahl commented on line 16 of module_build_service/utils/reuse.py 4 years ago

@cqi is correct. You don't want to keep reusing components after a component is encountered that can't be reused. If you do, then components in the same batch or later as the component that couldn't be reused will be in the buildroot, which is not desirable.

mprahl commented 4 years ago

I'm going to close this due to inactivity.