-
-
Notifications
You must be signed in to change notification settings - Fork 116
feat: "exec" command to start processes inside remote containers #139
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
Conversation
psviderski
left a comment
There was a problem hiding this 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/uncloud/service/exec.go
Outdated
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "exec [OPTIONS] SERVICE COMMAND [ARGS...]", | ||
| Short: "Execute a command in a running container", |
There was a problem hiding this comment.
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.
cmd/uncloud/service/exec.go
Outdated
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>—-cis 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 isservice-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.
There was a problem hiding this comment.
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.
psviderski
left a comment
There was a problem hiding this 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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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:
Left a few TODOs:
docker/client_exec.goand `pkg/client'service.goandserver.go