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

update CI images to include pasta #16525

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 15, 2022

Ref: containers/automation_images#246

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 15, 2022
@Luap99 Luap99 requested a review from cevich November 15, 2022 17:42
@Luap99 Luap99 force-pushed the CI-update-image branch 2 times, most recently from f96eef5 to 6775bc2 Compare November 15, 2022 18:42
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@Luap99
Copy link
Member Author

Luap99 commented Nov 16, 2022

FYI these are the installed versions:

Fedora release 37 (Thirty Seven)
Kernel:  6.0.8-300.fc37.x86_64
Cgroups:  cgroup2fs
aardvark-dns-1.2.0-6.fc37-x86_64
conmon-2.1.4-3.fc37-x86_64
containernetworking-plugins-1.1.1-8.fc37-x86_64
containers-common-1-73.fc37-noarch
container-selinux-2.191.0-1.fc37-noarch
criu-3.17.1-3.fc37-x86_64
crun-1.7-1.fc37-x86_64
golang-1.19.2-1.fc37-x86_64
libseccomp-2.5.3-3.fc37-x86_64
netavark-1.2.0-5.fc37-x86_64
passt-0^20221110.g4129764-1.fc37-x86_64
podman-4.3.1-1.fc37-x86_64
runc-1.1.4-1.fc37-x86_64
skopeo-1.10.0-3.fc37-x86_64
slirp4netns-1.2.0-8.fc37-x86_64

@Luap99
Copy link
Member Author

Luap99 commented Nov 29, 2022

@giuseppe reping, could please take a look at the e2e failures, they look all cgroup related.

@giuseppe
Copy link
Member

it looks like a regression in crun 1.7. Taking a look...

@giuseppe
Copy link
Member

fix here: containers/crun#1084

I'll release 1.7.2 ASAP

@Luap99
Copy link
Member Author

Luap99 commented Nov 29, 2022

fix here: containers/crun#1084

I'll release 1.7.2 ASAP

Thank you.

@giuseppe
Copy link
Member

giuseppe commented Nov 29, 2022

@Luap99
Copy link
Member Author

Luap99 commented Dec 20, 2022

@giuseppe @adrianreber It looks like checkpoint/restore is broken on fedora 36

Error is

could not load `libcriu.so.2`

These are the version installed:

Kernel:  6.0.12-200.fc36.x86_64
criu-3.17.1-2.fc36-x86_64
crun-1.7.2-1.fc36-x86_64

How to fix this?

@adrianreber
Copy link
Collaborator

@giuseppe @adrianreber It looks like checkpoint/restore is broken on fedora 36

Error is

could not load `libcriu.so.2`

These are the version installed:

Kernel:  6.0.12-200.fc36.x86_64
criu-3.17.1-2.fc36-x86_64
crun-1.7.2-1.fc36-x86_64

How to fix this?

dnf -y install criu-libs

The crun fix is on its way: https://bodhi.fedoraproject.org/updates/FEDORA-2022-68051670ee

@Luap99 Luap99 force-pushed the CI-update-image branch 2 times, most recently from 265045c to 94212fe Compare January 10, 2023 17:06
@cevich
Copy link
Member

cevich commented Jan 10, 2023

I have an older image-update PR I've been kicking around for a while now. Since this one is better, and there's no sense in two separate image-update PRs, I'll close mine in favor of this one. I'll keep subscribed here though in case my eyeballs are needed.

@Luap99
Copy link
Member Author

Luap99 commented Jan 10, 2023

For reference, test failures are caused by a bind() kernel bug: https://bugzilla.redhat.com/show_bug.cgi?id=2159066

Luap99 added a commit to Luap99/libpod that referenced this pull request Jan 12, 2023
Something is wrong with WSL in the new windows images, see
containers#16525 (comment)

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@cevich
Copy link
Member

cevich commented Jan 12, 2023

We also need to make sure no new release-branches are created

Ugh...this may actually be the case. Well 💩

@Luap99 let me guess, your preference is to get this PR merged before any new releases, ya?

To be clear, what I'm worried about is:

    # FIXME FIXME FIXME: hard coding the windows ID to use an older image
    # something is wrong with WSL with the new images, see https://github.com/containers/podman/pull/16525#issuecomment-1379890071
    WINDOWS_AMI: "win-server-wsl-c5069932136759296"
    # WINDOWS_AMI: "win-server-wsl-${IMAGE_SUFFIX}"

That's going to end up in a release-branch. That makes me nervous. Our image timestamping automation can handle it just fine, it's the human-maintainers I'm worried about 😖

Context: It's both bad, and wrong (and often technically difficult) to update a CI VM-image set in release branches. If we merge this, it means everyone agrees to live with the de-sync, basically forever.

Am I being paranoid, or is this a real problem we should consider?

@Luap99
Copy link
Member Author

Luap99 commented Jan 12, 2023

Am I being paranoid, or is this a real problem we should consider?

No this is a problem. I don't like it either, I don't think we have to merge it right now. The blocked PR already skipped the test anyway and was merged earlier so it should not block somebodies work, at least I am not aware of anything.

My only point is that it makes sense to test on more recent images. At this point I really do not believe the current images reflect the software on actual users systems any more and therefore we might miss critical bugs because of it. I know we have gating tests in fedora but they only cover a subset compared to upstream CI.

@cevich
Copy link
Member

cevich commented Jan 12, 2023

No this is a problem.

Okay, thanks for the sanity-check. Let's wait for @n1hility to help fix Windows (what would we do without him!) and/or merge this PR after the release is branched. Maybe the kernel bug will be resolved by then (one can wish).

At this point I really do not believe the current images reflect the software on actual users systems any more

I 100% agree, the current images are based on a beta-build of F37 😕 The good thing is, I think in a few weeks you've blown through image-update problems I was failing to fix for more than a month 😁

@n1hility
Copy link
Member

n1hility commented Jan 16, 2023

@cevich It's a hairy problem, although I have a solution almost ready to go, just needs polishing. It will require PRs to images and podman to address.

@cevich
Copy link
Member

cevich commented Jan 16, 2023

@n1hility okay, thanks for the update and guesstimate. Last I looked, you're in-the-clear for an automation_images PR, there's nothing else happening over there ATM.

This PR is the main one trying to update images for Podman CI, so we can update the ID here when it's ready. Thanks again.

@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2023
Images were build here: containers/automation_images#249

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
use this target in the CI script to make sure it uses the correct gingko

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
It is not working in CI and locally, I filed containers#17074 to track it so we
can fix it later.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
It is only needed for one CI task, and that task already calls
`make .install.swagger` in setup_environment.sh.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
They should work with the new VM images.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@Luap99 Luap99 removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/needs-unit-tests do-not-merge/needs-confirmation Waiting for feedback from stakeholder before merging labels Jan 19, 2023
@Luap99
Copy link
Member Author

Luap99 commented Jan 19, 2023

Rebased to include the windows fix, this should turn green now unless I am the most unluckiest person ever.
@cevich @edsantiago PTAL again

@cevich
Copy link
Member

cevich commented Jan 19, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2023
@openshift-merge-robot openshift-merge-robot merged commit 7689e26 into containers:main Jan 19, 2023
@Luap99
Copy link
Member Author

Luap99 commented Jan 19, 2023

Not a single flake as far as I can tell, guess I am not so unlucky after all.

@Luap99 Luap99 deleted the CI-update-image branch January 19, 2023 16:31
@cevich
Copy link
Member

cevich commented Jan 19, 2023

Nope, sometimes all it takes is persistence, and teamwork 👍

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants