Virtcontainers store by sameo · Pull Request #1066 · kata-containers/runtime

sameo · 2018-12-20T18:08:35Z

Description and goals

This is a rewrite of the resource_storage and its filesystem_resource_storage backend, with the following goals:

Provide a neutral store API for virtcontainers. The API and the physical backend should be decoupled.
Provide a filesystem backend implementation and make it the default one for virtcontainers.
Provide a separate store package for the core virtcontainers code to consume. Since several components are depending on a store API, this will break these dependencies and allow us to move virtcontainers code base is monolithic #1059 further away.

Shortcomings

There are a lot of places in the virtcontainers code where we assume that our store backend will be a local filesystem one. This is practically just right but ideally we should not make any of those assumptions. This PR does not completely remove those assumptions, but the store package top level API is neutral enough so that we could eventually remove them from the core virtcontainers code.

egernst · 2019-01-07T17:18:52Z

Happy new year @sameo

I'm looking forward to seeing more dialogue on this PR w.r.t. the store itself.

Does it make sense to create a seperate PR for the commits which make use of the 'internal types' pkg? That should be ready to go as is? Would like to at least get that in for 1.5 (scheduled week of 1/23). WDYT?

sameo · 2019-01-08T08:56:15Z

@egernst Bonne Année a toi aussi!

I will extract the types package out of this PR and create a separate one today.

sameo · 2019-01-08T11:35:33Z

@egernst Bonne Année a toi aussi!

I will extract the types package out of this PR and create a separate one today.

@egernst See #1096

sameo · 2019-01-09T16:59:23Z

@egernst @sboeuf @jodh-intel @bergwolf @WeiZhang555
So I can start docker containers cleanly with this new storage implementation.
CI is probably going to shout at me, but I think this is ready to be reviewed.

sameo · 2019-01-10T18:15:31Z

Unit tests pass locally...

sboeuf

@sameo
I have several comments regarding this PR in the code, but here is the summary:

It globally looks good, and I really appreciate the way commits are split, it's neat and easy to understand.
I'm wondering if we're not missing the port of the specific implementations retrieving special devices.
I have some doubt this is really simplifying the existing code. The whole manager is neat but maybe a bit too much since virtcontainers will be the only consumer of this. Having a VCStore implementation calling into Store who calls into the backend interface eventually seems a lot of layers to me when I don't think anything else will reuse this package.

sboeuf · 2019-01-10T18:45:13Z

+)
+
+// Item represents a virtcontainers items that will be managed through the store.
+type Item uint8


Is it really important to put this on 8bits?

I want it to be a specific type. Are you suggesting this should be a string instead?

No, no, I was just thinking this could be a simple uint, but doesn't matter.

sboeuf · 2019-01-10T21:13:37Z

 	delete()
 	load(item Item, data interface{}) error
 	store(item Item, data interface{}) error
+	raw(id string) (string, error)


Could you please highlight in the commit message and through some comments in the code, some concrete use cases for this new API.

I did. Please let me know if this looks better.

Yes it helps a lot to understand :)
IMO, you should even mention

// For example the Firecracker code uses this API to create temp // files under the sandbox state root path, and uses them as block // driver backend placeholder.

from the commit message.

sboeuf · 2019-01-10T21:22:53Z

 	load(item Item, data interface{}) error
 	store(item Item, data interface{}) error
 	raw(id string) (string, error)
+	lock(item Item, exclusive bool) (string, error)


I don't get why we need those new locking API. If I followed until then, we have any Load() performing a read only lock, and Store() performing a read/write lock, which looks like it should be enough. Also, from the Kata API perspective, we have the locking of the pod, meaning we cannot access two containers of the same pod concurrently.

Ohh maybe I understand now, are you trying to do the pod locking through this API? And you're doing this because the lock file has to be created somewhere... It's a tricky one because it's not natural to have the locking being handled through the storage, but I guess that's how it was kind of working already.

