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

[feature] add support of a runc sandboxer #94

Merged
merged 9 commits into from
Dec 10, 2023

Conversation

abel-von
Copy link
Contributor

A remote runc sandboxer which will run a process in seperate namespaces and this process will serve the Task API by a unix domain socket, and this process will also be the process to carry the common namespaces of containers in the same pod. so that we don't have a pause process, the "shim" process will act as the "pause" process

@abel-von abel-von requested a review from a team as a code owner September 14, 2023 09:32
@abel-von abel-von force-pushed the support-runc-for-shim branch from f04a84c to b9c97a8 Compare September 14, 2023 09:34
@xiaods xiaods mentioned this pull request Sep 17, 2023
@xiaods
Copy link

xiaods commented Sep 18, 2023

any update on it?

@abel-von
Copy link
Contributor Author

Maybe we will have a community meeting next week to discuss the design. @xiaods

@xiaods
Copy link

xiaods commented Sep 23, 2023

Maybe we will have a community meeting next week to discuss the design. @xiaods

any update on it?

@xiaods
Copy link

xiaods commented Sep 29, 2023

Maybe we will have a community meeting next week to discuss the design. @xiaods

any update on this community meeting?

@Burning1020
Copy link
Member

Maybe we will have a community meeting next week to discuss the design. @xiaods

any update on this community meeting?

Sorry, Abel has been working for the Kubecon China, should we have it after this vocation? @abel-von Do you have time?

@xiaods
Copy link

xiaods commented Sep 30, 2023

Maybe we will have a community meeting next week to discuss the design. @xiaods

any update on this community meeting?

Sorry, Abel has been working for the Kubecon China, should we have it after this vocation? @abel-von Do you have time?

I just want to learn about the latest project progress and it doesn't matter about the time. I also need more time to understand this project, so there is no need to worry about time pressure. Thank you.

@abel-von
Copy link
Contributor Author

We will follow this design to make a runc sandboxer
image

@xiaods
Copy link

xiaods commented Nov 5, 2023

@abel-von if the runc sandboxer feature is implemented, can I replace the containerd with the kuasar component?

@xiaods
Copy link

xiaods commented Nov 17, 2023

any update?

{
struct task_struct *task = (typeof(task)) bpf_get_current_task();
struct process_exit_data_t data = {};
//data.start_time = PROCESS_START_TIME_NS,
Copy link

Choose a reason for hiding this comment

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

What does this line of code mean? If it's not necessary, it can be removed.

@Burning1020
Copy link
Member

any update?

The code is basically complete but still needs some time.

Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

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

File Header and CI are needed, the other comment could be resolved in follow up.

@@ -0,0 +1,36 @@
#include<linux/sched.h>
Copy link
Member

Choose a reason for hiding this comment

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

Need CopyRight file header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new version removed the eBPF file

@@ -0,0 +1,64 @@
use bcc::{BPF, Tracepoint};
Copy link
Member

Choose a reason for hiding this comment

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

I think we also shoule update CI workflow to test runc package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new version removed the eBPF file

runc/src/main.rs Outdated
mod exitsnoop;

pub const TASK_ADDRESS_SOCK: &str = "/run/kuasar/task.sock";
const DEFAULT_CONTAINERD_STATE_DIR: &str = "/run/containerd/";
Copy link
Member

Choose a reason for hiding this comment

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

Should BOUDLE_DIR be better? Runc sandboxer does not care about what exactly the container engine is, it only care about the bundle dir of container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

codes removed of the new version

}
let log_pipe = format!("{}/log", self.base_dir);
unsafe {
mkfifo(&*log_pipe, Mode::from_bits_unchecked(0700))
Copy link
Member

Choose a reason for hiding this comment

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

nix::unistd::mkfifo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

codes removed

}
let child = cmd
.spawn()
.map_err(|e| anyhow!("failed to spawn shim {}", e))?;
Copy link
Member

Choose a reason for hiding this comment

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

spawn runc sandbox parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

codes removed


pub async fn copy_io(pio: &ProcessIO, stdio: &Stdio, exit_signal: Arc<ExitSignal>) -> Result<()> {
if !pio.copy {
return Ok(());
Copy link
Member

Choose a reason for hiding this comment

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

Should open the read end of fifo.

@Burning1020
Copy link
Member

We also have to provide a make command for building.

@abel-von abel-von force-pushed the support-runc-for-shim branch from b9c97a8 to 353536e Compare December 6, 2023 16:45
@flyflypeng
Copy link
Member

We will follow this design to make a runc sandboxer image

I think it's necessary to update this architecture diagram corresponding to the runc sandboxer in the README.md and provide additional details about the design."

@abel-von abel-von requested review from a team as code owners December 7, 2023 02:47
@flyflypeng flyflypeng closed this Dec 7, 2023
@flyflypeng flyflypeng reopened this Dec 7, 2023
@abel-von abel-von force-pushed the support-runc-for-shim branch from 6dbd861 to 121951d Compare December 7, 2023 04:06
loop {
let buffer = read_count(reqr, 512).unwrap();
let id = String::from_utf8_lossy(&buffer[0..64]).to_string();
let mut zero_index = 64;
Copy link
Member

Choose a reason for hiding this comment

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

what's the meaning of the 64, a static const maybe suitable

runc/src/main.rs Show resolved Hide resolved
runc/src/main.rs Outdated
close(reqw).unwrap_or_default();
close(respr).unwrap_or_default();
prctl::set_child_subreaper(true).unwrap();
let comm = format!("[sandbox-parent]");
Copy link
Member

Choose a reason for hiding this comment

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

I find the naming convention for sandbox-parent somewhat ambiguous, conflicting with the naming convention of kernel threads in the Linux system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to to highlight the process as a special process, do you have any good suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's sufficient to distinguish using the unique process name directly, there's no need to add square brackets.

exit(0);
}
ForkResult::Child => {
let comm = format!("[sandbox-{}]", id);
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, naming convention is confused with kernel threads.

@abel-von abel-von force-pushed the support-runc-for-shim branch 2 times, most recently from 4e5874e to 228d3a9 Compare December 7, 2023 11:39
@abel-von abel-von force-pushed the support-runc-for-shim branch from 228d3a9 to d0bd129 Compare December 7, 2023 11:50
Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

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

lgtm

@Burning1020 Burning1020 merged commit 5b5a02d into kuasar-io:main Dec 10, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants