Skip to content

Add container attach/detach support #259

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add container attach/detach support #259

wants to merge 1 commit into from

Conversation

jacques-n
Copy link

@jacques-n jacques-n commented Jun 26, 2025 •
edited by jglogan
Loading

Closes #378.

Add container attach/detach support

This change introduces a new container attach command that allows users to attach
to running containers with terminal support.

Key changes:

  • Move stdio management for interactive run from client to server so that sessions can exist beyond CLI lifetime.
  • Update stdio to use a ring buffer so attach can get recent history.
  • Enable legacy (client driven stdio) and server stdio coexistence.

CLI Changes

  • Added container attach <id> subcommand for attaching to an existing session.
  • Added --legacy-stdio run flag to let newer clients talk to older servers.
  • Added --detach-keys option to run and attach to set detachment keys (defaulted to ctrl-p,ctrl-q)

Testing

  • Added integration tests for attach functionality
  • Added session management tests
  • Added fallback/legacy mode tests

Build changes

  • Added a couple of additional makefile flags to make it easier to target tests: INTEGRATION_TEST_FILTER, INTEGRATION_TEST_SKIP, TEST_FILTER
  • Removed the hand written TestCLI filter list and used a single filter with --no-parallel

@jacques-n jacques-n marked this pull request as draft June 26, 2025 11:54
@jacques-n jacques-n marked this pull request as ready for review June 28, 2025 01:27
@jacques-n
Copy link
Author

jacques-n commented Jul 7, 2025 •
edited
Loading

Seeing some failures here. Moving back to draft until resolved. Will also rebase.

@jacques-n jacques-n marked this pull request as draft July 7, 2025 23:52
@jacques-n
Add container attach/detach support
22aa3ea 
This change introduces a new `container attach` command that allows users to attach
to running containers with terminal support.

- Move stdio management for interactive run from client to server so that sessions can exist beyond CLI lifetime.
- Update stdio to use a ring buffer so attach can get recent history.
- Enable legacy (client driven stdio) and server stdio coexistence.

- Added `container attach <id>` subcommand for attaching to an existing session.
- Added `--legacy-stdio` run flag to let newer clients talk to older servers.
- Added `--detach-keys` option to run and attach to set detachment keys (defaulted to ctrl-p,ctrl-q)

- Added integration tests for attach functionality
- Added session management tests
- Added fallback/legacy mode tests
@jacques-n
Copy link
Author

Simplified some things and rebased. Added more integration tests and confirmed passing integration and unit tests.

@jacques-n jacques-n marked this pull request as ready for review July 10, 2025 04:01
@dcantah dcantah self-assigned this Jul 11, 2025
@dcantah
Copy link
Member

dcantah commented Jul 22, 2025 •
edited
Loading

We haven't forgotten you 😄. Couple questions before I get started. What was the rationale behind needing to support sessions that exist past the lifetime of some attached client? Is that the entire reason behind swapping the direction that IO is established? The other thing I'm not sure of if we'd want to support as well is the ringbuffer of output before attaching. I haven't dug too far into it, but while the asking for the history is optional, it looks like if we ask for a pty we always fill the buffer? To me this feature is somewhat awkward as (and this is likely just me being used to other tools) I'd expect that if I was attaching I would solely be getting output from that point in time. We have logs today that can give you a breakdown/historical/last N lines view of IO already, so I'm hesitant to add in this extra logic.

@jacques-n
Copy link
Author

We haven't forgotten you 😄

😄

What was the rationale behind needing to support sessions that exist past the lifetime of some attached client?

It's pretty common to start a container with a disconnected interactive terminal. I think that is why it is supported in docker. It allows one to separate container creation from exposing that terminal. We use it for starting interactive applications from remote clients without having to guarantee continuous remote connectivity. Think old school client/server. Could one reproduce similar by creating this on top of container by creating a secondary long-lived daemon to hold the sessions? Yes, but it creates a lot of additional complexity and you're basically replicating a bunch of container functionality.

ring buffer.

