runtime-rs: bind mount volumes in sandbox level #5607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
justxuewei
force-pushed
the
feat/sandbox-level-volume
branch
3 times, most recently
from
42f3d20
to
de7e580
Compare
|
Hi @liubin, I found some errors making ci failure in compiling Dragonball. However, this pull request didn't change Dragonball at all. How to fix this? Thanks. |
|
@justxuewei #5640 is fixing the clippy issue. |
justxuewei
force-pushed
the
feat/sandbox-level-volume
branch
from
de7e580
to
4bdcd35
Compare
justxuewei
force-pushed
the
feat/sandbox-level-volume
branch
2 times, most recently
from
98d3ae2
to
f1c1374
Compare
|
/test |
bergwolf
reviewed
Dec 1, 2022
| @@ -38,13 +38,29 @@ pub const PASSTHROUGH_FS_DIR: &str = "passthrough"; | |||
| const RAFS_DIR: &str = "rafs"; | |||
|
|
|||
| #[async_trait] | |||
| pub trait ShareFs: Send + Sync { | |||
| pub trait ShareFs: Send + Sync + Debug { | |||
| async fn share_rootfs(&self, config: ShareFsRootfsConfig) -> Result<ShareFsMountResult>; | ||
| async fn share_volume(&self, config: ShareFsVolumeConfig) -> Result<ShareFsMountResult>; | ||
| /// Upgrade to readwrite permission | ||
| async fn upgrade(&self, file_name: &str) -> Result<()>; |
There was a problem hiding this comment.
Please name it explicitly. Something like upgrade_rw and dowgrade_ro sound better.
| let host_dest = do_get_host_path(file_name, &self.id, "", true, false); | ||
| umount_timeout(&host_dest, 0).context("Umount readonly host dest")?; | ||
| let host_dest = do_get_host_path(file_name, &self.id, "", true, true); | ||
| umount_timeout(&host_dest, 0).context("Umount readwrite host dest")?; |
There was a problem hiding this comment.
The umount event is propagated to the ro virtiofs mount subtree. We only need to umount the rw directory bindmount.
| mounted_info.rw_ref_count += 1; | ||
| } | ||
| share_fs | ||
| .set_mounted_info(&m.source, mounted_info) |
There was a problem hiding this comment.
Do we need any lock when manipulating mounted_info? There are a whole bunch of race windows here.
There was a problem hiding this comment.
Yes, you are right. I am working on it.
| readonly, | ||
| ); | ||
| share_fs | ||
| .set_mounted_info(&m.source, mounted_info) |
There was a problem hiding this comment.
The same locking concern applies here too.
Implemented bind mount related managment on the sandbox side, involving bind mount a volume if it's not mounted before, upgrade permission to readwrite if there is a new container needs. Fixes: kata-containers#5588 Signed-off-by: Xuewei Niu <justxuewei@apache.org>
This commit implemented umonut controls and permission controls. When a volume is no longer referenced, it will be umounted immediately. When a volume mounted with readonly permission and a new coming container needs readwrite permission, the volume should be upgraded to readwrite permission. On the contrary, if a volume with readwrite permission and no container needs readwrite, then the volume should be downgraded. Fixes: kata-containers#5588 Signed-off-by: Xuewei Niu <justxuewei@apache.org>
Removed the `Debug` trait for the `ShareFs` and etc. Renamed `ShareFsMount::upgrade()` and `ShareFsMount::downgrade()` to `upgrade_to_rw()` and `downgrade_to_ro()`. Protected `mounted_info_set` with a mutex to avoid race conditions. Fixes: kata-containers#5588 Signed-off-by: Xuewei Niu <justxuewei@apache.org>
justxuewei
force-pushed
the
feat/sandbox-level-volume
branch
from
f1c1374
to
fdf0a7b
Compare
|
Thanks for your reviews @bergwolf. I've pushed a new commit to fix the issues you mentioned above. Please take a look. |
|
/test |
|
/retest |
lifupan
approved these changes
Dec 7, 2022
dborquez
pushed a commit
to dborquez/kata-containers
that referenced
this pull request
Jun 5, 2023
…ebug test-agent-shutdown: Fix false positive match
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the issue mentioned in #5588. In this pull request all shared volumes, except for rootfs, are bind mounted in sandbox level. That is all volumes referred to the same source on the host are bind mounted only once until no container references to it.
It supports dynamic permission controls as well. For example, when a volume mounted with readonly permission and a new coming container needs readwrite permission, the volume should be upgraded to readwrite permission. At the same time, if readwrite permission is no longer needed, then the volume should be downgraded.
Fixes: #5588
Signed-off-by: Xuewei Niu justxuewei@apache.org