From 1e365c1e1314ac407fe4ee155c18775dbc613d44 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 18 Sep 2018 12:14:42 +0100 Subject: [PATCH 1/2] CI: Fix linter errors Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep the `maligned` linter happy. Previously: `` virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned) virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned) ``` Signed-off-by: James O. D. Hunt (cherry picked from part of commit 526d55b4af796f0c6330f639cc956739c7574093) --- virtcontainers/monitor.go | 2 +- virtcontainers/pkg/mock/cc_proxy_mock.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/virtcontainers/monitor.go b/virtcontainers/monitor.go index fdf523a7d6..8e2a879746 100644 --- a/virtcontainers/monitor.go +++ b/virtcontainers/monitor.go @@ -18,9 +18,9 @@ type monitor struct { sandbox *Sandbox checkInterval time.Duration watchers []chan error + wg sync.WaitGroup running bool stopCh chan bool - wg sync.WaitGroup } func newMonitor(s *Sandbox) *monitor { diff --git a/virtcontainers/pkg/mock/cc_proxy_mock.go b/virtcontainers/pkg/mock/cc_proxy_mock.go index 052612ade8..23dd7f980d 100644 --- a/virtcontainers/pkg/mock/cc_proxy_mock.go +++ b/virtcontainers/pkg/mock/cc_proxy_mock.go @@ -25,7 +25,6 @@ type CCProxyMock struct { sync.Mutex t *testing.T - wg sync.WaitGroup connectionPath string // proxy socket @@ -44,6 +43,8 @@ type CCProxyMock struct { ShimDisconnected chan bool StdinReceived chan bool + wg sync.WaitGroup + stopped bool } From dbd78dab406aa5425faf13525bd8e352aed59a84 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 16 Oct 2018 11:11:31 +0100 Subject: [PATCH 2/2] create/run: Make bundle path default to cwd The bundle path was documented as defaulting to the current directory but was not being set to that value if not explicitly specified. Also moved factory creation code to a new `handleFactory()` function to avoid cyclomatic complexity issues. Fixes #821. (cherry picked from commit 8831245e30bc6d0e3c2e88f465016b4a05e7bf9a) Signed-off-by: James O. D. Hunt --- cli/create.go | 65 ++++++++++++++++++++++++++++++---------------- cli/create_test.go | 8 ++++++ cli/run_test.go | 44 ++++++++++++++++++++++++++----- 3 files changed, 88 insertions(+), 29 deletions(-) diff --git a/cli/create.go b/cli/create.go index dbbf2e4455..0bbc288ca6 100644 --- a/cli/create.go +++ b/cli/create.go @@ -92,6 +92,36 @@ var createCLICommand = cli.Command{ // Use a variable to allow tests to modify its value var getKernelParamsFunc = getKernelParams +func handleFactory(ctx context.Context, runtimeConfig oci.RuntimeConfig) { + if !runtimeConfig.FactoryConfig.Template { + return + } + + factoryConfig := vf.Config{ + Template: true, + VMConfig: vc.VMConfig{ + HypervisorType: runtimeConfig.HypervisorType, + HypervisorConfig: runtimeConfig.HypervisorConfig, + AgentType: runtimeConfig.AgentType, + AgentConfig: runtimeConfig.AgentConfig, + }, + } + + kataLog.WithField("factory", factoryConfig).Info("load vm factory") + + f, err := vf.NewFactory(ctx, factoryConfig, true) + if err != nil { + kataLog.WithError(err).Warn("load vm factory failed, about to create new one") + f, err = vf.NewFactory(ctx, factoryConfig, false) + if err != nil { + kataLog.WithError(err).Warn("create vm factory failed") + return + } + } + + vci.SetFactory(ctx, f) +} + func create(ctx context.Context, containerID, bundlePath, console, pidFilePath string, detach, systemdCgroup bool, runtimeConfig oci.RuntimeConfig) error { var err error @@ -103,6 +133,17 @@ func create(ctx context.Context, containerID, bundlePath, console, pidFilePath s setExternalLoggers(ctx, kataLog) span.SetTag("container", containerID) + if bundlePath == "" { + cwd, err := os.Getwd() + if err != nil { + return err + } + + kataLog.WithField("directory", cwd).Debug("Defaulting bundle path to current directory") + + bundlePath = cwd + } + // Checks the MUST and MUST NOT from OCI runtime specification if bundlePath, err = validCreateParams(ctx, containerID, bundlePath); err != nil { return err @@ -118,29 +159,7 @@ func create(ctx context.Context, containerID, bundlePath, console, pidFilePath s return err } - if runtimeConfig.FactoryConfig.Template { - factoryConfig := vf.Config{ - Template: true, - VMConfig: vc.VMConfig{ - HypervisorType: runtimeConfig.HypervisorType, - HypervisorConfig: runtimeConfig.HypervisorConfig, - AgentType: runtimeConfig.AgentType, - AgentConfig: runtimeConfig.AgentConfig, - }, - } - kataLog.WithField("factory", factoryConfig).Info("load vm factory") - f, err := vf.NewFactory(ctx, factoryConfig, true) - if err != nil { - kataLog.WithError(err).Warn("load vm factory failed, about to create new one") - f, err = vf.NewFactory(ctx, factoryConfig, false) - if err != nil { - kataLog.WithError(err).Warn("create vm factory failed") - } - } - if err == nil { - vci.SetFactory(ctx, f) - } - } + handleFactory(ctx, runtimeConfig) disableOutput := noNeedForOutput(detach, ociSpec.Process.Terminal) diff --git a/cli/create_test.go b/cli/create_test.go index 930f6832f0..287773e29d 100644 --- a/cli/create_test.go +++ b/cli/create_test.go @@ -13,6 +13,7 @@ import ( "io/ioutil" "os" "path/filepath" + "regexp" "strings" "testing" @@ -561,6 +562,13 @@ func TestCreateProcessCgroupsPathSuccessful(t *testing.T) { err = writeOCIConfigFile(spec, ociConfigFile) assert.NoError(err) + err = create(context.Background(), testContainerID, "", testConsole, pidFilePath, false, true, runtimeConfig) + assert.Error(err, "bundle path not set") + + re := regexp.MustCompile("config.json.*no such file or directory") + matches := re.FindAllStringSubmatch(err.Error(), -1) + assert.NotEmpty(matches) + for _, detach := range []bool{true, false} { err := create(context.Background(), testContainerID, bundlePath, testConsole, pidFilePath, detach, true, runtimeConfig) assert.NoError(err, "detached: %+v", detach) diff --git a/cli/run_test.go b/cli/run_test.go index 362e563897..2154f9db14 100644 --- a/cli/run_test.go +++ b/cli/run_test.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "testing" vc "github.com/kata-containers/runtime/virtcontainers" @@ -290,13 +291,44 @@ func TestRunContainerSuccessful(t *testing.T) { testingImpl.DeleteContainerFunc = nil }() - err = run(context.Background(), d.sandbox.ID(), d.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) + type errorTestArgs struct { + bundlePath string + + // regex string for text of error message + errorRE string + + // If true, expect a cli.ExitError, else expect any other type + // of error. + expectExitError bool + } + + args := []errorTestArgs{ + {"", "config.json: no such file or directory", false}, + {d.bundlePath, "", true}, + } - // should return ExitError with the message and exit code - e, ok := err.(*cli.ExitError) - assert.True(ok, "error should be a cli.ExitError: %s", err) - assert.Empty(e.Error()) - assert.NotZero(e.ExitCode()) + for i, a := range args { + err = run(context.Background(), d.sandbox.ID(), a.bundlePath, d.consolePath, "", d.pidFilePath, false, true, d.runtimeConfig) + assert.Errorf(err, "test args %d (%+v)", i, a) + + if a.errorRE == "" { + assert.Empty(err.Error()) + } else { + re := regexp.MustCompile(a.errorRE) + matches := re.FindAllStringSubmatch(err.Error(), -1) + assert.NotEmpty(matches) + } + + e, ok := err.(*cli.ExitError) + + if a.expectExitError { + // should return ExitError with the message and exit code + assert.Truef(ok, "test args %d (%+v): error should be a cli.ExitError: %s", i, a, err) + assert.NotZero(e.ExitCode()) + } else { + assert.Falsef(ok, "test args %d (%+v): error should not be a cli.ExitError: %s", i, a, err) + } + } } func TestRunContainerDetachSuccessful(t *testing.T) {