Switch podman stop/kill/wait handlers to use abi #9051

rhatdan · 2021-01-21T12:47:42Z

Change API Handlers to use the same functions that the
local podman uses.

At the same time:

implement remote API for --all and --ignore flags for podman stop
implement remote API for --all flags for podman stop

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2021-01-21T12:47:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

rhatdan · 2021-01-21T12:48:17Z

@edsantiago more missing podman-remote options and missing tests.

vrothberg

LGTM

vrothberg · 2021-01-21T13:21:46Z

make generate-bindings isn't happy in CI

vrothberg · 2021-01-21T13:28:41Z

Get the same locally after make generate-bindings:

podman (rm) $ git diff
diff --git a/pkg/bindings/containers/types_stop_options.go b/pkg/bindings/containers/types_stop_options.go
index c86cc2f9ffa9..d4e2eda8383c 100644
--- a/pkg/bindings/containers/types_stop_options.go
+++ b/pkg/bindings/containers/types_stop_options.go
@@ -119,7 +119,7 @@ func (o *StopOptions) GetIgnore() bool {
        return *o.Ignore
 }

-// WithToimeout
+// WithTimeout
 func (o *StopOptions) WithTimeout(value uint) *StopOptions {
        v := &value

mheon · 2021-01-21T16:12:46Z

pkg/api/handlers/compat/containers.go


-	// If the Container is stopped already, send a 409
-	if state == define.ContainerStateStopped || state == define.ContainerStateExited {
-		utils.Error(w, fmt.Sprintf("Container %s is not running", name), http.StatusConflict, errors.New(fmt.Sprintf("Cannot kill Container %s, it is not running", name)))


We need to retain some form of this check so we can return StatusConflict

I think you can check if ContainerKill returned either ErrCtrStateInvalid or ErrCtrStopped though

mheon · 2021-01-21T16:13:59Z

pkg/api/handlers/compat/containers.go

 		return
 	}

-	// Docker waits for the container to stop if the signal is 0 or
-	// SIGKILL.
-	if !utils.IsLibpodRequest(r) && (signal == 0 || syscall.Signal(signal) == syscall.SIGKILL) {


I think we need to retain this.

mheon · 2021-01-21T16:14:40Z

pkg/api/handlers/compat/containers_stop.go

 	}
-	// If the Container is stopped already, send a 304
-	if state == define.ContainerStateStopped || state == define.ContainerStateExited {


Need to retain this check so we can return StatusNotModified

Yuck, Fixed.

- stop: test --all and --ignore (containers#9051) - build: test /run/secrets (containers#8679, but see below) - sensitive mount points: deal with 'stat' failures - selinux: confirm useful diagnostics on unknown labels (containers#8946) The 'build' test is intended as a fix for containers#8679, in which 'podman build' does not mount secrets from mounts.conf. Unfortunately, as of this writing, 'podman build' does not pass the --default-mounts-file option to buildah, so there's no reasonable way to test this path. Still, we can at least confirm /run/secrets on 'podman run'. The /sys thing is related to containers#8949: RHEL8, rootless, cgroups v1. It's just a workaround to get gating tests to pass on RHEL. Signed-off-by: Ed Santiago <[email protected]>

mheon · 2021-01-25T21:19:24Z

pkg/api/handlers/compat/containers.go

-	err = con.Kill(signal)
-	if err != nil {
-		utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrapf(err, "unable to kill Container %s", name))
+	if report[0].Err != nil {


We can't do this in cases where more than one result can be returned (e.g. All is specified). You are going to lose errors on everything except the first container, which is HORRIBLE user experience - we've had to deal with this in Pod commands and play kube and it makes debugging user issues impossible.

This is going to require a change in return type to return a struct containing all the errors, which will make it incompatible with Docker. I suppose we could use the magic check that determines if this is a Libpod or Compat request to handle it?

(As an aside, this is why we decided to handle All client side, not server side, initially - it makes things so much simpler because we can re-use handlers and not worry about this).

Removed all, although I think in the long run this should be changed.

- stop: test --all and --ignore (containers#9051) - build: test /run/secrets (containers#8679, but see below) - sensitive mount points: deal with 'stat' failures - selinux: confirm useful diagnostics on unknown labels (containers#8946) The 'build' test is intended as a fix for containers#8679, in which 'podman build' does not mount secrets from mounts.conf. Unfortunately, as of this writing, 'podman build' does not pass the --default-mounts-file option to buildah, so there's no reasonable way to test this path. Still, we can at least confirm /run/secrets on 'podman run'. The /sys thing is related to containers#8949: RHEL8, rootless, cgroups v1. It's just a workaround to get gating tests to pass on RHEL. Signed-off-by: Ed Santiago <[email protected]>

mheon · 2021-01-28T19:43:32Z

pkg/api/handlers/compat/containers.go

@@ -242,6 +248,10 @@ func WaitContainer(w http.ResponseWriter, r *http.Request) {
 	// /{version}/containers/(name)/wait
 	exitCode, err := utils.WaitContainer(w, r)
 	if err != nil {
+		if errors.Cause(err) == define.ErrNoSuchCtr {


Is this necessary? Waiting on a container that has been removed by --rm will error now

Do you think we should at least log it. Or just ignore it.

mheon · 2021-01-28T19:44:18Z

pkg/api/handlers/compat/containers_restart.go

-	if _, found := r.URL.Query()["t"]; found {
-		timeout = uint(query.Timeout)
+	options := entities.RestartOptions{
+		All:     query.All,


All is still hazardous to use, we're going to lose every error except the first one.

I hope to fix this in the future. But it will be a breaking change in the API.

mheon · 2021-01-28T19:47:52Z

pkg/api/handlers/libpod/containers.go

@@ -148,6 +148,11 @@ func GetContainer(w http.ResponseWriter, r *http.Request) {
 func WaitContainer(w http.ResponseWriter, r *http.Request) {
 	exitCode, err := utils.WaitContainer(w, r)
 	if err != nil {
+		if errors.Cause(err) == define.ErrNoSuchCtr {
+			utils.ContainerNotFound(w, utils.GetName(r), err)


Hm. Should we error here? My concern is --rm - we could race against cleanup and error on a container being removed.

So just log it?

Yeah, logging seems right. Maybe someone will complain in the future and we'll have to do better, but for now this seems safest

mheon · 2021-01-28T19:48:30Z

pkg/api/handlers/utils/containers.go

 		return 0, err
 	}
-	return con.WaitForConditionWithInterval(interval, condition)
+	return report[0].ExitCode, report[0].Error


Should double-check len(report) > 0 - better safe than segfaulting

Change API Handlers to use the same functions that the local podman uses. At the same time: implement remote API for --all and --ignore flags for podman stop implement remote API for --all flags for podman stop Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2021-01-30T11:51:14Z

@containers/podman-maintainers PTAL

vrothberg

LGTM

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

vrothberg reviewed Jan 21, 2021

View reviewed changes

rhatdan force-pushed the rm branch from 722cf4d to 129e6d9 Compare January 21, 2021 15:59

mheon reviewed Jan 21, 2021

View reviewed changes

rhatdan force-pushed the rm branch 10 times, most recently from 575343f to 67c366d Compare January 24, 2021 15:43

edsantiago mentioned this pull request Jan 25, 2021

System tests: cover gaps from the last month #9091

Merged

rhatdan force-pushed the rm branch from 67c366d to dbe5911 Compare January 25, 2021 21:03

mheon reviewed Jan 25, 2021

View reviewed changes

rhatdan force-pushed the rm branch 3 times, most recently from 2ff8ace to f3be54a Compare January 25, 2021 23:20

rhatdan changed the title ~~Switch podman stop/kill handlers to use abi~~ Switch podman stop/kill/wait handlers to use abi Jan 25, 2021

rhatdan force-pushed the rm branch 2 times, most recently from 85ccccb to 3bf7658 Compare January 26, 2021 11:18

rhatdan force-pushed the rm branch 5 times, most recently from 3f6e657 to 5574a89 Compare January 26, 2021 22:23

rhatdan force-pushed the rm branch 3 times, most recently from f19a008 to 4bd24c0 Compare January 27, 2021 20:02

mheon reviewed Jan 28, 2021

View reviewed changes

rhatdan force-pushed the rm branch from 4bd24c0 to e790069 Compare January 29, 2021 14:02

rhatdan force-pushed the rm branch from e790069 to 073f76c Compare January 29, 2021 19:05

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

rhatdan mentioned this pull request Jan 30, 2021

Fix Docker APIv2 container wait endpoint #9048

Merged

vrothberg reviewed Feb 1, 2021

View reviewed changes

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2021

openshift-merge-robot merged commit 81a3ba3 into containers:master Feb 1, 2021

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

Switch podman stop/kill/wait handlers to use abi #9051

Switch podman stop/kill/wait handlers to use abi #9051

rhatdan commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

rhatdan commented Jan 21, 2021

vrothberg left a comment

vrothberg commented Jan 21, 2021

vrothberg commented Jan 21, 2021

mheon Jan 21, 2021

mheon Jan 21, 2021

mheon Jan 21, 2021

rhatdan Jan 26, 2021

mheon Jan 21, 2021

rhatdan Jan 26, 2021

mheon Jan 25, 2021

mheon Jan 25, 2021

rhatdan Jan 26, 2021

mheon Jan 28, 2021

rhatdan Jan 28, 2021

mheon Jan 28, 2021

rhatdan Jan 28, 2021

mheon Jan 28, 2021

rhatdan Jan 28, 2021

mheon Jan 28, 2021

mheon Jan 28, 2021

rhatdan commented Jan 30, 2021

vrothberg left a comment

Switch podman stop/kill/wait handlers to use abi #9051

Switch podman stop/kill/wait handlers to use abi #9051

Conversation

rhatdan commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

rhatdan commented Jan 21, 2021

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Jan 21, 2021

vrothberg commented Jan 21, 2021

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

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

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

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

Choose a reason for hiding this comment

rhatdan commented Jan 30, 2021

vrothberg left a comment

Choose a reason for hiding this comment