Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix podman pod create --infra-command and --infra-image #7621

Merged
merged 1 commit into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions cmd/podman/pods/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ func init() {
flags.StringVar(&createOptions.CGroupParent, "cgroup-parent", "", "Set parent cgroup for the pod")
flags.BoolVar(&createOptions.Infra, "infra", true, "Create an infra container associated with the pod to share namespaces with")
flags.StringVar(&createOptions.InfraConmonPidFile, "infra-conmon-pidfile", "", "Path to the file that will receive the POD of the infra container's conmon")
flags.StringVar(&createOptions.InfraImage, "infra-image", containerConfig.Engine.InfraImage, "The image of the infra container to associate with the pod")
flags.StringVar(&createOptions.InfraCommand, "infra-command", containerConfig.Engine.InfraCommand, "The command to run on the infra container when the pod is started")
flags.String("infra-image", containerConfig.Engine.InfraImage, "The image of the infra container to associate with the pod")
flags.String("infra-command", containerConfig.Engine.InfraCommand, "The command to run on the infra container when the pod is started")
flags.StringSliceVar(&labelFile, "label-file", []string{}, "Read in a line delimited file of labels")
flags.StringSliceVarP(&labels, "label", "l", []string{}, "Set metadata on pod (default [])")
flags.StringVarP(&createOptions.Name, "name", "n", "", "Assign a name to the pod")
Expand Down Expand Up @@ -92,7 +92,6 @@ func create(cmd *cobra.Command, args []string) error {
if cmd.Flag("infra-command").Changed {
return errors.New("cannot set infra-command without an infra container")
}
createOptions.InfraCommand = ""
if cmd.Flag("infra-image").Changed {
return errors.New("cannot set infra-image without an infra container")
}
Expand All @@ -104,6 +103,20 @@ func create(cmd *cobra.Command, args []string) error {
createOptions.Share = nil
} else {
createOptions.Share = strings.Split(share, ",")
if cmd.Flag("infra-command").Changed {
// Only send content to server side if user changed defaults
createOptions.InfraCommand, err = cmd.Flags().GetString("infra-command")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that necessary? It seems rather exotic. A comment would be very helpful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it is exotic, since this type of code is all over the cli. Basically we want to make sure the user made a change using the CLI, rather then sending the default settings over to the server. That way the server can differentiate if the user wanted a change or just to take defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we wouldn't take the defaults from the client containers.conf but from the server?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning for that? Do we document that somewhere?

I think there's some inconsistency among the commands w.r.t. to which defaults come from the client and server.

I am not against the idea but just want to understand if we have some policy for that decision.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All defaults should come from the server. Any defaults that come from the client are a bug. But I agree there are some coming from the client.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrothberg I now have comments, PTAL

if err != nil {
return err
}
}
if cmd.Flag("infra-image").Changed {
// Only send content to server side if user changed defaults
createOptions.InfraImage, err = cmd.Flags().GetString("infra-image")
if err != nil {
return err
}
}
}

if cmd.Flag("pod-id-file").Changed {
Expand Down
30 changes: 30 additions & 0 deletions libpod/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,36 @@ func WithUmask(umask string) CtrCreateOption {

// Pod Creation Options

// WithInfraImage sets the infra image for libpod.
// An infra image is used for inter-container kernel
// namespace sharing within a pod. Typically, an infra
// container is lightweight and is there to reap
// zombie processes within its pid namespace.
func WithInfraImage(img string) PodCreateOption {
return func(pod *Pod) error {
if pod.valid {
return define.ErrPodFinalized
}

pod.config.InfraContainer.InfraImage = img

return nil
}
}

// WithInfraCommand sets the command to
// run on pause container start up.
func WithInfraCommand(cmd []string) PodCreateOption {
return func(pod *Pod) error {
if pod.valid {
return define.ErrPodFinalized
}

pod.config.InfraContainer.InfraCommand = cmd
return nil
}
}

// WithPodName sets the name of the pod.
func WithPodName(name string) PodCreateOption {
return func(pod *Pod) error {
Expand Down
2 changes: 2 additions & 0 deletions libpod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ type InfraContainerConfig struct {
HostAdd []string `json:"hostsAdd,omitempty"`
Networks []string `json:"networks,omitempty"`
ExitCommand []string `json:"exitCommand,omitempty"`
InfraImage string `json:"infraImage,omitempty"`
InfraCommand []string `json:"infraCommand,omitempty"`
}

// ID retrieves the pod's ID
Expand Down
25 changes: 18 additions & 7 deletions libpod/runtime_pod_infra_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,26 @@ func (r *Runtime) makeInfraContainer(ctx context.Context, p *Pod, imgName, rawIm

isRootless := rootless.IsRootless()

entryCmd := []string{r.config.Engine.InfraCommand}
entrypointSet := len(p.config.InfraContainer.InfraCommand) > 0
entryPoint := p.config.InfraContainer.InfraCommand
entryCmd := []string{}
var options []CtrCreateOption
// I've seen circumstances where config is being passed as nil.
// Let's err on the side of safety and make sure it's safe to use.
if config != nil {
setEntrypoint := false
// default to entrypoint in image if there is one
if len(config.Entrypoint) > 0 {
entryCmd = config.Entrypoint
setEntrypoint = true
if !entrypointSet {
if len(config.Entrypoint) > 0 {
entrypointSet = true
entryPoint = config.Entrypoint
entryCmd = config.Entrypoint
}
}
if len(config.Cmd) > 0 {
// We can't use the default pause command, since we're
// sourcing from the image. If we didn't already set an
// entrypoint, set one now.
if !setEntrypoint {
if !entrypointSet {
// Use the Docker default "/bin/sh -c"
// entrypoint, as we're overriding command.
// If an image doesn't want this, it can
Expand Down Expand Up @@ -136,6 +140,9 @@ func (r *Runtime) makeInfraContainer(ctx context.Context, p *Pod, imgName, rawIm
options = append(options, WithRootFSFromImage(imgID, imgName, rawImageName))
options = append(options, WithName(containerName))
options = append(options, withIsInfra())
if entrypointSet {
options = append(options, WithEntrypoint(entryPoint))
}
if len(p.config.InfraContainer.ConmonPidFile) > 0 {
options = append(options, WithConmonPidFile(p.config.InfraContainer.ConmonPidFile))
}
Expand All @@ -151,7 +158,11 @@ func (r *Runtime) createInfraContainer(ctx context.Context, p *Pod) (*Container,
return nil, define.ErrRuntimeStopped
}

newImage, err := r.ImageRuntime().New(ctx, r.config.Engine.InfraImage, "", "", nil, nil, image.SigningOptions{}, nil, util.PullImageMissing)
img := p.config.InfraContainer.InfraImage
if img == "" {
img = r.config.Engine.InfraImage
}
newImage, err := r.ImageRuntime().New(ctx, img, "", "", nil, nil, image.SigningOptions{}, nil, util.PullImageMissing)
if err != nil {
return nil, err
}
Expand Down
17 changes: 0 additions & 17 deletions pkg/domain/infra/runtime_libpod.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,23 +227,6 @@ func getRuntime(ctx context.Context, fs *flag.FlagSet, opts *engineOpts) (*libpo

// TODO flag to set CNI plugins dir?

// TODO I don't think these belong here?
// Will follow up with a different PR to address
//
// Pod create options

infraImageFlag := fs.Lookup("infra-image")
if infraImageFlag != nil && infraImageFlag.Changed {
infraImage, _ := fs.GetString("infra-image")
options = append(options, libpod.WithDefaultInfraImage(infraImage))
}

infraCommandFlag := fs.Lookup("infra-command")
if infraCommandFlag != nil && infraImageFlag.Changed {
infraCommand, _ := fs.GetString("infra-command")
options = append(options, libpod.WithDefaultInfraCommand(infraCommand))
}

if !opts.withFDS {
options = append(options, libpod.WithEnableSDNotify())
}
Expand Down
9 changes: 9 additions & 0 deletions pkg/specgen/generate/pod_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ func createPodOptions(p *specgen.PodSpecGenerator, rt *libpod.Runtime) ([]libpod
if len(p.CNINetworks) > 0 {
options = append(options, libpod.WithPodNetworks(p.CNINetworks))
}

if len(p.InfraImage) > 0 {
options = append(options, libpod.WithInfraImage(p.InfraImage))
}

if len(p.InfraCommand) > 0 {
options = append(options, libpod.WithInfraCommand(p.InfraCommand))
}

switch p.NetNS.NSMode {
case specgen.Bridge, specgen.Default, "":
logrus.Debugf("Pod using default network mode")
Expand Down
7 changes: 0 additions & 7 deletions pkg/specgen/pod_validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,5 @@ func (p *PodSpecGenerator) Validate() error {
return exclusivePodOptions("NoManageHosts", "HostAdd")
}

// Set Defaults
if len(p.InfraImage) < 1 {
p.InfraImage = containerConfig.Engine.InfraImage
}
if len(p.InfraCommand) < 1 {
p.InfraCommand = []string{containerConfig.Engine.InfraCommand}
}
return nil
}
76 changes: 76 additions & 0 deletions test/e2e/pod_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,4 +329,80 @@ var _ = Describe("Podman pod create", func() {
Expect(session.ExitCode()).To(Equal(0))
}
})

It("podman create pod with defaults", func() {
name := "test"
session := podmanTest.Podman([]string{"pod", "create", "--name", name})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

check := podmanTest.Podman([]string{"pod", "inspect", name})
check.WaitWithDefaultTimeout()
Expect(check.ExitCode()).To(Equal(0))
data := check.InspectPodToJSON()

check1 := podmanTest.Podman([]string{"container", "inspect", "--format", "{{.Config.Entrypoint}}", data.Containers[0].ID})
check1.WaitWithDefaultTimeout()
Expect(check1.ExitCode()).To(Equal(0))
Expect(check1.OutputToString()).To(Equal("/pause"))
})

It("podman create pod with --infra-command", func() {
name := "test"
session := podmanTest.Podman([]string{"pod", "create", "--infra-command", "/pause1", "--name", name})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

check := podmanTest.Podman([]string{"pod", "inspect", name})
check.WaitWithDefaultTimeout()
Expect(check.ExitCode()).To(Equal(0))
data := check.InspectPodToJSON()

check1 := podmanTest.Podman([]string{"container", "inspect", "--format", "{{.Config.Entrypoint}}", data.Containers[0].ID})
check1.WaitWithDefaultTimeout()
Expect(check1.ExitCode()).To(Equal(0))
Expect(check1.OutputToString()).To(Equal("/pause1"))
})

It("podman create pod with --infra-image", func() {
dockerfile := `FROM docker.io/library/alpine:latest
entrypoint ["/fromimage"]
`
podmanTest.BuildImage(dockerfile, "localhost/infra", "false")
name := "test"
session := podmanTest.Podman([]string{"pod", "create", "--infra-image", "localhost/infra", "--name", name})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

check := podmanTest.Podman([]string{"pod", "inspect", name})
check.WaitWithDefaultTimeout()
Expect(check.ExitCode()).To(Equal(0))
data := check.InspectPodToJSON()

check1 := podmanTest.Podman([]string{"container", "inspect", "--format", "{{.Config.Entrypoint}}", data.Containers[0].ID})
check1.WaitWithDefaultTimeout()
Expect(check1.ExitCode()).To(Equal(0))
Expect(check1.OutputToString()).To(Equal("/fromimage"))
})

It("podman create pod with --infra-command --infra-image", func() {
dockerfile := `FROM docker.io/library/alpine:latest
entrypoint ["/fromimage"]
`
podmanTest.BuildImage(dockerfile, "localhost/infra", "false")
name := "test"
session := podmanTest.Podman([]string{"pod", "create", "--infra-image", "localhost/infra", "--infra-command", "/fromcommand", "--name", name})
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Equal(0))

check := podmanTest.Podman([]string{"pod", "inspect", name})
check.WaitWithDefaultTimeout()
Expect(check.ExitCode()).To(Equal(0))
data := check.InspectPodToJSON()

check1 := podmanTest.Podman([]string{"container", "inspect", "--format", "{{.Config.Entrypoint}}", data.Containers[0].ID})
check1.WaitWithDefaultTimeout()
Expect(check1.ExitCode()).To(Equal(0))
Expect(check1.OutputToString()).To(Equal("/fromcommand"))
})
})