virtcontainers: remove ConfigJSONKey annotation #216

WeiZhang555 · 2018-04-16T12:28:35Z

Currently we already saved "BundlePath" in sandbox and config's config, with
BundlePath, we can easily get config of json format from "BundlePath"/config.json,
So ConfigJSONKey is a duplicate and unnecessary field and we can remove it for
saving more space from persist data.

Fixes: #215

Signed-off-by: Zhang Wei [email protected]

WeiZhang555 · 2018-04-16T12:43:22Z

CI failed.
Adding a [WIP] for fixing the CI

WeiZhang555 · 2018-04-17T09:18:33Z

CI passed now, this is ready for review

ping @bergwolf @sboeuf @egernst @sameo

sboeuf · 2018-04-17T14:03:36Z

Thanks @WeiZhang555 I'll take a look

bergwolf · 2018-04-17T14:13:19Z

virtcontainers/kata_agent.go

+// better find a way to eliminate duplicate codes @weizhang555
+func ParseConfigJSON(bundlePath string) (*specs.Spec, error) {
+	configPath := filepath.Join(bundlePath, "config.json")
+	configByte, err := ioutil.ReadFile(configPath)


With this, we are pushing the cli dependence on config.json file to virtcontainers. If we are certain that the content of OCI annotations is exactly the same as config.json, and assuming the purpose of the PR is to reduce persistent data, can we still let the cli read config.json and pass its contents to virtcontainers, and skip persisting the OCI spec in virtcontainers?

Then it will need to filter out "configJSON" content every time we try to persist the data, it could be ugly codes and everywhere.

Are you worrying about the operation performance? I think it will influence a little on performance because it will need to open files more times, but the influence won't be much in my opinion. Though I don't have a test data. 😄

@WeiZhang555 I'm thinking about the following things:

We do not change the image bundle's config.json in hyperd for a container one. I think we can deal with it if required.

This requires config.json to be exactly as what's in the annotations. Are we certain about it?

If we ever change the OCI spec in virtcontainers, do we update the config.json file?

I didn't fully get your concerns. But in theory, I think it's better virtcontainers do never change config.json file because it's an input from upper level component or user, and we can add custom or additional modifications into virtcontainer's persist data but not original bundle config.json file.
Is this what you want to discuss?

Yes, that's it. We do change the OCI spec in certain cases (e.g., #138) and we do need to persist the new OCI spec in such case to allow future fetchSandbox() caller to get the same spec view of the container. I agree with you that we should NOT change the contents of config.json. So it means we need to find a way to persist optional modification to the spec and merge with config.json when reading it. If you do not want to handle it in this PR, we need to file an issue to make sure it gets implemented later.

but we don't want to persist the modified ones

That's what I wanted to fix. If we decide not to persist the modified spec, that's a design decision and we need to be really careful handling it because we are not recording what's sent to the agent and other virtcontainers API calls are getting different OCI spec than the one sent to the agent.

I think this is by design, at least in the case you provided.

For example, if we want to pass a volume, OCI spec should say "mount /host/a to /guest/b", but virtcontainer should first mount /host/a to a 9pfs dir, then send a message to kata-agent to tell it "mount /host/virtcontainers/9pfs/host/a to /guest/b". But for virtcontainers in host, it doesn't need to persist message "mount /host/virtcontainers/9pfs/host/a to /guest/b" in host. So still the OCI spec in host doesn't need to be modified in such case.
What do you think? I think this is good for now, unless in future we really have a need to modify the original OCI spec.

Fair enough. createContainer() in kata_agent.go seems to be the only user of the OCI spec in virtcontainers. I might just be over worried.

Another thing is container hooks. If we read the spec so late, how do we handle pre-start hooks?

@bergwolf for container hooks, it will be stored inside SandboxConfig from OCI spec, so when the sandbox runs the hooks, it doesn't need to read OCI spec again.

https://github.com/kata-containers/runtime/blob/master/virtcontainers/pkg/oci/utils.go#L494

oops, sorry I missed it. We are reading the OCI spec file twice, one in cli.Create and the other in virtcontainers.createContainer.

bergwolf · 2018-04-18T08:11:08Z

cli/tags

@@ -0,0 +1,1172 @@
+!_TAG_FILE_FORMAT	2


btw, this file seems to be a mistake.

Oh, no, this is terrible.

I added this by mistake, this is generated by gotags
Thanks!

bergwolf · 2018-04-18T08:47:49Z

virtcontainers/kata_agent.go

+// better find a way to eliminate duplicate codes @weizhang555
+func ParseConfigJSON(bundlePath string) (*specs.Spec, error) {
+	configPath := filepath.Join(bundlePath, "config.json")
+	configByte, err := ioutil.ReadFile(configPath)


Fair enough. createContainer() in kata_agent.go seems to be the only user of the OCI spec in virtcontainers. I might just be over worried.

Another thing is container hooks. If we read the spec so late, how do we handle pre-start hooks?

codecov · 2018-04-18T09:23:02Z

Codecov Report

Merging #216 into master will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   65.31%   65.27%   -0.04%     
==========================================
  Files          74       73       -1     
  Lines        7740     7713      -27     
==========================================
- Hits         5055     5035      -20     
+ Misses       2135     2134       -1     
+ Partials      550      544       -6

Impacted Files	Coverage Δ
virtcontainers/kata_agent.go	`32.6% <0%> (-0.73%)`	⬇️
cli/create.go	`80.48% <100%> (-0.88%)`	⬇️
virtcontainers/pkg/oci/utils.go	`78.68% <62.5%> (+1%)`	⬆️
virtcontainers/hypervisor.go	`72.09% <0%> (-0.33%)`	⬇️
cli/config.go	`88.93% <0%> (-0.27%)`	⬇️
cli/kill.go	`100% <0%> (ø)`	⬆️
virtcontainers/noop_network.go	`75% <0%> (ø)`	⬆️
cli/main.go	`92.37% <0%> (ø)`	⬆️
virtcontainers/utils.go	`70% <0%> (ø)`	⬆️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea789db...20d199c. Read the comment docs.

bergwolf

I'm still a bit uncomfortable with making the config.json file part of the API. But I can't find a hard blocker for the PR. And I guess we are tied to the image spec any way.

Maybe we can pass the OCI spec and new arguments to CreateSandbox() and CreateContainer() and thus do not have to persist it with sandbox config. I'm not sure.

I'll let you guys decide and live with group consensus. cc @sboeuf @laijs

bergwolf · 2018-04-18T10:08:11Z

virtcontainers/kata_agent.go

+// better find a way to eliminate duplicate codes @weizhang555
+func ParseConfigJSON(bundlePath string) (*specs.Spec, error) {
+	configPath := filepath.Join(bundlePath, "config.json")
+	configByte, err := ioutil.ReadFile(configPath)


oops, sorry I missed it. We are reading the OCI spec file twice, one in cli.Create and the other in virtcontainers.createContainer.

Currently we already saved "BundlePath" in sandbox and config's config, with `BundlePath`, we can easily get config of json format from "BundlePath"/config.json, So ConfigJSONKey is a duplicate and unnecessary field and we can remove it for saving more space from persist data. Fixes: kata-containers#215 Signed-off-by: Zhang Wei <[email protected]>

sboeuf

@WeiZhang555 I am sorry but this PR is a no go for me. Yes, we store both the ConfigJSONKey and the BundlePathKey, but this allows for less filesystem accesses. Your PR introduces more accesses to the filesystem and more Unmarshalling functions than the code before, because you rely on the bundle path instead of the byte array describing the config.json.
In case of a container creation, your code calls twice the same function to retrieve the ociSpec, which means too much for no apparent reason/gain.

sboeuf · 2018-04-19T22:47:17Z

-1

WeiZhang555 · 2018-04-20T03:17:25Z

@sboeuf I understand your concerns.

. Your PR introduces more accesses to the filesystem and more Unmarshalling functions than the code before,

The only concern for me is it truly introduced more accesses to the filesystem, but it didn't introduce more Unmarshalling functions, because in original code, the spec data is stored in annotation as string, you still need to unmarshal it to specs.Spec in every time you want to use it.

If you check the sandbox and container's config in virtcontainers, you can see the spec data is stored in 3 part, that means the content in config.json is duplicated and we save extra 3 exactly same data in 3 different positions. That's why I want to remove this because it's not really a necessary part.

I think I will find a better way to reduce the filesystem access, maybe at least merge three part of config.json into one.

WeiZhang555 · 2018-04-20T03:18:16Z

I'm closing this right now, and will try if I can find a better implementation for reducing FS access

sboeuf · 2018-04-20T07:43:01Z

@WeiZhang555 Thanks. I am always down for some code optimizations :)

WeiZhang555 · 2018-04-20T09:23:38Z

@sboeuf Never mind, I believe I can find a better way 😃

unix.SockaddrNetlink represents a netlink client with the Pid being the netlink socket address. This can be assigned the actual pid of the process, but in case we have two netlink sockets opened at the same time, this results in errors in binding to the netlink socket. Assign this to zero which means the kernel takes care of assigning it. See http://man7.org/linux/man-pages/man7/netlink.7.html Fixes kata-containers#216 Signed-off-by: Archana Shinde <[email protected]>

amshinde added the review label Apr 16, 2018

WeiZhang555 force-pushed the to-do branch from c770a7d to 29d2a54 Compare April 16, 2018 12:35

WeiZhang555 changed the title ~~Remove ConfigJSONKey annotation~~ virtcontainers: remove ConfigJSONKey annotation Apr 16, 2018

WeiZhang555 changed the title ~~virtcontainers: remove ConfigJSONKey annotation~~ [wip]virtcontainers: remove ConfigJSONKey annotation Apr 16, 2018

WeiZhang555 force-pushed the to-do branch 4 times, most recently from fbdcb79 to 3997b01 Compare April 17, 2018 06:34

WeiZhang555 changed the title ~~[wip]virtcontainers: remove ConfigJSONKey annotation~~ virtcontainers: remove ConfigJSONKey annotation Apr 17, 2018

bergwolf reviewed Apr 17, 2018

View reviewed changes

bergwolf reviewed Apr 18, 2018

View reviewed changes

WeiZhang555 force-pushed the to-do branch from 3997b01 to 951dbf4 Compare April 18, 2018 09:22

bergwolf reviewed Apr 18, 2018

View reviewed changes

WeiZhang555 force-pushed the to-do branch from 951dbf4 to 20d199c Compare April 19, 2018 06:21

sboeuf suggested changes Apr 19, 2018

View reviewed changes

WeiZhang555 closed this Apr 20, 2018

amshinde removed the review label Apr 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virtcontainers: remove ConfigJSONKey annotation #216

virtcontainers: remove ConfigJSONKey annotation #216

WeiZhang555 commented Apr 16, 2018

WeiZhang555 commented Apr 16, 2018

WeiZhang555 commented Apr 17, 2018

sboeuf commented Apr 17, 2018

bergwolf Apr 17, 2018

WeiZhang555 Apr 17, 2018 •

edited

Loading

bergwolf Apr 17, 2018

WeiZhang555 Apr 17, 2018

bergwolf Apr 17, 2018

bergwolf Apr 18, 2018

WeiZhang555 Apr 18, 2018

bergwolf Apr 18, 2018

WeiZhang555 Apr 18, 2018

bergwolf Apr 18, 2018

bergwolf Apr 18, 2018

WeiZhang555 Apr 18, 2018

bergwolf Apr 18, 2018

codecov bot commented Apr 18, 2018 •

edited

Loading

bergwolf left a comment

bergwolf Apr 18, 2018

sboeuf left a comment

sboeuf commented Apr 19, 2018

WeiZhang555 commented Apr 20, 2018

WeiZhang555 commented Apr 20, 2018

sboeuf commented Apr 20, 2018

WeiZhang555 commented Apr 20, 2018

virtcontainers: remove ConfigJSONKey annotation #216

virtcontainers: remove ConfigJSONKey annotation #216

Conversation

WeiZhang555 commented Apr 16, 2018

WeiZhang555 commented Apr 16, 2018

WeiZhang555 commented Apr 17, 2018

sboeuf commented Apr 17, 2018

Choose a reason for hiding this comment

WeiZhang555 Apr 17, 2018 • edited Loading

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

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

codecov bot commented Apr 18, 2018 • edited Loading

Codecov Report

bergwolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 19, 2018

WeiZhang555 commented Apr 20, 2018

WeiZhang555 commented Apr 20, 2018

sboeuf commented Apr 20, 2018

WeiZhang555 commented Apr 20, 2018

WeiZhang555 Apr 17, 2018 •

edited

Loading

codecov bot commented Apr 18, 2018 •

edited

Loading