From 163a08177627818bcee631d4a37851cc74468f47 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Wed, 25 Apr 2018 09:01:53 -0700 Subject: [PATCH 1/2] cli: Check sandbox state before to issue a StopSandbox The way a delete works, it was always trying to stop the sandbox, even when the force flag was not enabled. Because we want to be able to stop the sandbox from a kill command, this means a sandbox stop might be called twice, and we don't want the second stop to fail, leading to the failure of the delete command. That's why this commit checks for the sandbox status before to try stopping the sandbox. Fixes #246 Signed-off-by: Sebastien Boeuf --- cli/delete.go | 9 ++++++++- cli/delete_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/cli/delete.go b/cli/delete.go index b775b6a11a..11db6fc5b0 100644 --- a/cli/delete.go +++ b/cli/delete.go @@ -105,10 +105,17 @@ func delete(containerID string, force bool) error { } func deleteSandbox(sandboxID string) error { - if _, err := vci.StopSandbox(sandboxID); err != nil { + status, err := vci.StatusSandbox(sandboxID) + if err != nil { return err } + if oci.StateToOCIState(status.State) != oci.StateStopped { + if _, err := vci.StopSandbox(sandboxID); err != nil { + return err + } + } + if _, err := vci.DeleteSandbox(sandboxID); err != nil { return err } diff --git a/cli/delete_test.go b/cli/delete_test.go index 89c5715466..66debda43e 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -192,6 +192,23 @@ func TestDeleteSandbox(t *testing.T) { assert.Error(err) assert.True(vcmock.IsMockError(err)) + testingImpl.StatusSandboxFunc = func(sandboxID string) (vc.SandboxStatus, error) { + return vc.SandboxStatus{ + ID: sandbox.ID(), + State: vc.State{ + State: vc.StateReady, + }, + }, nil + } + + defer func() { + testingImpl.StatusSandboxFunc = nil + }() + + err = delete(sandbox.ID(), false) + assert.Error(err) + assert.True(vcmock.IsMockError(err)) + testingImpl.StopSandboxFunc = func(sandboxID string) (vc.VCSandbox, error) { return sandbox, nil } @@ -297,11 +314,21 @@ func TestDeleteSandboxRunning(t *testing.T) { assert.Error(err) assert.False(vcmock.IsMockError(err)) + testingImpl.StatusSandboxFunc = func(sandboxID string) (vc.SandboxStatus, error) { + return vc.SandboxStatus{ + ID: sandbox.ID(), + State: vc.State{ + State: vc.StateRunning, + }, + }, nil + } + testingImpl.StopSandboxFunc = func(sandboxID string) (vc.VCSandbox, error) { return sandbox, nil } defer func() { + testingImpl.StatusSandboxFunc = nil testingImpl.StopSandboxFunc = nil }() @@ -525,6 +552,15 @@ func TestDeleteCLIFunctionSuccess(t *testing.T) { }, nil } + testingImpl.StatusSandboxFunc = func(sandboxID string) (vc.SandboxStatus, error) { + return vc.SandboxStatus{ + ID: sandbox.ID(), + State: vc.State{ + State: vc.StateReady, + }, + }, nil + } + testingImpl.StopSandboxFunc = func(sandboxID string) (vc.VCSandbox, error) { return sandbox, nil } From 07af4edea96b51610f861aeec39627005753a5fc Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Wed, 25 Apr 2018 09:07:34 -0700 Subject: [PATCH 2/2] cli: Stop the sandbox on a KILL The same way a caller of "kata-runtime kill 12345" expects the container 12345 to be killed, the same call to a container representing a sandbox should actually kill the sandbox, meaning it would be stopped after the container has been killed. This way, the caller knows the VM is stopped after kill returns. This is an issue raised by Openshift and Kubernetes tests. They call into delete way after the call to kill has been submitted, and in the meantime they kill all processes related to the container, meaning they do kill the VM before we could do it ourselves. In this case, the delete responsible of stopping the VM comes too late and it returns an error when trying to destroy the sandbox while trying to communicate with the agent since the VM is not here anymore. This commit addresses this issue by letting "kill" call into StopSandbox() if the command relates to a sandbox instead of a simple container. Fixes #246 Signed-off-by: Sebastien Boeuf --- cli/kill.go | 16 ++++++++++- cli/kill_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/cli/kill.go b/cli/kill.go index 52d2744367..d8af662baa 100644 --- a/cli/kill.go +++ b/cli/kill.go @@ -12,6 +12,7 @@ import ( "syscall" vc "github.com/kata-containers/runtime/virtcontainers" + "github.com/kata-containers/runtime/virtcontainers/pkg/oci" "github.com/urfave/cli" ) @@ -115,7 +116,20 @@ func kill(containerID, signal string, all bool) error { return nil } - _, err = vci.StopContainer(sandboxID, containerID) + containerType, err := oci.GetContainerType(status.Annotations) + if err != nil { + return err + } + + switch containerType { + case vc.PodSandbox: + _, err = vci.StopSandbox(sandboxID) + case vc.PodContainer: + _, err = vci.StopContainer(sandboxID, containerID) + default: + return fmt.Errorf("Invalid container type found") + } + return err } diff --git a/cli/kill_test.go b/cli/kill_test.go index 5796c04fad..3f1866478f 100644 --- a/cli/kill_test.go +++ b/cli/kill_test.go @@ -12,6 +12,7 @@ import ( "testing" vc "github.com/kata-containers/runtime/virtcontainers" + vcAnnotations "github.com/kata-containers/runtime/virtcontainers/pkg/annotations" "github.com/kata-containers/runtime/virtcontainers/pkg/vcmock" "github.com/stretchr/testify/assert" ) @@ -24,6 +25,10 @@ var ( testStopContainerFuncReturnNil = func(sandboxID, containerID string) (vc.VCContainer, error) { return &vcmock.Container{}, nil } + + testStopSandboxFuncReturnNil = func(sandboxID string) (vc.VCSandbox, error) { + return &vcmock.Sandbox{}, nil + } ) func TestProcessSignal(t *testing.T) { @@ -61,13 +66,18 @@ func testKillCLIFunctionTerminationSignalSuccessful(t *testing.T, sig string) { State: vc.StateRunning, } + annotations := map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodContainer), + } + testingImpl.KillContainerFunc = testKillContainerFuncReturnNil testingImpl.StopContainerFunc = testStopContainerFuncReturnNil testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { - return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, map[string]string{}), nil + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil } defer func() { testingImpl.KillContainerFunc = nil + testingImpl.StopContainerFunc = nil testingImpl.ListSandboxFunc = nil }() @@ -75,6 +85,21 @@ func testKillCLIFunctionTerminationSignalSuccessful(t *testing.T, sig string) { set.Parse([]string{testContainerID, sig}) execCLICommandFunc(assert, killCLICommand, set, false) + + annotations = map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), + } + + testingImpl.StopContainerFunc = nil + testingImpl.StopSandboxFunc = testStopSandboxFuncReturnNil + testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil + } + defer func() { + testingImpl.StopSandboxFunc = nil + }() + + execCLICommandFunc(assert, killCLICommand, set, false) } func TestKillCLIFunctionSigkillSuccessful(t *testing.T) { @@ -114,12 +139,18 @@ func TestKillCLIFunctionNoSignalSuccessful(t *testing.T) { State: vc.StateRunning, } + annotations := map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodContainer), + } + testingImpl.KillContainerFunc = testKillContainerFuncReturnNil + testingImpl.StopContainerFunc = testStopContainerFuncReturnNil testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { - return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, map[string]string{}), nil + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil } defer func() { testingImpl.KillContainerFunc = nil + testingImpl.StopContainerFunc = nil testingImpl.ListSandboxFunc = nil }() @@ -127,6 +158,21 @@ func TestKillCLIFunctionNoSignalSuccessful(t *testing.T) { set.Parse([]string{testContainerID}) execCLICommandFunc(assert, killCLICommand, set, false) + + annotations = map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), + } + + testingImpl.StopContainerFunc = nil + testingImpl.StopSandboxFunc = testStopSandboxFuncReturnNil + testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil + } + defer func() { + testingImpl.StopSandboxFunc = nil + }() + + execCLICommandFunc(assert, killCLICommand, set, false) } func TestKillCLIFunctionEnableAllSuccessful(t *testing.T) { @@ -136,6 +182,10 @@ func TestKillCLIFunctionEnableAllSuccessful(t *testing.T) { State: vc.StateRunning, } + annotations := map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodContainer), + } + testingImpl.KillContainerFunc = func(sandboxID, containerID string, signal syscall.Signal, all bool) error { if !all { return fmt.Errorf("Expecting -all flag = true, Got false") @@ -143,11 +193,13 @@ func TestKillCLIFunctionEnableAllSuccessful(t *testing.T) { return nil } + testingImpl.StopContainerFunc = testStopContainerFuncReturnNil testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { - return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, map[string]string{}), nil + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil } defer func() { testingImpl.KillContainerFunc = nil + testingImpl.StopContainerFunc = nil testingImpl.ListSandboxFunc = nil }() @@ -156,6 +208,21 @@ func TestKillCLIFunctionEnableAllSuccessful(t *testing.T) { set.Parse([]string{testContainerID}) execCLICommandFunc(assert, killCLICommand, set, false) + + annotations = map[string]string{ + vcAnnotations.ContainerTypeKey: string(vc.PodSandbox), + } + + testingImpl.StopContainerFunc = nil + testingImpl.StopSandboxFunc = testStopSandboxFuncReturnNil + testingImpl.ListSandboxFunc = func() ([]vc.SandboxStatus, error) { + return newSingleContainerSandboxStatusList(testSandboxID, testContainerID, state, state, annotations), nil + } + defer func() { + testingImpl.StopSandboxFunc = nil + }() + + execCLICommandFunc(assert, killCLICommand, set, false) } func TestKillCLIFunctionNoContainerIDFailure(t *testing.T) {