Add podman rm --depend #12694

rhatdan · 2021-12-23T11:46:31Z

This option causes Podman to not only remove the specified containers
but all of the containers that depend on the specified
containers.
Fixes: #10360

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

openshift-ci · 2021-12-23T11:46:34Z

[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-12-23T11:54:33Z

@mheon @vrothberg @giuseppe PTAL
Please make sure I am not doing something stupid (Recursive loops) or if you have a better way to speed this up, I am all ears.

vrothberg

The performance looks good to me. We're first collecting all containers and can then partition them for parallel removal.

However, if we had to preserve the relative order and remove containers starting at the leafs, then it won't work as is.

I let @mheon weigh in

cmd/podman/containers/rm.go

vrothberg · 2021-12-23T13:23:02Z

libpod/runtime_ctr.go

@@ -915,6 +915,44 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol
 	return id, cleanupErr
 }

+// DependMap populates of map of all containers depending on the specified container


Suggested change

// DependMap populates of map of all containers depending on the specified container

// DependMap populates a map of all containers depending on the specified container

Comment seems unrelated to the actual function?

vrothberg · 2021-12-23T13:34:14Z

libpod/runtime_ctr.go

+// DependMap populates of map of all containers depending on the specified container
+func (r *Runtime) DependMap(ctx context.Context, ctr *Container, ctrMap map[string]*Container) error {
+	r.lock.RLock()
+	defer r.lock.RUnlock()


Do we really need to lock? I think we can read the data in any case.

Rewritten, and yes we will need the lock.

This is just the runtime lock, we need to take the container lock (c.lock.Lock()) instead

Also, the lock needs to be in removeDepend() - otherwise we don't take the lock when we recurse onto other containers.

Can probably avoid removeDepend entirely given this, runtime lock is superfluous

Please check the code again.

vrothberg · 2021-12-23T13:37:34Z

libpod/runtime_ctr.go

+		}
+		for _, cid := range ctr.Dependencies() {
+			if cid == searchCtr.ID() {
+				ctrMap[ctr.ID()] = ctr


Can we turn the map into map[string]string?

Currently there is a bug since we're using the loop variable. ctr will be added to the map but will change with the next iteration of the loop. That means all entries of ctr will point to the last item in allCtrs. Using a map[string]string will avoid that problem AND it forces callers to look up the containers another time before removal - they may have already been removed in the meantime.

vrothberg · 2021-12-23T13:38:55Z

pkg/bindings/containers/types.go

@@ -138,6 +138,7 @@ type PruneOptions struct {
 //go:generate go run ../generator/generator.go RemoveOptions
 // RemoveOptions are optional options for removing containers
 type RemoveOptions struct {
+	Depend  *bool


Also needs changes for the handlers and swagger.

mheon · 2021-12-23T14:12:21Z

docs/source/markdown/podman-rm.1.md

@@ -56,6 +60,11 @@ Remove a container by its name *mywebserver*
 $ podman rm mywebserver
 ```

+Remove a *mywebserver* container and all of the containers that depend on it
+```
+$ podman rm --depend mywebserver


Does this print every container that was removed? I got a little concerned because there are no changes to cmd/podman

Yes, the container is added to the reports list when being removed in the API. The Podman client just walks the list.

Added a test to verify.

mheon · 2021-12-23T14:15:03Z

pkg/api/handlers/compat/containers.go

@@ -36,6 +36,7 @@ func RemoveContainer(w http.ResponseWriter, r *http.Request) {
 	query := struct {
 		Force         bool  `schema:"force"`
 		Ignore        bool  `schema:"ignore"`
+		Depend        bool  `schema:"depend"`


I don't think we want this in the compat API, the return type won't make it clear that multiple containers were removed. Having it only in the fancy version that can already to multiple containers seems better?

It is only handled in libpod not in compat.

mheon · 2021-12-23T14:17:16Z

libpod/runtime_ctr.go

@@ -915,6 +915,44 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol
 	return id, cleanupErr
 }

+// DependMap populates of map of all containers depending on the specified container
+func (r *Runtime) DependMap(ctx context.Context, ctr *Container, ctrMap map[string]*Container) error {


Maybe change the signature to not accept a map[string]*Container but to allocate it here and return it instead? I don't think many people will call DependMap on partially-generated maps?

I am passing in the map with the list of all the containers currently added in the podman run command. This prevents us from having to lookup containers that are currently being deleted.

mheon · 2021-12-23T14:18:20Z

libpod/runtime_ctr.go

+		if ctrMap[ctr.ID()] != nil {
+			continue
+		}
+		for _, cid := range ctr.Dependencies() {


It should be safe to recurse onto the dependencies of the container, the DB has a built-in guarantee of no cycles.

mheon · 2021-12-23T14:20:17Z

pkg/domain/infra/abi/containers.go

@@ -357,6 +357,26 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 		return reports, nil
 	}

+	if !options.All && options.Depend {


Ordering seems a potential concern. Dependencies can have dependencies, and the order in which we remove them has to depend on that - it's more graph traversal than for loop. We may be able to reuse some of the pod code? Pod start builds a graph and traverses it to start containers (unfortunately, in the wrong direction)

I rewrote this to handle everything in libpod directly.

cdoern · 2021-12-23T16:21:36Z

libpod/runtime_ctr.go

+		if ctrMap[ctr.ID()] != nil {
+			continue
+		}
+		for _, cid := range ctr.Dependencies() {


if a container depends on infra within a pod, will this remove infra but keep the pod in existence? Could bring up some weird issues.

I think it'll just hard-fail because infra containers can't be removed except by removing their pod - so podman rm -f --depend infra_ctr should fail without doing anything

./bin/podman rm --depend 35965a814e0b
Error: container 35965a814e0ba301bc5d19bd1acaa1809af443a09c4d3f9ebacc02a454bc72f3 is the infra container of pod f72b7a1abf8f76a69e4a859542103db4184bcd7d07b7afaf73608eb6db870a0e and cannot be removed without removing the pod

rhatdan · 2021-12-23T16:49:35Z

./bin/podman rm --depend 35965a814e0b
Error: container 35965a814e0ba301bc5d19bd1acaa1809af443a09c4d3f9ebacc02a454bc72f3 is the infra container of pod f72b7a1abf8f76a69e4a859542103db4184bcd7d07b7afaf73608eb6db870a0e and cannot be removed without removing the pod

rhatdan · 2021-12-26T18:12:19Z

@vrothberg @mheon @cdoern PTAL
@containers/podman-maintainers PTAL

mheon · 2022-01-04T15:27:46Z

cmd/podman/root.go

@@ -137,7 +137,7 @@ func persistentPreRunE(cmd *cobra.Command, args []string) error {
 				runtimeFlag := cmd.Root().Flags().Lookup("runtime")
 				if runtimeFlag == nil {
 					return errors.Errorf(
-						"Unexcpected error setting runtime to '%s' for restore",
+						"Unexpected error setting runtime to '%s' for restore",


can you un-capitalize this? Go errors should be lowercased

mheon · 2022-01-04T16:01:02Z

pkg/api/handlers/compat/containers.go

@@ -57,6 +58,7 @@ func RemoveContainer(w http.ResponseWriter, r *http.Request) {
 	if utils.IsLibpodRequest(r) {
 		options.Volumes = query.LibpodVolumes
 		options.Timeout = query.Timeout
+		options.Depend = query.Depend


Does this handle returns properly? As in, if we remove >1 container, we tell the server that?

Yes it should return all of the containers.

Added a test to verify.

TomSweeneyRedHat · 2022-01-05T22:01:32Z

Changes LGTM, but you're still fighting with the CI @rhatdan

edsantiago · 2022-01-06T13:06:15Z

Current failures are:

[MIRROR] crun-1.3-2.fc35.x86_64.rpm: Status code: 404 for https://kojipkgs.fedoraproject.org//work/tasks/684/80280684/crun-1.3-2.fc35.x86_64.rpm (IP: 38.145.60.20)

I've restarted twice already, no change. This is either some sort of dnf mirror flake that will resolve itself, or some sort of bug in our VM setup. For now I will assume the former.

edsantiago · 2022-01-06T19:07:08Z

APIv2 tests are barfing because a DELETE operation, which should return 204, is now returning 200.

TomSweeneyRedHat

LGTM
and dare I say?
Happy Green Test Buttons!

rhatdan · 2022-01-08T11:02:23Z

@mheon PTAL
@containers/podman-maintainers PTAL

This turned out to be a lot more work then originally thought.

umohnani8 · 2022-01-10T10:39:39Z

LGTM

edsantiago · 2022-01-10T12:55:08Z

test/apiv2/20-containers.at

@@ -85,7 +85,7 @@ else
    fi
 fi

-t DELETE libpod/containers/$cid 204
+t DELETE libpod/containers/$cid 200


All these 204->200 changes trouble me. @jwhonce could you please confirm whether or not these are OK?

This is the breaking API. 200 means no data is returned 204 Means data is returned, which is new, As I understand it.

Other way around: 200 returns data, 204 does not.

One other thing, this change should probably be reflected in the swagger docs. I don't see that as part of this PR, but it's been too long since I looked at swagger so I may just be missing something obvious.

Oh and one last thing: since 200 returns data, if this is really truly a desired change, could you add some data checks to these tests? Something like

-t DELETE libpod/containers/$cid 200 +t DELETE libpod/containers/$cid 200 length=1 .[0].Id=$cid

Ok I updated Swagger, Yes I screwed up. Before if you removed a container you got back no data, now you get back a list of containers that were removed or failed to be removed.

Basically if you send in the depend flag, the rm command can remove one or more containers.

Eventually we should move options.All to the server side as well.

This option causes Podman to not only remove the specified containers but all of the containers that depend on the specified containers. Fixes: containers#10360 Also ran codespell on the code Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2022-01-11T21:06:47Z

@edsantiago PTAL

edsantiago · 2022-01-11T21:13:29Z

LGTM, feel free to merge manually.

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2021

vrothberg reviewed Dec 23, 2021

View reviewed changes

mheon reviewed Dec 23, 2021

View reviewed changes

cdoern reviewed Dec 23, 2021

View reviewed changes

rhatdan force-pushed the depend branch 2 times, most recently from b0ddf65 to 72e3387 Compare December 23, 2021 16:46

rhatdan force-pushed the depend branch 4 times, most recently from 19dc7bd to cb22727 Compare December 23, 2021 19:15

mheon reviewed Jan 4, 2022

View reviewed changes

rhatdan force-pushed the depend branch 9 times, most recently from d6854ca to bbca288 Compare January 5, 2022 20:40

rhatdan force-pushed the depend branch from bbca288 to 7fe0d5e Compare January 6, 2022 11:24

rhatdan added the breaking-change A change that will require a full version bump, i.e. 3.* to 4.* label Jan 6, 2022

rhatdan force-pushed the depend branch from 7fe0d5e to c0ecbbd Compare January 6, 2022 15:43

rhatdan force-pushed the depend branch 7 times, most recently from da1f83f to 6c0bd6f Compare January 7, 2022 20:30

TomSweeneyRedHat reviewed Jan 8, 2022

View reviewed changes

edsantiago reviewed Jan 10, 2022

View reviewed changes

rhatdan force-pushed the depend branch 5 times, most recently from 2653799 to cbdf7d6 Compare January 10, 2022 19:00

Add podman rm --depend

8f2358e

This option causes Podman to not only remove the specified containers but all of the containers that depend on the specified containers. Fixes: containers#10360 Also ran codespell on the code Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the depend branch from cbdf7d6 to 8f2358e Compare January 11, 2022 19:34

rhatdan merged commit 3404ad0 into containers:main Jan 11, 2022

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 bot locked as resolved and limited conversation to collaborators Sep 22, 2023

	// DependMap populates of map of all containers depending on the specified container
	// DependMap populates a map of all containers depending on the specified container

Add podman rm --depend #12694

Add podman rm --depend #12694

Conversation

rhatdan commented Dec 23, 2021

openshift-ci bot commented Dec 23, 2021

rhatdan commented Dec 23, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 23, 2021

rhatdan commented Dec 26, 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

TomSweeneyRedHat commented Jan 5, 2022

edsantiago commented Jan 6, 2022

edsantiago commented Jan 6, 2022

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

rhatdan commented Jan 8, 2022

umohnani8 commented Jan 10, 2022

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 11, 2022

edsantiago commented Jan 11, 2022