From d10e77e1bcd25306ab8afd4ce7da16eed3c67840 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 7 Sep 2022 18:31:23 +0200 Subject: [PATCH] fix podman pod inspect to support multiple pods Just like the other inspect commands `podman pod inspect p1 p2` should return the json for both. To correctly implement this we follow the container inspect logic, this allows use to reuse the global inspect command. Note: To not break the existing single pod output format for podman pod inspect I added a pod-legacy inspect type. This is only used to make sure we will print the pod as single json and not an array like for the other commands. We cannot use the pod type since podman inspect --type pod did return an array and we should not break that as well. Fixes #15674 Signed-off-by: Paul Holzinger --- cmd/podman/common/inspect.go | 4 ++ cmd/podman/inspect/inspect.go | 72 ++++++++----------------- cmd/podman/pods/inspect.go | 54 ++++--------------- pkg/domain/entities/engine_container.go | 2 +- pkg/domain/entities/pods.go | 9 ---- pkg/domain/infra/abi/pods.go | 58 ++++++++++++++------ pkg/domain/infra/tunnel/pods.go | 27 +++++++--- test/e2e/common_test.go | 2 +- test/e2e/pod_inspect_test.go | 17 ++++++ 9 files changed, 119 insertions(+), 126 deletions(-) diff --git a/cmd/podman/common/inspect.go b/cmd/podman/common/inspect.go index 12a5af5a92..f82161d31c 100644 --- a/cmd/podman/common/inspect.go +++ b/cmd/podman/common/inspect.go @@ -11,6 +11,10 @@ const ( NetworkType = "network" // PodType is the pod type. PodType = "pod" + // PodLegacyType is the pod type for backwards compatibility with the old pod inspect code. + // This allows us to use the shared inspect code but still provide the correct output format + // when podman pod inspect was called. + PodLegacyType = "pod-legacy" // VolumeType is the volume type VolumeType = "volume" ) diff --git a/cmd/podman/inspect/inspect.go b/cmd/podman/inspect/inspect.go index d519bc7d91..ccabd7614e 100644 --- a/cmd/podman/inspect/inspect.go +++ b/cmd/podman/inspect/inspect.go @@ -15,7 +15,6 @@ import ( "github.com/containers/podman/v4/cmd/podman/common" "github.com/containers/podman/v4/cmd/podman/registry" "github.com/containers/podman/v4/cmd/podman/validate" - "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/pkg/domain/entities" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -55,18 +54,10 @@ type inspector struct { containerEngine entities.ContainerEngine imageEngine entities.ImageEngine options entities.InspectOptions - podOptions entities.PodInspectOptions } // newInspector creates a new inspector based on the specified options. func newInspector(options entities.InspectOptions) (*inspector, error) { - switch options.Type { - case common.ImageType, common.ContainerType, common.AllType, common.PodType, common.NetworkType, common.VolumeType: - // Valid types. - default: - return nil, fmt.Errorf("invalid type %q: must be %q, %q, %q, %q, %q, or %q", options.Type, - common.ImageType, common.ContainerType, common.PodType, common.NetworkType, common.VolumeType, common.AllType) - } if options.Type == common.ImageType { if options.Latest { return nil, fmt.Errorf("latest is not supported for type %q", common.ImageType) @@ -78,15 +69,10 @@ func newInspector(options entities.InspectOptions) (*inspector, error) { if options.Type == common.PodType && options.Size { return nil, fmt.Errorf("size is not supported for type %q", common.PodType) } - podOpts := entities.PodInspectOptions{ - Latest: options.Latest, - Format: options.Format, - } return &inspector{ containerEngine: registry.ContainerEngine(), imageEngine: registry.ImageEngine(), options: options, - podOptions: podOpts, }, nil } @@ -140,34 +126,16 @@ func (i *inspector) inspect(namesOrIDs []string) error { for i := range ctrData { data = append(data, ctrData[i]) } - case common.PodType: - for _, pod := range namesOrIDs { - i.podOptions.NameOrID = pod - podData, err := i.containerEngine.PodInspect(ctx, i.podOptions) - if err != nil { - if !strings.Contains(err.Error(), define.ErrNoSuchPod.Error()) { - errs = []error{err} - } else { - return err - } - } else { - errs = nil - data = append(data, podData) - } + case common.PodType, common.PodLegacyType: + podData, allErrs, err := i.containerEngine.PodInspect(ctx, namesOrIDs, i.options) + if err != nil { + return err } - if i.podOptions.Latest { // latest means there are no names in the namesOrID array - podData, err := i.containerEngine.PodInspect(ctx, i.podOptions) - if err != nil { - if !strings.Contains(err.Error(), define.ErrNoSuchPod.Error()) { - errs = []error{err} - } else { - return err - } - } else { - errs = nil - data = append(data, podData) - } + errs = allErrs + for i := range podData { + data = append(data, podData[i]) } + case common.NetworkType: networkData, allErrs, err := registry.ContainerEngine().NetworkInspect(ctx, namesOrIDs, i.options) if err != nil { @@ -198,7 +166,14 @@ func (i *inspector) inspect(namesOrIDs []string) error { var err error switch { case report.IsJSON(i.options.Format) || i.options.Format == "": - err = printJSON(data) + if i.options.Type == common.PodLegacyType && len(data) == 1 { + // We need backwards compat with the old podman pod inspect behavior. + // https://github.com/containers/podman/pull/15675 + // TODO (5.0): consider removing this to better match other commands. + err = printJSON(data[0]) + } else { + err = printJSON(data) + } default: // Landing here implies user has given a custom --format row := inspectNormalize(i.options.Format, tmpType) @@ -221,7 +196,7 @@ func (i *inspector) inspect(namesOrIDs []string) error { return nil } -func printJSON(data []interface{}) error { +func printJSON(data interface{}) error { enc := json.NewEncoder(os.Stdout) // by default, json marshallers will force utf=8 from // a string. this breaks healthchecks that use <,>, &&. @@ -282,14 +257,13 @@ func (i *inspector) inspectAll(ctx context.Context, namesOrIDs []string) ([]inte data = append(data, networkData[0]) continue } - i.podOptions.NameOrID = name - podData, err := i.containerEngine.PodInspect(ctx, i.podOptions) + + podData, errs, err := i.containerEngine.PodInspect(ctx, []string{name}, i.options) if err != nil { - if !strings.Contains(err.Error(), define.ErrNoSuchPod.Error()) { - return nil, nil, err - } - } else { - data = append(data, podData) + return nil, nil, err + } + if len(errs) == 0 { + data = append(data, podData[0]) continue } if len(errs) > 0 { diff --git a/cmd/podman/pods/inspect.go b/cmd/podman/pods/inspect.go index 082e8d9a12..22e781cdf8 100644 --- a/cmd/podman/pods/inspect.go +++ b/cmd/podman/pods/inspect.go @@ -1,23 +1,14 @@ package pods import ( - "context" - "errors" - "os" - "text/template" - - "github.com/containers/common/pkg/report" "github.com/containers/podman/v4/cmd/podman/common" + "github.com/containers/podman/v4/cmd/podman/inspect" "github.com/containers/podman/v4/cmd/podman/registry" "github.com/containers/podman/v4/cmd/podman/validate" "github.com/containers/podman/v4/pkg/domain/entities" "github.com/spf13/cobra" ) -var ( - inspectOptions = entities.PodInspectOptions{} -) - var ( inspectDescription = `Display the configuration for a pod by name or id @@ -27,10 +18,12 @@ var ( Use: "inspect [options] POD [POD...]", Short: "Displays a pod configuration", Long: inspectDescription, - RunE: inspect, + RunE: inspectExec, ValidArgsFunction: common.AutocompletePods, Example: `podman pod inspect podID`, } + + inspectOpts = &entities.InspectOptions{} ) func init() { @@ -41,40 +34,15 @@ func init() { flags := inspectCmd.Flags() formatFlagName := "format" - flags.StringVarP(&inspectOptions.Format, formatFlagName, "f", "json", "Format the output to a Go template or json") + flags.StringVarP(&inspectOpts.Format, formatFlagName, "f", "json", "Format the output to a Go template or json") _ = inspectCmd.RegisterFlagCompletionFunc(formatFlagName, common.AutocompleteFormat(&entities.PodInspectReport{})) - validate.AddLatestFlag(inspectCmd, &inspectOptions.Latest) + validate.AddLatestFlag(inspectCmd, &inspectOpts.Latest) } -func inspect(cmd *cobra.Command, args []string) error { - if len(args) < 1 && !inspectOptions.Latest { - return errors.New("you must provide the name or id of a running pod") - } - if len(args) > 0 && inspectOptions.Latest { - return errors.New("--latest and containers cannot be used together") - } - - if !inspectOptions.Latest { - inspectOptions.NameOrID = args[0] - } - responses, err := registry.ContainerEngine().PodInspect(context.Background(), inspectOptions) - if err != nil { - return err - } - - if report.IsJSON(inspectOptions.Format) { - enc := json.NewEncoder(os.Stdout) - enc.SetIndent("", " ") - return enc.Encode(responses) - } - - // Cannot use report.New() as it enforces {{range .}} for OriginUser templates - tmpl := template.New(cmd.Name()).Funcs(template.FuncMap(report.DefaultFuncs)) - format := report.NormalizeFormat(inspectOptions.Format) - tmpl, err = tmpl.Parse(format) - if err != nil { - return err - } - return tmpl.Execute(os.Stdout, *responses) +func inspectExec(cmd *cobra.Command, args []string) error { + // We need backwards compat with the old podman pod inspect behavior. + // https://github.com/containers/podman/pull/15675 + inspectOpts.Type = common.PodLegacyType + return inspect.Inspect(args, *inspectOpts) } diff --git a/pkg/domain/entities/engine_container.go b/pkg/domain/entities/engine_container.go index 69adc97325..19b666f8e0 100644 --- a/pkg/domain/entities/engine_container.go +++ b/pkg/domain/entities/engine_container.go @@ -75,7 +75,7 @@ type ContainerEngine interface { PodCreate(ctx context.Context, specg PodSpec) (*PodCreateReport, error) PodClone(ctx context.Context, podClone PodCloneOptions) (*PodCloneReport, error) PodExists(ctx context.Context, nameOrID string) (*BoolReport, error) - PodInspect(ctx context.Context, options PodInspectOptions) (*PodInspectReport, error) + PodInspect(ctx context.Context, namesOrID []string, options InspectOptions) ([]*PodInspectReport, []error, error) PodKill(ctx context.Context, namesOrIds []string, options PodKillOptions) ([]*PodKillReport, error) PodLogs(ctx context.Context, pod string, options PodLogsOptions) error PodPause(ctx context.Context, namesOrIds []string, options PodPauseOptions) ([]*PodPauseReport, error) diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index b672434d8e..55e2fd5747 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -438,15 +438,6 @@ type PodPSOptions struct { Sort string } -type PodInspectOptions struct { - Latest bool - - // Options for the API. - NameOrID string - - Format string -} - type PodInspectReport struct { *define.InspectPodData } diff --git a/pkg/domain/infra/abi/pods.go b/pkg/domain/infra/abi/pods.go index 03c8082c49..68f2fa125a 100644 --- a/pkg/domain/infra/abi/pods.go +++ b/pkg/domain/infra/abi/pods.go @@ -505,23 +505,49 @@ func (ic *ContainerEngine) PodPs(ctx context.Context, options entities.PodPSOpti return reports, nil } -func (ic *ContainerEngine) PodInspect(ctx context.Context, options entities.PodInspectOptions) (*entities.PodInspectReport, error) { - var ( - pod *libpod.Pod - err error - ) - // Look up the pod. +func (ic *ContainerEngine) PodInspect(ctx context.Context, nameOrIDs []string, options entities.InspectOptions) ([]*entities.PodInspectReport, []error, error) { if options.Latest { - pod, err = ic.Libpod.GetLatestPod() - } else { - pod, err = ic.Libpod.LookupPod(options.NameOrID) - } - if err != nil { - return nil, fmt.Errorf("unable to look up requested container: %w", err) + pod, err := ic.Libpod.GetLatestPod() + if err != nil { + return nil, nil, err + } + inspect, err := pod.Inspect() + if err != nil { + return nil, nil, err + } + + return []*entities.PodInspectReport{ + { + InspectPodData: inspect, + }, + }, nil, nil } - inspect, err := pod.Inspect() - if err != nil { - return nil, err + + var errs []error + podReport := make([]*entities.PodInspectReport, 0, len(nameOrIDs)) + for _, name := range nameOrIDs { + pod, err := ic.Libpod.LookupPod(name) + if err != nil { + // ErrNoSuchPod is non-fatal, other errors will be + // treated as fatal. + if errors.Is(err, define.ErrNoSuchPod) { + errs = append(errs, fmt.Errorf("no such pod %s", name)) + continue + } + return nil, nil, err + } + + inspect, err := pod.Inspect() + if err != nil { + // ErrNoSuchPod is non-fatal, other errors will be + // treated as fatal. + if errors.Is(err, define.ErrNoSuchPod) { + errs = append(errs, fmt.Errorf("no such pod %s", name)) + continue + } + return nil, nil, err + } + podReport = append(podReport, &entities.PodInspectReport{InspectPodData: inspect}) } - return &entities.PodInspectReport{InspectPodData: inspect}, nil + return podReport, errs, nil } diff --git a/pkg/domain/infra/tunnel/pods.go b/pkg/domain/infra/tunnel/pods.go index bcbd32d1b8..f9314dcfee 100644 --- a/pkg/domain/infra/tunnel/pods.go +++ b/pkg/domain/infra/tunnel/pods.go @@ -3,10 +3,12 @@ package tunnel import ( "context" "errors" + "fmt" "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/pkg/bindings/pods" "github.com/containers/podman/v4/pkg/domain/entities" + "github.com/containers/podman/v4/pkg/errorhandling" "github.com/containers/podman/v4/pkg/util" ) @@ -223,14 +225,25 @@ func (ic *ContainerEngine) PodPs(ctx context.Context, opts entities.PodPSOptions return pods.List(ic.ClientCtx, options) } -func (ic *ContainerEngine) PodInspect(ctx context.Context, options entities.PodInspectOptions) (*entities.PodInspectReport, error) { - switch { - case options.Latest: - return nil, errors.New("latest is not supported") - case options.NameOrID == "": - return nil, errors.New("NameOrID must be specified") +func (ic *ContainerEngine) PodInspect(ctx context.Context, namesOrIDs []string, options entities.InspectOptions) ([]*entities.PodInspectReport, []error, error) { + var errs []error + podReport := make([]*entities.PodInspectReport, 0, len(namesOrIDs)) + for _, name := range namesOrIDs { + inspect, err := pods.Inspect(ic.ClientCtx, name, nil) + if err != nil { + errModel, ok := err.(*errorhandling.ErrorModel) + if !ok { + return nil, nil, err + } + if errModel.ResponseCode == 404 { + errs = append(errs, fmt.Errorf("no such pod %q", name)) + continue + } + return nil, nil, err + } + podReport = append(podReport, inspect) } - return pods.Inspect(ic.ClientCtx, options.NameOrID, nil) + return podReport, errs, nil } func (ic *ContainerEngine) PodStats(ctx context.Context, namesOrIds []string, opts entities.PodStatsOptions) ([]*entities.PodStatsReport, error) { diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index 2d7c47a7fa..8fe89f32eb 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -571,7 +571,7 @@ func (s *PodmanSessionIntegration) InspectContainerToJSON() []define.InspectCont func (s *PodmanSessionIntegration) InspectPodToJSON() define.InspectPodData { var i define.InspectPodData err := jsoniter.Unmarshal(s.Out.Contents(), &i) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) return i } diff --git a/test/e2e/pod_inspect_test.go b/test/e2e/pod_inspect_test.go index 351317cc52..cefdee40ac 100644 --- a/test/e2e/pod_inspect_test.go +++ b/test/e2e/pod_inspect_test.go @@ -118,4 +118,21 @@ var _ = Describe("Podman pod inspect", func() { Expect(inspectOut.OutputToString()).To(ContainSubstring(macAddr)) }) + + It("podman inspect two pods", func() { + _, ec, podid1 := podmanTest.CreatePod(nil) + Expect(ec).To(Equal(0)) + + _, ec, podid2 := podmanTest.CreatePod(nil) + Expect(ec).To(Equal(0)) + + inspect := podmanTest.Podman([]string{"pod", "inspect", podid1, podid2}) + inspect.WaitWithDefaultTimeout() + Expect(inspect).Should(Exit(0)) + Expect(inspect.OutputToString()).To(BeValidJSON()) + podData := inspect.InspectPodArrToJSON() + Expect(podData).To(HaveLen(2)) + Expect(podData[0]).To(HaveField("ID", podid1)) + Expect(podData[1]).To(HaveField("ID", podid2)) + }) })