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

Cirrus: Bump up to Fedora 35 & Ubuntu 21.10 #3599

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

cevich
Copy link
Member

@cevich cevich commented Oct 25, 2021

What type of PR is this?

/kind other

What this PR does / why we need it:

The Fedora 35 cloud images have switched to UEFI boot with a GPT
partition. Formerly, all Fedora images included support for runtime
re-partitioning. However, the requirement to test alternate storage
has since been dropped/removed. Rather than maintain a disused
feature, and supporting scripts, these Fedora VM images have reverted
to the default: Automatically resize to 100% on boot.

How to verify it

CI testing will pass

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Please wait for containers/podman#11795 to merge first.

Does this PR introduce a user-facing change?

None

@cevich
Copy link
Member Author

cevich commented Oct 26, 2021

@nalind @edsantiago Sn F35 some "No Op" test is timing out after 45minutes. Sorry I don't know the unit-tests well at all, so am not sure where to even begin looking. Would you mind taking a peek and advising what/where needs fixing by whom?

@edsantiago
Copy link
Member

I tried, but this failure is beyond me. Sorry.

@nalind
Copy link
Member

nalind commented Oct 26, 2021

TestNoop() is a unit test function in the chroot package. For some reason, the helper it invokes in the chroot environment that it sets up appears to be hanging.

@cevich
Copy link
Member Author

cevich commented Oct 26, 2021

Thanks for taking a look @edsantiago

At the VM-image level, the only other case where I've seen something completely hang, is with the podman bindings test. Ironically, also during a build-test.

@nalind is there anything I can do to help get this fixed? Odd that it would only affect F35, is go1.16.8 to blame maybe?

Edit: Ref: podman bindings build test failure logs

@nalind
Copy link
Member

nalind commented Oct 27, 2021

@nalind is there anything I can do to help get this fixed? Odd that it would only affect F35, is go1.16.8 to blame maybe?

I've managed to narrow it down to an interaction between something that changes based on how we build the statically-linked unit test helper, which the unit tests invoke to ensure that chroot isolation at least does the things we think it does, and the seccomp filter which the tests use when they invoke the unit test helper, which hasn't been the same as what we use outside of unit tests for a while now.

Switching from using the external linker to statically link the helper to letting the Go compiler use its internal linker seems to work around it, somehow, but the tests could always stand to be a better reflection of how things work outside of the tests, so I'm going to try that first.

Can you cherry-pick the tip of https://github.com/nalind/buildah-1/tree/chroot-unit-test-seccomp branch into this PR and see if that helps? It does for me on my mostly-F35 box.

@cevich
Copy link
Member Author

cevich commented Oct 28, 2021

so I'm going to try that first.

Good, yes, I'm almost always in favor of spending more time to fix it right, rather than constantly worry forever. And thanks for going deep, that's some black-magic-voodoo you dug up, I can't even spell half those words! Yes, happy to cherry-pick, and of course you're welcome to temporarily rebase off this PR too. Please let me know if there's any other ways I can help.

@cevich
Copy link
Member Author

cevich commented Nov 2, 2021

@nalind that seems to have done the trick.

@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2021

@cevich is this still a draft or ready to merge?

@cevich
Copy link
Member Author

cevich commented Nov 4, 2021

is this still a draft or ready to merge?

Thanks for checking, yes it's still a draft. It's using a F35beta image whereas it's actually been released now. I'll update this PR again once the podman one is ready to go, so we can keep common image-IDs across the projects (helpful for debugging).

@cevich cevich force-pushed the update_to_f35 branch 2 times, most recently from 3740488 to 6c9a250 Compare November 4, 2021 16:44
@cevich
Copy link
Member Author

cevich commented Nov 5, 2021

@nalind were you going to make a separate PR for your 'chroot-unit-test-seccomp' branch, or should I just include it here?

@nalind
Copy link
Member

nalind commented Nov 5, 2021

@nalind were you going to make a separate PR for your 'chroot-unit-test-seccomp' branch, or should I just include it here?

Feel free to pull it in if it makes things simpler.

@cevich
Copy link
Member Author

cevich commented Nov 5, 2021

Feel free to pull it in if it makes things simpler.

I think it does, just wasn't sure if you wanted to have scrutiny beyond this PR. Seems like the fix works, so 🤷‍♂️ we can run with it.

For this PR in general, the status is same as I said a few days ago: I'd like to wait until we get green-lights on the podman-side simply out of convenience to share the same image. My plan is to open similar PRs for all the other container repos. at that time as well.

@cevich cevich force-pushed the update_to_f35 branch 2 times, most recently from 8335c09 to 9e14fed Compare November 12, 2021 15:33
@cevich cevich changed the title [WIP] Cirrus: Bump Fedora to release 35 [WIP] Cirrus: Bump up to Fedora 35 & Ubuntu 21.10 Nov 12, 2021
@cevich cevich force-pushed the update_to_f35 branch 2 times, most recently from 1be0677 to e6ef919 Compare November 17, 2021 20:49
@rhatdan
Copy link
Member

rhatdan commented Nov 18, 2021

Is this still a WIP?

@cevich
Copy link
Member Author

cevich commented Nov 18, 2021

Is this still a WIP?

Only barely. I can un-WIP it if it's needed right now...but I think having the podman tests actually pass adds a bit more confidence the images are safe to use elsewhere. I'm not going to press that as a hard requirement though.

nalind and others added 2 commits November 18, 2021 14:07
When we link our test helper statically using the external linker, the
hardwired default seccomp filter we get from the runtime-tools generator
triggers a hang in it at startup.

Rather than switch to the internal linker, which seems to work around
this, start using the same seccomp filter for unit tests that we
actually use in real life, leaving analysis of which difference between
the two is responsible for it for another day.

Signed-off-by: Nalin Dahyabhai <[email protected]>
The Fedora 35 cloud images have switched to UEFI boot with a GPT
partition. Formerly, all Fedora images included support for runtime
re-partitioning. However, the requirement to test alternate storage
has since been dropped/removed.  Rather than maintain a disused
feature, and supporting scripts, these Fedora VM images have reverted
to the default: Automatically resize to 100% on boot.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich changed the title [WIP] Cirrus: Bump up to Fedora 35 & Ubuntu 21.10 Cirrus: Bump up to Fedora 35 & Ubuntu 21.10 Nov 18, 2021
@cevich cevich marked this pull request as ready for review November 18, 2021 19:08
@cevich
Copy link
Member Author

cevich commented Nov 18, 2021

Rebased / force-pushed / un-WIP/draft the PR. Assuming it passes, please merge if there is a need and/or containers/podman#11795 gets merged - whichever happens first 😄

@rhatdan
Copy link
Member

rhatdan commented Nov 18, 2021

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants