persist: baseline persist data format #883

WeiZhang555 · 2018-11-05T03:09:47Z

Fixes #803

This demonstrate how to use new virtcontainers/persist/api/ package brought by #874

~~This PR is only for demo, please don't merge it.~~

This is ready for review and merge now :-)

WeiZhang555 · 2018-11-05T03:14:35Z

/test

WeiZhang555 · 2018-11-06T08:05:08Z

/test

raravena80 · 2018-12-20T22:26:05Z

@WeiZhang555 any updates?

jodh-intel

Hi @WeiZhang555 - This is interesting! A few initial comments (more tomorrow probably).

A couple of other thoughts:

We'll need super-careful tests for this.

If we do use CurPersistVersion, we need set of valid and invalid state files for every value of CurPersistVersion so that we can assert the expected behaviour in unit tests.
Features like this are hard to test

Hence, it would be very useful to be able to request the runtime dump the state at any time (to the journal or an arbitrary file) either by:
- adding a new sub-command (kata-runtime kata-state show or something).
- adding a SIGUSR2 signal handler (killall -USR2 kata-runtime).

virtcontainers/persist/fs/fs.go

jodh-intel · 2019-01-08T12:32:37Z

virtcontainers/persist.go

+)
+
+// PersistVersion set persist data version to current version in runtime
+func (s *Sandbox) PersistVersion() {


The names of these functions suggest that as soon as you call them, the data will be persisted to disk. But that doesn't appear to be what is happening - persistence only occurs when Dump() is called. This is potentially misleading.

How about:

Renaming Dump() to Save() (clearer as Dump almost suggests some sort of debug operation to me).

Renaming Persist*() to AddToTransaction() or something like that (AddToTrn()?) as that makes it clear this isn't actually saving any data (yet).

Nice suggestion! I'll rename the functions, thanks!

I just reworked the whole PR, and renamed PersistVersion to versionSaveCallback, and Dump to ToDisk. Not sure if this is clear enough for users.
Anyway, comments addressed

virtcontainers/sandbox.go

virtcontainers/persist/fs/fs.go

jodh-intel · 2019-01-08T15:23:12Z

virtcontainers/persist/fs/fs.go

+
+		fs.containerState[cid] = cstate
+	}
+	return nil


I appreciate this is a demo only, but:

There is no validation performed on the decoded values.

There is no check on whether CurPersistVersion looks reasonable (what if it was set to 99 in the on-disk file)?

CurPersistVersion is hardcoded, so it will always be valid. Restoring from disk can get a 99 version number, I can add some validation during restoring.

Not sure currently. Let's keep it as it is for now :-)

jodh-intel

Just a few comments.

Ping @kata-containers/runtime - this is a big PR which needs more eyes on it!

You can start by looking just at virtcontainers/persist/api/ (or just heading over to the corresponding #874).

virtcontainers/persist/manager.go

virtcontainers/persist/api/interface.go

WeiZhang555 · 2019-01-24T01:54:20Z

@jodh-intel Thanks for reviewing!

Recently I was busy on other stuffs, the PR can work but test cases would need some fixes. I will try to rebase and rework to make it work soon.

raravena80 · 2019-02-04T20:41:55Z

@WeiZhang555 ping, any updates? Thx

Save and restore state from persist.json instead of state.json Signed-off-by: Wei Zhang <[email protected]>

Address some comments: * fix persist driver func names for better understanding * modify some logic, add some returned error etc Signed-off-by: Wei Zhang <[email protected]>

Set new persist storage driver "virtcontainers/persist/" as "experimental" feature. One day when this can fully work and we're ready to move to 2.0, we'll move it from "experimental" feature to formal feature. At that time, the "virtcontainers/filesystem_resource_storage.go" can be removed completely. Signed-off-by: Wei Zhang <[email protected]>

For experimental features, state.json won't be updated, so modify some unit test to skip. Signed-off-by: Wei Zhang <[email protected]>

add more unit tests. Signed-off-by: Wei Zhang <[email protected]>

Address review comments Signed-off-by: Wei Zhang <[email protected]>

* Fix potential panic by nil pointer. * Address comments. Signed-off-by: Wei Zhang <[email protected]>

WeiZhang555 · 2019-04-19T08:15:45Z

/test

WeiZhang555 · 2019-04-19T14:03:46Z

All tests are green now 🍾 🎆

The last commit is for testing the experimental new store driver, which must be deprecated before merging. I'll remove the last commit for testing compatibility with original store

Last commit:

diff --git a/Makefile b/Makefile
index 072aca5..13c81fa 100644
--- a/Makefile
+++ b/Makefile
@@ -158,7 +158,7 @@ DEFMEMSLOTS := 10
 DEFBRIDGES := 1
 DEFDISABLEGUESTSECCOMP := true
 #Default experimental features enabled
-DEFAULTEXPFEATURES := []
+DEFAULTEXPFEATURES := "[\"newstore\"]"

WeiZhang555 · 2019-04-19T14:05:49Z

/test

sboeuf

@WeiZhang555 The PR looks pretty good (two comments in the code), but I have two concerns:

You use the same store wording that is already part of virtcontainers to store sandbox and containers data already with the store API located at virtcontainers/store.
This PR duplicates the way to store data, which should rely on the virtcontainers/store code instead. Is it something that you're planning fix as follow up PR?

sboeuf · 2019-04-19T16:28:33Z

virtcontainers/persist.go

+	errContainerPersistNotExist = errors.New("container doesn't exist in persist data")
+)
+
+func (s *Sandbox) dumpState(ss *persistapi.SandboxState, cs map[string]persistapi.ContainerState) error {


Global comment for this file:
Having new Sandbox methods out of the sandbox.go file sounds pretty confusing to me.

I can move them to sandbox.go, I am worried that sandbox.go is already so large, the single file has over 1800 lines, that's why I put the store part to one single file.

I can merge this file into sandbox.go in a following PR if you insist .

Ok course you can do that in a follow up PR :)
But I'd like to have input from @bergwolf @amshinde on this too.

