Support -t (terminal) option. #17
Conversation
This commit does the following: - Moves the code to run the `runtime` process from `run.rs` to `process.rs` into the `RuntimeProcess` struct. - Moves the common code shared between `create` and `exec` commands to `session.rs` into the `RuntimeSession` struct. - The `RuntimeSession` uses the `RuntimeProcess` in high-level way. - The basic `exec.rs` is implemented using `RuntimeSession`. - The exit code is now passed from the commands to the `main` and returned there. Signed-off-by: Jan Kaluza <[email protected]>
This commit does the following: - Handles the `--exec-attach` option. - Handles the _OCI_ATTACHPIPE and _OCI_STARTPIPE. It is tested by the conmon-v2 tests run as part of `make test`. Signed-off-by: Jan Kaluza <[email protected]>
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. - Disables #![allow(clippy::collapsible_if)] lint globally. The Rust version in CI does not support collapsible ifs with `let =` expressions. Signed-off-by: Jan Kaluza <[email protected]>
This commit adds simple logging mechanism based on the `log` and `chrono` crates to write conmon debugging logs in the bundle directory. The directory can also be changed using the env variables. Signed-off-by: Jan Kaluza <[email protected]>
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. Signed-off-by: Jan Kaluza <[email protected]>
This commit adds simple logging mechanism based on the `log` and `chrono` crates to write conmon debugging logs in the bundle directory. The directory can also be changed using the env variables. Signed-off-by: Jan Kaluza <[email protected]>
This commit does few small changes related to buffers used in the code: - The `read_pipe` now accepts buffer reference instead of creating new buffer on each call and copying it back to caller. - The `str::from_utf8` is used when appropriate. - The confusing `bytes` variable is renamed to `n`, indicating it is the number of bytes read. - The `if` is used to detect EOF instead of `match` for a better readibility. Signed-off-by: Jan Kaluza <[email protected]>
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. Signed-off-by: Jan Kaluza <[email protected]>
This commit does few small changes related to buffers used in the code: - The `read_pipe` now accepts buffer reference instead of creating new buffer on each call and copying it back to caller. - The `str::from_utf8` is used when appropriate. - The confusing `bytes` variable is renamed to `n`, indicating it is the number of bytes read. - The `if` is used to detect EOF instead of `match` for a better readibility. Signed-off-by: Jan Kaluza <[email protected]>
This commit handles the --leave-stdin-open flag. When used, the workerfd_stdin is not closed once the remote client disconnects the attach socket. Signed-off-by: Jan Kaluza <[email protected]>
This commit does following: - Allows hyphens in the *-arg options. This is used by podman to pass options like `--exit-command-arg --root`. - Adds more logging related to exit code and runtime exit status message. - Forwards data from container stdout/stderr to attach socket clients. Signed-off-by: Jan Kaluza <[email protected]>
Summary of ChangesHello @jankaluza, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant refactoring to support the -t (terminal) option and improve I/O handling within conmon. The changes involve splitting runtime execution logic into RuntimeProcess and RuntimeSession for better modularity, and centralizing Unix domain socket management in a new unix_socket module. The handle_stdio function has been completely rewritten to use a more robust event-loop-based approach, capable of managing various socket types for stdin, stdout, stderr, attach, and terminal I/O. Additionally, a new file logging mechanism has been integrated, and dependency updates have been made to support these new features. The refactoring generally improves the code's structure and maintainability, making it more flexible for future enhancements.
| disable_version_flag = true | ||
| )] | ||
| #[derive(Default)] | ||
| #[derive(Default, Debug)] |
There was a problem hiding this comment.
Adding Debug derive to the Opts struct is a good practice, especially for command-line argument parsing. It significantly aids in debugging by allowing easy printing of the parsed options.
|
|
||
| /// Additional arg to pass to the exit command. Can be specified multiple times | ||
| #[arg(long = "exit-command-arg")] | ||
| #[arg(long = "exit-command-arg", allow_hyphen_values = true)] |
There was a problem hiding this comment.
Adding allow_hyphen_values = true to exit-command-arg, restore-arg, and runtime-arg is a necessary and correct change. This ensures that arguments containing hyphens are parsed correctly by clap, which is common in many command-line interfaces.
| pub conmon_pidfile: Option<PathBuf>, | ||
| pub container_pidfile: PathBuf, | ||
| pub bundle: PathBuf, | ||
| pub full_attach: bool, | ||
| pub socket_dir_path: Option<PathBuf>, | ||
| pub stdin: bool, | ||
| pub leave_stdin_open: bool, | ||
| pub terminal: bool, |
There was a problem hiding this comment.
Moving conmon_pidfile, container_pidfile, bundle, full_attach, socket_dir_path, stdin, leave_stdin_open, and terminal into CommonCfg is an excellent refactoring. This centralizes common configuration options, reducing duplication and making the CreateCfg, ExecCfg, and RestoreCfg structs more focused on their specific concerns. This improves maintainability and clarity.
| pub fn exec(&self, log_plugin: &mut dyn LogPlugin) -> ConmonResult<ExitCode> { | ||
| // Start the `runtime create` session. | ||
| let mut runtime_session = crate::runtime::session::RuntimeSession::new(); | ||
| runtime_session.launch(&self.cfg.common, self, false)?; | ||
|
|
||
| // === | ||
| // Now, after the `run_runtime`, we are in the child process of our original process | ||
| // (See `run_runtime` code and description for more information). | ||
| // Now, after the `launch()`, we are in the child process of our original process, | ||
| // because we double-fork in the RuntimeProcess::spawn. | ||
| // (See `RuntimeProcess::spawn` code and description for more information). | ||
| // === | ||
|
|
||
| // Wait until the `runtime create` finishes. | ||
| let runtime_status = wait_for_runtime(runtime_pid)?; | ||
| if runtime_status != 0 { | ||
| // Pass the error to sync_pipe if there is one. | ||
| if let Some(fd) = sync_pipe_fd { | ||
| let err_bytes = read_pipe(&mainfd_stderr)?; | ||
| let err_str = String::from_utf8(err_bytes)?; | ||
| write_or_close_sync_fd(fd, -1, Some(&err_str), self.cfg.common.api_version, false)?; | ||
| } | ||
| return Err(ConmonError::new( | ||
| format!("Runtime exited with status: {runtime_status}"), | ||
| 1, | ||
| )); | ||
| // In case of `--terminal`, wait until runtime creates the console socket. | ||
| if self.cfg.common.terminal { | ||
| runtime_session.wait_for_terminal_creation()?; | ||
| } | ||
|
|
||
| // Pass the container_pid to sync_pipe if there is one. | ||
| if let Some(fd) = sync_pipe_fd { | ||
| let container_pid = self.read_container_pid()?; | ||
| write_or_close_sync_fd(fd, container_pid, None, self.cfg.common.api_version, false)?; | ||
| } | ||
| // Wait until the `runtime create` finishes and return an error in case it fails. | ||
| runtime_session.wait_for_success(self.cfg.common.api_version)?; | ||
|
|
||
| runtime_session.write_container_pid_file(&self.cfg.common)?; | ||
|
|
||
| // === | ||
| // Now we wait for an external application like podman to really start the container. | ||
| // and handle the containers stdio or its termination. | ||
| // === | ||
|
|
||
| // Handle the stdio. | ||
| handle_stdio(log_plugin, mainfd_stdout, mainfd_stderr)?; | ||
| // Run the eventloop to forward log messages to log plugin. | ||
| runtime_session.run_event_loop(log_plugin, self.cfg.common.leave_stdin_open)?; | ||
|
|
||
| Ok(()) | ||
| Ok(ExitCode::SUCCESS) | ||
| } | ||
| } |
There was a problem hiding this comment.
The refactoring of the exec function to utilize the new RuntimeSession is a significant improvement. It encapsulates the complex logic of process spawning, pipe/socket management, and synchronization into a dedicated session object, making the Create command's exec method much cleaner and easier to understand. This greatly enhances modularity and maintainability.
| pub fn exec(&self, log_plugin: &mut dyn LogPlugin) -> ConmonResult<ExitCode> { | ||
| let mut runtime_session = crate::runtime::session::RuntimeSession::new(); | ||
| runtime_session.launch(&self.cfg.common, self, self.cfg.attach)?; | ||
|
|
||
| Ok(()) | ||
| // === | ||
| // Now, after the `launch`, we are in the child process of our original process | ||
| // (See `RuntimeProcess::spawn` code and description for more information). | ||
| // === | ||
|
|
||
| // Run the eventloop to forward log messages to log plugin. | ||
| runtime_session.run_event_loop(log_plugin, self.cfg.common.leave_stdin_open)?; | ||
|
|
||
| // Wait for the `runtime exec` to finish and write its exit code. | ||
| runtime_session.wait()?; | ||
| runtime_session.write_container_pid_file(&self.cfg.common)?; | ||
| runtime_session.write_exit_code(self.cfg.common.api_version)?; | ||
|
|
||
| Ok(ExitCode::from(runtime_session.exit_code() as u8)) | ||
| } |
There was a problem hiding this comment.
Similar to create.rs, refactoring the exec function to use RuntimeSession is a positive change. It streamlines the execution flow and delegates complex I/O and process management to the RuntimeSession, leading to a more organized and maintainable codebase.
| // Go to next socket in case we want to keep this one. | ||
| i += 1; | ||
| } else { | ||
| // Remove the socket. | ||
| let socket = sockets.swap_remove(i); | ||
| fds.swap_remove(i); |
There was a problem hiding this comment.
The logic for leave_stdin_open is correctly implemented. When leave_stdin_open is false, workerfd_stdin.take() ensures that the stdin pipe is closed after the console client disconnects, preventing the container from hanging while waiting for input. This is an important detail for correct terminal behavior.
| }, | ||
| stat::{Mode, fchmod}, | ||
| }, | ||
| unistd::{mkstemp, symlinkat, unlink}, | ||
| }; | ||
|
|
||
| use ::log::info; | ||
|
|
||
| // Type of the UnixSocket and RemoteSocket. | ||
| #[derive(Debug, Copy, Clone, Eq, PartialEq, Default)] |
There was a problem hiding this comment.
The SocketType enum clearly defines the different purposes of Unix sockets within conmon. This explicit typing improves code readability and helps in understanding the role of each socket in the system.
|
|
||
| pub fn path(&self) -> Option<&PathBuf> { | ||
| self.path.as_ref() | ||
| } | ||
|
|
||
| /// Generates the socket path and starts listening for new client (remote) connections. | ||
| pub fn listen( | ||
| &mut self, | ||
| path: Option<PathBuf>, | ||
| sock_type: SockType, | ||
| sock_flags: SockFlag, | ||
| perms: Mode, | ||
| ) -> ConmonResult<()> { | ||
| let mut full_path: PathBuf; | ||
| let mut dir_fd: Option<OwnedFd> = None; | ||
|
|
||
| if let Some(path) = path { | ||
| // We have some path, but we need a full-path. | ||
| // If the path is a full-path, use it. | ||
| // If it's not, generate the full-path using socket_parent_dir() and | ||
| // prefix the path with it. | ||
| full_path = path.to_owned(); | ||
| let fallback; | ||
| let dir = if let Some(parent) = path.parent() { | ||
| if !parent.is_empty() { | ||
| parent | ||
| } else { | ||
| fallback = self.socket_parent_dir()?; | ||
| let fallback_path = fallback.as_path(); | ||
| full_path = fallback_path.join(path); | ||
| fallback_path | ||
| } | ||
| } else { | ||
| fallback = self.socket_parent_dir()?; | ||
| let fallback_path = fallback.as_path(); | ||
| full_path = fallback_path.join(path); | ||
| fallback_path | ||
| }; | ||
|
|
||
| // Create the parent-directory of full-path. | ||
| let flags = OFlag::O_CREAT | OFlag::O_CLOEXEC | OFlag::O_PATH; | ||
| let dfd = open(dir, flags, Mode::from_bits_truncate(0o600))?; | ||
|
|
||
| // Store the dir_fd, because we will be creating the socket in this dir. | ||
| dir_fd = Some(dfd); | ||
| } else { | ||
| // We do not have a path, so create temporary one. | ||
| let tmpdir = std::env::temp_dir(); | ||
| full_path = tmpdir.join("conmon-term.XXXXXX"); | ||
| let (fd_tmp, x) = mkstemp(&full_path)?; | ||
| full_path = x; | ||
| drop(fd_tmp); | ||
| } | ||
|
|
||
| // Remove old socket if present. | ||
| unlink(&full_path).or_else(|e| { | ||
| if e == nix::Error::ENOENT { | ||
| Ok(()) | ||
| } else { | ||
| Err(ConmonError::new( | ||
| format!("Failed to remove old socket {full_path:?}: {e}"), | ||
| 1, | ||
| )) | ||
| } | ||
| })?; | ||
|
|
||
| // Now bind & listen on the console socket path. | ||
| let fd = socket(AddressFamily::Unix, sock_type, sock_flags, None)?; | ||
|
|
||
| self.bind_relative_to_dir(&fd, dir_fd.as_ref(), &full_path, perms)?; | ||
| listen(&fd, Backlog::MAXCONN)?; | ||
| info!("Listening on {full_path:?}"); |
There was a problem hiding this comment.
The UnixSocket::listen function provides robust logic for creating and binding Unix domain sockets. It handles various scenarios, including generating temporary paths, removing old sockets, and setting appropriate permissions. The use of dir_fd for relative path binding is a good detail for flexible socket placement.
| let addr: nix::sys::socket::sockaddr_un = unsafe { std::mem::zeroed() }; | ||
| addr.sun_path.len() | ||
| } | ||
|
|
||
| /// Generates the socket parent directory based on the UnixSocket options. | ||
| fn socket_parent_dir(&mut self) -> ConmonResult<PathBuf> { | ||
| let base_path = if self.use_full_attach_path { | ||
| self.bundle_path.to_owned() | ||
| } else if let Some(cuuid) = &self.cuuid { | ||
| if let Some(socket_path) = &self.socket_path { | ||
| socket_path.join(cuuid) | ||
| } else { | ||
| "".into() | ||
| } | ||
| } else { | ||
| "".into() | ||
| }; | ||
|
|
||
| if base_path.is_empty() { | ||
| return Err(ConmonError::new( | ||
| "Base path for socket cannot be determined", | ||
| 1, | ||
| )); | ||
| } | ||
|
|
||
| if self.use_full_attach_path { | ||
| // nothing else to do | ||
| return Ok(base_path); | ||
| } | ||
|
|
||
| let desired_len = self.max_socket_path_len(); | ||
| let mut base_path_bytes = base_path.as_os_str().as_bytes().to_vec(); | ||
| if base_path_bytes.len() >= desired_len - 1 { | ||
| // chop last char | ||
| if let Some(last) = base_path_bytes.last_mut() { | ||
| *last = b'\0'; | ||
| } | ||
| } | ||
| let new_base = PathBuf::from(OsStr::from_bytes( | ||
| base_path_bytes | ||
| .iter() | ||
| .take_while(|b| **b != 0) | ||
| .copied() | ||
| .collect::<Vec<_>>() | ||
| .as_slice(), | ||
| )); | ||
|
|
||
| // Remove old symlink if present | ||
| unlink(&new_base).or_else(|e| { | ||
| if e == nix::Error::ENOENT { | ||
| Ok(()) | ||
| } else { | ||
| Err(e) | ||
| } | ||
| })?; | ||
|
|
There was a problem hiding this comment.
The socket_parent_dir function correctly handles the complexities of generating socket paths, including considerations for use_full_attach_path, cuuid, and truncating paths to fit sockaddr_un length limits. This attention to detail prevents common issues with Unix domain socket paths.
| Unix(UnixSocket), | ||
| Remote(RemoteSocket), | ||
| } | ||
|
|
||
| impl Socket { | ||
| pub fn handle_data( | ||
| &mut self, | ||
| log_plugin: &mut dyn LogPlugin, | ||
| new_sockets: &mut Vec<RemoteSocket>, | ||
| workerfd_stdin: Option<&OwnedFd>, | ||
| console_fds: &Vec<i32>, | ||
| terminal_fds: &Vec<i32>, | ||
| ) -> ConmonResult<bool> { | ||
| match self { | ||
| Socket::Unix(l) => { | ||
| if let Some(remote) = l.accept()? { | ||
| new_sockets.push(remote); | ||
| } | ||
| return Ok(true); | ||
| } | ||
| Socket::Remote(r) => { | ||
| let bytes_read = r.read()?; | ||
| if bytes_read == 0 { | ||
| return Ok(false); | ||
| } | ||
| match r.socket_type { | ||
| SocketType::Stdout | SocketType::Stderr | SocketType::Terminal => { | ||
| // Forward data to logs. | ||
| let is_stderr = r.socket_type == SocketType::Stderr; | ||
| let _ = log_plugin.write(!is_stderr, &r.buf[..bytes_read]); | ||
|
|
||
| // Forward data to remote sockets attached to `attach` socket. | ||
| // The data is prefixed with single byte indicating whether | ||
| // it is stdout or stderr. | ||
| let prefix_buf: &[u8]; | ||
| if is_stderr { | ||
| prefix_buf = &[3]; // stdout | ||
| } else { | ||
| prefix_buf = &[2]; // stderr | ||
| } | ||
| for &fd in console_fds { | ||
| let borrowed = unsafe { std::os::fd::BorrowedFd::borrow_raw(fd) }; | ||
| let iov = [ | ||
| std::io::IoSlice::new(prefix_buf), | ||
| std::io::IoSlice::new(&r.buf[..bytes_read]), | ||
| ]; | ||
| writev(borrowed, &iov)?; | ||
| } | ||
| } | ||
| SocketType::Console => { | ||
| // Console socket: forward data to container's stdin. | ||
| if let Some(workerfd_stdin) = workerfd_stdin.as_ref() { | ||
| write(workerfd_stdin, &r.buf[..bytes_read])?; | ||
| } | ||
| // Forward data to terminal. | ||
| for &fd in terminal_fds { | ||
| debug!("Forwarding to terminal {}", fd); | ||
| let borrowed = unsafe { std::os::fd::BorrowedFd::borrow_raw(fd) }; | ||
| write(borrowed, &r.buf[..bytes_read])?; | ||
| } | ||
| } | ||
| SocketType::Notify => {} | ||
| SocketType::Attach => {} |
There was a problem hiding this comment.
The Socket::handle_data method is a well-designed abstraction for processing incoming data from different socket types. It effectively dispatches to specific logic based on the SocketType, ensuring that data is correctly logged, forwarded to attach clients (with appropriate prefixes), or sent to the container's stdin/terminal. This modular approach enhances the clarity and maintainability of the I/O event loop.
This commit implements support for `podman -it`. It does following: - Handles the `-t` option. - Adds new `RuntimeSession.console_socket` which is passed to runtime so it can send the newly created terminal which is used to communicate with the container. - Adds new `RuntimeSession.terminal_socket` to represent the received terminal fd. - The `handle_stdio` is rewritten. Everything is `Socket` now. - The `handle_stdio` now only handles the poll logic. The data handling logic happens in the `Socket.handle_data`. Signed-off-by: Jan Kaluza <[email protected]>
1 participant
This commit implements support for
podman -it. It does following:-toption.RuntimeSession.console_socketwhich is passed to runtimeso it can send the newly created terminal which is used to communicate
with the container.
RuntimeSession.terminal_socketto represent the receivedterminal fd.
handle_stdiois rewritten. Everything isSocketnow.handle_stdionow only handles the poll logic. The data handlinglogic happens in the
Socket.handle_data.Signed-off-by: Jan Kaluza [email protected]