PR#1755: Do Not Merge - Avoid libmodulemd symbol clash - fm-orchestrator

fm-orchestrator

#1755 Do Not Merge - Avoid libmodulemd symbol clash

Opened 2 years ago by breilly. Modified 2 years ago

breilly/fm-orchestrator dnfworkaround into master

Avoid libmodulemd symbol clash

Brendan Reilly • 2 years ago

e8d0874

module_build_service/builder/MockModuleBuilder.py

file modified

+8 -1

		`@@ -8,7 +8,6 @@`
		`import subprocess`
		`import threading`

		`- import dnf`
		`import koji`
		`import kobo.rpmlib`
		`import platform`
		`@@ -182,6 +181,10 @@`
		`"""`
		`Returns the $releasever variable used in the system when expanding .repo files.`
		`"""`
		`+ # import dnf in function to avoid symbol name clashing`
		`+ # see: https://pagure.io/releng/issue/10850`
		`+ import dnf`
		`+`
		`dnf_base = dnf.Base()`
		`return dnf_base.conf.releasever`

		`@@ -196,6 +199,10 @@`
		:param str platform_id: The `name:stream` of a fake platform module to generate in this
		`method. When not set, the /etc/os-release is parsed to get the PLATFORM_ID.`
		`"""`
		`+ # import dnf in function to avoid symbol name clashing`
		`+ # see: https://pagure.io/releng/issue/10850`
		`+ import dnf`
		`+`
		`log.info("Loading available RPM repositories.")`
		`dnf_base = dnf.Base()`
		`dnf_base.read_all_repos()`

module_build_service/scheduler/default_modules.py

file modified

