Support for terminal window resize. #18
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]>
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]>
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 introduces significant enhancements to conmon's I/O and process management capabilities. The primary goal is to enable dynamic terminal window resizing for containers, which is achieved through new control FIFOs and a sophisticated event-driven I/O handling system. The changes also include a major refactoring of how runtime processes are managed, centralizing configuration and improving the overall modularity and observability of the application with a new file-based logging system. 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 and well-executed refactoring to support terminal window resizing and improve overall code structure. The introduction of RuntimeSession and the move to an event-driven model with poll are excellent changes that enhance modularity and maintainability. The code is now cleaner and more extensible. I've identified a few areas for improvement: a potential bug in an error handling path, a case where error messages could be truncated, and some swapped comments that could cause confusion. Overall, this is a very positive step forward for the codebase.
src/runtime/session.rs
Outdated
| } else { | ||
| if Errno::last() == Errno::ESRCH { | ||
| // Process exited. | ||
| info!( | ||
| "Container process {} has exited (detected via kill probe)", | ||
| self.container_pid | ||
| ); | ||
| // We cannot get real exit status. | ||
| self.container_status = 0; | ||
| self.container_pid = -1; | ||
| return Ok(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a potential bug in the error handling for the kill probe. If libc::kill fails with an error other than ESRCH (for example, EPERM due to permissions), the logic falls through and terminates the event loop. This could prematurely end conmon's monitoring. It would be more robust to log these other errors and continue the event loop, as the issue might be transient or a misconfiguration that doesn't imply the container has exited.
} else {
let err = Errno::last();
if err == Errno::ESRCH {
// Process exited.
info!(
"Container process {} has exited (detected via kill probe)",
self.container_pid
);
// We cannot get real exit status.
self.container_status = 0;
self.container_pid = -1;
return Ok(false);
}
// Some other error from kill, log it and continue.
warn!(
"kill probe for container process {} failed: {}",
self.container_pid, err
);
return Ok(true);
}| let mut err_bytes = [0u8; 8192]; | ||
| let n = read_pipe(mainfd_stderr, &mut err_bytes)?; | ||
| let err_str = std::str::from_utf8(&err_bytes[..n])?; |
There was a problem hiding this comment.
The TODO comment on the following lines is accurate. This implementation reads from the runtime's stderr at most once, which could truncate the error message if it exceeds 8KB. This might lead to the loss of crucial diagnostic information. To ensure the full error message is captured, I recommend reading from the stderr pipe in a loop until it's fully drained (EOF). Using String::from_utf8_lossy would also make this more robust against non-UTF8 output.
let mut err_vec = Vec::new();
let mut buf = [0u8; 8192];
loop {
let n = read_pipe(mainfd_stderr, &mut buf)?;
if n == 0 {
break;
}
err_vec.extend_from_slice(&buf[..n]);
}
let err_str = String::from_utf8_lossy(&err_vec);| &[3] // stdout | ||
| } else { | ||
| &[2] // stderr |
There was a problem hiding this comment.
The comments for the stdout and stderr prefixes appear to be swapped. This is misleading and could cause confusion for anyone reading this code in the future. The prefix &[3] is used for stderr, and &[2] is used for stdout.
| &[3] // stdout | |
| } else { | |
| &[2] // stderr | |
| &[3] // stderr | |
| } else { | |
| &[2] // stdout |
jankaluza
force-pushed
the
ctrl
branch
2 times, most recently
from
2876844 to
4b94855
Compare
This commit does the following: - Adds the ctl.rs for functions related to `ctl` and `winsz` fifo creation and data handling. - Creates the `ctl_fifo` and `winsz_fifo`. - Handles these new RemoteSockets in `handle_stdio`. - Extends RemoteSocket buffer to be a rolling buffer, so we can handle stream of short new-line terminated messages in `ctl` and `winsz`. - Adds `idle_callback` function so we can detect when runtime or container terminates and quit the event-loop. Signed-off-by: Jan Kaluza <[email protected]>
1 participant
This commit does the following:
ctlandwinszfifocreation and data handling.
ctl_fifoandwinsz_fifo.handle_stdio.stream of short new-line terminated messages in
ctlandwinsz.idle_callbackfunction so we can detect when runtime orcontainer terminates and quit the event-loop.
Signed-off-by: Jan Kaluza [email protected]