-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Allocation autocomplete, client api #2995
Conversation
b971ee2
to
cc97937
Compare
cc97937
to
3630cbc
Compare
e67d6a3
to
e255d75
Compare
api/job_resources.go
Outdated
"github.com/hashicorp/nomad/nomad/structs" | ||
) | ||
|
||
type JobResources struct { |
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.
Why JobResources
and not just Resources
. The object can list more than just jobs.
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.
Yes, that was one thing I wasn't sure about, as currently a resource in the api package is commented as "Resource encapsulates the required resources of a given task or task group."
api/job_resources.go
Outdated
} | ||
|
||
// List returns a list of all resources for a particular context. If a | ||
// context is not specified, matches for all contezts are returned. |
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.
contezts -> contexts
api/job_resources.go
Outdated
|
||
// List returns a list of all resources for a particular context. If a | ||
// context is not specified, matches for all contezts are returned. | ||
func (j *JobResources) List(prefix string, context string) (*structs.ResourceListResponse, error) { |
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.
Don't want to take or return anything from structs
in this package. Create a copy of the ListResponse here.
nomad/resources_endpoint.go
Outdated
@@ -73,6 +73,15 @@ func getResourceIter(context, prefix string, ws memdb.WatchSet, state *state.Sta | |||
} | |||
} | |||
|
|||
// If the length of a string is odd, return a subset of the string to the last | |||
// even character | |||
func roundDownIfOdd(s string) string { |
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 am fine with this if it is a place holder to a slightly more advanced implementation. My concern here is two fold:
- Jobs don't need to be rounded, only UUIDs.
- If I do a prefix of ab1 and I get a return of: ["ab12..", "ab23...", "ab34", ...] something seems broken :)
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.
Ok, understood. So we can round the UUID for the query, and then use a filter function for the remaining character.
api/job_resources.go
Outdated
var resp structs.ResourceListResponse | ||
req := &structs.ResourceListRequest{Prefix: prefix, Context: context} | ||
|
||
_, err := j.client.write("/v1/resources/", req, &resp, nil) |
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.
is /v1/resources
a new endpoint?
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.
We renamed it to /v1/search in a later commit, but this is a new endpoint.
438c137
to
7b6f82e
Compare
api/search.go
Outdated
|
||
// List returns a list of matches for a particular context and prefix. If a | ||
// context is not specified, matches for all contexts are returned. | ||
func (s *Search) List(prefix, context string) (*structs.SearchResponse, error) { |
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.
List -> PrefixSearch
api/search.go
Outdated
import ( | ||
"github.com/hashicorp/nomad/nomad/structs" | ||
) | ||
|
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.
Can we add consts for the various contexts and use them in the commands pkg
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.
Definitely. These should live in the api package?
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.
Probably both where API pkg defines them from structs
nomad/search_endpoint.go
Outdated
if len(prefix)%2 == 0 { | ||
return prefix | ||
} | ||
return prefix[:len(prefix)-1] |
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.
l := len(prefix)
return prefix[:l-l%2]
nomad/search_endpoint.go
Outdated
continue | ||
} | ||
|
||
if !isSubset(prefix, id) { | ||
break |
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.
break or continue?
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.
Yes, I caught this after pushing.
8518a9a
to
e59e21a
Compare
nomad/search_endpoint.go
Outdated
st "github.com/hashicorp/nomad/nomad/structs" | ||
) | ||
|
||
// truncateLimit is the maximum number of matches that will be returned for a |
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.
Put comment over field
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.
And below
nomad/search_endpoint.go
Outdated
|
||
memdb "github.com/hashicorp/go-memdb" | ||
"github.com/hashicorp/nomad/nomad/state" | ||
st "github.com/hashicorp/nomad/nomad/structs" |
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.
Can you keep it as structs. Avoid named imports unless there is a conflict or the name is terribly long
nomad/search_endpoint.go
Outdated
continue | ||
} | ||
|
||
if !isSubset(prefix, id) { |
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.
nomad/search_endpoint.go
Outdated
contexts = []st.Context{args.Context} | ||
} | ||
|
||
for _, e := range contexts { |
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.
e
-> context
or ctx
nomad/structs/structs.go
Outdated
@@ -89,6 +89,17 @@ const ( | |||
GetterModeDir = "dir" | |||
) | |||
|
|||
// Context is a type which is searchable via a unique identifier. |
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.
Comment doesn't make much sense.
Context defines the scope in which a search for Nomad object operates
cc5a49f
to
26c8aea
Compare
small refactors
comment updates
only applies to uuids, not jobs
refactor for simplicity
fix up endpoints
26c8aea
to
fad3d3d
Compare
fad3d3d
to
3319fb5
Compare
api/search.go
Outdated
} | ||
|
||
// PrefixSearch returns a list of matches for a particular context and prefix. If a | ||
// context is not specified, matches for all contexts are returned. |
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: Might be worth deleting the last line of the comment since we now assume there will always be a context, just contexts.All
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. |
This PR introduces two changes:
nomad alloc-status
for allocation idsOne thing to consider is that we use the term
resource
in #2964, but this term already exists in for machine resources in/api/resources
. I used/api/job_resources
to differentiate, but let me know if this should be named something differently or included in/api/resources