From 235c37e6489a2d46af00f756e649db0acb553001 Mon Sep 17 00:00:00 2001 From: Jianyong Wu Date: Wed, 13 Mar 2019 06:00:13 -0400 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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/7] 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) { From 52f899df6abfd7fffd308ac8b99ae83075c1607b Mon Sep 17 00:00:00 2001 From: Ganesh Maharaj Mahalingam Date: Fri, 15 Mar 2019 14:03:24 -0700 Subject: [PATCH 6/7] lint: Update go linter from gometalinter to golangci-lint. gometalinter is deprecated and will be archived April '19. The suggestion is to switch to golangci-lint which is apparently 5x faster than gometalinter. Partially Fixes: #1377 Signed-off-by: Ganesh Maharaj Mahalingam (cherry picked from commit f4428761cbd0418c8024f0356897f10417ab38bd) --- .travis.yml | 2 +- cli/delete.go | 2 +- cli/kata-check.go | 1 + cli/kata-check_amd64_test.go | 14 ++++---- cli/kata-check_ppc64le_test.go | 14 ++++---- cli/kill.go | 2 +- cli/oci.go | 2 +- cli/ps.go | 2 +- cli/start.go | 2 +- cli/update.go | 2 +- containerd-shim-v2/metrics.go | 4 +-- containerd-shim-v2/service.go | 6 ++-- containerd-shim-v2/utils.go | 2 +- pkg/katautils/config_test.go | 32 +++++++++---------- versions.yaml | 5 +++ virtcontainers/container.go | 7 ++-- virtcontainers/device/drivers/generic_test.go | 14 ++++---- virtcontainers/device/manager/manager.go | 2 +- virtcontainers/device/manager/utils.go | 6 +--- virtcontainers/example_pod_run_test.go | 2 -- virtcontainers/fc.go | 3 +- virtcontainers/hack/virtc/main.go | 6 ++-- virtcontainers/hypervisor.go | 20 +++--------- virtcontainers/hypervisor_test.go | 2 +- virtcontainers/kata_agent.go | 2 +- virtcontainers/pkg/hyperstart/hyperstart.go | 4 +-- virtcontainers/pkg/mock/mock.go | 6 ++-- virtcontainers/pkg/oci/utils.go | 4 +-- virtcontainers/qemu.go | 8 ++--- virtcontainers/qemu_arch_base.go | 4 +-- virtcontainers/qemu_arch_base_test.go | 8 ++--- virtcontainers/sandbox.go | 6 ++-- virtcontainers/store/vc.go | 6 +--- virtcontainers/types.go | 6 +--- virtcontainers/types/capabilities.go | 15 ++------- 35 files changed, 91 insertions(+), 132 deletions(-) diff --git a/.travis.yml b/.travis.yml index c37d597889..a30873a554 100644 --- a/.travis.yml +++ b/.travis.yml @@ -15,7 +15,7 @@ os: go_import_path: github.com/kata-containers/runtime go: - - "1.10.x" + - "1.11.x" env: - target_branch=$TRAVIS_BRANCH diff --git a/cli/delete.go b/cli/delete.go index 2db13821e8..200fbe869e 100644 --- a/cli/delete.go +++ b/cli/delete.go @@ -45,7 +45,7 @@ EXAMPLE: } args := context.Args() - if args.Present() == false { + if !args.Present() { return fmt.Errorf("Missing container ID, should at least provide one") } diff --git a/cli/kata-check.go b/cli/kata-check.go index 22c615fcf7..edab3a8f58 100644 --- a/cli/kata-check.go +++ b/cli/kata-check.go @@ -243,6 +243,7 @@ func checkKernelModules(modules map[string]kernelModule, handler kernelParamHand // genericHostIsVMContainerCapable checks to see if the host is theoretically capable // of creating a VM container. +//nolint: unused,deadcode func genericHostIsVMContainerCapable(details vmContainerCapableDetails) error { cpuinfo, err := getCPUInfo(details.cpuInfoFile) if err != nil { diff --git a/cli/kata-check_amd64_test.go b/cli/kata-check_amd64_test.go index 8cb06167fb..efec707390 100644 --- a/cli/kata-check_amd64_test.go +++ b/cli/kata-check_amd64_test.go @@ -400,50 +400,50 @@ func TestArchKernelParamHandler(t *testing.T) { type testData struct { onVMM bool + expectIgnore bool fields logrus.Fields msg string - expectIgnore bool } data := []testData{ - {true, logrus.Fields{}, "", false}, - {false, logrus.Fields{}, "", false}, + {true, false, logrus.Fields{}, ""}, + {false, false, logrus.Fields{}, ""}, { + false, false, logrus.Fields{ // wrong type "parameter": 123, }, "foo", - false, }, { + false, false, logrus.Fields{ "parameter": "unrestricted_guest", }, "", - false, }, { + true, true, logrus.Fields{ "parameter": "unrestricted_guest", }, "", - true, }, { false, + true, logrus.Fields{ "parameter": "nested", }, "", - true, }, } diff --git a/cli/kata-check_ppc64le_test.go b/cli/kata-check_ppc64le_test.go index e0267d768f..29859c30cb 100644 --- a/cli/kata-check_ppc64le_test.go +++ b/cli/kata-check_ppc64le_test.go @@ -124,50 +124,50 @@ func TestArchKernelParamHandler(t *testing.T) { type testData struct { onVMM bool + expectIgnore bool fields logrus.Fields msg string - expectIgnore bool } data := []testData{ - {true, logrus.Fields{}, "", false}, - {false, logrus.Fields{}, "", false}, + {true, false, logrus.Fields{}, ""}, + {false, false, logrus.Fields{}, ""}, { + false, false, logrus.Fields{ // wrong type "parameter": 123, }, "foo", - false, }, { + false, false, logrus.Fields{ "parameter": "unrestricted_guest", }, "", - false, }, { + true, true, logrus.Fields{ "parameter": "unrestricted_guest", }, "", - true, }, { false, + true, logrus.Fields{ "parameter": "nested", }, "", - true, }, } diff --git a/cli/kill.go b/cli/kill.go index 97b3a549b2..4c93ecf02f 100644 --- a/cli/kill.go +++ b/cli/kill.go @@ -46,7 +46,7 @@ EXAMPLE: } args := context.Args() - if args.Present() == false { + if !args.Present() { return fmt.Errorf("Missing container ID") } diff --git a/cli/oci.go b/cli/oci.go index 2e6677cb32..8ffac2df4a 100644 --- a/cli/oci.go +++ b/cli/oci.go @@ -97,7 +97,7 @@ func validCreateParams(ctx context.Context, containerID, bundlePath string) (str if err != nil { return "", fmt.Errorf("Invalid bundle path '%s': %s", bundlePath, err) } - if fileInfo.IsDir() == false { + if !fileInfo.IsDir() { return "", fmt.Errorf("Invalid bundle path '%s', it should be a directory", bundlePath) } diff --git a/cli/ps.go b/cli/ps.go index dd2f3c8ca3..93aa4f3327 100644 --- a/cli/ps.go +++ b/cli/ps.go @@ -35,7 +35,7 @@ var psCLICommand = cli.Command{ return err } - if context.Args().Present() == false { + if !context.Args().Present() { return fmt.Errorf("Missing container ID, should at least provide one") } diff --git a/cli/start.go b/cli/start.go index 9ab495e324..249cc1faa3 100644 --- a/cli/start.go +++ b/cli/start.go @@ -34,7 +34,7 @@ var startCLICommand = cli.Command{ } args := context.Args() - if args.Present() == false { + if !args.Present() { return fmt.Errorf("Missing container ID, should at least provide one") } diff --git a/cli/update.go b/cli/update.go index edbe5262b2..21a2e69e87 100644 --- a/cli/update.go +++ b/cli/update.go @@ -137,7 +137,7 @@ other options are ignored. span, _ := katautils.Trace(ctx, "update") defer span.Finish() - if context.Args().Present() == false { + if !context.Args().Present() { return fmt.Errorf("Missing container ID, should at least provide one") } diff --git a/containerd-shim-v2/metrics.go b/containerd-shim-v2/metrics.go index a07486c1c2..43633d1f64 100644 --- a/containerd-shim-v2/metrics.go +++ b/containerd-shim-v2/metrics.go @@ -42,9 +42,7 @@ func statsToMetrics(cgStats *vc.CgroupStats) *cgroups.Metrics { } var perCPU []uint64 - for _, v := range cgStats.CPUStats.CPUUsage.PercpuUsage { - perCPU = append(perCPU, v) - } + perCPU = append(perCPU, cgStats.CPUStats.CPUUsage.PercpuUsage...) metrics := &cgroups.Metrics{ Hugetlb: hugetlb, diff --git a/containerd-shim-v2/service.go b/containerd-shim-v2/service.go index 7ed3971ae3..95e7abd0d6 100644 --- a/containerd-shim-v2/service.go +++ b/containerd-shim-v2/service.go @@ -592,7 +592,7 @@ func (s *service) Pause(ctx context.Context, r *taskAPI.PauseRequest) (*ptypes.E } s.send(&eventstypes.TaskPaused{ - c.id, + ContainerID: c.id, }) return empty, err @@ -620,7 +620,7 @@ func (s *service) Resume(ctx context.Context, r *taskAPI.ResumeRequest) (*ptypes } s.send(&eventstypes.TaskResumed{ - c.id, + ContainerID: c.id, }) return empty, err @@ -838,8 +838,6 @@ func (s *service) checkProcesses(e exit) { ExitStatus: uint32(e.status), ExitedAt: e.timestamp, }) - - return } func (s *service) getContainer(id string) (*container, error) { diff --git a/containerd-shim-v2/utils.go b/containerd-shim-v2/utils.go index c768cf131f..812e9f6ca9 100644 --- a/containerd-shim-v2/utils.go +++ b/containerd-shim-v2/utils.go @@ -102,7 +102,7 @@ func validBundle(containerID, bundlePath string) (string, error) { if err != nil { return "", fmt.Errorf("Invalid bundle path '%s': %s", bundlePath, err) } - if fileInfo.IsDir() == false { + if !fileInfo.IsDir() { return "", fmt.Errorf("Invalid bundle path '%s', it should be a directory", bundlePath) } diff --git a/pkg/katautils/config_test.go b/pkg/katautils/config_test.go index d33ab2958e..d8c81116b3 100644 --- a/pkg/katautils/config_test.go +++ b/pkg/katautils/config_test.go @@ -941,13 +941,13 @@ func TestHypervisorDefaults(t *testing.T) { assert.Equal(h.defaultVCPUs(), uint32(numCPUs), "default vCPU number is wrong") h.DefaultMaxVCPUs = 2 - assert.Equal(h.defaultMaxVCPUs(), uint32(h.DefaultMaxVCPUs), "default max vCPU number is wrong") + assert.Equal(h.defaultMaxVCPUs(), uint32(2), "default max vCPU number is wrong") h.DefaultMaxVCPUs = uint32(numCPUs) + 1 assert.Equal(h.defaultMaxVCPUs(), uint32(numCPUs), "default max vCPU number is wrong") maxvcpus := vc.MaxQemuVCPUs() - h.DefaultMaxVCPUs = uint32(maxvcpus) + 1 + h.DefaultMaxVCPUs = maxvcpus + 1 assert.Equal(h.defaultMaxVCPUs(), uint32(numCPUs), "default max vCPU number is wrong") h.MemorySize = 1024 @@ -1398,7 +1398,7 @@ func TestUpdateRuntimeConfigurationVMConfig(t *testing.T) { Hypervisor: map[string]hypervisor{ qemuHypervisorTableType: { NumVCPUs: int32(vcpus), - MemorySize: uint32(mem), + MemorySize: mem, Path: "/", Kernel: "/", Image: "/", @@ -1586,18 +1586,18 @@ func TestCheckFactoryConfig(t *testing.T) { type testData struct { factoryEnabled bool + expectError bool imagePath string initrdPath string - expectError bool } data := []testData{ - {false, "", "", false}, - {false, "image", "", false}, - {false, "", "initrd", false}, + {false, false, "", ""}, + {false, false, "image", ""}, + {false, false, "", "initrd"}, - {true, "", "initrd", false}, - {true, "image", "", true}, + {true, false, "", "initrd"}, + {true, true, "image", ""}, } for i, d := range data { @@ -1626,19 +1626,19 @@ func TestCheckNetNsConfigShimTrace(t *testing.T) { assert := assert.New(t) type testData struct { - disableNetNs bool networkModel vc.NetInterworkingModel + disableNetNs bool shimTrace bool expectError bool } data := []testData{ - {false, vc.NetXConnectMacVtapModel, false, false}, - {false, vc.NetXConnectMacVtapModel, true, true}, - {true, vc.NetXConnectMacVtapModel, true, true}, - {true, vc.NetXConnectMacVtapModel, false, true}, - {true, vc.NetXConnectNoneModel, false, false}, - {true, vc.NetXConnectNoneModel, true, false}, + {vc.NetXConnectMacVtapModel, false, false, false}, + {vc.NetXConnectMacVtapModel, false, true, true}, + {vc.NetXConnectMacVtapModel, true, true, true}, + {vc.NetXConnectMacVtapModel, true, false, true}, + {vc.NetXConnectNoneModel, true, false, false}, + {vc.NetXConnectNoneModel, true, true, false}, } for i, d := range data { diff --git a/versions.yaml b/versions.yaml index 717b7a0259..4a66e8f78b 100644 --- a/versions.yaml +++ b/versions.yaml @@ -217,6 +217,11 @@ externals: .*/v?([\d\.]+)\.tar\.gz version: "v2.0.5" + golangci-lint: + description: "utility to run various golang linters" + url: "https://install.goreleaser.com/github.com/golangci/golangci-lint.sh" + version: "v1.15.0" + kubernetes: description: "Kubernetes project container manager" url: "https://github.com/kubernetes/kubernetes" diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 337d07dec7..5a991fe4ec 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -628,7 +628,7 @@ func newContainer(sandbox *Sandbox, contConfig ContainerConfig) (*Container, err span, _ := sandbox.trace("newContainer") defer span.Finish() - if contConfig.valid() == false { + if !contConfig.valid() { return &Container{}, fmt.Errorf("Invalid container configuration") } @@ -1187,10 +1187,7 @@ func (c *Container) hotplugDrive() error { // isDriveUsed checks if a drive has been used for container rootfs func (c *Container) isDriveUsed() bool { - if c.state.Fstype == "" { - return false - } - return true + return !(c.state.Fstype == "") } func (c *Container) removeDrive() (err error) { diff --git a/virtcontainers/device/drivers/generic_test.go b/virtcontainers/device/drivers/generic_test.go index 5b0c28484f..e80f6accb7 100644 --- a/virtcontainers/device/drivers/generic_test.go +++ b/virtcontainers/device/drivers/generic_test.go @@ -13,20 +13,20 @@ import ( func TestBumpAttachCount(t *testing.T) { type testData struct { - attach bool attachCount uint expectedAC uint + attach bool expectSkip bool expectErr bool } data := []testData{ - {true, 0, 0, false, false}, - {true, 1, 2, true, false}, - {true, intMax, intMax, true, true}, - {false, 0, 0, true, true}, - {false, 1, 1, false, false}, - {false, intMax, intMax - 1, true, false}, + {0, 0, true, false, false}, + {1, 2, true, true, false}, + {intMax, intMax, true, true, true}, + {0, 0, false, true, true}, + {1, 1, false, false, false}, + {intMax, intMax - 1, false, true, false}, } dev := &GenericDevice{} diff --git a/virtcontainers/device/manager/manager.go b/virtcontainers/device/manager/manager.go index 9086fa455b..f2566432b9 100644 --- a/virtcontainers/device/manager/manager.go +++ b/virtcontainers/device/manager/manager.go @@ -191,7 +191,7 @@ func (dm *deviceManager) DetachDevice(id string, dr api.DeviceReceiver) error { if !ok { return ErrDeviceNotExist } - if d.GetAttachCount() <= 0 { + if d.GetAttachCount() == 0 { return ErrDeviceNotAttached } diff --git a/virtcontainers/device/manager/utils.go b/virtcontainers/device/manager/utils.go index 0a63537600..e5ccb99715 100644 --- a/virtcontainers/device/manager/utils.go +++ b/virtcontainers/device/manager/utils.go @@ -33,9 +33,5 @@ func isVFIO(hostPath string) bool { // isBlock checks if the device is a block device. func isBlock(devInfo config.DeviceInfo) bool { - if devInfo.DevType == "b" { - return true - } - - return false + return devInfo.DevType == "b" } diff --git a/virtcontainers/example_pod_run_test.go b/virtcontainers/example_pod_run_test.go index 5b93479079..bd3cb22101 100644 --- a/virtcontainers/example_pod_run_test.go +++ b/virtcontainers/example_pod_run_test.go @@ -68,6 +68,4 @@ func Example_createAndStartSandbox() { if err != nil { fmt.Printf("Could not run sandbox: %s", err) } - - return } diff --git a/virtcontainers/fc.go b/virtcontainers/fc.go index c197e80a15..6c5c06a670 100644 --- a/virtcontainers/fc.go +++ b/virtcontainers/fc.go @@ -217,7 +217,7 @@ func (fc *firecracker) waitVMM(timeout int) error { return nil } - if int(time.Now().Sub(timeStart).Seconds()) > timeout { + if int(time.Since(timeStart).Seconds()) > timeout { return fmt.Errorf("Failed to connect to firecrackerinstance (timeout %ds): %v", timeout, err) } @@ -624,7 +624,6 @@ func (fc *firecracker) addDevice(devInfo interface{}, devType deviceType) error return fc.fcAddVsock(v) default: fc.Logger().WithField("unknown-device-type", devInfo).Error("Adding device") - break } return nil diff --git a/virtcontainers/hack/virtc/main.go b/virtcontainers/hack/virtc/main.go index 2c25d43031..67a35e6a36 100644 --- a/virtcontainers/hack/virtc/main.go +++ b/virtcontainers/hack/virtc/main.go @@ -140,17 +140,17 @@ func buildSandboxConfig(context *cli.Context) (vc.SandboxConfig, error) { machineType := context.String("machine-type") vmMemory := context.Uint("vm-memory") agentType, ok := context.Generic("agent").(*vc.AgentType) - if ok != true { + if !ok { return vc.SandboxConfig{}, fmt.Errorf("Could not convert agent type") } proxyType, ok := context.Generic("proxy").(*vc.ProxyType) - if ok != true { + if !ok { return vc.SandboxConfig{}, fmt.Errorf("Could not convert proxy type") } shimType, ok := context.Generic("shim").(*vc.ShimType) - if ok != true { + if !ok { return vc.SandboxConfig{}, fmt.Errorf("Could not convert shim type") } diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index 49885081a0..24c56debf5 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -66,15 +66,9 @@ const ( // NetDev is the network device type. netDev - // SerialDev is the serial device type. - serialDev // nolint: varcheck,unused - // BlockDev is the block device type. blockDev - // ConsoleDev is the console device type. - consoleDev // nolint: varcheck,unused - // SerialPortDev is the serial port device type. serialPortDev @@ -407,11 +401,7 @@ func (conf *HypervisorConfig) assetPath(t types.AssetType) (string, error) { func (conf *HypervisorConfig) isCustomAsset(t types.AssetType) bool { _, ok := conf.customAssets[t] - if ok { - return true - } - - return false + return ok } // KernelAssetPath returns the guest kernel path @@ -476,12 +466,12 @@ func SerializeParams(params []Param, delim string) []string { if p.Key == "" && p.Value == "" { continue } else if p.Key == "" { - parameters = append(parameters, fmt.Sprintf("%s", p.Value)) + parameters = append(parameters, fmt.Sprint(p.Value)) } else if p.Value == "" { - parameters = append(parameters, fmt.Sprintf("%s", p.Key)) + parameters = append(parameters, fmt.Sprint(p.Key)) } else if delim == "" { - parameters = append(parameters, fmt.Sprintf("%s", p.Key)) - parameters = append(parameters, fmt.Sprintf("%s", p.Value)) + parameters = append(parameters, fmt.Sprint(p.Key)) + parameters = append(parameters, fmt.Sprint(p.Value)) } else { parameters = append(parameters, fmt.Sprintf("%s%s%s", p.Key, delim, p.Value)) } diff --git a/virtcontainers/hypervisor_test.go b/virtcontainers/hypervisor_test.go index 1a012b9bab..cc4d903c2b 100644 --- a/virtcontainers/hypervisor_test.go +++ b/virtcontainers/hypervisor_test.go @@ -444,7 +444,7 @@ type testNestedVMMData struct { expected bool } -// nolint: unused +// nolint: unused,deadcode func genericTestRunningOnVMM(t *testing.T, data []testNestedVMMData) { for _, d := range data { f, err := ioutil.TempFile("", "cpuinfo") diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 84352d84b7..0e62803218 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -801,7 +801,7 @@ func constraintGRPCSpec(grpcSpec *grpc.Spec, systemdCgroup bool, passSeccomp boo // Pass seccomp only if disable_guest_seccomp is set to false in // configuration.toml and guest image is seccomp capable. - if passSeccomp == false { + if !passSeccomp { grpcSpec.Linux.Seccomp = nil } diff --git a/virtcontainers/pkg/hyperstart/hyperstart.go b/virtcontainers/pkg/hyperstart/hyperstart.go index 893af0b9c6..6a55c775c6 100644 --- a/virtcontainers/pkg/hyperstart/hyperstart.go +++ b/virtcontainers/pkg/hyperstart/hyperstart.go @@ -261,7 +261,7 @@ func (h *Hyperstart) IsStarted() bool { h.SetDeadline(time.Time{}) - if ret == false { + if !ret { h.CloseSockets() } @@ -423,7 +423,7 @@ func (h *Hyperstart) SendIoMessage(ttyMsg *TtyMessage) error { // CodeFromCmd translates a string command to its corresponding code. func (h *Hyperstart) CodeFromCmd(cmd string) (uint32, error) { _, ok := CodeList[cmd] - if ok == false { + if !ok { return math.MaxUint32, fmt.Errorf("unknown command '%s'", cmd) } diff --git a/virtcontainers/pkg/mock/mock.go b/virtcontainers/pkg/mock/mock.go index 69c66aa9e4..d8b03ea08b 100644 --- a/virtcontainers/pkg/mock/mock.go +++ b/virtcontainers/pkg/mock/mock.go @@ -90,14 +90,14 @@ func StartShim(config ShimMockConfig) error { } // Print some traces to stdout - fmt.Fprintf(os.Stdout, ShimStdoutOutput) + fmt.Fprint(os.Stdout, ShimStdoutOutput) os.Stdout.Close() // Print some traces to stderr - fmt.Fprintf(os.Stderr, ShimStderrOutput) + fmt.Fprint(os.Stderr, ShimStderrOutput) os.Stderr.Close() - fmt.Fprintf(f, "INFO: Shim exited properly\n") + fmt.Fprint(f, "INFO: Shim exited properly\n") return nil } diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 269edc9620..9bea6203b0 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -79,7 +79,7 @@ const ( // Refer to: https://github.com/opencontainers/runtime-spec/commit/37391fb type CompatOCIProcess struct { spec.Process - Capabilities interface{} `json:"capabilities,omitempty" platform:"linux"` + Capabilities interface{} `json:"capabilities,omitempty" platform:"linux"` //nolint:govet } // CompatOCISpec is a structure inheriting from spec.Spec defined @@ -89,7 +89,7 @@ type CompatOCIProcess struct { // Refer to: https://github.com/opencontainers/runtime-spec/commit/37391fb type CompatOCISpec struct { spec.Spec - Process *CompatOCIProcess `json:"process,omitempty"` + Process *CompatOCIProcess `json:"process,omitempty"` //nolint:govet } // FactoryConfig is a structure to set the VM factory configuration. diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 808227e913..8258be2fb6 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -122,11 +122,7 @@ func newQMPLogger() qmpLogger { } func (l qmpLogger) V(level int32) bool { - if level != 0 { - return true - } - - return false + return level != 0 } func (l qmpLogger) Infof(format string, v ...interface{}) { @@ -615,7 +611,7 @@ func (q *qemu) waitSandbox(timeout int) error { break } - if int(time.Now().Sub(timeStart).Seconds()) > timeout { + if int(time.Since(timeStart).Seconds()) > timeout { return fmt.Errorf("Failed to connect to QEMU instance (timeout %ds): %v", timeout, err) } diff --git a/virtcontainers/qemu_arch_base.go b/virtcontainers/qemu_arch_base.go index 7b6bdc05bc..4200ea22cb 100644 --- a/virtcontainers/qemu_arch_base.go +++ b/virtcontainers/qemu_arch_base.go @@ -287,9 +287,7 @@ func (q *qemuArchBase) appendConsole(devices []govmmQemu.Device, path string) [] devices = append(devices, serial) - var console govmmQemu.CharDevice - - console = govmmQemu.CharDevice{ + console := govmmQemu.CharDevice{ Driver: govmmQemu.Console, Backend: govmmQemu.Socket, DeviceID: "console0", diff --git a/virtcontainers/qemu_arch_base_test.go b/virtcontainers/qemu_arch_base_test.go index 2a5f551afe..2d8cba11ea 100644 --- a/virtcontainers/qemu_arch_base_test.go +++ b/virtcontainers/qemu_arch_base_test.go @@ -120,15 +120,15 @@ func TestQemuArchBaseKernelParameters(t *testing.T) { qemuArchBase := newQemuArchBase() // with debug params - expectedParams := []Param(qemuArchBaseKernelParams) - debugParams := []Param(qemuArchBaseKernelParamsDebug) + expectedParams := qemuArchBaseKernelParams + debugParams := qemuArchBaseKernelParamsDebug expectedParams = append(expectedParams, debugParams...) p := qemuArchBase.kernelParameters(true) assert.Equal(expectedParams, p) // with non-debug params - expectedParams = []Param(qemuArchBaseKernelParams) - nonDebugParams := []Param(qemuArchBaseKernelParamsNonDebug) + expectedParams = qemuArchBaseKernelParams + nonDebugParams := qemuArchBaseKernelParamsNonDebug expectedParams = append(expectedParams, nonDebugParams...) p = qemuArchBase.kernelParameters(false) assert.Equal(expectedParams, p) diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index b0fc242e12..fef07b3de8 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -195,7 +195,7 @@ func (s *Sandbox) Logger() *logrus.Entry { // Annotations returns any annotation that a user could have stored through the sandbox. func (s *Sandbox) Annotations(key string) (string, error) { value, exist := s.config.Annotations[key] - if exist == false { + if !exist { return "", fmt.Errorf("Annotations key %s does not exist", key) } @@ -478,7 +478,7 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor span, ctx := trace(ctx, "newSandbox") defer span.Finish() - if sandboxConfig.valid() == false { + if !sandboxConfig.valid() { return nil, fmt.Errorf("Invalid sandbox configuration") } @@ -1029,7 +1029,7 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro s.config.Containers = append(s.config.Containers, contConfig) // Sandbox is reponsable to update VM resources needed by Containers - s.updateResources() + err = s.updateResources() if err != nil { return nil, err } diff --git a/virtcontainers/store/vc.go b/virtcontainers/store/vc.go index b51cf725aa..6906428da0 100644 --- a/virtcontainers/store/vc.go +++ b/virtcontainers/store/vc.go @@ -300,9 +300,5 @@ func ContainerRuntimeRootPath(sandboxID, containerID string) string { // VCSandboxStoreExists returns true if a sandbox store already exists. func VCSandboxStoreExists(ctx context.Context, sandboxID string) bool { s := stores.findStore(SandboxConfigurationRoot(sandboxID)) - if s != nil { - return true - } - - return false + return s != nil } diff --git a/virtcontainers/types.go b/virtcontainers/types.go index 91dc2f0a7c..82cde0b511 100644 --- a/virtcontainers/types.go +++ b/virtcontainers/types.go @@ -18,9 +18,5 @@ const ( // IsSandbox determines if the container type can be considered as a sandbox. // We can consider a sandbox in case we have a PodSandbox or a RegularContainer. func (cType ContainerType) IsSandbox() bool { - if cType == PodSandbox { - return true - } - - return false + return cType == PodSandbox } diff --git a/virtcontainers/types/capabilities.go b/virtcontainers/types/capabilities.go index c361601ef5..fbd86a6209 100644 --- a/virtcontainers/types/capabilities.go +++ b/virtcontainers/types/capabilities.go @@ -20,10 +20,7 @@ type Capabilities struct { // IsBlockDeviceSupported tells if an hypervisor supports block devices. func (caps *Capabilities) IsBlockDeviceSupported() bool { - if caps.flags&blockDeviceSupport != 0 { - return true - } - return false + return caps.flags&blockDeviceSupport != 0 } // SetBlockDeviceSupport sets the block device support capability to true. @@ -33,10 +30,7 @@ func (caps *Capabilities) SetBlockDeviceSupport() { // IsBlockDeviceHotplugSupported tells if an hypervisor supports hotplugging block devices. func (caps *Capabilities) IsBlockDeviceHotplugSupported() bool { - if caps.flags&blockDeviceHotplugSupport != 0 { - return true - } - return false + return caps.flags&blockDeviceHotplugSupport != 0 } // SetBlockDeviceHotplugSupport sets the block device hotplugging capability to true. @@ -46,10 +40,7 @@ func (caps *Capabilities) SetBlockDeviceHotplugSupport() { // IsMultiQueueSupported tells if an hypervisor supports device multi queue support. func (caps *Capabilities) IsMultiQueueSupported() bool { - if caps.flags&multiQueueSupport != 0 { - return true - } - return false + return caps.flags&multiQueueSupport != 0 } // SetMultiQueueSupport sets the device multi queue capability to true. From d9c275878ba354fae484615978a1cb70b2a6e68e Mon Sep 17 00:00:00 2001 From: Penny Zheng Date: Thu, 28 Mar 2019 12:41:03 +0800 Subject: [PATCH 7/7] linter: remove deadcode linter check for generic item After we switched golang linter to golangci-lint, we has extra 'deadcode' linter check, and we need to remove this linter check for all generic items. Fixes: #1432 Signed-off-by: Penny Zheng (cherry picked from commit 2e5194e2795ae997a5d2ca0406fff6dd8bee3770) --- cli/kata-check.go | 6 +++--- cli/kata-check_test.go | 6 +++--- cli/kata-env_test.go | 2 +- virtcontainers/hypervisor_test.go | 4 ++-- virtcontainers/qemu.go | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cli/kata-check.go b/cli/kata-check.go index edab3a8f58..26bc9f25e0 100644 --- a/cli/kata-check.go +++ b/cli/kata-check.go @@ -52,9 +52,9 @@ const ( kernelPropertyCorrect = "Kernel property value correct" // these refer to fields in the procCPUINFO file - genericCPUFlagsTag = "flags" // nolint: varcheck, unused - genericCPUVendorField = "vendor_id" // nolint: varcheck, unused - genericCPUModelField = "model name" // nolint: varcheck, unused + genericCPUFlagsTag = "flags" // nolint: varcheck, unused, deadcode + genericCPUVendorField = "vendor_id" // nolint: varcheck, unused, deadcode + genericCPUModelField = "model name" // nolint: varcheck, unused, deadcode ) // variables rather than consts to allow tests to modify them diff --git a/cli/kata-check_test.go b/cli/kata-check_test.go index 6fcf3aa230..c9dbd1cbb2 100644 --- a/cli/kata-check_test.go +++ b/cli/kata-check_test.go @@ -27,14 +27,14 @@ type testModuleData struct { contents string } -// nolint: structcheck, unused +// nolint: structcheck, unused, deadcode type testCPUData struct { vendorID string flags string expectError bool } -// nolint: structcheck, unused +// nolint: structcheck, unused, deadcode type testCPUDetail struct { contents string expectedVendor string @@ -147,7 +147,7 @@ func makeCPUInfoFile(path, vendorID, flags string) error { return ioutil.WriteFile(path, contents.Bytes(), testFileMode) } -// nolint: unused +// nolint: unused, deadcode func genericTestGetCPUDetails(t *testing.T, validVendor string, validModel string, validContents string, data []testCPUDetail) { tmpdir, err := ioutil.TempDir("", "") if err != nil { diff --git a/cli/kata-env_test.go b/cli/kata-env_test.go index cf4b0e8894..7ee8d2e8fb 100644 --- a/cli/kata-env_test.go +++ b/cli/kata-env_test.go @@ -244,7 +244,7 @@ func getExpectedAgentDetails(config oci.RuntimeConfig) (AgentInfo, error) { }, nil } -// nolint: unused +// nolint: unused, deadcode func genericGetExpectedHostDetails(tmpdir string, expectedVendor string, expectedModel string, expectedVMContainerCapable bool) (HostInfo, error) { type filesToCreate struct { file string diff --git a/virtcontainers/hypervisor_test.go b/virtcontainers/hypervisor_test.go index cc4d903c2b..27f0bc78ee 100644 --- a/virtcontainers/hypervisor_test.go +++ b/virtcontainers/hypervisor_test.go @@ -437,14 +437,14 @@ func TestGetHostMemorySizeKb(t *testing.T) { } } -// nolint: unused +// nolint: unused, deadcode type testNestedVMMData struct { content []byte expectedErr bool expected bool } -// nolint: unused,deadcode +// nolint: unused, deadcode func genericTestRunningOnVMM(t *testing.T, data []testNestedVMMData) { for _, d := range data { f, err := ioutil.TempFile("", "cpuinfo") diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 8258be2fb6..3280b04845 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -1425,7 +1425,7 @@ func (q *qemu) resizeMemory(reqMemMB uint32, memoryBlockSizeMB uint32) (uint32, } // genericAppendBridges appends to devices the given bridges -// nolint: unused +// nolint: unused, deadcode func genericAppendBridges(devices []govmmQemu.Device, bridges []types.PCIBridge, machineType string) []govmmQemu.Device { bus := defaultPCBridgeBus switch machineType { @@ -1457,7 +1457,7 @@ func genericAppendBridges(devices []govmmQemu.Device, bridges []types.PCIBridge, return devices } -// nolint: unused +// nolint: unused, deadcode func genericBridges(number uint32, machineType string) []types.PCIBridge { var bridges []types.PCIBridge var bt types.PCIType @@ -1490,7 +1490,7 @@ func genericBridges(number uint32, machineType string) []types.PCIBridge { return bridges } -// nolint: unused +// nolint: unused, deadcode func genericMemoryTopology(memoryMb, hostMemoryMb uint64, slots uint8, memoryOffset uint32) govmmQemu.Memory { // image NVDIMM device needs memory space 1024MB // See https://github.com/clearcontainers/runtime/issues/380