Bug 1656019

Created attachment 1511313 [details]
reproducer

attaching my reproducer - two repos with conflicting default stream

(In reply to Karel Srot from comment #1)
> Created attachment 1511313 [details]
> reproducer
> 
> attaching my reproducer - two repos with conflicting default stream

Can you please attach the contents of /etc/yum.repos.d as well?

# cat > /etc/yum.repos.d/test.repo <<EOF
[orig]
name=orig
baseurl=file:///tmp/orig
gpgcheck=0

[copy]
name=copy
baseurl=file:///tmp/copy
gpgcheck=0
EOF

I can reproduce this issue and I confirm it's a bug. I assume at the moment that it is a libmodulemd bug in the merge logic.

OK, the problem is *not* in libmodulemd after all. We're properly reporting an error here, which DNF turns into a C++ exception... which they later catch, log as a warning and then ignore.

The catch-and-ignore happens in ModulePackageContainer::add() and ModulePackageContainer::addDefaultsFromDisk().

This needs to be fixed at the libdnf layer, so reassigning.

Fixed by https://github.com/rpm-software-management/libdnf/commit/c3b9bc32b8b5d1ba2443f42648a6b7fcf2b5e825.

How to test?
Run reproducer provided by reporter.

How to test? Part 2.
A logger.warning should be present.

(In reply to Jaroslav Mracek from comment #8)
> How to test? Part 2.
> A logger.warning should be present.

Is a warning a fatal error? The effect here should be that the repodata is considered corrupt and therefore not usable.

Patches:
https://github.com/rpm-software-management/libdnf/pull/654
https://github.com/rpm-software-management/dnf/pull/1284


Now it will fail with all information provided by libmodulemd. The solution has to be confirmed by Modularity team that the behavior is intended (see patches above).

Risks:
1. Change in API (libdnf, hawkey, dnf):
The change produce new errors that could be not handled on side of API users
(anaconda, lorax, ...).

2. Different behavior that according to reporter will effect wide range of users

3. If conflict in defaults appears it will be impossible to upgrade, downgrade, install packages, or even consume security updates till conflict is resolved.

(In reply to Jaroslav Mracek from comment #14)
> 2. Different behavior that according to reporter will effect wide range of
> users

Just to add. I was pointing out that the issue may not be that uncommon. However, as  Stephen pointed out RH policy is not to change default, therefore the issue should not appear just by enabling and outdated copy or the repo. 
Moreover, even if there would be a conflict I believe it is better to error out rather than following _randomly_ (confirmed by testing that the default profile selected may differ per system) selected default profile.

Setting FutureFeature as this requires an API change.

We discussed this at the interlock meeting today and, considering the impact when using third party or misconfigured repos, decided to implement this fix in 8.0.0.

I did tested the behaviour with dnf-4.0.9.1-1.el8.noarch libdnf-0.22.4-1.el8.x86_64 and the output is quite confusing.

# dnf --disablerepo=\* --enablerepo=bz1577196\* module list
Last metadata expiration check: 0:06:18 ago on Thu 20 Dec 2018 08:59:37 AM EST.
Conflicting default streams when merging defaults for module ModuleX
bz1577196copy
Name                                Stream                                   Profiles                               Summary                                          
ModuleX                             myStreamB [d]                            default                                Module ModuleX summary                           

bz1577196orig
Name                                Stream                                   Profiles                               Summary                                          
ModuleX                             myStream                                 default [d]                            Module ModuleX summary                           

Hint: [d]efault, [e]nabled, [x]disabled, [i]nstalled

So, there is a conflict but still the default stream/profile is indicated. This is a bit strange.
Let's try to install ModuleX then.

# dnf --disablerepo=\* --enablerepo=bz1577196\* install ModuleX
Last metadata expiration check: 0:00:50 ago on Thu 20 Dec 2018 09:06:06 AM EST.
Conflicting default streams when merging defaults for module ModuleX
No match for argument: ModuleX
Error: Unable to find a match

OK, failed to install due to conflicting streams? Still, it was listed above as available, let's try again

# dnf --disablerepo=\* --enablerepo=bz1577196\* install ModuleX:myStream/default
Last metadata expiration check: 0:01:43 ago on Thu 20 Dec 2018 09:06:06 AM EST.
Conflicting default streams when merging defaults for module ModuleX
No match for argument: ModuleX:myStream/default
Error: Unable to find a match

This is also weird. There is a conflict in defaults but I did specify module:stream/profile so no defaults should apply.

The behavior above is confusing. Either the module should not be even listed... or even better, given the conflict is in defaults only defaults should be completely ignored, however the module should be still available and installable.

Stephen, Petr, what is your opinion?

(In reply to Karel Srot from comment #20)
> The behavior above is confusing. Either the module should not be even
> listed... or even better, given the conflict is in defaults only defaults
> should be completely ignored, however the module should be still available
> and installable.
> 
> Stephen, Petr, what is your opinion?

I'm of two minds here. On the one hand, if we've gotten into a situation where conflicting defaults are present, it does indicate a serious problem with the repodata. But maybe it's not absolutely necessary to completely fail processing. I suppose we *could* treat conflicting defaults as "no defaults exist for this module". Then at least users would be able to set their choices manually.

This change would have to occur in libmodulemd, as right now the library treats it as a fatal error. It will mean new API, since currently we can only return a complete, merged object OR an error message. Is this the route we want to take?

If so, I'll ask that the DNF folks please tell me what they want the API to look like and I'll get it written immediately.

Hello,
can we get a devel feedback on the problem above? I am fine with accepting current behaviour for 8.0, just want to double check whether we should file a new bug for the proposed change (do not use conflicting defaults).

(In reply to Karel Srot from comment #22)
> Hello,
> can we get a devel feedback on the problem above? I am fine with accepting
> current behaviour for 8.0, just want to double check whether we should file
> a new bug for the proposed change (do not use conflicting defaults).

Sorry, we forgot to report the decision back to this Bugzilla.

After careful consideration, we determined that there were no scenarios where it would be unsafe to treat a conflict of default streams as having "no default stream". So we have implemented a change in libmodulemd (see BZ#1666871) to do so. DNF did not require any code changes to address this.

The case of conflicts between the default profiles for a stream is, however, impossible to resolve automatically and should still result in DNF having a hard failure and reporting the conflict. We should probably use this ticket to track testing that this behavior is honored.

I would rather propose to file a new bug for the default profile conflicts.

(In reply to Karel Srot from comment #25)
> I would rather propose to file a new bug for the default profile conflicts.

Fine by me.