From 1e84e1a8db1187ca25db91cef6649b678ab3400d Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 21 Dec 2022 09:47:56 +0100 Subject: [PATCH] infra/abi: refactor ContainerRm The function grew into a big hairy ball over time and I personally refrained from touching it as it seemed fragile. Hence, refactor the function into something more comprehensible and maintainable. There is still potential for improvement but I want to tackle one thing at a time. [NO NEW TESTS NEEDED] as it shouldn't change behavior. Signed-off-by: Valentin Rothberg --- pkg/domain/infra/abi/containers.go | 116 +++++++++-------------------- test/system/055-rm.bats | 2 +- 2 files changed, 38 insertions(+), 80 deletions(-) diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 522c441a5a..3d81a7d8f5 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -383,60 +383,37 @@ func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Cont func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, options entities.RmOptions) ([]*reports.RmReport, error) { rmReports := []*reports.RmReport{} - names := namesOrIds - // Attempt to remove named containers directly from storage, if container is defined in libpod - // this will fail and code will fall through to removing the container from libpod.` - tmpNames := []string{} - for _, ctr := range names { - report := reports.RmReport{Id: ctr} - report.Err = ic.Libpod.RemoveStorageContainer(ctr, options.Force) - //nolint:gocritic - if report.Err == nil { - // remove container names that we successfully deleted - rmReports = append(rmReports, &report) - } else if errors.Is(report.Err, define.ErrNoSuchCtr) || errors.Is(report.Err, define.ErrCtrExists) { - // There is still a potential this is a libpod container - tmpNames = append(tmpNames, ctr) - } else { - if _, err := ic.Libpod.LookupContainer(ctr); errors.Is(err, define.ErrNoSuchCtr) { - // remove container failed, but not a libpod container - rmReports = append(rmReports, &report) - continue - } - // attempt to remove as a libpod container - tmpNames = append(tmpNames, ctr) - } + containers, err := getContainers(ic.Libpod, getContainersOptions{ + all: options.All, + latest: options.Latest, + filters: options.Filters, + names: namesOrIds, + ignore: true, // Force ignore as `podman rm` also handles external containers + }) + if err != nil { + return nil, err } - names = tmpNames - containers, err := getContainers(ic.Libpod, getContainersOptions{all: options.All, latest: options.Latest, filters: options.Filters, names: names, ignore: options.Ignore}) - if err != nil && !(options.Ignore && errors.Is(err, define.ErrNoSuchCtr)) { - // Failed to get containers. If force is specified, get the containers ID - // and evict them - if !options.Force { - return nil, err - } - - for _, ctr := range names { - logrus.Debugf("Evicting container %q", ctr) - report := reports.RmReport{ - Id: ctr, - RawInput: ctr, - } - _, err := ic.Libpod.EvictContainer(ctx, ctr, options.Volumes) - if err != nil { - if options.Ignore && errors.Is(err, define.ErrNoSuchCtr) { - logrus.Debugf("Ignoring error (--allow-missing): %v", err) - rmReports = append(rmReports, &report) + libpodContainers := make([]*libpod.Container, 0, len(containers)) + idToRawInput := make(map[string]string, len(containers)) + for i, ctr := range containers { + if ctr.doesNotExist { + // If the container does not exist in Podman's database, it may + // be an external one. Hence, try removing the external + // "storage" container. + if err := ic.Libpod.RemoveStorageContainer(ctr.rawInput, options.Force); err != nil { + if options.Ignore && (errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrExists)) { continue } - report.Err = err - rmReports = append(rmReports, &report) - continue + return nil, err } - rmReports = append(rmReports, &report) + rmReports = append(rmReports, &reports.RmReport{RawInput: ctr.rawInput}) + } else { + // If the container exists in the Podman database, we + // can remove it correctly below. + libpodContainers = append(libpodContainers, containers[i].Container) + idToRawInput[ctr.ID()] = ctr.rawInput } - return rmReports, nil } alreadyRemoved := make(map[string]bool) @@ -446,44 +423,23 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, rmReports = append(rmReports, newReports[i]) } } - if !options.All && options.Depend { - // Add additional containers based on dependencies to container map - for _, ctr := range containers { - if ctr.doesNotExist || alreadyRemoved[ctr.ID()] { + + // First, remove dependend containers. + if options.All || options.Depend { + for _, ctr := range libpodContainers { + // When `All` is set, remove the infra containers and + // their dependencies first. Otherwise, we'd error out. + // + // TODO: All this logic should probably live in libpod. + if alreadyRemoved[ctr.ID()] || (options.All && !ctr.IsInfra()) { continue } - reports, err := ic.Libpod.RemoveDepend(ctx, ctr.Container, options.Force, options.Volumes, options.Timeout) + reports, err := ic.Libpod.RemoveDepend(ctx, ctr, options.Force, options.Volumes, options.Timeout) if err != nil { return rmReports, err } addReports(reports) } - return rmReports, nil - } - - // If --all is set, make sure to remove the infra containers' - // dependencies before doing the parallel removal below. - if options.All { - for _, ctr := range containers { - if ctr.doesNotExist || alreadyRemoved[ctr.ID()] || !ctr.IsInfra() { - continue - } - reports, err := ic.Libpod.RemoveDepend(ctx, ctr.Container, options.Force, options.Volumes, options.Timeout) - if err != nil { - return rmReports, err - } - addReports(reports) - } - } - - idToRawInput := make(map[string]string, len(containers)) - libpodContainers := make([]*libpod.Container, 0, len(containers)) - for i, c := range containers { - if c.doesNotExist { - continue - } - idToRawInput[c.ID()] = c.rawInput - libpodContainers = append(libpodContainers, containers[i].Container) } errMap, err := parallelctr.ContainerOp(ctx, libpodContainers, func(c *libpod.Container) error { @@ -495,6 +451,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, if err != nil { return nil, err } + for ctr, err := range errMap { if alreadyRemoved[ctr.ID()] { continue @@ -505,6 +462,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, report.RawInput = idToRawInput[ctr.ID()] rmReports = append(rmReports, report) } + return rmReports, nil } diff --git a/test/system/055-rm.bats b/test/system/055-rm.bats index a555bd2b5a..51f2752e0b 100644 --- a/test/system/055-rm.bats +++ b/test/system/055-rm.bats @@ -108,7 +108,7 @@ load helpers @test "podman container rm --force bogus" { run_podman 1 container rm bogus - is "$output" "Error: no container with name or ID \"bogus\" found: no such container" "Should print error" + is "$output" "Error: no container with ID or name \"bogus\" found: no such container" "Should print error" run_podman container rm --force bogus is "$output" "" "Should print no output" }