From 2eeb5dc2235d43d0cc1a7b8c4c8411621d398cce Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 14:39:06 +1000 Subject: [PATCH 1/9] runtime: Don't use fixed /tmp/mountPoint path Several tests in kata_agent_test.go create /tmp/mountPoint as a dummy directory to mount. This is not cleaned up after the test. Although it is in /tmp, that's still a little messy and can be confusing to a user. In addition, because it uses the same name every time, it allows for one run of the test to interfere with the next. Use the built in t.TempDir() to use an automatically named and deleted temporary directory instead. Signed-off-by: David Gibson --- src/runtime/virtcontainers/kata_agent_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/kata_agent_test.go b/src/runtime/virtcontainers/kata_agent_test.go index af26edc0c6..f4d996736a 100644 --- a/src/runtime/virtcontainers/kata_agent_test.go +++ b/src/runtime/virtcontainers/kata_agent_test.go @@ -191,8 +191,7 @@ func TestKataAgentSendReq(t *testing.T) { func TestHandleEphemeralStorage(t *testing.T) { k := kataAgent{} var ociMounts []specs.Mount - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() mount := specs.Mount{ Type: KataEphemeralDevType, @@ -212,8 +211,7 @@ func TestHandleEphemeralStorage(t *testing.T) { func TestHandleLocalStorage(t *testing.T) { k := kataAgent{} var ociMounts []specs.Mount - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() mount := specs.Mount{ Type: KataLocalDevType, @@ -688,8 +686,7 @@ func TestHandleShm(t *testing.T) { // In case the type of mount is ephemeral, the container mount is not // shared with the sandbox shm. ociMounts[0].Type = KataEphemeralDevType - mountSource := "/tmp/mountPoint" - os.Mkdir(mountSource, 0755) + mountSource := t.TempDir() ociMounts[0].Source = mountSource k.handleShm(ociMounts, sandbox) From 90b2f5b7760d80134e4c93869e53669f5e870717 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 19:10:10 +1000 Subject: [PATCH 2/9] runtime: Make SetupOCIConfigFile clean up after itself SetupOCIConfigFile creates a temporary directory with os.MkDirTemp(). This means the callers need to register a deferred function to remove it again. At least one of them was commented out meaning that a /temp/katatest- directory was leftover after the unit tests ran. Change to using t.TempDir() which as well as better matching other parts of the tests means the testing framework will handle cleaning it up. Signed-off-by: David Gibson --- src/runtime/pkg/containerd-shim-v2/create_test.go | 6 ------ src/runtime/pkg/containerd-shim-v2/delete_test.go | 5 ++--- src/runtime/pkg/containerd-shim-v2/service_test.go | 3 +-- src/runtime/pkg/katatestutils/utils.go | 5 ++--- src/runtime/pkg/katautils/create_test.go | 12 +++--------- 5 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/runtime/pkg/containerd-shim-v2/create_test.go b/src/runtime/pkg/containerd-shim-v2/create_test.go index 40ffde59f0..994de7c436 100644 --- a/src/runtime/pkg/containerd-shim-v2/create_test.go +++ b/src/runtime/pkg/containerd-shim-v2/create_test.go @@ -50,7 +50,6 @@ func TestCreateSandboxSuccess(t *testing.T) { }() tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - // defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -99,7 +98,6 @@ func TestCreateSandboxFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -137,7 +135,6 @@ func TestCreateSandboxConfigFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -187,7 +184,6 @@ func TestCreateContainerSuccess(t *testing.T) { } tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -227,7 +223,6 @@ func TestCreateContainerFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -278,7 +273,6 @@ func TestCreateContainerConfigFail(t *testing.T) { }() tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) diff --git a/src/runtime/pkg/containerd-shim-v2/delete_test.go b/src/runtime/pkg/containerd-shim-v2/delete_test.go index f84f5e596e..7d959e15a5 100644 --- a/src/runtime/pkg/containerd-shim-v2/delete_test.go +++ b/src/runtime/pkg/containerd-shim-v2/delete_test.go @@ -7,7 +7,6 @@ package containerdshim import ( - "os" "testing" taskAPI "github.com/containerd/containerd/runtime/v2/task" @@ -25,8 +24,8 @@ func TestDeleteContainerSuccessAndFail(t *testing.T) { MockID: testSandboxID, } - rootPath, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(rootPath) + _, bundlePath, _ := ktu.SetupOCIConfigFile(t) + _, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) diff --git a/src/runtime/pkg/containerd-shim-v2/service_test.go b/src/runtime/pkg/containerd-shim-v2/service_test.go index b501df99cc..f895b4e852 100644 --- a/src/runtime/pkg/containerd-shim-v2/service_test.go +++ b/src/runtime/pkg/containerd-shim-v2/service_test.go @@ -41,8 +41,7 @@ func TestServiceCreate(t *testing.T) { assert := assert.New(t) - tmpdir, bundleDir, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundleDir, _ := ktu.SetupOCIConfigFile(t) ctx := context.Background() diff --git a/src/runtime/pkg/katatestutils/utils.go b/src/runtime/pkg/katatestutils/utils.go index 7750c23deb..527c9cfbc7 100644 --- a/src/runtime/pkg/katatestutils/utils.go +++ b/src/runtime/pkg/katatestutils/utils.go @@ -346,11 +346,10 @@ func IsInGitHubActions() bool { func SetupOCIConfigFile(t *testing.T) (rootPath string, bundlePath, ociConfigFile string) { assert := assert.New(t) - tmpdir, err := os.MkdirTemp("", "katatest-") - assert.NoError(err) + tmpdir := t.TempDir() bundlePath = filepath.Join(tmpdir, "bundle") - err = os.MkdirAll(bundlePath, testDirMode) + err := os.MkdirAll(bundlePath, testDirMode) assert.NoError(err) ociConfigFile = filepath.Join(bundlePath, "config.json") diff --git a/src/runtime/pkg/katautils/create_test.go b/src/runtime/pkg/katautils/create_test.go index 56e7fa27a0..15b4561137 100644 --- a/src/runtime/pkg/katautils/create_test.go +++ b/src/runtime/pkg/katautils/create_test.go @@ -212,7 +212,6 @@ func TestCreateSandboxConfigFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -246,7 +245,6 @@ func TestCreateSandboxFail(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -269,7 +267,6 @@ func TestCreateSandboxAnnotations(t *testing.T) { assert := assert.New(t) tmpdir, bundlePath, _ := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) runtimeConfig, err := newTestRuntimeConfig(tmpdir, testConsole, true) assert.NoError(err) @@ -342,8 +339,7 @@ func TestCheckForFips(t *testing.T) { func TestCreateContainerContainerConfigFail(t *testing.T) { assert := assert.New(t) - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) @@ -370,8 +366,7 @@ func TestCreateContainerContainerConfigFail(t *testing.T) { func TestCreateContainerFail(t *testing.T) { assert := assert.New(t) - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) @@ -405,8 +400,7 @@ func TestCreateContainer(t *testing.T) { mockSandbox.CreateContainerFunc = nil }() - tmpdir, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) - defer os.RemoveAll(tmpdir) + _, bundlePath, ociConfigFile := ktu.SetupOCIConfigFile(t) spec, err := compatoci.ParseConfigJSON(bundlePath) assert.NoError(err) From f7ba21c86fc78fc36699204f4333df49995d5ebf Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Apr 2022 21:01:28 +1000 Subject: [PATCH 3/9] runtime: Clean up mock hook logs in tests The tests in hook_test.go run a mock hook binary, which does some debug logging to /tmp/mock_hook.log. Currently we don't clean up those logs when the tests are done. Use a test cleanup function to do this. Signed-off-by: David Gibson --- src/runtime/pkg/katautils/hook_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/runtime/pkg/katautils/hook_test.go b/src/runtime/pkg/katautils/hook_test.go index 9ef48499bb..b7bb03409c 100644 --- a/src/runtime/pkg/katautils/hook_test.go +++ b/src/runtime/pkg/katautils/hook_test.go @@ -22,6 +22,7 @@ var testContainerIDHook = "test-container-id" var testControllerIDHook = "test-controller-id" var testBinHookPath = "mockhook/hook" var testBundlePath = "/test/bundle" +var mockHookLogFile = "/tmp/mock_hook.log" func getMockHookBinPath() string { return testBinHookPath @@ -49,12 +50,17 @@ func createWrongHook() specs.Hook { } } +func cleanMockHookLogFile() { + _ = os.Remove(mockHookLogFile) +} + func TestRunHook(t *testing.T) { if tc.NotValid(ktu.NeedRoot()) { t.Skip(ktu.TestDisabledNeedRoot) } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() spec := specs.Spec{} @@ -87,6 +93,7 @@ func TestPreStartHooks(t *testing.T) { } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() @@ -129,6 +136,7 @@ func TestPostStartHooks(t *testing.T) { } assert := assert.New(t) + t.Cleanup(cleanMockHookLogFile) ctx := context.Background() @@ -173,6 +181,7 @@ func TestPostStopHooks(t *testing.T) { assert := assert.New(t) ctx := context.Background() + t.Cleanup(cleanMockHookLogFile) // Hooks field is nil spec := specs.Spec{} From bec59f9e398247d3946443c6c57f731f185a9b03 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 12:10:59 +1000 Subject: [PATCH 4/9] runtime: Make bind mount tests better clean up after themselves There are several tests in mount_test.go which perform a sample bind mount. These need a corresponding unmount to clean up afterwards or attempting to delete the temporary files will fail due to the existing mountpoint. Most of them had such an unmount, but TestBindMountInvalidPgtypes was missing one. In addition, the existing unmounts where done inconsistently - one was simply inline (so wouldn't be executed if the test fails too early) and one is a defer. Change them all to use the t.Cleanup mechanism. For the dummy mountpoint files, rather than cleaning them up after the test, the tests were removing them at the beginning of the test. That stops the test being messed up by a previous run, but messily. Since these are created in a private temporary directory anyway, if there's something already there, that indicates a problem we shouldn't ignore. In fact we don't need to explicitly remove these at all - they'll be removed along with the rest of the private temporary directory. Signed-off-by: David Gibson --- src/runtime/virtcontainers/mount_test.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/runtime/virtcontainers/mount_test.go b/src/runtime/virtcontainers/mount_test.go index b658de8295..056fa2c140 100644 --- a/src/runtime/virtcontainers/mount_test.go +++ b/src/runtime/virtcontainers/mount_test.go @@ -381,6 +381,12 @@ func TestBindMountFailingMount(t *testing.T) { assert.Error(err) } +func cleanupFooMount() { + dest := filepath.Join(testDir, "fooDirDest") + + syscall.Unmount(dest, 0) +} + func TestBindMountSuccessful(t *testing.T) { assert := assert.New(t) if tc.NotValid(ktu.NeedRoot()) { @@ -389,9 +395,7 @@ func TestBindMountSuccessful(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) @@ -401,8 +405,6 @@ func TestBindMountSuccessful(t *testing.T) { err = bindMount(context.Background(), source, dest, false, "private") assert.NoError(err) - - syscall.Unmount(dest, 0) } func TestBindMountReadonlySuccessful(t *testing.T) { @@ -413,9 +415,7 @@ func TestBindMountReadonlySuccessful(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) @@ -426,8 +426,6 @@ func TestBindMountReadonlySuccessful(t *testing.T) { err = bindMount(context.Background(), source, dest, true, "private") assert.NoError(err) - defer syscall.Unmount(dest, 0) - // should not be able to create file in read-only mount destFile := filepath.Join(dest, "foo") _, err = os.OpenFile(destFile, os.O_CREATE, mountPerm) @@ -442,9 +440,7 @@ func TestBindMountInvalidPgtypes(t *testing.T) { source := filepath.Join(testDir, "fooDirSrc") dest := filepath.Join(testDir, "fooDirDest") - syscall.Unmount(dest, 0) - os.Remove(source) - os.Remove(dest) + t.Cleanup(cleanupFooMount) err := os.MkdirAll(source, mountPerm) assert.NoError(err) From 1719a8b49152d4ee5f3e944324fd9dd1f70bbbb9 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:29:06 +1000 Subject: [PATCH 5/9] runtime: Don't abuse MockStorageRootPath() for factory tests A number of unit tests under virtcontainers/factory use MockStorageRootPath() as a general purpose temporary directory. This doesn't make sense: the mockfs driver isn't even in use here since we only call EnableMockTesting for the pase virtcontainers package, not the subpackages. Instead use t.TempDir() which is for exactly this purpose. As a bonus it also handles the cleanup, so we don't need MockStorageDestroy any more. Signed-off-by: David Gibson --- .../virtcontainers/factory/cache/cache_test.go | 4 +--- .../factory/direct/direct_test.go | 4 +--- .../virtcontainers/factory/factory_test.go | 18 +++++++----------- .../factory/template/template_test.go | 4 +--- .../virtcontainers/persist/fs/mockfs.go | 4 ---- 5 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/runtime/virtcontainers/factory/cache/cache_test.go b/src/runtime/virtcontainers/factory/cache/cache_test.go index d0971ef12b..9f2fa3a5b5 100644 --- a/src/runtime/virtcontainers/factory/cache/cache_test.go +++ b/src/runtime/virtcontainers/factory/cache/cache_test.go @@ -13,14 +13,12 @@ import ( vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/factory/direct" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" ) func TestTemplateFactory(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/direct/direct_test.go b/src/runtime/virtcontainers/factory/direct/direct_test.go index 724b6cb66f..862fcb6365 100644 --- a/src/runtime/virtcontainers/factory/direct/direct_test.go +++ b/src/runtime/virtcontainers/factory/direct/direct_test.go @@ -12,14 +12,12 @@ import ( "github.com/stretchr/testify/assert" vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" ) func TestTemplateFactory(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/factory_test.go b/src/runtime/virtcontainers/factory/factory_test.go index 71af52d71e..80cff101df 100644 --- a/src/runtime/virtcontainers/factory/factory_test.go +++ b/src/runtime/virtcontainers/factory/factory_test.go @@ -12,7 +12,6 @@ import ( vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/factory/base" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/mock" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" @@ -39,10 +38,10 @@ func TestNewFactory(t *testing.T) { _, err = NewFactory(ctx, config, false) assert.Error(err) - defer fs.MockStorageDestroy() + testDir := t.TempDir() config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ - KernelPath: fs.MockStorageRootPath(), - ImagePath: fs.MockStorageRootPath(), + KernelPath: testDir, + ImagePath: testDir, } // direct @@ -69,7 +68,7 @@ func TestNewFactory(t *testing.T) { defer hybridVSockTTRPCMock.Stop() config.Template = true - config.TemplatePath = fs.MockStorageRootPath() + config.TemplatePath = testDir f, err = NewFactory(ctx, config, false) assert.Nil(err) f.CloseFactory(ctx) @@ -134,8 +133,7 @@ func TestCheckVMConfig(t *testing.T) { err = checkVMConfig(config1, config2) assert.Nil(err) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() config1.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, @@ -155,8 +153,7 @@ func TestCheckVMConfig(t *testing.T) { func TestFactoryGetVM(t *testing.T) { assert := assert.New(t) - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, @@ -321,8 +318,7 @@ func TestDeepCompare(t *testing.T) { config.VMConfig = vc.VMConfig{ HypervisorType: vc.MockHypervisor, } - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() config.VMConfig.HypervisorConfig = vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/factory/template/template_test.go b/src/runtime/virtcontainers/factory/template/template_test.go index 55c7bc5968..c067c793e6 100644 --- a/src/runtime/virtcontainers/factory/template/template_test.go +++ b/src/runtime/virtcontainers/factory/template/template_test.go @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/assert" vc "github.com/kata-containers/kata-containers/src/runtime/virtcontainers" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/mock" ) @@ -32,8 +31,7 @@ func TestTemplateFactory(t *testing.T) { templateWaitForAgent = 1 * time.Microsecond - testDir := fs.MockStorageRootPath() - defer fs.MockStorageDestroy() + testDir := t.TempDir() hyperConfig := vc.HypervisorConfig{ KernelPath: testDir, diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index f972782cf4..23e09c8c04 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -30,10 +30,6 @@ func MockRunVMStoragePath() string { return filepath.Join(MockStorageRootPath(), vmPathSuffix) } -func MockStorageDestroy() { - os.RemoveAll(MockStorageRootPath()) -} - func MockFSInit() (persistapi.PersistDriver, error) { driver, err := Init() if err != nil { From 963d03ea8a7cf9d9dc298c5c6c80e98a0dec7606 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 13:11:56 +1000 Subject: [PATCH 6/9] runtime: Export StoragePathSuffix storagePathSuffix defines the file path suffix - "vc" - used for Kata's persistent storage information, as a private constant. We duplicate this information in fc.go which also needs it. Export it from fs.go instead, so it can be used in fc.go. Signed-off-by: David Gibson --- src/runtime/virtcontainers/fc.go | 5 ++--- src/runtime/virtcontainers/persist/fs/fs.go | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/fc.go b/src/runtime/virtcontainers/fc.go index 6137e32cf2..4630a810c8 100644 --- a/src/runtime/virtcontainers/fc.go +++ b/src/runtime/virtcontainers/fc.go @@ -27,6 +27,7 @@ import ( hv "github.com/kata-containers/kata-containers/src/runtime/pkg/hypervisors" "github.com/kata-containers/kata-containers/src/runtime/pkg/katautils/katatrace" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/config" + "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client" models "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/models" ops "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/firecracker/client/operations" @@ -84,8 +85,6 @@ const ( fcMetricsFifo = "metrics.fifo" defaultFcConfig = "fcConfig.json" - // storagePathSuffix mirrors persist/fs/fs.go:storagePathSuffix - storagePathSuffix = "vc" ) // Specify the minimum version of firecracker supported @@ -244,7 +243,7 @@ func (fc *firecracker) setPaths(hypervisorConfig *HypervisorConfig) { // /// hypervisorName := filepath.Base(hypervisorConfig.HypervisorPath) //fs.RunStoragePath cannot be used as we need exec perms - fc.chrootBaseDir = filepath.Join("/run", storagePathSuffix) + fc.chrootBaseDir = filepath.Join("/run", fs.StoragePathSuffix) fc.vmPath = filepath.Join(fc.chrootBaseDir, hypervisorName, fc.id) fc.jailerRoot = filepath.Join(fc.vmPath, "root") // auto created by jailer diff --git a/src/runtime/virtcontainers/persist/fs/fs.go b/src/runtime/virtcontainers/persist/fs/fs.go index 630ed76364..407f765568 100644 --- a/src/runtime/virtcontainers/persist/fs/fs.go +++ b/src/runtime/virtcontainers/persist/fs/fs.go @@ -29,11 +29,11 @@ const dirMode = os.FileMode(0700) | os.ModeDir // fileMode is the permission bits used for creating a file const fileMode = os.FileMode(0600) -// storagePathSuffix is the suffix used for all storage paths +// StoragePathSuffix is the suffix used for all storage paths // // Note: this very brief path represents "virtcontainers". It is as // terse as possible to minimise path length. -const storagePathSuffix = "vc" +const StoragePathSuffix = "vc" // sandboxPathSuffix is the suffix used for sandbox storage const sandboxPathSuffix = "sbs" @@ -64,7 +64,7 @@ func Init() (persistapi.PersistDriver, error) { return &FS{ sandboxState: &persistapi.SandboxState{}, containerState: make(map[string]persistapi.ContainerState), - storageRootPath: filepath.Join("/run", storagePathSuffix), + storageRootPath: filepath.Join("/run", StoragePathSuffix), driverName: "fs", }, nil } From 5d8438e9397c5f44ad460bbb695501c46b3616da Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 14:46:44 +1000 Subject: [PATCH 7/9] runtime: Move mockfs control global into mockfs.go virtcontainers/persist/fs/mockfs.go defines a mock filesystem type for testing. A global variable in virtcontainers/persist/manager.go is used to force use of the mock fs rather than a normal one. This patch moves the global, and the EnableMockTesting() function which sets it into mockfs.go. This is slightly cleaner to begin with, and will allow some further enhancements. Signed-off-by: David Gibson --- .../virtcontainers/persist/fs/mockfs.go | 13 +++++++ .../virtcontainers/persist/fs/mockfs_test.go | 34 +++++++++++++++++++ src/runtime/virtcontainers/persist/manager.go | 10 ++---- .../virtcontainers/persist/manager_test.go | 14 -------- .../virtcontainers/virtcontainers_test.go | 3 +- 5 files changed, 51 insertions(+), 23 deletions(-) create mode 100644 src/runtime/virtcontainers/persist/fs/mockfs_test.go diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index 23e09c8c04..4c4f492693 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -13,11 +13,17 @@ import ( persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" ) +var mockTesting = false + type MockFS struct { // inherit from FS. Overwrite if needed. *FS } +func EnableMockTesting() { + mockTesting = true +} + func MockStorageRootPath() string { return filepath.Join(os.TempDir(), "vc", "mockfs") } @@ -46,3 +52,10 @@ func MockFSInit() (persistapi.PersistDriver, error) { return &MockFS{fsDriver}, nil } + +func MockAutoInit() (persistapi.PersistDriver, error) { + if mockTesting { + return MockFSInit() + } + return nil, nil +} diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go new file mode 100644 index 0000000000..99709a78e6 --- /dev/null +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -0,0 +1,34 @@ +// Copyright Red Hat. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package fs + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMockAutoInit(t *testing.T) { + assert := assert.New(t) + orgMockTesting := mockTesting + defer func() { + mockTesting = orgMockTesting + }() + + mockTesting = false + + fsd, err := MockAutoInit() + assert.Nil(fsd) + assert.NoError(err) + + // Testing mock driver + mockTesting = true + fsd, err = MockAutoInit() + assert.NoError(err) + expectedFS, err := MockFSInit() + assert.NoError(err) + assert.Equal(expectedFS, fsd) +} diff --git a/src/runtime/virtcontainers/persist/manager.go b/src/runtime/virtcontainers/persist/manager.go index a104bdcabf..32504c932b 100644 --- a/src/runtime/virtcontainers/persist/manager.go +++ b/src/runtime/virtcontainers/persist/manager.go @@ -28,13 +28,8 @@ var ( RootFSName: fs.Init, RootlessFSName: fs.RootlessInit, } - mockTesting = false ) -func EnableMockTesting() { - mockTesting = true -} - // GetDriver returns new PersistDriver according to driver name func GetDriverByName(name string) (persistapi.PersistDriver, error) { if expErr != nil { @@ -56,8 +51,9 @@ func GetDriver() (persistapi.PersistDriver, error) { return nil, expErr } - if mockTesting { - return fs.MockFSInit() + mock, err := fs.MockAutoInit() + if mock != nil || err != nil { + return mock, err } if rootless.IsRootless() { diff --git a/src/runtime/virtcontainers/persist/manager_test.go b/src/runtime/virtcontainers/persist/manager_test.go index 074ca92665..4347f9adc2 100644 --- a/src/runtime/virtcontainers/persist/manager_test.go +++ b/src/runtime/virtcontainers/persist/manager_test.go @@ -27,12 +27,6 @@ func TestGetDriverByName(t *testing.T) { func TestGetDriver(t *testing.T) { assert := assert.New(t) - orgMockTesting := mockTesting - defer func() { - mockTesting = orgMockTesting - }() - - mockTesting = false fsd, err := GetDriver() assert.NoError(err) @@ -46,12 +40,4 @@ func TestGetDriver(t *testing.T) { assert.NoError(err) assert.Equal(expectedFS, fsd) - - // Testing mock driver - mockTesting = true - fsd, err = GetDriver() - assert.NoError(err) - expectedFS, err = fs.MockFSInit() - assert.NoError(err) - assert.Equal(expectedFS, fsd) } diff --git a/src/runtime/virtcontainers/virtcontainers_test.go b/src/runtime/virtcontainers/virtcontainers_test.go index cb03e2351b..6a3d7fa580 100644 --- a/src/runtime/virtcontainers/virtcontainers_test.go +++ b/src/runtime/virtcontainers/virtcontainers_test.go @@ -15,7 +15,6 @@ import ( "syscall" "testing" - "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/utils" "github.com/sirupsen/logrus" @@ -108,7 +107,7 @@ func setupClh() { func TestMain(m *testing.M) { var err error - persist.EnableMockTesting() + fs.EnableMockTesting() flag.Parse() From ef6d54a78106f2a1c8e8c25dae3bb91f2d76cf65 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:02:48 +1000 Subject: [PATCH 8/9] runtime: Let MockFSInit create a mock fs driver at any path Currently MockFSInit always creates the mockfs at the fixed path /tmp/vc/mockfs. This change allows it to be initialized at any path given as a parameter. This allows the tests in fs_test.go to be simplified, because the by using a temporary directory from t.TempDir(), which is automatically cleaned up, we don't need to manually trigger initTestDir() (which is misnamed, it's actually a cleanup function). For now we still use the fixed path when auto-creating the mockfs in MockAutoInit(), but we'll change that later. Signed-off-by: David Gibson --- .../virtcontainers/persist/fs/fs_test.go | 26 +++++-------------- .../virtcontainers/persist/fs/mockfs.go | 6 ++--- .../virtcontainers/persist/fs/mockfs_test.go | 2 +- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/runtime/virtcontainers/persist/fs/fs_test.go b/src/runtime/virtcontainers/persist/fs/fs_test.go index 91af4af159..370b87c527 100644 --- a/src/runtime/virtcontainers/persist/fs/fs_test.go +++ b/src/runtime/virtcontainers/persist/fs/fs_test.go @@ -14,8 +14,8 @@ import ( "github.com/stretchr/testify/assert" ) -func getFsDriver() (*FS, error) { - driver, err := MockFSInit() +func getFsDriver(t *testing.T) (*FS, error) { + driver, err := MockFSInit(t.TempDir()) if err != nil { return nil, fmt.Errorf("failed to init fs driver") } @@ -27,16 +27,8 @@ func getFsDriver() (*FS, error) { return fs.FS, nil } -func initTestDir() func() { - return func() { - os.RemoveAll(MockStorageRootPath()) - } -} - func TestFsLockShared(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -61,9 +53,7 @@ func TestFsLockShared(t *testing.T) { } func TestFsLockExclusive(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -89,9 +79,7 @@ func TestFsLockExclusive(t *testing.T) { } func TestFsDriver(t *testing.T) { - defer initTestDir()() - - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) @@ -162,12 +150,10 @@ func TestFsDriver(t *testing.T) { } func TestGlobalReadWrite(t *testing.T) { - defer initTestDir()() - relPath := "test/123/aaa.json" data := "hello this is testing global read write" - fs, err := getFsDriver() + fs, err := getFsDriver(t) assert.Nil(t, err) assert.NotNil(t, fs) diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index 4c4f492693..dca4acad42 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -36,7 +36,7 @@ func MockRunVMStoragePath() string { return filepath.Join(MockStorageRootPath(), vmPathSuffix) } -func MockFSInit() (persistapi.PersistDriver, error) { +func MockFSInit(rootPath string) (persistapi.PersistDriver, error) { driver, err := Init() if err != nil { return nil, fmt.Errorf("Could not create Mock FS driver: %v", err) @@ -47,7 +47,7 @@ func MockFSInit() (persistapi.PersistDriver, error) { return nil, fmt.Errorf("Could not create Mock FS driver") } - fsDriver.storageRootPath = MockStorageRootPath() + fsDriver.storageRootPath = rootPath fsDriver.driverName = "mockfs" return &MockFS{fsDriver}, nil @@ -55,7 +55,7 @@ func MockFSInit() (persistapi.PersistDriver, error) { func MockAutoInit() (persistapi.PersistDriver, error) { if mockTesting { - return MockFSInit() + return MockFSInit(MockStorageRootPath()) } return nil, nil } diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go index 99709a78e6..9cd5832895 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs_test.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -28,7 +28,7 @@ func TestMockAutoInit(t *testing.T) { mockTesting = true fsd, err = MockAutoInit() assert.NoError(err) - expectedFS, err := MockFSInit() + expectedFS, err := MockFSInit(MockStorageRootPath()) assert.NoError(err) assert.Equal(expectedFS, fsd) } From 1b931f420339f8849e98517b38ba609a7fa7c9d3 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 6 Apr 2022 15:12:49 +1000 Subject: [PATCH 9/9] runtime: Allock mockfs storage to be placed in any directory Currently EnableMockTesting() takes no arguments and will always place the mock storage in the fixed location /tmp/vc/mockfs. This means that one test run can interfere with the next one if anything isn't cleaned up (and there are other bugs which means that happens). If if those were fixed this would allow developers testing on the same machine to interfere with each other. So, allow the mockfs to be placed at an arbitrary place given as a parameter to EnableMockTesting(). In TestMain() we place it under our existing temporary directory, so we don't need any additional cleanup just for the mockfs. fixes #4140 Signed-off-by: David Gibson --- src/runtime/virtcontainers/persist/fs/mockfs.go | 14 ++++++++------ .../virtcontainers/persist/fs/mockfs_test.go | 8 ++++---- src/runtime/virtcontainers/virtcontainers_test.go | 6 ++---- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/runtime/virtcontainers/persist/fs/mockfs.go b/src/runtime/virtcontainers/persist/fs/mockfs.go index dca4acad42..18045f1ee5 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs.go @@ -7,25 +7,27 @@ package fs import ( "fmt" - "os" "path/filepath" persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api" ) -var mockTesting = false +var mockRootPath = "" type MockFS struct { // inherit from FS. Overwrite if needed. *FS } -func EnableMockTesting() { - mockTesting = true +func EnableMockTesting(rootPath string) { + mockRootPath = rootPath } func MockStorageRootPath() string { - return filepath.Join(os.TempDir(), "vc", "mockfs") + if mockRootPath == "" { + panic("Using uninitialized mock storage root path") + } + return mockRootPath } func MockRunStoragePath() string { @@ -54,7 +56,7 @@ func MockFSInit(rootPath string) (persistapi.PersistDriver, error) { } func MockAutoInit() (persistapi.PersistDriver, error) { - if mockTesting { + if mockRootPath != "" { return MockFSInit(MockStorageRootPath()) } return nil, nil diff --git a/src/runtime/virtcontainers/persist/fs/mockfs_test.go b/src/runtime/virtcontainers/persist/fs/mockfs_test.go index 9cd5832895..f2c1abd766 100644 --- a/src/runtime/virtcontainers/persist/fs/mockfs_test.go +++ b/src/runtime/virtcontainers/persist/fs/mockfs_test.go @@ -13,19 +13,19 @@ import ( func TestMockAutoInit(t *testing.T) { assert := assert.New(t) - orgMockTesting := mockTesting + orgMockRootPath := mockRootPath defer func() { - mockTesting = orgMockTesting + mockRootPath = orgMockRootPath }() - mockTesting = false + mockRootPath = "" fsd, err := MockAutoInit() assert.Nil(fsd) assert.NoError(err) // Testing mock driver - mockTesting = true + mockRootPath = t.TempDir() fsd, err = MockAutoInit() assert.NoError(err) expectedFS, err := MockFSInit(MockStorageRootPath()) diff --git a/src/runtime/virtcontainers/virtcontainers_test.go b/src/runtime/virtcontainers/virtcontainers_test.go index 6a3d7fa580..b7117000ce 100644 --- a/src/runtime/virtcontainers/virtcontainers_test.go +++ b/src/runtime/virtcontainers/virtcontainers_test.go @@ -57,8 +57,6 @@ var testHyperstartTtySocket = "" // cleanUp Removes any stale sandbox/container state that can affect // the next test to run. func cleanUp() { - os.RemoveAll(fs.MockRunStoragePath()) - os.RemoveAll(fs.MockRunVMStoragePath()) syscall.Unmount(GetSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow) os.RemoveAll(testDir) os.MkdirAll(testDir, DirMode) @@ -107,8 +105,6 @@ func setupClh() { func TestMain(m *testing.M) { var err error - fs.EnableMockTesting() - flag.Parse() logger := logrus.NewEntry(logrus.New()) @@ -125,6 +121,8 @@ func TestMain(m *testing.M) { panic(err) } + fs.EnableMockTesting(filepath.Join(testDir, "mockfs")) + fmt.Printf("INFO: Creating virtcontainers test directory %s\n", testDir) err = os.MkdirAll(testDir, DirMode) if err != nil {