Yeah, I considered both options. But the primary use case I think user expectation would be playback. Imagine starting an interactive detached container and then attaching to it a few seconds later. Do you expect to see what just happened? I suspect people would. The best analog would be screen. (I admit that It's nowhere near a perfect analog.) If you ran a command in screen and then detach then reattach, you don't lose context.

One alt impl would be to internally invoke the log infra. The concern was the complexity of trying to split the stream correctly--e.g. ensuring exactly once semantics. That's what the ring buffer provides that I think would be invasive to add to the logs.

@dcantah
Copy link
Member

dcantah commented Jul 22, 2025

It's pretty common to start a container with a disconnected interactive terminal.

I think I need to remember how IO was done here (I usually stay over in library land 😬) a tad more, but my remembrance was: We allocate 2 pipes (and 3 if -i was supplied) and send them to the daemon to use to send output to the client. However, the other bit I vaguely remember (you'd think I'd remember more as I added it..) output is always asked for from the process because we always write to a log file in addition to the pipes sent over by the client. This is how container log functions, we just tail/read(2) etc. this log file by having the daemon just send us the fd of it over xpc, and if any pipes were sent by the client we just multiplex the writes to the pipes as well. Every container is always ran in a separate process that the daemon spins up, so there should always be something holding open the IO for the lifetime of the container, that's the part that I wasn't super clear on if you found anything that falls over today. I'd imagine with the way IO is setup attach could basically be: send 2/3 new pipe fds to Sandbox process (the process running the container/VM), lock on some data structure that holds the current set of fds to multiplex to, and then continue writing.

@dcantah
Copy link
Member

dcantah commented Jul 22, 2025

After a reworking of some types locally, I'm somewhat convinced server initiated stdio may be a blessing. I'll be looking at this tomorrow morning

dcantah
dcantah reviewed Jul 23, 2025
View reviewed changes
Sources/CLI/Container/ContainerAttach.swift
try ensureRunning(container: container)

// Check if container has terminal enabled
guard container.configuration.initProcess.terminal else {
Copy link
Member

Choose a reason for hiding this comment

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

Why? If you just wanted to attach to see stdout/err really quick this shouldn't be required I'd imagine. Likewise even if you wanted to pipe some stdin I'd think you wouldn't need a pty

dcantah
dcantah reviewed Jul 23, 2025
View reviewed changes
Sources/CLI/Container/ContainerAttach.swift
var noHistory = false

@Option(name: .customLong("detach-keys"), help: "Override the key sequence for detaching a container")
var detachKeys: String?
Copy link
Member

Choose a reason for hiding this comment

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

Could not be happier about this 😆 I was honestly hoping someone would implement it.

@dcantah
Copy link
Member

dcantah commented Jul 23, 2025 •
edited
Loading

One thing I can confidently say before reviewing the rest, I think we'd either want to move wholesale to server provided stdio or client and not a mix. I know you left in the old way so it's not an even bigger change/controversial, but we'd definitely just want one or the other. I'm honestly somewhat leaning towards the server end like I mentioned after trying to do a refactor the other day that was miserable, but I'll need to keep reading here and thinking about pitfalls

@egernst egernst mentioned this pull request Aug 4, 2025
Open
1 task
@egernst egernst added enhancement New feature or request next Items in scope for the next few milestones labels Aug 4, 2025
@egernst egernst added this to the 2025-09 milestone Sep 2, 2025
@jglogan jglogan removed this from the 2025-09 milestone Sep 10, 2025
@Mcrich23
Copy link
Contributor

Hey! So sorry, but in an effort to make plugin development more possible #603 and #635 have lead to the CLI folder being renamed to ContainerCommands and that will impact the merging of your pull request. Just an FYI, so you understand the issue when you are resolving conflicts.

@jacques-n jacques-n closed this Sep 19, 2025
@jacques-n
Copy link
Author

Closing as not a priority. Others are free to pick up this code and start again if it becomes a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcantah dcantah dcantah left review comments

Assignees

No one assigned

Labels

enhancement New feature or request next Items in scope for the next few milestones

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Request]: Attach the terminal to a running container.

5 participants

@jacques-n @dcantah @Mcrich23 @jglogan @egernst