persist: address comments

Address some comments.

Signed-off-by: Wei Zhang <weizhang555@gmail.com>

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 {

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 {

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,

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-")
+	fs.TestSetStorageRootPath(filepath.Join(testDir, "vc"))
+
+	defer func() {
+		os.RemoveAll(testDir)
+		fs.TestSetStorageRootPath(rootPathSave)
+	}()
+
 	assert.Nil(err)
-	store.VCStorePrefix = testDir
+
+	runPathSave := fs.RunStoragePath()
+	fs.TestSetRunStoragePath(filepath.Join(testDir, "vc", "run"))
+
 	defer func() {
 		os.RemoveAll(testDir)
-		store.VCStorePrefix = ""
+		fs.TestSetRunStoragePath(runPathSave)
 	}()
 
 	hyperConfig := vc.HypervisorConfig{

virtcontainers/factory/factory_test.go

@@ -9,17 +9,21 @@ 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"
 )
 
 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,

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,

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

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

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.RemoveAll(tempRoot)
 
-	os.Exit(m.Run())
+	os.Exit(ret)
 }
 
 func TestAddAssetAnnotations(t *testing.T) {

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.