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

disable downloading filelists by default #4859

Merged
merged 1 commit into from
May 5, 2024

Conversation

lukewarmtemp
Copy link
Contributor

@lukewarmtemp lukewarmtemp commented Mar 4, 2024

Fixes coreos/fedora-coreos-tracker#1643

Disable filelists from being downloaded by default for Fedora 40. Adds
the DNF_CONTEXT_SETUP_SACK_FLAG_SKIP_FILELISTS where applicable,
which prevents the filelists from being downloaded.

For the changes in rpmostree_context_prepare(), disabling filelists is
conditional on whether a filepath was specified for rpm-ostree install
(ie. rpm-ostree install /usr/bin/htop).

For the changes in refresh_md_transaction_execute(), rpm-ostree refresh-md will not download filelists if they were not previously
downloaded. If they were previously downloaded, the filelists will be
updated.

Since there are many functions affected by this change, the option to
return to the previous filelist behaviour is also implemented. This is
done by adding:

[Service]
Environment="DOWNLOAD_FILELISTS=true"

in a newly created conf file under /etc/systemd/system/rpm-ostreed.service.d
For example: /etc/systemd/system/rpm-ostreed.service.d/filelists.conf

Copy link

openshift-ci bot commented Mar 4, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
src/daemon/rpmostreed-transaction-types.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-core.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-core.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-core.cxx Outdated Show resolved Hide resolved
@lukewarmtemp lukewarmtemp changed the title Luyang filelists disable downloading filelists by default Mar 5, 2024
@cgwalters
Copy link
Member

I don't object to this at all, but I am uncertain about the bigger picture here versus aligning more with the dnf codebase more directly - e.g. dnf5 porting, or trying to do an even bigger hammer like forking off dnf as a binary...

@cgwalters
Copy link
Member

I forgot to really be clear: Thanks so much for working on this! It is an important and notable change. And dropping filelists is a giant optimization for those who don't need them for sure, and having parity would be good.

I'm basically just saying though this will not be the last thing of this form where we have some nontrivial divergence from dnf and it is worth taking the opportunity to weigh changes like this versus larger structural changes to align more.

@jmarrero
Copy link
Member

jmarrero commented Mar 8, 2024

I am a bit confused as in the past porting rpm-ostree to dnf5 we did not see as a priority and I am not sure how forking the dnf binary vs dnf forking us on ostree systems would work, sounds like the could end up more tied up?

I guess, my question is, should we create a concrete plan before continuing to extend/modify rpm-ostree and basically just deal with bug fixes and put a hold on any other feature request while we trace what rpm-ostree in the ostree native container stable world looks like?

@travier
Copy link
Member

travier commented Mar 8, 2024

I'm definitely +1 for doing this change. This will impact both server side and client side composes so everyone would benefit from it.

@cgwalters
Copy link
Member

I am a bit confused as in the past porting rpm-ostree to dnf5 #2139 (comment)

You're right, I shouldn't have mentioned that particularly given the delays in dnf5.

@dustymabe
Copy link
Member

Since F40 is quickly approaching can we try to finish this off?

@lukewarmtemp lukewarmtemp self-assigned this Mar 26, 2024
@lukewarmtemp lukewarmtemp marked this pull request as ready for review March 26, 2024 22:00
@lukewarmtemp lukewarmtemp force-pushed the luyang-filelists branch 9 times, most recently from 8665dac to 2b73bb1 Compare April 22, 2024 03:27
@jmarrero
Copy link
Member

this looks mostly lgtm, @lukewarmtemp can you squash the commits?

@jmarrero
Copy link
Member

jmarrero commented May 3, 2024

@lukewarmtemp mind rebasing?

Disable filelists from being downloaded by default for Fedora 40. Adds
the `DNF_CONTEXT_SETUP_SACK_FLAG_SKIP_FILELISTS` where applicable,
which prevents the filelists from being downloaded.

For the changes in `rpmostree_context_prepare()`, disabling filelists is
conditional on whether a filepath was specified for `rpm-ostree install`
(ie. rpm-ostree install /usr/bin/htop).

For the changes in refresh_md_transaction_execute(), `rpm-ostree
refresh-md` will not download filelists if they were not previously
downloaded. If they were previously downloaded, the filelists will be
updated.

Since there are many functions affected by this change, the option to
return to the previous filelist behaviour is also implemented. This is
done by adding:

[Service]
Environment="DOWNLOAD_FILELISTS=true"

in a newly created conf file under /etc/systemd/system/rpm-ostreed.service.d
For example: /etc/systemd/system/rpm-ostreed.service.d/filelists.conf
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

@jmarrero jmarrero merged commit 22bf04d into coreos:main May 5, 2024
17 checks passed
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.

Fedora 40 Changes: DNF: Do not download filelists by default
6 participants