From 4a298cb9b74c02852b36257ce3dca6c3419c0f0d Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Sun, 1 Dec 2019 20:39:02 +0800 Subject: [PATCH] persist: address comments Address some comments. Signed-off-by: Wei Zhang --- virtcontainers/acrn.go | 2 +- virtcontainers/clh_test.go | 4 +- virtcontainers/factory/cache/cache_test.go | 10 +++++ virtcontainers/factory/direct/direct_test.go | 21 +++++++++-- virtcontainers/factory/factory_test.go | 37 +++++++++++++++++++ .../factory/template/template_test.go | 11 ++++++ virtcontainers/persist/fs/fs.go | 16 +++++--- virtcontainers/persist/fs/fs_test.go | 22 +---------- virtcontainers/pkg/oci/utils_test.go | 31 ++++++++++++---- virtcontainers/virtcontainers_test.go | 5 ++- 10 files changed, 117 insertions(+), 42 deletions(-) diff --git a/virtcontainers/acrn.go b/virtcontainers/acrn.go index ac3ba20ec0..1b1c7f7df4 100644 --- a/virtcontainers/acrn.go +++ b/virtcontainers/acrn.go @@ -805,7 +805,7 @@ func (a *Acrn) storeInfo() error { jsonOut, err := json.Marshal(a.info) if err != nil { - return fmt.Errorf("Could not marshall data: %s", err) + return fmt.Errorf("Could not marshal data: %s", err) } if err := store.GlobalWrite(relPath, jsonOut); err != nil { diff --git a/virtcontainers/clh_test.go b/virtcontainers/clh_test.go index 380d1073b6..0e96186f01 100644 --- a/virtcontainers/clh_test.go +++ b/virtcontainers/clh_test.go @@ -13,8 +13,8 @@ import ( "testing" "github.com/kata-containers/runtime/virtcontainers/device/config" + "github.com/kata-containers/runtime/virtcontainers/persist/fs" chclient "github.com/kata-containers/runtime/virtcontainers/pkg/cloud-hypervisor/client" - "github.com/kata-containers/runtime/virtcontainers/store" "github.com/pkg/errors" "github.com/stretchr/testify/assert" ) @@ -184,7 +184,7 @@ func TestCloudHypervisorCleanupVM(t *testing.T) { t.Errorf("cloudHypervisor.cleanupVM() expected error != %v", err) } - dir := filepath.Join(store.RunVMStoragePath(), clh.id) + dir := filepath.Join(fs.RunVMStoragePath(), clh.id) os.MkdirAll(dir, os.ModePerm) if err := clh.cleanupVM(false); err != nil { diff --git a/virtcontainers/factory/cache/cache_test.go b/virtcontainers/factory/cache/cache_test.go index 5d8ab24ca6..8489fe42d9 100644 --- a/virtcontainers/factory/cache/cache_test.go +++ b/virtcontainers/factory/cache/cache_test.go @@ -8,6 +8,7 @@ package cache import ( "context" "io/ioutil" + "os" "path/filepath" "testing" @@ -18,10 +19,19 @@ import ( "github.com/kata-containers/runtime/virtcontainers/persist/fs" ) +var rootPathSave = fs.StorageRootPath() + func TestTemplateFactory(t *testing.T) { assert := assert.New(t) testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() + hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, diff --git a/virtcontainers/factory/direct/direct_test.go b/virtcontainers/factory/direct/direct_test.go index 5ca7df2986..20eec0eb89 100644 --- a/virtcontainers/factory/direct/direct_test.go +++ b/virtcontainers/factory/direct/direct_test.go @@ -9,23 +9,36 @@ import ( "context" "io/ioutil" "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" vc "github.com/kata-containers/runtime/virtcontainers" - "github.com/kata-containers/runtime/virtcontainers/store" + "github.com/kata-containers/runtime/virtcontainers/persist/fs" ) +var rootPathSave = fs.StorageRootPath() + func TestTemplateFactory(t *testing.T) { assert := assert.New(t) testDir, err := ioutil.TempDir("", "vmfactory-tmp-") - assert.Nil(err) - store.VCStorePrefix = testDir + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + defer func() { os.RemoveAll(testDir) - store.VCStorePrefix = "" + fs.TestSetStorageRootPath(rootPathSave) + }() + + assert.Nil(err) + + runPathSave := fs.RunStoragePath() + fs.TestSetRunStoragePath(filepath.Join(testDir, "vc", "run")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetRunStoragePath(runPathSave) }() hyperConfig := vc.HypervisorConfig{ diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go index e04fee41ef..91e0a2c32f 100644 --- a/virtcontainers/factory/factory_test.go +++ b/virtcontainers/factory/factory_test.go @@ -9,10 +9,12 @@ import ( "context" "io/ioutil" "os" + "path/filepath" "testing" vc "github.com/kata-containers/runtime/virtcontainers" "github.com/kata-containers/runtime/virtcontainers/factory/base" + "github.com/kata-containers/runtime/virtcontainers/persist/fs" "github.com/kata-containers/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -20,6 +22,8 @@ import ( const testDisabledAsNonRoot = "Test disabled as requires root privileges" +var rootPathSave = fs.StorageRootPath() + func TestNewFactory(t *testing.T) { var config Config @@ -41,6 +45,12 @@ func TestNewFactory(t *testing.T) { assert.Error(err) testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, @@ -110,6 +120,12 @@ func TestVMConfigValid(t *testing.T) { assert := assert.New(t) testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() config := vc.VMConfig{ HypervisorType: vc.MockHypervisor, @@ -159,6 +175,13 @@ func TestCheckVMConfig(t *testing.T) { assert.Nil(err) testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() + config1.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, @@ -178,6 +201,13 @@ func TestFactoryGetVM(t *testing.T) { assert := assert.New(t) testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() + hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, @@ -337,6 +367,13 @@ func TestDeepCompare(t *testing.T) { ProxyType: vc.NoopProxyType, } testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() + config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go index 3f61ba3a37..150d81e2b6 100644 --- a/virtcontainers/factory/template/template_test.go +++ b/virtcontainers/factory/template/template_test.go @@ -9,16 +9,20 @@ import ( "context" "io/ioutil" "os" + "path/filepath" "testing" "time" "github.com/stretchr/testify/assert" vc "github.com/kata-containers/runtime/virtcontainers" + "github.com/kata-containers/runtime/virtcontainers/persist/fs" ) const testDisabledAsNonRoot = "Test disabled as requires root privileges" +var rootPathSave = fs.StorageRootPath() + func TestTemplateFactory(t *testing.T) { if os.Geteuid() != 0 { t.Skip(testDisabledAsNonRoot) @@ -29,6 +33,13 @@ func TestTemplateFactory(t *testing.T) { templateWaitForAgent = 1 * time.Microsecond testDir, _ := ioutil.TempDir("", "vmfactory-tmp-") + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) + + defer func() { + os.RemoveAll(testDir) + fs.TestSetStorageRootPath(rootPathSave) + }() + hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, ImagePath: testDir, diff --git a/virtcontainers/persist/fs/fs.go b/virtcontainers/persist/fs/fs.go index aa124d7c1b..33acebecd6 100644 --- a/virtcontainers/persist/fs/fs.go +++ b/virtcontainers/persist/fs/fs.go @@ -69,6 +69,12 @@ func TestSetRunStoragePath(path string) { } } +func TestSetStorageRootPath(path string) { + StorageRootPath = func() string { + return path + } +} + // FS storage driver implementation type FS struct { sandboxState *persistapi.SandboxState @@ -319,7 +325,7 @@ func (fs *FS) GlobalWrite(relativePath string, data []byte) error { _, err = os.Stat(dir) if os.IsNotExist(err) { if err := os.MkdirAll(dir, dirMode); err != nil { - fs.Logger().WithError(err).Errorf("failed to create dir %q", dir) + fs.Logger().WithError(err).WithField("directory", dir).Error("failed to create dir") return err } } else if err != nil { @@ -328,13 +334,13 @@ func (fs *FS) GlobalWrite(relativePath string, data []byte) error { f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, fileMode) if err != nil { - fs.Logger().WithError(err).Errorf("failed to open file %q for writting", path) + fs.Logger().WithError(err).WithField("file", path).Error("failed to open file for writting") return err } defer f.Close() if _, err := f.Write(data); err != nil { - fs.Logger().WithError(err).Errorf("failed to write file %q: %v ", path, err) + fs.Logger().WithError(err).WithField("file", path).Error("failed to write file") return err } return nil @@ -349,14 +355,14 @@ func (fs *FS) GlobalRead(relativePath string) ([]byte, error) { f, err := os.Open(path) if err != nil { - fs.Logger().WithError(err).Errorf("failed to open file %q for reading", path) + fs.Logger().WithError(err).WithField("file", path).Error("failed to open file for reading") return nil, err } defer f.Close() data, err := ioutil.ReadAll(f) if err != nil { - fs.Logger().WithError(err).Errorf("failed to read file %q: %v ", path, err) + fs.Logger().WithError(err).WithField("file", path).Error("failed to read file") return nil, err } return data, nil diff --git a/virtcontainers/persist/fs/fs_test.go b/virtcontainers/persist/fs/fs_test.go index 72c836cba0..431baa9a04 100644 --- a/virtcontainers/persist/fs/fs_test.go +++ b/virtcontainers/persist/fs/fs_test.go @@ -33,16 +33,12 @@ func initTestDir() func() { testDir, _ := ioutil.TempDir("", "vc-tmp-") // allow the tests to run without affecting the host system. rootSave := StorageRootPath() - StorageRootPath = func() string { - return filepath.Join(testDir, "vc") - } + TestSetStorageRootPath(filepath.Join(testDir, "vc")) os.MkdirAll(testDir, dirMode) return func() { - StorageRootPath = func() string { - return rootSave - } + TestSetStorageRootPath(rootSave) os.RemoveAll(testDir) } } @@ -54,13 +50,6 @@ func TestFsLockShared(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, fs) - testDir, err := ioutil.TempDir("", "fs-tmp-") - assert.Nil(t, err) - TestSetRunStoragePath(testDir) - defer func() { - os.RemoveAll(testDir) - }() - sid := "test-fs-driver" fs.sandboxState.SandboxContainer = sid sandboxDir, err := fs.sandboxDir(sid) @@ -116,13 +105,6 @@ func TestFsDriver(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, fs) - testDir, err := ioutil.TempDir("", "fs-tmp-") - assert.Nil(t, err) - TestSetRunStoragePath(testDir) - defer func() { - os.RemoveAll(testDir) - }() - ss := persistapi.SandboxState{} cs := make(map[string]persistapi.ContainerState) // missing sandbox container id diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index 592cf67d5b..49976fcf52 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -29,11 +29,15 @@ import ( ) const ( - tempBundlePath = "/tmp/virtc/ocibundle/" - containerID = "virtc-oci-test" - consolePath = "/tmp/virtc/console" - fileMode = os.FileMode(0640) - dirMode = os.FileMode(0750) + containerID = "virtc-oci-test" + fileMode = os.FileMode(0640) + dirMode = os.FileMode(0750) +) + +var ( + tempRoot = "" + tempBundlePath = "" + consolePath = "" ) func createConfig(fileName string, fileData string) (string, error) { @@ -633,16 +637,27 @@ func TestGetShmSizeBindMounted(t *testing.T) { } func TestMain(m *testing.M) { + var err error + tempRoot, err = ioutil.TempDir("", "virtc-") + if err != nil { + panic(err) + } + + tempBundlePath = filepath.Join(tempRoot, "ocibundle") + consolePath = filepath.Join(tempRoot, "console") + /* Create temp bundle directory if necessary */ - err := os.MkdirAll(tempBundlePath, dirMode) + err = os.MkdirAll(tempBundlePath, dirMode) if err != nil { fmt.Printf("Unable to create %s %v\n", tempBundlePath, err) os.Exit(1) } - defer os.RemoveAll(tempBundlePath) + ret := m.Run() - os.Exit(m.Run()) + os.RemoveAll(tempRoot) + + os.Exit(ret) } func TestAddAssetAnnotations(t *testing.T) { diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go index 7bda400692..a690e438c3 100644 --- a/virtcontainers/virtcontainers_test.go +++ b/virtcontainers/virtcontainers_test.go @@ -51,8 +51,6 @@ var testVirtiofsdPath = "" var testHyperstartCtlSocket = "" var testHyperstartTtySocket = "" -var savedRunVMStoragePathFunc func() string - // cleanUp Removes any stale sandbox/container state that can affect // the next test to run. func cleanUp() { @@ -160,10 +158,13 @@ func TestMain(m *testing.M) { // allow the tests to run without affecting the host system. runPathSave := fs.RunStoragePath() + rootPathSave := fs.StorageRootPath() fs.TestSetRunStoragePath(filepath.Join(testDir, "vc", "run")) + fs.TestSetStorageRootPath(filepath.Join(testDir, "vc")) defer func() { fs.TestSetRunStoragePath(runPathSave) + fs.TestSetStorageRootPath(rootPathSave) }() // set now that configStoragePath has been overridden.