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.

Sign up for GitHub

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

Jump to bottom

VM images: bump to 2023-11-16 #20610

Merged
merged 3 commits into from
Nov 17, 2023
Merged

VM images: bump to 2023-11-16 #20610

merged 3 commits into from
Nov 17, 2023

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Nov 6, 2023 •
edited
Loading

No change in base, just newer packages (especially crun-1.11-1)

Then fix broken tests.

Signed-off-by: Ed Santiago santiago@redhat.com

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 6, 2023
@edsantiago edsantiago mentioned this pull request Nov 6, 2023
Merged
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit ad99ec81efaa41c710dab2cab87e168d9ad7adec. @martinpitt, @jelly, @mvollmer please check.

@edsantiago
Copy link
Member Author

All sorts of failures with zstd in the message. @flouthoc @rhatdan PTAL. This is a VM-only change, not a real PR.

@rhatdan
Copy link
Member

rhatdan commented Nov 6, 2023

@giuseppe PTAL

@flouthoc
Copy link
Collaborator

flouthoc commented Nov 7, 2023 •
edited
Loading

Test was added by me, but I don't know in which PR the default compression is changed to zstd-chunked, tests expects default compression to be gzip. Is it changed in the VM itself ? otherwise this should have failed in the PR where this change was made. @giuseppe Any ideas if default compression is changed to zstd-chunked for all compression and is this change on the VM image itself ?

@martinpitt
Copy link
Contributor

@flouthoc : that change was introduced in containers-common, via direct upload to Fedora/podman-next COPR, not via a PR. See #20611

@flouthoc
Copy link
Collaborator

flouthoc commented Nov 7, 2023

ah it is that rawhide now defaults to zstd-chunked

@rhatdan
Copy link
Member

rhatdan commented Nov 7, 2023

Yes someone has to fix the tests or figure out if zstd:chunked is broken. I am at KubeCon and have no time to investigate.

@flouthoc flouthoc mentioned this pull request Nov 7, 2023
Closed
@flouthoc
Copy link
Collaborator

flouthoc commented Nov 7, 2023

@rhatdan @edsantiago Created a PR #20617

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@edsantiago
Copy link
Member Author

I pulled in @giuseppe's #20617. I added my own commit with kludgy fixes to system tests. Everything is still broken.

Can we PLEASE roll back that zstd change?

@vrothberg
Copy link
Member

@rhatdan can we revert please? This is blocking feature work in #20356.

@edsantiago
Copy link
Member Author

Executive decision, I am forcing an override in the CI images. New VM build in progress.

@rhatdan
Copy link
Member

rhatdan commented Nov 9, 2023

Reverting this is just pushing it down the road. We need to figure these issues out and get tests to pass.

@edsantiago
Copy link
Member Author

Full consensus and agreement from the team (we discussed this in scrum). We just decided to get the problems ironed out first, and then change the default.

@vrothberg
Copy link
Member

Reverting this is just pushing it down the road. We need to figure these issues out and get tests to pass.

Yes, tests need to pass. But reverting is inevitable in my eyes since we can't just wait for somebody to do something at some point. Who's responsible for fixing it?

I think we assumed that tests will pass without actually testing it. There might be real issues that require more than massaging tests.

@giuseppe
Copy link
Member

tests pass with #20633

We need the famous fix for c/image as well as another one for blobcache. Only a few tests need to be adjusted as they assume gzip or the docker format is used.

I think the safest is to revert the change in Rawhide since it affects users and we know there are real issues at the moment (like the PR for c/image). Once we settle on that, we need to change the default in c/common and seal our CI with it.

@edsantiago edsantiago changed the title VM images: bump to 2023-11-06 VM images: bump to 2023-11-15 Nov 16, 2023
@edsantiago edsantiago mentioned this pull request Nov 16, 2023
Merged
@edsantiago
VM images: bump to 2023-11-16
d711c3f 
F39 released. Hoping for newer crun and pasta.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
pasta tests: remove some skips
23ead49 
Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Test fixes for debian
d2a4ec8 
Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago changed the title VM images: bump to 2023-11-15 VM images: bump to 2023-11-16 Nov 17, 2023
@edsantiago
Copy link
Member Author

@cevich, @containers/podman-maintainers PTAL. This is a bump to release f39 (up from beta), and to crun-1.11.2 on all Fedoras (debian uses runc so we don't care about crun). See package version list.

edsantiago
edsantiago commented Nov 17, 2023
View reviewed changes
test/e2e/run_test.go
@@ -1545,14 +1545,18 @@ VOLUME %s`, ALPINE, volPath, volPath)
container.WaitWithDefaultTimeout()
Expect(container).Should(Exit(0))
checkLines(container.OutputToStringArray())
Expect(container.ErrorToString()).To(ContainSubstring("Running scope as unit: "))
Copy link
Member Author

Choose a reason for hiding this comment

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

systemd/systemd@7693661

Luap99
Luap99 approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2023

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, Luap99, 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:
  • OWNERS [Luap99,edsantiago,rhatdan]

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

@openshift-merge-bot openshift-merge-bot bot merged commit 5c97e12 into containers:main Nov 17, 2023
93 of 94 checks passed
@edsantiago edsantiago deleted the bump_vm_images branch November 17, 2023 12:48
@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 Feb 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@rhatdan rhatdan

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@edsantiago @rhatdan @flouthoc @martinpitt @vrothberg @giuseppe @Luap99