podman kube play --replace should force removal of pods and containers #20457

rhatdan · 2023-10-23T20:23:19Z

Fixes: #20025

Does this PR introduce a user-facing change?

None

[NO NEW TESTS NEEDED]

vrothberg

Please split the doc changes into a separate commit.

Looking purely at the changes, I don't think it's sufficient to fix #20025? Please add a regression test to make sure it does. The problem in #20025 is that "external" containers are not being removed/replaced, so we could probably do a buildah from --name=$expected-name and use that name in a kube YAML to cause a name collision.

vrothberg · 2023-10-24T06:23:11Z

pkg/bindings/kube/types.go

@@ -42,6 +42,8 @@ type PlayOptions struct {
 	LogDriver *string
 	// LogOptions for the container. For example: journald
 	LogOptions *[]string
+	// Replace - replace existing pods and containers
+	Replace *bool


That screams for tests. It seems kube play --replace is not tested on the remote clients or the tests are incomplete.

It was being ignored on remote, since it was never setup to be passed in to remote side.

pkg/domain/infra/abi/play.go

TomSweeneyRedHat · 2023-10-25T20:27:25Z

pkg/api/handlers/libpod/kube.go

 		StaticIPs:          staticIPs,
 		StaticMACs:         staticMACs,
-		IsRemote:           true,
+		UseLongAnnotations: query.NoTrunc,
+		Username:           username,
 		Userns:             query.Userns,


I like the alpha ordering, but man, GitHub doesn't make it easy to review.

IMO such changes should be part of a separate commit. That'll reduce the noise.

TomSweeneyRedHat · 2023-10-25T20:32:37Z

LGTM

vrothberg

Code LGTM

I'd prefer @edsantiago's blessing on the test.

vrothberg · 2023-10-26T06:41:04Z

pkg/api/handlers/libpod/kube.go

 		StaticIPs:          staticIPs,
 		StaticMACs:         staticMACs,
-		IsRemote:           true,
+		UseLongAnnotations: query.NoTrunc,
+		Username:           username,
 		Userns:             query.Userns,


IMO such changes should be part of a separate commit. That'll reduce the noise.

vrothberg · 2023-10-26T06:42:43Z

test/system/700-play.bats

+    # Force removal of container
+    run_podman rm --force -t0 test_pod-test
+    # Create container external using buildah with same name
+    buildah from --name test_pod-test $IMAGE


This probably needs a run buildah and status checks.

Not really. buildah here is much better than run buildah.

Reason: run is tricky. It hides exit status and output. In this case here, if plain buildah fails, BATS will immediately fail and will show all stdout/stderr. If you run buildah, though, you then need to add cumbersome if $status blah checks, and then will miss stdout/stderr if you check in the wrong order.

edsantiago · 2023-10-26T12:16:27Z

test/system/700-play.bats

+    run_podman play kube $PODMAN_TMPDIR/test.yaml
+    # Force removal of container
+    run_podman rm --force -t0 test_pod-test
+    # Create container external using buildah with same name


However, I do find container external confusing. Should that be external container?

edsantiago · 2023-10-26T12:18:21Z

test/system/700-play.bats

@@ -411,6 +411,23 @@ _EOF
    run_podman rmi -f userimage:latest
 }

+@test "podman kube play --replace external storage" {


Also, as a general rule, when a test is super-confusing (like this one, WTF is it supposed to test) I like to add header comments like "Regression test for #12345; an external container, such as from buildah, caused a "container name is already in use" failure.

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

Fixes: containers#20025 Signed-off-by: Daniel J Walsh <[email protected]>

edsantiago · 2023-10-30T11:21:21Z

test/system/700-play.bats

+# Ocassionaly a remnant storage container is left behind which causes
+# podman play kube --replace to fail. This tests created a conflicting
+# storage container name using buildah to make sure --replace, still
+# functions proplery by removing the storage container.


Typos (Occasionally, properly) and unclear wording ("tests created"), but the former will be fixed some day on the next spellcheck PR, and the latter is not worth a repush. (But if you have to repush for other reasons, please fix).

LGTM

vrothberg

/lgtm

openshift-ci · 2023-10-30T11:45:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

rhatdan · 2023-10-30T13:38:51Z

Seems to work.

$ buildah from --name dan alpine
dan
$ podman run --name dan alpine echo hi
Error: creating container storage: the container name "dan" is already in use by d296206717c64cd5c2988f3aabcf89cf38e7a2010ea52f083c5fd63d7398fbeb. You have to remove that container to be able to reuse that name: that name is already in use
$ podman run --replace --name dan alpine echo hi
dan
hi

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 23, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 23, 2023

vrothberg reviewed Oct 24, 2023

View reviewed changes

rhatdan force-pushed the pod branch 2 times, most recently from d152b6e to 83a097c Compare October 25, 2023 11:38

TomSweeneyRedHat reviewed Oct 25, 2023

View reviewed changes

vrothberg approved these changes Oct 26, 2023

View reviewed changes

edsantiago reviewed Oct 26, 2023

View reviewed changes

rhatdan added 2 commits October 27, 2023 07:25

Sort kube options alphabetically

3ce62d3

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

podman kube play --replace should force removal of pods and containers

91df369

Fixes: containers#20025 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the pod branch from 83a097c to 91df369 Compare October 27, 2023 11:30

edsantiago reviewed Oct 30, 2023

View reviewed changes

vrothberg approved these changes Oct 30, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Oct 30, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2023

openshift-ci bot merged commit 4871182 into containers:main Oct 30, 2023
99 checks passed

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 Jan 29, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman kube play --replace should force removal of pods and containers #20457

podman kube play --replace should force removal of pods and containers #20457

rhatdan commented Oct 23, 2023

vrothberg left a comment

vrothberg Oct 24, 2023

rhatdan Oct 24, 2023

TomSweeneyRedHat Oct 25, 2023

vrothberg Oct 26, 2023

TomSweeneyRedHat commented Oct 25, 2023

vrothberg left a comment

vrothberg Oct 26, 2023

vrothberg Oct 26, 2023

edsantiago Oct 26, 2023

edsantiago Oct 26, 2023

edsantiago Oct 26, 2023

rhatdan Oct 27, 2023

edsantiago Oct 30, 2023

vrothberg left a comment

openshift-ci bot commented Oct 30, 2023

rhatdan commented Oct 30, 2023 •

edited

Loading

podman kube play --replace should force removal of pods and containers #20457

podman kube play --replace should force removal of pods and containers #20457

Conversation

rhatdan commented Oct 23, 2023

Does this PR introduce a user-facing change?

vrothberg left a 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

TomSweeneyRedHat commented Oct 25, 2023

vrothberg left a 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

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 30, 2023

rhatdan commented Oct 30, 2023 • edited Loading

rhatdan commented Oct 30, 2023 •

edited

Loading