Resolve rpm (rich) dependencies via libsolv #1122

dparalen · 2018-06-19T15:18:52Z

Please note this requires a libsolv version exposing Python bindings necessary to support fileprovides deps. @ignatenkobrain made an F26 build with which I was able to test this PR manually.
Without it will just blow-up O:-)

pep8speaks · 2018-06-19T15:18:54Z

Hello @dparalen! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 16, 2018 at 16:39 Hours UTC

pulpbot · 2018-06-19T15:47:22Z

Can one of the admins verify this patch?

pulpbot · 2018-06-19T15:47:44Z

Can one of the admins verify this patch?

ignatenkobrain · 2018-06-21T08:15:36Z

I'm looking for this.

ignatenkobrain · 2018-06-21T08:16:46Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+            elif unit_flags == 'GE':
+                rel_flags = solv.REL_EQ | solv.REL_GT
+            else:
+                # fancier flags; might not be needed actually


yeah, this should be an error

ack; will raise something here instead

ignatenkobrain · 2018-06-21T08:17:24Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+    pool = solvable.repo.pool
+    if unit_name.startswith('('):
+        # the Rich/Boolean dependencies have just the 'name' attribute
+        # this is always in the form: '(foo >= 1.2 AND bar != 0.9)'


(foo >= 1.2 with foo < 2.0) should be better example

right; will update

ignatenkobrain · 2018-06-21T08:19:29Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+    def find_dependent_rpms(self, units):
+        solver = self.pool.Solver()
+        if self.target_repo:
+            # to avoid dependency conflicts between units being copied into a repository


meh, sounds hacky

so I guess since we don't really track these in the Mongo I can drop it actually

