PR#982: Fix getting the name-version of filtered RPMs. - fm-orchestrator

fm-orchestrator

#982 Fix getting the name-version of filtered RPMs.

Closed 5 years ago by mprahl. Opened 5 years ago by jkaluza.

jkaluza/fm-orchestrator filters into master

Fix filters generation

Jan Kaluza • 5 years ago

62a793a

module_build_service/builder/KojiModuleBuilder.py

file modified

+11

		`@@ -1070,6 +1070,17 @@`

		`return weights`

		`+ @classmethod`
		`+ def get_built_rpms_in_module_build(cls, build):`
		`+ """`
		`+ :param ModuleBuild build: Module build to get the built RPMs from.`
		`+ :return: list of NVRs`
		`+ """`
		`+ koji_session = KojiModuleBuilder.get_session(conf, None)`
		`+ rpms = koji_session.listTaggedRPMS(build.koji_tag, latest=True)[0]`
		`+ nvrs = set(kobo.rpmlib.make_nvr(rpm, force_epoch=True) for rpm in rpms)`
		`+ return list(nvrs)`
		`+`
		`def finalize(self):`
		`# Only import to koji CG if the module is "done".`
		`if self.config.koji_enable_content_generator and self.module.state == 3:`

module_build_service/builder/base.py

file modified

		`@@ -337,6 +337,14 @@`
		`raise NotImplementedError()`

		`@classmethod`
		`+ def get_built_rpms_in_module_build(cls, build):`
		`+ """`
		`+ :param ModuleBuild build: Module build to get the built RPMs from.`
		`+ :return: list of NVRs`
		`+ """`
		`+ raise NotImplementedError()`
		`+`
		`+ @classmethod`
		`def recover_orphaned_artifact(cls, component_build):`
		`"""`
		`Searches for a complete build of an artifact belonging to the module and sets the`

module_build_service/resolver/DBResolver.py

file modified

+10 -3

		`@@ -20,6 +20,9 @@`
		`# SOFTWARE.`
		`#`
		`# Written by Matt Prahl <mprahl@redhat.com>`
		`+ # Jan Kaluza <jkaluza@redhat.com>`
		`+`
		`+ import kobo.rpmlib`

		`from module_build_service import log`
		`from module_build_service.resolver.base import GenericResolver`
		`@@ -177,6 +180,7 @@`
		`:param requires: a dictionary with the module name as the key and the stream as the value`
		`:return: a dictionary`
		`"""`
		`+ from module_build_service.builder import GenericBuilder`
		`new_requires = {}`
		`with models.make_session(self.config) as session:`
		`for nsvc in requires:`
		`@@ -236,9 +240,12 @@`
		`# Find out the particular NVR of filtered packages`
		`rpm_filter = mmd.get_rpm_filter()`
		`if rpm_filter and rpm_filter.get():`
		`- for rpm in build.component_builds:`
		`- if rpm.package in rpm_filter.get():`
		`- filtered_rpms.append(rpm.nvr)`
		`+ rpm_filter = rpm_filter.get()`
		`+ built_nvrs = GenericBuilder.get_built_rpms_in_module_build(build)`
		`+ for nvr in built_nvrs:`
		`+ parsed_nvr = kobo.rpmlib.parse_nvr(nvr)`
		`+ if parsed_nvr["name"] in rpm_filter:`
		`+ filtered_rpms.append(nvr)`

		`new_requires[module_name] = {`
		`'ref': commit_hash,`

tests/test_builder/test_koji.py

file modified

+20

		`@@ -510,3 +510,23 @@`
		`else:`
		`expected_calls = []`
		`assert session.packageListBlock.mock_calls == expected_calls`
		`+`
		`+ @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')`
		`+ def test_get_built_rpms_in_module_build(self, get_session):`
		`+ session = MagicMock()`
		`+ session.listTaggedRPMS.return_value = ([`
		`+ {'build_id': 735939, 'name': 'tar', 'extra': None, 'arch': 'ppc64le',`
		`+ 'buildtime': 1533299221, 'id': 6021394, 'epoch': 2, 'version': '1.30',`
		`+ 'metadata_only': False, 'release': '4.el8+1308+551bfa71',`
		`+ 'buildroot_id': 4321122, 'payloadhash': '0621ab2091256d21c47dcac868e7fc2a',`
		`+ 'size': 878684},`
		`+ {'build_id': 735939, 'name': 'bar', 'extra': None, 'arch': 'ppc64le',`
		`+ 'buildtime': 1533299221, 'id': 6021394, 'epoch': 2, 'version': '1.30',`
		`+ 'metadata_only': False, 'release': '4.el8+1308+551bfa71',`
		`+ 'buildroot_id': 4321122, 'payloadhash': '0621ab2091256d21c47dcac868e7fc2a',`
		`+ 'size': 878684}], [])`
		`+ get_session.return_value = session`
		`+`
		`+ ret = KojiModuleBuilder.get_built_rpms_in_module_build(self.module)`
		`+ assert set(ret) == set(`
		`+ ['bar-2:1.30-4.el8+1308+551bfa71', 'tar-2:1.30-4.el8+1308+551bfa71'])`

