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

agent: Add support for local storage #521

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

awprice
Copy link
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

devimc commented Apr 5, 2019

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

@awprice
Copy link
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

devimc commented Apr 8, 2019

/test

@amshinde
Copy link
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
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
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
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
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
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>
// Default to os.ModePerm.
opts := parseOptions(storage.Options)
mode := os.ModePerm
if val, ok := opts["mode"]; ok {
Copy link
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
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
Member

/test

@amshinde
Copy link
Member

/test

@awprice
Copy link
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
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
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