From 89717d0b6370b59fb6214e8a245846e50a2d607c Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 12 Feb 2019 02:07:53 -0800 Subject: [PATCH 1/3] Don't log config at info level. Signed-off-by: Lantao Liu --- pkg/server/container_create.go | 1 + pkg/server/instrumented_service.go | 6 +++--- pkg/server/sandbox_run.go | 4 +++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 1d3960550..06d4c0a09 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -76,6 +76,7 @@ func init() { // CreateContainer creates a new container in the given PodSandbox. func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateContainerRequest) (_ *runtime.CreateContainerResponse, retErr error) { config := r.GetConfig() + logrus.Debugf("Container config %+v", config) sandboxConfig := r.GetSandboxConfig() sandbox, err := c.sandboxStore.Get(r.GetPodSandboxId()) if err != nil { diff --git a/pkg/server/instrumented_service.go b/pkg/server/instrumented_service.go index e7c4a97a6..88e05f266 100644 --- a/pkg/server/instrumented_service.go +++ b/pkg/server/instrumented_service.go @@ -52,7 +52,7 @@ func (in *instrumentedService) RunPodSandbox(ctx context.Context, r *runtime.Run if err := in.checkInitialized(); err != nil { return nil, err } - logrus.Infof("RunPodSandbox with config %+v", r.GetConfig()) + logrus.Infof("RunPodsandbox for %+v", r.GetConfig().GetMetadata()) defer func() { if err != nil { logrus.WithError(err).Errorf("RunPodSandbox for %+v failed, error", r.GetConfig().GetMetadata()) @@ -142,8 +142,8 @@ func (in *instrumentedService) CreateContainer(ctx context.Context, r *runtime.C if err := in.checkInitialized(); err != nil { return nil, err } - logrus.Infof("CreateContainer within sandbox %q with container config %+v and sandbox config %+v", - r.GetPodSandboxId(), r.GetConfig(), r.GetSandboxConfig()) + logrus.Infof("CreateContainer within sandbox %q for container %+v", + r.GetPodSandboxId(), r.GetConfig().GetMetadata()) defer func() { if err != nil { logrus.WithError(err).Errorf("CreateContainer within sandbox %q for %+v failed", diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index f844a0107..0631e65ce 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -28,6 +28,7 @@ import ( "github.com/containerd/containerd/oci" cni "github.com/containerd/go-cni" "github.com/containerd/typeurl" + "github.com/davecgh/go-spew/spew" imagespec "github.com/opencontainers/image-spec/specs-go/v1" runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" @@ -55,6 +56,7 @@ func init() { // the sandbox is in ready state. func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandboxRequest) (_ *runtime.RunPodSandboxResponse, retErr error) { config := r.GetConfig() + logrus.Debugf("Sandbox config %+v", config) // Generate unique id and name for the sandbox and reserve the name. id := util.GenerateID() @@ -149,7 +151,7 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox if err != nil { return nil, errors.Wrap(err, "failed to generate sandbox container spec") } - logrus.Debugf("Sandbox container spec: %+v", spec) + logrus.Debugf("Sandbox container %q spec: %#+v", id, spew.NewFormatter(spec)) var specOpts []oci.SpecOpts userstr, err := generateUserString( From ec6dd37691231dd628c1c7c77cfb5bb1287c50be Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 12 Feb 2019 02:50:53 -0800 Subject: [PATCH 2/3] Add env cache. Signed-off-by: Lantao Liu --- pkg/server/container_create.go | 27 +++++++------- pkg/server/container_create_test.go | 9 +++-- pkg/server/helpers.go | 38 ++++++++++++++++++-- pkg/server/helpers_test.go | 55 +++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 19 deletions(-) diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 06d4c0a09..549d4bf27 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -35,7 +35,6 @@ import ( imagespec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/runc/libcontainer/devices" runtimespec "github.com/opencontainers/runtime-spec/specs-go" - "github.com/opencontainers/runtime-tools/generate" "github.com/opencontainers/runtime-tools/validate" "github.com/opencontainers/selinux/go-selinux/label" "github.com/pkg/errors" @@ -518,7 +517,7 @@ func (c *criService) generateContainerMounts(sandboxID string, config *runtime.C // setOCIProcessArgs sets process args. It returns error if the final arg list // is empty. -func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { +func setOCIProcessArgs(g *generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { command, args := config.GetCommand(), config.GetArgs() // The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go // TODO(random-liu): Clearly define the commands overwrite behavior. @@ -540,7 +539,7 @@ func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, i // addImageEnvs adds environment variables from image config. It returns error if // an invalid environment variable is encountered. -func addImageEnvs(g *generate.Generator, imageEnvs []string) error { +func addImageEnvs(g *generator, imageEnvs []string) error { for _, e := range imageEnvs { kv := strings.SplitN(e, "=", 2) if len(kv) != 2 { @@ -551,7 +550,7 @@ func addImageEnvs(g *generate.Generator, imageEnvs []string) error { return nil } -func setOCIPrivileged(g *generate.Generator, config *runtime.ContainerConfig) error { +func setOCIPrivileged(g *generator, config *runtime.ContainerConfig) error { // Add all capabilities in privileged mode. g.SetupPrivileged(true) setOCIBindMountsPrivileged(g) @@ -572,7 +571,7 @@ func clearReadOnly(m *runtimespec.Mount) { } // addDevices set device mapping without privilege. -func (c *criService) addOCIDevices(g *generate.Generator, devs []*runtime.Device) error { +func (c *criService) addOCIDevices(g *generator, devs []*runtime.Device) error { spec := g.Config for _, device := range devs { path, err := c.os.ResolveSymbolicLink(device.HostPath) @@ -604,7 +603,7 @@ func (c *criService) addOCIDevices(g *generate.Generator, devs []*runtime.Device } // addDevices set device mapping with privilege. -func setOCIDevicesPrivileged(g *generate.Generator) error { +func setOCIDevicesPrivileged(g *generator) error { spec := g.Config hostDevices, err := devices.HostDevices() if err != nil { @@ -635,7 +634,7 @@ func setOCIDevicesPrivileged(g *generate.Generator) error { } // addOCIBindMounts adds bind mounts. -func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.Mount, mountLabel string) error { +func (c *criService) addOCIBindMounts(g *generator, mounts []*runtime.Mount, mountLabel string) error { // Sort mounts in number of parts. This ensures that high level mounts don't // shadow other mounts. sort.Sort(orderedMounts(mounts)) @@ -740,7 +739,7 @@ func (c *criService) addOCIBindMounts(g *generate.Generator, mounts []*runtime.M return nil } -func setOCIBindMountsPrivileged(g *generate.Generator) { +func setOCIBindMountsPrivileged(g *generator) { spec := g.Config // clear readonly for /sys and cgroup for i, m := range spec.Mounts { @@ -756,7 +755,7 @@ func setOCIBindMountsPrivileged(g *generate.Generator) { } // setOCILinuxResourceCgroup set container cgroup resource limit. -func setOCILinuxResourceCgroup(g *generate.Generator, resources *runtime.LinuxContainerResources) { +func setOCILinuxResourceCgroup(g *generator, resources *runtime.LinuxContainerResources) { if resources == nil { return } @@ -769,7 +768,7 @@ func setOCILinuxResourceCgroup(g *generate.Generator, resources *runtime.LinuxCo } // setOCILinuxResourceOOMScoreAdj set container OOMScoreAdj resource limit. -func setOCILinuxResourceOOMScoreAdj(g *generate.Generator, resources *runtime.LinuxContainerResources, restrictOOMScoreAdjFlag bool) error { +func setOCILinuxResourceOOMScoreAdj(g *generator, resources *runtime.LinuxContainerResources, restrictOOMScoreAdjFlag bool) error { if resources == nil { return nil } @@ -799,7 +798,7 @@ func getOCICapabilitiesList() []string { } // Adds capabilities to all sets relevant to root (bounding, permitted, effective, inheritable) -func addProcessRootCapability(g *generate.Generator, c string) error { +func addProcessRootCapability(g *generator, c string) error { if err := g.AddProcessCapabilityBounding(c); err != nil { return err } @@ -816,7 +815,7 @@ func addProcessRootCapability(g *generate.Generator, c string) error { } // Drops capabilities to all sets relevant to root (bounding, permitted, effective, inheritable) -func dropProcessRootCapability(g *generate.Generator, c string) error { +func dropProcessRootCapability(g *generator, c string) error { if err := g.DropProcessCapabilityBounding(c); err != nil { return err } @@ -833,7 +832,7 @@ func dropProcessRootCapability(g *generate.Generator, c string) error { } // setOCICapabilities adds/drops process capabilities. -func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability) error { +func setOCICapabilities(g *generator, capabilities *runtime.Capability) error { if capabilities == nil { return nil } @@ -879,7 +878,7 @@ func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability) } // setOCINamespaces sets namespaces. -func setOCINamespaces(g *generate.Generator, namespaces *runtime.NamespaceOption, sandboxPid uint32) { +func setOCINamespaces(g *generator, namespaces *runtime.NamespaceOption, sandboxPid uint32) { g.AddOrReplaceLinuxNamespace(string(runtimespec.NetworkNamespace), getNetworkNamespace(sandboxPid)) // nolint: errcheck g.AddOrReplaceLinuxNamespace(string(runtimespec.IPCNamespace), getIPCNamespace(sandboxPid)) // nolint: errcheck g.AddOrReplaceLinuxNamespace(string(runtimespec.UTSNamespace), getUTSNamespace(sandboxPid)) // nolint: errcheck diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index c769ddbce..c33d0afb0 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -450,8 +450,9 @@ func TestContainerSpecCommand(t *testing.T) { } { config, _, imageConfig, _ := getCreateContainerTestData() - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) config.Command = test.criEntrypoint config.Args = test.criArgs imageConfig.Entrypoint = test.imageEntrypoint @@ -645,8 +646,9 @@ func TestPrivilegedBindMount(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) c := newTestCRIService() c.addOCIBindMounts(&g, nil, "") if test.privileged { @@ -754,8 +756,9 @@ func TestMountPropagation(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - g, err := generate.New("linux") + og, err := generate.New("linux") assert.NoError(t, err) + g := newCustomGenerator(og) c := newTestCRIService() c.os.(*ostesting.FakeOS).LookupMountFn = test.fakeLookupMountFn err = c.addOCIBindMounts(&g, []*runtime.Mount{test.criMount}, "") diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index 228a349a4..a1cbd4229 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -391,10 +391,44 @@ func buildLabels(configLabels map[string]string, containerType string) map[strin } // newSpecGenerator creates a new spec generator for the runtime spec. -func newSpecGenerator(spec *runtimespec.Spec) generate.Generator { +func newSpecGenerator(spec *runtimespec.Spec) generator { g := generate.NewFromSpec(spec) g.HostSpecific = true - return g + return newCustomGenerator(g) +} + +// generator is a custom generator with some functions overridden +// used by the cri plugin. +// TODO(random-liu): Upstream this fix. +type generator struct { + generate.Generator + envCache map[string]int +} + +func newCustomGenerator(g generate.Generator) generator { + return generator{ + Generator: g, + envCache: make(map[string]int), + } +} + +// AddProcessEnv overrides the original AddProcessEnv. It uses +// a map to cache and override envs. +func (g *generator) AddProcessEnv(key, value string) { + if len(g.envCache) == 0 { + // Call AddProccessEnv once to initialize the spec. + g.Generator.AddProcessEnv(key, value) + g.envCache[key] = 0 + return + } + spec := g.Config + env := fmt.Sprintf("%s=%s", key, value) + if idx, ok := g.envCache[key]; !ok { + spec.Process.Env = append(spec.Process.Env, env) + g.envCache[key] = len(spec.Process.Env) - 1 + } else { + spec.Process.Env[idx] = env + } } func getPodCNILabels(id string, config *runtime.PodSandboxConfig) map[string]string { diff --git a/pkg/server/helpers_test.go b/pkg/server/helpers_test.go index 5d33cd127..95e34a8dc 100644 --- a/pkg/server/helpers_test.go +++ b/pkg/server/helpers_test.go @@ -321,3 +321,58 @@ func TestRestrictOOMScoreAdj(t *testing.T) { require.NoError(t, err) assert.Equal(t, got, current+1) } + +func TestCustomGenerator(t *testing.T) { + for desc, test := range map[string]struct { + kv [][2]string + expected []string + expectNil bool + }{ + "empty": { + expectNil: true, + }, + "single env": { + kv: [][2]string{ + {"a", "b"}, + }, + expected: []string{"a=b"}, + }, + "multiple envs": { + kv: [][2]string{ + {"a", "b"}, + {"c", "d"}, + {"e", "f"}, + }, + expected: []string{ + "a=b", + "c=d", + "e=f", + }, + }, + "env override": { + kv: [][2]string{ + {"k1", "v1"}, + {"k2", "v2"}, + {"k3", "v3"}, + {"k3", "v4"}, + {"k1", "v5"}, + }, + expected: []string{ + "k1=v5", + "k2=v2", + "k3=v4", + }, + }, + } { + t.Logf("TestCase %q", desc) + g := newSpecGenerator(nil) + for _, kv := range test.kv { + g.AddProcessEnv(kv[0], kv[1]) + } + if test.expectNil { + assert.Nil(t, g.Config) + } else { + assert.Equal(t, test.expected, g.Config.Process.Env) + } + } +} From 877c1cadc1992cb9823504a6dfa105558d64d454 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 12 Feb 2019 10:29:45 -0800 Subject: [PATCH 3/3] Include default envs from containerd. Signed-off-by: Lantao Liu --- pkg/server/helpers.go | 9 ++++++++- pkg/server/helpers_test.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index a1cbd4229..c4a0044ce 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -406,10 +406,17 @@ type generator struct { } func newCustomGenerator(g generate.Generator) generator { - return generator{ + cg := generator{ Generator: g, envCache: make(map[string]int), } + if g.Config != nil && g.Config.Process != nil { + for i, env := range g.Config.Process.Env { + kv := strings.SplitN(env, "=", 2) + cg.envCache[kv[0]] = i + } + } + return cg } // AddProcessEnv overrides the original AddProcessEnv. It uses diff --git a/pkg/server/helpers_test.go b/pkg/server/helpers_test.go index 95e34a8dc..c0eb29839 100644 --- a/pkg/server/helpers_test.go +++ b/pkg/server/helpers_test.go @@ -25,6 +25,7 @@ import ( runcoptions "github.com/containerd/containerd/runtime/v2/runc/options" "github.com/docker/distribution/reference" imagedigest "github.com/opencontainers/go-digest" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" @@ -324,6 +325,7 @@ func TestRestrictOOMScoreAdj(t *testing.T) { func TestCustomGenerator(t *testing.T) { for desc, test := range map[string]struct { + existing []string kv [][2]string expected []string expectNil bool @@ -356,16 +358,44 @@ func TestCustomGenerator(t *testing.T) { {"k3", "v3"}, {"k3", "v4"}, {"k1", "v5"}, + {"k4", "v6"}, }, expected: []string{ "k1=v5", "k2=v2", "k3=v4", + "k4=v6", + }, + }, + "existing env": { + existing: []string{ + "k1=v1", + "k2=v2", + "k3=v3", + }, + kv: [][2]string{ + {"k3", "v4"}, + {"k2", "v5"}, + {"k4", "v6"}, + }, + expected: []string{ + "k1=v1", + "k2=v5", + "k3=v4", + "k4=v6", }, }, } { t.Logf("TestCase %q", desc) - g := newSpecGenerator(nil) + var spec *runtimespec.Spec + if len(test.existing) > 0 { + spec = &runtimespec.Spec{ + Process: &runtimespec.Process{ + Env: test.existing, + }, + } + } + g := newSpecGenerator(spec) for _, kv := range test.kv { g.AddProcessEnv(kv[0], kv[1]) }