ping @bergwolf @amshinde again, let's finish the discussion :)

virtcontainers/persist/manager.go

WeiZhang555 · 2019-04-20T01:15:55Z

/test

WeiZhang555 · 2019-04-20T01:22:12Z

@sboeuf

You use the same store wording that is already part of virtcontainers to store sandbox and containers data already with the store API located at virtcontainers/store.
This PR duplicates the way to store data, which should rely on the virtcontainers/store code instead. Is it something that you're planning fix as follow up PR?

No, the PR is meant to replace virtcontainers/store, they store data in different ways so can't share interface and logics, the final goal is to remove virtcontainers/store totally and rename newStore to store in release 2.0

sboeuf · 2019-04-20T01:30:49Z

@WeiZhang555

No, the PR is meant to replace virtcontainers/store, they store data in different ways so can't share interface and logics, the final goal is to remove virtcontainers/store totally and rename newStore to store in release 2.0

Ah that makes sense, thanks for the clarification. Make sure this is highly documented as this might create some confusion from a code base perspective.

sboeuf · 2019-04-20T01:32:54Z

LGTM

Modify lisense header from 2018 to 2019. Signed-off-by: Wei Zhang <[email protected]>

WeiZhang555 · 2019-04-20T02:05:17Z

/test

WeiZhang555 · 2019-04-20T04:03:48Z

ping @kata-containers/ci

We need a separate CI job for testing experimental features since experimental feature can run in different code path as this PR does.

I think we can reuse the exisiting job, such as jenkins-ci-ubuntu-18-04, the chosing Job must be "required" since breaking experimental features are also not allowed.

The modification for this experimental feature is adding one single line to configuration.toml

experimental=["newstore"]

Thanks!

chavafg · 2019-04-25T18:58:18Z

@WeiZhang555 I have added an envar KATA_EXPERIMENTAL_FEATURES=true to the Fedora jobs, which I will set as "required" in github.
I opened PR kata-containers/tests#1501 in the tests repo to make the necessary changes on the configuration.toml, ptal.

WeiZhang555 · 2019-04-26T02:26:23Z

Thanks @chavafg !

Cross refs to kata-containers/tests#1501 and copying comments here:

I checked the CI job metrics: https://github.com/kata-containers/ci#ci-job-matrix and found that fedora didn't cover all test cases. Is that possible to also enable K8s test in Fedora Job? Since I found that my WIP PR (#1575) about newstore feature was failing the K8s test and Fedora test doesn't cover it.

WeiZhang555 added the do-not-merge label Nov 5, 2018

WeiZhang555 mentioned this pull request Nov 5, 2018

[WIP][RFC]persist: baseline persist data format #874

Closed

3 tasks

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch 3 times, most recently from c2c472c to 06728ae Compare November 6, 2018 02:58

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from 06728ae to 5f57ee5 Compare November 26, 2018 01:34

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch 3 times, most recently from 209428c to 80827f9 Compare January 7, 2019 03:32

This comment has been minimized.

Sign in to view

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch 2 times, most recently from df17755 to e58557b Compare January 7, 2019 14:34

This comment has been minimized.

Sign in to view

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from e58557b to f47580b Compare January 8, 2019 10:59

This comment has been minimized.

Sign in to view

jodh-intel reviewed Jan 8, 2019

View reviewed changes

jodh-intel mentioned this pull request Jan 8, 2019

Replace the resourceStorage implementation #1099

Closed

WeiZhang555 mentioned this pull request Jan 13, 2019

Virtcontainers store #1066

Merged

jodh-intel reviewed Jan 23, 2019

View reviewed changes

virtcontainers/persist/manager.go Outdated Show resolved Hide resolved