+20 -99

		`@@ -5,8 +5,8 @@`
		`import os`
		`import shutil`
		`import tempfile`
		`+ import subprocess`

		`- import dnf`
		`import kobo.rpmlib`
		`import koji`
		`import packaging.version`
		`@@ -230,18 +230,20 @@`
		`:param bool force_for_old_dnf: add the conflicts even if libdnf can't handle them`
		`:raise RuntimeError: when a Koji query fails`
		`"""`
		`- if (not force_for_old_dnf`
		`- and packaging.version.parse(dnf.VERSION) < packaging.version.parse('4.2.19')):`
		`- # For local builds, we can't use this code unless libdnf uses libmodulemd2`
		`- # (done in libdnf-0.45) - we can't check the libdnf version, so use`
		`- # dnf-4.2.19 (which requires libdnf-0.45) as a proxy.`
		`- log.warning(`
		`- "The necessary conflicts could not be generated due to RHBZ#1693683. "`
		`- "Some RPMs from the base modules (%s) may end up being used over modular RPMs. "`
		`- "This may result in different behavior than a production build.",`
		`- ", ".join(conf.base_module_names)`
		`- )`
		`- return`
		`+ # Commenting out for dnf workaround`
		`+ # See: https://pagure.io/releng/issue/10850`
		`+ #if (not force_for_old_dnf`
		`+ # and packaging.version.parse(dnf.VERSION) < packaging.version.parse('4.2.19')):`
		`+ # # For local builds, we can't use this code unless libdnf uses libmodulemd2`
		`+ # # (done in libdnf-0.45) - we can't check the libdnf version, so use`
		`+ # # dnf-4.2.19 (which requires libdnf-0.45) as a proxy.`
		`+ # log.warning(`
		`+ # "The necessary conflicts could not be generated due to RHBZ#1693683. "`
		`+ # "Some RPMs from the base modules (%s) may end up being used over modular RPMs. "`
		`+ # "This may result in different behavior than a production build.",`
		`+ # ", ".join(conf.base_module_names)`
		`+ # )`
		`+ # return`

		`log.info("Finding any buildrequired modules that collide with the RPMs in the base modules")`
		`bm_tags = set()`
		`@@ -345,91 +347,10 @@`

		`return nevras`

		`-`
		`def _get_rpms_in_external_repo(repo_url, arches, cache_dir_name):`
		`- """`
		`- Get the available RPMs in the external repo for the provided arches.`
		`-`
		`- :param str repo_url: the URL of the external repo with the "$arch" variable included`
		`- :param list arches: the list of arches to query the external repo for`
		`- :param str cache_dir_name: the cache directory name under f"{conf.cache_dir}/dnf"`
		`- :return: a set of the RPM NEVRAs`
		`- :rtype: set`
		`- :raise RuntimeError: if the cache is not writeable or the external repo couldn't be loaded`
		`- :raises ValueError: if there is no "$arch" variable in repo URL`
		`- """`
		`- if "$arch" not in repo_url:`
		`- raise ValueError(`
		`- "The external repo {} does not contain the $arch variable".format(repo_url)`
		`- )`
		`-`
		`- base = dnf.Base()`
		`- try:`
		`- dnf_conf = base.conf`
		`- # Expire the metadata right away so that when a repo is loaded, it will always check to`
		`- # see if the external repo has been updated`
		`- dnf_conf.metadata_expire = 0`
		`+ # Calling an external script using subprocess to avoid importing dnf`
		`+ # See: https://pagure.io/releng/issue/10850`
		`+ nevras = subprocess.check_output(['mbs-get-rpms-in-external-repo', repo_url, cache_dir_name,`
		`+ conf.cache_dir, str(conf.dnf_timeout), str(conf.dnf_minrate)] + arches)`

		`- cache_location = os.path.join(conf.cache_dir, "dnf", cache_dir_name)`
		`- try:`
		`- # exist_ok=True can't be used in Python 2`
		`- os.makedirs(cache_location, mode=0o0770)`
		`- except OSError as e:`
		`- # Don't fail if the directories already exist`
		`- if e.errno != errno.EEXIST:`
		`- log.exception("Failed to create the cache directory %s", cache_location)`
		`- raise RuntimeError("The MBS cache is not writeable.")`
		`-`
		`- # Tell DNF to use the cache directory`
		`- dnf_conf.cachedir = cache_location`
		`- # Don't skip repos that can't be synchronized`
		`- dnf_conf.skip_if_unavailable = False`
		`- dnf_conf.timeout = conf.dnf_timeout`
		`- # Get rid of everything to be sure it's a blank slate. This doesn't delete the cached repo`
		`- # data.`
		`- base.reset(repos=True, goal=True, sack=True)`
		`-`
		`- # Add a separate repo for each architecture`
		`- for arch in arches:`
		`- # Convert arch to canon_arch. This handles cases where Koji "i686" arch is mapped to`
		`- # "i386" when generating RPM repository.`
		`- canon_arch = koji.canonArch(arch)`
		`- repo_name = "repo_{}".format(canon_arch)`
		`- repo_arch_url = repo_url.replace("$arch", canon_arch)`
		`- base.repos.add_new_repo(`
		`- repo_name, dnf_conf, baseurl=[repo_arch_url], minrate=conf.dnf_minrate,`
		`- )`
		`-`
		`- try:`
		`- # Load the repos in parallel`
		`- base.update_cache()`
		`- except dnf.exceptions.RepoError:`
		`- msg = "Failed to load the external repos"`
		`- log.exception(msg)`
		`- raise RuntimeError(msg)`
		`-`
		`- # dnf will not always raise an error on repo failures, so we check explicitly`
		`- for repo_name in base.repos:`
		`- if not base.repos[repo_name].metadata:`
		`- msg = "Failed to load metadata for repo %s" % repo_name`
		`- log.exception(msg)`
		`- raise RuntimeError(msg)`
		`-`
		`- base.fill_sack(load_system_repo=False)`
		`-`
		`- # Return all the available RPMs`
		`- nevras = set()`
		`- for rpm in base.sack.query().available():`
		`- rpm_dict = {`
		`- "arch": rpm.arch,`
		`- "epoch": rpm.epoch,`
		`- "name": rpm.name,`
		`- "release": rpm.release,`
		`- "version": rpm.version,`
		`- }`
		`- nevra = kobo.rpmlib.make_nvra(rpm_dict, force_epoch=True)`
		`- nevras.add(nevra)`
		`- finally:`
		`- base.close()`
		`-`
		`- return nevras`
		`+ return set(nevras.split())`

module_build_service_workarounds/__init__.py

file added

empty or binary file added

module_build_service_workarounds/externalrepo.py

file added

+107

		`@@ -0,0 +1,107 @@`
		`+ import os`
		`+ import sys`
		`+ assert "gi" not in sys.modules`
		`+ import errno`
		`+ import koji`
		`+ import kobo.rpmlib`
		`+ import dnf`
		`+`
		`+ # Moved _get_rpms_in_external_repo logic to a separate script to avoid symbol clash with dnf`
		`+ # See: https://pagure.io/releng/issue/10850`
		`+`
		`+ def main(argv=sys.argv):`
		`+ #def main(repo_url, cache_dir_name, cache_dir, dnf_timeout, dnf_minrate, arches):`
		`+ """`
		`+ Get the available RPMs in the external repo for the provided arches.`
		`+`
		`+ :param str repo_url: the URL of the external repo with the "$arch" variable included`
		`+ :param list arches: the list of arches to query the external repo for`
		`+ :param str cache_dir_name: the cache directory name under f"{conf.cache_dir}/dnf"`
		`+ :return: a set of the RPM NEVRAs`
		`+ :rtype: set`
		`+ :raise RuntimeError: if the cache is not writeable or the external repo couldn't be loaded`
		`+ :raises ValueError: if there is no "$arch" variable in repo URL`
		`+ """`
		`+`
		`+ repo_url = argv[1]`
		`+ cache_dir_name = argv[2]`
		`+ cache_dir = argv[3]`
		`+ dnf_timeout = argv[4]`
		`+ dnf_minrate = argv[5]`
		`+ arches = argv[6:]`
		`+`
		`+ if "$arch" not in repo_url:`
		`+ raise ValueError(`
		`+ "The external repo {} does not contain the $arch variable".format(repo_url)`
		`+ )`
		`+`
		`+ base = dnf.Base()`
		`+ try:`
		`+ dnf_conf = base.conf`
		`+ # Expire the metadata right away so that when a repo is loaded, it will always check to`
		`+ # see if the external repo has been updated`
		`+ dnf_conf.metadata_expire = 0`
		`+`
		`+ cache_location = os.path.join(cache_dir, "dnf", cache_dir_name)`
		`+ try:`
		`+ # exist_ok=True can't be used in Python 2`
		`+ os.makedirs(cache_location, mode=0o0770)`
		`+ except OSError as e:`
		`+ # Don't fail if the directories already exist`
		`+ if e.errno != errno.EEXIST:`
		`+ #log.exception("Failed to create the cache directory %s", cache_location)`
		`+ raise RuntimeError("The MBS cache is not writeable.")`
		`+`
		`+ # Tell DNF to use the cache directory`
		`+ dnf_conf.cachedir = cache_location`
		`+ # Don't skip repos that can't be synchronized`
		`+ dnf_conf.skip_if_unavailable = False`
		`+ dnf_conf.timeout = int(dnf_timeout)`
		`+ # Get rid of everything to be sure it's a blank slate. This doesn't delete the cached repo`
		`+ # data.`
		`+ base.reset(repos=True, goal=True, sack=True)`
		`+`
		`+ # Add a separate repo for each architecture`
		`+ for arch in arches:`
		`+ # Convert arch to canon_arch. This handles cases where Koji "i686" arch is mapped to`
		`+ # "i386" when generating RPM repository.`
		`+ canon_arch = koji.canonArch(arch)`
		`+ repo_name = "repo_{}".format(canon_arch)`
		`+ repo_arch_url = repo_url.replace("$arch", canon_arch)`
		`+ base.repos.add_new_repo(`
		`+ repo_name, dnf_conf, baseurl=[repo_arch_url], minrate=int(dnf_minrate),`
		`+ )`
		`+`
		`+ try:`
		`+ # Load the repos in parallel`
		`+ base.update_cache()`
		`+ except dnf.exceptions.RepoError:`
		`+ msg = "Failed to load the external repos"`
		`+ #log.exception(msg)`
		`+ raise RuntimeError(msg)`
		`+`
		`+ # dnf will not always raise an error on repo failures, so we check explicitly`
		`+ for repo_name in base.repos:`
		`+ if not base.repos[repo_name].metadata:`
		`+ msg = "Failed to load metadata for repo %s" % repo_name`
		`+ #log.exception(msg)`
		`+ raise RuntimeError(msg)`
		`+`
		`+ base.fill_sack(load_system_repo=False)`
		`+`
		`+ # Return all the available RPMs`
		`+ nevras = set()`
		`+ for rpm in base.sack.query().available():`
		`+ rpm_dict = {`
		`+ "arch": rpm.arch,`
		`+ "epoch": rpm.epoch,`
		`+ "name": rpm.name,`
		`+ "release": rpm.release,`
		`+ "version": rpm.version,`
		`+ }`
		`+ nevra = kobo.rpmlib.make_nvra(rpm_dict, force_epoch=True)`
		`+ nevras.add(nevra)`
		`+ finally:`
		`+ base.close()`
		`+`
		`+ print(" ".join(nevras))`

