Skip to content

Conversation

@tonyo
Copy link
Collaborator

@tonyo tonyo commented Oct 10, 2025

The PR adds initial implementation for "uc service exec".
In the current implementation, a random container from the service is picked if the service has multiple replicas.

Done:

  • Docs
  • Tests

Left a few TODOs:

  • Handling Ctrl-C and other signals; currently we don't forward the signals via the stream
  • We can be smarter about TTY detection; currently the code mirrors "docker compose" behavior
  • Merging docker/client_exec.go and `pkg/client'
  • Implementation should be better split between service.go and server.go

Copy link
Owner

@psviderski psviderski left a comment

Choose a reason for hiding this comment

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

Amazing work! 👏
Don't be scared by the number of comments. Some of them just share some context. I hope this review is helpful.


cmd := &cobra.Command{
Use: "exec [OPTIONS] SERVICE COMMAND [ARGS...]",
Short: "Execute a command in a running container",
Copy link
Owner

Choose a reason for hiding this comment

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

nit: I prefer to not use the container term on its own as the main compute primitive in uncloud is a service, not container. So "Execute a command in a running service" or "Execute a command in a running service container" would be more consistent with other commands.
I think I used "service container" in multiple flag descriptions already.

Short: "Execute a command in a running container",
Long: `Execute a command in a running container within a service.
If the service has multiple replicas, the command will be executed in the first container.
Copy link
Owner

Choose a reason for hiding this comment

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

"the first container" is vague as it's unclear what the sorting criterion is. In fact, the current InspectService is non-deterministic as the results from multiple machines may arrive in random?/based on latency? order and we don't sort them automatically.

I think we should either say that it's an arbitrary/random container or implement some deterministic ordering and describe what it is here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part (how to pick the container) I haven't finalized yet, we can discuss.
By default it can be random, but we can also let people provide the container ID, and also let specify a machine where it runs.

So e.g. we have

✓ $ uc inspect test
ID:    6133c696ea9003e49620465261be4aff
Name:  test
Mode:  replicated

CONTAINER ID   IMAGE         CREATED          STATUS          MACHINE
2b250daefc24   alpine:3.20   22 minutes ago   Up 22 minutes   machine-1

So it'll be possible to do
uc exec test -c 2b250daefc24 (this will uniquely identify a container)
or
uc exec test -m machine-1 (this might be ambiguous, so we'll have to document it clearly).
Wdyt?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should come up with a universal language for selecting a service container and use it consistently for all commands where it's applicable (exec, logs, rm, restart, etc.). For example,

  • <service-name-or-id> --container <container-name-or-id>-c is already used by --context
  • <service-name-or-id>/<container-name-or-id> — can support specifying multiple service containers
  • <service-name-or-id> <container-name-or-id> — positional arguments (can specify only one service container)
  • <container-name-or-id> — by default, a container name is service-name-RANDOM_ASCII{4}, there is minor chance of collision if many containers on multiple machines.

I like <service-name-or-id>/<container-name-or-id> which is concise. But it's a bit harder to discover than the explicit --container flag. This format can potentially support project-or-namespace/service/container in the future.

I also like how we already consistently use -m|--machines filter in multiple commands. So I think it make sense for this command as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implemented "--container" for now; alternative options with slashes will need a bit more thinking and better understanding of how we want to move forward with the project (namespace) support.

@tonyo tonyo marked this pull request as ready for review November 3, 2025 23:32
@tonyo tonyo merged commit abea3e2 into main Nov 5, 2025
4 checks passed
@tonyo tonyo deleted the tonyo/uc-exec-1 branch November 5, 2025 22:47
Copy link
Owner

@psviderski psviderski left a comment

Choose a reason for hiding this comment

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

What a piece of work! Thank you for your effort in putting this all together ❤️

}
// The stdin goroutine may still be blocked in stream.Recv() waiting for client data,
// so cancel it explicitly.
cancel()
Copy link
Owner

Choose a reason for hiding this comment

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

I see the intention but it doesn't seem it works this way. The passed handlerCtx is not handled in any way in handleServerExecInput. I'm not sure if it's possible to interrupt .Recv but maybe it's not even needed as it will unblock when the client disconnects anyway.
But this cancelable context is a bit misleading

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes, missed the context cancellation handling, added it here and updated the comments: 30bb33f
You're right that .Recv is blocking and in some cases we won't be able to cancel the input-handling goroutine properly, but I think it still makes sense to attempt the best-effort cancellation to reduce potentially cryptic error messages in the logs.

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.

3 participants