Fix Docker APIv2 container wait endpoint #9048

matejvasek · 2021-01-21T02:46:04Z

resolves #8697
resolves #9157

container wait functions are now cancel-able via context.Context
container wait functions can wait for multiple states, not just single one
docker container wait endpoint now accepts docker conditions: not-running, removed, next-exit
docker container wait endpoint now sends headers in advance before actual waiting

pkg/api/handlers/utils/containers.go

matejvasek · 2021-01-27T19:41:43Z

@mheon PTAL

matejvasek · 2021-01-27T19:44:03Z

test/apiv2/26-containersWait.at

+
+t POST "containers/nonExistent/wait?condition=next-exit" '' 404
+
+podman create --name "${CTR}" --entrypoint '["sleep", "0.5"]' "${IMAGE}"


I wish I could have just --entrypoint '["true"]' here, however such a container exits too fast. 😞

meaning the container is in running state for such a short duration we are not capable of detecting that

matejvasek · 2021-01-27T19:44:43Z

pkg/api/handlers/utils/containers.go

+}
+
+func waitRemoved(ctx context.Context, con *libpod.Container, interval time.Duration) (int32, error) {
+	code, err := con.WaitForConditionWithInterval(ctx, interval, define.ContainerStateUnknown)


Here I am not waiting for specific state but for an error.

mheon · 2021-01-27T19:49:45Z

@rhatdan I think this conflicts with some of the work you've been doing, and I think I prefer this version - it enables more functionality that we don't have yet.

rhatdan · 2021-01-27T20:22:35Z

I disagree, I am fine with most of this, but it should all be implemented in the standard API code. We should not have three different ways of doing the same function. Too much risk of them drifting, as I have already seen with every single interface I have looked at.

matejvasek · 2021-01-27T20:29:18Z

@rhatdan do you mean I should add the three new condition to the define.ContainerStatus "enum" and then handle it in WaitForConditionWithInterval() ? What you are working on?

mheon · 2021-01-27T20:31:26Z

@rhatdan I disagree. Our API should not support the Docker-specific constants or the specifics of their operation mode. We can try and unify these but it's going to be a massive pain of conditional code

rhatdan · 2021-01-29T14:43:06Z

My point being that podman-remote wait and podman wait should ultimately call into pkg/domain/infra/abi/ wait function.
If Docker API needs some fancy handling, I am fine with adding it.
I would like to get this merged along with the Stop/Remove/Wait patch that I have going into todays release.

mheon · 2021-01-29T17:10:43Z

@rhatdan I don't believe the way Docker's Wait and our Wait work are compatible at this point. The handling for the custom condition codes is fundamentally different from the way our Wait code functions.

matejvasek · 2021-01-29T17:16:05Z

I think that really different is next-exit condition. Seems to me that in podman condition == state. This is not true for next-exit since it describes transition between two states instead of one particular state.

matejvasek · 2021-01-29T17:19:38Z

Condition not-running is not that different what podman has. As for removed, there is no such a state since the container no longer exists 😄 , so I am basically waiting for define.ErrNoSuchCtr.

matejvasek · 2021-01-29T18:00:06Z

Note that next-exit may fail for short living containers (even if polling interval is 1ns).

matejvasek · 2021-01-29T18:03:44Z

I did experiment with condition variables it the shared memory as replacement for polling, but results weren't satisfactory.

matejvasek · 2021-01-30T00:18:34Z

To make next-exit truly reliable we probably need to introduce some better inter-process notification mechanism. Any ideas what we could use?

rhatdan · 2021-01-30T12:03:32Z

Let's get #9051 in and then rebase this on it.
Please review ^^

rhatdan · 2021-01-30T12:03:46Z

/approve

openshift-ci-robot · 2021-01-30T12:03:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek, 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~~ [rhatdan]

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

Signed-off-by: Matej Vasek <[email protected]>

matejvasek · 2021-02-04T13:09:46Z

pkg/bindings/containers/types.go

@@ -176,7 +176,7 @@ type UnpauseOptions struct{}
 //go:generate go run ../generator/generator.go WaitOptions
 // WaitOptions are optional options for waiting on containers
 type WaitOptions struct {
-	Condition *define.ContainerStatus
+	Condition []define.ContainerStatus


I deliberately choose not to change this to plural for preserving some degree of compatibility.
With this, older client can call new server safely. Similarly new client can call older server, but only as long as the condition contains at most one element.

If I understand generator correctly uri param is derived just from member name. Maybe we can introduce some custom tag. So it could look like:

type WaitOptions struct { Conditions []define.ContainerStatus `uriparam:"condition"` Interval *string `uriparam:"interval"` }

Meaning the name is changed, but on wire it's the same.

matejvasek · 2021-02-04T13:20:58Z

libpod/container_api.go

@@ -478,13 +479,15 @@ func (c *Container) RemoveArtifact(name string) error {
 }

 // Wait blocks until the container exits and returns its exit code.
-func (c *Container) Wait() (int32, error) {
-	return c.WaitWithInterval(DefaultWaitInterval)
+func (c *Container) Wait(ctx context.Context) (int32, error) {


I made the wait functions cancel-able. Useful for not leaking gorutines when client cancels request (closes connection abruptly). For CLI podman it's not used since context passed here is not canceled on SIGTERM or SIGINT. CLI chooses IMO rather brutal os.Exit().

libpod/container_api.go

mheon · 2021-02-04T14:36:44Z

libpod/container_api.go

@@ -495,41 +498,111 @@ func (c *Container) WaitWithInterval(waitTimeout time.Duration) (int32, error) {
 	}
 	chWait := make(chan error, 1)

-	defer close(chWait)


It seems like this could still be useful, given that ctx.Done() is a goroutine and not guaranteed to fire at the end of the function, but rather when the entire handler returns, AFAIK

Probably shouldn't hurt, but also it's not necessary to close the channel here.

mheon · 2021-02-04T14:39:54Z

libpod/container_api.go

+
+	var wg sync.WaitGroup
+
+	if waitForExit {


It seems like both this and the len(wantedStates) > 0 below can both be active at the same time - it looks like we don't need that, though, that goroutine seems to take care of the same cases this one does. Should we only do this waitForExit bit if the user is only waiting for Stopped and Exited, and return immediately after?

As of now in practice only one is active. I future they might be both active. Waiting for terminal state differs from non-terminal. For instance if user condition is [Stopped, Removing], both gorutines have to be active. One is waiting for Stopped and the other one is waiting for Removing.

The len(wantedStates) > 0 gorutine is not taking care of non-terminal states.

mheon · 2021-02-04T14:41:30Z

Done reviewing. @rhatdan @jwhonce PTAL

Signed-off-by: Matej Vasek <[email protected]>

mheon · 2021-02-04T18:18:27Z

@containers/podman-maintainers PTAL

rhatdan · 2021-02-04T20:25:57Z

Great work @matejvasek Thanks.
/lgtm

rhatdan · 2021-02-05T09:33:19Z

/hold cancel

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2021

mheon reviewed Jan 21, 2021

View reviewed changes

pkg/api/handlers/utils/containers.go Outdated Show resolved Hide resolved

mheon reviewed Jan 21, 2021

View reviewed changes

pkg/api/handlers/utils/containers.go Outdated Show resolved Hide resolved

matejvasek force-pushed the apiv2_wait branch 3 times, most recently from bbf73df to ce81023 Compare January 27, 2021 19:23

matejvasek requested a review from mheon January 27, 2021 19:41

matejvasek changed the title ~~[WIP] APIv2 wait~~ APIv2 wait endpoint Jan 27, 2021

matejvasek marked this pull request as ready for review January 27, 2021 19:42

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2021

matejvasek commented Jan 27, 2021

View reviewed changes

mheon mentioned this pull request Jan 29, 2021

podman-remote returns invalid information to docker #9157

Closed

rhatdan added the Backport to v3.0 label Jan 30, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2021

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2021

matejvasek force-pushed the apiv2_wait branch from eec5e06 to 208a07a Compare February 3, 2021 20:17

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2021

matejvasek added 2 commits February 3, 2021 21:49

Improve container libpod.Wait*() functions

570e158

Signed-off-by: Matej Vasek <[email protected]>

Improve ContainerEngine.ContainerWait()

fc38580

Signed-off-by: Matej Vasek <[email protected]>

matejvasek force-pushed the apiv2_wait branch from f104da7 to 11ef933 Compare February 3, 2021 20:50

matejvasek added 3 commits February 3, 2021 22:10

Implement Docker wait conditions

4a219aa

Signed-off-by: Matej Vasek <[email protected]>

Add test for Docker APIv2 wait

3c57bc8

Signed-off-by: Matej Vasek <[email protected]>

Increase timeouts in some tests

2b8d6ca

Signed-off-by: Matej Vasek <[email protected]>

matejvasek force-pushed the apiv2_wait branch from 11ef933 to 2b8d6ca Compare February 3, 2021 21:10

matejvasek changed the title ~~APIv2 wait endpoint~~ Fix Docker APIv2 container wait endpoint Feb 4, 2021

matejvasek commented Feb 4, 2021

View reviewed changes

mheon reviewed Feb 4, 2021

View reviewed changes

libpod/container_api.go Outdated Show resolved Hide resolved

mheon reviewed Feb 4, 2021

View reviewed changes

Fix per review request

05444cb

Signed-off-by: Matej Vasek <[email protected]>

openshift-ci-robot assigned rhatdan Feb 4, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021

openshift-merge-robot merged commit 42d4652 into containers:master Feb 5, 2021

mheon mentioned this pull request Feb 8, 2021

[v3.0] Backport final breaking API change to v3.0 #9273

Merged

edigaryev mentioned this pull request Feb 12, 2021

API: container wait endpoint regression in 3.0.0 #9351

Closed

matejvasek mentioned this pull request Feb 17, 2021

API: fix libpod's container wait endpoint condition conversion #9414

Merged

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 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Docker APIv2 container wait endpoint #9048

Fix Docker APIv2 container wait endpoint #9048

matejvasek commented Jan 21, 2021 •

edited

Loading

matejvasek commented Jan 27, 2021

matejvasek Jan 27, 2021

matejvasek Jan 27, 2021

matejvasek Jan 27, 2021

mheon commented Jan 27, 2021

rhatdan commented Jan 27, 2021

matejvasek commented Jan 27, 2021 •

edited

Loading

mheon commented Jan 27, 2021

rhatdan commented Jan 29, 2021

mheon commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 30, 2021

rhatdan commented Jan 30, 2021

rhatdan commented Jan 30, 2021

openshift-ci-robot commented Jan 30, 2021

matejvasek Feb 4, 2021 •

edited

Loading

matejvasek Feb 4, 2021 •

edited

Loading

matejvasek Feb 4, 2021

mheon Feb 4, 2021

matejvasek Feb 4, 2021 •

edited

Loading

mheon Feb 4, 2021

matejvasek Feb 4, 2021

matejvasek Feb 4, 2021

mheon commented Feb 4, 2021

mheon commented Feb 4, 2021

rhatdan commented Feb 4, 2021

rhatdan commented Feb 5, 2021


		t POST "containers/nonExistent/wait?condition=next-exit" '' 404

		podman create --name "${CTR}" --entrypoint '["sleep", "0.5"]' "${IMAGE}"

Fix Docker APIv2 container wait endpoint #9048

Fix Docker APIv2 container wait endpoint #9048

Conversation

matejvasek commented Jan 21, 2021 • edited Loading

matejvasek commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jan 27, 2021

rhatdan commented Jan 27, 2021

matejvasek commented Jan 27, 2021 • edited Loading

mheon commented Jan 27, 2021

rhatdan commented Jan 29, 2021

mheon commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 29, 2021

matejvasek commented Jan 30, 2021

rhatdan commented Jan 30, 2021

rhatdan commented Jan 30, 2021

openshift-ci-robot commented Jan 30, 2021

matejvasek Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

matejvasek Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejvasek Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Feb 4, 2021

mheon commented Feb 4, 2021

rhatdan commented Feb 4, 2021

rhatdan commented Feb 5, 2021

matejvasek commented Jan 21, 2021 •

edited

Loading

matejvasek commented Jan 27, 2021 •

edited

Loading

matejvasek Feb 4, 2021 •

edited

Loading

matejvasek Feb 4, 2021 •

edited

Loading

matejvasek Feb 4, 2021 •

edited

Loading