Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create mount_rootfs method #2953

Merged
merged 1 commit into from
Nov 6, 2024
Merged

create mount_rootfs method #2953

merged 1 commit into from
Nov 6, 2024

Conversation

Gekko0114
Copy link
Contributor

The implementation of mounting differs between Youki and runc.
#2718

Therefore, we want to modify Youki to adopt runc's mounting logic.
As the first step, I've created a new method mount_rootfs.
I will implement the mounting logic step by step in this method.

@Gekko0114
Copy link
Contributor Author

Hi @utam0k, @YJDoc2,

I've submitted a small PR as the first step towards integrating SELinux into Youki's mounting logic.
Could you take a look?
Also I'd like to know which commands should be run to verify that the code changes are safe.

@utam0k utam0k added the kind/experimental `/experimental` label Oct 14, 2024
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 14, 2024

@Gekko0114 , Thanks for implementing this, but may I ask you to wait for/coordinate with #2923 as that also changes the mount impl.

@Gekko0114
Copy link
Contributor Author

Sure, then I will wait this PR will be merged.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 29, 2024

@Gekko0114 the other PR is merged, you can rebase on main and continue your work here.

Signed-off-by: Hiroyuki Moriya <41197469+Gekko0114@users.noreply.github.com>
@Gekko0114
Copy link
Contributor Author

Hi @YJDoc2,
Could you review it since I've rebased this PR ?
I would like to merge this PR first, and after that I will create other PRs using SELinux like runc.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 1, 2024

Hey, it looks ok overall, but can you give a bit more reasoning behind this change? I mean, I understand that this is done to make the logic same in youki and runc, but why is that needed, and what this split of function will achieve (eventually) ?

@Gekko0114
Copy link
Contributor Author

Hi @YJDoc2

In Runc, processing differs for each device of the mount point. https://github.com/opencontainers/runc/blob/d5e4c33001d74176222fe8f48a323f3e8ad89999/libcontainer/rootfs_linux.go#L485
However, Youki doesn't currently handle this like Runc. As a result, Youki fails the OCI tests. #2688

Therefore, I plan to address this issue. First I created a mount_to_rootfs function, and then I will add processing for each device of the mount point.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 6, 2024

ok, Ideally we should try to do it our own way in a single function if possible, but I see that the individual mount fn in runc is quite complex, so maybe we should break our function in two as well. In the following PRs, please check if we can do it differently or more rust-idiomatically. Merging this for now.

@YJDoc2 YJDoc2 merged commit 444cc4f into youki-dev:main Nov 6, 2024
26 checks passed
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/experimental `/experimental`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants