From 235c37e6489a2d46af00f756e649db0acb553001 Mon Sep 17 00:00:00 2001 From: Jianyong Wu Date: Wed, 13 Mar 2019 06:00:13 -0400 Subject: [PATCH 1/5] Factory: Fix fake return value issue on creating template Now, function NewFactory will return nil even create template does't complete. As for this, it will tell user that factory has been initialized no matter whether the template is created or not. This patch correct it by adding another return value of error in NewFactory. Testing initFactoryCommand when enable template will need root privilege to mount tmpfs. So skip it for no-root user. Testing initFactoryCommand func will create template, but no proxy type assigned to VMconfig which will using katabuiltinProxy instead. this will lead to failure for this type of proxy will check proxyparams which contains many null value. This commit fix it by substitute katabuiltinProxy as noopProxy when for test purpose. Fixes: #1333 Signed-off-by: Jianyong Wu --- cli/factory.go | 1 + cli/factory_test.go | 7 +++++++ virtcontainers/factory/factory.go | 5 ++++- virtcontainers/factory/factory_test.go | 11 +++++++++++ virtcontainers/factory/template/template.go | 15 +++++++-------- virtcontainers/factory/template/template_test.go | 11 +++++++++-- 6 files changed, 39 insertions(+), 11 deletions(-) diff --git a/cli/factory.go b/cli/factory.go index da1793dd7b..55316e35ae 100644 --- a/cli/factory.go +++ b/cli/factory.go @@ -185,6 +185,7 @@ var initFactoryCommand = cli.Command{ HypervisorConfig: runtimeConfig.HypervisorConfig, AgentType: runtimeConfig.AgentType, AgentConfig: runtimeConfig.AgentConfig, + ProxyType: runtimeConfig.ProxyType, }, } kataLog.WithField("factory", factoryConfig).Info("create vm factory") diff --git a/cli/factory_test.go b/cli/factory_test.go index faf5028d73..ff7609a8fb 100644 --- a/cli/factory_test.go +++ b/cli/factory_test.go @@ -17,6 +17,8 @@ import ( vc "github.com/kata-containers/runtime/virtcontainers" ) +const testDisabledAsNonRoot = "Test disabled as requires root privileges" + func TestFactoryCLIFunctionNoRuntimeConfig(t *testing.T) { assert := assert.New(t) @@ -63,9 +65,14 @@ func TestFactoryCLIFunctionInit(t *testing.T) { assert.Nil(err) // With template + if os.Geteuid() != 0 { + t.Skip(testDisabledAsNonRoot) + } + runtimeConfig.FactoryConfig.Template = true runtimeConfig.HypervisorType = vc.MockHypervisor runtimeConfig.AgentType = vc.NoopAgentType + runtimeConfig.ProxyType = vc.NoopProxyType ctx.App.Metadata["runtimeConfig"] = runtimeConfig fn, ok = initFactoryCommand.Action.(func(context *cli.Context) error) assert.True(ok) diff --git a/virtcontainers/factory/factory.go b/virtcontainers/factory/factory.go index 4bbe198372..15712c9540 100644 --- a/virtcontainers/factory/factory.go +++ b/virtcontainers/factory/factory.go @@ -67,7 +67,10 @@ func NewFactory(ctx context.Context, config Config, fetchOnly bool) (vc.Factory, return nil, err } } else { - b = template.New(ctx, config.VMConfig) + b, err = template.New(ctx, config.VMConfig) + if err != nil { + return nil, err + } } } else if config.VMCache && config.Cache == 0 { b, err = grpccache.New(ctx, config.VMCacheEndpoint) diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go index a8e08606c2..7ff3df8cc8 100644 --- a/virtcontainers/factory/factory_test.go +++ b/virtcontainers/factory/factory_test.go @@ -8,6 +8,7 @@ package factory import ( "context" "io/ioutil" + "os" "testing" vc "github.com/kata-containers/runtime/virtcontainers" @@ -17,6 +18,8 @@ import ( "github.com/stretchr/testify/assert" ) +const testDisabledAsNonRoot = "Test disabled as requires root privileges" + func TestNewFactory(t *testing.T) { var config Config @@ -53,6 +56,10 @@ func TestNewFactory(t *testing.T) { f.CloseFactory(ctx) // template + if os.Geteuid() != 0 { + t.Skip(testDisabledAsNonRoot) + } + config.Template = true f, err = NewFactory(ctx, config, false) assert.Nil(err) @@ -187,6 +194,10 @@ func TestFactoryGetVM(t *testing.T) { ctx := context.Background() // direct factory + if os.Geteuid() != 0 { + t.Skip(testDisabledAsNonRoot) + } + f, err := NewFactory(ctx, Config{VMConfig: vmConfig}, false) assert.Nil(err) diff --git a/virtcontainers/factory/template/template.go b/virtcontainers/factory/template/template.go index 968b61c28c..26740e3c91 100644 --- a/virtcontainers/factory/template/template.go +++ b/virtcontainers/factory/template/template.go @@ -15,7 +15,6 @@ import ( vc "github.com/kata-containers/runtime/virtcontainers" "github.com/kata-containers/runtime/virtcontainers/factory/base" - "github.com/kata-containers/runtime/virtcontainers/factory/direct" "github.com/kata-containers/runtime/virtcontainers/store" ) @@ -42,14 +41,13 @@ func Fetch(config vc.VMConfig) (base.FactoryBase, error) { } // New creates a new VM template factory. -func New(ctx context.Context, config vc.VMConfig) base.FactoryBase { +func New(ctx context.Context, config vc.VMConfig) (base.FactoryBase, error) { statePath := store.RunVMStoragePath + "/template" t := &template{statePath, config} err := t.prepareTemplateFiles() if err != nil { - // fallback to direct factory if template is not supported. - return direct.New(ctx, config) + return nil, err } defer func() { if err != nil { @@ -59,11 +57,10 @@ func New(ctx context.Context, config vc.VMConfig) base.FactoryBase { err = t.createTemplateVM(ctx) if err != nil { - // fallback to direct factory if template is not supported. - return direct.New(ctx, config) + return nil, err } - return t + return t, nil } // Config returns template factory's configuration. @@ -116,7 +113,9 @@ func (t *template) createTemplateVM(ctx context.Context) error { config.HypervisorConfig.MemoryPath = t.statePath + "/memory" config.HypervisorConfig.DevicesStatePath = t.statePath + "/state" // template vm uses builtin proxy - config.ProxyType = templateProxyType + if config.ProxyType != "noopProxy" { + config.ProxyType = templateProxyType + } vm, err := vc.NewVM(ctx, config) if err != nil { diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go index d08a2ab69e..f2c4e9da84 100644 --- a/virtcontainers/factory/template/template_test.go +++ b/virtcontainers/factory/template/template_test.go @@ -17,7 +17,13 @@ import ( vc "github.com/kata-containers/runtime/virtcontainers" ) +const testDisabledAsNonRoot = "Test disabled as requires root privileges" + func TestTemplateFactory(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip(testDisabledAsNonRoot) + } + assert := assert.New(t) templateWaitForAgent = 1 * time.Microsecond @@ -40,7 +46,8 @@ func TestTemplateFactory(t *testing.T) { ctx := context.Background() // New - f := New(ctx, vmConfig) + f, err := New(ctx, vmConfig) + assert.Nil(err) // Config assert.Equal(f.Config(), vmConfig) @@ -74,7 +81,7 @@ func TestTemplateFactory(t *testing.T) { assert.Nil(err) err = tt.createTemplateVM(ctx) - assert.Error(err) + assert.Nil(err) templateProxyType = vc.NoopProxyType vm, err = tt.GetBaseVM(ctx, vmConfig) From 0003b4c6999e32ed59202d3b5247bcbf3505c235 Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Tue, 26 Mar 2019 16:18:16 +0800 Subject: [PATCH 2/5] tests: do cleanUp() always in the end Fixes: #1422 Detect failing test case: ``` .... === RUN TestEnterContainerFailingContNotStarted --- PASS: TestEnterContainerFailingContNotStarted (0.01s) === RUN TestEnterContainer --- FAIL: TestEnterContainer (0.00s) Error Trace: sandbox_test.go:1154 Error: Expected value not to be nil. Messages: Entering non-running container should fail Error Trace: sandbox_test.go:1157 Error: Expected nil, but got: &errors.errorString{s:"Can not move from running to running"} Messages: Failed to start sandbox: Can not move from running to running FAIL ``` `TestEnterContainerFailingContNotStarted` calls `cleanUp` at function begging but it doesn't clean its garbage after it ends. `TestEnterContainer` only call `cleanUp` in the end but it doesn't do cleanUp in the begging, that gives first test case a chance to impact latter one. This commit modifies all the test cases, let them all do the cleanUp() in the end. The policy here is: "everyone needs to take their garbage away when they leave" :) Signed-off-by: Wei Zhang --- virtcontainers/api_test.go | 122 +++++++++++++++--------------- virtcontainers/cc_shim_test.go | 2 +- virtcontainers/kata_shim_test.go | 2 +- virtcontainers/noop_agent_test.go | 2 +- virtcontainers/sandbox_test.go | 2 +- 5 files changed, 65 insertions(+), 65 deletions(-) diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index 8e27d727cd..8f10dc9987 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -199,7 +199,7 @@ func newTestSandboxConfigKataAgent() SandboxConfig { } func TestCreateSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -232,7 +232,7 @@ func TestCreateSandboxHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigHyperstartAgent() @@ -265,7 +265,7 @@ func TestCreateSandboxKataAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigKataAgent() @@ -302,7 +302,7 @@ func TestCreateSandboxKataAgentSuccessful(t *testing.T) { } func TestCreateSandboxFailing(t *testing.T) { - cleanUp() + defer cleanUp() config := SandboxConfig{} @@ -313,7 +313,7 @@ func TestCreateSandboxFailing(t *testing.T) { } func TestDeleteSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() ctx := context.Background() config := newTestSandboxConfigNoop() @@ -345,7 +345,7 @@ func TestDeleteSandboxHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigHyperstartAgent() @@ -390,7 +390,7 @@ func TestDeleteSandboxKataAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigKataAgent() @@ -438,7 +438,7 @@ func TestDeleteSandboxKataAgentSuccessful(t *testing.T) { } func TestDeleteSandboxFailing(t *testing.T) { - cleanUp() + defer cleanUp() sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) os.Remove(sandboxDir) @@ -450,7 +450,7 @@ func TestDeleteSandboxFailing(t *testing.T) { } func TestStartSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -461,7 +461,7 @@ func TestStartSandboxNoopAgentSuccessful(t *testing.T) { } func TestStartSandboxHyperstartAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() if os.Geteuid() != 0 { t.Skip(testDisabledAsNonRoot) @@ -501,7 +501,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigKataAgent() @@ -538,7 +538,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) { } func TestStartSandboxFailing(t *testing.T) { - cleanUp() + defer cleanUp() sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) os.Remove(sandboxDir) @@ -550,7 +550,7 @@ func TestStartSandboxFailing(t *testing.T) { } func TestStopSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -567,7 +567,7 @@ func TestStopSandboxNoopAgentSuccessful(t *testing.T) { } func TestPauseThenResumeSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -632,7 +632,7 @@ func TestStopSandboxHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigHyperstartAgent() @@ -668,7 +668,7 @@ func TestStopSandboxKataAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigKataAgent() @@ -705,7 +705,7 @@ func TestStopSandboxKataAgentSuccessful(t *testing.T) { } func TestStopSandboxFailing(t *testing.T) { - cleanUp() + defer cleanUp() sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) os.Remove(sandboxDir) @@ -717,7 +717,7 @@ func TestStopSandboxFailing(t *testing.T) { } func TestRunSandboxNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -738,7 +738,7 @@ func TestRunSandboxHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigHyperstartAgent() @@ -780,7 +780,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigKataAgent() @@ -823,7 +823,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) { } func TestRunSandboxFailing(t *testing.T) { - cleanUp() + defer cleanUp() config := SandboxConfig{} @@ -834,7 +834,7 @@ func TestRunSandboxFailing(t *testing.T) { } func TestListSandboxSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -851,7 +851,7 @@ func TestListSandboxSuccessful(t *testing.T) { } func TestListSandboxNoSandboxDirectory(t *testing.T) { - cleanUp() + defer cleanUp() _, err := ListSandbox(context.Background()) if err != nil { @@ -860,7 +860,7 @@ func TestListSandboxNoSandboxDirectory(t *testing.T) { } func TestStatusSandboxSuccessfulStateReady(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() hypervisorConfig := HypervisorConfig{ @@ -919,7 +919,7 @@ func TestStatusSandboxSuccessfulStateReady(t *testing.T) { } func TestStatusSandboxSuccessfulStateRunning(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() hypervisorConfig := HypervisorConfig{ @@ -983,7 +983,7 @@ func TestStatusSandboxSuccessfulStateRunning(t *testing.T) { } func TestStatusSandboxFailingFetchSandboxConfig(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -1003,7 +1003,7 @@ func TestStatusSandboxFailingFetchSandboxConfig(t *testing.T) { } func TestStatusPodSandboxFailingFetchSandboxState(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -1035,7 +1035,7 @@ func newTestContainerConfigNoop(contID string) ContainerConfig { } func TestCreateContainerSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1067,7 +1067,7 @@ func TestCreateContainerSuccessful(t *testing.T) { } func TestCreateContainerFailingNoSandbox(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1098,7 +1098,7 @@ func TestCreateContainerFailingNoSandbox(t *testing.T) { } func TestDeleteContainerSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1140,7 +1140,7 @@ func TestDeleteContainerSuccessful(t *testing.T) { } func TestDeleteContainerFailingNoSandbox(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" c, err := DeleteContainer(context.Background(), testSandboxID, contID) @@ -1150,7 +1150,7 @@ func TestDeleteContainerFailingNoSandbox(t *testing.T) { } func TestDeleteContainerFailingNoContainer(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1174,7 +1174,7 @@ func TestDeleteContainerFailingNoContainer(t *testing.T) { } func TestStartContainerNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1205,7 +1205,7 @@ func TestStartContainerNoopAgentSuccessful(t *testing.T) { } func TestStartContainerFailingNoSandbox(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" c, err := StartContainer(context.Background(), testSandboxID, contID) @@ -1215,7 +1215,7 @@ func TestStartContainerFailingNoSandbox(t *testing.T) { } func TestStartContainerFailingNoContainer(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1239,7 +1239,7 @@ func TestStartContainerFailingNoContainer(t *testing.T) { } func TestStartContainerFailingSandboxNotStarted(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1276,7 +1276,7 @@ func TestStartContainerFailingSandboxNotStarted(t *testing.T) { } func TestStopContainerNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1317,7 +1317,7 @@ func TestStartStopContainerHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigHyperstartAgent() @@ -1378,7 +1378,7 @@ func TestStartStopSandboxHyperstartAgentSuccessfulWithDefaultNetwork(t *testing. t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() config := newTestSandboxConfigHyperstartAgentDefaultNetwork() @@ -1425,7 +1425,7 @@ func TestStartStopSandboxHyperstartAgentSuccessfulWithDefaultNetwork(t *testing. } func TestStopContainerFailingNoSandbox(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" c, err := StopContainer(context.Background(), testSandboxID, contID) @@ -1435,7 +1435,7 @@ func TestStopContainerFailingNoSandbox(t *testing.T) { } func TestStopContainerFailingNoContainer(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1459,7 +1459,7 @@ func TestStopContainerFailingNoContainer(t *testing.T) { } func testKillContainerFromContReadySuccessful(t *testing.T, signal syscall.Signal) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1501,7 +1501,7 @@ func TestKillContainerFromContReadySuccessful(t *testing.T) { } func TestEnterContainerNoopAgentSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1544,7 +1544,7 @@ func TestEnterContainerHyperstartAgentSuccessful(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigHyperstartAgent() @@ -1608,7 +1608,7 @@ func TestEnterContainerHyperstartAgentSuccessful(t *testing.T) { } func TestEnterContainerFailingNoSandbox(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" cmd := newBasicTestCmd() @@ -1619,7 +1619,7 @@ func TestEnterContainerFailingNoSandbox(t *testing.T) { } func TestEnterContainerFailingNoContainer(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1645,7 +1645,7 @@ func TestEnterContainerFailingNoContainer(t *testing.T) { } func TestEnterContainerFailingContNotStarted(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1679,7 +1679,7 @@ func TestEnterContainerFailingContNotStarted(t *testing.T) { } func TestStatusContainerSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1730,7 +1730,7 @@ func TestStatusContainerSuccessful(t *testing.T) { } func TestStatusContainerStateReady(t *testing.T) { - cleanUp() + defer cleanUp() // (homage to a great album! ;) contID := "101" @@ -1796,7 +1796,7 @@ func TestStatusContainerStateReady(t *testing.T) { } func TestStatusContainerStateRunning(t *testing.T) { - cleanUp() + defer cleanUp() // (homage to a great album! ;) contID := "101" @@ -1872,7 +1872,7 @@ func TestStatusContainerStateRunning(t *testing.T) { } func TestStatusContainerFailing(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1893,7 +1893,7 @@ func TestStatusContainerFailing(t *testing.T) { } func TestStatsContainerFailing(t *testing.T) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() @@ -1914,7 +1914,7 @@ func TestStatsContainerFailing(t *testing.T) { } func TestStatsContainer(t *testing.T) { - cleanUp() + defer cleanUp() assert := assert.New(t) contID := "100" @@ -1960,7 +1960,7 @@ func TestStatsContainer(t *testing.T) { } func TestProcessListContainer(t *testing.T) { - cleanUp() + defer cleanUp() assert := assert.New(t) @@ -2255,7 +2255,7 @@ func BenchmarkStartStop10ContainerQemuHypervisorHyperstartAgentNetworkNoop(b *te } func TestFetchSandbox(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -2272,7 +2272,7 @@ func TestFetchSandbox(t *testing.T) { } func TestFetchStatefulSandbox(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -2291,14 +2291,14 @@ func TestFetchStatefulSandbox(t *testing.T) { } func TestFetchNonExistingSandbox(t *testing.T) { - cleanUp() + defer cleanUp() _, err := FetchSandbox(context.Background(), "some-non-existing-sandbox-name") assert.NotNil(t, err, "fetch non-existing sandbox should fail") } func TestReleaseSandbox(t *testing.T) { - cleanUp() + defer cleanUp() config := newTestSandboxConfigNoop() @@ -2315,7 +2315,7 @@ func TestUpdateContainer(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() ctx := context.Background() @@ -2368,7 +2368,7 @@ func TestPauseResumeContainer(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() ctx := context.Background() @@ -2410,7 +2410,7 @@ func TestNetworkOperation(t *testing.T) { t.Skip(testDisabledAsNonRoot) } - cleanUp() + defer cleanUp() assert := assert.New(t) inf := &vcTypes.Interface{ diff --git a/virtcontainers/cc_shim_test.go b/virtcontainers/cc_shim_test.go index 9a1e1045e9..3c6733998c 100644 --- a/virtcontainers/cc_shim_test.go +++ b/virtcontainers/cc_shim_test.go @@ -327,7 +327,7 @@ func newConsole() (*os.File, string, error) { } func TestCCShimStartWithConsoleSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() master, console, err := newConsole() t.Logf("Console created for tests:%s\n", console) diff --git a/virtcontainers/kata_shim_test.go b/virtcontainers/kata_shim_test.go index f0f16e448f..ec77383a2b 100644 --- a/virtcontainers/kata_shim_test.go +++ b/virtcontainers/kata_shim_test.go @@ -273,7 +273,7 @@ func TestKataShimStartWithConsoleNonExistingFailure(t *testing.T) { } func TestKataShimStartWithConsoleSuccessful(t *testing.T) { - cleanUp() + defer cleanUp() master, console, err := newConsole() t.Logf("Console created for tests:%s\n", console) diff --git a/virtcontainers/noop_agent_test.go b/virtcontainers/noop_agent_test.go index ad471f4647..e62cbe0594 100644 --- a/virtcontainers/noop_agent_test.go +++ b/virtcontainers/noop_agent_test.go @@ -15,7 +15,7 @@ import ( ) func testCreateNoopContainer() (*Sandbox, *Container, error) { - cleanUp() + defer cleanUp() contID := "100" config := newTestSandboxConfigNoop() diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index f9d78cce49..56d12b7832 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -1614,7 +1614,7 @@ func checkSandboxRemains() error { } func TestSandboxCreationFromConfigRollbackFromCreateSandbox(t *testing.T) { - cleanUp() + defer cleanUp() assert := assert.New(t) ctx := context.Background() hConf := newHypervisorConfig(nil, nil) From 715c6dab8dbf58ddd52351afeb5b3fe6e18ffba7 Mon Sep 17 00:00:00 2001 From: Ace-Tang Date: Tue, 26 Mar 2019 20:00:50 +0800 Subject: [PATCH 3/5] qemu: fix qemu leak when failed to start container do cleanup inside startVM() if start vm get error Fixes: #1426 Signed-off-by: Ace-Tang --- virtcontainers/api.go | 2 +- virtcontainers/sandbox.go | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/virtcontainers/api.go b/virtcontainers/api.go index 67ce10ca94..172b33efa8 100644 --- a/virtcontainers/api.go +++ b/virtcontainers/api.go @@ -108,7 +108,7 @@ func createSandboxFromConfig(ctx context.Context, sandboxConfig SandboxConfig, f } }() - if err := s.getAndStoreGuestDetails(); err != nil { + if err = s.getAndStoreGuestDetails(); err != nil { return nil, err } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 630f282f41..b0fc242e12 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -894,7 +894,7 @@ func (s *Sandbox) ListRoutes() ([]*vcTypes.Route, error) { } // startVM starts the VM. -func (s *Sandbox) startVM() error { +func (s *Sandbox) startVM() (err error) { span, ctx := s.trace("startVM") defer span.Finish() @@ -925,6 +925,12 @@ func (s *Sandbox) startVM() error { return err } + defer func() { + if err != nil { + s.hypervisor.stopSandbox() + } + }() + // In case of vm factory, network interfaces are hotplugged // after vm is started. if s.factory != nil { From d615ebd7b919cf4e25cf2e1ebac0a7f4984a744a Mon Sep 17 00:00:00 2001 From: Gabi Beyer Date: Mon, 1 Apr 2019 12:44:03 -0700 Subject: [PATCH 4/5] doc: update architecture.md link update architecture.md link, since it has moved to within the design/ directory. Fixes: #1462 Signed-off-by: Gabi Beyer --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 76ec285f81..26a2f471b0 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ See the ## Architecture overview -See the [architecture overview](https://github.com/kata-containers/documentation/blob/master/architecture.md) +See the [architecture overview](https://github.com/kata-containers/documentation/blob/master/design/architecture.md) for details on the Kata Containers design. ## Configuration From 94e9202c850adf93e297d8bb10f558ec51800c40 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 26 Mar 2019 03:31:28 -0700 Subject: [PATCH 5/5] config: validate proxy path Like shim, we should validate the proxy path if it is provided. Fixes: #1424 Signed-off-by: Peng Tao --- pkg/katautils/config.go | 16 ++++++++++----- pkg/katautils/config_test.go | 38 ++++++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/pkg/katautils/config.go b/pkg/katautils/config.go index a5145ec168..6c3008d0b0 100644 --- a/pkg/katautils/config.go +++ b/pkg/katautils/config.go @@ -357,12 +357,13 @@ func (h hypervisor) getInitrdAndImage() (initrd string, image string, err error) return } -func (p proxy) path() string { - if p.Path == "" { - return defaultProxyPath +func (p proxy) path() (string, error) { + path := p.Path + if path == "" { + path = defaultProxyPath } - return p.Path + return ResolvePath(path) } func (p proxy) debug() bool { @@ -606,8 +607,13 @@ func updateRuntimeConfigProxy(configPath string, tomlConf tomlConfig, config *oc config.ProxyType = vc.KataProxyType } + path, err := proxy.path() + if err != nil { + return err + } + config.ProxyConfig = vc.ProxyConfig{ - Path: proxy.path(), + Path: path, Debug: proxy.debug(), } } diff --git a/pkg/katautils/config_test.go b/pkg/katautils/config_test.go index 545dd11623..d33ab2958e 100644 --- a/pkg/katautils/config_test.go +++ b/pkg/katautils/config_test.go @@ -1122,13 +1122,43 @@ func TestHypervisorDefaultsGuestHookPath(t *testing.T) { } func TestProxyDefaults(t *testing.T) { + assert := assert.New(t) + + tmpdir, err := ioutil.TempDir(testDir, "") + assert.NoError(err) + defer os.RemoveAll(tmpdir) + + testProxyPath := filepath.Join(tmpdir, "proxy") + testProxyLinkPath := filepath.Join(tmpdir, "proxy-link") + + err = createEmptyFile(testProxyPath) + assert.NoError(err) + + err = syscall.Symlink(testProxyPath, testProxyLinkPath) + assert.NoError(err) + + savedProxyPath := defaultProxyPath + + defer func() { + defaultProxyPath = savedProxyPath + }() + + defaultProxyPath = testProxyPath p := proxy{} + path, err := p.path() + assert.NoError(err) + assert.Equal(path, defaultProxyPath, "default proxy path wrong") - assert.Equal(t, p.path(), defaultProxyPath, "default proxy path wrong") + // test path resolution + defaultProxyPath = testProxyLinkPath + p = proxy{} + path, err = p.path() + assert.NoError(err) + assert.Equal(path, testProxyPath) - path := "/foo/bar/baz/proxy" - p.Path = path - assert.Equal(t, p.path(), path, "custom proxy path wrong") + assert.False(p.debug()) + p.Debug = true + assert.True(p.debug()) } func TestShimDefaults(t *testing.T) {