Fix /v1/jobs/statuses pagination for jobs that share a ModifyIndex - #28178
Fix /v1/jobs/statuses pagination for jobs that share a ModifyIndex#28178afreidah wants to merge 4 commits into
Conversation
github-project-automation
Bot
moved this to Needs Triage
in Nomad - Community Issues Triage
7df789b to
839f5b3
Compare
|
Also, this is what the diff would look like on the full refactor I mentioned in this PR and in the issue.. It actually isn't as bad as I was thinking it was going to be unless I missed something. main...afreidah:nomad:pagination-tokenizers-shared-helper If it is preferred to go that direction I can close this PR and push that one, or update this branch to match that one and re-push. Otherwise the narrow bare-minimum fix is ready and I can just file the rest as another gh issue if that is preferable. |
gulducat
left a comment
gulducat
left a comment
There was a problem hiding this comment.
Heya @afreidah! Thanks for taking this on!
Sorry for the delay; it's summer-break time so my availability is spotty, but I wanted to keep up with this with ya personally.
The backend work looks sound to me. I'm open to the refactor too, which I agree doesn't look too bad, but I'd like that as a separate follow-up PR if you want to go ahead with it, so we can keep this one pretty UI-centric.
I have more concerns about the frontend stuff, some minor code thoughts, but more substantively we have some UX design decisions to consider. Let me know what ya think down in that comment.
| // Stack of the page-start cursors we've navigated through, so "prev" can step | ||
| // back without re-deriving a token. Each entry is the cursorAt that started a | ||
| // page (null for the first page). The pagination token is opaque, so we never | ||
| // do arithmetic on it. | ||
| @tracked previousTokens = []; |
There was a problem hiding this comment.
I'll leave overall UI comments here as a general discussion thread.
This all seems pretty functional to me, but some observations:
This array can have 3 different types in it: a string, null, or undefined. This is pretty pedantic (which is rich coming from a backend dev), but I think we should always make sure this.cursorAt is either a string or undefined, never null. From what I've gathered, JS tends to omit undefined values from map serialization, which I think is what we want if there's no extant cursor. e.g. JSON.stringify:
$ node -e 'console.log(JSON.stringify({"u": undefined, "n": null}))'
{"n":null}
But for the array, IMO it should be empty if there's no "history", rather than a length=1 [undefined].
Buuuuut more meaningfully, this previousTokens stack turns "prev" into more of a browser < button, i.e. it's not really a "page back" but a "history back" button. It's efficient in the sense that we don't have to request more jobs than we need, but it also means that a user can't easily accomplish something like "go to the penultimate page" or generally go backwards from the last page.
The last point is a bit hairy, but I think we need an answer for it to proceed here. Basically I think we either need it to behave like a pager, or we should somehow change the UI appearance to make it clearer that it's a "history back" button instead of an arbitrary page-navigator.
Personally, I want it to behave like a page-nav, even if that means we need to request one or double extra jobs to accommodate the result.
What do you think?
There was a problem hiding this comment.
@gulducat you raise some good points I hadn't considered, thanks! I'll go take a deeper look at your first point because I think you are 100% correct about the serialization/undefined point. Shouldn't be a big deal for me to write a test for that and then tweak it.
As for the second point....yeah, that is a good one, I hadn't even considered it from that perspective. I'm gonna dig into this one a bit deeper and then reply again but I am inclined to agree with you that this should not be doing things that spill into a larger UI change just to fix a small bug and the right way forward is probably to make sure it just fixes the bug without changing the conceptual logic of the web interface even if it isn't the most efficient way possible.
I'll take a look at both shortly and see what I find and come up with and bounce the ideas back off you. Thanks for the useful feedback!
There was a problem hiding this comment.
also, 👍 for using 'penultimate' in a sentence! I haven't heard anybody but me say that word since I graduated from a music conservatory 17 years ago. 'The penultimate measure' in reference to sheet music was a very commonly heard phrase at Peabody as you can imagine haha. Taking a look at the code now, might have some comments in a little bit. The undefined thing looks pretty straightforward at first glance...famous last words.
There was a problem hiding this comment.
spent a bit of time with it last night, I think I pretty much have a handle on what changes need to be made so we can keep the fix narrow. I'll try to push an update to the PR later today after work.
There was a problem hiding this comment.
Oh dang, I've done some music composition and hadn't heard "penultimate measure" before. A bit of poking around led me here for "second-species counterpoint" that includes the "penultimate note" and "penultimate bar" but the wording there just seems coincidental, not something that's like official or formal music terminology. 🤷
839f5b3 to
5b36d70
Compare
|
@gulducat I deleted the previousTokens stack and restored main's existing page-nav (loadPreviousPageToken). The only change now is widening the cursor from a bare ModifyIndex to ModifyIndex + Namespace + ID so prev/last point at a single job when several share an index. cursorAt is now always a string or undefined, never null. Added tests for shared-ModifyIndex paging (forward + back) and prev-from-last. Larger refactor stays out per your note — tracked in #28211. Thanks for the useful information! Let me know if you think this is the right approach. I don't think that failed test has anything to do with this PR. |
gulducat
left a comment
gulducat
left a comment
There was a problem hiding this comment.
This looks great! I would merge as-is, but a couple comments for your consideration.
(PS thanks for bearing with me as I take even more summer time off work 🏖️)
| // namespace compared field-by-field, so "team" sorts before | ||
| // "team-a" (matching memdb's null-separated key order), unlike a | ||
| // whole-string compare where '.' > '-' would reverse them. |
| test('Previous from the last page navigates to the penultimate page', async function (assert) { | ||
| // Jumping to the last page and clicking Previous should land on the | ||
| // penultimate page, not on a previously-visited page. |
| // Stack of the page-start cursors we've navigated through, so "prev" can step | ||
| // back without re-deriving a token. Each entry is the cursorAt that started a | ||
| // page (null for the first page). The pagination token is opaque, so we never | ||
| // do arithmetic on it. | ||
| @tracked previousTokens = []; |
There was a problem hiding this comment.
Oh dang, I've done some music composition and hadn't heard "penultimate measure" before. A bit of poking around led me here for "second-species counterpoint" that includes the "penultimate note" and "penultimate bar" but the wording there just seems coincidental, not something that's like official or formal music terminology. 🤷
|
@gulducat thanks for the feedback, I'll double check everything you commented on and get back to you. Always use and enjoy your vacation time! On a side note when you linked second species counterpoint I was hoping you were going to link to the Johann Fux counterpoint book from the 1700s that is written using the socratic method and teaches you counterpoint by reading a conversation between a student and a teacher that is hilarious but actually does a great job of going through 5 species...if you wrote anything for guitar send it my way (or really easy piano stuff because I'm not great at it)! Discord name is same as my github name. Maybe it was just my ear training teacher Clinton Adams at Peabody that said "penultimate measure" and I had class with him every day for three years so I heard it a lot haha. |
5b36d70 to
6504ff1
Compare
|
@gulducat I made some tiny changes based on your comments:
Let me know if that adequately covers any concerns and if anything else is needed as well. Thanks! |
|
nice, no failure on the flaky test this time. |
gulducat
left a comment
gulducat
left a comment
There was a problem hiding this comment.
We're getting so close!
I think the changelog can be tightened a bit to expect less context from the reader, and I have one concern to run past you. Everything in this PR is good, but it's a bit sticky running against older servers.
Let me know what ya think!
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
| core: Fixed a bug where jobs sharing a ModifyIndex could break /v1/jobs/statuses pagination, causing the jobs index page to repeat a job across pages, stall when paging past a group of such jobs, or jump to an incorrect last page | |||
There was a problem hiding this comment.
I think this was prefixed ui: at some point? This asks the reader to connect /v1/jobs/statuses (what even is that? they might think) with the UI.
| return undefined; | ||
| } | ||
| const namespace = job.belongsTo('namespace').id() || 'default'; | ||
| return `${job.modifyIndex}.${namespace}.${job.plainId}`; |
There was a problem hiding this comment.
I found a relatively minor problem that stems from the UI expecting the server to be able to handle this new format, i.e. we're breaking backcompat between the new UI and older servers.
In cases where we can use the nextToken provided by the server, it's all gravy; we just treat that token as opaque and use it. But if we need to generate one like this, then an older server won't know what the new format means, and it'll return zero jobs.
Straightforward to reproduce by hitting the API on any released version:
➜ nomad version
Nomad v2.0.3
➜ nomad operator api '/v1/jobs/statuses?per_page=3' \
| jq -r '.[] | [.ModifyIndex, .Namespace, .ID] | join(".")'
534.default.pyweb-1
533.default.pyweb-10
530.default.pyweb-8
# then try to use one as a token
➜ nomad operator api '/v1/jobs/statuses?per_page=3&next_token=533.default.pyweb-10'
[]You can see it in action by running the ui separately. I did it like this:
➜ cd ui
➜ sed -i 's/port 4646/port 4200/' package.json
➜ npm run start:proxy
> nomad-ui@0.0.0 start:proxy
> USE_MIRAGE=false ember server --port 4200 --proxy http://127.0.0.1:4646
... and it'll build the UI and start running it on port 4200 against a localhost nomad backend ...In particular, if you hit last, or if you hit next twice, then prev.
I haven't done much to see what it would take to account for this, but I called it a "relatively minor problem", because I don't imagine a lot of people would upgrade their UI without updating the cluster (and still expect it all to work right).
I'd personally be okay just biting the bullet and shipping this, if it takes any more than even minor contortions to accomplish full backcompat.
There was a problem hiding this comment.
I chatted with my colleagues, and we would really like to retain forward-and-back compat across UI and server versions (shucks!), namely so that operators can have reliable visibility during upgrades.
Ideally, we can pull that off by getting the new UI code to treat tokens as opaque all of the time, and never construct one itself from scratch. I don't recall why we put knowledge of the token-source (modifyIndex) in the UI back when we created this stuff, so maybe there's a good reason? But let's see if we can pull that inside-baseball out of the UI code.
|
sounds good, i'll dig into that a bit later and see if we can get this squared away properly! Thanks. |
6504ff1 to
f6ff20f
Compare
|
Thanks @gulducat - reproduced the mismatch exactly as you described, so I get what the concern is now. Changelog: done - re-prefixed ui: and reworded so it doesn't lean on knowing what /v1/jobs/statuses is. Backcompat / opaque tokens: Regarding your "why does the UI know about modifyIndex" question: from what I have gathered, prev and last fire a reverse: true query to find where the previous page starts. The catch is that the query's nextToken points at the overflow job (the one used to keep walking backwards), not at the forward cursor the page actually renders from. Since the server doesn't give the UI a forward token it can use, the old code built one itself out of the job's fields. That hand-built token is the one and only place the UI has to know the token's internal format, and it's exactly what an older server can't parse. The fix: only ever reuse a token the server minted, never invent one. Then each server speaks its own dialect back to itself - bare int on old, compound on new - and LB/rolling-upgrade skew stops mattering. next/first already work this way...this brings us back to why I tried the previousTokens stack originally but we decided to try and avoid that if possible since it turned it into more of a history than a paginator, For forward navigation a history stack and a "true page-nav prev" are identical, since the only way to reach page N is paging through 1...N-1. They only diverge after a last jump. That leaves last as the one honest gap: jumping to the end has no history to pop and no server token to reuse, so it needs either reconstruction (the thing we're killing) or rendering the reverse page in place without a token. The latter is the right end state and matches the original TODO comment in the code already. Or at least that is my initial thoughts on how to deal with this. I am VERY open to any ideas I haven't considered. Otherwise, reconciling the true-nav and the LB/rolling-upgrade is a bit trickier! This is fun though. I'll keep stewing on it while I wait to hear what you guys think. |
|
Just taking stock here, which buttons need to work at which times:
First and last are easy:
Next and prev are easy, if the user is continuing in one direction, but trickier if they bounce around.
If we want to never construct the token ourselves, then I think we'll have to make an extra API call to get the server to tell us the "or whatever token" conditions, i.e. those in the opposite direction of the user's last move. It would sure be nice if the API also responded with a Sadly, regardless I don't think we can entirely avoid the mid-upgrade issue, because one server can give us a new token that we unwittingly bring to an old server for a subsequent request. But I still think it's worthwhile to aim for the UI being token-agnostic. Does all that ^ seem right to you? PS I feel compelled to ask, I hope no offense: Were parts of that last reply from Claude? Some of the language sounded LLM-y to me. Sad world where words like "honest" can make other phrases also stand out as potential robo-gen, and I start to wonder who I'm talking to. :\ |
f6ff20f to
399ac21
Compare
ModifyIndex is not unique across jobs, so once the jobs modify_index index became non-unique (hashicorp#28158) the /v1/jobs/statuses pagination cursor (ModifyIndexTokenizer) could no longer identify a unique position: jobs sharing a ModifyIndex were returned on more than one page, and a group larger than per_page pinned the cursor so older jobs were unreachable. Server: add ModifyIndexAndNamespaceIDTokenizer, which tokenizes on ModifyIndex + Namespace + ID -- matching the memdb iteration order of the non-unique index, which breaks ties on the (Namespace, ID) primary key -- with a legacy bare-integer fallback for rolling upgrades, and use it for Job.Statuses. Retire the now-unused ModifyIndexTokenizer. UI: the page token is now a compound cursor an older server cannot parse, so the jobs index must stay token-agnostic to keep working against a mixed-version cluster (e.g. mid rolling-upgrade, or behind a load balancer). Reuse only server-minted tokens: keep a small history of forward tokens and pop it for "prev", so next/prev/first never construct a token. Only "last", which has no prior token to reuse, still builds one. Reset pagination to the first page on a page-size change so stale tokens are not reused. Fixes hashicorp#28167
399ac21 to
0853567
Compare
|
@gulducat so I think your breakdown matches what I have in my head, and hopefully I implemented it in a way that follows what you were thinking as an option. Meant to get this update pushed sooner but I've been super busy with work and my wife and I moving to a new apartment and while I was itching to get the latest take finished and pushed, the need for sleep won out haha. Here's the annoying part for prev/last, and correct me if I've got it wrong. To draw any page, the UI hands the server a token that basically says "start the page here." Every time the server returns a page it gives back exactly one of these tokens: the start-here token for the next page. So "next" is free -- we're already holding the token for it. But the server never gives back a start-here token for the previous page, so "prev" and "last" have nothing to send and have to come up with one by building it from a job's fields. That build-it-by-hand step isn't new -- main already does it. It just never mattered because main's token is a bare ModifyIndex, which is exactly the format the server round-trips and both old and new servers understand. The problem only shows up once the cursor becomes the compound ModifyIndex+Namespace+ID token: that same hand-built step now produces a token an older server can't parse. So the real issue isn't that we construct a token (it always has), it's that the constructed one is now in a format old servers don't understand. what this iteration has: rather than an extra API call to fetch the opposite-direction token, I reuse the forward tokens we've already been handed: the controller keeps a small stack of the page-start tokens it's seen and pops it for "prev" (sad compromise to keep tokens opaque, but totally open to better ideas!). So next/first (and prev, as long as you paged forward to get there) now page using only server-minted tokens -- bare-integer against an older server, compound against a new one -- and never construct anything. That keeps them version-agnostic through a rolling upgrade etc... As we found, the one headache here is "last" (and any prev while paging back from it): there's no prior token to reuse, so it still constructs a cursor. That's the single non-opaque path left. I left it as-is with a comment, since -- as you already rightly pointed out -- we probably can't fully close the mid-upgrade window anyway (one server can hand us a token we then carry to another), so I didn't want to do some farther reaching refactor for a partial guarantee. I also reset pagination to the first page on a page-size change since the stacked tokens go stale when the page boundaries move. regarding your "it'd be nice if the API returned a Prevtoken header, or jobs had a PageToken field" -- that would be awesome haha, and this logic belongs on the server anyways probably....would be fun to dig into that but that seemed like way beyond the scope of this issue so I didn't even play with the idea yet. Tests: added an acceptance test that pages next/prev/first and asserts the UI never constructs a token on those paths (and that "last" does, so the boundary is intentional and doesn't silently drift), plus one for the page-size reset. It is so annoying how these chat bots are - unfortunately - making language we all use seem robotic and I find myself doing it too now even when seeing things I actually say myself lol. The one that really bugs me is that it seems like the chatbots are using "footgun" a lot now and I use that constantly ever since I had a really awesome manager in the mid 2000s for years that I learned a TON from while working at Optoro in DC who said 'footgun' constantly and it became a big part of my vocabulary and yet if I see anybody else say it I start seeing red flags. Honestly (lol!), I think one of the unexpected 'footguns' (couldn't help myself) with these chatbots and seeing their output constantly is that we are also starting to talk like them too which is an interesting/scary thought. The funniest thing is I spent more time editing that comment than any of the other ones rofl. 🤷 damn chatbots stealing our adjectives and snarky sarcastic catchphrases! Let me know your thoughts on how to best move forward. I'm down for a less temporary fix that takes a bit more work but that is probably out of scope and less of a bug fix. |
gulducat
left a comment
gulducat
left a comment
There was a problem hiding this comment.
Thanks a bunch for your stamina on this one @afreidah.
Couple more comments for you: The first I just share my thinking on why one questionable aspect is probably fine? And the other still feels pretty blocky, as we're still constructing the token in more cases than it may seem.
Lmk what you think!
(And thanks for the robot clarification. One of my pet peeves is claude squatting on "load-bearing" 😋 and oh boy "and seeing their output constantly is that we are also starting to talk like them" is such a frightening trap.)
| // Stack of visited page-start tokens, oldest first, excluding the current | ||
| // page. "prev" pops it to page back using a server-minted token instead of |
There was a problem hiding this comment.
The basic problem with keeping a previous-token stack (aside from it behaving like a history instead of a pager) is that the server-side state can change. That's always a racy problem with these multiple separate connections, but a history stack drastically widens the time period wherein the UI's page memory can drift from server state (from like sub-second to arbitrarily-many seconds).
In other words, this is not only a stack, it's a cache, and a cache with no way to determine whether it's still valid :\
But! I think practically speaking, we should only have to worry about jobs going away, being dropped from the list after GC, because we shouldn't ever see a job leap into existence in between two previously-established indexes. In that case, I think this works well enough? When paging back, if a job has disappeared, it should just be replaced with the first one on the current page.
Soooo, I think this is fine? Can you spot anything I'm missing on this point?
| // No history: we arrived via "last". Reverse-query for the previous | ||
| // page and construct its boundary cursor. The extra request returns |
There was a problem hiding this comment.
This issue is broader than clicking on last; it also applies to cold-visiting a URL that someone else maybe copied and shared (chat, ticket, email, etc), or even if you just do a page refresh on any page past page 2 (page 2 is unaffected, because loadPreviousPageToken() gets no token, so we hit line 135 and call it a day).
This parenthetical in your comment hits on this (emphasis mine)
So next/first (and prev, as long as you paged forward to get there) now page using only server-minted tokens
You're right that the current status quo is to "build" the cursor, with math on modifyIndex, but as this PR introduces breakage in that approach, I really think to change it, we need to altogether avoid constructing the cursor, and somehow only use what we can ask the server for.
As I see it:
- for
last(initial call does not get a nextToken looking forward), just do reverse=true with no token - for
prev(we did get a nextToken looking forward)- use nextToken reverse=true and page_size=+1 to get the reverseToken (what
loadPreviousPageToken()aims to do, right?) - then do reverse=true with reverseToken to get the page we actually want to see
- use nextToken reverse=true and page_size=+1 to get the reverseToken (what
I have a strong (but vague) sense that maybe a little dedicated class for handling the pagination logic might help us clean this up? rather than tacking everything onto JobsIndexController, but A) it would be more of a refactor, and B) I have no idea if that's at all idiomatic with ember.js... How does that strike you?
At any rate, we cannot fully avoid the possibility of hitting multiple different server versions with these requests, which could produce token-format conflicts, but if we're futzing with this logic, there should be a way to get the UI out of the token-gen business.
There was a problem hiding this comment.
@gulducat - regarding the stack: I don't think you're missing anything on the GC case. A job dropping out between two indexes just shifts the page contents; paging back still lands on a valid boundary. I think I agree we should drop it though, because the server-minted prev below makes it unnecessary, so we delete the cache instead of reasoning about when it's stale or not. You were right to want to avoid the stack originally and I think you are right that we can still avoid it...
On construction: agreed, and prev works cleanly server-minted. One reverse query seeded by the current cursor hands back the previous page's start token, and it works from a cold cursorAt too, so no stack required, boom!
The snag is last: Page N's nextToken is empty, so the server never mints a forward cursor for the last page. Two options:
- keep one construction, scoped to
lastonly, everything else server-minted. - represent each page as
(token, reverse)and render the last page (and back from it) from reverse queries. Zero construction, but the deep-link param has to carry direction. This is probably where your dedicated little class would come in. I gotta defer to your judgement when it comes to javascript but I think maybe the below idea could work, I'll report back after testing it.
I was thinking something like this for the rough shape of the last-only version. next/first/prev reuse server-minted tokens; last is the single spot that builds one, since nothing points back
into the final page.
@action async handlePageChange(page) {
this.jobQueryIndex = 0;
this.jobAllocsQueryIndex = 0;
if (page === 'next') {
if (!this.nextToken) return;
this.cursorAt = this.nextToken; // server-minted
} else if (page === 'prev') {
if (!this.cursorAt) return;
// A reverse query seeded at the current page's first job returns the
// previous page's start token as meta.nextToken. Keep only that token and
// render forward; the returned jobs are discarded. Works from a cold
// cursorAt (refresh / shared URL) since it reads the query param.
const res = await this.loadReverse({ next_token: this.cursorAt });
this.cursorAt = res.meta.nextToken || undefined;
} else if (page === 'first') {
this.cursorAt = undefined;
} else if (page === 'last') {
// The one page with no forward token to seed from. Fetch it in reverse and
// build the cursor from its newest job (its first in display order).
const res = await this.loadReverse();
const sorted = res.sortBy('modifyIndex');
this.cursorAt = this.cursorFor(sorted[sorted.length - 1]);
}
}
// Reverse query used by prev (to read the previous page's start token) and by
// last (to fetch the final page). per_page is exactly pageSize: prev reads
// meta.nextToken rather than picking a job, so it needs no overlap job.
async loadReverse({ next_token = undefined, per_page = this.pageSize } = {}) {
return this.store.query(
'job',
{ next_token, per_page, reverse: true },
{ adapterOptions: { method: 'GET' } },
);
}The previousTokens stack and its resets go away, and cursorFor ends up called
only from last.
The only issue I see here is I think (haven't tested it yet) that if you hit prev until you hit the first page the prev button will still show as a valid link until you click it again.
I'm gonna play around with it a bit tonight probably and test this out and see if anything else occurs to me.
There was a problem hiding this comment.
ok started playing with it because I was bored.....I think this works. But I don't like the fact that if you prev to the first page it still acts like there is a prev page until you click it again. There IS one way I can think of to avoid that without constructing a token or maintaining a history stack. A small per_page additional query that does a reverse query from the landed cursor and if nothing is newer than it we know it is on the first page and can disable that link.
something along these lines:
@tracked onFirstPage = true; // drives prev/first disabled state
// True if nothing is newer than cursorAt, i.e. this is the first page. One
// per_page=1 reverse probe; empty meta.nextToken means no newer job exists.
async reachedStart(cursorAt) {
if (!cursorAt) return true;
const probe = await this.loadReverse({ next_token: cursorAt, per_page: 1 });
return !probe.meta.nextToken;
}
Wire it into the nav that can land on the first page (and the route, for cold-load/refresh):
} else if (page === 'prev') {
if (this.onFirstPage) return;
const prevPage = await this.loadReverse({ next_token: this.cursorAt });
this.cursorAt = prevPage.meta.nextToken || undefined;
this.onFirstPage = await this.reachedStart(this.cursorAt);
} else if (page === 'first') {
this.cursorAt = undefined;
this.onFirstPage = true;
} else if (page === 'next') {
this.cursorAt = this.nextToken;
this.onFirstPage = false;
}
the downside is that it is yet another request has to fire every time and we already added one additional one for the reverse peak...but it feels a bit sloppy to have it falsely advertising that there was another prev page and web ui job page navigation isn't exactly an intensive process otherwise or something that people are expecting blinding speed on.
Gonna test this out and see if it works and makes sense.
There was a problem hiding this comment.
pushed the changes with the extra request to cover the first/prev links when on the first page. Let me know what you think about this approach. Added testing to verify the link active/inactive thing. Technically the current main generates two cursors still (prev, last) and now this only does one (last)....so...technically an improvement...
Still would love to do a follow-up look at having the server return two cursors instead of one so each direction is covered and handed out by the server and less of this ui juggling is required in the first place....but that is for a different time/gh-issue
Also pretty sure that is just a flakey test that failed again and isn't relevant to this PR/issue.
Replace the previousTokens history stack with prev navigation that reuses tokens the server mints. prev issues a reverse query seeded at the current page's first job and pages forward from the returned nextToken, so it never constructs a cursor and works from a cold cursorAt (a refreshed or shared URL). last remains the one page the server cannot mint a forward cursor for and is the sole place that builds one. Because prev now lands on a server-minted cursor rather than an empty one, the first page can no longer be inferred from cursorAt alone. Track it with an onFirstPage flag set by a per_page=1 reverse probe (reachedStart) that reports whether anything is newer than the cursor; the route runs the probe once on a cold load so first/previous are correct on a deep-linked page. Tests cover paging back onto the first page disabling first/previous, and cold-loading a shared URL past the first page rendering and paging back without constructing a token.
|
what do you think of that approach @gulducat ? I think it is a reasonable balance between the ideal solution (sever hands out next and prev every time and no ui messiness) and the current limitations of keeping the fix narrow. I'm totally down to re-work parts of it if you think there is a better approach...that is just more fun for me to have haha. To summarize what I currently have pushed here: The jobs list now pages using cursors the server hands back rather than ones the UI builds itself: next, previous, and first all reuse server-minted tokens, so the same job can't get repeated or skipped across pages. The only cursor the UI still constructs is for the "last" jump, because that's the one page boundary the server never mints a forward token for. Because prev now lands on a real token instead of an empty one, a small extra probe request tells the UI when it's back on the first page so the first/previous buttons disable correctly, and this all works even when cold-loading a refreshed or shared URL. This narrows cursor construction from two sites on current upstream main ("prev" and "last", both of which re-derive the cursor from a job's modifyIndex rather than reusing the server token) down to a single one ("last"). During a rolling upgrade, new servers understand both the old and new cursor formats, so any cursor an old server minted keeps working after that server is upgraded. The only gap is the reverse: a cursor minted by a new server that then gets routed to an old one, which can't parse the new format. Handling that cleanly would take elaborate artisanally-crafted code that only matters during the upgrade window itself, so it isn't worth the added complexity - the gap closes on its own once every server is upgraded and now there is bloat code that nobody needs. I can't really see a GOOD way to avoid this brief issue during an upgrade and would argue that doing a song and dance to try and avoid it is worse than the issue itself which would literally only exist for a few minutes during the upgrade and then never again. |
| // The one page the server can't mint a forward cursor for: nothing points | ||
| // back into the final page. Fetch it in reverse and build the cursor from | ||
| // its newest job (its first in display order). | ||
| const lastPage = await this.loadReverse(); | ||
| const sorted = lastPage.sortBy('modifyIndex'); | ||
| this.cursorAt = this.cursorFor(sorted[sorted.length - 1]); | ||
| this.onFirstPage = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think the cursorFor() method to derive the correct modify index string is going to bite us down the road if we ever want to change this fetch method again, and it sticks out a little bit here compared to the others in this if-stack.
I'd remove all the cursorFor() stuff and have this instead:
- paginate with
reverse=true - ask for 1 fewer jobs than you need
- set the nextToken from that response as
this.cursorAt
const lastPage = await this.loadReverse({
per_page: Math.max(this.pageSize - 1, 1),
});
this.cursorAt = lastPage.meta.nextToken || undefined;
There was a problem hiding this comment.
(this also makes this pagination style work in a situation where you have a new frontend and an older nomad client backend, which is a pretty common scenario in practice)
There was a problem hiding this comment.
thanks @philrenaud , I think this makes sense and I'm trying it out now and will report back.
| // pageSize: prev reads the minted token rather than picking a job out of the | ||
| // result, so it needs no overlap job. next_token defaults to undefined, which | ||
| // "last" uses to walk back from the end of the list. | ||
| async loadReverse({ next_token = undefined, per_page = this.pageSize } = {}) { |
There was a problem hiding this comment.
Note: just noticed we didn't have this in the earlier method either: I think this is supposed to take your current filter options into consideration. As it stands I believe you might believe you're paginating through your job list as you see it on page with filters applied, but this would return jobs that don't necessarily match that filter. I believe the second parameter on .query takes a filter object optionally.
There was a problem hiding this comment.
thanks, looking into this
There was a problem hiding this comment.
@philrenaud I pushed some changes after playing around with your suggestions, let me know if that aligns with what you were thinking. thanks!
… paging Address review feedback on the jobs list pagination: - last no longer constructs a cursor. It reverse-walks for one fewer than a page so the server's meta.nextToken lands on the final page's start, then reuses that minted token. cursorFor is removed, so the UI never builds a token, which also lets the list page against an older backend that mints the legacy token format. - loadReverse now carries the active filter and namespace to match the forward query. Without them, prev and last paged over a different set than the one on screen and could surface jobs the filter excludes. Tests reframed to server-minted-cursor guards, plus a new test that pages backward and to the last page with a filter applied.
|
@philrenaud and @gulducat what do you guys think? After really good input from both of you I think it is in an even better place than before. Let me know if you think additional things need to be looked into or if this looks good now. |
4 participants
What
/v1/jobs/statuses(which backs the UI jobs page) paginates with anext_tokencursor built from each job'sModifyIndexalone. That only worked whileModifyIndexwas unique per job. #28158 made the jobsmodify_indexindex non-unique - several jobs can legitimately share aModifyIndexwhen written in one Raft transaction - and once that's true, aModifyIndex-only cursor no longer identifies a single position in the list.This is the narrow fix for that: give the cursor a tiebreaker so it points at exactly one job -
ModifyIndex+Namespace+ID, compared numerically on the index, then by namespace, then by id. That matches the order the state store actually walks the non-uniquemodify_indexindex (which breaks ties on the(Namespace, ID)primary key), so paging lines up with the data.Relates to #28167.
Symptoms fixed
With jobs sharing a
ModifyIndexand pagination on (e.g. 30 jobs,per_page=25):ModifyIndexgroup is larger thanper_page, the cursor never advances past it; the same page repeats and older jobs become unreachable.All four share one root cause and are fixed by the single tokenizer change.
Reproduction
Same Docker A/B harness as #28132/#28158 (1 server + 1 client, no Consul). Jobs are forced to share a
ModifyIndexby giving every alloc the same absolute exit epoch, so their status writes coalesce into one Raft transaction:https://github.com/afreidah/nomad/tree/repro-jobs-statuses-28132/repro-28132
Walking
/v1/jobs/statuseswith a smallper_pageover jobs that share aModifyIndex, before the fix:After the fix the same walk returns every job exactly once and terminates.
Compatibility
Bare-integer tokens minted by older clients/servers are still accepted - a token that doesn't carry the namespace/id segments falls back to the previous index-only comparison - so rolling upgrades keep working.
Changes
nomad/state/paginator/tokenizer.go- replaceModifyIndexTokenizerwithModifyIndexAndNamespaceIDTokenizer(the statuses endpoint was its only caller).nomad/job_endpoint_statuses.go- use the new tokenizer.ui/- treat the pagination token as opaque (no more arithmetic on it); forward/back use a short token history, only "jump to last" derives a cursor. Mirage mock updated to the new token format.Testing
TestJob_Statuses_Pagination_SharedModifyIndexpages a set of jobs that all share aModifyIndexand asserts every job is returned exactly once and the walk terminates; tokenizer unit tests cover numeric index ordering, the namespace/id tiebreaker, and the legacy bare-integer fallback.Scope question
I kept this PR deliberately narrow - just the
ModifyIndexcursor tiebreaker. While investigating I found a related, separate pagination bug inNamespaceIDTokenizer(two namespaces liketeamandteam-acan stall/duplicate), and there's an open question in #28167 about whether the four tokenizers in this file should be consolidated onto a shared helper. I'm happy to either keep this narrow and track the namespace bug + refactor separately, or expand scope - whatever you'd prefer. Flagging here so the decision is visible rather than baked in.AI usage
The investigation, the reproduction harness, the design, and the verification are mine: I reproduced all the symptoms on live clusters, ran the before/after A/B by hand (API walks and clicking through both UIs), and confirmed the results. The Go changes (the tokenizer tiebreaker and its tests) are my own work.
For the UI portion I did utilize an AI assistant for implementation help with the JavaScript - I almost never write JavaScript and generally try to avoid it so I always have to look up syntax and libraries, and best practices although this wasn't that involved and I could mostly crib off of the surrounding code/style, so I utilized a bit of claude for the
ui/pagination changes (opaque token + token history), while making sure it followed the prescribed design and fit in with the style of the existing code. I reviewed and understand those changes and reviewed every line of any suggestion I took from it and I stand behind the whole PR that has come out of analysis and testing the last couple days; I just want to be transparent about where the assistance was used. So please pay extra close attention to the JavaScript code because unlike Go I'm not working with JavaScript every day and am more likely to have made subtle mistakes there.