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

Revert the ESP maximum size back to 600MiB #5081

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

hughsie
Copy link
Contributor

@hughsie hughsie commented Aug 25, 2023

The original issue was that 200MiB was too small, and we only increased the maximum for people experimenting with UKIs. Seeing as Anaconda chooses the maximum size in more cases than we'd like just drop the maximum size back down to 600MiB which is easily big enough for firmware updates.

This should fix several regressions like:

Thank you for your contribution!

Please check that your PR follows these rules:

  • Code conventions. tl;dr: Follow PEP8, wrap at 100 chars, and provide docstrings.

  • Commit message conventions. tl;dr: Heading, empty line, longer explanations, all wrapped manually. If in doubt, write a longer commit message with more details.

  • Tests pass and cover your changes. You can run tests locally manually, or have them run as part of the PR. A team member will have to enable tests manually after inspecting the PR.
    If you don't know how, ask us for help!

  • Release notes and docs if the PR adds something major or changes existing behavior.
    If you don't know how, ask us for help!

  • RHEL rules: If your PR is for a rhel-* branch, pay special attention to commit messages. Make sure all commit messages include a line linking the commit to a bug, such as Resolves: rhbz#123456. For more information, see the complete rules.
    If you don't know how, ask us for help!

Don't forget - if you don't know how, ask us for help!

And now that you read this all, you can delete it and type your own description of your changes :-)

@hughsie
Copy link
Contributor Author

hughsie commented Aug 25, 2023

@AdamWill

@github-actions github-actions bot added the f39 label Aug 25, 2023
@jkonecny12 jkonecny12 added f40 release note required Write a release note for this change. labels Aug 25, 2023
@hughsie
Copy link
Contributor Author

hughsie commented Aug 25, 2023

Release note check

This failed, but I think it's okay as the original PR to change the default included a release note.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

You might want to drop also docs/release-notes/bigger-esp.rst. Otherwise LGTM. Thank you for taking care of this!

@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Aug 28, 2023

For future records - this reverts #4711 and c253390 (sans the release note)

=> Not true, changes just the maximum size.

@AdamWill
Copy link
Contributor

You might want to drop also docs/release-notes/bigger-esp.rst. Otherwise LGTM. Thank you for taking care of this!

well, it should probably be amended? The ESP will still be bigger than before we started changing things here, just not so much bigger.

@VladimirSlavik
Copy link
Contributor

Oh. I missed that completely, thank you. In that case it's fine.

@AdamWill
Copy link
Contributor

I would say it's not completely accurate as written. I would change Description to:

"The minimum size of the EFI System Partition (ESP) created by Anaconda has changed from 200 MiB to 500 MiB. The maximum size, which is used in most cases, remains at 600 MiB."

@hughsie
Copy link
Contributor Author

hughsie commented Aug 28, 2023

Want me to update it to that? Im fine if someone wants to rewrite it on merge.

The original issue was that 200MiB was too small, and we only increased the
maximum for people experimenting with UKIs. Seeing as Anaconda chooses the
maximum size in more cases than we'd like just drop the maximum size back down
to 600MiB which is easily big enough for firmware updates.

This should fix several regressions like:

 * https://bugzilla.redhat.com/show_bug.cgi?id=2212121
 * https://bugzilla.redhat.com/show_bug.cgi?id=2214342

Signed-off-by: Richard Hughes <richard@hughsie.com>
@hughsie
Copy link
Contributor Author

hughsie commented Aug 29, 2023

Want me to update it to that

Narrator: He did that.

@VladimirSlavik
Copy link
Contributor

/kickstart-test --testtype smoke

@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Aug 29, 2023

/kickstart-test --testtype storage

(will need interpretation of results - runs also known failures)

@VladimirSlavik
Copy link
Contributor

Interpretation of the storage kickstart tests results:

  • Known failures:
    • autopart-luks-1 = gh774
    • raid-1 = gh777
    • storage-multipath-autopart = rhbz1853668
  • Failures without explanation:
    • lvm-luks-1
    • lvm-luks-2
    • lvm-luks-3
    • lvm-luks-4
    • part-luks-1
    • part-luks-2
    • part-luks-3
    • part-luks-4
    • raid-luks-1
    • raid-luks-2
    • raid-luks-3
    • raid-luks-4

... I think I see a theme there. Onwards to logs...

@VladimirSlavik
Copy link
Contributor

tl;dr I hope the commonality for LVM and part tests is the (explicit) storage layout in these tests, which is no longer valid. For the raid-luks group, it's more mysterious but the installation appears to succeed despite


Case by case:

  • lvm-luks-* fail because "Unable to allocate requested partition scheme." and manual intervention is needed
  • part-luks-* ditto
  • raid-luks-{1,2,4} appear to install successfully, but validation of the test fails because cat: /var/tmp/kstest-****/RESULT: No such file or directory; the %post script seems to run ok with exit 0 though...
  • raid-luks-3 dies because of "gnome-kiosk exited with status 1" which async kills the installer; this is highly likely to be unrelated to the PR changes and a fluke, hopefully

I think this indicates that mostly the tests need rewriting for the new storage layout, but I'll have to verify this manually.

Next: Run the same kickstart scenario manually, investigate the VM.

@hughsie @AdamWill Is this intended for beta already, or later? From the FE list I gather this is meant to go post-Beta, right?

@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Aug 31, 2023

/kickstart-test --testtype storage

(edit: The same test set failed again.)

@AdamWill
Copy link
Contributor

It is in the FE list: https://bugzilla.redhat.com/show_bug.cgi?id=2234951 . I would much prefer this land for Beta, it seems much safer for the behaviour to be the same in Beta and Final than not.

@rvykydal
Copy link
Contributor

rvykydal commented Sep 5, 2023

* `raid-luks-{1,2,4}` appear to install successfully, but validation of the test fails because `cat: /var/tmp/kstest-****/RESULT: No such file or directory`; the %post script seems to run ok with exit 0 though...

* `raid-luks-3` dies because of "gnome-kiosk exited with status 1" which async kills the installer; this is highly likely to be unrelated to the PR changes and a fluke, hopefully

I agree, probably rhinstaller/kickstart-tests#997

@VladimirSlavik
Copy link
Contributor

Well, duh. I missed that all of the failed tests are manual... they fail without the change as well.

$kickstart-tests> grep TESTTYPE {part,lvm,raid}-luks-* | grep manual
part-luks-1.sh:TESTTYPE="manual storage partition luks"
part-luks-2.sh:TESTTYPE="manual storage partition luks"
part-luks-3.sh:TESTTYPE="manual storage partition luks"
part-luks-4.sh:TESTTYPE="manual storage partition luks"
lvm-luks-1.sh:TESTTYPE="manual storage lvm luks"
lvm-luks-2.sh:TESTTYPE="manual storage lvm luks"
lvm-luks-3.sh:TESTTYPE="manual storage lvm luks"
lvm-luks-4.sh:TESTTYPE="manual storage lvm luks"
raid-luks-1.sh:TESTTYPE="manual storage raid luks"
raid-luks-2.sh:TESTTYPE="manual storage raid luks"
raid-luks-3.sh:TESTTYPE="manual storage raid luks"
raid-luks-4.sh:TESTTYPE="manual storage raid luks"

(I think the bit that tripped me is that we usually add the disable-tags at the end of the list, but manual is at front.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f39 f40 release note required Write a release note for this change.
Development

Successfully merging this pull request may close these issues.

5 participants