virtcontainers/persist/manager.go Outdated Show resolved Hide resolved

virtcontainers/persist/api/interface.go Outdated Show resolved Hide resolved

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from a373ba2 to 122fe74 Compare February 8, 2019 14:52

WeiZhang555 requested a review from a team as a code owner February 8, 2019 14:52

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch 3 times, most recently from 78aad02 to 3a4a50f Compare February 8, 2019 18:22

WeiZhang555 added 6 commits April 19, 2019 15:33

persist: save and restore state from persist.json

6e4149d

Save and restore state from persist.json instead of state.json Signed-off-by: Wei Zhang <[email protected]>

storage: address comments

504c706

Address some comments: * fix persist driver func names for better understanding * modify some logic, add some returned error etc Signed-off-by: Wei Zhang <[email protected]>

test: fix unit test

02f2122

For experimental features, state.json won't be updated, so modify some unit test to skip. Signed-off-by: Wei Zhang <[email protected]>

test: add unit test for new FS storage driver

0f52c8b

add more unit tests. Signed-off-by: Wei Zhang <[email protected]>

store: address comments

9bd4e50

Address review comments Signed-off-by: Wei Zhang <[email protected]>

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from 5ae8dd5 to 5c384f3 Compare April 19, 2019 07:38

bugfix: fix potential panic

3262da0

* Fix potential panic by nil pointer. * Address comments. Signed-off-by: Wei Zhang <[email protected]>

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from 5c384f3 to 40bc2ca Compare April 19, 2019 08:05

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from 40bc2ca to 3262da0 Compare April 19, 2019 14:05

sboeuf reviewed Apr 19, 2019

View reviewed changes

klynnrif approved these changes Apr 19, 2019

View reviewed changes

sboeuf approved these changes Apr 20, 2019

View reviewed changes

docs: fix lisence header to 2019

989b373

Modify lisense header from 2018 to 2019. Signed-off-by: Wei Zhang <[email protected]>

WeiZhang555 force-pushed the rfc-persist-data-standard-wip branch from 5ce73fa to 989b373 Compare April 20, 2019 02:05

gnawux merged commit b218229 into kata-containers:master Apr 20, 2019

WeiZhang555 deleted the rfc-persist-data-standard-wip branch April 20, 2019 04:35

WeiZhang555 mentioned this pull request Apr 23, 2019

newstore: removing deprecated files when use new store driver #1575

Merged

WeiZhang555 mentioned this pull request Apr 26, 2019

CI: add condition to be able to test experimental feature kata-containers/tests#1501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persist: baseline persist data format #883

persist: baseline persist data format #883

WeiZhang555 commented Nov 5, 2018 •

edited by gabibeyer

Loading

WeiZhang555 commented Nov 5, 2018

WeiZhang555 commented Nov 6, 2018

raravena80 commented Dec 20, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jodh-intel left a comment

jodh-intel Jan 8, 2019

WeiZhang555 Jan 13, 2019

WeiZhang555 Feb 8, 2019

jodh-intel Jan 8, 2019

WeiZhang555 Jan 13, 2019

WeiZhang555 Feb 8, 2019

jodh-intel left a comment

WeiZhang555 commented Jan 24, 2019

raravena80 commented Feb 4, 2019

WeiZhang555 commented Apr 19, 2019

WeiZhang555 commented Apr 19, 2019

WeiZhang555 commented Apr 19, 2019

sboeuf left a comment

sboeuf Apr 19, 2019

WeiZhang555 Apr 20, 2019 •

edited

Loading

sboeuf Apr 20, 2019

WeiZhang555 Apr 23, 2019

WeiZhang555 commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

sboeuf commented Apr 20, 2019

sboeuf commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

chavafg commented Apr 25, 2019

WeiZhang555 commented Apr 26, 2019 •

edited

Loading

persist: baseline persist data format #883

persist: baseline persist data format #883

Conversation

WeiZhang555 commented Nov 5, 2018 • edited by gabibeyer Loading

WeiZhang555 commented Nov 5, 2018

WeiZhang555 commented Nov 6, 2018

raravena80 commented Dec 20, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

WeiZhang555 commented Jan 24, 2019

raravena80 commented Feb 4, 2019

WeiZhang555 commented Apr 19, 2019

WeiZhang555 commented Apr 19, 2019

WeiZhang555 commented Apr 19, 2019

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 Apr 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

sboeuf commented Apr 20, 2019

sboeuf commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

WeiZhang555 commented Apr 20, 2019

chavafg commented Apr 25, 2019

WeiZhang555 commented Apr 26, 2019 • edited Loading

WeiZhang555 commented Nov 5, 2018 •

edited by gabibeyer

Loading

WeiZhang555 Apr 20, 2019 •

edited

Loading

WeiZhang555 commented Apr 26, 2019 •

edited

Loading