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

allocs: Add nomad alloc signal command #5515

Merged
merged 2 commits into from
Apr 26, 2019
Merged

Conversation

endocrimes
Copy link
Contributor

This command will be used to send a signal to either a single task within
an allocation, or all of the tasks if is omitted. If the sent
signal terminates the allocation, it will be treated as if the allocation
has crashed, rather than as if it was operator-terminated.

Signal validation is currently handled by the driver itself and nomad does
not attempt to restrict or validate them. There is future work to ensure
compatibility of drivers and portability of signal names.

This command will be used to send a signal to either a single task within an
allocation, or all of the tasks if <task-name> is omitted. If the sent signal
terminates the allocation, it will be treated as if the allocation has crashed,
rather than as if it was operator-terminated.

Signal validation is currently handled by the driver itself and nomad
does not attempt to restrict or validate them.
}

count := len(a.All)
if count > 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is preventing auto-complete from working if they have already typed nomad alloc -s SIGUSR1
is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh - nope. I'll take a look into the predictor to see how to work around that - good spot 😭

Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

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

some questions about autocomplete, otherwise, LGTM

return []string{}
} else if count <= 2 {
// Searching for an alloc because we either have only [signal] or [signal prefix]
resp, _, err := client.Search().PrefixSearch(a.Last, contexts.Allocs, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will return all extant allocations, even if they are stopped... do we want to (can we afford to) filter down by running allocations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's no good way to do this here tbh - I assume people usually want this for "i know the first few characters", which there's usually fairly minimal overlap of

@endocrimes endocrimes merged commit 91fa55f into master Apr 26, 2019
@endocrimes endocrimes deleted the dani/f-alloc-signal branch April 26, 2019 12:21
@@ -4,6 +4,8 @@ import (
"fmt"
"sort"
"time"

"github.com/hashicorp/nomad/nomad/structs"
Copy link
Contributor

Choose a reason for hiding this comment

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

The API package shouldn't reference the internal nomad/structs package - doing so leaks many internal packages and transitive dependencies to api clients and make dependency management more complicated, specially for go mod users.

Can you copy the relevant structs here?

@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 11, 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

4 participants