From 29b55ab88b0b6e9fee91e4938428bcb0c9c94179 Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Mon, 25 Nov 2019 19:09:33 +0800 Subject: [PATCH] persist: remove VCStore from container Remove VCStore from container struct. Signed-off-by: Wei Zhang --- virtcontainers/api_test.go | 42 +++--- virtcontainers/container.go | 189 +++--------------------- virtcontainers/container_test.go | 17 --- virtcontainers/kata_agent.go | 7 - virtcontainers/persist/api/interface.go | 5 +- virtcontainers/persist/fs/fs.go | 106 +++++++------ virtcontainers/persist/fs/fs_test.go | 47 ++++-- virtcontainers/sandbox.go | 42 ++---- virtcontainers/sandbox_test.go | 44 ------ virtcontainers/store/vc.go | 6 - virtcontainers/virtcontainers_test.go | 2 +- 11 files changed, 152 insertions(+), 355 deletions(-) diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index 740b9f1819..85c1ba762e 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -139,7 +139,7 @@ func TestCreateSandboxNoopAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) } @@ -176,7 +176,7 @@ func TestCreateSandboxKataAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) } @@ -203,7 +203,7 @@ func TestDeleteSandboxNoopAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -248,7 +248,7 @@ func TestDeleteSandboxKataAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -264,7 +264,7 @@ func TestDeleteSandboxFailing(t *testing.T) { defer cleanUp() assert := assert.New(t) - sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) + sandboxDir := store.SandboxRuntimeRootPath(testSandboxID) os.Remove(sandboxDir) p, err := DeleteSandbox(context.Background(), testSandboxID) @@ -328,7 +328,7 @@ func TestStartSandboxFailing(t *testing.T) { defer cleanUp() assert := assert.New(t) - sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) + sandboxDir := store.SandboxRuntimeRootPath(testSandboxID) os.Remove(sandboxDir) p, err := StartSandbox(context.Background(), testSandboxID) @@ -395,7 +395,7 @@ func TestStopSandboxKataAgentSuccessful(t *testing.T) { func TestStopSandboxFailing(t *testing.T) { defer cleanUp() - sandboxDir := store.SandboxConfigurationRootPath(testSandboxID) + sandboxDir := store.SandboxRuntimeRootPath(testSandboxID) os.Remove(sandboxDir) p, err := StopSandbox(context.Background(), testSandboxID, false) @@ -413,7 +413,7 @@ func TestRunSandboxNoopAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) } @@ -451,7 +451,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -680,7 +680,7 @@ func TestCreateContainerSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -734,7 +734,7 @@ func TestDeleteContainerSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -778,7 +778,7 @@ func TestDeleteContainerFailingNoContainer(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -835,7 +835,7 @@ func TestStartContainerFailingNoContainer(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -856,7 +856,7 @@ func TestStartContainerFailingSandboxNotStarted(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -936,7 +936,7 @@ func TestStopContainerFailingNoContainer(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -1040,7 +1040,7 @@ func TestEnterContainerFailingNoContainer(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -1093,7 +1093,7 @@ func TestStatusContainerSuccessful(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -1136,7 +1136,7 @@ func TestStatusContainerStateReady(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -1199,7 +1199,7 @@ func TestStatusContainerStateRunning(t *testing.T) { assert.NoError(err) assert.NotNil(p) - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) assert.NoError(err) @@ -1414,7 +1414,7 @@ func createAndStartSandbox(ctx context.Context, config SandboxConfig) (sandbox V return nil, "", err } - sandboxDir = store.SandboxConfigurationRootPath(sandbox.ID()) + sandboxDir = store.SandboxRuntimeRootPath(sandbox.ID()) _, err = os.Stat(sandboxDir) if err != nil { return nil, "", err @@ -1699,7 +1699,7 @@ func TestCleanupContainer(t *testing.T) { CleanupContainer(ctx, p.ID(), c.ID(), true) } - sandboxDir := store.SandboxConfigurationRootPath(p.ID()) + sandboxDir := store.SandboxRuntimeRootPath(p.ID()) _, err = os.Stat(sandboxDir) if err == nil { diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 66f71fcb13..f9fe271d0a 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -338,8 +338,6 @@ type Container struct { systemMountsInfo SystemMountsInfo ctx context.Context - - store *store.VCStore } // ID returns the container identifier string. @@ -391,13 +389,6 @@ func (c *Container) GetPid() int { func (c *Container) setStateFstype(fstype string) error { c.state.Fstype = fstype - if !c.sandbox.supportNewStore() { - // experimental runtime use "persist.json" which doesn't need "state.json" anymore - if err := c.storeState(); err != nil { - return err - } - } - return nil } @@ -421,48 +412,10 @@ func (c *Container) GetPatchedOCISpec() *specs.Spec { // storeContainer stores a container config. func (c *Container) storeContainer() error { - if c.sandbox.supportNewStore() { - if err := c.sandbox.Save(); err != nil { - return err - } - return nil + if err := c.sandbox.Save(); err != nil { + return err } - return c.store.Store(store.Configuration, *(c.config)) -} - -func (c *Container) storeProcess() error { - return c.store.Store(store.Process, c.process) -} - -func (c *Container) storeMounts() error { - return c.store.Store(store.Mounts, c.mounts) -} - -func (c *Container) storeDevices() error { - return c.store.Store(store.DeviceIDs, c.devices) -} - -func (c *Container) storeState() error { - return c.store.Store(store.State, c.state) -} - -func (c *Container) loadMounts() ([]Mount, error) { - var mounts []Mount - if err := c.store.Load(store.Mounts, &mounts); err != nil { - return []Mount{}, err - } - - return mounts, nil -} - -func (c *Container) loadDevices() ([]ContainerDevice, error) { - var devices []ContainerDevice - - if err := c.store.Load(store.DeviceIDs, &devices); err != nil { - return []ContainerDevice{}, err - } - - return devices, nil + return nil } // setContainerState sets both the in-memory and on-disk state of the @@ -476,17 +429,9 @@ func (c *Container) setContainerState(state types.StateString) error { // update in-memory state c.state.State = state - if c.sandbox.supportNewStore() { - // flush data to storage - if err := c.sandbox.Save(); err != nil { - return err - } - } else { - // experimental runtime use "persist.json" which doesn't need "state.json" anymore - // update on-disk state - if err := c.store.Store(store.State, c.state); err != nil { - return err - } + // flush data to storage + if err := c.sandbox.Save(); err != nil { + return err } return nil @@ -571,13 +516,6 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( if err := c.sandbox.devManager.AttachDevice(m.BlockDeviceID, c.sandbox); err != nil { return nil, nil, err } - - if !c.sandbox.supportNewStore() { - if err := c.sandbox.storeSandboxDevices(); err != nil { - //TODO: roll back? - return nil, nil, err - } - } continue } @@ -620,12 +558,6 @@ func (c *Container) mountSharedDirMounts(hostSharedDir, guestSharedDir string) ( sharedDirMounts[sharedDirMount.Destination] = sharedDirMount } - if !c.sandbox.supportNewStore() { - if err := c.storeMounts(); err != nil { - return nil, nil, err - } - } - return sharedDirMounts, ignoredMounts, nil } @@ -751,46 +683,19 @@ func newContainer(sandbox *Sandbox, contConfig *ContainerConfig) (*Container, er ctx: sandbox.ctx, } - storeAlreadyExists := store.VCContainerStoreExists(sandbox.ctx, c.sandboxID, c.id) - ctrStore, err := store.NewVCContainerStore(sandbox.ctx, c.sandboxID, c.id) - if err != nil { + // experimental runtime use "persist.json" instead of legacy "state.json" as storage + err := c.Restore() + if err == nil { + //container restored + return c, nil + } + + // Unexpected error + if !os.IsNotExist(err) && err != errContainerPersistNotExist { return nil, err } - defer func() { - if err != nil && !storeAlreadyExists { - if delerr := c.store.Delete(); delerr != nil { - c.Logger().WithError(delerr).WithField("cid", c.id).Error("delete store failed") - } - } - }() - - c.store = ctrStore - - // experimental runtime use "persist.json" instead of legacy "state.json" as storage - if c.sandbox.supportNewStore() { - err := c.Restore() - if err == nil { - //container restored - return c, nil - } - - // Unexpected error - if !os.IsNotExist(err) && err != errContainerPersistNotExist { - return nil, err - } - // Go to next step for first created container - } else { - state, err := c.store.LoadContainerState() - if err == nil { - c.state = state - } - - var process Process - if err := c.store.Load(store.Process, &process); err == nil { - c.process = process - } - } + // Go to next step for first created container if err = c.createMounts(); err != nil { return nil, err } @@ -803,17 +708,6 @@ func newContainer(sandbox *Sandbox, contConfig *ContainerConfig) (*Container, er } func (c *Container) createMounts() error { - // If sandbox supports "newstore", only newly created container can reach this function, - // so we don't call restore when `supportNewStore` is true - if !c.sandbox.supportNewStore() { - mounts, err := c.loadMounts() - if err == nil { - // restore mounts from disk - c.mounts = mounts - return nil - } - } - // Create block devices for newly created container if err := c.createBlockDevices(); err != nil { return err @@ -823,18 +717,6 @@ func (c *Container) createMounts() error { } func (c *Container) createDevices(contConfig *ContainerConfig) error { - // If sandbox supports "newstore", only newly created container can reach this function, - // so we don't call restore when `supportNewStore` is true - if !c.sandbox.supportNewStore() { - // Devices will be found in storage after create stage has completed. - // We load devices from storage at all other stages. - storedDevices, err := c.loadDevices() - if err == nil { - c.devices = storedDevices - return nil - } - } - // If devices were not found in storage, create Device implementations // from the configuration. This should happen at create. var storedDevices []ContainerDevice @@ -914,12 +796,6 @@ func (c *Container) create() (err error) { // inside the VM c.getSystemMountInfo() - if !c.sandbox.supportNewStore() { - if err = c.storeDevices(); err != nil { - return - } - } - process, err := c.sandbox.agent.createContainer(c.sandbox, c) if err != nil { return err @@ -932,13 +808,6 @@ func (c *Container) create() (err error) { } } - if !c.sandbox.supportNewStore() { - // Store the container process returned by the agent. - if err = c.storeProcess(); err != nil { - return - } - } - if err = c.setContainerState(types.StateReady); err != nil { return } @@ -964,7 +833,7 @@ func (c *Container) delete() error { } } - return c.store.Delete() + return c.sandbox.storeSandbox() } // checkSandboxRunning validates the container state. @@ -1383,12 +1252,6 @@ func (c *Container) plugDevice(devicePath string) error { if err := c.sandbox.devManager.AttachDevice(b.DeviceID(), c.sandbox); err != nil { return err } - - if !c.sandbox.supportNewStore() { - if err := c.sandbox.storeSandboxDevices(); err != nil { - return err - } - } } return nil } @@ -1419,12 +1282,6 @@ func (c *Container) removeDrive() (err error) { return err } } - - if !c.sandbox.supportNewStore() { - if err := c.sandbox.storeSandboxDevices(); err != nil { - return err - } - } } return nil @@ -1439,12 +1296,6 @@ func (c *Container) attachDevices() error { return err } } - - if !c.sandbox.supportNewStore() { - if err := c.sandbox.storeSandboxDevices(); err != nil { - return err - } - } return nil } @@ -1467,12 +1318,6 @@ func (c *Container) detachDevices() error { } } } - - if !c.sandbox.supportNewStore() { - if err := c.sandbox.storeSandboxDevices(); err != nil { - return err - } - } return nil } diff --git a/virtcontainers/container_test.go b/virtcontainers/container_test.go index 995fccee67..2ace0a7266 100644 --- a/virtcontainers/container_test.go +++ b/virtcontainers/container_test.go @@ -341,18 +341,6 @@ func TestContainerAddDriveDir(t *testing.T) { rootFs: RootFs{Target: fakeRootfs, Mounted: true}, } - containerStore, err := store.NewVCContainerStore(sandbox.ctx, sandbox.id, container.id) - assert.Nil(err) - container.store = containerStore - - // create state file - path := store.ContainerRuntimeRootPath(testSandboxID, container.ID()) - stateFilePath := filepath.Join(path, store.StateFile) - os.Remove(stateFilePath) - - _, err = os.Create(stateFilePath) - assert.NoError(err) - // Make the checkStorageDriver func variable point to a fake check function savedFunc := checkStorageDriver checkStorageDriver = func(major, minor int) (bool, error) { @@ -405,11 +393,6 @@ func TestContainerRootfsPath(t *testing.T) { rootFs: RootFs{Target: fakeRootfs, Mounted: true}, rootfsSuffix: "rootfs", } - cvcstore, err := store.NewVCContainerStore(context.Background(), - sandbox.id, - container.id) - assert.Nil(t, err) - container.store = cvcstore container.hotplugDrive() assert.Empty(t, container.rootfsSuffix) diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 141f5402d7..a08d8325e8 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -1444,13 +1444,6 @@ func (k *kataAgent) handleBlockVolumes(c *Container) ([]*grpc.Storage, error) { // device is detached with detachDevices() for a container. c.devices = append(c.devices, ContainerDevice{ID: id, ContainerPath: m.Destination}) - if !c.sandbox.supportNewStore() { - if err := c.storeDevices(); err != nil { - k.Logger().WithField("device", id).WithError(err).Error("store device failed") - return nil, err - } - } - vol := &grpc.Storage{} device := c.sandbox.devManager.GetDeviceByID(id) diff --git a/virtcontainers/persist/api/interface.go b/virtcontainers/persist/api/interface.go index a14dbc5e1f..433e5ad419 100644 --- a/virtcontainers/persist/api/interface.go +++ b/virtcontainers/persist/api/interface.go @@ -13,5 +13,8 @@ type PersistDriver interface { // We only support get data for one whole sandbox FromDisk(sid string) (SandboxState, map[string]ContainerState, error) // Destroy will remove everything from storage - Destroy() error + Destroy(sid string) error + // Lock locks the persist driver, "exclusive" decides whether the lock is exclusive or shared. + // It returns Unlock Function and errors + Lock(sid string, exclusive bool) (func() error, error) } diff --git a/virtcontainers/persist/fs/fs.go b/virtcontainers/persist/fs/fs.go index afc1c570c0..ac8d1c48b0 100644 --- a/virtcontainers/persist/fs/fs.go +++ b/virtcontainers/persist/fs/fs.go @@ -51,8 +51,6 @@ var RunStoragePath = func() string { type FS struct { sandboxState *persistapi.SandboxState containerState map[string]persistapi.ContainerState - - lockFile *os.File } var fsLog = logrus.WithField("source", "virtcontainers/persist/fs") @@ -77,21 +75,21 @@ func Init() (persistapi.PersistDriver, error) { }, nil } -func (fs *FS) sandboxDir() (string, error) { - id := fs.sandboxState.SandboxContainer - if id == "" { - return "", fmt.Errorf("sandbox container id required") - } - - return filepath.Join(RunStoragePath(), id), nil +func (fs *FS) sandboxDir(sandboxID string) (string, error) { + return filepath.Join(RunStoragePath(), sandboxID), nil } // ToDisk sandboxState and containerState to disk func (fs *FS) ToDisk(ss persistapi.SandboxState, cs map[string]persistapi.ContainerState) (retErr error) { + id := ss.SandboxContainer + if id == "" { + return fmt.Errorf("sandbox container id required") + } + fs.sandboxState = &ss fs.containerState = cs - sandboxDir, err := fs.sandboxDir() + sandboxDir, err := fs.sandboxDir(id) if err != nil { return err } @@ -100,15 +98,10 @@ func (fs *FS) ToDisk(ss persistapi.SandboxState, cs map[string]persistapi.Contai return err } - if err := fs.lock(); err != nil { - return err - } - defer fs.unlock() - // if error happened, destroy all dirs defer func() { if retErr != nil { - if err := fs.Destroy(); err != nil { + if err := fs.Destroy(id); err != nil { fs.Logger().WithError(err).Errorf("failed to destroy dirs") } } @@ -155,6 +148,27 @@ func (fs *FS) ToDisk(ss persistapi.SandboxState, cs map[string]persistapi.Contai } } + // Walk sandbox dir and find container. + files, err := ioutil.ReadDir(sandboxDir) + if err != nil { + return err + } + + // Remove non-existing containers + for _, file := range files { + if !file.IsDir() { + continue + } + // Container dir exists. + cid := file.Name() + + // Container should be removed when container id doesn't exist in cs. + if _, ok := cs[cid]; !ok { + if err := os.RemoveAll(filepath.Join(sandboxDir, cid)); err != nil { + return err + } + } + } return nil } @@ -165,18 +179,11 @@ func (fs *FS) FromDisk(sid string) (persistapi.SandboxState, map[string]persista return ss, nil, fmt.Errorf("restore requires sandbox id") } - fs.sandboxState.SandboxContainer = sid - - sandboxDir, err := fs.sandboxDir() + sandboxDir, err := fs.sandboxDir(sid) if err != nil { return ss, nil, err } - if err := fs.lock(); err != nil { - return ss, nil, err - } - defer fs.unlock() - // get sandbox configuration from persist data sandboxFile := filepath.Join(sandboxDir, persistFile) f, err := os.OpenFile(sandboxFile, os.O_RDONLY, fileMode) @@ -224,8 +231,12 @@ func (fs *FS) FromDisk(sid string) (persistapi.SandboxState, map[string]persista } // Destroy removes everything from disk -func (fs *FS) Destroy() error { - sandboxDir, err := fs.sandboxDir() +func (fs *FS) Destroy(sandboxID string) error { + if sandboxID == "" { + return fmt.Errorf("sandbox container id required") + } + + sandboxDir, err := fs.sandboxDir(sandboxID) if err != nil { return err } @@ -236,39 +247,42 @@ func (fs *FS) Destroy() error { return nil } -func (fs *FS) lock() error { - sandboxDir, err := fs.sandboxDir() +func (fs *FS) Lock(sandboxID string, exclusive bool) (func() error, error) { + if sandboxID == "" { + return nil, fmt.Errorf("sandbox container id required") + } + + sandboxDir, err := fs.sandboxDir(sandboxID) if err != nil { - return err + return nil, err } f, err := os.Open(sandboxDir) if err != nil { - return err + return nil, err } - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + var lockType int + if exclusive { + lockType = syscall.LOCK_EX | syscall.LOCK_NB + } else { + lockType = syscall.LOCK_SH | syscall.LOCK_NB + } + + if err := syscall.Flock(int(f.Fd()), lockType); err != nil { f.Close() - return err + return nil, err } - fs.lockFile = f - return nil -} + unlockFunc := func() error { + defer f.Close() + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_UN); err != nil { + return err + } -func (fs *FS) unlock() error { - if fs.lockFile == nil { return nil } - - lockFile := fs.lockFile - defer lockFile.Close() - fs.lockFile = nil - if err := syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN); err != nil { - return err - } - - return nil + return unlockFunc, nil } // TestSetRunStoragePath set RunStoragePath to path diff --git a/virtcontainers/persist/fs/fs_test.go b/virtcontainers/persist/fs/fs_test.go index 4b5d853f5e..75bb4d879f 100644 --- a/virtcontainers/persist/fs/fs_test.go +++ b/virtcontainers/persist/fs/fs_test.go @@ -40,18 +40,25 @@ func TestFsLock(t *testing.T) { os.RemoveAll(testDir) }() - fs.sandboxState.SandboxContainer = "test-fs-driver" - sandboxDir, err := fs.sandboxDir() + sid := "test-fs-driver" + fs.sandboxState.SandboxContainer = sid + sandboxDir, err := fs.sandboxDir(sid) assert.Nil(t, err) err = os.MkdirAll(sandboxDir, dirMode) assert.Nil(t, err) - assert.Nil(t, fs.lock()) - assert.NotNil(t, fs.lock()) + unlockFunc, err := fs.Lock(sid, false) + assert.Nil(t, err) + unlockFunc2, err := fs.Lock(sid, false) + assert.Nil(t, err) + _, err = fs.Lock(sid, true) + assert.NotNil(t, err) - assert.Nil(t, fs.unlock()) - assert.Nil(t, fs.unlock()) + assert.Nil(t, unlockFunc()) + // double unlock should return error + assert.NotNil(t, unlockFunc()) + assert.Nil(t, unlockFunc2()) } func TestFsDriver(t *testing.T) { @@ -88,7 +95,7 @@ func TestFsDriver(t *testing.T) { assert.Equal(t, ss.SandboxContainer, id) assert.Equal(t, ss.State, "") - // flush all to disk + // flush all to disk. ss.State = "running" assert.Nil(t, fs.ToDisk(ss, cs)) ss, cs, err = fs.FromDisk(id) @@ -99,9 +106,31 @@ func TestFsDriver(t *testing.T) { assert.Equal(t, ss.SandboxContainer, id) assert.Equal(t, ss.State, "running") - assert.Nil(t, fs.Destroy()) + // add new container state. + cs["test-container"] = persistapi.ContainerState{ + State: "ready", + } + assert.Nil(t, fs.ToDisk(ss, cs)) + ss, cs, err = fs.FromDisk(id) + assert.Nil(t, err) + assert.NotNil(t, ss) + assert.Equal(t, len(cs), 1) + c, ok := cs["test-container"] + assert.True(t, ok) + assert.Equal(t, c.State, "ready") - dir, err := fs.sandboxDir() + // clean all container. + cs = make(map[string]persistapi.ContainerState) + assert.Nil(t, fs.ToDisk(ss, cs)) + ss, cs, err = fs.FromDisk(id) + assert.Nil(t, err) + assert.NotNil(t, ss) + assert.Equal(t, len(cs), 0) + + // destroy whole sandbox dir. + assert.Nil(t, fs.Destroy(id)) + + dir, err := fs.sandboxDir(id) assert.Nil(t, err) assert.NotEqual(t, len(dir), 0) diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 9dfebf07b8..dea3cfe654 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -597,23 +597,9 @@ func (s *Sandbox) storeSandbox() error { span, _ := s.trace("storeSandbox") defer span.Finish() - if s.supportNewStore() { - // flush data to storage - if err := s.Save(); err != nil { - return err - } - } else { - err := s.store.Store(store.Configuration, *(s.config)) - if err != nil { - return err - } - - for _, container := range s.containers { - err = container.store.Store(store.Configuration, *(container.config)) - if err != nil { - return err - } - } + // flush data to storage + if err := s.Save(); err != nil { + return err } return nil } @@ -777,7 +763,7 @@ func (s *Sandbox) Delete() error { s.agent.cleanup(s) if s.supportNewStore() { - if err := s.newStore.Destroy(); err != nil { + if err := s.newStore.Destroy(s.id); err != nil { return err } } @@ -1070,7 +1056,6 @@ func (s *Sandbox) stopVM() error { if s.disableVMShutdown { // Do not kill the VM - allow the agent to shut it down // (only used to support static agent tracing). - s.Logger().Info("Not stopping VM") return nil } @@ -1118,7 +1103,6 @@ func (s *Sandbox) fetchContainers() error { // This should be called only when the sandbox is already created. // It will add new container config to sandbox.config.Containers func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, error) { - storeAlreadyExists := store.VCContainerStoreExists(s.ctx, s.id, contConfig.ID) // Create the container. c, err := newContainer(s, &contConfig) if err != nil { @@ -1134,11 +1118,6 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro // delete container config s.config.Containers = s.config.Containers[:len(s.config.Containers)-1] } - if !storeAlreadyExists { - if delerr := c.store.Delete(); delerr != nil { - c.Logger().WithError(delerr).WithField("cid", c.id).Error("delete store failed") - } - } } }() @@ -1152,6 +1131,13 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro return nil, err } + defer func() { + // Rollback if error happens. + if err != nil { + s.removeContainer(c.id) + } + }() + // Sandbox is reponsable to update VM resources needed by Containers // Update resources after having added containers to the sandbox, since // container status is requiered to know if more resources should be added. @@ -1160,12 +1146,6 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro return nil, err } - // Store it. - err = c.storeContainer() - if err != nil { - return nil, err - } - if err = s.cgroupsUpdate(); err != nil { return nil, err } diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index 08d43449d7..40cb284f4e 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -7,7 +7,6 @@ package virtcontainers import ( "context" - "encoding/json" "fmt" "io/ioutil" "os" @@ -668,17 +667,6 @@ func TestContainerStateSetFstype(t *testing.T) { cImpl, ok := c.(*Container) assert.True(ok) - containerStore, err := store.NewVCContainerStore(sandbox.ctx, sandbox.id, c.ID()) - assert.NoError(err) - - cImpl.store = containerStore - - path := store.ContainerRuntimeRootPath(testSandboxID, c.ID()) - stateFilePath := filepath.Join(path, store.StateFile) - - f, err := os.Create(stateFilePath) - assert.NoError(err) - state := types.ContainerState{ State: "ready", Fstype: "vfs", @@ -686,34 +674,10 @@ func TestContainerStateSetFstype(t *testing.T) { cImpl.state = state - stateData := `{ - "state":"ready", - "fstype":"vfs", - }` - - n, err := f.WriteString(stateData) - defer f.Close() - assert.NoError(err) - assert.Equal(n, len(stateData)) - newFstype := "ext4" err = cImpl.setStateFstype(newFstype) assert.NoError(err) assert.Equal(cImpl.state.Fstype, newFstype) - - fileData, err := ioutil.ReadFile(stateFilePath) - assert.NoError(err) - - // experimental features doesn't write state.json - if sandbox.supportNewStore() { - return - } - - var res types.ContainerState - err = json.Unmarshal([]byte(string(fileData)), &res) - assert.NoError(err) - assert.Equal(res.Fstype, newFstype) - assert.Equal(res.State, state.State) } const vfioPath = "/dev/vfio/" @@ -916,8 +880,6 @@ func TestCreateContainer(t *testing.T) { _, err = s.CreateContainer(contConfig) assert.NotNil(t, err, "Should failed to create a duplicated container") assert.Equal(t, len(s.config.Containers), 1, "Container config list length from sandbox structure should be 1") - ret := store.VCContainerStoreExists(s.ctx, testSandboxID, contID) - assert.True(t, ret, "Should not delete container store that already existed") } func TestDeleteContainer(t *testing.T) { @@ -1023,8 +985,6 @@ func TestDeleteStoreWhenCreateContainerFail(t *testing.T) { s.state.CgroupPath = filepath.Join(testDir, "bad-cgroup") _, err = s.CreateContainer(contConfig) assert.NotNil(t, err, "Should fail to create container due to wrong cgroup") - ret := store.VCContainerStoreExists(s.ctx, testSandboxID, contID) - assert.False(t, ret, "Should delete configuration root after failed to create a container") } func TestDeleteStoreWhenNewContainerFail(t *testing.T) { @@ -1307,10 +1267,6 @@ func TestPreAddDevice(t *testing.T) { } container.state.State = types.StateReady - containerStore, err := store.NewVCContainerStore(sandbox.ctx, sandbox.id, container.id) - assert.Nil(t, err) - container.store = containerStore - // create state file path := store.ContainerRuntimeRootPath(testSandboxID, container.ID()) err = os.MkdirAll(path, store.DirMode) diff --git a/virtcontainers/store/vc.go b/virtcontainers/store/vc.go index 79642b0593..92e7eb3687 100644 --- a/virtcontainers/store/vc.go +++ b/virtcontainers/store/vc.go @@ -334,9 +334,3 @@ func VCSandboxStoreExists(ctx context.Context, sandboxID string) bool { s := stores.findStore(SandboxConfigurationRoot(sandboxID)) return s != nil } - -// VCContainerStoreExists returns true if a container store already exists. -func VCContainerStoreExists(ctx context.Context, sandboxID string, containerID string) bool { - s := stores.findStore(ContainerConfigurationRoot(sandboxID, containerID)) - return s != nil -} diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go index e9af85826d..1a6f41c64e 100644 --- a/virtcontainers/virtcontainers_test.go +++ b/virtcontainers/virtcontainers_test.go @@ -175,7 +175,7 @@ func TestMain(m *testing.M) { // allow the tests to run without affecting the host system. store.ConfigStoragePath = func() string { return filepath.Join(testDir, store.StoragePathSuffix, "config") } store.RunStoragePath = func() string { return filepath.Join(testDir, store.StoragePathSuffix, "run") } - fs.TestSetRunStoragePath(filepath.Join(testDir, "vc", "sbs")) + fs.TestSetRunStoragePath(filepath.Join(testDir, "vc", "run")) defer func() { store.ConfigStoragePath = ConfigStoragePathSaved