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

[v4.6] Backports and updated release notes #19218

Merged
merged 34 commits into from
Jul 13, 2023

Conversation

ashley-cui
Copy link
Member

Bugfix backports and release notes. I think we're missing a couple more but I'll fold those into the release cut PR tomorrow.

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 Jul 12, 2023
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jul 12, 2023
@TomSweeneyRedHat
Copy link
Member

@ashley-cui what is here looks good. However, @vrothberg bumped c/common with this commit. I think we might want to at least vendor from the top of the 0.55 branch in c/common, if not make a new 0.55.2 and vendor that. I think either approach is fine for the RC, but we'll need to do a complete dance before final.

@ashley-cui
Copy link
Member Author

ashley-cui commented Jul 12, 2023

Got it, I'll drop that commit for now, I believe we're cutting a new common tomorrow so once that's done, I'll grab that commit and backport it with the version bump. Thanks @TomSweeneyRedHat !

@ashley-cui
Copy link
Member Author

ashley-cui commented Jul 12, 2023

Ugh, Read that in a hurry and didn't read it correctly. If I'm not mistaken, @vrothberg was planning to cut a new common once containers/common#1505 merges. I'll mark this as a draft for now, and once the new common is cut, I'll vendor it with this PR, then do the version bump in another PR tmr. Thanks!

Basically, this will be a draft until a new common is cut.

@ashley-cui ashley-cui marked this pull request as draft July 12, 2023 22:35
@ashley-cui ashley-cui marked this pull request as draft July 12, 2023 22:35
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023
@vrothberg
Copy link
Member

containers/common#1505 just merged, so I'll cut a new patch release and drop the info here once it's done :)

@vrothberg
Copy link
Member

Ready https://github.com/containers/common/releases/tag/v0.55.2 🏁

@vrothberg
Copy link
Member

@ashley-cui, can you also cherry-pick #19210?

ashley-cui and others added 17 commits July 13, 2023 09:04
Signed-off-by: Ashley Cui <[email protected]>
* add e2e test

Signed-off-by: danishprakash <[email protected]>
Handle more TOCTOUs operating on listed images.  Also pull in
containers/common/pull/1520 and containers/common/pull/1522 which do the
same on the internal layer tree.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2216700
Signed-off-by: Valentin Rothberg <[email protected]>
we were silently ignoring --device-cgroup-rule in rootless mode.  Make
sure an error is returned if the user tries to use it.

Closes: containers#18698

Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
This is limited to images that don't depend on complex cgroup or capability
setups but does cover enough functionality to be useful.

[NO NEW TESTS NEEDED]

Signed-off-by: Doug Rabson <[email protected]>
There was a huge cut and paste of mount options which were not constent
in parsing tmpfs, bind and volume mounts.  Consolidated into a single
function to guarantee all parse the same.

Fixes: containers#18995

Signed-off-by: Daniel J Walsh <[email protected]>
This is just useless noise and gets us closer to what
Docker returns.

Signed-off-by: Daniel J Walsh <[email protected]>
This adds define.BindOptions to declare the mount options for bind-like
mounts (nullfs on FreeBSD). Note: this mirrors identical declarations in
buildah and it may be preferable to use buildah's copies throughout
podman.

[NO NEW TESTS NEEDED]

Signed-off-by: Doug Rabson <[email protected]>
With PR containers#14167, the pod-level security Context ID are supported, while the markdown says it isn't.
This patch fixes it.

```
None
```

Signed-off-by: Fabian Wiesel <[email protected]>
When working on Linux emulation on FreeBSD, I assumed that
SpecGenerator.ImageOS was always populated from the image's OS value but
in fact, this value comes from the CLI --os flag if set, otherwise "".
This broke running FreeBSD native containers unless --os=freebsd was
also set. Fix the problem by getting the value from the image itself.

This is a strong incentive for me to complete a stalled project to enable
podman system tests on FreeBSD.

[NO NEW TESTS NEEDED]

Signed-off-by: Doug Rabson <[email protected]>
An empty range caused a panic as parseOptionIDs tried to check further
down for an @ at index 0 without taking into account that the splitted
out string could be empty.

Signed-off-by: Simon Brakhane <[email protected]>
Previously podman was using "MB" and "GB" (binary) for input but
"MB" and "GB" (decimal) for output, which was causing confusion.

Signed-off-by: Anders F Björklund <[email protected]>
Regarding "The command does not support more than one listening socket for the API service."
See this Podman source code: (a permalink into the main branch as of 2 July 2023)
https://github.com/containers/podman/blob/539be58163a1730af0d84b39fcde585983cd9925/cmd/podman/system/service_abi.go#L48-L50

Move up the paragraph "The REST API provided ...".

Move up the sentence "Note: The default systemd ...".

Signed-off-by: Erik Sjölund <[email protected]>
vrothberg and others added 11 commits July 13, 2023 09:05
Previous tests have worked by pure chance since the client and server
ran on the same host; the server picked up the credentials created by
the client login.

Extend the gating tests and add a new integration test which is further
capable of exercising the remote code.