tests/test_resolver/test_db.py

file modified

+31

		`@@ -120,6 +120,37 @@`
		`]`
		`assert set(result) == set(expected)`

		`+ @patch("module_build_service.builder.base.GenericBuilder.get_built_rpms_in_module_build")`
		`+ def test_resolve_requires(self, built_rpms):`
		`+ build = models.ModuleBuild.query.get(2)`
		`+ mmd = build.mmd()`
		`+ filter_list = Modulemd.SimpleSet()`
		`+ filter_list.add("foo")`
		`+ filter_list.add("bar")`
		`+ mmd.set_rpm_filter(filter_list)`
		`+ build.modulemd = mmd.dumps()`
		`+ db.session.commit()`
		`+`
		`+ built_rpms.return_value = [`
		`+ "foo-0:2.4.48-3.el8+1308+551bfa71",`
		`+ "foo-debuginfo-0:2.4.48-3.el8+1308+551bfa71",`
		`+ "bar-0:2.5.48-3.el8+1308+551bfa71",`
		`+ "bar-debuginfo-0:2.5.48-3.el8+1308+551bfa71",`
		`+ "x-0:2.5.48-3.el8+1308+551bfa71",`
		`+ "x-debuginfo-0:2.5.48-3.el8+1308+551bfa71"]`
		`+`
		`+ resolver = mbs_resolver.GenericResolver.create(tests.conf, backend='db')`
		`+ result = resolver.resolve_requires([":".join([`
		`+ build.name, build.stream, build.version, build.context])])`
		`+`
		`+ assert result == {`
		`+ 'testmodule': {`
		`+ 'stream': 'master', 'version': '20170109091357', 'context': u'78e4a6fd',`
		`+ 'ref': 'ff1ea79fc952143efeed1851aa0aa006559239ba',`
		`+ 'filtered_rpms': [`
		`+ 'foo-0:2.4.48-3.el8+1308+551bfa71',`
		`+ 'bar-0:2.5.48-3.el8+1308+551bfa71']}}`
		`+`
		`def test_resolve_profiles(self):`
		`"""`
		`Tests that the profiles get resolved recursively`

jkaluza commented 5 years ago

Previous code expected the MMD RPM filters to actually be component filters - that means the previous code expected component names like "httpd" there, but in fact, the MMD filter is RPM based, so there are RPM names of particular RPMs built from the component - like for example "httpd-debuginfo", "mod_ssl", "httpd-filesystem", ...

Because of that, the "Conflicts" section of module-build-macros did not contain all RPMs - it only contained those with same name as SRPM name from which they were built.

This PR fixes this by listing all the RPMs tagged in Koji tag of each (build-)required module and checking if the package is filtered by MMD filter section. If so, it stores it into the filtered_rpms as we did previously and it later gets added to Conflicts section of module-build-service.

mprahl commented on line 29 of module_build_service/resolver/DBResolver.py 5 years ago

Optional: You could assign rpm_filter.get() to a variable to avoid calling it on every iteration of the loop.

mprahl commented on line 12 of module_build_service/builder/KojiModuleBuilder.py 5 years ago

listTaggedRPMs should be listTaggedRPMS (capital S at the end)

mprahl commented on line 12 of module_build_service/builder/KojiModuleBuilder.py 5 years ago

Optional: Since builds isn't used, you could avoid storing it in memory with something like:

rpms = koji_session.listTaggedRPMs(build.koji_tag, latest=True)[0]

mprahl commented on line 13 of module_build_service/builder/KojiModuleBuilder.py 5 years ago

Optional: Since you parse the NVR in code that uses this method, perhaps you could store the parsed NVR as a list of dictionaries where each dict would be:

{
    'name': name,
    'version': version,
    'release': release,
    'epoch': epoch,
    'nvr': nvr
}

Then you wouldn't have to call kobo it code that uses method.

mprahl commented on line 7 of module_build_service/builder/base.py 5 years ago

nsvc => build

mprahl commented on line 9 of tests/test_builder/test_koji.py 5 years ago

As mentioned previously, listTaggedRPMs should be listTaggedRPMS.

mprahl commented 5 years ago

@jkaluza looks good. I left one mandatory comment to address and the rest are optional. Jenkins tests are broken and are fixed as part of PR#981 so as long as the tests pass locally, the tests should be fine.

mprahl commented 5 years ago

@jkaluza could you please rebase this so that the Jenkins tests run?

rebased onto d4b8cd5bf51c4cd215a866a8a69f85f4bc99af08

5 years ago

mprahl commented 5 years ago

@jkaluza could you please rebase this again?

rebased onto 7b8e833c036971d05aef0e2dc3c791fc5b32ee68

5 years ago

rebased onto 4aa7c96ecc49332876ebd378ac63fed9ae18d68f

5 years ago

rebased onto 1e778e523f5fe19fe97de95c723e0acc8d52031b

5 years ago