Unless we share the global path for pods (in types maybe), and we could create a separate lock/ package.

@sameo did you miss this comment?

sboeuf · 2019-01-10T21:31:36Z

-		l.Info("Device type found")
-
-		switch d.Type {
-		case string(config.DeviceVFIO):


Where have you moved specific fetch implementation related to devices? I could not find it in virtcontainers/store/filesystem.go

Right, I'm fixing that now...

I need to add a lot more unit tests to vc_test.go

sameo · 2019-01-11T10:35:38Z

/test

sameo · 2019-01-11T12:23:16Z

/test

It's going to be used to completely clean a Store away. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

This is basically a Store dispatcher, for storing items into their right Store (either configuration or state). There's very little logic here, except for finding out which store an item belongs to in the virtcontainers context. vc.go also provides virtcontainers specific utilities. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

The Raw API creates a raw item, i.e. an item that must be handled directly by the caller. A raw item is one that's not defined by the store.Item enum, i.e. it is a custom, caller defined one. The caller gets a URL back and is responsible for handling the item directly from this URL. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

The ItemLock API allows for taking shared and exclusive locks on all items. For virtcontainers, this is specialized into taking locks on the Lock item, and will be used for sandbox locking. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo · 2019-02-06T13:31:13Z

Basic CI passes, let's retest.

sameo · 2019-02-06T13:31:20Z

/retest

We convert the whole virtcontainers code to use the store package instead of the resource_storage one. The resource_storage removal will happen in a separate change for a more logical split. This change is fairly big but mostly does not change the code logic. What really changes is when we create a store for a container or a sandbox. We now need to explictly do so instead of just assigning a filesystem{} instance. Other than that, the logic is kept intact. Fixes: kata-containers#1099 Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

Now that we converted the virtcontainers code to the store package, we can remove all the resource storage old code. Fixes: kata-containers#1099 Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

With the Stores conversion, the newContainer() cyclomatic complexity went over 15. We fix that by extracting the block devices creation routine out of newContainer. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

We are creating Store directories but never removing them. Calling into a VM factory created vm Stop() will now clean the VM Store artifacts up. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo · 2019-02-07T00:04:20Z

I just fixed a network NS leak caused by a typo in the Store conversion patch.

sameo · 2019-02-07T00:04:28Z

/retest

sameo · 2019-02-07T10:41:46Z

@sboeuf @WeiZhang555 @bergwolf @jodh-intel The CI passes now.

WeiZhang555 · 2019-02-11T08:06:19Z

LGTM

Let's retest it again before it can be merged.

/retest

WeiZhang555

LGTM

sameo · 2019-02-11T15:33:32Z

@WeiZhang555 The fc and ARM CI are broken, as expected :-/

The store refactor (kata-containers#1066) inadvertently broke runtime tracing as it created new contexts containing trace spans. Reworking the store changes to re-use the existing context resolves the problem since runtime tracing assumes a single context. Fixes kata-containers#1277. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

sameo force-pushed the topic/state-storage branch 3 times, most recently from 596b3fe to 6cb93e6 Compare December 21, 2018 17:46

sameo force-pushed the topic/state-storage branch from 6cb93e6 to a6f776b Compare January 7, 2019 16:03

sameo force-pushed the topic/state-storage branch 3 times, most recently from 23f026c to e7cf9cd Compare January 8, 2019 15:44

sameo added the do-not-merge label Jan 8, 2019

sameo changed the title ~~[RFC] [WIP] [DNM] Virtcontainers store~~ [RFC] Virtcontainers store Jan 8, 2019

sameo force-pushed the topic/state-storage branch from e7cf9cd to ce86229 Compare January 9, 2019 16:56

sameo removed the do-not-merge label Jan 9, 2019

sameo changed the title ~~[RFC] Virtcontainers store~~ Virtcontainers store Jan 9, 2019

sameo force-pushed the topic/state-storage branch 5 times, most recently from 73b15c6 to c8e3852 Compare January 10, 2019 18:15

sameo force-pushed the topic/state-storage branch 2 times, most recently from fabbd8a to 0d8c289 Compare January 10, 2019 18:55

sboeuf reviewed Jan 10, 2019

View reviewed changes

sameo force-pushed the topic/state-storage branch from 0d8c289 to 3608420 Compare January 11, 2019 09:53

sameo force-pushed the topic/state-storage branch from 3608420 to 04dec93 Compare January 11, 2019 10:54

sameo force-pushed the topic/state-storage branch from 04dec93 to 375a1a5 Compare January 11, 2019 13:33

Samuel Ortiz added 4 commits February 6, 2019 14:19

virtcontainers: store: Add a Delete API

ef11bf5

It's going to be used to completely clean a Store away. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

virtcontainers: store: Add a ItemLock API

2ecffda

The ItemLock API allows for taking shared and exclusive locks on all items. For virtcontainers, this is specialized into taking locks on the Lock item, and will be used for sandbox locking. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo force-pushed the topic/state-storage branch from e283ff1 to a2a73d4 Compare February 6, 2019 13:23

Samuel Ortiz added 4 commits February 7, 2019 00:59

virtcontainers: Remove the resource storage original implementation

f8e7e30

Now that we converted the virtcontainers code to the store package, we can remove all the resource storage old code. Fixes: kata-containers#1099 Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

virtcontainers: Fix container.go cyclomatic complexity

7b0376f

With the Stores conversion, the newContainer() cyclomatic complexity went over 15. We fix that by extracting the block devices creation routine out of newContainer. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

virtcontainers: Fix Store related vm factory leak

bb99e41

We are creating Store directories but never removing them. Calling into a VM factory created vm Stop() will now clean the VM Store artifacts up. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

sameo force-pushed the topic/state-storage branch from a2a73d4 to bb99e41 Compare February 7, 2019 00:01

sameo requested review from WeiZhang555, jodh-intel and sboeuf February 7, 2019 10:42

sboeuf approved these changes Feb 7, 2019

View reviewed changes

WeiZhang555 approved these changes Feb 11, 2019

View reviewed changes

egernst approved these changes Feb 11, 2019

View reviewed changes

egernst merged commit 6431f1f into kata-containers:master Feb 11, 2019

jodh-intel mentioned this pull request Feb 25, 2019

Runtime tracing broken #1277

Closed

egernst mentioned this pull request Feb 26, 2019

Stable 1.5.1 rc #1281

Merged

jodh-intel mentioned this pull request Feb 28, 2019

Add SetLogger() to store package #1297

Closed

jodh-intel mentioned this pull request Mar 4, 2019

tracing: Fix tracing #1317

Merged

jodh-intel mentioned this pull request Jun 5, 2020

Use single storage implementation for Kata 2.0 kata-containers/kata-containers#252

Closed

Conversation

sameo commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description and goals

Shortcomings

Uh oh!

egernst commented Jan 7, 2019

Uh oh!

sameo commented Jan 8, 2019

Uh oh!

sameo commented Jan 8, 2019

Uh oh!

sameo commented Jan 9, 2019

Uh oh!

sameo commented Jan 10, 2019

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sameo commented Jan 11, 2019

Uh oh!

sameo commented Jan 11, 2019

Uh oh!

sameo commented Feb 6, 2019

Uh oh!

sameo commented Feb 6, 2019

Uh oh!

sameo commented Feb 7, 2019

Uh oh!

sameo commented Feb 7, 2019

Uh oh!

sameo commented Feb 7, 2019

Uh oh!

WeiZhang555 commented Feb 11, 2019

Uh oh!

WeiZhang555 left a comment

Choose a reason for hiding this comment

Uh oh!

sameo commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sameo commented Dec 20, 2018 •

edited

Loading