Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install module package IDs on module enable #1656

Conversation

evan-goode
Copy link
Member

This is a draft PR since tests are failing, and I'm sure there's something wrong with my understanding of the problem or the proposed fix. I'm opening this PR to get a second pair of eyes.

For https://issues.redhat.com/browse/RHEL-16580.

When enabling a module for an incompatible architecture (i.e. enabling an AArch64 module on an x86_64 system), DNF 4 users should see an appropriate error message. Currently, users only see "nothing provides" errors with no mention of the architecture incompatibility:

# dnf4 module enable php:remi-7.4
Modular dependency problem:

 Problem: nothing provides requested module(composer:2)
Error: Problems in request:
Modular dependency problems:

 Problem 1: nothing provides requested module(composer:2)
 Problem 2: nothing provides requested module(php:remi-7.4)

One way to achieve the desired result is to, on module enable, request to install the module package ID, rather than just requesting to install Provides: module(MODULENAME:STREAM). DNF 5 does so here (1, 2).

This patch attempts to introduce the same fix in DNF 4. After this patch, the output of the same command as above shows "does not have compatible architecture" errors, as we desire:

# dnf4 module enable php:remi-7.4
Modular dependency problems:

 Problem 1: nothing provides requested module(composer:2)
 Problem 2: conflicting requests
  - module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64 does not have a compatible architecture
  - nothing provides module(php) needed by module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64
  - nothing provides module(platform:el8) needed by module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64
Error: Problems in request:
Modular dependency problems:

 Problem 1: nothing provides requested module(composer:2)
 Problem 2: conflicting requests
  - module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64 does not have a compatible architecture
  - nothing provides module(php) needed by module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64
  - nothing provides module(platform:el8) needed by module composer:2:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64
 Problem 3: nothing provides requested module(php:remi-7.4)
 Problem 4: conflicting requests
  - module php:remi-7.4:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64 does not have a compatible architecture
  - nothing provides module(platform:el8) needed by module php:remi-7.4:20240416075243:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64

@evan-goode evan-goode marked this pull request as draft April 18, 2024 22:23
@j-mracek
Copy link
Contributor

This patch cannot work for more reasons specially when there are more module items from module stream.
Example available modules - moduleA:stream1:1111:asdf.x86_64, moduleA:stream1:2222:asdf.x86_64

dnf|dnf5 module enable moduleA:stream1

Workflow -> dnf4

  1. Resolve spec to moduleA:stream1:1111:asdf.x86_64, moduleA:stream1:1111:asdf.x86_64 (there is an extra processing to handle globs)
  2. Modify module persistor (store strings name:stream) by enable(const ModulePackage * module)
  3. resolve module dependencies according to module presistor
    Iterate over all modules and according to persistor actual state enabled and default module items set to install according to provide module(name:stream)
    install provide module(moduleA:stream1)
    install provide module(moduleA:stream1)

Workflow -> dnf5

  1. Resolve spec to moduleA:stream1:1111:asdf.x86_64, moduleA:stream1:2222:asdf.x86_64 (there is an extra processing to handle globs)
  2. Modify module persistor (store strings name:stream) by enable(const ModulePackage * module)
  3. Store module items that we want to enabled in separate container for each spec and name:stream
  4. resolve module dependencies according to module presistor
    Iterate over all modules and according to persistor actual state enabled and default module items set to install according to provide module(name:stream)
    install provide module(moduleA:stream1)
    install provide module(moduleA:stream1)
    install oneof moduleA:stream1:1111:asdf.x86_64, moduleA:stream1:2222:dddd.x86_64

Workflow -> dnf4 with your patch

  1. Resolve spec to moduleA:stream1:1111:asdf.x86_64, moduleA:stream1:asdf:dddd.x86_64 (there is an extra processing to handle globs)
  2. Modify module persistor (store strings name:stream) by enable(const ModulePackage * module)
  3. resolve module dependencies according to module presistor
    Iterate over all modules and according to persistor actual state enabled and default module items set to install according to provide module(name:stream) plus add install request for each module item
    install provide module(moduleA:stream1)
    install provide module(moduleA:stream1)
    install package moduleA:stream1:1111:asdf.x86_64
    install package moduleA:stream1:2222:asdf.x86_64

The last two install package requests are conflicting therefore it will fail for request where updates are already available.

@j-mracek
Copy link
Contributor

I see two solution and both requires additional method in API of ModulePackageContainer. This is required because ModulePackageContainer has not enough information to provide better results:

  1. add enable_spec method - it would require to move some logic from https://github.com/rpm-software-management/dnf/blob/master/dnf/module/module_base.py , but it might also require to move additional logic for install, because it is connected. It means that the scope is not small
  2. add enable(vector<ModulePackage>) - it can be very simple and require only items with the same name:stream. It can enable module => modify persistor, but remember the vector and when module resolve is called then creates install oneof for each vector. The method can be called from Python by def _create_module_dict_and_enable(self, module_list, spec, enable=True):.

Both options require new information stored, therefore it must be cleaned when rollback() is called.

evan-goode added a commit to evan-goode/dnf that referenced this pull request Apr 23, 2024
Module packages should be explicitly specified when enabling modules,
see rpm-software-management/libdnf#1656.
@evan-goode
Copy link
Member Author

I see two solution and both requires additional method in API of ModulePackageContainer. This is required because ModulePackageContainer has not enough information to provide better results:

1. add `enable_spec` method  - it would require to move some logic from https://github.com/rpm-software-management/dnf/blob/master/dnf/module/module_base.py  , but  it might also require to move additional logic for `install`, because it is connected. It means that the scope is not small

2. add `enable(vector<ModulePackage>)` - it can be very simple and require only items with the same name:stream. It can enable module => modify persistor, but remember the vector and when module resolve is called then creates `install oneof` for each vector. The method can be called from Python by `def _create_module_dict_and_enable(self, module_list, spec, enable=True):`.

Both options require new information stored, therefore it must be cleaned when rollback() is called.

Thanks very much for the info. I've implemented solution (2) as you describe, and I think this is close to the behavior we want. The error message when enabling an incompatible module is now quite similar to DNF 5's:

Modular dependency problem:

 Problem: nothing provides requested module(composer:2)
Error: Problems in request:
Modular dependency problems:

 Problem 1: nothing provides requested module(composer:2)
 Problem 2: nothing provides requested module(php:remi-7.4)
 Problem 3: conflicting requests
  - module php:remi-7.4:20240420123953:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64 does not have a compatible architecture
  - nothing provides module(platform:el8) needed by module php:remi-7.4:20240420123953:00000000.aarch64 from rpms.remirepo.net_enterprise_8_modular_aarch64

However, with this patch and rpm-software-management/dnf#2086, DNF tests (cmake test in the dnf repository) are failing:

 Modular dependency problems:
 
  Problem: module httpd:2.4:1:.noarch from _all conflicts with module(httpd) provided by httpd:2.4:2:.noarch from _all
   - module httpd:2.4:2:.noarch from _all conflicts with module(httpd) provided by httpd:2.4:1:.noarch from _all
   - cannot install the best candidate for the job
   - conflicting requests

Looking into it.

@j-mracek j-mracek self-assigned this Apr 29, 2024
libdnf/goal/Goal.hpp Outdated Show resolved Hide resolved
@evan-goode evan-goode force-pushed the evan-goode/modular-arch-bug branch from 2925b24 to 433252b Compare May 2, 2024 18:00
@evan-goode evan-goode marked this pull request as ready for review May 2, 2024 18:09
@evan-goode evan-goode force-pushed the evan-goode/modular-arch-bug branch from 433252b to 45aa4db Compare May 2, 2024 18:12
Like in DNF 5, we should require oneof the module items when a module is
enabled. This way, a clearer error message is printed when trying to
enable a module for an incompatible architecture.

For https://issues.redhat.com/browse/RHEL-16580.
@evan-goode evan-goode force-pushed the evan-goode/modular-arch-bug branch from 45aa4db to b112041 Compare May 2, 2024 19:15
@evan-goode
Copy link
Member Author

Closing this, the issue that would be fixed is not severe enough to warrant the breakage this could cause.

@evan-goode evan-goode closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants