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

Bump to v3.4.2 #12274

Merged
merged 51 commits into from
Nov 12, 2021
Merged

Bump to v3.4.2 #12274

merged 51 commits into from
Nov 12, 2021

Conversation

mheon
Copy link
Member

@mheon mheon commented Nov 11, 2021

As the title says

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2021
@mheon
Copy link
Member Author

mheon commented Nov 11, 2021

Lot of small changes this time around - lot of commits but most are docs.

@rhatdan
Copy link
Member

rhatdan commented Nov 11, 2021

LGTM
If it passes tests.

@TomSweeneyRedHat
Copy link
Member

@mheon, couple red tests

@@ -389,7 +408,7 @@
- Fixed a bug where images with empty layers were stored incorrectly, causing them to be unable to be pushed or saved.
- Fixed a bug where the `podman rmi` command could fail to remove corrupt images from storage.
- Fixed a bug where the remote Podman client's `podman save` command did not support the `oci-dir` and `docker-dir` formats ([#9742](https://github.com/containers/podman/issues/9742)).
- Fixed a bug where volume mounts from `podman play kube` created with a trailing `/` in the container path were were not properly superceding named volumes from the image ([#9618](https://github.com/containers/podman/issues/9618)).
- Fixed a bug where volume mounts from `podman play kube` created with a trailing `/` in the container path were were not properly superseding named volumes from the image ([#9618](https://github.com/containers/podman/issues/9618)).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Fixed a bug where volume mounts from `podman play kube` created with a trailing `/` in the container path were were not properly superseding named volumes from the image ([#9618](https://github.com/containers/podman/issues/9618)).
- Fixed a bug where volume mounts from `podman play kube` created with a trailing `/` in the container path were were not properly superseding named volumes from the image ([#9618](https://github.com/containers/podman/issues/9618)).

Copy link
Member

Choose a reason for hiding this comment

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

The above fix has a "were were" in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are old release notes, so I don't want to change them beyond spelling fixes

Copy link
Member

Choose a reason for hiding this comment

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

Sure, get the tests to pass and we can merge.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks on the were were @rhatdan, thought I'd killed one in my comment.

@TomSweeneyRedHat
Copy link
Member

one nit and a whole lot of red unhappiness in the tests

@Luap99
Copy link
Member

Luap99 commented Nov 12, 2021

I think you have to drop 605c9e77289e2010c027758d003f101f12785584. The journald test will not pass without the required journald fixes which are only in the main branch because of breaking changes to the log endpoint.

@mheon
Copy link
Member Author

mheon commented Nov 12, 2021

We have a request to get that fix landed, so that could be problematic.

@Luap99
Copy link
Member

Luap99 commented Nov 12, 2021

I am able to get the test working with this diff:

diff --git a/libpod/container_log_linux.go b/libpod/container_log_linux.go
index 4029d0af7..b6b780bab 100644
--- a/libpod/container_log_linux.go
+++ b/libpod/container_log_linux.go
@@ -140,6 +140,7 @@ func (c *Container) readFromJournal(ctx context.Context, options *logs.LogOption
                        doTail = false
                }
                lastReadCursor := ""
+               partial := ""
                for {
                        select {
                        case <-ctx.Done():
@@ -229,6 +230,12 @@ func (c *Container) readFromJournal(ctx context.Context, options *logs.LogOption
                                logrus.Errorf("Failed parse log line: %v", err)
                                return
                        }
+                       if logLine.Partial() {
+                               partial += logLine.Msg
+                               continue
+                       }
+                       logLine.Msg = partial + logLine.Msg
+                       partial = ""
                        if doTail {
                                tailQueue = append(tailQueue, logLine)
                                continue

This is not a breaking change, it just combines the partial lines into one single line like it is done for the file backend:

if nll.Partial() {
partial += nll.Msg
continue
} else if !nll.Partial() && len(partial) > 0 {
nll.Msg = partial + nll.Msg
partial = ""
}

@mheon
Copy link
Member Author

mheon commented Nov 12, 2021

Force-pushed with fix - thanks bunches @Luap99

@mheon
Copy link
Member Author

mheon commented Nov 12, 2021

Looks good, integration tests are all passing.

@mheon
Copy link
Member Author

mheon commented Nov 12, 2021

System tests are failing, unrelated to earlier failures. I think I might have grabbed a patch from Ed that depends on functionality in system connection ls that isn't in this branch.

@mheon
Copy link
Member Author

mheon commented Nov 12, 2021

Pulled the patch out, repushed

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
stweil and others added 10 commits November 12, 2021 11:08
fuse-overlayfs is usually the package name.

Signed-off-by: Junichi Uekawa <[email protected]>
If podman uses Workdir="/" or the workdir specified in the image, it
should not add it to the yaml.
If Podman find environment variables in the image, they should not
get added to the yaml.

If the container or pod do not have changes to SELinux we should not
print seLinuxOpt{}

If the container or pod do not change any dns options the yaml should
not have a dnsOption={}

If the container is not privileged it should not have privileged=false
in the yaml.

Fixes: containers#11995

Signed-off-by: Daniel J Walsh <[email protected]>
If we interrupt pod removal between removing containers and
removing the whole pod, the infra ID was still in the DB, and
most pod operations would try to retrieve the infra container
(and would this fail). Clear the infra ID from the DB just before
we remove all containers to prevent this.

Fixes containers#12034

[NO NEW TESTS NEEDED] This is a very narrow race and I have no
idea how to repro it.

Signed-off-by: Matthew Heon <[email protected]>
...and fix one instance where there was no check

Signed-off-by: Ed Santiago <[email protected]>
Made changes so that if the pod contains all exited containers and only infra is running, remove the pod.

resolves containers#11713

Signed-off-by: cdoern <[email protected]>
When looking for a cursor that matches the first journal entry for a
given container, wait and try to find it using exponential backoff.

[NO NEW TESTS NEEDED]

Signed-off-by: Nalin Dahyabhai <[email protected]>
- change the type to forking to allow fork.
- add default.target for user systemd service

Signed-off-by: Easton Man <[email protected]>
On Docker this is ignored, and it should be on Podman as
well. This is documented in the man page.

Fixes: containers#12002

Signed-off-by: Daniel J Walsh <[email protected]>
machacekondra and others added 22 commits November 12, 2021 11:08
This PR fixes the case when the API return HTTP 409 response. Where the
API return the body format different then for other HTTP error codes.

Signed-off-by: Ondra Machacek <[email protected]>
Should not change behavior, just to set a consistent
precedent for code introduced in future commits.

Signed-off-by: Miloslav Trmač <[email protected]>
... to include all fields.

Signed-off-by: Miloslav Trmač <[email protected]>
Instead using the OS-wide system default policy, use
the one in this repo, and adjust the expected results
(as well as making the test stricter).

Signed-off-by: Miloslav Trmač <[email protected]>
the --cgroups=split test changes the current cgroup as it creates a
sub-cgroup.  This can cause a race condition in tests that are reading
the current cgroup.

Closes: containers#11191

Signed-off-by: Giuseppe Scrivano <[email protected]>
The returned error was not checked, thus the test could hang forever
since it blocks on the log channel.

Also handle unexpectedEOF like EOF.

Fixes containers#12176

Signed-off-by: Paul Holzinger <[email protected]>
Descriptions of flags don't need to start with whitespace of their own.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Only log API access entries when --log-level set to Info or below.

Fixes containers#12181

Signed-off-by: Jhon Honce <[email protected]>
A comment was made on internal mailing list about confusion on SELinux
labeling of volumes. This PR makes it a little more clear about when
you should or should not relabel.

We need a similar comment in podman pod create, but it does not support
--security-opt processing yet.

Signed-off-by: Daniel J Walsh <[email protected]>
Address the TOCTOU when generating random names by having at most 10
attempts to assign a random name when creating a pod or container.

[NO TESTS NEEDED] since I do not know a way to force a conflict with
randomly generated names in a reasonable time frame.

Fixes: containers#11735
Signed-off-by: Valentin Rothberg <[email protected]>
- remove 'NO TESTS NEEDED' as a valid bypass string. Henceforth
  only 'NO NEW TESTS NEEDED' will work.

- add a debugging aid for containers#11871, in which bodhi tests time out
  in nslookup.

Signed-off-by: Ed Santiago <[email protected]>
When starting a container libpod/runtime_pod_linux.go:NewPod calls
libpod/lock/lock.go:AllocateLock ends up in here.  If you exceed
num_locks, in response to a "podman run ..." you will see:

 Error: error allocating lock for new container: no space left on device

As noted inline, this error is technically true as it is talking about
the SHM area, but for anyone who has not dug into the source (i.e. me,
before a few hours ago :) your initial thought is going to be that
your disk is full.  I spent quite a bit of time trying to diagnose
what disk, partition, overlay, etc. was filling up before I realised
this was actually due to leaking from failing containers.

This overrides this case to give a more explicit message that
hopefully puts people on the right track to fixing this faster.  You
will now see:

 $ ./bin/podman run --rm -it fedora bash
 Error: error allocating lock for new container: allocation failed; exceeded num_locks (20)

[NO NEW TESTS NEEDED] (just changes an existing error message)

Signed-off-by: Ian Wienand <[email protected]>
Ensure that rebuilds happen when .c files are updated in the source
tree.

Signed-off-by: Ian Wienand <[email protected]>
We now do not copy the `bin` directory to the target nix sources to
avoid skipping the build because "everything is up to date".

Fixes containers#12198

Signed-off-by: Sascha Grunert <[email protected]>
[NO NEW TESTS NEEDED]

Signed-off-by: Boaz Shuster <[email protected]>
Podman and Docker will not commit changes via RUN command
of a VOLUME directory, so we need to chown path first.

Not doing do will cause: https://bugzilla.redhat.com/show_bug.cgi?id=2009266

Signed-off-by: Jindrich Novy <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
The cni plugins need access to /run/cni and the dnsname plugin needs
access to /run/containers.

The race condition was basically that a `podman stop` could either do the
cleanup itself or the spawned cleanup process would do the cleanup if it
was fast enough. The `podman stop` is executed on the host while the
podman cleanup process is executed in the "parent container". The parent
container contains older plugins than on the host. The dnsname plugin
before version 1.3 could error and this would prevent CNI from
doing a proper cleanup. The plugin errors because it could not find its
files in /run/containers. On my system the test always failed because
the cleanup process was always faster than the stop process. However in
the CI VMs the stop process was usually faster and so it failed only
sometimes.

Fixes containers#11558

Signed-off-by: Paul Holzinger <[email protected]>
Patch originally by Paul Holzinger (sourced from [1]).

This is necessary to get the tests to pass in order to include a
batch of other, related journald fixes in `podman logs`.

[1] containers#12274 (comment)

Signed-off-by: Matthew Heon <[email protected]>
Signed-off-by: Matthew Heon <[email protected]>
Signed-off-by: Matthew Heon <[email protected]>
Signed-off-by: Matthew Heon <[email protected]>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
@rhatdan
Copy link
Member

rhatdan commented Nov 12, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4808a63 into containers:v3.4 Nov 12, 2021
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.