Skip to content
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

nomad exec part 2: CLI #5633

Merged
merged 3 commits into from
May 15, 2019
Merged

nomad exec part 2: CLI #5633

merged 3 commits into from
May 15, 2019

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented Apr 30, 2019

Sequel to #5632 .

This PR adds the actual nomad exec CLI command, that's inspired by docker exec with some modification:

nomad alloc exec --job example-job /bin/bash

The command is primarily intended for interactive debugging so it makes the default more sensible for that case:

  • Default to passing stdin to ultimate command - in docker exec terminology, --interactive is enabled by default.
    • I found docker terminology very confusing (since passing stdin isn't necessary interactive), so I used the long form --stdin instead, matching kubectl flag
  • Default to setting up a psudo-terminal, a tty, if stdin is detected to be a tty. So when running inside an interactive session, -tty is enabled.

For explicitly negating the options, we use the golang flag way of disabling (e.g. --tty=false, --stdin=false).

I added this as a separate PR to separate the CLI UX review from internal plumbing of nomad exec.

@jippi
Copy link
Contributor

jippi commented May 1, 2019

@notnoop nice, this seems like a similar thing to nomad-helper attach ?

@notnoop
Copy link
Contributor Author

notnoop commented May 1, 2019

@jippi indeed, they are functionally similar. By this being built-in, it can work without ssh access (only using the nomad API which can handle forwarding across federated regions) and other drivers besides docker. When targeting an exec task for example, the command will run inside the exec task chroot/container environment.

@jippi
Copy link
Contributor

jippi commented May 3, 2019

@notnoop thats great! :) Any plans to build in similar "interactive" flow as my tool have? we use it a ton internally, both on infra and dev side - and being guided through each selection of job, alloc and task is a pretty loved feature (opposite to a random alloc if omitted)

@42wim
Copy link
Contributor

42wim commented May 3, 2019

Looks more like the exec functionality of https://github.com/42wim/nomadctld ;)

Maybe make /bin/sh default command if not specified? That's what you probably need most of the time.

Copy link
Contributor

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

Good start, but I'd like to see command line argument handling cleaned up a little. I'd also like to see test cases for handling of -job and --task, for both happy and sad paths.

command/alloc_exec.go Outdated Show resolved Hide resolved
return mergeAutocompleteFlags(c.Meta.AutocompleteFlags(FlagSetClient),
complete.Flags{
"--task": complete.PredictAnything,
"-job": complete.PredictAnything,
Copy link
Contributor

Choose a reason for hiding this comment

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

We could predict the job here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite - this matches the pattern of nomad logs where --job is boolean flag about whether to interpret the first argument is an alloc or a job id. So --job itself doesn't take any arguments technically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - In which case the documentation for -job needs to be updated to show that it's parsed as a boolean flag

command/alloc_exec.go Show resolved Hide resolved
}

if numArgs := len(args); numArgs < 1 {
if job {

This comment was marked as outdated.

command/alloc_exec.go Outdated Show resolved Hide resolved
command/alloc_exec.go Show resolved Hide resolved
@notnoop notnoop force-pushed the f-nomad-exec-parts-01-base branch from bffe3d2 to 980e4f5 Compare May 9, 2019 20:49
@notnoop notnoop changed the base branch from f-nomad-exec-parts-01-base to master May 9, 2019 23:39
@notnoop notnoop force-pushed the f-nomad-exec-parts-02-cli branch from f6969b3 to 96ac203 Compare May 9, 2019 23:42
Copy link
Member

@nickethier nickethier left a comment

Choose a reason for hiding this comment

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

I would say that alloc_exec.go could use some additional comments throughout especially in execImpl. Working with terminals/ttys has bit me in the past before. Otherwise the impl LGTM.

Only one question I had before hitting approve:
Is there an escape sequence similar to ~. to break the session when using a tty? I think something like that would be really useful over having to send a SIGTERM to the pid.

-job
Use a random allocation from the specified job ID.

-i
Copy link
Member

Choose a reason for hiding this comment

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

If this default's to true then how do you unset it? -i false? I'm imagining the inverse since that's how docker works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified - our CLI library is different from docker - it needs to be -i=false.

return isTerminal
}

func setRawTerminal(stream interface{}) (cleanup func(), err error) {
Copy link
Member

Choose a reason for hiding this comment

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

comment on this func would be helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@notnoop notnoop requested a review from nickethier May 15, 2019 14:39
@notnoop
Copy link
Contributor Author

notnoop commented May 15, 2019

Is there an escape sequence similar to ~. to break the session when using a tty? I think something like that would be really useful over having to send a SIGTERM to the pid.

This is an excellent suggestion. I'm going to follow up with adding it in a separate PR.

@notnoop notnoop merged commit 72b2fb1 into master May 15, 2019
@notnoop notnoop deleted the f-nomad-exec-parts-02-cli branch May 18, 2019 00:47
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants