From 341a988e0640373ff8719bfa6dec3c29a0185e6d Mon Sep 17 00:00:00 2001 From: Wei Zhang Date: Mon, 22 Apr 2019 19:08:33 +0800 Subject: [PATCH] persist: simplify persist api Fixes #803 Simplify new store API to make the code easier to understand and use. Signed-off-by: Wei Zhang --- virtcontainers/container.go | 4 +- virtcontainers/persist.go | 71 +++++++++++-------------- virtcontainers/persist/api/interface.go | 10 +--- virtcontainers/persist/fs/fs.go | 43 ++++++--------- virtcontainers/persist/fs/fs_test.go | 34 +++++------- virtcontainers/persist_test.go | 23 ++------ virtcontainers/sandbox.go | 10 +--- virtcontainers/types/sandbox.go | 5 ++ 8 files changed, 74 insertions(+), 126 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 6030d7355..d1408d915 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -392,7 +392,7 @@ func (c *Container) GetAnnotations() map[string]string { // storeContainer stores a container config. func (c *Container) storeContainer() error { if c.sandbox.supportNewStore() { - if err := c.sandbox.newStore.ToDisk(); err != nil { + if err := c.sandbox.Save(); err != nil { return err } } @@ -447,7 +447,7 @@ func (c *Container) setContainerState(state types.StateString) error { if c.sandbox.supportNewStore() { // flush data to storage - if err := c.sandbox.newStore.ToDisk(); err != nil { + if err := c.sandbox.Save(); err != nil { return err } } else { diff --git a/virtcontainers/persist.go b/virtcontainers/persist.go index b6bc5814e..ccb5e3153 100644 --- a/virtcontainers/persist.go +++ b/virtcontainers/persist.go @@ -19,7 +19,16 @@ var ( errContainerPersistNotExist = errors.New("container doesn't exist in persist data") ) -func (s *Sandbox) dumpState(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { +func (s *Sandbox) dumpVersion(ss *persistapi.SandboxState) { + // New created sandbox has a uninitialized `PersistVersion` which should be set to current version when do the first saving; + // Old restored sandbox should keep its original version and shouldn't be modified any more after it's initialized. + ss.PersistVersion = s.state.PersistVersion + if ss.PersistVersion == 0 { + ss.PersistVersion = persistapi.CurPersistVersion + } +} + +func (s *Sandbox) dumpState(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) { ss.SandboxContainer = s.id ss.GuestMemoryBlockSizeMB = s.state.GuestMemoryBlockSizeMB ss.State = string(s.state.State) @@ -45,13 +54,10 @@ func (s *Sandbox) dumpState(ss *persistapi.SandboxState, cs map[string]persistap delete(cs, id) } } - - return nil } -func (s *Sandbox) dumpHypervisor(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { +func (s *Sandbox) dumpHypervisor(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) { ss.HypervisorState.BlockIndex = s.state.BlockIndex - return nil } func deviceToDeviceState(devices []api.Device) (dss []persistapi.DeviceState) { @@ -61,7 +67,7 @@ func deviceToDeviceState(devices []api.Device) (dss []persistapi.DeviceState) { return } -func (s *Sandbox) dumpDevices(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { +func (s *Sandbox) dumpDevices(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) { ss.Devices = deviceToDeviceState(s.devManager.GetAllDevices()) for id, cont := range s.containers { @@ -90,48 +96,34 @@ func (s *Sandbox) dumpDevices(ss *persistapi.SandboxState, cs map[string]persist delete(cs, id) } } +} + +func (s *Sandbox) Save() error { + var ( + ss = persistapi.SandboxState{} + cs = make(map[string]persistapi.ContainerState) + ) + + s.dumpVersion(&ss) + s.dumpState(&ss, cs) + s.dumpHypervisor(&ss, cs) + s.dumpDevices(&ss, cs) + + if err := s.newStore.ToDisk(ss, cs); err != nil { + return err + } return nil } -// 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 - return nil - }) -} - -// stateSaveCallback register hook to set sandbox and container state to persist -func (s *Sandbox) stateSaveCallback() { - s.newStore.AddSaveCallback("state", s.dumpState) -} - -// hvStateSaveCallback register hook to save hypervisor state to persist data -func (s *Sandbox) hvStateSaveCallback() { - s.newStore.AddSaveCallback("hypervisor", s.dumpHypervisor) -} - -// PersistDevices register hook to save device informations -func (s *Sandbox) devicesSaveCallback() { - s.newStore.AddSaveCallback("devices", s.dumpDevices) -} - -func (s *Sandbox) getSbxAndCntStates() (*persistapi.SandboxState, map[string]persistapi.ContainerState, error) { - if err := s.newStore.FromDisk(s.id); err != nil { - return nil, nil, err - } - - return s.newStore.GetStates() -} - // Restore will restore sandbox data from persist file on disk func (s *Sandbox) Restore() error { - ss, _, err := s.getSbxAndCntStates() + ss, _, err := s.newStore.FromDisk(s.id) if err != nil { return err } + s.state.PersistVersion = ss.PersistVersion s.state.GuestMemoryBlockSizeMB = ss.GuestMemoryBlockSizeMB s.state.BlockIndex = ss.HypervisorState.BlockIndex s.state.State = types.StateString(ss.State) @@ -141,8 +133,9 @@ func (s *Sandbox) Restore() error { } // Restore will restore container data from persist file on disk +// TODO: func (c *Container) Restore() error { - _, cs, err := c.sandbox.getSbxAndCntStates() + _, cs, err := c.sandbox.newStore.FromDisk(c.sandbox.id) if err != nil { return err } diff --git a/virtcontainers/persist/api/interface.go b/virtcontainers/persist/api/interface.go index 0b94a61a6..a14dbc5e1 100644 --- a/virtcontainers/persist/api/interface.go +++ b/virtcontainers/persist/api/interface.go @@ -8,16 +8,10 @@ package persistapi // PersistDriver is interface describing operations to save/restore persist data type PersistDriver interface { // ToDisk flushes data to disk(or other storage media such as a remote db) - ToDisk() error + ToDisk(SandboxState, map[string]ContainerState) 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) + FromDisk(sid string) (SandboxState, map[string]ContainerState, error) // Destroy will remove everything from storage Destroy() error - // GetStates will return SandboxState and ContainerState(s) directly - GetStates() (*SandboxState, map[string]ContainerState, error) } diff --git a/virtcontainers/persist/fs/fs.go b/virtcontainers/persist/fs/fs.go index df3bc5b35..1e7552785 100644 --- a/virtcontainers/persist/fs/fs.go +++ b/virtcontainers/persist/fs/fs.go @@ -44,7 +44,6 @@ var runStoragePath = filepath.Join("/run", storagePathSuffix, sandboxPathSuffix) type FS struct { sandboxState *persistapi.SandboxState containerState map[string]persistapi.ContainerState - setFuncs map[string]persistapi.SetFunc lockFile *os.File } @@ -68,7 +67,6 @@ func Init() (persistapi.PersistDriver, error) { return &FS{ sandboxState: &persistapi.SandboxState{}, containerState: make(map[string]persistapi.ContainerState), - setFuncs: make(map[string]persistapi.SetFunc), }, nil } @@ -82,11 +80,9 @@ func (fs *FS) sandboxDir() (string, error) { } // ToDisk sandboxState and containerState to disk -func (fs *FS) ToDisk() (retErr error) { - // call registered hooks to set sandboxState and containerState - for _, fun := range fs.setFuncs { - fun(fs.sandboxState, fs.containerState) - } +func (fs *FS) ToDisk(ss persistapi.SandboxState, cs map[string]persistapi.ContainerState) (retErr error) { + fs.sandboxState = &ss + fs.containerState = cs sandboxDir, err := fs.sandboxDir() if err != nil { @@ -146,20 +142,21 @@ func (fs *FS) ToDisk() (retErr error) { } // FromDisk restores state for sandbox with name sid -func (fs *FS) FromDisk(sid string) error { +func (fs *FS) FromDisk(sid string) (persistapi.SandboxState, map[string]persistapi.ContainerState, error) { + ss := persistapi.SandboxState{} if sid == "" { - return fmt.Errorf("restore requires sandbox id") + return ss, nil, fmt.Errorf("restore requires sandbox id") } fs.sandboxState.SandboxContainer = sid sandboxDir, err := fs.sandboxDir() if err != nil { - return err + return ss, nil, err } if err := fs.lock(); err != nil { - return err + return ss, nil, err } defer fs.unlock() @@ -167,18 +164,18 @@ func (fs *FS) FromDisk(sid string) error { sandboxFile := filepath.Join(sandboxDir, persistFile) f, err := os.OpenFile(sandboxFile, os.O_RDONLY, fileMode) if err != nil { - return err + return ss, nil, err } defer f.Close() if err := json.NewDecoder(f).Decode(fs.sandboxState); err != nil { - return err + return ss, nil, err } // walk sandbox dir and find container files, err := ioutil.ReadDir(sandboxDir) if err != nil { - return err + return ss, nil, err } for _, file := range files { @@ -194,18 +191,19 @@ func (fs *FS) FromDisk(sid string) error { if os.IsNotExist(err) { continue } - return err + return ss, nil, err } var cstate persistapi.ContainerState if err := json.NewDecoder(cf).Decode(&cstate); err != nil { - return err + return ss, nil, err } cf.Close() fs.containerState[cid] = cstate } - return nil + + return *fs.sandboxState, fs.containerState, nil } // Destroy removes everything from disk @@ -221,17 +219,6 @@ func (fs *FS) Destroy() error { return nil } -// GetStates returns SandboxState and ContainerState -func (fs *FS) GetStates() (*persistapi.SandboxState, map[string]persistapi.ContainerState, error) { - return fs.sandboxState, fs.containerState, nil -} - -// AddSaveCallback registers processing hooks for Dump -func (fs *FS) AddSaveCallback(name string, f persistapi.SetFunc) { - // only accept last registered hook with same name - fs.setFuncs[name] = f -} - func (fs *FS) lock() error { sandboxDir, err := fs.sandboxDir() if err != nil { diff --git a/virtcontainers/persist/fs/fs_test.go b/virtcontainers/persist/fs/fs_test.go index e9ad01b34..1d618a7f4 100644 --- a/virtcontainers/persist/fs/fs_test.go +++ b/virtcontainers/persist/fs/fs_test.go @@ -51,31 +51,21 @@ func TestFsDriver(t *testing.T) { assert.Nil(t, err) assert.NotNil(t, fs) - fs.AddSaveCallback("test", func(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { - return nil - }) + ss := persistapi.SandboxState{} + cs := make(map[string]persistapi.ContainerState) // missing sandbox container id - assert.NotNil(t, fs.ToDisk()) + assert.NotNil(t, fs.ToDisk(ss, cs)) id := "test-fs-driver" - // missing sandbox container id - fs.AddSaveCallback("test", func(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { - ss.SandboxContainer = id - return nil - }) - assert.Nil(t, fs.ToDisk()) - - fs.AddSaveCallback("test1", func(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error { - ss.State = "running" - return nil - }) + ss.SandboxContainer = id + assert.Nil(t, fs.ToDisk(ss, cs)) // try non-existent dir - assert.NotNil(t, fs.FromDisk("test-fs")) + _, _, err = fs.FromDisk("test-fs") + assert.NotNil(t, err) - // since we didn't call ToDisk, Callbacks are not invoked, and state is still empty in disk file - assert.Nil(t, fs.FromDisk(id)) - ss, cs, err := fs.GetStates() + // since we didn't call ToDisk, state is still empty in disk file + ss, cs, err = fs.FromDisk(id) assert.Nil(t, err) assert.NotNil(t, ss) assert.Equal(t, len(cs), 0) @@ -84,9 +74,9 @@ func TestFsDriver(t *testing.T) { assert.Equal(t, ss.State, "") // flush all to disk - assert.Nil(t, fs.ToDisk()) - assert.Nil(t, fs.FromDisk(id)) - ss, cs, err = fs.GetStates() + ss.State = "running" + 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) diff --git a/virtcontainers/persist_test.go b/virtcontainers/persist_test.go index 425b33d6c..12f359842 100644 --- a/virtcontainers/persist_test.go +++ b/virtcontainers/persist_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" + "github.com/kata-containers/runtime/virtcontainers/device/manager" exp "github.com/kata-containers/runtime/virtcontainers/experimental" "github.com/kata-containers/runtime/virtcontainers/persist" "github.com/kata-containers/runtime/virtcontainers/types" @@ -74,6 +75,7 @@ func TestSandboxRestore(t *testing.T) { sandbox := Sandbox{ id: "test-exp", containers: container, + devManager: manager.NewDeviceManager(manager.VirtioSCSI, nil), hypervisor: &mockHypervisor{}, ctx: context.Background(), config: &sconfig, @@ -89,7 +91,7 @@ func TestSandboxRestore(t *testing.T) { assert.True(t, os.IsNotExist(err)) // disk data are empty - err = sandbox.newStore.ToDisk() + err = sandbox.Save() assert.Nil(t, err) err = sandbox.Restore() @@ -98,14 +100,12 @@ func TestSandboxRestore(t *testing.T) { assert.Equal(t, sandbox.state.GuestMemoryBlockSizeMB, uint32(0)) assert.Equal(t, sandbox.state.BlockIndex, 0) - // register callback function - sandbox.stateSaveCallback() - + // set state data and save again sandbox.state.State = types.StateString("running") sandbox.state.GuestMemoryBlockSizeMB = uint32(1024) sandbox.state.BlockIndex = 2 // flush data to disk - err = sandbox.newStore.ToDisk() + err = sandbox.Save() assert.Nil(t, err) // empty the sandbox @@ -116,18 +116,5 @@ func TestSandboxRestore(t *testing.T) { assert.Nil(t, err) assert.Equal(t, sandbox.state.State, types.StateString("running")) assert.Equal(t, sandbox.state.GuestMemoryBlockSizeMB, uint32(1024)) - // require hvStateSaveCallback to make it savable - assert.Equal(t, sandbox.state.BlockIndex, 0) - - // after use hvStateSaveCallbck, BlockIndex can be saved now - sandbox.state.BlockIndex = 2 - sandbox.hvStateSaveCallback() - err = sandbox.newStore.ToDisk() - assert.Nil(t, err) - err = sandbox.Restore() - assert.Nil(t, err) - assert.Equal(t, sandbox.state.State, types.StateString("running")) - assert.Equal(t, sandbox.state.GuestMemoryBlockSizeMB, uint32(1024)) assert.Equal(t, sandbox.state.BlockIndex, 2) - } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 3ade96204..e013426b0 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -481,18 +481,10 @@ func createSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Fac s.devManager = deviceManager.NewDeviceManager(sandboxConfig.HypervisorConfig.BlockDeviceDriver, devices) if s.supportNewStore() { - // register persist hook for now, data will be written to disk by ToDisk() - s.stateSaveCallback() - s.hvStateSaveCallback() - s.devicesSaveCallback() - if err := s.Restore(); err == nil && s.state.State != "" { return s, nil } - // if sandbox doesn't exist, set persist version to current version - // otherwise do nothing - s.verSaveCallback() } else { // We first try to fetch the sandbox state from storage. // If it exists, this means this is a re-creation, i.e. @@ -619,7 +611,7 @@ func (s *Sandbox) storeSandbox() error { if s.supportNewStore() { // flush data to storage - if err := s.newStore.ToDisk(); err != nil { + if err := s.Save(); err != nil { return err } } diff --git a/virtcontainers/types/sandbox.go b/virtcontainers/types/sandbox.go index 731736516..2259e86d5 100644 --- a/virtcontainers/types/sandbox.go +++ b/virtcontainers/types/sandbox.go @@ -43,6 +43,11 @@ type SandboxState struct { // CgroupPath is the cgroup hierarchy where sandbox's processes // including the hypervisor are placed. CgroupPath string `json:"cgroupPath,omitempty"` + + // PersistVersion indicates current storage api version. + // It's also known as ABI version of kata-runtime. + // Note: it won't be written to disk + PersistVersion uint `json:"-"` } // Valid checks that the sandbox state is valid.