dparalen · 2018-06-22T18:16:01Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+        '_content_type_id',
+        'id',
+    ])
+    rpm_exclude_fields = set([


the thing is, RPM.objects.only('provides').as_pymongo() doesn't expand the list of provides objects. I couldn't get it working in any other way than RPM.objects.exclude(...).as_pymongo() :-/

dparalen · 2018-06-25T19:06:13Z

we might have to consider implementing the libsolv solver as a (default) alternative to the current solver; this is what I've encountered testing on Fedora28 content:

[milan@pulp2F28 pulp_rpm]$ pulp-admin rpm repo copy group -f f_everything -t foo --str-eq='id=gnome-desktop' --recursive
This command may be exited via ctrl+c without affecting the request.


[|]
Running...

Task Failed

Encountered problems solving: nothing provides /etc/xdg/autostart needed by
xdg-user-dirs-0:0.17-1.fc28.x86_64

[milan@pulp2F28 pulp_rpm]$

i.e there might be issues in the users repo content that could be considered repo copy regressions by them if we only provide the libsolv solver :-/
Another option would be to enforce the action no matter missing dependencies but I don't yet know how to do that.

--->%---EDITED---

Turns out I'm not processing directories in file-provides... but the concern remains about a repo being not properly "closed" for dependencies, rendering the recursive copies unsatisfiable. A user might consider this a regression with the libsolv solver.

ignatenkobrain · 2018-06-26T16:26:22Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+    repodata = solvable.repo.first_repodata()
+    unit_dirs = unit.get('files', {}).get('dir', [])
+    for dirname in unit_dirs:
+        repodata.str2dir(dirname.encode('utf-8'), create=True)


here you need just /, and then use add_dirstr() with the directory.

jlsherrill · 2018-06-26T17:51:02Z

@dparalen i think it will actually be fairly 'normal' for repos to not completely solve. Things like epel, optional, or extras do not provide all their dependencies, they depend on some base os repo which would not be part of the solver. I think we still have to copy what we know about in those instances.

dparalen · 2018-06-27T07:59:51Z

@jlsherrill yeah, I thought so. My implementation is more strict right now but libsolv can be tricked into the behaviour of the "old" solver if the hard requirements are treated as weak e.g requires -> recommends or probably by mocking the unsatisfied dependencies and retrying the solving.

These tricks will affect the set of the reported dependent packages. Once we implement weak dependency processing in Pulp, pulling-in all the recommended dependencies all the time because of the requires->recommends shift will result in a different set of packages being resolved than the strict requires alone would have pulled.

My gut feeling is though to expose these behaviour options as settings thru the REST API to the user to chose their preferred solver behaviour for a particular (repository) case. Folks might as well be interested in seeing what particular content would have been mocked to satisfy the dependencies for some sort of content sanity checking. But it may be out of scope for this PR.

What do you think?

Thanks,
milan

dparalen · 2018-06-27T08:20:34Z

I'll investigate how to just ignore the problems the solver reported...

dparalen · 2018-06-27T10:09:49Z

So it seems it's fine to just "ignore" the problems ;)

dparalen · 2018-06-28T12:43:27Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+    """A specific, rpm-unit-type filelist attribute conversion."""
+    repodata = solvable.repo.first_repodata()
+    unit_files = unit.get('files', {}).get('file', [])
+    unit_files.extend(unit.get('files', {}).get('dir', []))


both the rpm.files['file'] and rpm.files['dir'] have to be processed the same os.path.dirname(); os.path.basename() way for libsolv to understand.

dkliban

This looks good to me.

However, I don't think we can merge this until the required changes for libsolv Python bindings have been released. If we merge this now, our nightly test runners will start failing until the newer libsolv bindings are available for EL7 and F28. We also will not be able to release this code until that dependency is available.

dralley · 2018-07-10T01:07:29Z

Jenkins tests are failing for no module named "solv"

dparalen · 2018-07-10T16:47:41Z

@dralley, thanks a lot for test!
I shall update the requirements, stay tuned!

The Pulp RPM plugin switches depsolving implementation to a libsolv-based one[1]. This patch adds a dependency on the python2 libsolv bindings that include necessary functionality, esp. the file-provides Python libsolv API. [1] pulp/pulp_rpm#1122

dparalen · 2018-07-11T10:32:31Z

OK test

ipanova · 2018-07-11T10:33:02Z

ok test

dparalen · 2018-07-11T15:01:12Z

ok test

daviddavis · 2018-07-12T15:22:07Z

ok test

dparalen · 2018-07-13T12:50:32Z

plugins/pulp_rpm/plugins/importers/yum/pulp_solv.py

+    plain_attribute_factory('arch'),
+    plain_attribute_factory('vendor'),
+    rpm_dependency_attribute_factory('requires'),
+    rpm_dependency_attribute_factory('conflicts'),


I shall remove the unused fields for now; if in the future required, we'd better add on an as-needed basis.

How come that conflicts do not exist?!

we don't track these in Mongo so it has no effect; Can a repo have conflicting units inside? I guess it can, or?

dkliban

Thank you for putting this together.

I tested using pulp-smash tests for recursively copying units between repos.

dkliban · 2018-07-13T14:27:59Z

Please add a release note stating that we added libsolv as a dependency and that it's now being used when RPMs are being copied recursively between repos.

dparalen · 2018-07-13T15:56:29Z

OK, so I had a private chat with @ignatenkobrain and turns out we have to process the "negative" dependencies too:

foo depends on webserver but conflicts with libnginx
nginx provides webserver but also libnginx
the source repo contains both the nginx and httpd
recursive copy of foo might pick up nginx since Pulp ignores the conflicts field
dnf installing from the target repo would panic

This unfortunately is the case with the current code so I hope I'm not making it worse.
I'd say we should correct this sooner than later tho.

Thanks @ignatenkobrain for pointing it out

ignatenkobrain · 2018-07-13T15:59:46Z

nginx provides webserver but also libnginx

Missing Requires libnginx here ;)

And also you missed some input that httpd also Provides: webserver

dkliban · 2018-07-13T16:02:14Z

We should file an issue in pulp.plan.io about this. This is not a regression.

dparalen · 2018-07-13T16:31:42Z

@dkliban, I've just created https://pulp.plan.io/issues/3853

ipanova · 2018-07-16T12:41:47Z

docs/user-guide/release-notes/2.17.x.rst

+* The rich/boolean dependency solving is now supported when e.g recursively
+  copying RPM content between repositories.
+
+* More conservative dependency solving is now supported when e.g recursively


can you explain 'conservative dependency solving'?

It's a literal copy from the related issue headline; would you like me rephrasing it in the release note?

ipanova · 2018-07-16T12:55:57Z

plugins/pulp_rpm/plugins/importers/yum/associate.py

+        solver = pulp_solv.Solver(source_repo, target_repo=dest_repo)
+        solver.load()
+    else:
+        solver = None


what about getting rid of unnecessary if statement on L#52 and:

if recursive: solver = pulp_solv.Solver(source_repo, target_repo=dest_repo) solver.load() else: solver = None recursive = False

in my understanding this option is a boolean which is whether true or false.
in addition i noticed that this option is not documented in technical reference at all https://docs.pulpproject.org/plugins/pulp_rpm/tech-reference/yum-plugins.html#yum-importer

i'd also set a default value to False:

# get config items that we care about recursive = config.get(constants.CONFIG_RECURSIVE, False)

lemme update

if you could also update the tech. reference docs would be great

Sure I'll check it

ipanova · 2018-07-16T14:13:56Z

plugins/pulp_rpm/plugins/importers/yum/associate.py


+                            # FIXME: not really true anymore:


could you just remove the fixme since it's a not a problem and just leaving the note of silent skip of unresolvable deps.

ipanova · 2018-07-16T14:18:02Z

plugins/pulp_rpm/plugins/importers/yum/associate.py

-        repo_controller.associate_single_unit(repository=dest_repo, unit=unit)
-        unit_set.add(unit)
+        _LOGGER.debug('Copying unit: %s', unit)
+        # FIXME: this should be done in bulks


what's the status of this fixme?

will call it TODO:

dparalen · 2018-07-16T14:20:24Z

@ipanova thanks for the review!

Pulp now uses libsolv as a dependency solver to address recursive RPM unit dependency solving during e.g unit copy Fixes: #3715 https://pulp.plan.io/issues/3715 Fixes: pulp#2478 https://pulp.plan.io/issues/2478

dparalen · 2018-07-16T16:42:33Z

updated; hopefully I didn't miss anything

ipanova · 2018-07-17T11:54:19Z

ok test

dparalen force-pushed the issue_3715_steel branch from 28aa4c2 to 0bf8349 Compare June 19, 2018 15:25

dparalen force-pushed the issue_3715_steel branch from 0bf8349 to 7da685f Compare June 20, 2018 10:37

ignatenkobrain reviewed Jun 21, 2018

View reviewed changes

dparalen force-pushed the issue_3715_steel branch 3 times, most recently from a8657e8 to c8bd2d4 Compare June 22, 2018 17:59

dparalen commented Jun 22, 2018

View reviewed changes

dparalen force-pushed the issue_3715_steel branch 3 times, most recently from 45ad7f7 to e7b058c Compare June 25, 2018 14:12

dparalen force-pushed the issue_3715_steel branch from e7b058c to a00f62b Compare June 26, 2018 15:55

ignatenkobrain reviewed Jun 26, 2018

View reviewed changes

dparalen force-pushed the issue_3715_steel branch from a00f62b to 87750d6 Compare June 26, 2018 17:19

dparalen force-pushed the issue_3715_steel branch from 87750d6 to bd14f71 Compare June 27, 2018 10:04

dparalen mentioned this pull request Jun 27, 2018

As a user, I can import RPM package which has rich dependencies in *Requires* pulp/pulp-smash#901

Closed

7 tasks

dparalen force-pushed the issue_3715_steel branch 2 times, most recently from 31e96f8 to 97c72ff Compare June 28, 2018 09:41

dparalen commented Jun 28, 2018

View reviewed changes

dkliban requested changes Jun 28, 2018

View reviewed changes

dparalen mentioned this pull request Jul 11, 2018

Python2-solv required for depsolving pulp/pulp-packaging#45

Merged

dparalen force-pushed the issue_3715_steel branch from 97c72ff to 960eab9 Compare July 12, 2018 13:22

dparalen commented Jul 13, 2018

View reviewed changes

dkliban approved these changes Jul 13, 2018

View reviewed changes

dparalen force-pushed the issue_3715_steel branch 2 times, most recently from b176364 to 0d718dd Compare July 13, 2018 15:38

ipanova reviewed Jul 16, 2018

View reviewed changes

dparalen force-pushed the issue_3715_steel branch from 0d718dd to 14c7338 Compare July 16, 2018 16:35

Resolve rpm (rich) dependencies via libsolv

f85007c

Pulp now uses libsolv as a dependency solver to address recursive RPM unit dependency solving during e.g unit copy Fixes: #3715 https://pulp.plan.io/issues/3715 Fixes: pulp#2478 https://pulp.plan.io/issues/2478

dparalen force-pushed the issue_3715_steel branch from 14c7338 to f85007c Compare July 16, 2018 16:39

ipanova approved these changes Jul 17, 2018

View reviewed changes

ipanova merged commit 17a2f5d into pulp:2-master Jul 17, 2018

Resolve rpm (rich) dependencies via libsolv #1122

Resolve rpm (rich) dependencies via libsolv #1122

Conversation

dparalen commented Jun 19, 2018 • edited

pep8speaks commented Jun 19, 2018 • edited

Comment last updated on July 16, 2018 at 16:39 Hours UTC

pulpbot commented Jun 19, 2018

pulpbot commented Jun 19, 2018

ignatenkobrain commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dparalen commented Jun 25, 2018 • edited

Choose a reason for hiding this comment

jlsherrill commented Jun 26, 2018

dparalen commented Jun 27, 2018

dparalen commented Jun 27, 2018

dparalen commented Jun 27, 2018

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

dralley commented Jul 10, 2018

dparalen commented Jul 10, 2018

dparalen commented Jul 11, 2018

ipanova commented Jul 11, 2018

dparalen commented Jul 11, 2018

daviddavis commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

dkliban commented Jul 13, 2018

dparalen commented Jul 13, 2018 • edited

ignatenkobrain commented Jul 13, 2018

dkliban commented Jul 13, 2018

dparalen commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dparalen commented Jul 16, 2018

dparalen commented Jul 16, 2018

ipanova commented Jul 17, 2018

dparalen commented Jun 19, 2018 •

edited

pep8speaks commented Jun 19, 2018 •

edited

dparalen commented Jun 25, 2018 •

edited

dparalen commented Jul 13, 2018 •

edited