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

Increase the metadata size limit to 128MB #2865

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

barthalion
Copy link
Contributor

Flathub has hit the 10MB limit in 2022, and we had to drop some CPU architectures from the main summary to subsummaries, effectively cutting off users running too old Flatpak version. Despite that, the main summary containing only x86_64 is already at 7MB. As this is eventually going to happen to subsummaries as well, preemptively bump the limit 12 times.

It takes between 2 and 3 years for a change like this to roll out across Linux distibutions so the best time for this was yesterday.

fixes #2715

Flathub has hit the 10MB limit in 2022, and we had to drop less popular
CPU architectures from the main summary to subsummaries, effectively
cutting off users running too old Flatpak version. Despite that, the
main summary containing only x86_64 is already at 7MB. As this is
eventually going to happen to subsummaries as well, preemptively bump
the limit 12 times.

It takes between 2 and 3 years for a change like this to roll out across
Linux distributions so the best time for this was yesterday.

fixes ostreedev#2715
@openshift-ci
Copy link

openshift-ci bot commented May 25, 2023

Hi @barthalion. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@barthalion
Copy link
Contributor Author

Looks like a single test hardcodes the size to test against the modified variable, I will get that fixed later today.

@dbnicholson
Copy link
Member

/ok-to-test

@barthalion
Copy link
Contributor Author

I don't think I understand how test-pull-large-metadata is supposed to work. It overwrites a commit object with zeroes, so it ends up complaining about a corrupted object, although only if I change its size from 20MB?

@dbnicholson
Copy link
Member

I don't think I understand how test-pull-large-metadata is supposed to work. It overwrites a commit object with zeroes, so it ends up complaining about a corrupted object, although only if I change its size from 20MB?

The pull is going to fail regardless because the commit object is corrupted. However, what the test is checking for is that it explicitly fails with a size too big error. That check happens before the commit object is read, so you'd get an exceeded maximum error in the case that the object is too big. If it's not too big (> 128MB with your change), then you'll get a Corrupted commit object error.

tests/test-pull-large-metadata.sh Outdated Show resolved Hide resolved
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Marking as needs work

@cgwalters cgwalters added triaged This issue has been evaluated and is valid reward/medium Fixing this will be notably useful labels May 30, 2023
@barthalion
Copy link
Contributor Author

@dbnicholson
Copy link
Member

https://github.com/ostreedev/ostree/actions/runs/5256285362/jobs/9497375004?pr=2865 looks like a fluke.

Indeed. test-concurrency.py occasionally fails for as yet undetermined reasons #2038. I restarted the jobs so it would get green but I don't think there are any further issues.

@dbnicholson dbnicholson dismissed cgwalters’s stale review June 13, 2023 14:59

Issues were addressed

@dbnicholson dbnicholson merged commit 9244518 into ostreedev:main Jun 13, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test reward/medium Fixing this will be notably useful triaged This issue has been evaluated and is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata size limit too low
3 participants