Add pagination, filtering and sort to more API endpoints #12186

lgfa29 · 2022-03-03T21:53:37Z

This PR builds on previous PR s(#11648, #12034, #12128, #12090) to add pagination, filtering, and sorting to the following endpoints:

/v1/acl/tokens
/v1/allocations
/v1/jobs
/v1/volumes

Adding sort turned out to be more complex for job and volumes because their IDs are not globally unique, they need to use a CompoundIndex with namespace and ID. Adding CreateIndex as an additional component of this index resulted in an error when trying to do prefix query, so reverse sorting is not supported /v1/jobs and /v1/volumes.

This PR also renamed the Ascending flag to Reverse so its meaning is more generic. Ascending assumes that the default order is descending, but that may not always be the case.

It also refactors the paginator (more details in 1a5d07c) and the implementation of wildcard namespace listing for /v1/jobs and /v1/allocations to avoid having two different methods that are almost the same.

Documentation will be provided in #12094

Closes #11742
Closes #7118

Note to reviewers: it's probably better to review by commit, the work of implementing additional endpoints is somewhat repetitive, and some of the changes resulted in a significant diff, but are easier to understand in isolation.

As more endpoints started using the API response paginator, new patterns started to emerge that required either more flexible configuration or a lot of re-used code. This commit splits the paginator into three logical components: - The Paginator is reponsible for splitting the values of an iterator into chunks of the requested size. - The Tokenizer returns a page token that is used to uniquely identify elements of the iterator. - The Filter is used to decide if a given element should be added to the page or skipped altogether. The Paginator implementation has returned, more or less, to its initial clean design and implementation. Additional logic and flow customization can be provided by implementing the Tokenizer and Filter interfaces. One Tokenizer implementation is provided (StructsTokenizer) which can be configured to emit tokens using common fields found in structs, such as ID, Namespace, and CreateIndex. The go-bexpr Evaluator implements the Filter interface, so it can be used directly. Another implementation provided is the NamespaceFilter, which can be used to apply filtering logic based on the namespaces the ACL token used for the request is allowed to access.

It's currently not possible to provide consistent sorting by CreateIndex to this endpoint because it requires a Namespace and ID to be able to uniquely identify a volume. This reduces the type of go-memdb indexes that can be created where the raft CreateIndex would be in a position that would allow sorting with prefix match.

It's currently not possible to provide consistent sorting by CreateIndex to this endpoint because it requires a Namespace and ID to be able to uniquely identify a job. This reduces the type of go-memdb indexes that can be created where the raft CreateIndex would be in a position that would allow sorting with prefix match.

Co-authored-by: Seth Hoenig <seth.a.hoenig@gmail.com>

Refactor the allocations list endpoint to merge the listAllNamespaces special case into the normal List function. Co-authored-by: Seth Hoenig <seth.a.hoenig@gmail.com>

Refactor the jobs list endpoint to merge the listAllNamespaces special case into the normal List function.

Using reverse provides a more generic intention that the results should be returned in the reverse of the default order instead of assuming any particular order.

shoenig

LGTM - no blockers, just rambling thoughts. We could refactor this until the end of time, and then start all over once generics get here.

.changelog/12186.txt

shoenig · 2022-03-04T14:12:32Z

api/api.go

-	// Currently only supported by evaluations.List and deployments.list endpoints.
-	Ascending bool
+	// Currently only supported by specific endpoints.
+	Reverse bool


Thanks for knocking this out!

shoenig · 2022-03-04T14:27:33Z

nomad/state/paginator/tokenizer.go

+	}
+}
+
+func (it StructsTokenizer) GetToken(raw interface{}) string {


Not a blocker, just thinking: it seems like we could boil this down to a GetToken() string interface that is implemented by the types that otherwise implement GetID, GetNamespace, and/or GetCreateIndex. And then we'd just call that, instead of building the same value dynamically via StructsTokenizer.

I floated something similar to @lgfa29 in slack and he pointed out that the specific token we want is set by the caller and not by the type. So you'd end up with something like GetToken(byNamespace, byID, byCreateIndex bool) string or GetToken(tokenOpts) which doesn't feel a lot better.

Yup, I couldn't find a nice interface since the token format depends on the query params used in the request. The struct doesn't know which format it should use by itself.

nomad/state/paginator/tokenizer.go

tgross · 2022-03-04T14:48:31Z

nomad/state/state_store.go

 // Evals returns an iterator over all the evaluations in ascending or descending
 // order of CreationIndex as determined by the ascending parameter.
-func (s *StateStore) Evals(ws memdb.WatchSet, ascending bool) (memdb.ResultIterator, error) {
+func (s *StateStore) Evals(ws memdb.WatchSet, reverse bool) (memdb.ResultIterator, error) {


For the docstring, it's the reverse parameter now.

But seeing this API makes me wonder if we should have a const we can use for clarity at the call-site. Something like:

const SortedReverse = true const Sorted = false

Ah good call, that would avoid mysterious boolean args, thanks! 🙂

I called it SortedDefault because just store.Evals(ws, state.Sorted) felt like there would be a Unsorted option 😅

I also didn't create a custom type because this is just a boolean that would often be loaded from somewhere else (like a request). So it felt a bit much to require type casting.

Not to nitpick, but I do hate bool parameters as you can rarely tell what they're for at the call site. Here's an alternative similar to Tim's if you still have energy left to mess with it:

type SortOrder bool const SortAscending SortOrder = true const SortDescending SortOrder = false func (s *StateStore) Eval(ws memdb.WatchSet, sort SortOrder) (memdb.ResultIterator, error) { /* ... */ } func SomeCallSite() { iter, err := state.Eval(ws, SortAscending) // ...as opposed to this without the const & type def: iter, err := state.Eval(ws, true) }

Yeah, that's what I started doing. I can follow through with that.

I also notice while writing the docs that it's super confusing that reverse only works with certain combination of query params, so I'm going to make it work in all cases. The good thing about renaming it to reverse is that it just means using GetReverse instead of Get, no need to mess with the index schema to get some kind of specific order out 🙂

Actually, scratch that last part. It would add a lot of extra code (mainly tests, but still) to this PR that is already pretty big. I will raise smaller follow-up PRs for each endpoint.

@tgross @schmichael I used a named type value for setting the order in 6784b79 if you would like to take another look.

ugh..sorry for the early ping, I'm working on fixing the tests.

Ok, tests are passing now. I accidentally inverted the values of SortDefault and SortReverse 😅

New commit with the changes is cb8c6d0

tgross · 2022-03-04T14:53:09Z

nomad/state/paginator/tokenizer.go

+	}
+}
+
+func (it StructsTokenizer) GetToken(raw interface{}) string {


I floated something similar to @lgfa29 in slack and he pointed out that the specific token we want is set by the caller and not by the type. So you'd end up with something like GetToken(byNamespace, byID, byCreateIndex bool) string or GetToken(tokenOpts) which doesn't feel a lot better.

tgross · 2022-03-04T14:53:54Z

nomad/state/paginator/paginator.go

@@ -1,4 +1,4 @@
-package state
+package paginator


Good call moving this out to its own package

I debated whether or not it should still be in nomad/state or just nomad, but I figure there isn't really much difference for now.

nomad/state/paginator/paginator.go

github-actions · 2022-10-25T02:46:09Z

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.

lgfa29 added a commit that referenced this pull request Mar 3, 2022

changelog: add entry for #12186

26359f2

vercel bot temporarily deployed to Preview – nomad March 3, 2022 22:01 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 3, 2022 22:04 View deployment

lgfa29 and others added 7 commits March 3, 2022 17:27

api: add filter, pagination, and sorting to /v1/acl/tokens

d5a0d7f

api: add filter, pagination, and sorting to /v1/allocations

ef098d7

Co-authored-by: Seth Hoenig <seth.a.hoenig@gmail.com>

api: refactor allocations list endpoint

194c407

Refactor the allocations list endpoint to merge the listAllNamespaces special case into the normal List function. Co-authored-by: Seth Hoenig <seth.a.hoenig@gmail.com>

api: refactor jobs list endpoint

cac9ec4

Refactor the jobs list endpoint to merge the listAllNamespaces special case into the normal List function.

lgfa29 added a commit that referenced this pull request Mar 3, 2022

changelog: add entry for #12186

ce97a03

lgfa29 force-pushed the f-api-sort-filter-paginate branch from 26359f2 to ce97a03 Compare March 3, 2022 22:32

vercel bot temporarily deployed to Preview – nomad March 3, 2022 22:32 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 3, 2022 22:32 View deployment

lgfa29 added this to the 1.3.0 milestone Mar 3, 2022

lgfa29 added 2 commits March 3, 2022 17:59

api: rename ascending query param to reverse

8582efb

Using reverse provides a more generic intention that the results should be returned in the reverse of the default order instead of assuming any particular order.

changelog: add entry for #12186

e181fd6

lgfa29 force-pushed the f-api-sort-filter-paginate branch from ce97a03 to e181fd6 Compare March 3, 2022 23:00

vercel bot temporarily deployed to Preview – nomad March 3, 2022 23:00 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 3, 2022 23:00 View deployment

lgfa29 requested review from shoenig, schmichael and tgross March 3, 2022 23:31

lgfa29 added the theme/api HTTP API and SDK issues label Mar 3, 2022

shoenig approved these changes Mar 4, 2022

View reviewed changes

tgross approved these changes Mar 4, 2022

View reviewed changes

lgfa29 added 2 commits March 7, 2022 15:00

remove unused code

e79ecf8

changelog: update entry for #12186

837884c

vercel bot temporarily deployed to Preview – nomad March 7, 2022 23:35 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 7, 2022 23:35 View deployment

lgfa29 force-pushed the f-api-sort-filter-paginate branch from b4ce09a to b1dfc48 Compare March 7, 2022 23:41

vercel bot temporarily deployed to Preview – nomad March 7, 2022 23:42 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 7, 2022 23:42 View deployment

fix typo

f7392b8

lgfa29 force-pushed the f-api-sort-filter-paginate branch from b1dfc48 to 6784b79 Compare March 8, 2022 14:54

vercel bot deployed to Preview – nomad-storybook-and-ui March 8, 2022 14:54 View deployment

vercel bot temporarily deployed to Preview – nomad March 8, 2022 14:54 Inactive

use named value to indicate reverse sorting

cb8c6d0

lgfa29 force-pushed the f-api-sort-filter-paginate branch from 6784b79 to cb8c6d0 Compare March 8, 2022 16:23

vercel bot deployed to Preview – nomad-storybook-and-ui March 8, 2022 16:23 View deployment

vercel bot temporarily deployed to Preview – nomad March 8, 2022 16:23 Inactive

lgfa29 mentioned this pull request Mar 9, 2022

api: apply consistent behaviour of the reverse query parameter #12244

Merged

lgfa29 merged commit 154264f into main Mar 9, 2022

lgfa29 deleted the f-api-sort-filter-paginate branch March 9, 2022 01:54

This was referenced Mar 9, 2022

api: sort and filter allocations list endpoint #12090

Closed

fix CSI volume list test #12245

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination, filtering and sort to more API endpoints #12186

Add pagination, filtering and sort to more API endpoints #12186

lgfa29 commented Mar 3, 2022 •

edited

Loading

shoenig left a comment

shoenig Mar 4, 2022

shoenig Mar 4, 2022

tgross Mar 4, 2022

lgfa29 Mar 7, 2022

tgross Mar 4, 2022

lgfa29 Mar 7, 2022

lgfa29 Mar 7, 2022

schmichael Mar 8, 2022 •

edited

Loading

lgfa29 Mar 8, 2022

lgfa29 Mar 8, 2022

lgfa29 Mar 8, 2022

lgfa29 Mar 8, 2022

tgross Mar 4, 2022

tgross Mar 4, 2022

lgfa29 Mar 7, 2022

github-actions bot commented Oct 25, 2022

Add pagination, filtering and sort to more API endpoints #12186

Add pagination, filtering and sort to more API endpoints #12186

Conversation

lgfa29 commented Mar 3, 2022 • edited Loading

shoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2022

lgfa29 commented Mar 3, 2022 •

edited

Loading

schmichael Mar 8, 2022 •

edited

Loading