From 616f26cfe55ff73dd9724da71f756497dd1faafb Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Mon, 8 Apr 2019 23:36:59 -0700 Subject: [PATCH 1/3] types: split sandbox and container state Since they do not really share many of the fields. Fixes: #1434 Signed-off-by: Peng Tao --- cli/delete.go | 4 ++-- cli/delete_test.go | 18 +++++++------- cli/events_test.go | 2 +- cli/exec_test.go | 22 ++++++++--------- cli/kill_test.go | 18 +++++++------- cli/main_test.go | 2 +- cli/network_test.go | 2 +- cli/pause_test.go | 8 +++---- cli/ps_test.go | 2 +- cli/update_test.go | 4 ++-- containerd-shim-v2/pause_test.go | 8 +++---- containerd-shim-v2/utils.go | 2 +- virtcontainers/api_test.go | 12 +++++----- virtcontainers/cgroups_test.go | 2 +- virtcontainers/container.go | 8 +++---- virtcontainers/container_test.go | 12 +++++----- virtcontainers/kata_agent_test.go | 2 +- virtcontainers/pkg/oci/utils.go | 6 ++--- virtcontainers/pkg/oci/utils_test.go | 16 ++++++------- virtcontainers/sandbox.go | 6 ++--- virtcontainers/sandbox_test.go | 34 +++++++++++++------------- virtcontainers/store/vc.go | 19 +++++++++++---- virtcontainers/types/container.go | 33 +++++++++++++++++++++++++ virtcontainers/types/sandbox.go | 36 +++++++++++++++------------- 24 files changed, 163 insertions(+), 115 deletions(-) create mode 100644 virtcontainers/types/container.go diff --git a/cli/delete.go b/cli/delete.go index 200fbe869e..19dc4fb7a2 100644 --- a/cli/delete.go +++ b/cli/delete.go @@ -102,7 +102,7 @@ func delete(ctx context.Context, containerID string, force bool) error { } forceStop := false - if oci.StateToOCIState(status.State) == oci.StateRunning { + if oci.StateToOCIState(status.State.State) == oci.StateRunning { if !force { return fmt.Errorf("Container still running, should be stopped") } @@ -140,7 +140,7 @@ func deleteSandbox(ctx context.Context, sandboxID string) error { return err } - if oci.StateToOCIState(status.State) != oci.StateStopped { + if oci.StateToOCIState(status.State.State) != oci.StateStopped { if _, err := vci.StopSandbox(ctx, sandboxID); err != nil { return err } diff --git a/cli/delete_test.go b/cli/delete_test.go index 51fc553bb9..8590e4bd64 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -168,7 +168,7 @@ func TestDeleteSandbox(t *testing.T) { vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "ready", }, }, nil @@ -185,7 +185,7 @@ func TestDeleteSandbox(t *testing.T) { testingImpl.StatusSandboxFunc = func(ctx context.Context, sandboxID string) (vc.SandboxStatus, error) { return vc.SandboxStatus{ ID: sandbox.ID(), - State: types.State{ + State: types.SandboxState{ State: types.StateReady, }, }, nil @@ -246,7 +246,7 @@ func TestDeleteInvalidContainerType(t *testing.T) { vcAnnotations.ContainerTypeKey: "InvalidType", vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "created", }, }, nil @@ -285,7 +285,7 @@ func TestDeleteSandboxRunning(t *testing.T) { vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "running", }, }, nil @@ -303,7 +303,7 @@ func TestDeleteSandboxRunning(t *testing.T) { testingImpl.StatusSandboxFunc = func(ctx context.Context, sandboxID string) (vc.SandboxStatus, error) { return vc.SandboxStatus{ ID: sandbox.ID(), - State: types.State{ + State: types.SandboxState{ State: types.StateRunning, }, }, nil @@ -365,7 +365,7 @@ func TestDeleteRunningContainer(t *testing.T) { vcAnnotations.ContainerTypeKey: string(vc.PodContainer), vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "running", }, }, nil @@ -448,7 +448,7 @@ func TestDeleteContainer(t *testing.T) { vcAnnotations.ContainerTypeKey: string(vc.PodContainer), vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "ready", }, }, nil @@ -548,7 +548,7 @@ func TestDeleteCLIFunctionSuccess(t *testing.T) { vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), vcAnnotations.ConfigJSONKey: configJSON, }, - State: types.State{ + State: types.ContainerState{ State: "ready", }, }, nil @@ -557,7 +557,7 @@ func TestDeleteCLIFunctionSuccess(t *testing.T) { testingImpl.StatusSandboxFunc = func(ctx context.Context, sandboxID string) (vc.SandboxStatus, error) { return vc.SandboxStatus{ ID: sandbox.ID(), - State: types.State{ + State: types.SandboxState{ State: types.StateReady, }, }, nil diff --git a/cli/events_test.go b/cli/events_test.go index b71ca1b3f8..f54aaa0afd 100644 --- a/cli/events_test.go +++ b/cli/events_test.go @@ -112,7 +112,7 @@ func TestEventsCLISuccessful(t *testing.T) { Annotations: map[string]string{ vcAnnotations.ContainerTypeKey: string(vc.PodContainer), }, - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil diff --git a/cli/exec_test.go b/cli/exec_test.go index 30edaf4cbb..a916f66b00 100644 --- a/cli/exec_test.go +++ b/cli/exec_test.go @@ -78,7 +78,7 @@ func TestExecuteErrors(t *testing.T) { } testingImpl.StatusContainerFunc = func(ctx context.Context, sandboxID, containerID string) (vc.ContainerStatus, error) { - return newSingleContainerStatus(testContainerID, types.State{}, annotations), nil + return newSingleContainerStatus(testContainerID, types.ContainerState{}, annotations), nil } defer func() { @@ -100,7 +100,7 @@ func TestExecuteErrors(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - containerState := types.State{} + containerState := types.ContainerState{} testingImpl.StatusContainerFunc = func(ctx context.Context, sandboxID, containerID string) (vc.ContainerStatus, error) { return newSingleContainerStatus(testContainerID, containerState, annotations), nil } @@ -110,7 +110,7 @@ func TestExecuteErrors(t *testing.T) { assert.False(vcmock.IsMockError(err)) // Container paused - containerState = types.State{ + containerState = types.ContainerState{ State: types.StatePaused, } testingImpl.StatusContainerFunc = func(ctx context.Context, sandboxID, containerID string) (vc.ContainerStatus, error) { @@ -122,7 +122,7 @@ func TestExecuteErrors(t *testing.T) { assert.False(vcmock.IsMockError(err)) // Container stopped - containerState = types.State{ + containerState = types.ContainerState{ State: types.StateStopped, } testingImpl.StatusContainerFunc = func(ctx context.Context, sandboxID, containerID string) (vc.ContainerStatus, error) { @@ -159,7 +159,7 @@ func TestExecuteErrorReadingProcessJson(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -208,7 +208,7 @@ func TestExecuteErrorOpeningConsole(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -275,7 +275,7 @@ func TestExecuteWithFlags(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -365,7 +365,7 @@ func TestExecuteWithFlagsDetached(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -444,7 +444,7 @@ func TestExecuteWithInvalidProcessJson(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -496,7 +496,7 @@ func TestExecuteWithValidProcessJson(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -597,7 +597,7 @@ func TestExecuteWithEmptyEnvironmentValue(t *testing.T) { vcAnnotations.ConfigJSONKey: configJSON, } - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } diff --git a/cli/kill_test.go b/cli/kill_test.go index 45ba4225b2..cd8910cf99 100644 --- a/cli/kill_test.go +++ b/cli/kill_test.go @@ -65,7 +65,7 @@ func TestProcessSignal(t *testing.T) { func testKillCLIFunctionTerminationSignalSuccessful(t *testing.T, sig string) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -123,7 +123,7 @@ func TestKillCLIFunctionSigtermSuccessful(t *testing.T) { func TestKillCLIFunctionNotTerminationSignalSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -151,7 +151,7 @@ func TestKillCLIFunctionNotTerminationSignalSuccessful(t *testing.T) { func TestKillCLIFunctionNoSignalSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -201,7 +201,7 @@ func TestKillCLIFunctionNoSignalSuccessful(t *testing.T) { func TestKillCLIFunctionEnableAllSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -289,7 +289,7 @@ func TestKillCLIFunctionContainerNotExistFailure(t *testing.T) { func TestKillCLIFunctionInvalidSignalFailure(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -317,7 +317,7 @@ func TestKillCLIFunctionInvalidSignalFailure(t *testing.T) { func TestKillCLIFunctionStatePausedSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StatePaused, } @@ -348,7 +348,7 @@ func TestKillCLIFunctionStatePausedSuccessful(t *testing.T) { func TestKillCLIFunctionInvalidStateStoppedFailure(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateStopped, } @@ -376,7 +376,7 @@ func TestKillCLIFunctionInvalidStateStoppedFailure(t *testing.T) { func TestKillCLIFunctionKillContainerFailure(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -401,7 +401,7 @@ func TestKillCLIFunctionKillContainerFailure(t *testing.T) { func TestKillCLIFunctionInvalidStateStoppedAllSuccess(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateStopped, } diff --git a/cli/main_test.go b/cli/main_test.go index f9a65fce81..0f3743af34 100644 --- a/cli/main_test.go +++ b/cli/main_test.go @@ -445,7 +445,7 @@ func writeOCIConfigFile(spec oci.CompatOCISpec, configPath string) error { return ioutil.WriteFile(configPath, bytes, testFileMode) } -func newSingleContainerStatus(containerID string, containerState types.State, annotations map[string]string) vc.ContainerStatus { +func newSingleContainerStatus(containerID string, containerState types.ContainerState, annotations map[string]string) vc.ContainerStatus { return vc.ContainerStatus{ ID: containerID, State: containerState, diff --git a/cli/network_test.go b/cli/network_test.go index a4d408d6dd..f6708eb07d 100644 --- a/cli/network_test.go +++ b/cli/network_test.go @@ -39,7 +39,7 @@ var ( func TestNetworkCliFunction(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } diff --git a/cli/pause_test.go b/cli/pause_test.go index 2fa1df4be7..07d3c60b40 100644 --- a/cli/pause_test.go +++ b/cli/pause_test.go @@ -30,7 +30,7 @@ var ( func TestPauseCLIFunctionSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -78,7 +78,7 @@ func TestPauseCLIFunctionContainerNotExistFailure(t *testing.T) { func TestPauseCLIFunctionPauseContainerFailure(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -103,7 +103,7 @@ func TestPauseCLIFunctionPauseContainerFailure(t *testing.T) { func TestResumeCLIFunctionSuccessful(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -150,7 +150,7 @@ func TestResumeCLIFunctionContainerNotExistFailure(t *testing.T) { func TestResumeCLIFunctionPauseContainerFailure(t *testing.T) { assert := assert.New(t) - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } diff --git a/cli/ps_test.go b/cli/ps_test.go index 5a5728533c..c214793f25 100644 --- a/cli/ps_test.go +++ b/cli/ps_test.go @@ -96,7 +96,7 @@ func TestPSSuccessful(t *testing.T) { testingImpl.StatusContainerFunc = func(ctx context.Context, sandboxID, containerID string) (vc.ContainerStatus, error) { return vc.ContainerStatus{ - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, ID: sandbox.ID(), diff --git a/cli/update_test.go b/cli/update_test.go index c1d7cf3ca1..089d421715 100644 --- a/cli/update_test.go +++ b/cli/update_test.go @@ -95,7 +95,7 @@ func TestUpdateCLIFailure(t *testing.T) { Annotations: map[string]string{ vcAnnotations.ContainerTypeKey: string(vc.PodContainer), }, - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil @@ -168,7 +168,7 @@ func TestUpdateCLISuccessful(t *testing.T) { Annotations: map[string]string{ vcAnnotations.ContainerTypeKey: string(vc.PodContainer), }, - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil diff --git a/containerd-shim-v2/pause_test.go b/containerd-shim-v2/pause_test.go index 212e64e8cd..ff15ab99a2 100644 --- a/containerd-shim-v2/pause_test.go +++ b/containerd-shim-v2/pause_test.go @@ -39,7 +39,7 @@ func TestPauseContainerSuccess(t *testing.T) { return vc.ContainerStatus{ ID: testContainerID, Annotations: make(map[string]string), - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil @@ -87,7 +87,7 @@ func TestPauseContainerFail(t *testing.T) { return vc.ContainerStatus{ ID: testContainerID, Annotations: make(map[string]string), - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil @@ -130,7 +130,7 @@ func TestResumeContainerSuccess(t *testing.T) { return vc.ContainerStatus{ ID: testContainerID, Annotations: make(map[string]string), - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil @@ -178,7 +178,7 @@ func TestResumeContainerFail(t *testing.T) { return vc.ContainerStatus{ ID: testContainerID, Annotations: make(map[string]string), - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, }, }, nil diff --git a/containerd-shim-v2/utils.go b/containerd-shim-v2/utils.go index 812e9f6ca9..b704b17454 100644 --- a/containerd-shim-v2/utils.go +++ b/containerd-shim-v2/utils.go @@ -48,7 +48,7 @@ func cleanupContainer(ctx context.Context, sid, cid, bundlePath string) error { return err } - if oci.StateToOCIState(status.State) != oci.StateStopped { + if oci.StateToOCIState(status.State.State) != oci.StateStopped { err := sandbox.KillContainer(cid, syscall.SIGKILL, true) if err != nil { logrus.WithError(err).WithField("container", cid).Warn("failed to kill container") diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index 5ba48eeb71..1f376a4fcc 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -877,7 +877,7 @@ func TestStatusSandboxSuccessfulStateReady(t *testing.T) { expectedStatus := SandboxStatus{ ID: testSandboxID, - State: types.State{ + State: types.SandboxState{ State: types.StateReady, }, Hypervisor: MockHypervisor, @@ -887,7 +887,7 @@ func TestStatusSandboxSuccessfulStateReady(t *testing.T) { ContainersStatus: []ContainerStatus{ { ID: containerID, - State: types.State{ + State: types.ContainerState{ State: types.StateReady, CgroupPath: utils.DefaultCgroupPath, }, @@ -936,7 +936,7 @@ func TestStatusSandboxSuccessfulStateRunning(t *testing.T) { expectedStatus := SandboxStatus{ ID: testSandboxID, - State: types.State{ + State: types.SandboxState{ State: types.StateRunning, }, Hypervisor: MockHypervisor, @@ -946,7 +946,7 @@ func TestStatusSandboxSuccessfulStateRunning(t *testing.T) { ContainersStatus: []ContainerStatus{ { ID: containerID, - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, CgroupPath: utils.DefaultCgroupPath, }, @@ -1770,7 +1770,7 @@ func TestStatusContainerStateReady(t *testing.T) { expectedStatus := ContainerStatus{ ID: contID, - State: types.State{ + State: types.ContainerState{ State: types.StateReady, CgroupPath: utils.DefaultCgroupPath, }, @@ -1846,7 +1846,7 @@ func TestStatusContainerStateRunning(t *testing.T) { expectedStatus := ContainerStatus{ ID: contID, - State: types.State{ + State: types.ContainerState{ State: types.StateRunning, CgroupPath: utils.DefaultCgroupPath, }, diff --git a/virtcontainers/cgroups_test.go b/virtcontainers/cgroups_test.go index 4e93c2dd43..a9a7c920c0 100644 --- a/virtcontainers/cgroups_test.go +++ b/virtcontainers/cgroups_test.go @@ -123,7 +123,7 @@ func TestUpdateCgroups(t *testing.T) { }() s := &Sandbox{ - state: types.State{ + state: types.SandboxState{ CgroupPath: "", }, } diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 0409c154cd..083186f037 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -70,7 +70,7 @@ type Process struct { // ContainerStatus describes a container status. type ContainerStatus struct { ID string - State types.State + State types.ContainerState PID int StartTime time.Time RootFs string @@ -292,7 +292,7 @@ type Container struct { containerPath string rootfsSuffix string - state types.State + state types.ContainerState process Process @@ -656,7 +656,7 @@ func newContainer(sandbox *Sandbox, contConfig ContainerConfig) (*Container, err configPath: store.ContainerConfigurationRootPath(sandbox.id, contConfig.ID), containerPath: filepath.Join(sandbox.id, contConfig.ID), rootfsSuffix: "rootfs", - state: types.State{}, + state: types.ContainerState{}, process: Process{}, mounts: contConfig.Mounts, ctx: sandbox.ctx, @@ -669,7 +669,7 @@ func newContainer(sandbox *Sandbox, contConfig ContainerConfig) (*Container, err c.store = ctrStore - state, err := c.store.LoadState() + state, err := c.store.LoadContainerState() if err == nil { c.state = state } diff --git a/virtcontainers/container_test.go b/virtcontainers/container_test.go index 8dfdafc384..6d2f2fc655 100644 --- a/virtcontainers/container_test.go +++ b/virtcontainers/container_test.go @@ -343,7 +343,7 @@ func TestCheckSandboxRunningNotRunningFailure(t *testing.T) { func TestCheckSandboxRunningSuccessful(t *testing.T) { c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, @@ -356,7 +356,7 @@ func TestContainerEnterErrorsOnContainerStates(t *testing.T) { assert := assert.New(t) c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, @@ -382,7 +382,7 @@ func TestContainerWaitErrorState(t *testing.T) { assert := assert.New(t) c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, @@ -408,7 +408,7 @@ func TestKillContainerErrorState(t *testing.T) { assert := assert.New(t) c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, @@ -427,7 +427,7 @@ func TestWinsizeProcessErrorState(t *testing.T) { assert := assert.New(t) c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, @@ -453,7 +453,7 @@ func TestProcessIOStream(t *testing.T) { assert := assert.New(t) c := &Container{ sandbox: &Sandbox{ - state: types.State{ + state: types.SandboxState{ State: types.StateRunning, }, }, diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 378c4fc657..788e090d0d 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -754,7 +754,7 @@ func TestAgentCreateContainer(t *testing.T) { id: "barfoo", sandboxID: "foobar", sandbox: sandbox, - state: types.State{ + state: types.ContainerState{ Fstype: "xfs", }, config: &ContainerConfig{ diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index a255d50c3a..dec81ce299 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -613,7 +613,7 @@ func StatusToOCIState(status vc.ContainerStatus) spec.State { return spec.State{ Version: spec.Version, ID: status.ID, - Status: StateToOCIState(status.State), + Status: StateToOCIState(status.State.State), Pid: status.PID, Bundle: status.Annotations[vcAnnotations.BundlePathKey], Annotations: status.Annotations, @@ -621,8 +621,8 @@ func StatusToOCIState(status vc.ContainerStatus) spec.State { } // StateToOCIState translates a virtcontainers container state into an OCI one. -func StateToOCIState(state types.State) string { - switch state.State { +func StateToOCIState(state types.StateString) string { + switch state { case types.StateReady: return StateCreated case types.StateRunning: diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index b29726625d..ea6de017a4 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -271,7 +271,7 @@ func TestStatusToOCIStateSuccessfulWithReadyState(t *testing.T) { testPID := 12345 testRootFs := "testRootFs" - state := types.State{ + state := types.ContainerState{ State: types.StateReady, } @@ -307,7 +307,7 @@ func TestStatusToOCIStateSuccessfulWithRunningState(t *testing.T) { testPID := 12345 testRootFs := "testRootFs" - state := types.State{ + state := types.ContainerState{ State: types.StateRunning, } @@ -342,7 +342,7 @@ func TestStatusToOCIStateSuccessfulWithStoppedState(t *testing.T) { testPID := 12345 testRootFs := "testRootFs" - state := types.State{ + state := types.ContainerState{ State: types.StateStopped, } @@ -403,28 +403,28 @@ func TestStatusToOCIStateSuccessfulWithNoState(t *testing.T) { } func TestStateToOCIState(t *testing.T) { - var state types.State + var state types.StateString if ociState := StateToOCIState(state); ociState != "" { t.Fatalf("Expecting \"created\" state, got \"%s\"", ociState) } - state.State = types.StateReady + state = types.StateReady if ociState := StateToOCIState(state); ociState != "created" { t.Fatalf("Expecting \"created\" state, got \"%s\"", ociState) } - state.State = types.StateRunning + state = types.StateRunning if ociState := StateToOCIState(state); ociState != "running" { t.Fatalf("Expecting \"created\" state, got \"%s\"", ociState) } - state.State = types.StateStopped + state = types.StateStopped if ociState := StateToOCIState(state); ociState != "stopped" { t.Fatalf("Expecting \"created\" state, got \"%s\"", ociState) } - state.State = types.StatePaused + state = types.StatePaused if ociState := StateToOCIState(state); ociState != "paused" { t.Fatalf("Expecting \"paused\" state, got \"%s\"", ociState) } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 58fe1f5add..d207fc018e 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -43,7 +43,7 @@ const ( // SandboxStatus describes a sandbox status. type SandboxStatus struct { ID string - State types.State + State types.SandboxState Hypervisor HypervisorType HypervisorConfig HypervisorConfig Agent AgentType @@ -173,7 +173,7 @@ type Sandbox struct { runPath string configPath string - state types.State + state types.SandboxState networkNS NetworkNamespace @@ -520,7 +520,7 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor containers: map[string]*Container{}, runPath: store.SandboxRuntimeRootPath(sandboxConfig.ID), configPath: store.SandboxConfigurationRootPath(sandboxConfig.ID), - state: types.State{}, + state: types.SandboxState{}, annotationsLock: &sync.RWMutex{}, wg: &sync.WaitGroup{}, shmSize: sandboxConfig.ShmSize, diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index 109b1609a7..4a2c960aaf 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -188,7 +188,7 @@ func testSandboxStateTransition(t *testing.T, state types.StateString, newState } defer cleanUp() - p.state = types.State{ + p.state = types.SandboxState{ State: state, } @@ -245,7 +245,7 @@ func TestSandboxStatePausedReady(t *testing.T) { } func testStateValid(t *testing.T, stateStr types.StateString, expected bool) { - state := &types.State{ + state := &types.SandboxState{ State: stateStr, } @@ -267,7 +267,7 @@ func TestStateValidFailing(t *testing.T) { } func TestValidTransitionFailingOldStateMismatch(t *testing.T) { - state := &types.State{ + state := &types.SandboxState{ State: types.StateReady, } @@ -450,7 +450,7 @@ func TestSandboxEnterSuccessful(t *testing.T) { } } -func testCheckInitSandboxAndContainerStates(p *Sandbox, initialSandboxState types.State, c *Container, initialContainerState types.State) error { +func testCheckInitSandboxAndContainerStates(p *Sandbox, initialSandboxState types.SandboxState, c *Container, initialContainerState types.ContainerState) error { if p.state.State != initialSandboxState.State { return fmt.Errorf("Expected sandbox state %v, got %v", initialSandboxState.State, p.state.State) } @@ -462,7 +462,7 @@ func testCheckInitSandboxAndContainerStates(p *Sandbox, initialSandboxState type return nil } -func testForceSandboxStateChangeAndCheck(t *testing.T, p *Sandbox, newSandboxState types.State) error { +func testForceSandboxStateChangeAndCheck(t *testing.T, p *Sandbox, newSandboxState types.SandboxState) error { // force sandbox state change if err := p.setSandboxState(newSandboxState.State); err != nil { t.Fatalf("Unexpected error: %v (sandbox %+v)", err, p) @@ -476,7 +476,7 @@ func testForceSandboxStateChangeAndCheck(t *testing.T, p *Sandbox, newSandboxSta return nil } -func testForceContainerStateChangeAndCheck(t *testing.T, p *Sandbox, c *Container, newContainerState types.State) error { +func testForceContainerStateChangeAndCheck(t *testing.T, p *Sandbox, c *Container, newContainerState types.ContainerState) error { // force container state change if err := c.setContainerState(newContainerState.State); err != nil { t.Fatalf("Unexpected error: %v (sandbox %+v)", err, p) @@ -490,7 +490,7 @@ func testForceContainerStateChangeAndCheck(t *testing.T, p *Sandbox, c *Containe return nil } -func testCheckSandboxOnDiskState(p *Sandbox, sandboxState types.State) error { +func testCheckSandboxOnDiskState(p *Sandbox, sandboxState types.SandboxState) error { // check on-disk state is correct if p.state.State != sandboxState.State { return fmt.Errorf("Expected state %v, got %v", sandboxState.State, p.state.State) @@ -499,7 +499,7 @@ func testCheckSandboxOnDiskState(p *Sandbox, sandboxState types.State) error { return nil } -func testCheckContainerOnDiskState(c *Container, containerState types.State) error { +func testCheckContainerOnDiskState(c *Container, containerState types.ContainerState) error { // check on-disk state is correct if c.state.State != containerState.State { return fmt.Errorf("Expected state %v, got %v", containerState.State, c.state.State) @@ -525,12 +525,12 @@ func TestSandboxSetSandboxAndContainerState(t *testing.T) { t.Fatalf("Expected 1 container found %v", l) } - initialSandboxState := types.State{ + initialSandboxState := types.SandboxState{ State: types.StateReady, } // After a sandbox creation, a container has a READY state - initialContainerState := types.State{ + initialContainerState := types.ContainerState{ State: types.StateReady, } @@ -550,7 +550,7 @@ func TestSandboxSetSandboxAndContainerState(t *testing.T) { t.Fatal(err) } - newSandboxState := types.State{ + newSandboxState := types.SandboxState{ State: types.StateRunning, } @@ -558,7 +558,7 @@ func TestSandboxSetSandboxAndContainerState(t *testing.T) { t.Error(err) } - newContainerState := types.State{ + newContainerState := types.ContainerState{ State: types.StateStopped, } @@ -792,7 +792,7 @@ func TestContainerSetStateBlockIndex(t *testing.T) { t.Fatal(err) } - state := types.State{ + state := types.ContainerState{ State: "stopped", Fstype: "vfs", } @@ -825,7 +825,7 @@ func TestContainerSetStateBlockIndex(t *testing.T) { t.Fatal() } - var res types.State + var res types.ContainerState err = json.Unmarshal([]byte(string(fileData)), &res) if err != nil { t.Fatal(err) @@ -888,7 +888,7 @@ func TestContainerStateSetFstype(t *testing.T) { t.Fatal(err) } - state := types.State{ + state := types.ContainerState{ State: "ready", Fstype: "vfs", BlockIndex: 3, @@ -923,7 +923,7 @@ func TestContainerStateSetFstype(t *testing.T) { t.Fatal() } - var res types.State + var res types.ContainerState err = json.Unmarshal([]byte(string(fileData)), &res) if err != nil { t.Fatal(err) @@ -1582,7 +1582,7 @@ func TestStartNetworkMonitor(t *testing.T) { func TestSandboxStopStopped(t *testing.T) { s := &Sandbox{ ctx: context.Background(), - state: types.State{State: types.StateStopped}, + state: types.SandboxState{State: types.StateStopped}, } err := s.Stop() diff --git a/virtcontainers/store/vc.go b/virtcontainers/store/vc.go index 6906428da0..eecaca2ea3 100644 --- a/virtcontainers/store/vc.go +++ b/virtcontainers/store/vc.go @@ -109,12 +109,23 @@ func (s *VCStore) Delete() error { return nil } -// LoadState loads an returns a virtcontainer state -func (s *VCStore) LoadState() (types.State, error) { - var state types.State +// LoadSandboxState loads an returns a virtcontainer state +func (s *VCStore) LoadState() (types.SandboxState, error) { + var state types.SandboxState if err := s.state.Load(State, &state); err != nil { - return types.State{}, err + return types.SandboxState{}, err + } + + return state, nil +} + +// LoadContainerState loads an returns a virtcontainer state +func (s *VCStore) LoadContainerState() (types.ContainerState, error) { + var state types.ContainerState + + if err := s.state.Load(State, &state); err != nil { + return types.ContainerState{}, err } return state, nil diff --git a/virtcontainers/types/container.go b/virtcontainers/types/container.go new file mode 100644 index 0000000000..7b3b5e2bc3 --- /dev/null +++ b/virtcontainers/types/container.go @@ -0,0 +1,33 @@ +// Copyright (c) 2019 hyper.sh +// +// SPDX-License-Identifier: Apache-2.0 +// + +package types + +// ContainerState is a sandbox state structure. +type ContainerState struct { + State StateString `json:"state"` + + BlockDeviceID string + // Index of the block device passed to hypervisor. + BlockIndex int `json:"blockIndex"` + + // File system of the rootfs incase it is block device + Fstype string `json:"fstype"` + + // CgroupPath is the cgroup hierarchy where sandbox's processes + // including the hypervisor are placed. + CgroupPath string `json:"cgroupPath,omitempty"` +} + +// Valid checks that the container state is valid. +func (state *ContainerState) Valid() bool { + return state.State.valid() +} + +// ValidTransition returns an error if we want to move to +// an unreachable state. +func (state *ContainerState) ValidTransition(oldState StateString, newState StateString) error { + return state.State.validTransition(oldState, newState) +} diff --git a/virtcontainers/types/sandbox.go b/virtcontainers/types/sandbox.go index bd1d6b167f..002230c393 100644 --- a/virtcontainers/types/sandbox.go +++ b/virtcontainers/types/sandbox.go @@ -27,17 +27,13 @@ const ( StateStopped StateString = "stopped" ) -// State is a sandbox state structure. -type State struct { +// SandboxState is a sandbox state structure +type SandboxState struct { State StateString `json:"state"` - BlockDeviceID string // Index of the block device passed to hypervisor. BlockIndex int `json:"blockIndex"` - // File system of the rootfs incase it is block device - Fstype string `json:"fstype"` - // Pid is the process id of the sandbox container which is the first // container to be started. Pid int `json:"pid"` @@ -54,9 +50,19 @@ type State struct { } // Valid checks that the sandbox state is valid. -func (state *State) Valid() bool { +func (state *SandboxState) Valid() bool { + return state.State.valid() +} + +// ValidTransition returns an error if we want to move to +// an unreachable state. +func (state *SandboxState) ValidTransition(oldState StateString, newState StateString) error { + return state.State.validTransition(oldState, newState) +} + +func (state *StateString) valid() bool { for _, validState := range []StateString{StateReady, StateRunning, StatePaused, StateStopped} { - if state.State == validState { + if *state == validState { return true } } @@ -64,14 +70,12 @@ func (state *State) Valid() bool { return false } -// ValidTransition returns an error if we want to move to -// an unreachable state. -func (state *State) ValidTransition(oldState StateString, newState StateString) error { - if state.State != oldState { - return fmt.Errorf("Invalid state %s (Expecting %s)", state.State, oldState) +func (state *StateString) validTransition(oldState StateString, newState StateString) error { + if *state != oldState { + return fmt.Errorf("Invalid state %v (Expecting %v)", state, oldState) } - switch state.State { + switch *state { case StateReady: if newState == StateRunning || newState == StateStopped { return nil @@ -93,8 +97,8 @@ func (state *State) ValidTransition(oldState StateString, newState StateString) } } - return fmt.Errorf("Can not move from %s to %s", - state.State, newState) + return fmt.Errorf("Can not move from %v to %v", + state, newState) } // Volume is a shared volume between the host and the VM, From 03ee25d4eff162ca1c257fc849b5b516e3d491da Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 9 Apr 2019 01:14:23 -0700 Subject: [PATCH 2/3] agent: treat container as shared pidns whenever it has pidns path Current approach cannot work for shimv2 as there is no kata-shim thus sandbox.state.pid is always -1. Let's just simplify things by always making a container share pidns if it has a pidns path. Signed-off-by: Peng Tao --- virtcontainers/kata_agent.go | 34 +++++++------------------------ virtcontainers/kata_agent_test.go | 20 +++--------------- 2 files changed, 10 insertions(+), 44 deletions(-) diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 56a3caa661..f8bea7f767 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -1071,10 +1071,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, // We need to give the OCI spec our absolute rootfs path in the guest. grpcSpec.Root.Path = rootPath - sharedPidNs, err := k.handlePidNamespace(grpcSpec, sandbox) - if err != nil { - return nil, err - } + sharedPidNs := k.handlePidNamespace(grpcSpec, sandbox) passSeccomp := !sandbox.config.DisableGuestSeccomp && sandbox.seccompSupported @@ -1191,7 +1188,7 @@ func (k *kataAgent) handleBlockVolumes(c *Container) []*grpc.Storage { // handlePidNamespace checks if Pid namespace for a container needs to be shared with its sandbox // pid namespace. This function also modifies the grpc spec to remove the pid namespace // from the list of namespaces passed to the agent. -func (k *kataAgent) handlePidNamespace(grpcSpec *grpc.Spec, sandbox *Sandbox) (bool, error) { +func (k *kataAgent) handlePidNamespace(grpcSpec *grpc.Spec, sandbox *Sandbox) bool { sharedPidNs := false pidIndex := -1 @@ -1201,29 +1198,11 @@ func (k *kataAgent) handlePidNamespace(grpcSpec *grpc.Spec, sandbox *Sandbox) (b } pidIndex = i - - if ns.Path == "" || sandbox.state.Pid == 0 { - break - } - - pidNsPath := fmt.Sprintf("/proc/%d/ns/pid", sandbox.state.Pid) - - // Check if pid namespace path is the same as the sandbox - if ns.Path == pidNsPath { - sharedPidNs = true - } else { - ln, err := filepath.EvalSymlinks(ns.Path) - if err != nil { - return sharedPidNs, err - } - - // We have arbitrary pid namespace path here. - if ln != pidNsPath { - return sharedPidNs, fmt.Errorf("Pid namespace path %s other than sandbox %s", ln, pidNsPath) - } + // host pidns path does not make sense in kata. Let's just align it with + // sandbox namespace whenever it is set. + if ns.Path != "" { sharedPidNs = true } - break } @@ -1231,7 +1210,8 @@ func (k *kataAgent) handlePidNamespace(grpcSpec *grpc.Spec, sandbox *Sandbox) (b if pidIndex >= 0 { grpcSpec.Linux.Namespaces = append(grpcSpec.Linux.Namespaces[:pidIndex], grpcSpec.Linux.Namespaces[pidIndex+1:]...) } - return sharedPidNs, nil + + return sharedPidNs } func (k *kataAgent) startContainer(sandbox *Sandbox, c *Container) error { diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 788e090d0d..cc3b8a97b7 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -574,8 +574,7 @@ func TestHandlePidNamespace(t *testing.T) { k := kataAgent{} - sharedPid, err := k.handlePidNamespace(g, sandbox) - assert.Nil(err) + sharedPid := k.handlePidNamespace(g, sandbox) assert.False(sharedPid) assert.False(testIsPidNamespacePresent(g)) @@ -592,32 +591,19 @@ func TestHandlePidNamespace(t *testing.T) { g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) g.Linux.Namespaces = append(g.Linux.Namespaces, utsNs) - sharedPid, err = k.handlePidNamespace(g, sandbox) - assert.Nil(err) + sharedPid = k.handlePidNamespace(g, sandbox) assert.False(sharedPid) assert.False(testIsPidNamespacePresent(g)) - sandbox.state.Pid = 112 pidNs = pb.LinuxNamespace{ Type: string(specs.PIDNamespace), Path: "/proc/112/ns/pid", } g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) - sharedPid, err = k.handlePidNamespace(g, sandbox) - assert.Nil(err) + sharedPid = k.handlePidNamespace(g, sandbox) assert.True(sharedPid) assert.False(testIsPidNamespacePresent(g)) - - // Arbitrary path - pidNs = pb.LinuxNamespace{ - Type: string(specs.PIDNamespace), - Path: "/proc/234/ns/pid", - } - g.Linux.Namespaces = append(g.Linux.Namespaces, pidNs) - - _, err = k.handlePidNamespace(g, sandbox) - assert.NotNil(err) } func TestAgentPathAPI(t *testing.T) { From c41459963509eaa43a3c05f85f895bf57d0867e1 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 9 Apr 2019 01:30:09 -0700 Subject: [PATCH 3/3] types: remove pid from sandbox state No longer needed. Signed-off-by: Peng Tao --- virtcontainers/cgroups_test.go | 7 +++---- virtcontainers/kata_agent_test.go | 1 - virtcontainers/sandbox.go | 1 - virtcontainers/types/sandbox.go | 4 ---- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/virtcontainers/cgroups_test.go b/virtcontainers/cgroups_test.go index a9a7c920c0..27d848f5c5 100644 --- a/virtcontainers/cgroups_test.go +++ b/virtcontainers/cgroups_test.go @@ -154,8 +154,7 @@ func TestUpdateCgroups(t *testing.T) { // fake workload cmd := exec.Command("tail", "-f", "/dev/null") assert.NoError(cmd.Start()) - s.state.Pid = cmd.Process.Pid - s.hypervisor = &mockHypervisor{mockPid: s.state.Pid} + s.hypervisor = &mockHypervisor{mockPid: cmd.Process.Pid} // no containers err = s.updateCgroups() @@ -167,7 +166,7 @@ func TestUpdateCgroups(t *testing.T) { s.containers = map[string]*Container{ "abc": { process: Process{ - Pid: s.state.Pid, + Pid: cmd.Process.Pid, }, config: &ContainerConfig{ Annotations: containerAnnotations, @@ -175,7 +174,7 @@ func TestUpdateCgroups(t *testing.T) { }, "xyz": { process: Process{ - Pid: s.state.Pid, + Pid: cmd.Process.Pid, }, config: &ContainerConfig{ Annotations: containerAnnotations, diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index cc3b8a97b7..cda2dc26f6 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -570,7 +570,6 @@ func TestHandlePidNamespace(t *testing.T) { } sandbox := &Sandbox{} - sandbox.state.Pid = 0 k := kataAgent{} diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index d207fc018e..7f88973adc 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -1009,7 +1009,6 @@ func (s *Sandbox) addContainer(c *Container) error { ann := c.GetAnnotations() if ann[annotations.ContainerTypeKey] == string(PodSandbox) { - s.state.Pid = c.process.Pid s.state.CgroupPath = c.state.CgroupPath return s.store.Store(store.State, s.state) } diff --git a/virtcontainers/types/sandbox.go b/virtcontainers/types/sandbox.go index 002230c393..7317365168 100644 --- a/virtcontainers/types/sandbox.go +++ b/virtcontainers/types/sandbox.go @@ -34,10 +34,6 @@ type SandboxState struct { // Index of the block device passed to hypervisor. BlockIndex int `json:"blockIndex"` - // Pid is the process id of the sandbox container which is the first - // container to be started. - Pid int `json:"pid"` - // GuestMemoryBlockSizeMB is the size of memory block of guestos GuestMemoryBlockSizeMB uint32 `json:"guestMemoryBlockSize"`