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

virtcontainers: Add support for ephemeral volumes #458

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

harche
Copy link
Contributor

@harche harche commented Jul 3, 2018

Ephemeral volumes should not be passed at 9pfs mounts.
They should be created inside the VM.

This patch disables ephemeral volumes from getting
mounted as 9pfs from the host and instead a corresponding
tmpfs is created inside the VM.

Fixes : #61

Signed-off-by: Harshal Patil harshal.patil@in.ibm.com

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 170751 KB
Proxy: 4692 KB
Shim: 8828 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007096 KB

@harche
Copy link
Contributor Author

harche commented Jul 3, 2018

This PR implements the discussion #443 to avoid using devices at sandbox level.

@WeiZhang555
Copy link
Member

The design looks good to me, but need changes from kata-agent before we can merge this.

@bergwolf
Copy link
Member

bergwolf commented Jul 3, 2018

@harche It seems that the PR works w/o agent change, right? The missing piece in kata agent is just the ref counting part but tmpfs handling is already there.

@harche
Copy link
Contributor Author

harche commented Jul 4, 2018

@bergwolf @WeiZhang555 This PR works w/o agent change.

@harche
Copy link
Contributor Author

harche commented Jul 4, 2018

@WeiZhang555 Since this PR works without any changes in kata-agent, I think once I fix the CI issues around cyclomatic complexities it can be merged.

Also, I think I can close #307, as we aren't going to pursue that path anymore. What do you guys think? @WeiZhang555 @bergwolf

Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

Thanks for reworking this @harche.
This looks good to me, I have only one comment but this could be addressed in a separate PR as part of some refactoring.

cli/create.go Outdated
// of the same pod the already existing volume is reused.
for idx, mnt := range ociSpec.Mounts {
if IsEphemeralStorage(mnt.Source) {
ociSpec.Mounts[idx].Type = "ephemeral"
Copy link

Choose a reason for hiding this comment

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

I think we need to define the type ephemeral (as other types) inside a separate package that can be imported both from the agent and the runtime. This can be done in a follow up PR, but this would be better than simply hardcode the type at each end of the chain (runtime on one side, and the agent on the other side.)

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162611 KB
Proxy: 5103 KB
Shim: 8848 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007384 KB

@codecov
Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #458 into master will increase coverage by 0.08%.
The diff coverage is 39.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   64.16%   64.25%   +0.08%     
==========================================
  Files          87       87              
  Lines        8942     8969      +27     
==========================================
+ Hits         5738     5763      +25     
- Misses       2584     2586       +2     
  Partials      620      620
Impacted Files Coverage Δ
virtcontainers/kata_agent.go 31.86% <0%> (+1.09%) ⬆️
cli/utils.go 100% <100%> (ø) ⬆️
cli/create.go 80.45% <50%> (+0.69%) ⬆️

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 545fe0c...b821a5d. Read the comment docs.

@WeiZhang555
Copy link
Member

@harche It seems that the PR works w/o agent change, right? The missing piece in kata agent is just the ref counting part but tmpfs handling is already there.

I believe it can work w/o agent change, as @bergwolf described, we need a reference counter in kata-agent to mark when the "ephemeral" volume can be unmounted, without this, the exit of any container could lead to umount of the tmpfs which will break the "ephemeral volume", this could happen when one container in POD failed to start and has to be restarted several times.

@bergwolf
Copy link
Member

@harche Could you please fix UT coverage and remove the wip tag? Then the PR is ready to get merged.

@harche harche force-pushed the without_devices branch from 9e4b479 to 76c8d07 Compare July 17, 2018 06:23
@harche harche changed the title [WIP] [Do Not Merge] virtcontainers: Add support for ephemeral volumes virtcontainers: Add support for ephemeral volumes Jul 17, 2018
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 160254 KB
Proxy: 5072 KB
Shim: 8781 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007236 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 17, 2018

Build succeeded (third-party-check pipeline).

@harche harche force-pushed the without_devices branch from 76c8d07 to eb51da8 Compare July 17, 2018 09:18
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162602 KB
Proxy: 4910 KB
Shim: 8850 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007212 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 17, 2018

Build succeeded (third-party-check pipeline).

@harche harche force-pushed the without_devices branch from eb51da8 to eb148ac Compare July 17, 2018 10:17
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 160507 KB
Proxy: 5022 KB
Shim: 8907 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007400 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 17, 2018

Build succeeded (third-party-check pipeline).

Ephemeral volumes should not be passed at 9pfs mounts.
They should be created inside the VM.

This patch disables ephemeral volumes from getting
mounted as 9pfs from the host and instead a corresponding
tmpfs is created inside the VM.

Fixes : kata-containers#61

Signed-off-by: Harshal Patil <harshal.patil@in.ibm.com>
@harche harche force-pushed the without_devices branch from eb148ac to b821a5d Compare July 18, 2018 02:11
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 160822 KB
Proxy: 4831 KB
Shim: 8809 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007212 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 18, 2018

Build succeeded (third-party-check pipeline).

@bergwolf
Copy link
Member

The codecov failure codecov/patch — 39.13% of diff hit (target 64.16%) looks bogus since clearly the patch is already properly tested with UT.

I think we can safely merge this but I want to understand why codecov/patch fails. Looking at https://codecov.io/gh/kata-containers/runtime/commit/b821a5df4c1adb154613b80f981ba16684ed8b80, the change in kata_agent.go is clearly tested by UT but diff coverage reports 0%.

In that case, I would suggest we either remove codecov/patch, or always ignore it during reviewing.

@egernst @sboeuf @jodh-intel @grahamwhaley @WeiZhang555 @gnawux WDYT?

@bergwolf
Copy link
Member

Moving codecov/patch discussion to kata-containers/tests/issues/508 so this PR can be merged.

@bergwolf bergwolf merged commit 81c073f into kata-containers:master Jul 18, 2018
@harche harche deleted the without_devices branch July 18, 2018 07:23
@egernst egernst mentioned this pull request Aug 22, 2018
@sboeuf sboeuf added the feature New functionality label Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants