state: make jobs modify_index index non-unique - #28158
Conversation
|
glad I went back and read @gulducat 's original PR for this....made me realize that I missed something and while this is still an improvement, it exposes a previously hidden pagination issue on the jobs page. ModifyIndexTokenizer builds its cursor from ModifyIndex alone, with no unique identifier involved (since previously the ModifyIndex WAS the unique identifier) — fine when the index was unique, but now jobs sharing a ModifyIndex can span a page boundary so jobs at the cursor boundary get returned on two pages, and if more jobs share a ModifyIndex than fit in one page, the cursor never advances past that group, so a paginating client can't reach anything older. I updated my a/b reproduction harness to also show what pagination does with the change in this PR: output (condensed since the gh issue has the full output there prior to this pagination update): The fix for that is slightly more involved so I haven't pushed anything for that to keep this PR narrow and a follow-up gh issue could be filed. I'm starting to look into it because it is fun, but I'll leave just the narrowly scoped PR for now. |
gulducat
left a comment
There was a problem hiding this comment.
Thank you @afreidah!
This is really great work: enthusiastic and shrewd troubleshooting with some history-spelunking and clear communication. I would be pleased as punch to receive more from you on the extended full-page-spanning singular-ModifyIndex issue, if you have time and interest to pursue it. Just file an issue if a PR will take a bit of time to put together, to be sure we don't lose track of it!
As is often the case, a lot of effort went into a simple one-line fix, nice and easy to evaluate. I only have the one nit-picky ask to move the test into the state_store_test hive, then we'll get this merged and backported.
There was a problem hiding this comment.
As much as I hate to say "go add more code to a 10,000 line file", state_store_test.go currently houses all the tests for methods like this one, so I think we should put this little fella with its family.
There's a chunk of them called TestStateStore_JobsBy*; maybe put this one after those, under TestStateStore_JobsByGC.
There was a problem hiding this comment.
awesome! I moved the test into that file and re-pushed.
I'll drop a gh issue for the extended full-page-spanning singular-ModifyIndex issue that I have started looking into as well.
Thanks!
The jobs-table "modify_index" memdb index was declared Unique:true, but ModifyIndex is not unique across jobs: when a single Raft transaction writes several jobs (for example, rescheduling allocations after a node goes down), those jobs share a ModifyIndex. A unique index can only hold one object per key, so colliding jobs were silently dropped from any query iterating this index -- notably Job.Statuses, which backs the /v1/jobs/statuses endpoint and the UI jobs page -- while remaining visible via the unique "id" index used by /v1/jobs and the CLI. Mark the index non-unique and add a regression test asserting that all jobs sharing a ModifyIndex are returned by JobsByModifyIndex. Fixes hashicorp#28132
9b1b8f2 to
c32cb6f
Compare
|
@gulducat not sure if those two tests are regularly flaky but I'm pretty sure it isn't anything related to this PR. Looks like they are failing in other CI runs too, but if there is anything I need to do on my end just let me know! |
gulducat
left a comment
There was a problem hiding this comment.
Looks great, thanks!
I verified that the failing tests were unrelated; the fix has been merged, and I'll make sure this commit lands happy in main before backporting :)
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. 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. On the web UI, treat the page token as opaque: navigate with a history stack instead of doing arithmetic on the cursor, and only synthesize a cursor for the "last" page. Fixes hashicorp#28167
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. 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. On the web UI, treat the page token as opaque: navigate with a history stack instead of doing arithmetic on the cursor, and only synthesize a cursor for the "last" page. Fixes hashicorp#28167
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. 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. On the web UI, widen the page cursor from a bare ModifyIndex to the full ModifyIndex+Namespace+ID token so the prev and last buttons point at a single job; the pagination flow is otherwise unchanged. Fixes hashicorp#28167
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. 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. On the web UI, widen the page cursor from a bare ModifyIndex to the full ModifyIndex+Namespace+ID token so the prev and last buttons point at a single job; the pagination flow is otherwise unchanged. Fixes hashicorp#28167
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. 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. On the web UI, widen the page cursor from a bare ModifyIndex to the full ModifyIndex+Namespace+ID token so the prev and last buttons point at a single job; the pagination flow is otherwise unchanged. Fixes hashicorp#28167
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
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
What
The jobs-table
modify_indexmemdb index was declaredUnique: true, butModifyIndexis not unique across jobs. When a single Raft transaction writes several jobs at once (e.g. rescheduling allocations after a node goes down), those jobs share aModifyIndex. A unique memdb index can only hold one object per key, so colliding jobs were silently dropped from any query that iterates this index — notablyJob.Statuses, which backs the/v1/jobs/statusesendpoint and the UI jobs page — while remaining visible via the uniqueidindex used by/v1/jobsand the CLI.This change marks the index non-unique and adds a regression test.
Fixes #28132.
Reproduction (on a live cluster)
Jobs sharing a
ModifyIndex(one Raft txn) vanish fromstatusesbut not/v1/jobs, even on a single unpaginated page:The 8 missing jobs are exactly the ones sharing
ModifyIndex2020598 (7 jobs) and 2019281 (1 of the pair).A self-contained Docker A/B harness that reproduces this and verifies the fix (1 server + 1 client, no Consul) is here: https://github.com/afreidah/nomad/tree/repro-jobs-statuses-28132/repro-28132
Root cause
nomad/state/schema.go,jobTableSchema()— themodify_indexindex usedUnique: trueon a non-unique field.Job.Statusesiterates it viaJobsByModifyIndex(getSorted(txn, sort, "jobs", "modify_index"));/v1/jobsiterates the uniqueidindex and is unaffected. The index was bornUnique: truein #20130 (May 2024) and never changed, so this has been latent since the statuses endpoint shipped.Testing
TestStateStore_JobsByModifyIndex_SharedModifyIndexasserts every job sharing aModifyIndexis returned. It fails before the fix (only the last writer survives) and passes after:AI usage
I found this on my own cluster and personally dug through my logs/metrics/etc to try and figure it out and used an AI assistant to help dig through the state/paginator source to quickly find the relevant parts of the code I wanted to look at based on my analysis and theories until I was confident I had reached the root cause after chasing a few red herrings and had what I needed to setup the reproduction + test to prove it existed and what condition would cause it. I Manually verified the issue against my live v2.0.3 cluster with nomad cli and curl commands because the issue is actively persisting until I re-deploy those jobs. No AI assistance is relevant in the actual go source changes since the entire fix is flipping a bool on one line. I 100% stand behind everything in this PR as the result of personal analysis and debugging on my real homelab cluster.