setup.py

file modified

		`@@ -42,6 +42,7 @@`
		`"mbs-upgradedb = module_build_service.manage:upgradedb",`
		`"mbs-frontend = module_build_service.manage:run",`
		`"mbs-manager = module_build_service.manage:manager_wrapper",`
		`+ "mbs-get-rpms-in-external-repo = module_build_service_workarounds.externalrepo:main"`
		`],`
		`"moksha.consumer": "mbsconsumer = module_build_service.scheduler.consumer:MBSConsumer",`
		`"mbs.messaging_backends": [`

breilly commented 2 years ago

Importing dnf in the MBS process causes a symbol clash on RHEL 7. This
is a temporary fix to avoid that, and should not be merged.

See https://pagure.io/releng/issue/10850

breilly commented 2 years ago

Filing this PR to track the patch we're making to our hosts, but once they're upgraded to RHEL 8/Fedora this should be dropped.

This change also breaks unit tests for external repos - the tests can't mock out the dnf return values as they're in a subprocess call.

otaylor commented on line 108 of module_build_service/externalrepo.py 2 years ago

Return doesn't look right here - I'd expect 'print(" ".join(nevras))' or something like that

otaylor commented on line 28 of module_build_service/scheduler/default_modules.py 2 years ago

If the workaround was made permanent (or when MBS is just made to hard require rhel8 or newer dnf), this and force_for_old_dnf could all drop out - this is precisely to avoid libmodulemd1 and libmodulemd2 in the same process as it turns out - https://bugzilla.redhat.com/show_bug.cgi?id=1693683

otaylor commented on line 139 of module_build_service/scheduler/default_modules.py 2 years ago

And this would need to strip/split the output

otaylor commented on line 5 of setup.py 2 years ago

I think module_build_service.init.py will get imported in this case - which is hopefully harmless, but it does drag in various other parts of MBS. Maybe add a assert "gi" not in sys.modules to externalrepo.py?

otaylor commented 2 years ago

It woudl be really nice if the unit tests for externalrepos worked - would give a lot more confidence in the change :-) .... don't really have a great idea for that - I guess you could put the DNF mocks directly in externalrepo.py and activate them with an environment variable.

1 new commit added

Move externalrepo script to new module

2 years ago

2 new commits added

Move externalrepo script to new module
Avoid libmodulemd symbol clash

2 years ago

breilly commented 2 years ago

Thanks for looking it over @otaylor!
I'd also like to get the unit tests working, I'll see what I can do about that.

I also had to move the new script to its own module, to avoid pulling in anything from module_build_service.

Edited 2 years ago by breilly

rebased onto e8d0874

2 years ago

breilly commented 2 years ago

Still unable to get unit tests working as intended, but I did get them running individually by doing the mocks in the externalrepo script directly each time - caught a couple of issues that way, but it should be working as intended now.