From 9bd4e5008c544cc87e80f78c16954d6247d6a0f0 Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Tue, 12 Mar 2019 11:56:44 +0800 Subject: [PATCH] store: address comments Address review comments Signed-off-by: Wei Zhang --- virtcontainers/container.go | 12 +++---- virtcontainers/persist.go | 8 ++--- virtcontainers/persist/api/interface.go | 6 ++-- virtcontainers/persist/api/sandbox.go | 3 +- virtcontainers/persist/fs/fs.go | 43 +++++++++++++++++-------- virtcontainers/persist/fs/fs_test.go | 6 ++-- 6 files changed, 45 insertions(+), 33 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 9858a2f2ab..6030d73550 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -376,8 +376,7 @@ func (c *Container) setStateFstype(fstype string) error { if !c.sandbox.supportNewStore() { // experimental runtime use "persist.json" which doesn't need "state.json" anymore - err := c.storeState() - if err != nil { + if err := c.storeState(); err != nil { return err } } @@ -454,8 +453,7 @@ func (c *Container) setContainerState(state types.StateString) error { } else { // experimental runtime use "persist.json" which doesn't need "state.json" anymore // update on-disk state - err := c.store.Store(store.State, c.state) - if err != nil { + if err := c.store.Store(store.State, c.state); err != nil { return err } } @@ -619,9 +617,9 @@ func (c *Container) unmountHostMounts() error { return nil } -func filterDevices(sandbox *Sandbox, c *Container, devices []ContainerDevice) (ret []ContainerDevice) { +func filterDevices(c *Container, devices []ContainerDevice) (ret []ContainerDevice) { for _, dev := range devices { - major, _ := sandbox.devManager.GetDeviceByID(dev.ID).GetMajorMinor() + major, _ := c.sandbox.devManager.GetDeviceByID(dev.ID).GetMajorMinor() if _, ok := cdromMajors[major]; ok { c.Logger().WithFields(logrus.Fields{ "device": dev.ContainerPath, @@ -773,7 +771,7 @@ func (c *Container) createDevices(contConfig ContainerConfig) error { GID: info.GID, }) } - c.devices = filterDevices(c.sandbox, c, storedDevices) + c.devices = filterDevices(c, storedDevices) } return nil } diff --git a/virtcontainers/persist.go b/virtcontainers/persist.go index 7fdc6fa3c9..16612149d4 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -94,7 +94,7 @@ func (s *Sandbox) dumpDevices(ss *persistapi.SandboxState, cs map[string]persist return nil } -// versionCallback set persist data version to current version in runtime +// verSaveCallback set persist data version to current version in runtime func (s *Sandbox) verSaveCallback() { s.newStore.AddSaveCallback("version", func(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { ss.PersistVersion = persistapi.CurPersistVersion @@ -102,12 +102,12 @@ func (s *Sandbox) verSaveCallback() { }) } -// PersistState register hook to set sandbox and container state to persist +// stateSaveCallback register hook to set sandbox and container state to persist func (s *Sandbox) stateSaveCallback() { s.newStore.AddSaveCallback("state", s.dumpState) } -// PersistHvState register hook to save hypervisor state to persist data +// hvStateSaveCallback register hook to save hypervisor state to persist data func (s *Sandbox) hvStateSaveCallback() { s.newStore.AddSaveCallback("hypervisor", s.dumpHypervisor) } @@ -118,7 +118,7 @@ func (s *Sandbox) devicesSaveCallback() { } func (s *Sandbox) getSbxAndCntStates() (*persistapi.SandboxState, map[string]persistapi.ContainerState, error) { - if err := s.newStore.Restore(s.id); err != nil { + if err := s.newStore.FromDisk(s.id); err != nil { return nil, nil, err } diff --git a/virtcontainers/persist/api/interface.go b/virtcontainers/persist/api/interface.go index 9956dcb5d8..71a9bbba2a 100644 --- a/virtcontainers/persist/api/interface.go +++ b/virtcontainers/persist/api/interface.go @@ -9,13 +9,13 @@ package persistapi type PersistDriver interface { // ToDisk flushes data to disk(or other storage media such as a remote db) ToDisk() error + // FromDisk will restore all data for sandbox with `sid` from storage. + // We only support get data for one whole sandbox + FromDisk(sid string) error // AddSaveCallback addes callback function named `name` to driver storage list // The callback functions will be invoked when calling `ToDisk()`, notice that // callback functions are not order guaranteed, AddSaveCallback(name string, f SetFunc) - // Restore will restore all data for sandbox with `sid` from storage. - // We only support get data for one whole sandbox - Restore(sid string) error // Destroy will remove everything from storage Destroy() error // GetStates will return SandboxState and ContainerState(s) directly diff --git a/virtcontainers/persist/api/sandbox.go b/virtcontainers/persist/api/sandbox.go index 71403eac06..1489fae9d0 100644 --- a/virtcontainers/persist/api/sandbox.go +++ b/virtcontainers/persist/api/sandbox.go @@ -43,8 +43,7 @@ type HypervisorState struct { HotpluggedMemory int UUID string HotplugVFIOOnRootBus bool - // TODO: should this be map[index]bool to indicate available block id?? - BlockIndex int + BlockIndex int } // ProxyState save proxy state data diff --git a/virtcontainers/persist/fs/fs.go b/virtcontainers/persist/fs/fs.go index 3e40658af1..9b1e6be85b 100644 --- a/virtcontainers/persist/fs/fs.go +++ b/virtcontainers/persist/fs/fs.go @@ -15,13 +15,14 @@ import ( "syscall" persistapi "github.com/kata-containers/runtime/virtcontainers/persist/api" + "github.com/sirupsen/logrus" ) // persistFile is the file name for JSON sandbox/container configuration const persistFile = "persist.json" // dirMode is the permission bits used for creating a directory -const dirMode = os.FileMode(0750) +const dirMode = os.FileMode(0700) // fileMode is the permission bits used for creating a file const fileMode = os.FileMode(0640) @@ -48,6 +49,15 @@ type FS struct { lockFile *os.File } +var fsLog = logrus.WithField("source", "virtcontainers/persist/fs") + +// Logger returns a logrus logger appropriate for logging Store messages +func (fs *FS) Logger() *logrus.Entry { + return fsLog.WithFields(logrus.Fields{ + "subsystem": "persist", + }) +} + // Name returns driver name func Name() string { return "fs" @@ -63,11 +73,12 @@ func Init() (persistapi.PersistDriver, error) { } func (fs *FS) sandboxDir() (string, error) { - if fs.sandboxState.SandboxContainer == "" { + id := fs.sandboxState.SandboxContainer + if id == "" { return "", fmt.Errorf("sandbox container id required") } - return filepath.Join(runStoragePath, fs.sandboxState.SandboxContainer), nil + return filepath.Join(runStoragePath, id), nil } // ToDisk sandboxState and containerState to disk @@ -77,14 +88,6 @@ func (fs *FS) ToDisk() (retErr error) { fun(fs.sandboxState, fs.containerState) } - // if error happened, destroy all dirs - defer func() { - if retErr != nil { - // TODO: log error - fs.Destroy() - } - }() - sandboxDir, err := fs.sandboxDir() if err != nil { return err @@ -95,13 +98,25 @@ func (fs *FS) ToDisk() (retErr error) { } if err := fs.lock(); err != nil { + if err1 := fs.Destroy(); err1 != nil { + fs.Logger().WithError(err1).Errorf("failed to destroy dirs") + } return err } defer fs.unlock() + // if error happened, destroy all dirs + defer func() { + if retErr != nil { + if err := fs.Destroy(); err != nil { + fs.Logger().WithError(err).Errorf("failed to destroy dirs") + } + } + }() + // persist sandbox configuration data sandboxFile := filepath.Join(sandboxDir, persistFile) - f, err := os.OpenFile(sandboxFile, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fileMode) + f, err := os.OpenFile(sandboxFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileMode) if err != nil { return err } @@ -133,8 +148,8 @@ func (fs *FS) ToDisk() (retErr error) { return nil } -// Restore state for sandbox with name sid -func (fs *FS) Restore(sid string) error { +// FromDisk restores state for sandbox with name sid +func (fs *FS) FromDisk(sid string) error { if sid == "" { return fmt.Errorf("restore requires sandbox id") } diff --git a/virtcontainers/persist/fs/fs_test.go b/virtcontainers/persist/fs/fs_test.go index 7dc778a8ec..e9ad01b340 100644 --- a/virtcontainers/persist/fs/fs_test.go +++ b/virtcontainers/persist/fs/fs_test.go @@ -71,10 +71,10 @@ func TestFsDriver(t *testing.T) { }) // try non-existent dir - assert.NotNil(t, fs.Restore("test-fs")) + assert.NotNil(t, fs.FromDisk("test-fs")) // since we didn't call ToDisk, Callbacks are not invoked, and state is still empty in disk file - assert.Nil(t, fs.Restore(id)) + assert.Nil(t, fs.FromDisk(id)) ss, cs, err := fs.GetStates() assert.Nil(t, err) assert.NotNil(t, ss) @@ -85,7 +85,7 @@ func TestFsDriver(t *testing.T) { // flush all to disk assert.Nil(t, fs.ToDisk()) - assert.Nil(t, fs.Restore(id)) + assert.Nil(t, fs.FromDisk(id)) ss, cs, err = fs.GetStates() assert.Nil(t, err) assert.NotNil(t, ss)