From cb603b8a3e4a4a1da194ed020caf270fa85f6f5b Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Wed, 15 Jul 2020 16:33:24 -0400 Subject: Support default profile for apparmor Currently you can not apply an ApparmorProfile if you specify --privileged. This patch will allow both to be specified simultaniosly. By default Apparmor should be disabled if the user specifies --privileged, but if the user specifies --security apparmor:PROFILE, with --privileged, we should do both. Added e2e run_apparmor_test.go Signed-off-by: Daniel J Walsh --- libpod/container_internal_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libpod') diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index cb70aea62..018e2d5a4 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -244,7 +244,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) { } // Apply AppArmor checks and load the default profile if needed. - if !c.config.Privileged { + if len(c.config.Spec.Process.ApparmorProfile) > 0 { updatedProfile, err := apparmor.CheckProfileAndLoadDefault(c.config.Spec.Process.ApparmorProfile) if err != nil { return nil, err -- cgit v1.2.3-54-g00ecf From e21a6368f9308292641ed5ce58321b1cd46abdc9 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Fri, 17 Jul 2020 13:58:22 +0200 Subject: abi: set default umask and rlimits the code got lost in the migration to podman 2.0, reintroduce it. Closes: https://github.com/containers/podman/issues/6989 Signed-off-by: Giuseppe Scrivano Signed-off-by: Matthew Heon --- cmd/podman/early_init_linux.go | 39 ++++++++++++++++++++++++++++++++++++ cmd/podman/early_init_unsupported.go | 6 ++++++ cmd/podman/root.go | 1 + libpod/define/config.go | 10 +++++++++ pkg/domain/infra/abi/system.go | 22 -------------------- pkg/spec/spec.go | 15 +++++++------- pkg/specgen/generate/oci.go | 15 +++++++------- 7 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 cmd/podman/early_init_linux.go create mode 100644 cmd/podman/early_init_unsupported.go (limited to 'libpod') diff --git a/cmd/podman/early_init_linux.go b/cmd/podman/early_init_linux.go new file mode 100644 index 000000000..b43450a7f --- /dev/null +++ b/cmd/podman/early_init_linux.go @@ -0,0 +1,39 @@ +package main + +import ( + "fmt" + "os" + "syscall" + + "github.com/containers/libpod/v2/libpod/define" + "github.com/pkg/errors" +) + +func setRLimits() error { + rlimits := new(syscall.Rlimit) + rlimits.Cur = define.RLimitDefaultValue + rlimits.Max = define.RLimitDefaultValue + if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { + if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { + return errors.Wrapf(err, "error getting rlimits") + } + rlimits.Cur = rlimits.Max + if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { + return errors.Wrapf(err, "error setting new rlimits") + } + } + return nil +} + +func setUMask() { + // Be sure we can create directories with 0755 mode. + syscall.Umask(0022) +} + +func earlyInitHook() { + if err := setRLimits(); err != nil { + fmt.Fprint(os.Stderr, "Failed to set rlimits: "+err.Error()) + } + + setUMask() +} diff --git a/cmd/podman/early_init_unsupported.go b/cmd/podman/early_init_unsupported.go new file mode 100644 index 000000000..4e748559f --- /dev/null +++ b/cmd/podman/early_init_unsupported.go @@ -0,0 +1,6 @@ +// +build !linux + +package main + +func earlyInitHook() { +} diff --git a/cmd/podman/root.go b/cmd/podman/root.go index 7c54da91a..b2c9f9c2c 100644 --- a/cmd/podman/root.go +++ b/cmd/podman/root.go @@ -77,6 +77,7 @@ func init() { cobra.OnInitialize( loggingHook, syslogHook, + earlyInitHook, ) rootFlags(rootCmd, registry.PodmanConfig()) diff --git a/libpod/define/config.go b/libpod/define/config.go index 900a363d8..64b24d9e2 100644 --- a/libpod/define/config.go +++ b/libpod/define/config.go @@ -75,3 +75,13 @@ const JSONLogging = "json-file" // NoLogging is the string conmon expects when specifying to use no log driver whatsoever const NoLogging = "none" + +// Strings used for --sdnotify option to podman +const ( + SdNotifyModeContainer = "container" + SdNotifyModeConmon = "conmon" + SdNotifyModeIgnore = "ignore" +) + +// DefaultRlimitValue is the value set by default for nofile and nproc +const RLimitDefaultValue = uint64(1048576) diff --git a/pkg/domain/infra/abi/system.go b/pkg/domain/infra/abi/system.go index be14f52b8..435902ded 100644 --- a/pkg/domain/infra/abi/system.go +++ b/pkg/domain/infra/abi/system.go @@ -8,7 +8,6 @@ import ( "os/exec" "path/filepath" "strconv" - "syscall" "github.com/containers/common/pkg/config" "github.com/containers/libpod/v2/libpod/define" @@ -146,27 +145,6 @@ func movePauseProcessToScope() error { return utils.RunUnderSystemdScope(int(pid), "user.slice", "podman-pause.scope") } -func setRLimits() error { // nolint:deadcode,unused - rlimits := new(syscall.Rlimit) - rlimits.Cur = 1048576 - rlimits.Max = 1048576 - if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { - if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { - return errors.Wrapf(err, "error getting rlimits") - } - rlimits.Cur = rlimits.Max - if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, rlimits); err != nil { - return errors.Wrapf(err, "error setting new rlimits") - } - } - return nil -} - -func setUMask() { // nolint:deadcode,unused - // Be sure we can create directories with 0755 mode. - syscall.Umask(0022) -} - // checkInput can be used to verify any of the globalopt values func checkInput() error { // nolint:deadcode,unused return nil diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 6f52b88b1..b974772d5 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -505,10 +505,9 @@ func BlockAccessToKernelFilesystems(privileged, pidModeIsHost bool, g *generate. func addRlimits(config *CreateConfig, g *generate.Generator) error { var ( - kernelMax uint64 = 1048576 - isRootless = rootless.IsRootless() - nofileSet = false - nprocSet = false + isRootless = rootless.IsRootless() + nofileSet = false + nprocSet = false ) for _, u := range config.Resources.Ulimit { @@ -538,8 +537,8 @@ func addRlimits(config *CreateConfig, g *generate.Generator) error { // files and number of processes to the maximum they can be set to // (without overriding a sysctl) if !nofileSet { - max := kernelMax - current := kernelMax + max := define.RLimitDefaultValue + current := define.RLimitDefaultValue if isRootless { var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err != nil { @@ -555,8 +554,8 @@ func addRlimits(config *CreateConfig, g *generate.Generator) error { g.AddProcessRlimits("RLIMIT_NOFILE", max, current) } if !nprocSet { - max := kernelMax - current := kernelMax + max := define.RLimitDefaultValue + current := define.RLimitDefaultValue if isRootless { var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err != nil { diff --git a/pkg/specgen/generate/oci.go b/pkg/specgen/generate/oci.go index f770b0582..b62e851b3 100644 --- a/pkg/specgen/generate/oci.go +++ b/pkg/specgen/generate/oci.go @@ -20,10 +20,9 @@ import ( func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { var ( - kernelMax uint64 = 1048576 - isRootless = rootless.IsRootless() - nofileSet = false - nprocSet = false + isRootless = rootless.IsRootless() + nofileSet = false + nprocSet = false ) if s.Rlimits == nil { @@ -45,8 +44,8 @@ func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { // files and number of processes to the maximum they can be set to // (without overriding a sysctl) if !nofileSet { - max := kernelMax - current := kernelMax + max := define.RLimitDefaultValue + current := define.RLimitDefaultValue if isRootless { var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err != nil { @@ -62,8 +61,8 @@ func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) error { g.AddProcessRlimits("RLIMIT_NOFILE", max, current) } if !nprocSet { - max := kernelMax - current := kernelMax + max := define.RLimitDefaultValue + current := define.RLimitDefaultValue if isRootless { var rlimit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err != nil { -- cgit v1.2.3-54-g00ecf From 31ecb728d21b155ece248ec44afab59e1d61437e Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 13 Jul 2020 14:22:43 -0400 Subject: Include infra container information in `pod inspect` We had a field for this in the inspect data, but it was never being populated. Because of this, `podman pod inspect` stopped showing port bindings (and other infra container settings). Add code to populate the infra container inspect data, and add a test to ensure we don't regress again. Signed-off-by: Matthew Heon --- libpod/container_inspect.go | 17 +++------------- libpod/define/pod_inspect.go | 6 +++--- libpod/networking_linux.go | 16 +-------------- libpod/pod_api.go | 47 +++++++++++++++++++++++++++++++++++++++++++- libpod/util.go | 19 ++++++++++++++++++ test/e2e/pod_inspect_test.go | 20 +++++++++++++++++++ 6 files changed, 92 insertions(+), 33 deletions(-) (limited to 'libpod') diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go index 03684ddec..8c392d0d6 100644 --- a/libpod/container_inspect.go +++ b/libpod/container_inspect.go @@ -610,22 +610,11 @@ func (c *Container) generateInspectContainerHostConfig(ctrSpec *spec.Spec, named // Port bindings. // Only populate if we're using CNI to configure the network. - portBindings := make(map[string][]define.InspectHostPort) if c.config.CreateNetNS { - for _, port := range c.config.PortMappings { - key := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol) - hostPorts := portBindings[key] - if hostPorts == nil { - hostPorts = []define.InspectHostPort{} - } - hostPorts = append(hostPorts, define.InspectHostPort{ - HostIP: port.HostIP, - HostPort: fmt.Sprintf("%d", port.HostPort), - }) - portBindings[key] = hostPorts - } + hostConfig.PortBindings = makeInspectPortBindings(c.config.PortMappings) + } else { + hostConfig.PortBindings = make(map[string][]define.InspectHostPort) } - hostConfig.PortBindings = portBindings // Cap add and cap drop. // We need a default set of capabilities to compare against. diff --git a/libpod/define/pod_inspect.go b/libpod/define/pod_inspect.go index 7f06e16fc..634cbb728 100644 --- a/libpod/define/pod_inspect.go +++ b/libpod/define/pod_inspect.go @@ -3,8 +3,6 @@ package define import ( "net" "time" - - "github.com/cri-o/ocicni/pkg/ocicni" ) // InspectPodData contains detailed information on a pod's configuration and @@ -60,7 +58,7 @@ type InspectPodData struct { type InspectPodInfraConfig struct { // PortBindings are ports that will be forwarded to the infra container // and then shared with the pod. - PortBindings []ocicni.PortMapping + PortBindings map[string][]InspectHostPort // HostNetwork is whether the infra container (and thus the whole pod) // will use the host's network and not create a network namespace. HostNetwork bool @@ -89,6 +87,8 @@ type InspectPodInfraConfig struct { // HostAdd adds a number of hosts to the infra container's resolv.conf // which will be shared with the rest of the pod. HostAdd []string + // Networks is a list of CNI networks te pod will join. + Networks []string } // InspectPodContainerInfo contains information on a container in a pod. diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 7985e17e1..5cc282f3a 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -587,21 +587,7 @@ func getContainerNetIO(ctr *Container) (*netlink.LinkStatistics, error) { // network. func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, error) { settings := new(define.InspectNetworkSettings) - settings.Ports = make(map[string][]define.InspectHostPort) - if c.config.PortMappings != nil { - for _, port := range c.config.PortMappings { - key := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol) - mapping := settings.Ports[key] - if mapping == nil { - mapping = []define.InspectHostPort{} - } - mapping = append(mapping, define.InspectHostPort{ - HostIP: port.HostIP, - HostPort: fmt.Sprintf("%d", port.HostPort), - }) - settings.Ports[key] = mapping - } - } + settings.Ports = makeInspectPortBindings(c.config.PortMappings) // We can't do more if the network is down. if c.state.NetNS == nil { diff --git a/libpod/pod_api.go b/libpod/pod_api.go index a02b171e1..cc10a3355 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -481,6 +481,51 @@ func (p *Pod) Inspect() (*define.InspectPodData, error) { } } + // Infra config contains detailed information on the pod's infra + // container. + var infraConfig *define.InspectPodInfraConfig + if p.config.InfraContainer != nil && p.config.InfraContainer.HasInfraContainer { + infraConfig = new(define.InspectPodInfraConfig) + infraConfig.HostNetwork = p.config.InfraContainer.HostNetwork + infraConfig.StaticIP = p.config.InfraContainer.StaticIP + infraConfig.StaticMAC = p.config.InfraContainer.StaticMAC + infraConfig.NoManageResolvConf = p.config.InfraContainer.UseImageResolvConf + infraConfig.NoManageHosts = p.config.InfraContainer.UseImageHosts + + if len(p.config.InfraContainer.DNSServer) > 0 { + infraConfig.DNSServer = make([]string, 0, len(p.config.InfraContainer.DNSServer)) + for _, i := range p.config.InfraContainer.DNSServer { + infraConfig.DNSServer = append(infraConfig.DNSServer, i) + } + } + if len(p.config.InfraContainer.DNSSearch) > 0 { + infraConfig.DNSSearch = make([]string, 0, len(p.config.InfraContainer.DNSSearch)) + for _, i := range p.config.InfraContainer.DNSSearch { + infraConfig.DNSSearch = append(infraConfig.DNSSearch, i) + } + } + if len(p.config.InfraContainer.DNSOption) > 0 { + infraConfig.DNSOption = make([]string, 0, len(p.config.InfraContainer.DNSOption)) + for _, i := range p.config.InfraContainer.DNSOption { + infraConfig.DNSOption = append(infraConfig.DNSOption, i) + } + } + if len(p.config.InfraContainer.HostAdd) > 0 { + infraConfig.HostAdd = make([]string, 0, len(p.config.InfraContainer.HostAdd)) + for _, i := range p.config.InfraContainer.HostAdd { + infraConfig.HostAdd = append(infraConfig.HostAdd, i) + } + } + if len(p.config.InfraContainer.Networks) > 0 { + infraConfig.Networks = make([]string, 0, len(p.config.InfraContainer.Networks)) + for _, i := range p.config.InfraContainer.Networks { + infraConfig.Networks = append(infraConfig.Networks, i) + } + } + + infraConfig.PortBindings = makeInspectPortBindings(p.config.InfraContainer.PortBindings) + } + inspectData := define.InspectPodData{ ID: p.ID(), Name: p.Name(), @@ -495,7 +540,7 @@ func (p *Pod) Inspect() (*define.InspectPodData, error) { CgroupPath: p.state.CgroupPath, CreateInfra: false, InfraContainerID: p.state.InfraContainerID, - InfraConfig: nil, + InfraConfig: infraConfig, SharedNamespaces: sharesNS, NumContainers: uint(len(containers)), Containers: ctrs, diff --git a/libpod/util.go b/libpod/util.go index 7504295f0..8c2d946ba 100644 --- a/libpod/util.go +++ b/libpod/util.go @@ -15,6 +15,7 @@ import ( "github.com/containers/common/pkg/config" "github.com/containers/libpod/v2/libpod/define" "github.com/containers/libpod/v2/utils" + "github.com/cri-o/ocicni/pkg/ocicni" "github.com/fsnotify/fsnotify" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -254,3 +255,21 @@ func makeHTTPAttachHeader(stream byte, length uint32) []byte { binary.BigEndian.PutUint32(header[4:], length) return header } + +// Convert OCICNI port bindings into Inspect-formatted port bindings. +func makeInspectPortBindings(bindings []ocicni.PortMapping) map[string][]define.InspectHostPort { + portBindings := make(map[string][]define.InspectHostPort) + for _, port := range bindings { + key := fmt.Sprintf("%d/%s", port.ContainerPort, port.Protocol) + hostPorts := portBindings[key] + if hostPorts == nil { + hostPorts = []define.InspectHostPort{} + } + hostPorts = append(hostPorts, define.InspectHostPort{ + HostIP: port.HostIP, + HostPort: fmt.Sprintf("%d", port.HostPort), + }) + portBindings[key] = hostPorts + } + return portBindings +} diff --git a/test/e2e/pod_inspect_test.go b/test/e2e/pod_inspect_test.go index 5e3634435..64f943c80 100644 --- a/test/e2e/pod_inspect_test.go +++ b/test/e2e/pod_inspect_test.go @@ -3,6 +3,8 @@ package integration import ( "os" + "github.com/containers/libpod/v2/libpod/define" + . "github.com/containers/libpod/v2/test/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -79,4 +81,22 @@ var _ = Describe("Podman pod inspect", func() { index := len(inspectCreateCommand) - len(createCommand) Expect(inspectCreateCommand[index:]).To(Equal(createCommand)) }) + + It("podman pod inspect outputs port bindings", func() { + podName := "testPod" + create := podmanTest.Podman([]string{"pod", "create", "--name", testPod, "-p", "8080:80"}) + create.WaitWithDefaultTimeout() + Expect(create.ExitCode()).To(Equal(0)) + + inspectOut := podmanTest.Podman([]string{"pod", "inspect", podName}) + inspectOut.WaitWithDefaultTimeout() + Expect(inspectOut.ExitCode()).To(Equal(0)) + + inspectJSON := new(define.InspectPodData) + err := json.Unmarshal(inspectOut.Out.Contents(), inspectJSON) + Expect(err).To(BeNil()) + Expect(inspectJSON.InfraConfig).To(Not(BeNil())) + Expect(len(inspectJSON.PortBindings["80/tcp"])).To(Equal(1)) + Expect(inspectJSON.PortBindings["80/tcp"].HostPort).To(Equal("8080")) + }) }) -- cgit v1.2.3-54-g00ecf From cad36d4b7ad47ac0150c5b55472e92ca524c5b4d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 14 Jul 2020 09:07:08 -0400 Subject: Populate remaining unused fields in `pod inspect` We were hard-coding two fields to false, instead of grabbing their value from the pod config, which means that `pod inspect` would print the wrong value always. Fixes #6968 Signed-off-by: Matthew Heon --- libpod/pod_api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libpod') diff --git a/libpod/pod_api.go b/libpod/pod_api.go index cc10a3355..f889ba789 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -535,10 +535,10 @@ func (p *Pod) Inspect() (*define.InspectPodData, error) { State: podState, Hostname: p.config.Hostname, Labels: p.Labels(), - CreateCgroup: false, + CreateCgroup: p.config.UsePodCgroup, CgroupParent: p.CgroupParent(), CgroupPath: p.state.CgroupPath, - CreateInfra: false, + CreateInfra: infraConfig != nil, InfraContainerID: p.state.InfraContainerID, InfraConfig: infraConfig, SharedNamespaces: sharesNS, -- cgit v1.2.3-54-g00ecf From f74a28e08a920dd50eb9377336a6cabbe2d43ff1 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 14 Jul 2020 09:44:56 -0400 Subject: Fix lint Signed-off-by: Matthew Heon --- libpod/pod_api.go | 20 +++++--------------- test/e2e/pod_inspect_test.go | 7 ++++--- 2 files changed, 9 insertions(+), 18 deletions(-) (limited to 'libpod') diff --git a/libpod/pod_api.go b/libpod/pod_api.go index f889ba789..f2ef81bec 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -494,33 +494,23 @@ func (p *Pod) Inspect() (*define.InspectPodData, error) { if len(p.config.InfraContainer.DNSServer) > 0 { infraConfig.DNSServer = make([]string, 0, len(p.config.InfraContainer.DNSServer)) - for _, i := range p.config.InfraContainer.DNSServer { - infraConfig.DNSServer = append(infraConfig.DNSServer, i) - } + infraConfig.DNSServer = append(infraConfig.DNSServer, p.config.InfraContainer.DNSServer...) } if len(p.config.InfraContainer.DNSSearch) > 0 { infraConfig.DNSSearch = make([]string, 0, len(p.config.InfraContainer.DNSSearch)) - for _, i := range p.config.InfraContainer.DNSSearch { - infraConfig.DNSSearch = append(infraConfig.DNSSearch, i) - } + infraConfig.DNSSearch = append(infraConfig.DNSSearch, p.config.InfraContainer.DNSSearch...) } if len(p.config.InfraContainer.DNSOption) > 0 { infraConfig.DNSOption = make([]string, 0, len(p.config.InfraContainer.DNSOption)) - for _, i := range p.config.InfraContainer.DNSOption { - infraConfig.DNSOption = append(infraConfig.DNSOption, i) - } + infraConfig.DNSOption = append(infraConfig.DNSOption, p.config.InfraContainer.DNSOption...) } if len(p.config.InfraContainer.HostAdd) > 0 { infraConfig.HostAdd = make([]string, 0, len(p.config.InfraContainer.HostAdd)) - for _, i := range p.config.InfraContainer.HostAdd { - infraConfig.HostAdd = append(infraConfig.HostAdd, i) - } + infraConfig.HostAdd = append(infraConfig.HostAdd, p.config.InfraContainer.HostAdd...) } if len(p.config.InfraContainer.Networks) > 0 { infraConfig.Networks = make([]string, 0, len(p.config.InfraContainer.Networks)) - for _, i := range p.config.InfraContainer.Networks { - infraConfig.Networks = append(infraConfig.Networks, i) - } + infraConfig.Networks = append(infraConfig.Networks, p.config.InfraContainer.Networks...) } infraConfig.PortBindings = makeInspectPortBindings(p.config.InfraContainer.PortBindings) diff --git a/test/e2e/pod_inspect_test.go b/test/e2e/pod_inspect_test.go index 64f943c80..16bf1c4c9 100644 --- a/test/e2e/pod_inspect_test.go +++ b/test/e2e/pod_inspect_test.go @@ -1,6 +1,7 @@ package integration import ( + "encoding/json" "os" "github.com/containers/libpod/v2/libpod/define" @@ -84,7 +85,7 @@ var _ = Describe("Podman pod inspect", func() { It("podman pod inspect outputs port bindings", func() { podName := "testPod" - create := podmanTest.Podman([]string{"pod", "create", "--name", testPod, "-p", "8080:80"}) + create := podmanTest.Podman([]string{"pod", "create", "--name", podName, "-p", "8080:80"}) create.WaitWithDefaultTimeout() Expect(create.ExitCode()).To(Equal(0)) @@ -96,7 +97,7 @@ var _ = Describe("Podman pod inspect", func() { err := json.Unmarshal(inspectOut.Out.Contents(), inspectJSON) Expect(err).To(BeNil()) Expect(inspectJSON.InfraConfig).To(Not(BeNil())) - Expect(len(inspectJSON.PortBindings["80/tcp"])).To(Equal(1)) - Expect(inspectJSON.PortBindings["80/tcp"].HostPort).To(Equal("8080")) + Expect(len(inspectJSON.InfraConfig.PortBindings["80/tcp"])).To(Equal(1)) + Expect(inspectJSON.InfraConfig.PortBindings["80/tcp"][0].HostPort).To(Equal("8080")) }) }) -- cgit v1.2.3-54-g00ecf From 24f0175d90b78504e49882b11d72b96a79e58a3b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 14 Jul 2020 13:21:05 -0400 Subject: Add SystemdMode to inspect for containers This allows us to determine if the container auto-detected that systemd was in use, and correctly activated systemd integration. Use this to wire up some integration tests to verify that systemd integration is working properly. Signed-off-by: Matthew Heon Signed-off-by: Matthew Heon --- libpod/container_inspect.go | 1 + libpod/define/container_inspect.go | 57 ++++++++++++++++++++++++++++++++++++++ libpod/define/ctr_inspect.go | 54 ------------------------------------ test/e2e/systemd_test.go | 35 +++++++++++++++++++++++ 4 files changed, 93 insertions(+), 54 deletions(-) delete mode 100644 libpod/define/ctr_inspect.go (limited to 'libpod') diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go index 8c392d0d6..cd2e41be2 100644 --- a/libpod/container_inspect.go +++ b/libpod/container_inspect.go @@ -289,6 +289,7 @@ func (c *Container) generateInspectContainerConfig(spec *spec.Spec) *define.Insp ctrConfig.OpenStdin = c.config.Stdin ctrConfig.Image = c.config.RootfsImageName + ctrConfig.SystemdMode = c.config.Systemd // Leave empty is not explicitly overwritten by user if len(c.config.Command) != 0 { diff --git a/libpod/define/container_inspect.go b/libpod/define/container_inspect.go index a3cf4304f..7807f9e8b 100644 --- a/libpod/define/container_inspect.go +++ b/libpod/define/container_inspect.go @@ -54,6 +54,10 @@ type InspectContainerConfig struct { // CreateCommand is the full command plus arguments of the process the // container has been created with. CreateCommand []string `json:"CreateCommand,omitempty"` + // SystemdMode is whether the container is running in systemd mode. In + // systemd mode, the container configuration is customized to optimize + // running systemd in the container. + SystemdMode bool `json:"SystemdMode,omitempty"` } // InspectRestartPolicy holds information about the container's restart policy. @@ -628,3 +632,56 @@ type InspectContainerData struct { Config *InspectContainerConfig `json:"Config"` HostConfig *InspectContainerHostConfig `json:"HostConfig"` } + +// InspectExecSession contains information about a given exec session. +type InspectExecSession struct { + // CanRemove is legacy and used purely for compatibility reasons. + // Will always be set to true, unless the exec session is running. + CanRemove bool `json:"CanRemove"` + // ContainerID is the ID of the container this exec session is attached + // to. + ContainerID string `json:"ContainerID"` + // DetachKeys are the detach keys used by the exec session. + // If set to "" the default keys are being used. + // Will show "" if no detach keys are set. + DetachKeys string `json:"DetachKeys"` + // ExitCode is the exit code of the exec session. Will be set to 0 if + // the exec session has not yet exited. + ExitCode int `json:"ExitCode"` + // ID is the ID of the exec session. + ID string `json:"ID"` + // OpenStderr is whether the container's STDERR stream will be attached. + // Always set to true if the exec session created a TTY. + OpenStderr bool `json:"OpenStderr"` + // OpenStdin is whether the container's STDIN stream will be attached + // to. + OpenStdin bool `json:"OpenStdin"` + // OpenStdout is whether the container's STDOUT stream will be attached. + // Always set to true if the exec session created a TTY. + OpenStdout bool `json:"OpenStdout"` + // Running is whether the exec session is running. + Running bool `json:"Running"` + // Pid is the PID of the exec session's process. + // Will be set to 0 if the exec session is not running. + Pid int `json:"Pid"` + // ProcessConfig contains information about the exec session's process. + ProcessConfig *InspectExecProcess `json:"ProcessConfig"` +} + +// InspectExecProcess contains information about the process in a given exec +// session. +type InspectExecProcess struct { + // Arguments are the arguments to the entrypoint command of the exec + // session. + Arguments []string `json:"arguments"` + // Entrypoint is the entrypoint for the exec session (the command that + // will be executed in the container). + Entrypoint string `json:"entrypoint"` + // Privileged is whether the exec session will be started with elevated + // privileges. + Privileged bool `json:"privileged"` + // Tty is whether the exec session created a terminal. + Tty bool `json:"tty"` + // User is the user the exec session was started as. + User string `json:"user"` +} diff --git a/libpod/define/ctr_inspect.go b/libpod/define/ctr_inspect.go deleted file mode 100644 index b7cd13f82..000000000 --- a/libpod/define/ctr_inspect.go +++ /dev/null @@ -1,54 +0,0 @@ -package define - -// InspectExecSession contains information about a given exec session. -type InspectExecSession struct { - // CanRemove is legacy and used purely for compatibility reasons. - // Will always be set to true, unless the exec session is running. - CanRemove bool `json:"CanRemove"` - // ContainerID is the ID of the container this exec session is attached - // to. - ContainerID string `json:"ContainerID"` - // DetachKeys are the detach keys used by the exec session. - // If set to "" the default keys are being used. - // Will show "" if no detach keys are set. - DetachKeys string `json:"DetachKeys"` - // ExitCode is the exit code of the exec session. Will be set to 0 if - // the exec session has not yet exited. - ExitCode int `json:"ExitCode"` - // ID is the ID of the exec session. - ID string `json:"ID"` - // OpenStderr is whether the container's STDERR stream will be attached. - // Always set to true if the exec session created a TTY. - OpenStderr bool `json:"OpenStderr"` - // OpenStdin is whether the container's STDIN stream will be attached - // to. - OpenStdin bool `json:"OpenStdin"` - // OpenStdout is whether the container's STDOUT stream will be attached. - // Always set to true if the exec session created a TTY. - OpenStdout bool `json:"OpenStdout"` - // Running is whether the exec session is running. - Running bool `json:"Running"` - // Pid is the PID of the exec session's process. - // Will be set to 0 if the exec session is not running. - Pid int `json:"Pid"` - // ProcessConfig contains information about the exec session's process. - ProcessConfig *InspectExecProcess `json:"ProcessConfig"` -} - -// InspectExecProcess contains information about the process in a given exec -// session. -type InspectExecProcess struct { - // Arguments are the arguments to the entrypoint command of the exec - // session. - Arguments []string `json:"arguments"` - // Entrypoint is the entrypoint for the exec session (the command that - // will be executed in the container). - Entrypoint string `json:"entrypoint"` - // Privileged is whether the exec session will be started with elevated - // privileges. - Privileged bool `json:"privileged"` - // Tty is whether the exec session created a terminal. - Tty bool `json:"tty"` - // User is the user the exec session was started as. - User string `json:"user"` -} diff --git a/test/e2e/systemd_test.go b/test/e2e/systemd_test.go index 143b8f59f..05c0d3fd0 100644 --- a/test/e2e/systemd_test.go +++ b/test/e2e/systemd_test.go @@ -112,5 +112,40 @@ WantedBy=multi-user.target systemctl.WaitWithDefaultTimeout() Expect(systemctl.ExitCode()).To(Equal(0)) Expect(strings.Contains(systemctl.OutputToString(), "State:")).To(BeTrue()) + + result := podmanTest.Podman([]string{"inspect", ctrName}) + result.WaitWithDefaultTimeout() + Expect(result.ExitCode()).To(Equal(0)) + conData := result.InspectContainerToJSON() + Expect(len(conData)).To(Equal(1)) + Expect(conData[0].Config.SystemdMode).To(BeTrue()) + }) + + It("podman create container with systemd entrypoint triggers systemd mode", func() { + ctrName := "testCtr" + run := podmanTest.Podman([]string{"create", "--name", ctrName, "--entrypoint", "/sbin/init", ubi_init}) + run.WaitWithDefaultTimeout() + Expect(run.ExitCode()).To(Equal(0)) + + result := podmanTest.Podman([]string{"inspect", ctrName}) + result.WaitWithDefaultTimeout() + Expect(result.ExitCode()).To(Equal(0)) + conData := result.InspectContainerToJSON() + Expect(len(conData)).To(Equal(1)) + Expect(conData[0].Config.SystemdMode).To(BeTrue()) + }) + + It("podman create container with systemd=always triggers systemd mode", func() { + ctrName := "testCtr" + run := podmanTest.Podman([]string{"create", "--name", ctrName, "--systemd", "always", ALPINE}) + run.WaitWithDefaultTimeout() + Expect(run.ExitCode()).To(Equal(0)) + + result := podmanTest.Podman([]string{"inspect", ctrName}) + result.WaitWithDefaultTimeout() + Expect(result.ExitCode()).To(Equal(0)) + conData := result.InspectContainerToJSON() + Expect(len(conData)).To(Equal(1)) + Expect(conData[0].Config.SystemdMode).To(BeTrue()) }) }) -- cgit v1.2.3-54-g00ecf From 0630d19b34c6cea674fc5f830fe308b8a1206259 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 7 Jul 2020 17:19:59 -0400 Subject: Fix container and pod create commands for remote create In `podman inspect` output for containers and pods, we include the command that was used to create the container. This is also used by `podman generate systemd --new` to generate unit files. With remote podman, the generated create commands were incorrect since we sourced directly from os.Args on the server side, which was guaranteed to be `podman system service` (or some variant thereof). The solution is to pass the command along in the Specgen or PodSpecgen, where we can source it from the client's os.Args. This will still be VERY iffy for mixed local/remote use (doing a `podman --remote run ...` on a remote client then a `podman generate systemd --new` on the server on the same container will not work, because the `--remote` flag will slip in) but at the very least the output of `podman inspect` will be correct. We can look into properly handling `--remote` (parsing it out would be a little iffy) in a future PR. Signed-off-by: Matthew Heon Signed-off-by: Matthew Heon --- cmd/podman/common/specgen.go | 3 +++ cmd/podman/pods/create.go | 2 ++ libpod/options.go | 8 ++++---- pkg/domain/entities/pods.go | 2 ++ pkg/spec/createconfig.go | 4 ---- pkg/specgen/generate/container_create.go | 4 +++- pkg/specgen/generate/pod_create.go | 4 +++- pkg/specgen/podspecgen.go | 6 ++++++ pkg/specgen/specgen.go | 7 +++++++ pkg/varlinkapi/create.go | 4 ---- 10 files changed, 30 insertions(+), 14 deletions(-) (limited to 'libpod') diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go index b4f786da2..3722d45b2 100644 --- a/cmd/podman/common/specgen.go +++ b/cmd/podman/common/specgen.go @@ -378,6 +378,9 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string } var command []string + // Include the command used to create the container. + s.ContainerCreateCommand = os.Args + // Build the command // If we have an entry point, it goes first if c.Entrypoint != nil { diff --git a/cmd/podman/pods/create.go b/cmd/podman/pods/create.go index 0e2a085fd..d57a2f2f7 100644 --- a/cmd/podman/pods/create.go +++ b/cmd/podman/pods/create.go @@ -149,6 +149,8 @@ func create(cmd *cobra.Command, args []string) error { } } + createOptions.CreateCommand = os.Args + if replace { if err := replacePod(createOptions.Name); err != nil { return err diff --git a/libpod/options.go b/libpod/options.go index 3120a35d7..bff3f3c18 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1371,12 +1371,12 @@ func WithHealthCheck(healthCheck *manifest.Schema2HealthConfig) CtrCreateOption // WithCreateCommand adds the full command plus arguments of the current // process to the container config. -func WithCreateCommand() CtrCreateOption { +func WithCreateCommand(cmd []string) CtrCreateOption { return func(ctr *Container) error { if ctr.valid { return define.ErrCtrFinalized } - ctr.config.CreateCommand = os.Args + ctr.config.CreateCommand = cmd return nil } } @@ -1553,12 +1553,12 @@ func WithPodHostname(hostname string) PodCreateOption { // WithPodCreateCommand adds the full command plus arguments of the current // process to the pod config. -func WithPodCreateCommand() PodCreateOption { +func WithPodCreateCommand(createCmd []string) PodCreateOption { return func(pod *Pod) error { if pod.valid { return define.ErrPodFinalized } - pod.config.CreateCommand = os.Args + pod.config.CreateCommand = createCmd return nil } } diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index 8023034ef..9e9b834ef 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -104,6 +104,7 @@ type PodRmReport struct { type PodCreateOptions struct { CGroupParent string + CreateCommand []string Hostname string Infra bool InfraImage string @@ -133,6 +134,7 @@ func (p PodCreateOptions) ToPodSpecGen(s *specgen.PodSpecGenerator) { } s.InfraImage = p.InfraImage s.SharedNamespaces = p.Share + s.PodCreateCommand = p.CreateCommand // Networking config s.NetNS = p.Net.Network diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index 879c66895..55c3238d2 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -406,10 +406,6 @@ func CreateContainerFromCreateConfig(ctx context.Context, r *libpod.Runtime, cre return nil, err } - // Set the CreateCommand explicitly. Some (future) consumers of libpod - // might not want to set it. - options = append(options, libpod.WithCreateCommand()) - ctr, err := r.NewContainer(ctx, runtimeSpec, options...) if err != nil { return nil, err diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index daf7f84d4..be1e3b48e 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -78,7 +78,9 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener } options := []libpod.CtrCreateOption{} - options = append(options, libpod.WithCreateCommand()) + if s.ContainerCreateCommand != nil { + options = append(options, libpod.WithCreateCommand(s.ContainerCreateCommand)) + } var newImage *image.Image if s.Rootfs != "" { diff --git a/pkg/specgen/generate/pod_create.go b/pkg/specgen/generate/pod_create.go index 690651a23..4fe1b6435 100644 --- a/pkg/specgen/generate/pod_create.go +++ b/pkg/specgen/generate/pod_create.go @@ -93,7 +93,9 @@ func createPodOptions(p *specgen.PodSpecGenerator) ([]libpod.PodCreateOption, er options = append(options, libpod.WithInfraContainerPorts(ports)) } options = append(options, libpod.WithPodCgroups()) - options = append(options, libpod.WithPodCreateCommand()) + if p.PodCreateCommand != nil { + options = append(options, libpod.WithPodCreateCommand(p.PodCreateCommand)) + } if len(p.InfraConmonPidFile) > 0 { options = append(options, libpod.WithInfraConmonPidFile(p.InfraConmonPidFile)) } diff --git a/pkg/specgen/podspecgen.go b/pkg/specgen/podspecgen.go index 600d27004..3c32ec365 100644 --- a/pkg/specgen/podspecgen.go +++ b/pkg/specgen/podspecgen.go @@ -49,6 +49,12 @@ type PodBasicConfig struct { // Conflicts with NoInfra=true. // Optional. SharedNamespaces []string `json:"shared_namespaces,omitempty"` + // PodCreateCommand is the command used to create this pod. + // This will be shown in the output of Inspect() on the pod, and may + // also be used by some tools that wish to recreate the pod + // (e.g. `podman generate systemd --new`). + // Optional. + PodCreateCommand []string `json:"pod_create_command,omitempty"` } // PodNetworkConfig contains networking configuration for a pod. diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 91bc36709..bd738f5a7 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -131,6 +131,13 @@ type ContainerBasicConfig struct { // Remove indicates if the container should be removed once it has been started // and exits Remove bool `json:"remove,omitempty"` + // ContainerCreateCommand is the command that was used to create this + // container. + // This will be shown in the output of Inspect() on the container, and + // may also be used by some tools that wish to recreate the container + // (e.g. `podman generate systemd --new`). + // Optional. + ContainerCreateCommand []string `json:"containerCreateCommand,omitempty"` } // ContainerStorageConfig contains information on the storage configuration of a diff --git a/pkg/varlinkapi/create.go b/pkg/varlinkapi/create.go index 5c5f075f7..ac93939d9 100644 --- a/pkg/varlinkapi/create.go +++ b/pkg/varlinkapi/create.go @@ -915,10 +915,6 @@ func CreateContainerFromCreateConfig(ctx context.Context, r *libpod.Runtime, cre return nil, err } - // Set the CreateCommand explicitly. Some (future) consumers of libpod - // might not want to set it. - options = append(options, libpod.WithCreateCommand()) - ctr, err := r.NewContainer(ctx, runtimeSpec, options...) if err != nil { return nil, err -- cgit v1.2.3-54-g00ecf From f9eb204d0ddb7e0d5fbffc849bd0ca10e2b17a29 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 9 Jul 2020 13:50:01 -0400 Subject: Remove all instances of named return "err" from Libpod This was inspired by https://github.com/cri-o/cri-o/pull/3934 and much of the logic for it is contained there. However, in brief, a named return called "err" can cause lots of code confusion and encourages using the wrong err variable in defer statements, which can make them work incorrectly. Using a separate name which is not used elsewhere makes it very clear what the defer should be doing. As part of this, remove a large number of named returns that were not used anywhere. Most of them were once needed, but are no longer necessary after previous refactors (but were accidentally retained). Signed-off-by: Matthew Heon --- libpod/container_api.go | 12 ++++----- libpod/container_exec.go | 20 +++++++-------- libpod/container_internal.go | 44 ++++++++++++++++---------------- libpod/container_internal_linux.go | 21 ++++++++------- libpod/container_internal_unsupported.go | 2 +- libpod/events/events_linux.go | 10 ++++---- libpod/networking_linux.go | 18 ++++++------- libpod/networking_unsupported.go | 6 ++--- libpod/oci_conmon_linux.go | 9 ++++--- libpod/runtime.go | 17 ++++++------ libpod/runtime_ctr.go | 22 ++++++++-------- libpod/state_test.go | 8 +++--- libpod/storage.go | 8 +++--- 13 files changed, 98 insertions(+), 99 deletions(-) (limited to 'libpod') diff --git a/libpod/container_api.go b/libpod/container_api.go index b37b05ff2..d19dc651b 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -29,7 +29,7 @@ import ( // Init requires that all dependency containers be started (e.g. pod infra // containers). The `recursive` parameter will, if set to true, start these // dependency containers before initializing this container. -func (c *Container) Init(ctx context.Context, recursive bool) (err error) { +func (c *Container) Init(ctx context.Context, recursive bool) error { span, _ := opentracing.StartSpanFromContext(ctx, "containerInit") span.SetTag("struct", "container") defer span.Finish() @@ -85,7 +85,7 @@ func (c *Container) Init(ctx context.Context, recursive bool) (err error) { // Start requites that all dependency containers (e.g. pod infra containers) be // running before being run. The recursive parameter, if set, will start all // dependencies before starting this container. -func (c *Container) Start(ctx context.Context, recursive bool) (err error) { +func (c *Container) Start(ctx context.Context, recursive bool) error { span, _ := opentracing.StartSpanFromContext(ctx, "containerStart") span.SetTag("struct", "container") defer span.Finish() @@ -112,7 +112,7 @@ func (c *Container) Start(ctx context.Context, recursive bool) (err error) { // Attach call occurs before Start). // In overall functionality, it is identical to the Start call, with the added // side effect that an attach session will also be started. -func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, recursive bool) (attachResChan <-chan error, err error) { +func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, recursive bool) (<-chan error, error) { if !c.batched { c.lock.Lock() defer c.lock.Unlock() @@ -150,7 +150,7 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachSt } // RestartWithTimeout restarts a running container and takes a given timeout in uint -func (c *Container) RestartWithTimeout(ctx context.Context, timeout uint) (err error) { +func (c *Container) RestartWithTimeout(ctx context.Context, timeout uint) error { if !c.batched { c.lock.Lock() defer c.lock.Unlock() @@ -160,7 +160,7 @@ func (c *Container) RestartWithTimeout(ctx context.Context, timeout uint) (err e } } - if err = c.checkDependenciesAndHandleError(); err != nil { + if err := c.checkDependenciesAndHandleError(); err != nil { return err } @@ -760,7 +760,7 @@ func (c *Container) Checkpoint(ctx context.Context, options ContainerCheckpointO } // Restore restores a container -func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOptions) (err error) { +func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOptions) error { logrus.Debugf("Trying to restore container %s", c.ID()) if !c.batched { c.lock.Lock() diff --git a/libpod/container_exec.go b/libpod/container_exec.go index 69da6fcfe..bd04ee9b9 100644 --- a/libpod/container_exec.go +++ b/libpod/container_exec.go @@ -779,25 +779,25 @@ func (c *Container) execOCILog(sessionID string) string { } // create a bundle path and associated files for an exec session -func (c *Container) createExecBundle(sessionID string) (err error) { +func (c *Container) createExecBundle(sessionID string) (retErr error) { bundlePath := c.execBundlePath(sessionID) - if createErr := os.MkdirAll(bundlePath, execDirPermission); createErr != nil { - return createErr + if err := os.MkdirAll(bundlePath, execDirPermission); err != nil { + return err } defer func() { - if err != nil { - if err2 := os.RemoveAll(bundlePath); err != nil { - logrus.Warnf("error removing exec bundle after creation caused another error: %v", err2) + if retErr != nil { + if err := os.RemoveAll(bundlePath); err != nil { + logrus.Warnf("error removing exec bundle after creation caused another error: %v", err) } } }() - if err2 := os.MkdirAll(c.execExitFileDir(sessionID), execDirPermission); err2 != nil { + if err := os.MkdirAll(c.execExitFileDir(sessionID), execDirPermission); err != nil { // The directory is allowed to exist - if !os.IsExist(err2) { - err = errors.Wrapf(err2, "error creating OCI runtime exit file path %s", c.execExitFileDir(sessionID)) + if !os.IsExist(err) { + return errors.Wrapf(err, "error creating OCI runtime exit file path %s", c.execExitFileDir(sessionID)) } } - return + return nil } // readExecExitCode reads the exit file for an exec session and returns diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 7a547e565..c44ba5fe6 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -209,7 +209,7 @@ func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error { // Handle container restart policy. // This is called when a container has exited, and was not explicitly stopped by // an API call to stop the container or pod it is in. -func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, err error) { +func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr error) { // If we did not get a restart policy match, exit immediately. // Do the same if we're not a policy that restarts. if !c.state.RestartPolicyMatch || @@ -240,7 +240,7 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, er logrus.Debugf("Restarting container %s due to restart policy %s", c.ID(), c.config.RestartPolicy) // Need to check if dependencies are alive. - if err = c.checkDependenciesAndHandleError(); err != nil { + if err := c.checkDependenciesAndHandleError(); err != nil { return false, err } @@ -262,9 +262,9 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (restarted bool, er } defer func() { - if err != nil { - if err2 := c.cleanup(ctx); err2 != nil { - logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) + if retErr != nil { + if err := c.cleanup(ctx); err != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err) } } }() @@ -767,7 +767,7 @@ func (c *Container) save() error { // Checks the container is in the right state, then initializes the container in preparation to start the container. // If recursive is true, each of the containers dependencies will be started. // Otherwise, this function will return with error if there are dependencies of this container that aren't running. -func (c *Container) prepareToStart(ctx context.Context, recursive bool) (err error) { +func (c *Container) prepareToStart(ctx context.Context, recursive bool) (retErr error) { // Container must be created or stopped to be started if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID()) @@ -784,9 +784,9 @@ func (c *Container) prepareToStart(ctx context.Context, recursive bool) (err err } defer func() { - if err != nil { - if err2 := c.cleanup(ctx); err2 != nil { - logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) + if retErr != nil { + if err := c.cleanup(ctx); err != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err) } } }() @@ -1132,7 +1132,7 @@ func (c *Container) reinit(ctx context.Context, retainRetries bool) error { // Initialize (if necessary) and start a container // Performs all necessary steps to start a container that is not running // Does not lock or check validity -func (c *Container) initAndStart(ctx context.Context) (err error) { +func (c *Container) initAndStart(ctx context.Context) (retErr error) { // If we are ContainerStateUnknown, throw an error if c.state.State == define.ContainerStateUnknown { return errors.Wrapf(define.ErrCtrStateInvalid, "container %s is in an unknown state", c.ID()) @@ -1150,9 +1150,9 @@ func (c *Container) initAndStart(ctx context.Context) (err error) { } defer func() { - if err != nil { - if err2 := c.cleanup(ctx); err2 != nil { - logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) + if retErr != nil { + if err := c.cleanup(ctx); err != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err) } } }() @@ -1321,7 +1321,7 @@ func (c *Container) unpause() error { } // Internal, non-locking function to restart a container -func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (err error) { +func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (retErr error) { if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateRunning, define.ContainerStateStopped, define.ContainerStateExited) { return errors.Wrapf(define.ErrCtrStateInvalid, "unable to restart a container in a paused or unknown state") } @@ -1358,9 +1358,9 @@ func (c *Container) restartWithTimeout(ctx context.Context, timeout uint) (err e } } defer func() { - if err != nil { - if err2 := c.cleanup(ctx); err2 != nil { - logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) + if retErr != nil { + if err := c.cleanup(ctx); err != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err) } } }() @@ -1682,7 +1682,7 @@ func (c *Container) cleanup(ctx context.Context) error { // delete deletes the container and runs any configured poststop // hooks. -func (c *Container) delete(ctx context.Context) (err error) { +func (c *Container) delete(ctx context.Context) error { span, _ := opentracing.StartSpanFromContext(ctx, "delete") span.SetTag("struct", "container") defer span.Finish() @@ -1701,7 +1701,7 @@ func (c *Container) delete(ctx context.Context) (err error) { // postDeleteHooks runs the poststop hooks (if any) as specified by // the OCI Runtime Specification (which requires them to run // post-delete, despite the stage name). -func (c *Container) postDeleteHooks(ctx context.Context) (err error) { +func (c *Container) postDeleteHooks(ctx context.Context) error { span, _ := opentracing.StartSpanFromContext(ctx, "postDeleteHooks") span.SetTag("struct", "container") defer span.Finish() @@ -1824,7 +1824,7 @@ func (c *Container) saveSpec(spec *spec.Spec) error { } // Warning: precreate hooks may alter 'config' in place. -func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (extensionStageHooks map[string][]spec.Hook, err error) { +func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (map[string][]spec.Hook, error) { allHooks := make(map[string][]spec.Hook) if c.runtime.config.Engine.HooksDir == nil { if rootless.IsRootless() { @@ -1938,7 +1938,7 @@ func (c *Container) checkReadyForRemoval() error { // writeJSONFile marshalls and writes the given data to a JSON file // in the bundle path -func (c *Container) writeJSONFile(v interface{}, file string) (err error) { +func (c *Container) writeJSONFile(v interface{}, file string) error { fileJSON, err := json.MarshalIndent(v, "", " ") if err != nil { return errors.Wrapf(err, "error writing JSON to %s for container %s", file, c.ID()) @@ -1953,7 +1953,7 @@ func (c *Container) writeJSONFile(v interface{}, file string) (err error) { // prepareCheckpointExport writes the config and spec to // JSON files for later export -func (c *Container) prepareCheckpointExport() (err error) { +func (c *Container) prepareCheckpointExport() error { // save live config if err := c.writeJSONFile(c.Config(), "config.dump"); err != nil { return err diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 018e2d5a4..cfcf9b823 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -610,7 +610,7 @@ func (c *Container) addNamespaceContainer(g *generate.Generator, ns LinuxNS, ctr return nil } -func (c *Container) exportCheckpoint(dest string, ignoreRootfs bool) (err error) { +func (c *Container) exportCheckpoint(dest string, ignoreRootfs bool) error { if (len(c.config.NamedVolumes) > 0) || (len(c.Dependencies()) > 0) { return errors.Errorf("Cannot export checkpoints of containers with named volumes or dependencies") } @@ -721,7 +721,7 @@ func (c *Container) exportCheckpoint(dest string, ignoreRootfs bool) (err error) return nil } -func (c *Container) checkpointRestoreSupported() (err error) { +func (c *Container) checkpointRestoreSupported() error { if !criu.CheckForCriu() { return errors.Errorf("Checkpoint/Restore requires at least CRIU %d", criu.MinCriuVersion) } @@ -731,7 +731,7 @@ func (c *Container) checkpointRestoreSupported() (err error) { return nil } -func (c *Container) checkpointRestoreLabelLog(fileName string) (err error) { +func (c *Container) checkpointRestoreLabelLog(fileName string) error { // Create the CRIU log file and label it dumpLog := filepath.Join(c.bundlePath(), fileName) @@ -748,7 +748,7 @@ func (c *Container) checkpointRestoreLabelLog(fileName string) (err error) { return nil } -func (c *Container) checkpoint(ctx context.Context, options ContainerCheckpointOptions) (err error) { +func (c *Container) checkpoint(ctx context.Context, options ContainerCheckpointOptions) error { if err := c.checkpointRestoreSupported(); err != nil { return err } @@ -818,7 +818,7 @@ func (c *Container) checkpoint(ctx context.Context, options ContainerCheckpointO return c.save() } -func (c *Container) importCheckpoint(input string) (err error) { +func (c *Container) importCheckpoint(input string) error { archiveFile, err := os.Open(input) if err != nil { return errors.Wrapf(err, "Failed to open checkpoint archive %s for import", input) @@ -847,8 +847,7 @@ func (c *Container) importCheckpoint(input string) (err error) { return nil } -func (c *Container) restore(ctx context.Context, options ContainerCheckpointOptions) (err error) { - +func (c *Container) restore(ctx context.Context, options ContainerCheckpointOptions) (retErr error) { if err := c.checkpointRestoreSupported(); err != nil { return err } @@ -858,7 +857,7 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti } if options.TargetFile != "" { - if err = c.importCheckpoint(options.TargetFile); err != nil { + if err := c.importCheckpoint(options.TargetFile); err != nil { return err } } @@ -944,9 +943,9 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti } defer func() { - if err != nil { - if err2 := c.cleanup(ctx); err2 != nil { - logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) + if retErr != nil { + if err := c.cleanup(ctx); err != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err) } } }() diff --git a/libpod/container_internal_unsupported.go b/libpod/container_internal_unsupported.go index a42c1d735..e6d94104c 100644 --- a/libpod/container_internal_unsupported.go +++ b/libpod/container_internal_unsupported.go @@ -18,7 +18,7 @@ func (c *Container) unmountSHM(mount string) error { return define.ErrNotImplemented } -func (c *Container) prepare() (err error) { +func (c *Container) prepare() error { return define.ErrNotImplemented } diff --git a/libpod/events/events_linux.go b/libpod/events/events_linux.go index ffb100be8..482d7d6dd 100644 --- a/libpod/events/events_linux.go +++ b/libpod/events/events_linux.go @@ -8,20 +8,20 @@ import ( ) // NewEventer creates an eventer based on the eventer type -func NewEventer(options EventerOptions) (eventer Eventer, err error) { +func NewEventer(options EventerOptions) (Eventer, error) { logrus.Debugf("Initializing event backend %s", options.EventerType) switch strings.ToUpper(options.EventerType) { case strings.ToUpper(Journald.String()): - eventer, err = newEventJournalD(options) + eventer, err := newEventJournalD(options) if err != nil { return nil, errors.Wrapf(err, "eventer creation") } + return eventer, nil case strings.ToUpper(LogFile.String()): - eventer = EventLogFile{options} + return EventLogFile{options}, nil case strings.ToUpper(Null.String()): - eventer = NewNullEventer() + return NewNullEventer(), nil default: return nil, errors.Errorf("unknown event logger type: %s", strings.ToUpper(options.EventerType)) } - return eventer, nil } diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 5cc282f3a..1e79e8732 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -141,18 +141,18 @@ func (r *Runtime) configureNetNS(ctr *Container, ctrNS ns.NetNS) ([]*cnitypes.Re } // Create and configure a new network namespace for a container -func (r *Runtime) createNetNS(ctr *Container) (n ns.NetNS, q []*cnitypes.Result, err error) { +func (r *Runtime) createNetNS(ctr *Container) (n ns.NetNS, q []*cnitypes.Result, retErr error) { ctrNS, err := netns.NewNS() if err != nil { return nil, nil, errors.Wrapf(err, "error creating network namespace for container %s", ctr.ID()) } defer func() { - if err != nil { - if err2 := netns.UnmountNS(ctrNS); err2 != nil { - logrus.Errorf("Error unmounting partially created network namespace for container %s: %v", ctr.ID(), err2) + if retErr != nil { + if err := netns.UnmountNS(ctrNS); err != nil { + logrus.Errorf("Error unmounting partially created network namespace for container %s: %v", ctr.ID(), err) } - if err2 := ctrNS.Close(); err2 != nil { - logrus.Errorf("Error closing partially created network namespace for container %s: %v", ctr.ID(), err2) + if err := ctrNS.Close(); err != nil { + logrus.Errorf("Error closing partially created network namespace for container %s: %v", ctr.ID(), err) } } }() @@ -188,7 +188,7 @@ func checkSlirpFlags(path string) (*slirpFeatures, error) { } // Configure the network namespace for a rootless container -func (r *Runtime) setupRootlessNetNS(ctr *Container) (err error) { +func (r *Runtime) setupRootlessNetNS(ctr *Container) error { path := r.config.Engine.NetworkCmdPath if path == "" { @@ -342,7 +342,7 @@ func waitForSync(syncR *os.File, cmd *exec.Cmd, logFile io.ReadSeeker, timeout t return nil } -func (r *Runtime) setupRootlessPortMapping(ctr *Container, netnsPath string) (err error) { +func (r *Runtime) setupRootlessPortMapping(ctr *Container, netnsPath string) error { syncR, syncW, err := os.Pipe() if err != nil { return errors.Wrapf(err, "failed to open pipe") @@ -420,7 +420,7 @@ func (r *Runtime) setupRootlessPortMapping(ctr *Container, netnsPath string) (er } // Configure the network namespace using the container process -func (r *Runtime) setupNetNS(ctr *Container) (err error) { +func (r *Runtime) setupNetNS(ctr *Container) error { nsProcess := fmt.Sprintf("/proc/%d/ns/net", ctr.state.PID) b := make([]byte, 16) diff --git a/libpod/networking_unsupported.go b/libpod/networking_unsupported.go index d5189709c..69f470ff7 100644 --- a/libpod/networking_unsupported.go +++ b/libpod/networking_unsupported.go @@ -4,11 +4,11 @@ package libpod import "github.com/containers/libpod/v2/libpod/define" -func (r *Runtime) setupRootlessNetNS(ctr *Container) (err error) { +func (r *Runtime) setupRootlessNetNS(ctr *Container) error { return define.ErrNotImplemented } -func (r *Runtime) setupNetNS(ctr *Container) (err error) { +func (r *Runtime) setupNetNS(ctr *Container) error { return define.ErrNotImplemented } @@ -16,7 +16,7 @@ func (r *Runtime) teardownNetNS(ctr *Container) error { return define.ErrNotImplemented } -func (r *Runtime) createNetNS(ctr *Container) (err error) { +func (r *Runtime) createNetNS(ctr *Container) error { return define.ErrNotImplemented } diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index e9d5cbaa3..e38646eec 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -190,7 +190,7 @@ func hasCurrentUserMapped(ctr *Container) bool { } // CreateContainer creates a container. -func (r *ConmonOCIRuntime) CreateContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) (err error) { +func (r *ConmonOCIRuntime) CreateContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) error { if !hasCurrentUserMapped(ctr) { for _, i := range []string{ctr.state.RunDir, ctr.runtime.config.Engine.TmpDir, ctr.config.StaticDir, ctr.state.Mountpoint, ctr.runtime.config.Engine.VolumePath} { if err := makeAccessible(i, ctr.RootUID(), ctr.RootGID()); err != nil { @@ -850,7 +850,7 @@ func (r *ConmonOCIRuntime) getLogTag(ctr *Container) (string, error) { } // createOCIContainer generates this container's main conmon instance and prepares it for starting -func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) (err error) { +func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) error { var stderrBuf bytes.Buffer runtimeDir, err := util.GetRuntimeDir() @@ -1297,8 +1297,9 @@ func (r *ConmonOCIRuntime) moveConmonToCgroupAndSignal(ctr *Container, cmd *exec return nil } -// newPipe creates a unix socket pair for communication -func newPipe() (parent *os.File, child *os.File, err error) { +// newPipe creates a unix socket pair for communication. +// Returns two files - first is parent, second is child. +func newPipe() (*os.File, *os.File, error) { fds, err := unix.Socketpair(unix.AF_LOCAL, unix.SOCK_SEQPACKET|unix.SOCK_CLOEXEC, 0) if err != nil { return nil, nil, err diff --git a/libpod/runtime.go b/libpod/runtime.go index bf1e203c1..24370d50e 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -126,7 +126,7 @@ func SetXdgDirs() error { // NewRuntime creates a new container runtime // Options can be passed to override the default configuration for the runtime -func NewRuntime(ctx context.Context, options ...RuntimeOption) (runtime *Runtime, err error) { +func NewRuntime(ctx context.Context, options ...RuntimeOption) (*Runtime, error) { conf, err := config.NewConfig("") if err != nil { return nil, err @@ -140,13 +140,13 @@ func NewRuntime(ctx context.Context, options ...RuntimeOption) (runtime *Runtime // functions can be used to mutate this configuration further. // An error will be returned if the configuration file at the given path does // not exist or cannot be loaded -func NewRuntimeFromConfig(ctx context.Context, userConfig *config.Config, options ...RuntimeOption) (runtime *Runtime, err error) { +func NewRuntimeFromConfig(ctx context.Context, userConfig *config.Config, options ...RuntimeOption) (*Runtime, error) { return newRuntimeFromConfig(ctx, userConfig, options...) } -func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...RuntimeOption) (runtime *Runtime, err error) { - runtime = new(Runtime) +func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...RuntimeOption) (*Runtime, error) { + runtime := new(Runtime) if conf.Engine.OCIRuntime == "" { conf.Engine.OCIRuntime = "runc" @@ -236,7 +236,7 @@ func getLockManager(runtime *Runtime) (lock.Manager, error) { // Make a new runtime based on the given configuration // Sets up containers/storage, state store, OCI runtime -func makeRuntime(ctx context.Context, runtime *Runtime) (err error) { +func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) { // Find a working conmon binary cPath, err := runtime.config.FindConmon() if err != nil { @@ -316,12 +316,11 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (err error) { return err } defer func() { - if err != nil && store != nil { + if retErr != nil && store != nil { // Don't forcibly shut down // We could be opening a store in use by another libpod - _, err2 := store.Shutdown(false) - if err2 != nil { - logrus.Errorf("Error removing store for partially-created runtime: %s", err2) + if _, err := store.Shutdown(false); err != nil { + logrus.Errorf("Error removing store for partially-created runtime: %s", err) } } }() diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index c073aabb5..e2bb696b0 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -34,7 +34,7 @@ type CtrCreateOption func(*Container) error type ContainerFilter func(*Container) bool // NewContainer creates a new container from a given OCI config. -func (r *Runtime) NewContainer(ctx context.Context, rSpec *spec.Spec, options ...CtrCreateOption) (c *Container, err error) { +func (r *Runtime) NewContainer(ctx context.Context, rSpec *spec.Spec, options ...CtrCreateOption) (*Container, error) { r.lock.Lock() defer r.lock.Unlock() if !r.valid { @@ -44,7 +44,7 @@ func (r *Runtime) NewContainer(ctx context.Context, rSpec *spec.Spec, options .. } // RestoreContainer re-creates a container from an imported checkpoint -func (r *Runtime) RestoreContainer(ctx context.Context, rSpec *spec.Spec, config *ContainerConfig) (c *Container, err error) { +func (r *Runtime) RestoreContainer(ctx context.Context, rSpec *spec.Spec, config *ContainerConfig) (*Container, error) { r.lock.Lock() defer r.lock.Unlock() if !r.valid { @@ -68,7 +68,7 @@ func (r *Runtime) RestoreContainer(ctx context.Context, rSpec *spec.Spec, config return r.setupContainer(ctx, ctr) } -func (r *Runtime) initContainerVariables(rSpec *spec.Spec, config *ContainerConfig) (c *Container, err error) { +func (r *Runtime) initContainerVariables(rSpec *spec.Spec, config *ContainerConfig) (*Container, error) { if rSpec == nil { return nil, errors.Wrapf(define.ErrInvalidArg, "must provide a valid runtime spec to create container") } @@ -122,7 +122,7 @@ func (r *Runtime) initContainerVariables(rSpec *spec.Spec, config *ContainerConf return ctr, nil } -func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ...CtrCreateOption) (c *Container, err error) { +func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ...CtrCreateOption) (*Container, error) { span, _ := opentracing.StartSpanFromContext(ctx, "newContainer") span.SetTag("type", "runtime") defer span.Finish() @@ -141,7 +141,7 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options .. return r.setupContainer(ctx, ctr) } -func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Container, err error) { +func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Container, retErr error) { // Validate the container if err := ctr.validate(); err != nil { return nil, err @@ -157,9 +157,9 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai logrus.Debugf("Allocated lock %d for container %s", ctr.lock.ID(), ctr.ID()) defer func() { - if err != nil { - if err2 := ctr.lock.Free(); err2 != nil { - logrus.Errorf("Error freeing lock for container after creation failed: %v", err2) + if retErr != nil { + if err := ctr.lock.Free(); err != nil { + logrus.Errorf("Error freeing lock for container after creation failed: %v", err) } } }() @@ -272,9 +272,9 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai return nil, err } defer func() { - if err != nil { - if err2 := ctr.teardownStorage(); err2 != nil { - logrus.Errorf("Error removing partially-created container root filesystem: %s", err2) + if retErr != nil { + if err := ctr.teardownStorage(); err != nil { + logrus.Errorf("Error removing partially-created container root filesystem: %s", err) } } }() diff --git a/libpod/state_test.go b/libpod/state_test.go index 30ddf5e24..ef4f6f2be 100644 --- a/libpod/state_test.go +++ b/libpod/state_test.go @@ -34,13 +34,13 @@ var ( ) // Get an empty BoltDB state for use in tests -func getEmptyBoltState() (s State, p string, m lock.Manager, err error) { +func getEmptyBoltState() (_ State, _ string, _ lock.Manager, retErr error) { tmpDir, err := ioutil.TempDir("", tmpDirPrefix) if err != nil { return nil, "", nil, err } defer func() { - if err != nil { + if retErr != nil { os.RemoveAll(tmpDir) } }() @@ -66,13 +66,13 @@ func getEmptyBoltState() (s State, p string, m lock.Manager, err error) { } // Get an empty in-memory state for use in tests -func getEmptyInMemoryState() (s State, p string, m lock.Manager, err error) { +func getEmptyInMemoryState() (_ State, _ string, _ lock.Manager, retErr error) { tmpDir, err := ioutil.TempDir("", tmpDirPrefix) if err != nil { return nil, "", nil, err } defer func() { - if err != nil { + if retErr != nil { os.RemoveAll(tmpDir) } }() diff --git a/libpod/storage.go b/libpod/storage.go index be79b3fc0..e497d0daf 100644 --- a/libpod/storage.go +++ b/libpod/storage.go @@ -66,7 +66,7 @@ func (metadata *RuntimeContainerMetadata) SetMountLabel(mountLabel string) { // CreateContainerStorage creates the storage end of things. We already have the container spec created // TO-DO We should be passing in an Image object in the future. -func (r *storageService) CreateContainerStorage(ctx context.Context, systemContext *types.SystemContext, imageName, imageID, containerName, containerID string, options storage.ContainerOptions) (cinfo ContainerInfo, err error) { +func (r *storageService) CreateContainerStorage(ctx context.Context, systemContext *types.SystemContext, imageName, imageID, containerName, containerID string, options storage.ContainerOptions) (_ ContainerInfo, retErr error) { span, _ := opentracing.StartSpanFromContext(ctx, "createContainerStorage") span.SetTag("type", "storageService") defer span.Finish() @@ -132,9 +132,9 @@ func (r *storageService) CreateContainerStorage(ctx context.Context, systemConte // If anything fails after this point, we need to delete the incomplete // container before returning. defer func() { - if err != nil { - if err2 := r.store.DeleteContainer(container.ID); err2 != nil { - logrus.Infof("%v deleting partially-created container %q", err2, container.ID) + if retErr != nil { + if err := r.store.DeleteContainer(container.ID); err != nil { + logrus.Infof("%v deleting partially-created container %q", err, container.ID) return } -- cgit v1.2.3-54-g00ecf From 84076bf95f485bccfd9e03108fb69a1f72b42918 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 9 Jul 2020 13:32:20 +0200 Subject: log API: add context to allow for cancelling Add a `context.Context` to the log APIs to allow for cancelling streaming (e.g., via `podman logs -f`). This fixes issues for the remote API where some go routines of the server will continue writing and produce nothing but heat and waste CPU cycles. Signed-off-by: Valentin Rothberg --- libpod/container_api.go | 2 +- libpod/container_log.go | 23 ++++++--- libpod/container_log_linux.go | 16 ++++++- libpod/container_log_unsupported.go | 4 +- pkg/api/handlers/compat/containers_logs.go | 76 +++++++++++++++--------------- pkg/domain/infra/abi/containers.go | 2 +- pkg/varlinkapi/containers.go | 2 +- 7 files changed, 73 insertions(+), 52 deletions(-) (limited to 'libpod') diff --git a/libpod/container_api.go b/libpod/container_api.go index d19dc651b..487f75e67 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -353,7 +353,7 @@ func (c *Container) HTTPAttach(httpCon net.Conn, httpBuf *bufio.ReadWriter, stre logOpts.WaitGroup.Wait() close(logChan) }() - if err := c.ReadLog(logOpts, logChan); err != nil { + if err := c.ReadLog(context.Background(), logOpts, logChan); err != nil { return err } logrus.Debugf("Done reading logs for container %s, %d bytes", c.ID(), logSize) diff --git a/libpod/container_log.go b/libpod/container_log.go index 97936c683..80f8e6e50 100644 --- a/libpod/container_log.go +++ b/libpod/container_log.go @@ -1,6 +1,7 @@ package libpod import ( + "context" "fmt" "os" "time" @@ -13,9 +14,9 @@ import ( ) // Log is a runtime function that can read one or more container logs. -func (r *Runtime) Log(containers []*Container, options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (r *Runtime) Log(ctx context.Context, containers []*Container, options *logs.LogOptions, logChannel chan *logs.LogLine) error { for _, ctr := range containers { - if err := ctr.ReadLog(options, logChannel); err != nil { + if err := ctr.ReadLog(ctx, options, logChannel); err != nil { return err } } @@ -23,25 +24,25 @@ func (r *Runtime) Log(containers []*Container, options *logs.LogOptions, logChan } // ReadLog reads a containers log based on the input options and returns loglines over a channel. -func (c *Container) ReadLog(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) ReadLog(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { switch c.LogDriver() { case define.NoLogging: return errors.Wrapf(define.ErrNoLogs, "this container is using the 'none' log driver, cannot read logs") case define.JournaldLogging: // TODO Skip sending logs until journald logs can be read - return c.readFromJournal(options, logChannel) + return c.readFromJournal(ctx, options, logChannel) case define.JSONLogging: // TODO provide a separate implementation of this when Conmon // has support. fallthrough case define.KubernetesLogging, "": - return c.readFromLogFile(options, logChannel) + return c.readFromLogFile(ctx, options, logChannel) default: return errors.Wrapf(define.ErrInternal, "unrecognized log driver %q, cannot read logs", c.LogDriver()) } } -func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromLogFile(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { t, tailLog, err := logs.GetLogFile(c.LogPath(), options) if err != nil { // If the log file does not exist, this is not fatal. @@ -62,8 +63,17 @@ func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *l } go func() { + defer options.WaitGroup.Done() + var partial string for line := range t.Lines { + select { + case <-ctx.Done(): + // the consumer has cancelled + return + default: + // fallthrough + } nll, err := logs.NewLogLine(line.Text) if err != nil { logrus.Error(err) @@ -82,7 +92,6 @@ func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *l logChannel <- nll } } - options.WaitGroup.Done() }() // Check if container is still running or paused if options.Follow { diff --git a/libpod/container_log_linux.go b/libpod/container_log_linux.go index fad3bf87c..00b2039a9 100644 --- a/libpod/container_log_linux.go +++ b/libpod/container_log_linux.go @@ -4,6 +4,7 @@ package libpod import ( + "context" "fmt" "io" "math" @@ -29,7 +30,7 @@ const ( bufLen = 16384 ) -func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromJournal(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { var config journal.JournalReaderConfig if options.Tail < 0 { config.NumFromTail = math.MaxUint64 @@ -65,13 +66,24 @@ func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *l if options.Follow { go func() { + done := make(chan bool) + until := make(chan time.Time) + go func() { + select { + case <-ctx.Done(): + until <- time.Time{} + case <-done: + // nothing to do anymore + } + }() follower := FollowBuffer{logChannel} - err := r.Follow(nil, follower) + err := r.Follow(until, follower) if err != nil { logrus.Debugf(err.Error()) } r.Close() options.WaitGroup.Done() + done <- true return }() return nil diff --git a/libpod/container_log_unsupported.go b/libpod/container_log_unsupported.go index 18882720a..f3b36619e 100644 --- a/libpod/container_log_unsupported.go +++ b/libpod/container_log_unsupported.go @@ -3,11 +3,13 @@ package libpod import ( + "context" + "github.com/containers/libpod/v2/libpod/define" "github.com/containers/libpod/v2/libpod/logs" "github.com/pkg/errors" ) -func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromJournal(_ context.Context, _ *logs.LogOptions, _ chan *logs.LogLine) error { return errors.Wrapf(define.ErrOSNotSupported, "Journald logging only enabled with systemd on linux") } diff --git a/pkg/api/handlers/compat/containers_logs.go b/pkg/api/handlers/compat/containers_logs.go index 8147f4d38..30ee030e8 100644 --- a/pkg/api/handlers/compat/containers_logs.go +++ b/pkg/api/handlers/compat/containers_logs.go @@ -92,7 +92,7 @@ func LogsFromContainer(w http.ResponseWriter, r *http.Request) { options.WaitGroup = &wg logChannel := make(chan *logs.LogLine, tail+1) - if err := runtime.Log([]*libpod.Container{ctnr}, options, logChannel); err != nil { + if err := runtime.Log(r.Context(), []*libpod.Container{ctnr}, options, logChannel); err != nil { utils.InternalServerError(w, errors.Wrapf(err, "Failed to obtain logs for Container '%s'", name)) return } @@ -105,50 +105,48 @@ func LogsFromContainer(w http.ResponseWriter, r *http.Request) { var frame strings.Builder header := make([]byte, 8) - for ok := true; ok; ok = query.Follow { - for line := range logChannel { - if _, found := r.URL.Query()["until"]; found { - if line.Time.After(until) { - break - } + for line := range logChannel { + if _, found := r.URL.Query()["until"]; found { + if line.Time.After(until) { + break } + } - // Reset buffer we're ready to loop again - frame.Reset() - switch line.Device { - case "stdout": - if !query.Stdout { - continue - } - header[0] = 1 - case "stderr": - if !query.Stderr { - continue - } - header[0] = 2 - default: - // Logging and moving on is the best we can do here. We may have already sent - // a Status and Content-Type to client therefore we can no longer report an error. - log.Infof("unknown Device type '%s' in log file from Container %s", line.Device, ctnr.ID()) + // Reset buffer we're ready to loop again + frame.Reset() + switch line.Device { + case "stdout": + if !query.Stdout { continue } - - if query.Timestamps { - frame.WriteString(line.Time.Format(time.RFC3339)) - frame.WriteString(" ") + header[0] = 1 + case "stderr": + if !query.Stderr { + continue } - frame.WriteString(line.Msg) + header[0] = 2 + default: + // Logging and moving on is the best we can do here. We may have already sent + // a Status and Content-Type to client therefore we can no longer report an error. + log.Infof("unknown Device type '%s' in log file from Container %s", line.Device, ctnr.ID()) + continue + } - binary.BigEndian.PutUint32(header[4:], uint32(frame.Len())) - if _, err := w.Write(header[0:8]); err != nil { - log.Errorf("unable to write log output header: %q", err) - } - if _, err := io.WriteString(w, frame.String()); err != nil { - log.Errorf("unable to write frame string: %q", err) - } - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() - } + if query.Timestamps { + frame.WriteString(line.Time.Format(time.RFC3339)) + frame.WriteString(" ") + } + frame.WriteString(line.Msg) + + binary.BigEndian.PutUint32(header[4:], uint32(frame.Len())) + if _, err := w.Write(header[0:8]); err != nil { + log.Errorf("unable to write log output header: %q", err) + } + if _, err := io.WriteString(w, frame.String()); err != nil { + log.Errorf("unable to write frame string: %q", err) + } + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() } } } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 596fc2cc1..8909f831d 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -924,7 +924,7 @@ func (ic *ContainerEngine) ContainerLogs(ctx context.Context, containers []strin } logChannel := make(chan *logs.LogLine, chSize) - if err := ic.Libpod.Log(ctrs, logOpts, logChannel); err != nil { + if err := ic.Libpod.Log(ctx, ctrs, logOpts, logChannel); err != nil { return err } diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 8650ba000..07b492331 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -754,7 +754,7 @@ func (i *VarlinkAPI) GetContainersLogs(call iopodman.VarlinkCall, names []string if err != nil { return call.ReplyErrorOccurred(err.Error()) } - if err := i.Runtime.Log(containers, &options, logChannel); err != nil { + if err := i.Runtime.Log(getContext(), containers, &options, logChannel); err != nil { return err } go func() { -- cgit v1.2.3-54-g00ecf