Note that fixing authentication support requires adding a new
`--authfile` CLi flag to `manifest inspect`.  This will at least allow
for passing an authfile to be bindings.  Username and password are not
yet supported.

Signed-off-by: Valentin Rothberg <[email protected]>
This endpoint queried the same package versions twice causing it to be
slower than info. Because it already called info we can just reuse the
package versions from there.

Signed-off-by: Paul Holzinger <[email protected]>
Do not use podman info/version as they are expensive and clutter the log
for no reason. Just checking if we can connect to the socket should be
good enough and much faster.

Fix the non existing error checking, so that we actually see an useful
error when this does not work.

Also change the interval, why wait 2s for a retry lets take 100ms steps
instead.

Fixes containers#19010

Signed-off-by: Paul Holzinger <[email protected]>
Since we have sqlite there is no point in duplicating this acroos two db
backends. Just set earlier when we validate the networks anyway.

Signed-off-by: Paul Holzinger <[email protected]>
We use the name as alias but using the hostname makes also sense and
this is what docker does. We have to keep the short id as well for
docker compat.

While adding some tests I removed some duplicated tests that were
executed twice for nv for no reason.

Fixes containers#17370

Signed-off-by: Paul Holzinger <[email protected]>
The change to use the custom dns server in aardvark-dns caused a
regression here because macvlan networks never returned the nameservers
in netavark and it also does not make sense to do so.

Instead check here if we got any network nameservers, if not we then use
the ones from the config if set otherwise fallback to host servers.

Fixes containers#19169

Signed-off-by: Paul Holzinger <[email protected]>
When I reworked pod removal to provide more detailed errors
(including per-container errors, not just a single multierror
with all errors squashed), I made it part of the struct returned
by the REST API and assumed that would be enough to get errors
through to clients. Unfortunately, in case of an overarching
error removing the pod (as any error with any container would
cause), we don't send the response struct that would include the
container errors - we just send a standardized REST error. We
could work around this with custom, potentially backwards
incompatible error handling for the REST pod delete endpoint, or
we could just do what was done before, and package up all the
errors in a multierror to send to the other side. Of those
options, the multierror seems far simpler.

Fixes containers#19159

Signed-off-by: Matt Heon <[email protected]>
The --authfile flag has been ignored.  Fix that and add a test to make
sure we won't regress another time.  Requires a new --tls-verify flag
to actually test the code.

Also bump c/common since common/pull/1538 is required to correctly check
for updates.  Note that I had to use the go-mod-edit-replace trick on
c/common as c/buildah would otherwise be moved back to 1.30.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2218315
Signed-off-by: Valentin Rothberg <[email protected]>
Make sure that starting a qemu machine uses proper exponential backoffs
and that a single variable isn't shared across multiple backoffs.

DO NOT BACKPORT: I want to avoid backporting this PR to the upcoming 4.6
release as it increases the flakiness of machine start (see containers#17403). On
my M2 machine, the flake rate seems to have increased with this change
and I strongly suspect that additional/redundant sleep after waiting for
the machine to be running and listening reduced the flakiness.  My hope
is to have more predictable behavior and find the sources of the flakes
soon.

[NO NEW TESTS NEEDED] - still too flaky to add a test to CI.

Signed-off-by: Valentin Rothberg <[email protected]>
During the exponential backoff waiting for the machine to be fully up
and running, also make sure that SSH is ready.  The systemd dependencies
of the ready.service include the sshd.service among others but that is
not enough.

Other CoreOS users reported the same issue on IRC, so I feel fairly
confident to use the pragmatic approach of making sure SSH works on the
client side.  containers#17403 is quite old and there are other pressing machine
issues that need attention.

[NO NEW TESTS NEEDED]

Fixes: containers#17403
Signed-off-by: Valentin Rothberg <[email protected]>
@ashley-cui ashley-cui marked this pull request as ready for review July 13, 2023 13:58
@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 Jul 13, 2023
edsantiago and others added 2 commits July 13, 2023 10:49
The podman-login tests have accumulated much cruft over the
years, because that's the only place where we run a local
registry, and the process was crufty: we actually start/stopped
the registry as the first & last tests of the file. Meaning,
you couldn't do 'hack/bats 150:just-one-test' because that
would skip the registry start. And just now, a completely
unrelated test has had to be shoved into the login file.

This PR revamps the whole thing, by adding a new registry helper
module that can be used anywhere. And, once the registry is
started, it just stays running until the end of tests. (This
requires BATS 1.7 or greater).

Signed-off-by: Ed Santiago <[email protected]>
Signed-off-by: Ashley Cui <[email protected]>
@ashley-cui
Copy link
Member Author

@containers/podman-maintainers looks like this will go green, PTAL :)

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

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, vrothberg

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 [ashley-cui,vrothberg]

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

@ashley-cui
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2023
@openshift-merge-robot openshift-merge-robot merged commit ec7f775 into containers:v4.6 Jul 13, 2023
@ashley-cui ashley-cui deleted the rc2 branch September 28, 2023 22:04
@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 Dec 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 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. kind/api-change Change to remote API; merits scrutiny 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.