Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

agent: Add support for local storage#521

Merged
chavafg merged 1 commit intokata-containers:masterfrom
awprice:local-storage-type
Apr 12, 2019
Merged

agent: Add support for local storage#521
chavafg merged 1 commit intokata-containers:masterfrom
awprice:local-storage-type

Conversation

@awprice
Copy link
Copy Markdown
Contributor

@awprice awprice commented Apr 5, 2019

Local storage is just a directory created inside the VM.
This will use whatever the storage driver is for the container
rootfs. This will normally be 9p, but in some cases could be
device mapper. In this case the directory will benefit from the improved
performance of device mapper.

Signed-off-by: Alex Price aprice@atlassian.com

@devimc
Copy link
Copy Markdown

devimc commented Apr 5, 2019

@awprice thanks for raising this, please file an issue and include it in you commit message (Fixes #XXXX)

@awprice awprice force-pushed the local-storage-type branch from 2b8b1c7 to 5c65b04 Compare April 8, 2019 00:34
@awprice
Copy link
Copy Markdown
Contributor Author

awprice commented Apr 8, 2019

@awprice thanks for raising this, please file an issue and include it in you commit message (Fixes #XXXX)

Thanks, done.

@devimc
Copy link
Copy Markdown

devimc commented Apr 8, 2019

/test

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 9, 2019

Retriggering failing CI's..

@amshinde amshinde requested a review from egernst April 9, 2019 01:12
mount.go Outdated
defer s.Unlock()
newStorage := s.setSandboxStorage(storage.MountPoint)
if newStorage {
return "", os.MkdirAll(storage.MountPoint, os.ModePerm)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it doesn't call commonStorageHandler please make sure readonly mount is handled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bergwolf I've just added support for parsing and extracting the mode from storage.Options and using that when creating the directory. I've also added new tests for this functionality.

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 9, 2019

@chavafg @ganeshmaharaj
I see both the initrd CIs failing with this error if I am not mistaken:

grpc_test.go:872:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runc/libcontainer/configs.Rlimit` composite literal uses unkeyed fields (govet)
		{unix.RLIMIT_CPU, 100, 120},
		^
grpc_test.go:873:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runc/libcontainer/configs.Rlimit` composite literal uses unkeyed fields (govet)
		{unix.RLIMIT_FSIZE, 100, 120},
		^
grpc_test.go:874:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runc/libcontainer/configs.Rlimit` composite literal uses unkeyed fields (govet)
		{unix.RLIMIT_DATA, 100, 120},
		^
grpc_test.go:891:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runtime-spec/specs-go.POSIXRlimit` composite literal uses unkeyed fields (govet)
		{"RLIMIT_CPU", 100, 120},
		^
grpc_test.go:892:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runtime-spec/specs-go.POSIXRlimit` composite literal uses unkeyed fields (govet)
		{"RLIMIT_FSIZE", 100, 120},
		^
grpc_test.go:893:3: composites: `github.com/kata-containers/agent/vendor/github.com/opencontainers/runtime-spec/specs-go.POSIXRlimit` composite literal uses unkeyed fields (govet)
		{"RLIMIT_DATA", 100, 120},

Why is the linter failing on the vendored packages, we should not be checking those.
And why are just the initrd CIs failing?

@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Apr 9, 2019

I am not sure why they failed, but yes it is weird that it only failed on 2 jobs. any idea @ganeshmaharaj?

@ganeshmaharaj
Copy link
Copy Markdown
Contributor

ganeshmaharaj commented Apr 9, 2019

@amshinde @chavafg it seems that we changed our vendor code recently. 39696c0. Not sure if we verified that all linter issues were resolved before landing it or if linter happens after updating the vendoring. this is just saying that our code has a an unkeyed field in our code.

-- Update
Sorry, seems like i read it all wrong. It was an issue which got solved with d815c97

@awprice awprice force-pushed the local-storage-type branch 2 times, most recently from d0d8254 to 7e8dd3f Compare April 10, 2019 06:40
Local storage is just a directory created inside the VM.
This will use whatever the storage driver is for the container
rootfs. This will normally be 9p, but in some cases could be
device mapper. In this case the directory will benefit from the improved
performance of device mapper.

Fixes kata-containers#524

Signed-off-by: Alex Price <aprice@atlassian.com>
@awprice awprice force-pushed the local-storage-type branch from 7e8dd3f to 8847998 Compare April 10, 2019 06:53
// Default to os.ModePerm.
opts := parseOptions(storage.Options)
mode := os.ModePerm
if val, ok := opts["mode"]; ok {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we pass dir mode here. Please add the corresponding part in kata-containers/runtime#1485

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the recommendation, I've updated the runtime PR with the mode.

@bergwolf
Copy link
Copy Markdown
Member

/test

@amshinde
Copy link
Copy Markdown
Member

/test

@awprice
Copy link
Copy Markdown
Contributor Author

awprice commented Apr 11, 2019

@ganeshmaharaj @chavafg I've noticed some of the jenkins tests are still failing, are they related to the changes in my PR? Is there anything I can do to get them to pass?

@ganeshmaharaj
Copy link
Copy Markdown
Contributor

@awprice a quick glance at the test failure shows that power8 failed cause the clean-up of the jenkins workspace failed which inturn fails a clone etc. nemu seems to be hitting a certain pattern of failures between mine and other PRs too. @chavafg do you have any idea about that?
the centos build should be removed from the CI as it seems it has been replaced by centos-7-4-q-25. @GabyCT

@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Apr 12, 2019

hi,
Power8 CI is not ready jet.
as @ganeshmaharaj figured out, centos job was renamed and nemu job has been unstable... we merged today a PR from @sboeuf which resolves some of the stability issues, but you would need to rebase. I think we can safely merge.

@chavafg chavafg merged commit 34209ea into kata-containers:master Apr 12, 2019
@awprice awprice deleted the local-storage-type branch April 12, 2019 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants