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

Do not download updateinfo and comps metadata by default #630

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Jun 20, 2023

Updateinfo and comps metadata were always downloaded and loaded by default. This is not necessary because each command configures the optional_metadata_types to download/load only what it requires.

Also there was a bug in detection whether updateinfo metadata are needed.

For: https://bugzilla.redhat.com/show_bug.cgi?id=2214520

In my testing when loading updateinfo metadata memory requirements spike at ~623 MiB but without them its only ~132 MiB (on F38 with clean cache). I will further investigate why the updateinfo metadata use so much during libsolv loading, it seems unexpected to me given their size.

@@ -187,8 +187,7 @@ class ConfigMain::Impl {
OptionBool debug_solver{false};
OptionStringList installonlypkgs{INSTALLONLYPKGS};
OptionStringList group_package_types{GROUP_PACKAGE_TYPES};
OptionStringSet optional_metadata_types{
OptionStringSet::ValueType{libdnf::METADATA_TYPE_COMPS, libdnf::METADATA_TYPE_UPDATEINFO}};
OptionStringSet optional_metadata_types{OptionStringSet::ValueType{}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can modify in dnf5 default but we should not modify library defaults. This default is insecure. I would rather investigate the cause then disabling loading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the blocker only because of the change of default type for download.

@j-mracek j-mracek added the blocked Further work on issue or PR is blocked by something else label Jun 21, 2023
@j-mracek
Copy link
Contributor

j-mracek commented Jun 21, 2023

This patch is not fixing the issue but hides it. It looks like that the issue is in processing updateinfo metadata, because I was unable to reproduce the issue with --setopt=optional_metadata_types=,comps. I do not recommend to merge the PR and I would rather wait for the proper solution.

Update:
The code is ok, and can by merged without modification of default download set. Then it will allow user overrides and workaround for the issue. The proper fix of the issue - openSUSE/libsolv#533

It fixes the condition and extracts it into a separate function.
'@' also denotes a module but modules are handled only by module
commands.
Also I am not sure there is a way how to differentiate a group from a
module by the spec.

This method is also used from commands that don't use comps in any way
such as repoquery so perhaps it would be better to have a separate
check only in relevant commands. On the other hand I like the unified
handling.
@j-mracek
Copy link
Contributor

LGTM

@j-mracek
Copy link
Contributor

@kontura May I ask you for rebase?

@kontura
Copy link
Contributor Author

kontura commented Jun 28, 2023

@kontura May I ask you for rebase?

Sure, rebased.

@kontura kontura added ready for review and removed blocked Further work on issue or PR is blocked by something else labels Jun 28, 2023
@kontura kontura added this pull request to the merge queue Jun 28, 2023
Merged via the queue into rpm-software-management:main with commit 274a300 Jun 28, 2023
@kontura kontura deleted the optional_metadata branch June 28, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants