From 7b16a35287b609829bd3a0fdb84dbf9e3d1ca49d Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Fri, 28 Jul 2017 03:51:24 +0000 Subject: [PATCH] Use new metadata store. Signed-off-by: Lantao Liu --- pkg/server/container_create.go | 57 +++++++----- pkg/server/container_create_test.go | 81 ++++++++--------- pkg/server/container_execsync.go | 11 +-- pkg/server/container_list.go | 38 ++++---- pkg/server/container_list_test.go | 114 +++++++++++++++--------- pkg/server/container_remove.go | 78 +++++++++-------- pkg/server/container_remove_test.go | 80 ++++++++--------- pkg/server/container_start.go | 44 +++++----- pkg/server/container_start_test.go | 113 ++++++++++++------------ pkg/server/container_status.go | 32 +++---- pkg/server/container_status_test.go | 42 +++++---- pkg/server/container_stop.go | 34 ++++---- pkg/server/container_stop_test.go | 73 +++++++++------- pkg/server/events.go | 34 ++++---- pkg/server/events_test.go | 70 ++++++++------- pkg/server/helpers.go | 27 +++--- pkg/server/image_list.go | 16 ++-- pkg/server/image_list_test.go | 6 +- pkg/server/image_pull.go | 87 +++++-------------- pkg/server/image_pull_test.go | 51 ----------- pkg/server/image_remove.go | 21 ++--- pkg/server/image_status.go | 16 ++-- pkg/server/image_status_test.go | 6 +- pkg/server/sandbox_list.go | 13 ++- pkg/server/sandbox_list_test.go | 32 ++++--- pkg/server/sandbox_remove.go | 20 ++--- pkg/server/sandbox_remove_test.go | 130 +++++++++++++++------------- pkg/server/sandbox_run.go | 40 ++++----- pkg/server/sandbox_run_test.go | 24 ++--- pkg/server/sandbox_status.go | 6 +- pkg/server/sandbox_status_test.go | 50 +++++------ pkg/server/sandbox_stop.go | 5 +- pkg/server/sandbox_stop_test.go | 107 +++++++++++++---------- pkg/server/service.go | 27 +++--- pkg/server/service_test.go | 15 ++-- 35 files changed, 791 insertions(+), 809 deletions(-) diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index 84709a44f..d4ce0aa6d 100644 --- a/pkg/server/container_create.go +++ b/pkg/server/container_create.go @@ -32,7 +32,7 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // CreateContainer creates a new container in the given PodSandbox. @@ -58,7 +58,7 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C // the same container. id := generateID() name := makeContainerName(config.GetMetadata(), sandboxConfig.GetMetadata()) - if err := c.containerNameIndex.Reserve(name, id); err != nil { + if err = c.containerNameIndex.Reserve(name, id); err != nil { return nil, fmt.Errorf("failed to reserve container name %q: %v", name, err) } defer func() { @@ -68,8 +68,8 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C } }() - // Create initial container metadata. - meta := metadata.ContainerMetadata{ + // Create initial internal container metadata. + meta := containerstore.Metadata{ ID: id, Name: name, SandboxID: sandboxID, @@ -78,18 +78,18 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C // Prepare container image snapshot. For container, the image should have // been pulled before creating the container, so do not ensure the image. - image := config.GetImage().GetImage() - imageMeta, err := c.localResolve(ctx, image) + imageRef := config.GetImage().GetImage() + image, err := c.localResolve(ctx, imageRef) if err != nil { - return nil, fmt.Errorf("failed to resolve image %q: %v", image, err) + return nil, fmt.Errorf("failed to resolve image %q: %v", imageRef, err) } - if imageMeta == nil { - return nil, fmt.Errorf("image %q not found", image) + if image == nil { + return nil, fmt.Errorf("image %q not found", imageRef) } // Generate container runtime spec. mounts := c.generateContainerMounts(getSandboxRootDir(c.rootDir, sandboxID), config) - spec, err := c.generateContainerSpec(id, sandbox.Pid, config, sandboxConfig, imageMeta.Config, mounts) + spec, err := c.generateContainerSpec(id, sandbox.Pid, config, sandboxConfig, image.Config, mounts) if err != nil { return nil, fmt.Errorf("failed to generate container %q spec: %v", id, err) } @@ -101,12 +101,12 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C // Prepare container rootfs. if config.GetLinux().GetSecurityContext().GetReadonlyRootfs() { - if _, err := c.snapshotService.View(ctx, id, imageMeta.ChainID); err != nil { - return nil, fmt.Errorf("failed to view container rootfs %q: %v", imageMeta.ChainID, err) + if _, err := c.snapshotService.View(ctx, id, image.ChainID); err != nil { + return nil, fmt.Errorf("failed to view container rootfs %q: %v", image.ChainID, err) } } else { - if _, err := c.snapshotService.Prepare(ctx, id, imageMeta.ChainID); err != nil { - return nil, fmt.Errorf("failed to prepare container rootfs %q: %v", imageMeta.ChainID, err) + if _, err := c.snapshotService.Prepare(ctx, id, image.ChainID); err != nil { + return nil, fmt.Errorf("failed to prepare container rootfs %q: %v", image.ChainID, err) } } defer func() { @@ -116,18 +116,18 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C } } }() - meta.ImageRef = imageMeta.ID + meta.ImageRef = image.ID // Create container root directory. containerRootDir := getContainerRootDir(c.rootDir, id) - if err := c.os.MkdirAll(containerRootDir, 0755); err != nil { + if err = c.os.MkdirAll(containerRootDir, 0755); err != nil { return nil, fmt.Errorf("failed to create container root directory %q: %v", containerRootDir, err) } defer func() { if retErr != nil { // Cleanup the container root directory. - if err := c.os.RemoveAll(containerRootDir); err != nil { + if err = c.os.RemoveAll(containerRootDir); err != nil { glog.Errorf("Failed to remove container root directory %q: %v", containerRootDir, err) } @@ -139,7 +139,7 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C Container: containers.Container{ ID: id, // TODO(random-liu): Checkpoint metadata into container labels. - Image: imageMeta.ID, + Image: image.ID, Runtime: defaultRuntime, Spec: &prototypes.Any{ TypeUrl: runtimespec.Version, @@ -158,12 +158,23 @@ func (c *criContainerdService) CreateContainer(ctx context.Context, r *runtime.C } }() - // Update container CreatedAt. - meta.CreatedAt = time.Now().UnixNano() + container, err := containerstore.NewContainer(meta, containerstore.Status{CreatedAt: time.Now().UnixNano()}) + if err != nil { + return nil, fmt.Errorf("failed to create internal container object for %q: %v", + id, err) + } + defer func() { + if retErr != nil { + // Cleanup container checkpoint on error. + if err := container.Delete(); err != nil { + glog.Errorf("Failed to cleanup container checkpoint for %q: %v", id, err) + } + } + }() + // Add container into container store. - if err := c.containerStore.Create(meta); err != nil { - return nil, fmt.Errorf("failed to add container metadata %+v into store: %v", - meta, err) + if err := c.containerStore.Add(container); err != nil { + return nil, fmt.Errorf("failed to add container %q into store: %v", id, err) } return &runtime.CreateContainerResponse{ContainerId: id}, nil diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index be25b255b..019a8dbb9 100644 --- a/pkg/server/container_create_test.go +++ b/pkg/server/container_create_test.go @@ -32,9 +32,11 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) func checkMount(t *testing.T, mounts []runtimespec.Mount, src, dest, typ string, @@ -443,64 +445,66 @@ func TestCreateContainer(t *testing.T) { testSandboxID := "test-sandbox-id" testSandboxPid := uint32(4321) config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData() - testSandboxMetadata := &metadata.SandboxMetadata{ - ID: testSandboxID, - Name: "test-sandbox-name", - Config: sandboxConfig, - Pid: testSandboxPid, + testSandbox := &sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testSandboxID, + Name: "test-sandbox-name", + Config: sandboxConfig, + Pid: testSandboxPid, + }, } testContainerName := makeContainerName(config.Metadata, sandboxConfig.Metadata) // Use an image id to avoid image name resolution. // TODO(random-liu): Change this to image name after we have complete image // management unit test framework. - testImage := config.GetImage().GetImage() + testImageRef := config.GetImage().GetImage() testChainID := "test-chain-id" - testImageMetadata := metadata.ImageMetadata{ - ID: testImage, + testImage := imagestore.Image{ + ID: testImageRef, ChainID: testChainID, Config: imageConfig, } for desc, test := range map[string]struct { - sandboxMetadata *metadata.SandboxMetadata + sandbox *sandboxstore.Sandbox reserveNameErr bool - imageMetadataErr bool + imageStoreErr bool prepareSnapshotErr error createRootDirErr error expectErr bool - expectMeta *metadata.ContainerMetadata + expectedMeta containerstore.Metadata }{ "should return error if sandbox does not exist": { - sandboxMetadata: nil, - expectErr: true, + sandbox: nil, + expectErr: true, }, "should return error if name is reserved": { - sandboxMetadata: testSandboxMetadata, - reserveNameErr: true, - expectErr: true, + sandbox: testSandbox, + reserveNameErr: true, + expectErr: true, }, "should return error if fail to create root directory": { - sandboxMetadata: testSandboxMetadata, + sandbox: testSandbox, createRootDirErr: errors.New("random error"), expectErr: true, }, "should return error if image is not pulled": { - sandboxMetadata: testSandboxMetadata, - imageMetadataErr: true, - expectErr: true, + sandbox: testSandbox, + imageStoreErr: true, + expectErr: true, }, "should return error if prepare snapshot fails": { - sandboxMetadata: testSandboxMetadata, + sandbox: testSandbox, prepareSnapshotErr: errors.New("random error"), expectErr: true, }, "should be able to create container successfully": { - sandboxMetadata: testSandboxMetadata, - expectErr: false, - expectMeta: &metadata.ContainerMetadata{ + sandbox: testSandbox, + expectErr: false, + expectedMeta: containerstore.Metadata{ Name: testContainerName, SandboxID: testSandboxID, - ImageRef: testImage, + ImageRef: testImageRef, Config: config, }, }, @@ -510,14 +514,14 @@ func TestCreateContainer(t *testing.T) { fake := c.containerService.(*servertesting.FakeContainersClient) fakeSnapshotClient := WithFakeSnapshotClient(c) fakeOS := c.os.(*ostesting.FakeOS) - if test.sandboxMetadata != nil { - assert.NoError(t, c.sandboxStore.Create(*test.sandboxMetadata)) + if test.sandbox != nil { + assert.NoError(t, c.sandboxStore.Add(*test.sandbox)) } if test.reserveNameErr { assert.NoError(t, c.containerNameIndex.Reserve(testContainerName, "random id")) } - if !test.imageMetadataErr { - assert.NoError(t, c.imageMetadataStore.Create(testImageMetadata)) + if !test.imageStoreErr { + c.imageStore.Add(testImage) } if test.prepareSnapshotErr != nil { fakeSnapshotClient.InjectError("prepare", test.prepareSnapshotErr) @@ -554,9 +558,7 @@ func TestCreateContainer(t *testing.T) { listResp, err := fake.List(context.Background(), &containers.ListContainersRequest{}) assert.NoError(t, err) assert.Empty(t, listResp.Containers, "containerd container should be cleaned up") - metas, err := c.containerStore.List() - assert.NoError(t, err) - assert.Empty(t, metas, "container metadata should not be created") + assert.Empty(t, c.containerStore.List(), "container metadata should not be created") continue } assert.NoError(t, err) @@ -571,7 +573,7 @@ func TestCreateContainer(t *testing.T) { createOpts, ok := containersCalls[0].Argument.(*containers.CreateContainerRequest) assert.True(t, ok, "should create containerd container") assert.Equal(t, id, createOpts.Container.ID, "container id should be correct") - assert.Equal(t, testImage, createOpts.Container.Image, "test image should be correct") + assert.Equal(t, testImageRef, createOpts.Container.Image, "test image should be correct") assert.Equal(t, id, createOpts.Container.RootFS, "rootfs should be correct") spec := &runtimespec.Spec{} assert.NoError(t, json.Unmarshal(createOpts.Container.Spec.Value, spec)) @@ -586,12 +588,11 @@ func TestCreateContainer(t *testing.T) { Parent: testChainID, }, prepareOpts, "prepare request should be correct") - meta, err := c.containerStore.Get(id) + container, err := c.containerStore.Get(id) assert.NoError(t, err) - require.NotNil(t, meta) - test.expectMeta.ID = id - // TODO(random-liu): Use fake clock to test CreatedAt. - test.expectMeta.CreatedAt = meta.CreatedAt - assert.Equal(t, test.expectMeta, meta, "container metadata should be created") + test.expectedMeta.ID = id + assert.Equal(t, test.expectedMeta, container.Metadata, "container metadata should be created") + assert.Equal(t, runtime.ContainerState_CONTAINER_CREATED, container.Status.Get().State(), + "container should be in created state") } } diff --git a/pkg/server/container_execsync.go b/pkg/server/container_execsync.go index dcdf9fc8a..c24602f76 100644 --- a/pkg/server/container_execsync.go +++ b/pkg/server/container_execsync.go @@ -45,15 +45,16 @@ func (c *criContainerdService) ExecSync(ctx context.Context, r *runtime.ExecSync } }() - // Get container metadata from our container store. - meta, err := c.containerStore.Get(r.GetContainerId()) + // Get container from our container store. + cntr, err := c.containerStore.Get(r.GetContainerId()) if err != nil { return nil, fmt.Errorf("an error occurred when try to find container %q: %v", r.GetContainerId(), err) } - id := meta.ID + id := cntr.ID - if meta.State() != runtime.ContainerState_CONTAINER_RUNNING { - return nil, fmt.Errorf("container %q is in %s state", id, criContainerStateToString(meta.State())) + state := cntr.Status.Get().State() + if state != runtime.ContainerState_CONTAINER_RUNNING { + return nil, fmt.Errorf("container %q is in %s state", id, criContainerStateToString(state)) } // Get exec process spec. diff --git a/pkg/server/container_list.go b/pkg/server/container_list.go index 573a9ae89..d0f587657 100644 --- a/pkg/server/container_list.go +++ b/pkg/server/container_list.go @@ -17,14 +17,12 @@ limitations under the License. package server import ( - "fmt" - "github.com/golang/glog" "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // ListContainers lists all containers matching the filter. @@ -36,33 +34,31 @@ func (c *criContainerdService) ListContainers(ctx context.Context, r *runtime.Li } }() - // List all container metadata from store. - metas, err := c.containerStore.List() - if err != nil { - return nil, fmt.Errorf("failed to list metadata from container store: %v", err) - } + // List all containers from store. + containersInStore := c.containerStore.List() var containers []*runtime.Container - for _, meta := range metas { - containers = append(containers, toCRIContainer(meta)) + for _, container := range containersInStore { + containers = append(containers, toCRIContainer(container)) } containers = c.filterCRIContainers(containers, r.GetFilter()) return &runtime.ListContainersResponse{Containers: containers}, nil } -// toCRIContainer converts container metadata into CRI container. -func toCRIContainer(meta *metadata.ContainerMetadata) *runtime.Container { +// toCRIContainer converts internal container object into CRI container. +func toCRIContainer(container containerstore.Container) *runtime.Container { + status := container.Status.Get() return &runtime.Container{ - Id: meta.ID, - PodSandboxId: meta.SandboxID, - Metadata: meta.Config.GetMetadata(), - Image: meta.Config.GetImage(), - ImageRef: meta.ImageRef, - State: meta.State(), - CreatedAt: meta.CreatedAt, - Labels: meta.Config.GetLabels(), - Annotations: meta.Config.GetAnnotations(), + Id: container.ID, + PodSandboxId: container.SandboxID, + Metadata: container.Config.GetMetadata(), + Image: container.Config.GetImage(), + ImageRef: container.ImageRef, + State: status.State(), + CreatedAt: status.CreatedAt, + Labels: container.Config.GetLabels(), + Annotations: container.Config.GetAnnotations(), } } diff --git a/pkg/server/container_list_test.go b/pkg/server/container_list_test.go index 4c98c2107..3570afca1 100644 --- a/pkg/server/container_list_test.go +++ b/pkg/server/container_list_test.go @@ -23,10 +23,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" - "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) func TestToCRIContainer(t *testing.T) { @@ -40,20 +39,25 @@ func TestToCRIContainer(t *testing.T) { Annotations: map[string]string{"c": "d"}, } createdAt := time.Now().UnixNano() - meta := &metadata.ContainerMetadata{ - ID: "test-id", - Name: "test-name", - SandboxID: "test-sandbox-id", - Config: config, - ImageRef: "test-image-ref", - Pid: 1234, - CreatedAt: createdAt, - StartedAt: time.Now().UnixNano(), - FinishedAt: time.Now().UnixNano(), - ExitCode: 1, - Reason: "test-reason", - Message: "test-message", - } + container, err := containerstore.NewContainer( + containerstore.Metadata{ + ID: "test-id", + Name: "test-name", + SandboxID: "test-sandbox-id", + Config: config, + ImageRef: "test-image-ref", + }, + containerstore.Status{ + Pid: 1234, + CreatedAt: createdAt, + StartedAt: time.Now().UnixNano(), + FinishedAt: time.Now().UnixNano(), + ExitCode: 1, + Reason: "test-reason", + Message: "test-message", + }, + ) + assert.NoError(t, err) expect := &runtime.Container{ Id: "test-id", PodSandboxId: "test-sandbox-id", @@ -65,7 +69,7 @@ func TestToCRIContainer(t *testing.T) { Labels: config.GetLabels(), Annotations: config.GetAnnotations(), } - c := toCRIContainer(meta) + c := toCRIContainer(container) assert.Equal(t, expect, c) } @@ -147,43 +151,67 @@ func TestFilterContainers(t *testing.T) { } } +// containerForTest is a helper type for test. +type containerForTest struct { + metadata containerstore.Metadata + status containerstore.Status +} + +func (c containerForTest) toContainer() (containerstore.Container, error) { + return containerstore.NewContainer(c.metadata, c.status) +} + func TestListContainers(t *testing.T) { c := newTestCRIContainerdService() createdAt := time.Now().UnixNano() startedAt := time.Now().UnixNano() finishedAt := time.Now().UnixNano() - containersInStore := []metadata.ContainerMetadata{ + containersInStore := []containerForTest{ { - ID: "1", - Name: "name-1", - SandboxID: "s-1", - Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-1"}}, - CreatedAt: createdAt, + metadata: containerstore.Metadata{ + ID: "1", + Name: "name-1", + SandboxID: "s-1", + Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-1"}}, + }, + status: containerstore.Status{CreatedAt: createdAt}, }, { - ID: "2", - Name: "name-2", - SandboxID: "s-1", - Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-2"}}, - CreatedAt: createdAt, - StartedAt: startedAt, + metadata: containerstore.Metadata{ + ID: "2", + Name: "name-2", + SandboxID: "s-1", + Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-2"}}, + }, + status: containerstore.Status{ + CreatedAt: createdAt, + StartedAt: startedAt, + }, }, { - ID: "3", - Name: "name-3", - SandboxID: "s-1", - Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-3"}}, - CreatedAt: createdAt, - StartedAt: startedAt, - FinishedAt: finishedAt, + metadata: containerstore.Metadata{ + ID: "3", + Name: "name-3", + SandboxID: "s-1", + Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-3"}}, + }, + status: containerstore.Status{ + CreatedAt: createdAt, + StartedAt: startedAt, + FinishedAt: finishedAt, + }, }, { - ID: "4", - Name: "name-4", - SandboxID: "s-2", - Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-4"}}, - CreatedAt: createdAt, + metadata: containerstore.Metadata{ + ID: "4", + Name: "name-4", + SandboxID: "s-2", + Config: &runtime.ContainerConfig{Metadata: &runtime.ContainerMetadata{Name: "name-4"}}, + }, + status: containerstore.Status{ + CreatedAt: createdAt, + }, }, } filter := &runtime.ContainerFilter{ @@ -215,7 +243,9 @@ func TestListContainers(t *testing.T) { // Inject test metadata for _, cntr := range containersInStore { - c.containerStore.Create(cntr) + container, err := cntr.toContainer() + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(container)) } resp, err := c.ListContainers(context.Background(), &runtime.ListContainersRequest{Filter: filter}) diff --git a/pkg/server/container_remove.go b/pkg/server/container_remove.go index 99c9fc213..5897d7f9b 100644 --- a/pkg/server/container_remove.go +++ b/pkg/server/container_remove.go @@ -25,7 +25,8 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // RemoveContainer removes the container. @@ -37,31 +38,29 @@ func (c *criContainerdService) RemoveContainer(ctx context.Context, r *runtime.R } }() - id := r.GetContainerId() + container, err := c.containerStore.Get(r.GetContainerId()) + if err != nil { + if err != store.ErrNotExist { + return nil, fmt.Errorf("an error occurred when try to find container %q: %v", r.GetContainerId(), err) + } + // Do not return error if container metadata doesn't exist. + glog.V(5).Infof("RemoveContainer called for container %q that does not exist", r.GetContainerId()) + return &runtime.RemoveContainerResponse{}, nil + } + id := container.ID // Set removing state to prevent other start/remove operations against this container // while it's being removed. - if err := c.setContainerRemoving(id); err != nil { - if !metadata.IsNotExistError(err) { - return nil, fmt.Errorf("failed to set removing state for container %q: %v", - id, err) - } - // Do not return error if container metadata doesn't exist. - glog.V(5).Infof("RemoveContainer called for container %q that does not exist", id) - return &runtime.RemoveContainerResponse{}, nil + if err := setContainerRemoving(container); err != nil { + return nil, fmt.Errorf("failed to set removing state for container %q: %v", id, err) } defer func() { - if retErr == nil { - // Cleanup all index after successfully remove the container. - c.containerNameIndex.ReleaseByKey(id) - return - } - // Reset removing if remove failed. - if err := c.resetContainerRemoving(id); err != nil { - // TODO(random-liu): Deal with update failure. Actually Removing doesn't need to - // be checkpointed, we only need it to have the same lifecycle with container metadata. - glog.Errorf("failed to reset removing state for container %q: %v", - id, err) + if retErr != nil { + // Reset removing if remove failed. + if err := resetContainerRemoving(container); err != nil { + // TODO(random-liu): Do not checkpoint `Removing` state. + glog.Errorf("failed to reset removing state for container %q: %v", id, err) + } } }() @@ -78,13 +77,17 @@ func (c *criContainerdService) RemoveContainer(ctx context.Context, r *runtime.R glog.V(5).Infof("Remove called for snapshot %q that does not exist", id) } - // Cleanup container root directory. containerRootDir := getContainerRootDir(c.rootDir, id) if err := c.os.RemoveAll(containerRootDir); err != nil { return nil, fmt.Errorf("failed to remove container root directory %q: %v", containerRootDir, err) } + // Delete container checkpoint. + if err := container.Delete(); err != nil { + return nil, fmt.Errorf("failed to delete container checkpoint for %q: %v", id, err) + } + // Delete containerd container. if _, err := c.containerService.Delete(ctx, &containers.DeleteContainerRequest{ID: id}); err != nil { if !isContainerdGRPCNotFoundError(err) { @@ -93,35 +96,34 @@ func (c *criContainerdService) RemoveContainer(ctx context.Context, r *runtime.R glog.V(5).Infof("Remove called for containerd container %q that does not exist", id, err) } - // Delete container metadata. - if err := c.containerStore.Delete(id); err != nil { - return nil, fmt.Errorf("failed to delete container metadata for %q: %v", id, err) - } + c.containerStore.Delete(id) + + c.containerNameIndex.ReleaseByKey(id) return &runtime.RemoveContainerResponse{}, nil } // setContainerRemoving sets the container into removing state. In removing state, the // container will not be started or removed again. -func (c *criContainerdService) setContainerRemoving(id string) error { - return c.containerStore.Update(id, func(meta metadata.ContainerMetadata) (metadata.ContainerMetadata, error) { +func setContainerRemoving(container containerstore.Container) error { + return container.Status.Update(func(status containerstore.Status) (containerstore.Status, error) { // Do not remove container if it's still running. - if meta.State() == runtime.ContainerState_CONTAINER_RUNNING { - return meta, fmt.Errorf("container %q is still running", id) + if status.State() == runtime.ContainerState_CONTAINER_RUNNING { + return status, fmt.Errorf("container is still running") } - if meta.Removing { - return meta, fmt.Errorf("container is already in removing state") + if status.Removing { + return status, fmt.Errorf("container is already in removing state") } - meta.Removing = true - return meta, nil + status.Removing = true + return status, nil }) } // resetContainerRemoving resets the container removing state on remove failure. So // that we could remove the container again. -func (c *criContainerdService) resetContainerRemoving(id string) error { - return c.containerStore.Update(id, func(meta metadata.ContainerMetadata) (metadata.ContainerMetadata, error) { - meta.Removing = false - return meta, nil +func resetContainerRemoving(container containerstore.Container) error { + return container.Status.Update(func(status containerstore.Status) (containerstore.Status, error) { + status.Removing = false + return status, nil }) } diff --git a/pkg/server/container_remove_test.go b/pkg/server/container_remove_test.go index d3c93ebeb..2f6caec33 100644 --- a/pkg/server/container_remove_test.go +++ b/pkg/server/container_remove_test.go @@ -25,13 +25,13 @@ import ( snapshotapi "github.com/containerd/containerd/api/services/snapshot" "github.com/containerd/containerd/api/types/mount" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // TestSetContainerRemoving tests setContainerRemoving sets removing @@ -39,20 +39,18 @@ import ( func TestSetContainerRemoving(t *testing.T) { testID := "test-id" for desc, test := range map[string]struct { - metadata *metadata.ContainerMetadata + status containerstore.Status expectErr bool }{ "should return error when container is in running state": { - metadata: &metadata.ContainerMetadata{ - ID: testID, + status: containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), }, expectErr: true, }, "should return error when container is in removing state": { - metadata: &metadata.ContainerMetadata{ - ID: testID, + status: containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), FinishedAt: time.Now().UnixNano(), @@ -61,8 +59,7 @@ func TestSetContainerRemoving(t *testing.T) { expectErr: true, }, "should not return error when container is not running and removing": { - metadata: &metadata.ContainerMetadata{ - ID: testID, + status: containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), FinishedAt: time.Now().UnixNano(), @@ -71,19 +68,18 @@ func TestSetContainerRemoving(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - c := newTestCRIContainerdService() - if test.metadata != nil { - assert.NoError(t, c.containerStore.Create(*test.metadata)) - } - err := c.setContainerRemoving(testID) - meta, getErr := c.containerStore.Get(testID) - assert.NoError(t, getErr) + container, err := containerstore.NewContainer( + containerstore.Metadata{ID: testID}, + test.status, + ) + assert.NoError(t, err) + err = setContainerRemoving(container) if test.expectErr { assert.Error(t, err) - assert.Equal(t, test.metadata, meta, "metadata should not be updated") + assert.Equal(t, test.status, container.Status.Get(), "metadata should not be updated") } else { assert.NoError(t, err) - assert.True(t, meta.Removing, "removing should be set") + assert.True(t, container.Status.Get().Removing, "removing should be set") } } } @@ -91,15 +87,14 @@ func TestSetContainerRemoving(t *testing.T) { func TestRemoveContainer(t *testing.T) { testID := "test-id" testName := "test-name" - testContainerMetadata := &metadata.ContainerMetadata{ - ID: testID, + testContainerStatus := &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), FinishedAt: time.Now().UnixNano(), } for desc, test := range map[string]struct { - metadata *metadata.ContainerMetadata + status *containerstore.Status removeSnapshotErr error deleteContainerErr error removeDirErr error @@ -107,16 +102,14 @@ func TestRemoveContainer(t *testing.T) { expectUnsetRemoving bool }{ "should return error when container is still running": { - metadata: &metadata.ContainerMetadata{ - ID: testID, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), }, expectErr: true, }, "should return error when there is ongoing removing": { - metadata: &metadata.ContainerMetadata{ - ID: testID, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), FinishedAt: time.Now().UnixNano(), @@ -124,40 +117,40 @@ func TestRemoveContainer(t *testing.T) { }, expectErr: true, }, - "should not return error if container metadata does not exist": { - metadata: nil, + "should not return error if container does not exist": { + status: nil, removeSnapshotErr: servertesting.SnapshotNotExistError, deleteContainerErr: servertesting.ContainerNotExistError, expectErr: false, }, "should not return error if snapshot does not exist": { - metadata: testContainerMetadata, + status: testContainerStatus, removeSnapshotErr: servertesting.SnapshotNotExistError, expectErr: false, }, "should return error if remove snapshot fails": { - metadata: testContainerMetadata, + status: testContainerStatus, removeSnapshotErr: errors.New("random error"), expectErr: true, }, "should not return error if containerd container does not exist": { - metadata: testContainerMetadata, + status: testContainerStatus, deleteContainerErr: servertesting.ContainerNotExistError, expectErr: false, }, "should return error if delete containerd container fails": { - metadata: testContainerMetadata, + status: testContainerStatus, deleteContainerErr: errors.New("random error"), expectErr: true, }, "should return error if remove container root fails": { - metadata: testContainerMetadata, + status: testContainerStatus, removeDirErr: errors.New("random error"), expectErr: true, expectUnsetRemoving: true, }, "should be able to remove container successfully": { - metadata: testContainerMetadata, + status: testContainerStatus, expectErr: false, }, } { @@ -166,9 +159,14 @@ func TestRemoveContainer(t *testing.T) { fake := c.containerService.(*servertesting.FakeContainersClient) fakeSnapshotClient := WithFakeSnapshotClient(c) fakeOS := c.os.(*ostesting.FakeOS) - if test.metadata != nil { + if test.status != nil { assert.NoError(t, c.containerNameIndex.Reserve(testName, testID)) - assert.NoError(t, c.containerStore.Create(*test.metadata)) + container, err := containerstore.NewContainer( + containerstore.Metadata{ID: testID}, + *test.status, + ) + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(container)) } fakeOS.RemoveAllFn = func(path string) error { assert.Equal(t, getContainerRootDir(c.rootDir, testID), path) @@ -202,19 +200,16 @@ func TestRemoveContainer(t *testing.T) { if !test.expectUnsetRemoving { continue } - meta, err := c.containerStore.Get(testID) + container, err := c.containerStore.Get(testID) assert.NoError(t, err) - require.NotNil(t, meta) // Also covers resetContainerRemoving. - assert.False(t, meta.Removing, "removing state should be unset") + assert.False(t, container.Status.Get().Removing, "removing state should be unset") continue } assert.NoError(t, err) assert.NotNil(t, resp) - meta, err := c.containerStore.Get(testID) - assert.Error(t, err) - assert.True(t, metadata.IsNotExistError(err)) - assert.Nil(t, meta, "container metadata should be removed") + _, err = c.containerStore.Get(testID) + assert.Equal(t, store.ErrNotExist, err) assert.NoError(t, c.containerNameIndex.Reserve(testName, testID), "container name should be released") mountsResp, err := fakeSnapshotClient.Mounts(context.Background(), &snapshotapi.MountsRequest{Key: testID}) @@ -229,6 +224,5 @@ func TestRemoveContainer(t *testing.T) { }) assert.NoError(t, err) assert.NotNil(t, resp, "remove should be idempotent") - } } diff --git a/pkg/server/container_start.go b/pkg/server/container_start.go index 369d0f2b3..32a2bcbe5 100644 --- a/pkg/server/container_start.go +++ b/pkg/server/container_start.go @@ -30,8 +30,8 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" "github.com/kubernetes-incubator/cri-containerd/pkg/server/agents" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // StartContainer starts the container. @@ -50,12 +50,12 @@ func (c *criContainerdService) StartContainer(ctx context.Context, r *runtime.St id := container.ID var startErr error - // start container in one transaction to avoid race with event monitor. - if err := c.containerStore.Update(id, func(meta metadata.ContainerMetadata) (metadata.ContainerMetadata, error) { - // Always apply metadata change no matter startContainer fails or not. Because startContainer + // update container status in one transaction to avoid race with event monitor. + if err := container.Status.Update(func(status containerstore.Status) (containerstore.Status, error) { + // Always apply status change no matter startContainer fails or not. Because startContainer // may change container state no matter it fails or succeeds. - startErr = c.startContainer(ctx, id, &meta) - return meta, nil + startErr = c.startContainer(ctx, id, container.Metadata, &status) + return status, nil }); startErr != nil { return nil, startErr } else if err != nil { @@ -65,36 +65,36 @@ func (c *criContainerdService) StartContainer(ctx context.Context, r *runtime.St } // startContainer actually starts the container. The function needs to be run in one transaction. Any updates -// to the metadata passed in will be applied to container store no matter the function returns error or not. -func (c *criContainerdService) startContainer(ctx context.Context, id string, meta *metadata.ContainerMetadata) (retErr error) { +// to the status passed in will be applied no matter the function returns error or not. +func (c *criContainerdService) startContainer(ctx context.Context, id string, meta containerstore.Metadata, status *containerstore.Status) (retErr error) { config := meta.Config // Return error if container is not in created state. - if meta.State() != runtime.ContainerState_CONTAINER_CREATED { - return fmt.Errorf("container %q is in %s state", id, criContainerStateToString(meta.State())) + if status.State() != runtime.ContainerState_CONTAINER_CREATED { + return fmt.Errorf("container %q is in %s state", id, criContainerStateToString(status.State())) } // Do not start the container when there is a removal in progress. - if meta.Removing { + if status.Removing { return fmt.Errorf("container %q is in removing state", id) } defer func() { if retErr != nil { // Set container to exited if fail to start. - meta.Pid = 0 - meta.FinishedAt = time.Now().UnixNano() - meta.ExitCode = errorStartExitCode - meta.Reason = errorStartReason - meta.Message = retErr.Error() + status.Pid = 0 + status.FinishedAt = time.Now().UnixNano() + status.ExitCode = errorStartExitCode + status.Reason = errorStartReason + status.Message = retErr.Error() } }() // Get sandbox config from sandbox store. - sandboxMeta, err := c.sandboxStore.Get(meta.SandboxID) + sandbox, err := c.sandboxStore.Get(meta.SandboxID) if err != nil { return fmt.Errorf("sandbox %q not found: %v", meta.SandboxID, err) } - sandboxConfig := sandboxMeta.Config + sandboxConfig := sandbox.Config sandboxID := meta.SandboxID // Make sure sandbox is running. sandboxInfo, err := c.taskService.Info(ctx, &execution.InfoRequest{ContainerID: sandboxID}) @@ -137,12 +137,12 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me if config.GetLogPath() != "" { // Only generate container log when log path is specified. logPath := filepath.Join(sandboxConfig.GetLogDirectory(), config.GetLogPath()) - if err := c.agentFactory.NewContainerLogger(logPath, agents.Stdout, stdoutPipe).Start(); err != nil { + if err = c.agentFactory.NewContainerLogger(logPath, agents.Stdout, stdoutPipe).Start(); err != nil { return fmt.Errorf("failed to start container stdout logger: %v", err) } // Only redirect stderr when there is no tty. if !config.GetTty() { - if err := c.agentFactory.NewContainerLogger(logPath, agents.Stderr, stderrPipe).Start(); err != nil { + if err = c.agentFactory.NewContainerLogger(logPath, agents.Stderr, stderrPipe).Start(); err != nil { return fmt.Errorf("failed to start container stderr logger: %v", err) } } @@ -192,7 +192,7 @@ func (c *criContainerdService) startContainer(ctx context.Context, id string, me } // Update container start timestamp. - meta.Pid = createResp.Pid - meta.StartedAt = time.Now().UnixNano() + status.Pid = createResp.Pid + status.StartedAt = time.Now().UnixNano() return nil } diff --git a/pkg/server/container_start_test.go b/pkg/server/container_start_test.go index c2d31083e..920f31b90 100644 --- a/pkg/server/container_start_test.go +++ b/pkg/server/container_start_test.go @@ -27,27 +27,29 @@ import ( "github.com/containerd/containerd/api/types/mount" "github.com/containerd/containerd/api/types/task" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) func TestStartContainer(t *testing.T) { testID := "test-id" testSandboxID := "test-sandbox-id" - testMetadata := &metadata.ContainerMetadata{ + testMetadata := containerstore.Metadata{ ID: testID, Name: "test-name", SandboxID: testSandboxID, - CreatedAt: time.Now().UnixNano(), } - testSandboxMetadata := &metadata.SandboxMetadata{ - ID: testSandboxID, - Name: "test-sandbox-name", + testStatus := &containerstore.Status{CreatedAt: time.Now().UnixNano()} + testSandbox := &sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testSandboxID, + Name: "test-sandbox-name", + }, } testSandboxContainer := &task.Task{ ID: testSandboxID, @@ -56,8 +58,8 @@ func TestStartContainer(t *testing.T) { } testMounts := []*mount.Mount{{Type: "bind", Source: "test-source"}} for desc, test := range map[string]struct { - containerMetadata *metadata.ContainerMetadata - sandboxMetadata *metadata.SandboxMetadata + status *containerstore.Status + sandbox *sandboxstore.Sandbox sandboxContainerdContainer *task.Task snapshotMountsErr bool prepareFIFOErr error @@ -67,50 +69,44 @@ func TestStartContainer(t *testing.T) { expectCalls []string expectErr bool }{ - "should return error when container metadata does not exist": { - containerMetadata: nil, - sandboxMetadata: testSandboxMetadata, + "should return error when container does not exist": { + status: nil, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, expectCalls: []string{}, expectErr: true, }, "should return error when container is not in created state": { - containerMetadata: &metadata.ContainerMetadata{ - ID: testID, - Name: "test-name", - SandboxID: testSandboxID, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), }, - sandboxMetadata: testSandboxMetadata, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, expectCalls: []string{}, expectErr: true, }, "should return error when container is in removing state": { - containerMetadata: &metadata.ContainerMetadata{ - ID: testID, - Name: "test-name", - SandboxID: testSandboxID, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), Removing: true, }, - sandboxMetadata: testSandboxMetadata, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, expectCalls: []string{}, expectErr: true, }, "should return error when sandbox does not exist": { - containerMetadata: testMetadata, - sandboxMetadata: nil, + status: testStatus, + sandbox: nil, sandboxContainerdContainer: testSandboxContainer, expectStateChange: true, expectCalls: []string{}, expectErr: true, }, "should return error when sandbox is not running": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: &task.Task{ ID: testSandboxID, Pid: uint32(4321), @@ -121,8 +117,8 @@ func TestStartContainer(t *testing.T) { expectErr: true, }, "should return error when snapshot mounts fails": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, snapshotMountsErr: true, expectStateChange: true, @@ -130,8 +126,8 @@ func TestStartContainer(t *testing.T) { expectErr: true, }, "should return error when fail to open streaming pipes": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, prepareFIFOErr: errors.New("open error"), expectStateChange: true, @@ -139,8 +135,8 @@ func TestStartContainer(t *testing.T) { expectErr: true, }, "should return error when fail to create container": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, createContainerErr: errors.New("create error"), expectStateChange: true, @@ -148,8 +144,8 @@ func TestStartContainer(t *testing.T) { expectErr: true, }, "should return error when fail to start container": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, startContainerErr: errors.New("start error"), expectStateChange: true, @@ -158,8 +154,8 @@ func TestStartContainer(t *testing.T) { expectErr: true, }, "should be able to start container successfully": { - containerMetadata: testMetadata, - sandboxMetadata: testSandboxMetadata, + status: testStatus, + sandbox: testSandbox, sandboxContainerdContainer: testSandboxContainer, expectStateChange: true, expectCalls: []string{"info", "create", "start"}, @@ -171,11 +167,16 @@ func TestStartContainer(t *testing.T) { fake := c.taskService.(*servertesting.FakeExecutionClient) fakeOS := c.os.(*ostesting.FakeOS) fakeSnapshotClient := WithFakeSnapshotClient(c) - if test.containerMetadata != nil { - assert.NoError(t, c.containerStore.Create(*test.containerMetadata)) + if test.status != nil { + cntr, err := containerstore.NewContainer( + testMetadata, + *test.status, + ) + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(cntr)) } - if test.sandboxMetadata != nil { - assert.NoError(t, c.sandboxStore.Create(*test.sandboxMetadata)) + if test.sandbox != nil { + assert.NoError(t, c.sandboxStore.Add(*test.sandbox)) } if test.sandboxContainerdContainer != nil { fake.SetFakeTasks([]task.Task{*test.sandboxContainerdContainer}) @@ -206,35 +207,39 @@ func TestStartContainer(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, resp) } - // Check container state. - meta, err := c.containerStore.Get(testID) - if !test.expectStateChange { - // Do not check the error, because container may not exist - // in the test case. - assert.Equal(t, meta, test.containerMetadata) + // Skip following validation if no container is injected initially. + if test.status == nil { continue } + // Check container state. + cntr, err := c.containerStore.Get(testID) assert.NoError(t, err) - require.NotNil(t, meta) + status := cntr.Status.Get() + if !test.expectStateChange { + assert.Equal(t, testMetadata, cntr.Metadata) + assert.Equal(t, *test.status, status) + continue + } if test.expectErr { t.Logf("container state should be in exited state when fail to start") - assert.Equal(t, runtime.ContainerState_CONTAINER_EXITED, meta.State()) - assert.Zero(t, meta.Pid) - assert.EqualValues(t, errorStartExitCode, meta.ExitCode) - assert.Equal(t, errorStartReason, meta.Reason) - assert.NotEmpty(t, meta.Message) + assert.Equal(t, runtime.ContainerState_CONTAINER_EXITED, status.State()) + assert.Zero(t, status.Pid) + assert.EqualValues(t, errorStartExitCode, status.ExitCode) + assert.Equal(t, errorStartReason, status.Reason) + assert.NotEmpty(t, status.Message) _, err := fake.Info(context.Background(), &execution.InfoRequest{ContainerID: testID}) assert.True(t, isContainerdGRPCNotFoundError(err), "containerd task should be cleaned up when fail to start") continue } t.Logf("container state should be running when start successfully") - assert.Equal(t, runtime.ContainerState_CONTAINER_RUNNING, meta.State()) + assert.Equal(t, runtime.ContainerState_CONTAINER_RUNNING, status.State()) info, err := fake.Info(context.Background(), &execution.InfoRequest{ContainerID: testID}) assert.NoError(t, err) pid := info.Task.Pid - assert.Equal(t, pid, meta.Pid) + assert.Equal(t, pid, status.Pid) assert.Equal(t, task.StatusRunning, info.Task.Status) + // Check runtime spec calls := fake.GetCalledDetails() createOpts, ok := calls[1].Argument.(*execution.CreateRequest) assert.True(t, ok, "2nd call should be create") diff --git a/pkg/server/container_status.go b/pkg/server/container_status.go index 515ff9b03..aa7dca8eb 100644 --- a/pkg/server/container_status.go +++ b/pkg/server/container_status.go @@ -21,10 +21,9 @@ import ( "github.com/golang/glog" "golang.org/x/net/context" - "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) // ContainerStatus inspects the container and returns the status. @@ -36,22 +35,23 @@ func (c *criContainerdService) ContainerStatus(ctx context.Context, r *runtime.C } }() - meta, err := c.containerStore.Get(r.GetContainerId()) + container, err := c.containerStore.Get(r.GetContainerId()) if err != nil { return nil, fmt.Errorf("an error occurred when try to find container %q: %v", r.GetContainerId(), err) } return &runtime.ContainerStatusResponse{ - Status: toCRIContainerStatus(meta), + Status: toCRIContainerStatus(container), }, nil } -// toCRIContainerStatus converts container metadata to CRI container status. -func toCRIContainerStatus(meta *metadata.ContainerMetadata) *runtime.ContainerStatus { - state := meta.State() - reason := meta.Reason - if state == runtime.ContainerState_CONTAINER_EXITED && reason == "" { - if meta.ExitCode == 0 { +// toCRIContainerStatus converts internal container object to CRI container status. +func toCRIContainerStatus(container containerstore.Container) *runtime.ContainerStatus { + meta := container.Metadata + status := container.Status.Get() + reason := status.Reason + if status.State() == runtime.ContainerState_CONTAINER_EXITED && reason == "" { + if status.ExitCode == 0 { reason = completeExitReason } else { reason = errorExitReason @@ -60,15 +60,15 @@ func toCRIContainerStatus(meta *metadata.ContainerMetadata) *runtime.ContainerSt return &runtime.ContainerStatus{ Id: meta.ID, Metadata: meta.Config.GetMetadata(), - State: state, - CreatedAt: meta.CreatedAt, - StartedAt: meta.StartedAt, - FinishedAt: meta.FinishedAt, - ExitCode: meta.ExitCode, + State: status.State(), + CreatedAt: status.CreatedAt, + StartedAt: status.StartedAt, + FinishedAt: status.FinishedAt, + ExitCode: status.ExitCode, Image: meta.Config.GetImage(), ImageRef: meta.ImageRef, Reason: reason, - Message: meta.Message, + Message: status.Message, Labels: meta.Config.GetLabels(), Annotations: meta.Config.GetAnnotations(), Mounts: meta.Config.GetMounts(), diff --git a/pkg/server/container_status_test.go b/pkg/server/container_status_test.go index 9e3f58189..8ef334993 100644 --- a/pkg/server/container_status_test.go +++ b/pkg/server/container_status_test.go @@ -22,13 +22,12 @@ import ( "github.com/stretchr/testify/assert" "golang.org/x/net/context" - "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) -func getContainerStatusTestData() (*metadata.ContainerMetadata, *runtime.ContainerStatus) { +func getContainerStatusTestData() (*containerstore.Metadata, *containerstore.Status, *runtime.ContainerStatus) { testID := "test-id" config := &runtime.ContainerConfig{ Metadata: &runtime.ContainerMetadata{ @@ -47,17 +46,18 @@ func getContainerStatusTestData() (*metadata.ContainerMetadata, *runtime.Contain createdAt := time.Now().UnixNano() startedAt := time.Now().UnixNano() - metadata := &metadata.ContainerMetadata{ + metadata := &containerstore.Metadata{ ID: testID, Name: "test-long-name", SandboxID: "test-sandbox-id", Config: config, ImageRef: "test-image-ref", + } + status := &containerstore.Status{ Pid: 1234, CreatedAt: createdAt, StartedAt: startedAt, } - expected := &runtime.ContainerStatus{ Id: testID, Metadata: config.GetMetadata(), @@ -72,7 +72,7 @@ func getContainerStatusTestData() (*metadata.ContainerMetadata, *runtime.Contain Mounts: config.GetMounts(), } - return metadata, expected + return metadata, status, expected } func TestToCRIContainerStatus(t *testing.T) { @@ -110,19 +110,21 @@ func TestToCRIContainerStatus(t *testing.T) { expectedReason: errorExitReason, }, } { - meta, expected := getContainerStatusTestData() - // Update metadata with test case. - meta.FinishedAt = test.finishedAt - meta.ExitCode = test.exitCode - meta.Reason = test.reason - meta.Message = test.message + metadata, status, expected := getContainerStatusTestData() + // Update status with test case. + status.FinishedAt = test.finishedAt + status.ExitCode = test.exitCode + status.Reason = test.reason + status.Message = test.message + container, err := containerstore.NewContainer(*metadata, *status) + assert.NoError(t, err) // Set expectation based on test case. expected.State = test.expectedState expected.Reason = test.expectedReason expected.FinishedAt = test.finishedAt expected.ExitCode = test.exitCode expected.Message = test.message - assert.Equal(t, expected, toCRIContainerStatus(meta), desc) + assert.Equal(t, expected, toCRIContainerStatus(container), desc) } } @@ -151,14 +153,16 @@ func TestContainerStatus(t *testing.T) { } { t.Logf("TestCase %q", desc) c := newTestCRIContainerdService() - meta, expected := getContainerStatusTestData() - // Update metadata with test case. - meta.FinishedAt = test.finishedAt - meta.Reason = test.reason + metadata, status, expected := getContainerStatusTestData() + // Update status with test case. + status.FinishedAt = test.finishedAt + status.Reason = test.reason + container, err := containerstore.NewContainer(*metadata, *status) + assert.NoError(t, err) if test.exist { - assert.NoError(t, c.containerStore.Create(*meta)) + assert.NoError(t, c.containerStore.Add(container)) } - resp, err := c.ContainerStatus(context.Background(), &runtime.ContainerStatusRequest{ContainerId: meta.ID}) + resp, err := c.ContainerStatus(context.Background(), &runtime.ContainerStatusRequest{ContainerId: container.ID}) if test.expectErr { assert.Error(t, err) assert.Nil(t, resp) diff --git a/pkg/server/container_stop.go b/pkg/server/container_stop.go index f4e8614c4..299550d4b 100644 --- a/pkg/server/container_stop.go +++ b/pkg/server/container_stop.go @@ -27,7 +27,8 @@ import ( "golang.org/x/sys/unix" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) const ( @@ -50,12 +51,12 @@ func (c *criContainerdService) StopContainer(ctx context.Context, r *runtime.Sto }() // Get container config from container store. - meta, err := c.containerStore.Get(r.GetContainerId()) + container, err := c.containerStore.Get(r.GetContainerId()) if err != nil { return nil, fmt.Errorf("an error occurred when try to find container %q: %v", r.GetContainerId(), err) } - if err := c.stopContainer(ctx, meta, time.Duration(r.GetTimeout())*time.Second); err != nil { + if err := c.stopContainer(ctx, container, time.Duration(r.GetTimeout())*time.Second); err != nil { return nil, err } @@ -63,32 +64,33 @@ func (c *criContainerdService) StopContainer(ctx context.Context, r *runtime.Sto } // stopContainer stops a container based on the container metadata. -func (c *criContainerdService) stopContainer(ctx context.Context, meta *metadata.ContainerMetadata, timeout time.Duration) error { - id := meta.ID +func (c *criContainerdService) stopContainer(ctx context.Context, container containerstore.Container, timeout time.Duration) error { + id := container.ID // Return without error if container is not running. This makes sure that // stop only takes real action after the container is started. - if meta.State() != runtime.ContainerState_CONTAINER_RUNNING { + state := container.Status.Get().State() + if state != runtime.ContainerState_CONTAINER_RUNNING { glog.V(2).Infof("Container to stop %q is not running, current state %q", - id, criContainerStateToString(meta.State())) + id, criContainerStateToString(state)) return nil } if timeout > 0 { stopSignal := unix.SIGTERM - imageMeta, err := c.imageMetadataStore.Get(meta.ImageRef) + image, err := c.imageStore.Get(container.ImageRef) if err != nil { // NOTE(random-liu): It's possible that the container is stopped, // deleted and image is garbage collected before this point. However, // the chance is really slim, even it happens, it's still fine to return // an error here. - return fmt.Errorf("failed to get image metadata %q: %v", meta.ImageRef, err) + return fmt.Errorf("failed to get image metadata %q: %v", container.ImageRef, err) } - if imageMeta.Config.StopSignal != "" { - stopSignal, err = signal.ParseSignal(imageMeta.Config.StopSignal) + if image.Config.StopSignal != "" { + stopSignal, err = signal.ParseSignal(image.Config.StopSignal) if err != nil { return fmt.Errorf("failed to parse stop signal %q: %v", - imageMeta.Config.StopSignal, err) + image.Config.StopSignal, err) } } glog.V(2).Infof("Stop container %q with signal %v", id, stopSignal) @@ -140,10 +142,10 @@ func (c *criContainerdService) waitContainerStop(ctx context.Context, id string, defer timeoutTimer.Stop() for { // Poll once before waiting for stopCheckPollInterval. - meta, err := c.containerStore.Get(id) + container, err := c.containerStore.Get(id) if err != nil { - if !metadata.IsNotExistError(err) { - return fmt.Errorf("failed to get container %q metadata: %v", id, err) + if err != store.ErrNotExist { + return fmt.Errorf("failed to get container %q: %v", id, err) } // Do not return error here because container was removed means // it is already stopped. @@ -151,7 +153,7 @@ func (c *criContainerdService) waitContainerStop(ctx context.Context, id string, return nil } // TODO(random-liu): Use channel with event handler instead of polling. - if meta.State() == runtime.ContainerState_CONTAINER_EXITED { + if container.Status.Get().State() == runtime.ContainerState_CONTAINER_EXITED { return nil } select { diff --git a/pkg/server/container_stop_test.go b/pkg/server/container_stop_test.go index 3aa6bdf97..80ee1e549 100644 --- a/pkg/server/container_stop_test.go +++ b/pkg/server/container_stop_test.go @@ -29,21 +29,21 @@ import ( "golang.org/x/sys/unix" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) func TestWaitContainerStop(t *testing.T) { id := "test-id" for desc, test := range map[string]struct { - metadata *metadata.ContainerMetadata + status *containerstore.Status cancel bool timeout time.Duration expectErr bool }{ "should return error if timeout exceeds": { - metadata: &metadata.ContainerMetadata{ - ID: id, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), }, @@ -51,8 +51,7 @@ func TestWaitContainerStop(t *testing.T) { expectErr: true, }, "should return error if context is cancelled": { - metadata: &metadata.ContainerMetadata{ - ID: id, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), }, @@ -61,13 +60,12 @@ func TestWaitContainerStop(t *testing.T) { expectErr: true, }, "should not return error if container is removed before timeout": { - metadata: nil, + status: nil, timeout: time.Hour, expectErr: false, }, "should not return error if container is stopped before timeout": { - metadata: &metadata.ContainerMetadata{ - ID: id, + status: &containerstore.Status{ CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), FinishedAt: time.Now().UnixNano(), @@ -77,8 +75,13 @@ func TestWaitContainerStop(t *testing.T) { }, } { c := newTestCRIContainerdService() - if test.metadata != nil { - assert.NoError(t, c.containerStore.Create(*test.metadata)) + if test.status != nil { + container, err := containerstore.NewContainer( + containerstore.Metadata{ID: id}, + *test.status, + ) + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(container)) } ctx := context.Background() if test.cancel { @@ -94,15 +97,14 @@ func TestWaitContainerStop(t *testing.T) { func TestStopContainer(t *testing.T) { testID := "test-id" testPid := uint32(1234) - testMetadata := metadata.ContainerMetadata{ - ID: testID, + testImageID := "test-image-id" + testStatus := containerstore.Status{ Pid: testPid, - ImageRef: "test-image-id", CreatedAt: time.Now().UnixNano(), StartedAt: time.Now().UnixNano(), } - testImageMetadata := metadata.ImageMetadata{ - ID: "test-image-id", + testImage := imagestore.Image{ + ID: testImageID, Config: &imagespec.ImageConfig{}, } testContainer := task.Task{ @@ -111,7 +113,7 @@ func TestStopContainer(t *testing.T) { Status: task.StatusRunning, } for desc, test := range map[string]struct { - metadata *metadata.ContainerMetadata + status *containerstore.Status containerdContainer *task.Task stopSignal string stopErr error @@ -120,20 +122,17 @@ func TestStopContainer(t *testing.T) { expectCalls []servertesting.CalledDetail }{ "should return error when container does not exist": { - metadata: nil, + status: nil, expectErr: true, expectCalls: []servertesting.CalledDetail{}, }, "should not return error when container is not running": { - metadata: &metadata.ContainerMetadata{ - ID: testID, - CreatedAt: time.Now().UnixNano(), - }, + status: &containerstore.Status{CreatedAt: time.Now().UnixNano()}, expectErr: false, expectCalls: []servertesting.CalledDetail{}, }, "should not return error if containerd task does not exist": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, // Since it's hard to inject event during `StopContainer` is running, // we only test the case that first stop returns error, but container @@ -166,7 +165,7 @@ func TestStopContainer(t *testing.T) { }, }, "should not return error if containerd task process already finished": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, stopErr: errors.New("os: process already finished"), expectErr: false, @@ -194,7 +193,7 @@ func TestStopContainer(t *testing.T) { }, }, "should return error if graceful stop returns random error": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, stopErr: errors.New("random stop error"), expectErr: true, @@ -210,7 +209,7 @@ func TestStopContainer(t *testing.T) { }, }, "should not return error if containerd task is gracefully stopped": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, expectErr: false, // deleted by the event monitor. @@ -230,7 +229,7 @@ func TestStopContainer(t *testing.T) { }, }, "should use stop signal specified in image config if not empty": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, stopSignal: "SIGHUP", expectErr: false, @@ -251,7 +250,7 @@ func TestStopContainer(t *testing.T) { }, }, "should directly kill container if timeout is 0": { - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainer, noTimeout: true, expectErr: false, @@ -279,16 +278,24 @@ func TestStopContainer(t *testing.T) { defer fake.Stop() c.taskService = fake - // Inject metadata. - if test.metadata != nil { - assert.NoError(t, c.containerStore.Create(*test.metadata)) + // Inject the container. + if test.status != nil { + cntr, err := containerstore.NewContainer( + containerstore.Metadata{ + ID: testID, + ImageRef: testImageID, + }, + *test.status, + ) + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(cntr)) } // Inject containerd task. if test.containerdContainer != nil { fake.SetFakeTasks([]task.Task{*test.containerdContainer}) } - testImageMetadata.Config.StopSignal = test.stopSignal - assert.NoError(t, c.imageMetadataStore.Create(testImageMetadata)) + testImage.Config.StopSignal = test.stopSignal + c.imageStore.Add(testImage) if test.stopErr != nil { fake.InjectError("kill", test.stopErr) } diff --git a/pkg/server/events.go b/pkg/server/events.go index 1ff70c788..0d9c96914 100644 --- a/pkg/server/events.go +++ b/pkg/server/events.go @@ -25,7 +25,7 @@ import ( "github.com/jpillora/backoff" "golang.org/x/net/context" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) const ( @@ -87,12 +87,12 @@ func (c *criContainerdService) handleEvent(e *task.Event) { // fine to leave out that case for now. // TODO(random-liu): [P2] Handle containerd-shim exit. case task.Event_EXIT: - meta, err := c.containerStore.Get(e.ID) + cntr, err := c.containerStore.Get(e.ID) if err != nil { - glog.Errorf("Failed to get container %q metadata: %v", e.ID, err) + glog.Errorf("Failed to get container %q: %v", e.ID, err) return } - if e.Pid != meta.Pid { + if e.Pid != cntr.Status.Get().Pid { // Non-init process died, ignore the event. return } @@ -103,16 +103,16 @@ func (c *criContainerdService) handleEvent(e *task.Event) { glog.Errorf("Failed to delete container %q: %v", e.ID, err) return } - err = c.containerStore.Update(e.ID, func(meta metadata.ContainerMetadata) (metadata.ContainerMetadata, error) { + err = cntr.Status.Update(func(status containerstore.Status) (containerstore.Status, error) { // If FinishedAt has been set (e.g. with start failure), keep as // it is. - if meta.FinishedAt != 0 { - return meta, nil + if status.FinishedAt != 0 { + return status, nil } - meta.Pid = 0 - meta.FinishedAt = e.ExitedAt.UnixNano() - meta.ExitCode = int32(e.ExitStatus) - return meta, nil + status.Pid = 0 + status.FinishedAt = e.ExitedAt.UnixNano() + status.ExitCode = int32(e.ExitStatus) + return status, nil }) if err != nil { glog.Errorf("Failed to update container %q state: %v", e.ID, err) @@ -120,11 +120,15 @@ func (c *criContainerdService) handleEvent(e *task.Event) { return } case task.Event_OOM: - err := c.containerStore.Update(e.ID, func(meta metadata.ContainerMetadata) (metadata.ContainerMetadata, error) { - meta.Reason = oomExitReason - return meta, nil + cntr, err := c.containerStore.Get(e.ID) + if err != nil { + glog.Errorf("Failed to get container %q: %v", e.ID, err) + } + err = cntr.Status.Update(func(status containerstore.Status) (containerstore.Status, error) { + status.Reason = oomExitReason + return status, nil }) - if err != nil && !metadata.IsNotExistError(err) { + if err != nil { glog.Errorf("Failed to update container %q oom: %v", e.ID, err) return } diff --git a/pkg/server/events_test.go b/pkg/server/events_test.go index b3ed3a83a..336d60fc7 100644 --- a/pkg/server/events_test.go +++ b/pkg/server/events_test.go @@ -27,8 +27,8 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" ) func TestHandleEvent(t *testing.T) { @@ -36,11 +36,13 @@ func TestHandleEvent(t *testing.T) { testPid := uint32(1234) testCreatedAt := time.Now().UnixNano() testStartedAt := time.Now().UnixNano() - // Container metadata in running state. - testMetadata := metadata.ContainerMetadata{ + testMetadata := containerstore.Metadata{ ID: testID, Name: "test-name", SandboxID: "test-sandbox-id", + } + // Container status in running state. + testStatus := containerstore.Status{ Pid: testPid, CreatedAt: testCreatedAt, StartedAt: testStartedAt, @@ -53,17 +55,14 @@ func TestHandleEvent(t *testing.T) { ExitStatus: 1, ExitedAt: testExitedAt, } - testFinishedMetadata := metadata.ContainerMetadata{ - ID: testID, - Name: "test-name", - SandboxID: "test-sandbox-id", + testFinishedStatus := containerstore.Status{ Pid: 0, CreatedAt: testCreatedAt, StartedAt: testStartedAt, FinishedAt: testExitedAt.UnixNano(), ExitCode: 1, } - assert.Equal(t, runtime.ContainerState_CONTAINER_RUNNING, testMetadata.State()) + assert.Equal(t, runtime.ContainerState_CONTAINER_RUNNING, testStatus.State()) testContainerdContainer := task.Task{ ID: testID, Pid: testPid, @@ -72,12 +71,12 @@ func TestHandleEvent(t *testing.T) { for desc, test := range map[string]struct { event *task.Event - metadata *metadata.ContainerMetadata + status *containerstore.Status containerdContainer *task.Task containerdErr error - expected *metadata.ContainerMetadata + expected *containerstore.Status }{ - "should not update state when no corresponding metadata for event": { + "should not update state when no corresponding container for event": { event: &testExitEvent, expected: nil, }, @@ -89,16 +88,16 @@ func TestHandleEvent(t *testing.T) { ExitStatus: 1, ExitedAt: testExitedAt, }, - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainerdContainer, - expected: &testMetadata, + expected: &testStatus, }, "should not update state when fail to delete containerd task": { event: &testExitEvent, - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainerdContainer, containerdErr: fmt.Errorf("random error"), - expected: &testMetadata, + expected: &testStatus, }, "should not update state for irrelevant events": { event: &task.Event{ @@ -106,31 +105,28 @@ func TestHandleEvent(t *testing.T) { Type: task.Event_PAUSED, Pid: testPid, }, - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainerdContainer, - expected: &testMetadata, + expected: &testStatus, }, "should update state when containerd task is already deleted": { event: &testExitEvent, - metadata: &testMetadata, - expected: &testFinishedMetadata, + status: &testStatus, + expected: &testFinishedStatus, }, "should update state when delete containerd task successfully": { event: &testExitEvent, - metadata: &testMetadata, + status: &testStatus, containerdContainer: &testContainerdContainer, - expected: &testFinishedMetadata, + expected: &testFinishedStatus, }, "should update exit reason when container is oom killed": { event: &task.Event{ ID: testID, Type: task.Event_OOM, }, - metadata: &testMetadata, - expected: &metadata.ContainerMetadata{ - ID: testID, - Name: "test-name", - SandboxID: "test-sandbox-id", + status: &testStatus, + expected: &containerstore.Status{ Pid: testPid, CreatedAt: testCreatedAt, StartedAt: testStartedAt, @@ -148,10 +144,14 @@ func TestHandleEvent(t *testing.T) { if test.event != nil { fakeEvents.Events <- test.event } - // Inject metadata. - if test.metadata != nil { - // Make sure that original data will not be changed. - assert.NoError(t, c.containerStore.Create(*test.metadata)) + // Inject internal container object. + if test.status != nil { + cntr, err := containerstore.NewContainer( // nolint: vetshadow + testMetadata, + *test.status, + ) + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(cntr)) } // Inject containerd task. if test.containerdContainer != nil { @@ -161,8 +161,12 @@ func TestHandleEvent(t *testing.T) { if test.containerdErr != nil { fake.InjectError("delete", test.containerdErr) } - c.handleEventStream(e) - got, _ := c.containerStore.Get(testID) - assert.Equal(t, test.expected, got) + assert.NoError(t, c.handleEventStream(e)) + if test.expected == nil { + continue + } + got, err := c.containerStore.Get(testID) + assert.NoError(t, err) + assert.Equal(t, *test.expected, got.Status.Get()) } } diff --git a/pkg/server/helpers.go b/pkg/server/helpers.go index d790ce31a..f95a7b382 100644 --- a/pkg/server/helpers.go +++ b/pkg/server/helpers.go @@ -36,7 +36,8 @@ import ( "google.golang.org/grpc/codes" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) const ( @@ -328,7 +329,7 @@ func getRepoDigestAndTag(namedRef reference.Named, digest imagedigest.Digest, sc // localResolve resolves image reference locally and returns corresponding image metadata. It returns // nil without error if the reference doesn't exist. -func (c *criContainerdService) localResolve(ctx context.Context, ref string) (*metadata.ImageMetadata, error) { +func (c *criContainerdService) localResolve(ctx context.Context, ref string) (*imagestore.Image, error) { _, err := imagedigest.Parse(ref) if err != nil { // ref is not image id, try to resolve it locally. @@ -336,7 +337,7 @@ func (c *criContainerdService) localResolve(ctx context.Context, ref string) (*m if err != nil { return nil, fmt.Errorf("invalid image reference %q: %v", ref, err) } - image, err := c.imageStoreService.Get(ctx, normalized.String()) + imageInContainerd, err := c.imageStoreService.Get(ctx, normalized.String()) if err != nil { if containerdmetadata.IsNotFound(err) { return nil, nil @@ -344,21 +345,21 @@ func (c *criContainerdService) localResolve(ctx context.Context, ref string) (*m return nil, fmt.Errorf("an error occurred when getting image %q from containerd image store: %v", normalized.String(), err) } - desc, err := image.Config(ctx, c.contentStoreService) + desc, err := imageInContainerd.Config(ctx, c.contentStoreService) if err != nil { return nil, fmt.Errorf("failed to get image config descriptor: %v", err) } ref = desc.Digest.String() } imageID := ref - meta, err := c.imageMetadataStore.Get(imageID) + image, err := c.imageStore.Get(imageID) if err != nil { - if metadata.IsNotExistError(err) { + if err == store.ErrNotExist { return nil, nil } return nil, fmt.Errorf("failed to get image %q metadata: %v", imageID, err) } - return meta, nil + return &image, nil } // getUserFromImage gets uid or user name of the image user. @@ -382,13 +383,13 @@ func getUserFromImage(user string) (*int64, string) { // ensureImageExists returns corresponding metadata of the image reference, if image is not // pulled yet, the function will pull the image. -func (c *criContainerdService) ensureImageExists(ctx context.Context, ref string) (*metadata.ImageMetadata, error) { - meta, err := c.localResolve(ctx, ref) +func (c *criContainerdService) ensureImageExists(ctx context.Context, ref string) (*imagestore.Image, error) { + image, err := c.localResolve(ctx, ref) if err != nil { return nil, fmt.Errorf("failed to resolve image %q: %v", ref, err) } - if meta != nil { - return meta, nil + if image != nil { + return image, nil } // Pull image to ensure the image exists resp, err := c.PullImage(ctx, &runtime.PullImageRequest{Image: &runtime.ImageSpec{Image: ref}}) @@ -396,10 +397,10 @@ func (c *criContainerdService) ensureImageExists(ctx context.Context, ref string return nil, fmt.Errorf("failed to pull image %q: %v", ref, err) } imageID := resp.GetImageRef() - meta, err = c.imageMetadataStore.Get(imageID) + newImage, err := c.imageStore.Get(imageID) if err != nil { // It's still possible that someone removed the image right after it is pulled. return nil, fmt.Errorf("failed to get image %q metadata after pulling: %v", imageID, err) } - return meta, nil + return &newImage, nil } diff --git a/pkg/server/image_list.go b/pkg/server/image_list.go index d1f8af59b..dc80d98ec 100644 --- a/pkg/server/image_list.go +++ b/pkg/server/image_list.go @@ -17,13 +17,11 @@ limitations under the License. package server import ( - "fmt" - "github.com/golang/glog" "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) // ListImages lists existing images. @@ -37,12 +35,10 @@ func (c *criContainerdService) ListImages(ctx context.Context, r *runtime.ListIm } }() - imageMetadataA, err := c.imageMetadataStore.List() - if err != nil { - return nil, fmt.Errorf("failed to list image metadata from store: %v", err) - } + imagesInStore := c.imageStore.List() + var images []*runtime.Image - for _, image := range imageMetadataA { + for _, image := range imagesInStore { // TODO(random-liu): [P0] Make sure corresponding snapshot exists. What if snapshot // doesn't exist? images = append(images, toCRIImage(image)) @@ -51,8 +47,8 @@ func (c *criContainerdService) ListImages(ctx context.Context, r *runtime.ListIm return &runtime.ListImagesResponse{Images: images}, nil } -// toCRIImage converts image metadata to CRI image type. -func toCRIImage(image *metadata.ImageMetadata) *runtime.Image { +// toCRIImage converts image to CRI image type. +func toCRIImage(image imagestore.Image) *runtime.Image { runtimeImage := &runtime.Image{ Id: image.ID, RepoTags: image.RepoTags, diff --git a/pkg/server/image_list_test.go b/pkg/server/image_list_test.go index 181cdd73b..e8606642c 100644 --- a/pkg/server/image_list_test.go +++ b/pkg/server/image_list_test.go @@ -25,12 +25,12 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) func TestListImages(t *testing.T) { c := newTestCRIContainerdService() - imagesInStore := []metadata.ImageMetadata{ + imagesInStore := []imagestore.Image{ { ID: "test-id-1", ChainID: "test-chainid-1", @@ -87,7 +87,7 @@ func TestListImages(t *testing.T) { } for _, i := range imagesInStore { - assert.NoError(t, c.imageMetadataStore.Create(i)) + c.imageStore.Add(i) } resp, err := c.ListImages(context.Background(), &runtime.ListImagesRequest{}) diff --git a/pkg/server/image_pull.go b/pkg/server/image_pull.go index 269c09864..6eb4992df 100644 --- a/pkg/server/image_pull.go +++ b/pkg/server/image_pull.go @@ -37,7 +37,7 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) // For image management: @@ -87,60 +87,41 @@ func (c *criContainerdService) PullImage(ctx context.Context, r *runtime.PullIma r.GetImage().GetImage(), retRes.GetImageRef()) } }() - image := r.GetImage().GetImage() + imageRef := r.GetImage().GetImage() // TODO(mikebrow): add truncIndex for image id - imageID, repoTag, repoDigest, err := c.pullImage(ctx, image, r.GetAuth()) + imageID, repoTag, repoDigest, err := c.pullImage(ctx, imageRef, r.GetAuth()) if err != nil { - return nil, fmt.Errorf("failed to pull image %q: %v", image, err) + return nil, fmt.Errorf("failed to pull image %q: %v", imageRef, err) } - glog.V(4).Infof("Pulled image %q with image id %q, repo tag %q, repo digest %q", image, imageID, + glog.V(4).Infof("Pulled image %q with image id %q, repo tag %q, repo digest %q", imageRef, imageID, repoTag, repoDigest) - _, err = c.imageMetadataStore.Get(imageID) - if err != nil && !metadata.IsNotExistError(err) { - return nil, fmt.Errorf("failed to get image %q metadata: %v", imageID, err) - } - // There is a known race here because the image metadata could be created after `Get`. - // TODO(random-liu): [P1] Do not use metadata store. Use simple in-memory data structure to - // maintain the id -> information index. And use the container image store as backup and - // recover in-memory state during startup. - if err == nil { - // Update existing image metadata. - if err := c.imageMetadataStore.Update(imageID, func(m metadata.ImageMetadata) (metadata.ImageMetadata, error) { - updateImageMetadata(&m, repoTag, repoDigest) - return m, nil - }); err != nil { - return nil, fmt.Errorf("failed to update image %q metadata: %v", imageID, err) - } - return &runtime.PullImageResponse{ImageRef: imageID}, err - } - // Get image information. - chainID, size, config, err := c.getImageInfo(ctx, image) + chainID, size, config, err := c.getImageInfo(ctx, imageRef) if err != nil { - return nil, fmt.Errorf("failed to get image %q information: %v", image, err) + return nil, fmt.Errorf("failed to get image %q information: %v", imageRef, err) } - - // NOTE(random-liu): the actual state in containerd is the source of truth, even we maintain - // in-memory image metadata, it's only for in-memory indexing. The image could be removed - // by someone else anytime, before/during/after we create the metadata. We should always - // check the actual state in containerd before using the image or returning status of the - // image. - - // Create corresponding image metadata. - newMeta := metadata.ImageMetadata{ + image := imagestore.Image{ ID: imageID, ChainID: chainID.String(), Size: size, Config: config, } - // Add the image reference used into repo tags. Note if the image is pulled with - // repo digest, it will also be added in to repo tags, which is fine. - updateImageMetadata(&newMeta, repoTag, repoDigest) - if err := c.imageMetadataStore.Create(newMeta); err != nil { - return nil, fmt.Errorf("failed to create image %q metadata: %v", imageID, err) + + if repoDigest != "" { + image.RepoDigests = []string{repoDigest} } + if repoTag != "" { + image.RepoTags = []string{repoTag} + } + c.imageStore.Add(image) + + // NOTE(random-liu): the actual state in containerd is the source of truth, even we maintain + // in-memory image store, it's only for in-memory indexing. The image could be removed + // by someone else anytime, before/during/after we create the metadata. We should always + // check the actual state in containerd before using the image or returning status of the + // image. return &runtime.PullImageResponse{ImageRef: imageID}, err } @@ -393,29 +374,3 @@ func (c *criContainerdService) waitForResourcesDownloading(ctx context.Context, } } } - -// insertToStringSlice is a helper function to insert a string into the string slice -// if the string is not in the slice yet. -func insertToStringSlice(ss []string, s string) []string { - found := false - for _, str := range ss { - if s == str { - found = true - break - } - } - if !found { - ss = append(ss, s) - } - return ss -} - -// updateImageMetadata updates existing image meta with new repoTag and repoDigest. -func updateImageMetadata(meta *metadata.ImageMetadata, repoTag, repoDigest string) { - if repoTag != "" { - meta.RepoTags = insertToStringSlice(meta.RepoTags, repoTag) - } - if repoDigest != "" { - meta.RepoDigests = insertToStringSlice(meta.RepoDigests, repoDigest) - } -} diff --git a/pkg/server/image_pull_test.go b/pkg/server/image_pull_test.go index 1272bbeb8..42fc6bb66 100644 --- a/pkg/server/image_pull_test.go +++ b/pkg/server/image_pull_test.go @@ -24,59 +24,8 @@ import ( "github.com/stretchr/testify/assert" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ) -func TestUpdateImageMetadata(t *testing.T) { - meta := metadata.ImageMetadata{ - ID: "test-id", - ChainID: "test-chain-id", - Size: 1234, - } - for desc, test := range map[string]struct { - repoTags []string - repoDigests []string - repoTag string - repoDigest string - expectedRepoTags []string - expectedRepoDigests []string - }{ - "Add duplicated repo tag and digest": { - repoTags: []string{"a", "b"}, - repoDigests: []string{"c", "d"}, - repoTag: "a", - repoDigest: "c", - expectedRepoTags: []string{"a", "b"}, - expectedRepoDigests: []string{"c", "d"}, - }, - "Add new repo tag and digest": { - repoTags: []string{"a", "b"}, - repoDigests: []string{"c", "d"}, - repoTag: "e", - repoDigest: "f", - expectedRepoTags: []string{"a", "b", "e"}, - expectedRepoDigests: []string{"c", "d", "f"}, - }, - "Add empty repo tag and digest": { - repoTags: []string{"a", "b"}, - repoDigests: []string{"c", "d"}, - repoTag: "", - repoDigest: "", - expectedRepoTags: []string{"a", "b"}, - expectedRepoDigests: []string{"c", "d"}, - }, - } { - t.Logf("TestCase %q", desc) - m := meta - m.RepoTags = test.repoTags - m.RepoDigests = test.repoDigests - updateImageMetadata(&m, test.repoTag, test.repoDigest) - assert.Equal(t, test.expectedRepoTags, m.RepoTags) - assert.Equal(t, test.expectedRepoDigests, m.RepoDigests) - } -} - func TestResources(t *testing.T) { const threads = 10 var wg sync.WaitGroup diff --git a/pkg/server/image_remove.go b/pkg/server/image_remove.go index 39061377e..6e3e23956 100644 --- a/pkg/server/image_remove.go +++ b/pkg/server/image_remove.go @@ -19,13 +19,10 @@ package server import ( "fmt" + containerdmetadata "github.com/containerd/containerd/metadata" "github.com/golang/glog" "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - - containerdmetadata "github.com/containerd/containerd/metadata" - - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ) // RemoveImage removes the image. @@ -41,29 +38,25 @@ func (c *criContainerdService) RemoveImage(ctx context.Context, r *runtime.Remov glog.V(2).Infof("RemoveImage %q returns successfully", r.GetImage().GetImage()) } }() - meta, err := c.localResolve(ctx, r.GetImage().GetImage()) + image, err := c.localResolve(ctx, r.GetImage().GetImage()) if err != nil { return nil, fmt.Errorf("can not resolve %q locally: %v", r.GetImage().GetImage(), err) } - if meta == nil { + if image == nil { // return empty without error when image not found. return &runtime.RemoveImageResponse{}, nil } + // Include all image references, including RepoTag, RepoDigest and id. - for _, ref := range append(append(meta.RepoTags, meta.RepoDigests...), meta.ID) { + for _, ref := range append(append(image.RepoTags, image.RepoDigests...), image.ID) { // TODO(random-liu): Containerd should schedule a garbage collection immediately, // and we may want to wait for the garbage collection to be over here. err = c.imageStoreService.Delete(ctx, ref) if err == nil || containerdmetadata.IsNotFound(err) { continue } - return nil, fmt.Errorf("failed to delete image reference %q for image %q: %v", ref, meta.ID, err) - } - if err = c.imageMetadataStore.Delete(meta.ID); err != nil { - if metadata.IsNotExistError(err) { - return &runtime.RemoveImageResponse{}, nil - } - return nil, fmt.Errorf("an error occurred when delete image %q matadata: %v", meta.ID, err) + return nil, fmt.Errorf("failed to delete image reference %q for image %q: %v", ref, image.ID, err) } + c.imageStore.Delete(image.ID) return &runtime.RemoveImageResponse{}, nil } diff --git a/pkg/server/image_status.go b/pkg/server/image_status.go index d4475b04b..f93462291 100644 --- a/pkg/server/image_status.go +++ b/pkg/server/image_status.go @@ -35,28 +35,28 @@ func (c *criContainerdService) ImageStatus(ctx context.Context, r *runtime.Image r.GetImage().GetImage(), retRes.GetImage()) } }() - meta, err := c.localResolve(ctx, r.GetImage().GetImage()) + image, err := c.localResolve(ctx, r.GetImage().GetImage()) if err != nil { return nil, fmt.Errorf("can not resolve %q locally: %v", r.GetImage().GetImage(), err) } - if meta == nil { + if image == nil { // return empty without error when image not found. return &runtime.ImageStatusResponse{}, nil } // TODO(random-liu): [P0] Make sure corresponding snapshot exists. What if snapshot // doesn't exist? runtimeImage := &runtime.Image{ - Id: meta.ID, - RepoTags: meta.RepoTags, - RepoDigests: meta.RepoDigests, - Size_: uint64(meta.Size), + Id: image.ID, + RepoTags: image.RepoTags, + RepoDigests: image.RepoDigests, + Size_: uint64(image.Size), } - uid, username := getUserFromImage(meta.Config.User) + uid, username := getUserFromImage(image.Config.User) if uid != nil { runtimeImage.Uid = &runtime.Int64Value{Value: *uid} } runtimeImage.Username = username - // TODO(mikebrow): write a ImageMetadata to runtim.Image converter + // TODO(mikebrow): write a ImageMetadata to runtime.Image converter return &runtime.ImageStatusResponse{Image: runtimeImage}, nil } diff --git a/pkg/server/image_status_test.go b/pkg/server/image_status_test.go index 059abdc46..41b45ec4b 100644 --- a/pkg/server/image_status_test.go +++ b/pkg/server/image_status_test.go @@ -25,12 +25,12 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) func TestImageStatus(t *testing.T) { testID := "sha256:d848ce12891bf78792cda4a23c58984033b0c397a55e93a1556202222ecc5ed4" - meta := metadata.ImageMetadata{ + image := imagestore.Image{ ID: testID, ChainID: "test-chain-id", RepoTags: []string{"a", "b"}, @@ -57,7 +57,7 @@ func TestImageStatus(t *testing.T) { require.NotNil(t, resp) assert.Nil(t, resp.GetImage()) - assert.NoError(t, c.imageMetadataStore.Create(meta)) + c.imageStore.Add(image) t.Logf("should return correct image status for exist image") resp, err = c.ImageStatus(context.Background(), &runtime.ImageStatusRequest{ diff --git a/pkg/server/sandbox_list.go b/pkg/server/sandbox_list.go index 69583e847..fea4f8bf0 100644 --- a/pkg/server/sandbox_list.go +++ b/pkg/server/sandbox_list.go @@ -27,7 +27,7 @@ import ( "github.com/containerd/containerd/api/types/task" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) // ListPodSandbox returns a list of Sandbox. @@ -39,11 +39,8 @@ func (c *criContainerdService) ListPodSandbox(ctx context.Context, r *runtime.Li } }() - // List all sandbox metadata from store. - sandboxesInStore, err := c.sandboxStore.List() - if err != nil { - return nil, fmt.Errorf("failed to list metadata from sandbox store: %v", err) - } + // List all sandboxes from store. + sandboxesInStore := c.sandboxStore.List() resp, err := c.taskService.List(ctx, &execution.ListRequest{}) if err != nil { @@ -68,7 +65,7 @@ func (c *criContainerdService) ListPodSandbox(ctx context.Context, r *runtime.Li state = runtime.PodSandboxState_SANDBOX_READY } - sandboxes = append(sandboxes, toCRISandbox(sandboxInStore, state)) + sandboxes = append(sandboxes, toCRISandbox(sandboxInStore.Metadata, state)) } sandboxes = c.filterCRISandboxes(sandboxes, r.GetFilter()) @@ -76,7 +73,7 @@ func (c *criContainerdService) ListPodSandbox(ctx context.Context, r *runtime.Li } // toCRISandbox converts sandbox metadata into CRI pod sandbox. -func toCRISandbox(meta *metadata.SandboxMetadata, state runtime.PodSandboxState) *runtime.PodSandbox { +func toCRISandbox(meta sandboxstore.Metadata, state runtime.PodSandboxState) *runtime.PodSandbox { return &runtime.PodSandbox{ Id: meta.ID, Metadata: meta.Config.GetMetadata(), diff --git a/pkg/server/sandbox_list_test.go b/pkg/server/sandbox_list_test.go index e1f8cfa4c..56684f63b 100644 --- a/pkg/server/sandbox_list_test.go +++ b/pkg/server/sandbox_list_test.go @@ -27,8 +27,8 @@ import ( "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) func TestToCRISandbox(t *testing.T) { @@ -43,7 +43,7 @@ func TestToCRISandbox(t *testing.T) { Annotations: map[string]string{"c": "d"}, } createdAt := time.Now().UnixNano() - meta := &metadata.SandboxMetadata{ + meta := sandboxstore.Metadata{ ID: "test-id", Name: "test-name", Config: config, @@ -137,21 +137,27 @@ func TestListPodSandbox(t *testing.T) { fake := c.taskService.(*servertesting.FakeExecutionClient) - sandboxesInStore := []metadata.SandboxMetadata{ + sandboxesInStore := []sandboxstore.Sandbox{ { - ID: "1", - Name: "name-1", - Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-1"}}, + Metadata: sandboxstore.Metadata{ + ID: "1", + Name: "name-1", + Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-1"}}, + }, }, { - ID: "2", - Name: "name-2", - Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-2"}}, + Metadata: sandboxstore.Metadata{ + ID: "2", + Name: "name-2", + Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-2"}}, + }, }, { - ID: "3", - Name: "name-3", - Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-3"}}, + Metadata: sandboxstore.Metadata{ + ID: "3", + Name: "name-3", + Config: &runtime.PodSandboxConfig{Metadata: &runtime.PodSandboxMetadata{Name: "name-3"}}, + }, }, } sandboxesInContainerd := []task.Task{ @@ -194,7 +200,7 @@ func TestListPodSandbox(t *testing.T) { // Inject test metadata for _, s := range sandboxesInStore { - c.sandboxStore.Create(s) + assert.NoError(t, c.sandboxStore.Add(s)) } // Inject fake containerd tasks diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go index 8493e1c7c..c064ddce8 100644 --- a/pkg/server/sandbox_remove.go +++ b/pkg/server/sandbox_remove.go @@ -26,7 +26,7 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" ) // RemovePodSandbox removes the sandbox. If there are running containers in the @@ -41,7 +41,7 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime. sandbox, err := c.sandboxStore.Get(r.GetPodSandboxId()) if err != nil { - if !metadata.IsNotExistError(err) { + if err != store.ErrNotExist { return nil, fmt.Errorf("an error occurred when try to find sandbox %q: %v", r.GetPodSandboxId(), err) } @@ -76,10 +76,7 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime. // not rely on this behavior. // TODO(random-liu): Introduce an intermediate state to avoid container creation after // this point. - cntrs, err := c.containerStore.List() - if err != nil { - return nil, fmt.Errorf("failed to list all containers: %v", err) - } + cntrs := c.containerStore.List() for _, cntr := range cntrs { if cntr.SandboxID != id { continue @@ -107,15 +104,12 @@ func (c *criContainerdService) RemovePodSandbox(ctx context.Context, r *runtime. glog.V(5).Infof("Remove called for sandbox container %q that does not exist", id, err) } - // Remove sandbox metadata from metadata store. Note that once the sandbox - // metadata is successfully deleted: + // Remove sandbox from sandbox store. Note that once the sandbox is successfully + // deleted: // 1) ListPodSandbox will not include this sandbox. // 2) PodSandboxStatus and StopPodSandbox will return error. - // 3) On-going operations which have held the metadata reference will not be - // affected. - if err := c.sandboxStore.Delete(id); err != nil { - return nil, fmt.Errorf("failed to delete sandbox metadata for %q: %v", id, err) - } + // 3) On-going operations which have held the reference will not be affected. + c.sandboxStore.Delete(id) // Release the sandbox name reserved for the sandbox. c.sandboxNameIndex.ReleaseByKey(id) diff --git a/pkg/server/sandbox_remove_test.go b/pkg/server/sandbox_remove_test.go index 71d37690e..17baef318 100644 --- a/pkg/server/sandbox_remove_test.go +++ b/pkg/server/sandbox_remove_test.go @@ -27,21 +27,25 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + "github.com/kubernetes-incubator/cri-containerd/pkg/store" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) func TestRemovePodSandbox(t *testing.T) { testID := "test-id" testName := "test-name" - testMetadata := metadata.SandboxMetadata{ - ID: testID, - Name: testName, + testSandbox := sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testID, + Name: testName, + }, } for desc, test := range map[string]struct { sandboxTasks []task.Task - injectMetadata bool + injectSandbox bool removeSnapshotErr error deleteContainerErr error taskInfoErr error @@ -51,60 +55,60 @@ func TestRemovePodSandbox(t *testing.T) { expectCalls []string }{ "should not return error if sandbox does not exist": { - injectMetadata: false, + injectSandbox: false, removeSnapshotErr: servertesting.SnapshotNotExistError, deleteContainerErr: servertesting.ContainerNotExistError, expectErr: false, expectCalls: []string{}, }, "should not return error if snapshot does not exist": { - injectMetadata: true, + injectSandbox: true, removeSnapshotErr: servertesting.SnapshotNotExistError, expectRemoved: getSandboxRootDir(testRootDir, testID), expectCalls: []string{"info"}, }, "should return error if remove snapshot fails": { - injectMetadata: true, + injectSandbox: true, removeSnapshotErr: fmt.Errorf("arbitrary error"), expectErr: true, expectCalls: []string{"info"}, }, "should return error when sandbox container task is not deleted": { - injectMetadata: true, - sandboxTasks: []task.Task{{ID: testID}}, - expectErr: true, - expectCalls: []string{"info"}, + injectSandbox: true, + sandboxTasks: []task.Task{{ID: testID}}, + expectErr: true, + expectCalls: []string{"info"}, }, "should return error when arbitrary containerd error is injected": { - injectMetadata: true, - taskInfoErr: fmt.Errorf("arbitrary error"), - expectErr: true, - expectCalls: []string{"info"}, + injectSandbox: true, + taskInfoErr: fmt.Errorf("arbitrary error"), + expectErr: true, + expectCalls: []string{"info"}, }, "should return error when error fs error is injected": { - injectMetadata: true, - injectFSErr: fmt.Errorf("fs error"), - expectRemoved: getSandboxRootDir(testRootDir, testID), - expectErr: true, - expectCalls: []string{"info"}, + injectSandbox: true, + injectFSErr: fmt.Errorf("fs error"), + expectRemoved: getSandboxRootDir(testRootDir, testID), + expectErr: true, + expectCalls: []string{"info"}, }, "should not return error if sandbox container does not exist": { - injectMetadata: true, + injectSandbox: true, deleteContainerErr: servertesting.ContainerNotExistError, expectRemoved: getSandboxRootDir(testRootDir, testID), expectCalls: []string{"info"}, }, "should return error if delete sandbox container fails": { - injectMetadata: true, + injectSandbox: true, deleteContainerErr: fmt.Errorf("arbitrary error"), expectRemoved: getSandboxRootDir(testRootDir, testID), expectErr: true, expectCalls: []string{"info"}, }, "should be able to successfully delete": { - injectMetadata: true, - expectRemoved: getSandboxRootDir(testRootDir, testID), - expectCalls: []string{"info"}, + injectSandbox: true, + expectRemoved: getSandboxRootDir(testRootDir, testID), + expectCalls: []string{"info"}, }, } { t.Logf("TestCase %q", desc) @@ -114,9 +118,9 @@ func TestRemovePodSandbox(t *testing.T) { fakeExecutionClient := c.taskService.(*servertesting.FakeExecutionClient) fakeSnapshotClient := WithFakeSnapshotClient(c) fakeExecutionClient.SetFakeTasks(test.sandboxTasks) - if test.injectMetadata { + if test.injectSandbox { c.sandboxNameIndex.Reserve(testName, testID) - c.sandboxStore.Create(testMetadata) + assert.NoError(t, c.sandboxStore.Add(testSandbox)) } if test.removeSnapshotErr == nil { fakeSnapshotClient.SetFakeMounts(testID, []*mount.Mount{ @@ -157,10 +161,8 @@ func TestRemovePodSandbox(t *testing.T) { assert.NotNil(t, res) assert.NoError(t, c.sandboxNameIndex.Reserve(testName, testID), "sandbox name should be released") - meta, err := c.sandboxStore.Get(testID) - assert.Error(t, err) - assert.True(t, metadata.IsNotExistError(err)) - assert.Nil(t, meta, "sandbox metadata should be removed") + _, err = c.sandboxStore.Get(testID) + assert.Equal(t, store.ErrNotExist, err, "sandbox should be removed") mountsResp, err := fakeSnapshotClient.Mounts(context.Background(), &snapshotapi.MountsRequest{Key: testID}) assert.Equal(t, servertesting.SnapshotNotExistError, err, "snapshot should be removed") assert.Nil(t, mountsResp) @@ -178,38 +180,49 @@ func TestRemovePodSandbox(t *testing.T) { func TestRemoveContainersInSandbox(t *testing.T) { testID := "test-id" testName := "test-name" - testMetadata := metadata.SandboxMetadata{ - ID: testID, - Name: testName, + testSandbox := sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testID, + Name: testName, + }, } - testContainersMetadata := []*metadata.ContainerMetadata{ + testContainers := []containerForTest{ { - ID: "test-cid-1", - Name: "test-cname-1", - SandboxID: testID, - FinishedAt: time.Now().UnixNano(), + metadata: containerstore.Metadata{ + ID: "test-cid-1", + Name: "test-cname-1", + SandboxID: testID, + }, + status: containerstore.Status{FinishedAt: time.Now().UnixNano()}, }, { - ID: "test-cid-2", - Name: "test-cname-2", - SandboxID: testID, - FinishedAt: time.Now().UnixNano(), + metadata: containerstore.Metadata{ + + ID: "test-cid-2", + Name: "test-cname-2", + SandboxID: testID, + }, + status: containerstore.Status{FinishedAt: time.Now().UnixNano()}, }, { - ID: "test-cid-3", - Name: "test-cname-3", - SandboxID: "other-sandbox-id", - FinishedAt: time.Now().UnixNano(), + metadata: containerstore.Metadata{ + ID: "test-cid-3", + Name: "test-cname-3", + SandboxID: "other-sandbox-id", + }, + status: containerstore.Status{FinishedAt: time.Now().UnixNano()}, }, } c := newTestCRIContainerdService() WithFakeSnapshotClient(c) assert.NoError(t, c.sandboxNameIndex.Reserve(testName, testID)) - assert.NoError(t, c.sandboxStore.Create(testMetadata)) - for _, cntr := range testContainersMetadata { - assert.NoError(t, c.containerNameIndex.Reserve(cntr.Name, cntr.ID)) - assert.NoError(t, c.containerStore.Create(*cntr)) + assert.NoError(t, c.sandboxStore.Add(testSandbox)) + for _, tc := range testContainers { + assert.NoError(t, c.containerNameIndex.Reserve(tc.metadata.Name, tc.metadata.ID)) + cntr, err := tc.toContainer() + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(cntr)) } res, err := c.RemovePodSandbox(context.Background(), &runtime.RemovePodSandboxRequest{ @@ -218,12 +231,11 @@ func TestRemoveContainersInSandbox(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, res) - meta, err := c.sandboxStore.Get(testID) - assert.Error(t, err) - assert.True(t, metadata.IsNotExistError(err)) - assert.Nil(t, meta, "sandbox metadata should be removed") + _, err = c.sandboxStore.Get(testID) + assert.Equal(t, store.ErrNotExist, err, "sandbox metadata should be removed") - cntrs, err := c.containerStore.List() - assert.NoError(t, err) - assert.Equal(t, testContainersMetadata[2:], cntrs, "container metadata should be removed") + cntrs := c.containerStore.List() + assert.Len(t, cntrs, 1) + assert.Equal(t, testContainers[2].metadata, cntrs[0].Metadata, "container should be removed") + assert.Equal(t, testContainers[2].status, cntrs[0].Status.Get(), "container should be removed") } diff --git a/pkg/server/sandbox_run.go b/pkg/server/sandbox_run.go index af1759b81..adb3b57e9 100644 --- a/pkg/server/sandbox_run.go +++ b/pkg/server/sandbox_run.go @@ -35,7 +35,7 @@ import ( "golang.org/x/sys/unix" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) // RunPodSandbox creates and starts a pod-level sandbox. Runtimes should ensure @@ -65,22 +65,23 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run } }() - // Create initial sandbox metadata. - meta := metadata.SandboxMetadata{ - ID: id, - Name: name, - Config: config, + // Create initial internal sandbox object. + sandbox := sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: id, + Name: name, + Config: config, + }, } // Ensure sandbox container image snapshot. - imageMeta, err := c.ensureImageExists(ctx, c.sandboxImage) + image, err := c.ensureImageExists(ctx, c.sandboxImage) if err != nil { return nil, fmt.Errorf("failed to get sandbox image %q: %v", defaultSandboxImage, err) } - - rootfsMounts, err := c.snapshotService.View(ctx, id, imageMeta.ChainID) + rootfsMounts, err := c.snapshotService.View(ctx, id, image.ChainID) if err != nil { - return nil, fmt.Errorf("failed to prepare sandbox rootfs %q: %v", imageMeta.ChainID, err) + return nil, fmt.Errorf("failed to prepare sandbox rootfs %q: %v", image.ChainID, err) } defer func() { if retErr != nil { @@ -99,7 +100,7 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run } // Create sandbox container. - spec, err := c.generateSandboxContainerSpec(id, config, imageMeta.Config) + spec, err := c.generateSandboxContainerSpec(id, config, image.Config) if err != nil { return nil, fmt.Errorf("failed to generate sandbox container spec: %v", err) } @@ -112,7 +113,7 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run Container: containers.Container{ ID: id, // TODO(random-liu): Checkpoint metadata into container labels. - Image: imageMeta.ID, + Image: image.ID, Runtime: defaultRuntime, Spec: &prototypes.Any{ TypeUrl: runtimespec.Version, @@ -205,19 +206,19 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run } }() - meta.Pid = createResp.Pid - meta.NetNS = getNetworkNamespace(createResp.Pid) + sandbox.Pid = createResp.Pid + sandbox.NetNS = getNetworkNamespace(createResp.Pid) if !config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostNetwork() { // Setup network for sandbox. // TODO(random-liu): [P2] Replace with permanent network namespace. podName := config.GetMetadata().GetName() - if err = c.netPlugin.SetUpPod(meta.NetNS, config.GetMetadata().GetNamespace(), podName, id); err != nil { + if err = c.netPlugin.SetUpPod(sandbox.NetNS, config.GetMetadata().GetNamespace(), podName, id); err != nil { return nil, fmt.Errorf("failed to setup network for sandbox %q: %v", id, err) } defer func() { if retErr != nil { // Teardown network if an error is returned. - if err := c.netPlugin.TearDownPod(meta.NetNS, config.GetMetadata().GetNamespace(), podName, id); err != nil { + if err := c.netPlugin.TearDownPod(sandbox.NetNS, config.GetMetadata().GetNamespace(), podName, id); err != nil { glog.Errorf("failed to destroy network for sandbox %q: %v", id, err) } } @@ -231,10 +232,9 @@ func (c *criContainerdService) RunPodSandbox(ctx context.Context, r *runtime.Run } // Add sandbox into sandbox store. - meta.CreatedAt = time.Now().UnixNano() - if err := c.sandboxStore.Create(meta); err != nil { - return nil, fmt.Errorf("failed to add sandbox metadata %+v into store: %v", - meta, err) + sandbox.CreatedAt = time.Now().UnixNano() + if err := c.sandboxStore.Add(sandbox); err != nil { + return nil, fmt.Errorf("failed to add sandbox %+v into store: %v", sandbox, err) } return &runtime.RunPodSandboxResponse{PodSandboxId: id}, nil diff --git a/pkg/server/sandbox_run_test.go b/pkg/server/sandbox_run_test.go index 8417ff55d..675a9d8f0 100644 --- a/pkg/server/sandbox_run_test.go +++ b/pkg/server/sandbox_run_test.go @@ -33,9 +33,9 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" ) func getRunPodSandboxTestData() (*runtime.PodSandboxConfig, *imagespec.ImageConfig, func(*testing.T, string, *runtimespec.Spec)) { @@ -290,13 +290,13 @@ func TestRunPodSandbox(t *testing.T) { return nopReadWriteCloser{}, nil } testChainID := "test-sandbox-chain-id" - imageMetadata := metadata.ImageMetadata{ + image := imagestore.Image{ ID: testSandboxImage, ChainID: testChainID, Config: imageConfig, } - // Insert sandbox image metadata. - assert.NoError(t, c.imageMetadataStore.Create(imageMetadata)) + // Insert sandbox image. + c.imageStore.Add(image) expectContainersClientCalls := []string{"create"} expectSnapshotClientCalls := []string{"view"} expectExecutionClientCalls := []string{"create", "start"} @@ -349,25 +349,25 @@ func TestRunPodSandbox(t *testing.T) { startID := calls[1].Argument.(*execution.StartRequest).ContainerID assert.Equal(t, id, startID, "start id should be correct") - meta, err := c.sandboxStore.Get(id) + sandbox, err := c.sandboxStore.Get(id) assert.NoError(t, err) - assert.Equal(t, id, meta.ID, "metadata id should be correct") - err = c.sandboxNameIndex.Reserve(meta.Name, "random-id") - assert.Error(t, err, "metadata name should be reserved") - assert.Equal(t, config, meta.Config, "metadata config should be correct") + assert.Equal(t, id, sandbox.ID, "sandbox id should be correct") + err = c.sandboxNameIndex.Reserve(sandbox.Name, "random-id") + assert.Error(t, err, "sandbox name should be reserved") + assert.Equal(t, config, sandbox.Config, "sandbox config should be correct") // TODO(random-liu): [P2] Add clock interface and use fake clock. - assert.NotZero(t, meta.CreatedAt, "metadata CreatedAt should be set") + assert.NotZero(t, sandbox.CreatedAt, "sandbox CreatedAt should be set") info, err := fakeExecutionClient.Info(context.Background(), &execution.InfoRequest{ContainerID: id}) assert.NoError(t, err) pid := info.Task.Pid - assert.Equal(t, meta.NetNS, getNetworkNamespace(pid), "metadata network namespace should be correct") + assert.Equal(t, sandbox.NetNS, getNetworkNamespace(pid), "sandbox network namespace should be correct") expectedCNICalls := []string{"SetUpPod"} assert.Equal(t, expectedCNICalls, fakeCNIPlugin.GetCalledNames(), "expect SetUpPod should be called") calls = fakeCNIPlugin.GetCalledDetails() pluginArgument := calls[0].Argument.(servertesting.CNIPluginArgument) expectedPluginArgument := servertesting.CNIPluginArgument{ - NetnsPath: meta.NetNS, + NetnsPath: sandbox.NetNS, Namespace: config.GetMetadata().GetNamespace(), Name: config.GetMetadata().GetName(), ContainerID: id, diff --git a/pkg/server/sandbox_status.go b/pkg/server/sandbox_status.go index 74a1ea743..b9364629d 100644 --- a/pkg/server/sandbox_status.go +++ b/pkg/server/sandbox_status.go @@ -27,7 +27,7 @@ import ( "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) // PodSandboxStatus returns the status of the PodSandbox. @@ -66,11 +66,11 @@ func (c *criContainerdService) PodSandboxStatus(ctx context.Context, r *runtime. glog.V(4).Infof("GetContainerNetworkStatus returns error: %v", err) } - return &runtime.PodSandboxStatusResponse{Status: toCRISandboxStatus(sandbox, state, ip)}, nil + return &runtime.PodSandboxStatusResponse{Status: toCRISandboxStatus(sandbox.Metadata, state, ip)}, nil } // toCRISandboxStatus converts sandbox metadata into CRI pod sandbox status. -func toCRISandboxStatus(meta *metadata.SandboxMetadata, state runtime.PodSandboxState, ip string) *runtime.PodSandboxStatus { +func toCRISandboxStatus(meta sandboxstore.Metadata, state runtime.PodSandboxState, ip string) *runtime.PodSandboxStatus { nsOpts := meta.Config.GetLinux().GetSecurityContext().GetNamespaceOptions() return &runtime.PodSandboxStatus{ Id: meta.ID, diff --git a/pkg/server/sandbox_status_test.go b/pkg/server/sandbox_status_test.go index 724eca828..de79d89e8 100644 --- a/pkg/server/sandbox_status_test.go +++ b/pkg/server/sandbox_status_test.go @@ -21,16 +21,14 @@ import ( "testing" "time" + "github.com/containerd/containerd/api/types/task" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" - - "github.com/containerd/containerd/api/types/task" - "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) // Variables used in the following test. @@ -41,7 +39,7 @@ const ( sandboxStatusTestNetNS = "test-netns" ) -func getSandboxStatusTestData() (*metadata.SandboxMetadata, *runtime.PodSandboxStatus) { +func getSandboxStatusTestData() (*sandboxstore.Sandbox, *runtime.PodSandboxStatus) { config := &runtime.PodSandboxConfig{ Metadata: &runtime.PodSandboxMetadata{ Name: "test-name", @@ -64,12 +62,14 @@ func getSandboxStatusTestData() (*metadata.SandboxMetadata, *runtime.PodSandboxS createdAt := time.Now().UnixNano() - metadata := &metadata.SandboxMetadata{ - ID: sandboxStatusTestID, - Name: "test-name", - Config: config, - CreatedAt: createdAt, - NetNS: sandboxStatusTestNetNS, + sandbox := &sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: sandboxStatusTestID, + Name: "test-name", + Config: config, + CreatedAt: createdAt, + NetNS: sandboxStatusTestNetNS, + }, } expectedStatus := &runtime.PodSandboxStatus{ @@ -90,13 +90,13 @@ func getSandboxStatusTestData() (*metadata.SandboxMetadata, *runtime.PodSandboxS Annotations: config.GetAnnotations(), } - return metadata, expectedStatus + return sandbox, expectedStatus } func TestPodSandboxStatus(t *testing.T) { for desc, test := range map[string]struct { sandboxTasks []task.Task - injectMetadata bool + injectSandbox bool injectErr error injectIP bool injectCNIErr error @@ -106,7 +106,7 @@ func TestPodSandboxStatus(t *testing.T) { expectedCNICalls []string }{ "sandbox status without metadata": { - injectMetadata: false, + injectSandbox: false, expectErr: true, expectCalls: []string{}, expectedCNICalls: []string{}, @@ -117,7 +117,7 @@ func TestPodSandboxStatus(t *testing.T) { Pid: 1, Status: task.StatusRunning, }}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_READY, expectCalls: []string{"info"}, expectedCNICalls: []string{"GetContainerNetworkStatus"}, @@ -128,14 +128,14 @@ func TestPodSandboxStatus(t *testing.T) { Pid: 1, Status: task.StatusStopped, }}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_NOTREADY, expectCalls: []string{"info"}, expectedCNICalls: []string{"GetContainerNetworkStatus"}, }, "sandbox status with non-existing sandbox container": { sandboxTasks: []task.Task{}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_NOTREADY, expectCalls: []string{"info"}, expectedCNICalls: []string{"GetContainerNetworkStatus"}, @@ -146,7 +146,7 @@ func TestPodSandboxStatus(t *testing.T) { Pid: 1, Status: task.StatusRunning, }}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_READY, injectErr: errors.New("arbitrary error"), expectErr: true, @@ -159,7 +159,7 @@ func TestPodSandboxStatus(t *testing.T) { Pid: 1, Status: task.StatusRunning, }}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_READY, expectCalls: []string{"info"}, injectIP: true, @@ -171,7 +171,7 @@ func TestPodSandboxStatus(t *testing.T) { Pid: 1, Status: task.StatusRunning, }}, - injectMetadata: true, + injectSandbox: true, expectState: runtime.PodSandboxState_SANDBOX_READY, expectCalls: []string{"info"}, expectedCNICalls: []string{"GetContainerNetworkStatus"}, @@ -179,14 +179,14 @@ func TestPodSandboxStatus(t *testing.T) { }, } { t.Logf("TestCase %q", desc) - metadata, expect := getSandboxStatusTestData() + sandbox, expect := getSandboxStatusTestData() expect.Network.Ip = "" c := newTestCRIContainerdService() fake := c.taskService.(*servertesting.FakeExecutionClient) fakeCNIPlugin := c.netPlugin.(*servertesting.FakeCNIPlugin) fake.SetFakeTasks(test.sandboxTasks) - if test.injectMetadata { - assert.NoError(t, c.sandboxStore.Create(*metadata)) + if test.injectSandbox { + assert.NoError(t, c.sandboxStore.Add(*sandbox)) } if test.injectErr != nil { fake.InjectError("info", test.injectErr) @@ -195,8 +195,8 @@ func TestPodSandboxStatus(t *testing.T) { fakeCNIPlugin.InjectError("GetContainerNetworkStatus", test.injectCNIErr) } if test.injectIP { - fakeCNIPlugin.SetFakePodNetwork(metadata.NetNS, metadata.Config.GetMetadata().GetNamespace(), - metadata.Config.GetMetadata().GetName(), sandboxStatusTestID, sandboxStatusTestIP) + fakeCNIPlugin.SetFakePodNetwork(sandbox.NetNS, sandbox.Config.GetMetadata().GetNamespace(), + sandbox.Config.GetMetadata().GetName(), sandboxStatusTestID, sandboxStatusTestIP) expect.Network.Ip = sandboxStatusTestIP } res, err := c.PodSandboxStatus(context.Background(), &runtime.PodSandboxStatusRequest{ diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index ca8062f10..4b9157dcd 100644 --- a/pkg/server/sandbox_stop.go +++ b/pkg/server/sandbox_stop.go @@ -50,10 +50,7 @@ func (c *criContainerdService) StopPodSandbox(ctx context.Context, r *runtime.St // and container may still be so production should not rely on this behavior. // TODO(random-liu): Delete the sandbox container before this after permanent network namespace // is introduced, so that no container will be started after that. - containers, err := c.containerStore.List() - if err != nil { - return nil, fmt.Errorf("failed to list all containers: %v", err) - } + containers := c.containerStore.List() for _, container := range containers { if container.SandboxID != id { continue diff --git a/pkg/server/sandbox_stop_test.go b/pkg/server/sandbox_stop_test.go index 21ebc73ee..ac314ee1c 100644 --- a/pkg/server/sandbox_stop_test.go +++ b/pkg/server/sandbox_stop_test.go @@ -30,23 +30,26 @@ import ( "google.golang.org/grpc/codes" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) func TestStopPodSandbox(t *testing.T) { testID := "test-id" - testSandbox := metadata.SandboxMetadata{ - ID: testID, - Name: "test-name", - Config: &runtime.PodSandboxConfig{ - Metadata: &runtime.PodSandboxMetadata{ - Name: "test-name", - Uid: "test-uid", - Namespace: "test-ns", - }}, - NetNS: "test-netns", + testSandbox := sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testID, + Name: "test-name", + Config: &runtime.PodSandboxConfig{ + Metadata: &runtime.PodSandboxMetadata{ + Name: "test-name", + Uid: "test-uid", + Namespace: "test-ns", + }}, + NetNS: "test-netns", + }, } testContainer := task.Task{ ID: testID, @@ -136,7 +139,7 @@ func TestStopPodSandbox(t *testing.T) { fake.SetFakeTasks(test.sandboxTasks) if test.injectSandbox { - assert.NoError(t, c.sandboxStore.Create(testSandbox)) + assert.NoError(t, c.sandboxStore.Add(testSandbox)) } if test.injectErr != nil { fake.InjectError("delete", test.injectErr) @@ -170,38 +173,53 @@ func TestStopPodSandbox(t *testing.T) { func TestStopContainersInSandbox(t *testing.T) { testID := "test-id" - testSandbox := metadata.SandboxMetadata{ - ID: testID, - Name: "test-name", - Config: &runtime.PodSandboxConfig{ - Metadata: &runtime.PodSandboxMetadata{ - Name: "test-name", - Uid: "test-uid", - Namespace: "test-ns", - }}, - NetNS: "test-netns", + testSandbox := sandboxstore.Sandbox{ + Metadata: sandboxstore.Metadata{ + ID: testID, + Name: "test-name", + Config: &runtime.PodSandboxConfig{ + Metadata: &runtime.PodSandboxMetadata{ + Name: "test-name", + Uid: "test-uid", + Namespace: "test-ns", + }}, + NetNS: "test-netns", + }, } - testContainers := []metadata.ContainerMetadata{ + testContainers := []containerForTest{ { - ID: "test-cid-1", - Name: "test-cname-1", - SandboxID: testID, - Pid: 2, - StartedAt: time.Now().UnixNano(), + metadata: containerstore.Metadata{ + ID: "test-cid-1", + Name: "test-cname-1", + SandboxID: testID, + }, + status: containerstore.Status{ + Pid: 2, + StartedAt: time.Now().UnixNano(), + }, }, { - ID: "test-cid-2", - Name: "test-cname-2", - SandboxID: testID, - Pid: 3, - StartedAt: time.Now().UnixNano(), + + metadata: containerstore.Metadata{ + ID: "test-cid-2", + Name: "test-cname-2", + SandboxID: testID, + }, + status: containerstore.Status{ + Pid: 3, + StartedAt: time.Now().UnixNano(), + }, }, { - ID: "test-cid-3", - Name: "test-cname-3", - SandboxID: "other-sandbox-id", - Pid: 4, - StartedAt: time.Now().UnixNano(), + metadata: containerstore.Metadata{ + ID: "test-cid-3", + Name: "test-cname-3", + SandboxID: "other-sandbox-id", + }, + status: containerstore.Status{ + Pid: 4, + StartedAt: time.Now().UnixNano(), + }, }, } testContainerdContainers := []task.Task{ @@ -232,9 +250,11 @@ func TestStopContainersInSandbox(t *testing.T) { defer fake.Stop() c.taskService = fake fake.SetFakeTasks(testContainerdContainers) - assert.NoError(t, c.sandboxStore.Create(testSandbox)) - for _, cntr := range testContainers { - assert.NoError(t, c.containerStore.Create(cntr)) + c.sandboxStore.Add(testSandbox) + for _, tc := range testContainers { + cntr, err := tc.toContainer() + assert.NoError(t, err) + assert.NoError(t, c.containerStore.Add(cntr)) } fakeCNIPlugin := c.netPlugin.(*servertesting.FakeCNIPlugin) @@ -257,8 +277,7 @@ func TestStopContainersInSandbox(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, res) - cntrs, err := c.containerStore.List() - assert.NoError(t, err) + cntrs := c.containerStore.List() assert.Len(t, cntrs, 3) expectedStates := map[string]runtime.ContainerState{ "test-cid-1": runtime.ContainerState_CONTAINER_EXITED, @@ -268,6 +287,6 @@ func TestStopContainersInSandbox(t *testing.T) { for id, expected := range expectedStates { cntr, err := c.containerStore.Get(id) assert.NoError(t, err) - assert.Equal(t, expected, cntr.State()) + assert.Equal(t, expected, cntr.Status.Get().State()) } } diff --git a/pkg/server/service.go b/pkg/server/service.go index 5b92ced0e..eb189bf3c 100644 --- a/pkg/server/service.go +++ b/pkg/server/service.go @@ -31,11 +31,12 @@ import ( healthapi "google.golang.org/grpc/health/grpc_health_v1" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata/store" osinterface "github.com/kubernetes-incubator/cri-containerd/pkg/os" "github.com/kubernetes-incubator/cri-containerd/pkg/registrar" "github.com/kubernetes-incubator/cri-containerd/pkg/server/agents" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) // k8sContainerdNamespace is the namespace we use to connect containerd. @@ -57,19 +58,19 @@ type criContainerdService struct { // sandboxImage is the image to use for sandbox container. // TODO(random-liu): Make this configurable via flag. sandboxImage string - // sandboxStore stores all sandbox metadata. - sandboxStore metadata.SandboxStore - // imageMetadataStore stores all image metadata. - imageMetadataStore metadata.ImageMetadataStore + // sandboxStore stores all resources associated with sandboxes. + sandboxStore *sandboxstore.Store // sandboxNameIndex stores all sandbox names and make sure each name // is unique. sandboxNameIndex *registrar.Registrar - // containerStore stores all container metadata. - containerStore metadata.ContainerStore + // containerStore stores all resources associated with containers. + containerStore *containerstore.Store // containerNameIndex stores all container names and make sure each // name is unique. containerNameIndex *registrar.Registrar - // containerService is containerd tasks client. + // imageStore stores all resources associated with images. + imageStore *imagestore.Store + // containerService is containerd containers client. containerService containers.ContainersClient // taskService is containerd tasks client. taskService execution.TasksClient @@ -96,7 +97,7 @@ type criContainerdService struct { // NewCRIContainerdService returns a new instance of CRIContainerdService func NewCRIContainerdService(containerdEndpoint, rootDir, networkPluginBinDir, networkPluginConfDir string) (CRIContainerdService, error) { - // TODO(random-liu): [P2] Recover from runtime state and metadata store. + // TODO(random-liu): [P2] Recover from runtime state and checkpoint. client, err := containerd.New(containerdEndpoint, containerd.WithDefaultNamespace(k8sContainerdNamespace)) if err != nil { @@ -107,9 +108,9 @@ func NewCRIContainerdService(containerdEndpoint, rootDir, networkPluginBinDir, n os: osinterface.RealOS{}, rootDir: rootDir, sandboxImage: defaultSandboxImage, - sandboxStore: metadata.NewSandboxStore(store.NewMetadataStore()), - containerStore: metadata.NewContainerStore(store.NewMetadataStore()), - imageMetadataStore: metadata.NewImageMetadataStore(store.NewMetadataStore()), + sandboxStore: sandboxstore.NewStore(), + containerStore: containerstore.NewStore(), + imageStore: imagestore.NewStore(), sandboxNameIndex: registrar.NewRegistrar(), containerNameIndex: registrar.NewRegistrar(), containerService: client.ContainerService(), diff --git a/pkg/server/service_test.go b/pkg/server/service_test.go index 4c15bfd24..3a5734aec 100644 --- a/pkg/server/service_test.go +++ b/pkg/server/service_test.go @@ -29,12 +29,13 @@ import ( "golang.org/x/net/context" "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata" - "github.com/kubernetes-incubator/cri-containerd/pkg/metadata/store" ostesting "github.com/kubernetes-incubator/cri-containerd/pkg/os/testing" "github.com/kubernetes-incubator/cri-containerd/pkg/registrar" agentstesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/agents/testing" servertesting "github.com/kubernetes-incubator/cri-containerd/pkg/server/testing" + containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container" + imagestore "github.com/kubernetes-incubator/cri-containerd/pkg/store/image" + sandboxstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/sandbox" ) type nopReadWriteCloser struct{} @@ -58,10 +59,10 @@ func newTestCRIContainerdService() *criContainerdService { os: ostesting.NewFakeOS(), rootDir: testRootDir, sandboxImage: testSandboxImage, - sandboxStore: metadata.NewSandboxStore(store.NewMetadataStore()), - imageMetadataStore: metadata.NewImageMetadataStore(store.NewMetadataStore()), + sandboxStore: sandboxstore.NewStore(), + imageStore: imagestore.NewStore(), sandboxNameIndex: registrar.NewRegistrar(), - containerStore: metadata.NewContainerStore(store.NewMetadataStore()), + containerStore: containerstore.NewStore(), containerNameIndex: registrar.NewRegistrar(), taskService: servertesting.NewFakeExecutionClient(), containerService: servertesting.NewFakeContainersClient(), @@ -88,11 +89,11 @@ func TestSandboxOperations(t *testing.T) { return nopReadWriteCloser{}, nil } // Insert sandbox image metadata. - assert.NoError(t, c.imageMetadataStore.Create(metadata.ImageMetadata{ + c.imageStore.Add(imagestore.Image{ ID: testSandboxImage, ChainID: "test-chain-id", Config: &imagespec.ImageConfig{Entrypoint: []string{"/pause"}}, - })) + }) config := &runtime.PodSandboxConfig{ Metadata: &runtime.PodSandboxMetadata{