From f12744ac7bb619d250fe5a36ff54583b57720cb0 Mon Sep 17 00:00:00 2001 From: Ace-Tang Date: Fri, 24 Aug 2018 11:41:38 +0800 Subject: [PATCH] bugfix: missing merge some config from image 1. add user, labels, stop signal and exposed ports config from image config in create container. 2. put merge funtion before initContainerStorage, make mount points generate completely. 3. fix not return error in merge funtion. Signed-off-by: Ace-Tang --- daemon/mgr/container.go | 27 +++-- daemon/mgr/container_types.go | 57 +++++++++- daemon/mgr/container_types_test.go | 174 +++++++++++++++++++++++++++++ 3 files changed, 241 insertions(+), 17 deletions(-) diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go index dd9da2546..7b731678a 100644 --- a/daemon/mgr/container.go +++ b/daemon/mgr/container.go @@ -355,6 +355,21 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty HostConfig: config.HostConfig, } + // merge image's config into container + if err := container.merge(func() (ocispec.ImageConfig, error) { + img, err := mgr.Client.GetImage(ctx, config.Image) + if err != nil { + return ocispec.ImageConfig{}, err + } + ociImage, err := containerdImageToOciImage(ctx, img) + if err != nil { + return ocispec.ImageConfig{}, err + } + return ociImage.Config, nil + }); err != nil { + return nil, err + } + // set container basefs, basefs is not created in pouchd, it will created // after create options passed to containerd. mgr.setBaseFS(ctx, container, id) @@ -382,18 +397,6 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty return nil, err } - // merge image's config into container - if err := container.merge(func() (ocispec.ImageConfig, error) { - img, err := mgr.Client.GetImage(ctx, config.Image) - ociImage, err := containerdImageToOciImage(ctx, img) - if err != nil { - return ocispec.ImageConfig{}, err - } - return ociImage.Config, nil - }); err != nil { - return nil, err - } - // Get snapshot UpperDir mounts, err := mgr.Client.GetMounts(ctx, id) if err != nil { diff --git a/daemon/mgr/container_types.go b/daemon/mgr/container_types.go index a9472b5e0..183a28e26 100644 --- a/daemon/mgr/container_types.go +++ b/daemon/mgr/container_types.go @@ -285,7 +285,7 @@ func (c *Container) StopTimeout() int64 { func (c *Container) merge(getconfig func() (v1.ImageConfig, error)) error { c.Lock() defer c.Unlock() - config, err := getconfig() + imageConf, err := getconfig() if err != nil { return err } @@ -294,19 +294,66 @@ func (c *Container) merge(getconfig func() (v1.ImageConfig, error)) error { // Otherwise use the image's configuration to fill it. if len(c.Config.Entrypoint) == 0 { if len(c.Config.Cmd) == 0 { - c.Config.Cmd = config.Cmd + c.Config.Cmd = imageConf.Cmd } - c.Config.Entrypoint = config.Entrypoint + c.Config.Entrypoint = imageConf.Entrypoint } // ContainerConfig.Env is new, and the ImageConfig.Env is old - newEnvSlice, err := mergeEnvSlice(c.Config.Env, config.Env) + newEnvSlice, err := mergeEnvSlice(c.Config.Env, imageConf.Env) if err != nil { return err } c.Config.Env = newEnvSlice if c.Config.WorkingDir == "" { - c.Config.WorkingDir = config.WorkingDir + c.Config.WorkingDir = imageConf.WorkingDir + } + + // merge user from image image config. + if c.Config.User == "" { + c.Config.User = imageConf.User + } + + // merge stop signal from image config. + if c.Config.StopSignal == "" { + c.Config.StopSignal = imageConf.StopSignal + } + + // merge label from image image config, if same label key exist, + // use container config. + if imageConf.Labels != nil { + if c.Config.Labels == nil { + c.Config.Labels = make(map[string]string) + } + for k, v := range c.Config.Labels { + imageConf.Labels[k] = v + } + c.Config.Labels = imageConf.Labels + } + + // merge exposed ports from image config, if same label key exist, + // use container config. + if len(imageConf.ExposedPorts) > 0 { + if c.Config.ExposedPorts == nil { + c.Config.ExposedPorts = make(map[string]interface{}) + } + for k, v := range imageConf.ExposedPorts { + if _, exist := c.Config.ExposedPorts[k]; !exist { + c.Config.ExposedPorts[k] = interface{}(v) + } + } + } + + // merge volumes from image config. + if len(imageConf.Volumes) > 0 { + if c.Config.Volumes == nil { + c.Config.Volumes = make(map[string]interface{}) + } + for k, v := range imageConf.Volumes { + if _, exist := c.Config.Volumes[k]; !exist { + c.Config.Volumes[k] = interface{}(v) + } + } } return nil diff --git a/daemon/mgr/container_types_test.go b/daemon/mgr/container_types_test.go index 1086cd181..48694acdc 100644 --- a/daemon/mgr/container_types_test.go +++ b/daemon/mgr/container_types_test.go @@ -1,11 +1,15 @@ package mgr import ( + "fmt" + "reflect" + "sort" "testing" "time" "github.com/alibaba/pouch/apis/types" "github.com/alibaba/pouch/pkg/utils" + "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" ) @@ -82,3 +86,173 @@ func TestContainer_FormatStatus(t *testing.T) { assert.Equal(t, err, tc.err, tc.name) } } + +func TestMerge(t *testing.T) { + assert := assert.New(t) + + type TPort struct { + p int + } + + type TVolume struct { + v int + } + + for idx, tc := range []struct { + c *Container + image v1.ImageConfig + expected *types.ContainerConfig + }{ + { + // test merge from image when both config is not empty + c: &Container{ + Config: &types.ContainerConfig{ + Cmd: []string{"a"}, + Entrypoint: []string{"b"}, + Env: []string{"e1=e1", "e2=e2"}, + User: "user1", + StopSignal: "1", + Labels: map[string]string{ + "l1": "l2", + "l3": "l4", + }, + ExposedPorts: map[string]interface{}{ + "e1": interface{}(TPort{ + p: 1, + }), + "e2": interface{}(TPort{ + p: 2, + }), + }, + Volumes: map[string]interface{}{ + "v1": interface{}(TVolume{ + v: 1, + }), + "v2": interface{}(TVolume{ + v: 2, + }), + }, + }, + }, + image: v1.ImageConfig{ + Cmd: []string{"ia"}, + Entrypoint: []string{"ib"}, + Env: []string{"e1=e1", "ie2=ie2"}, + User: "iuser1", + StopSignal: "2", + Labels: map[string]string{ + "il1": "l2", + "il3": "l4", + }, + ExposedPorts: map[string]struct{}{ + "ie1": {}, + "e2": {}, + }, + Volumes: map[string]struct{}{ + "iv1": {}, + "iv2": {}, + }, + }, + expected: &types.ContainerConfig{ + Cmd: []string{"a"}, + Entrypoint: []string{"b"}, + Env: []string{"e1=e1", "e2=e2", "ie2=ie2"}, + User: "user1", + StopSignal: "1", + Labels: map[string]string{ + "l3": "l4", + "il1": "l2", + "l1": "l2", + "il3": "l4", + }, + ExposedPorts: map[string]interface{}{ + "e1": interface{}(TPort{ + p: 1, + }), + "e2": interface{}(TPort{ + p: 2, + }), + "ie1": struct{}{}, + }, + Volumes: map[string]interface{}{ + "v1": interface{}(TVolume{ + v: 1, + }), + "v2": interface{}(TVolume{ + v: 2, + }), + "iv1": struct{}{}, + "iv2": struct{}{}, + }, + }, + }, + { + // test merge image config, when image config is empty + c: &Container{ + Config: &types.ContainerConfig{ + Cmd: []string{"a"}, + Entrypoint: []string{"b"}, + Env: []string{"e1=e1", "e2=e2"}, + User: "user1", + StopSignal: "1", + Labels: map[string]string{ + "l1": "l2", + "l3": "l4", + }, + }, + }, + image: v1.ImageConfig{}, + expected: &types.ContainerConfig{ + Cmd: []string{"a"}, + Entrypoint: []string{"b"}, + Env: []string{"e1=e1", "e2=e2"}, + User: "user1", + StopSignal: "1", + Labels: map[string]string{ + "l1": "l2", + "l3": "l4", + }, + }, + }, + { + // test merge image config, when container config is empty + c: &Container{ + Config: &types.ContainerConfig{}, + }, + image: v1.ImageConfig{ + Cmd: []string{"ia"}, + Entrypoint: []string{"ib"}, + Env: []string{"ie1=ie1", "ie2=ie2"}, + User: "iuser1", + StopSignal: "1", + Labels: map[string]string{ + "il1": "l2", + "il3": "l4", + }, + }, + expected: &types.ContainerConfig{ + Cmd: []string{"ia"}, + Entrypoint: []string{"ib"}, + Env: []string{"ie1=ie1", "ie2=ie2"}, + User: "iuser1", + StopSignal: "1", + Labels: map[string]string{ + "il1": "l2", + "il3": "l4", + }, + }, + }, + } { + err := tc.c.merge(func() (v1.ImageConfig, error) { + return tc.image, nil + }) + assert.NoError(err) + + // sort slice + sort.Strings(tc.c.Config.Env) + sort.Strings(tc.expected.Env) + + ret := reflect.DeepEqual(tc.c.Config, tc.expected) + assert.Equal(true, ret, fmt.Sprintf("test %d fails\n %+v should equal with %+v\n", idx, tc.c.Config, tc.expected)) + } +}