core: reject plans which include or cause duplicate alloc indexes. #18376
Conversation
Nomad allocation indexes are detailed as being unique across all allocations within a single version of a job. This is not enforced at any level and users have reported seeing duplicate indexes used for allocations running the same job version. In order to ensure this cannot happen, the plan applier will now perform a check and reject any plans which include or would cause duplication of indexes.
| // Delete the alloc state entry, so it doesn't impact any subsequent | ||
| // tests. | ||
| must.NoError(t, testState.DeleteEval(30, []string{}, []string{canaryAlloc.ID}, false)) |
There was a problem hiding this comment.
Should this (and similar bits in these tests) happen in a t.Cleanup so that subsequent tests can run even if a test fails?
| // Perform a test where duplicates already exist within state. This is | ||
| // possible on all but fresh clusters. | ||
| t.Run("duplicate exist in state", func(t *testing.T) { |
There was a problem hiding this comment.
I'm really glad we have this test.
This also raises a question about what happens if we have existing duplicate allocations and the scheduler submits a plan that ignores them? For example, if I have alloc[0] and alloc[0] and then scale up the job, the existing allocs will be untouched. Does it become impossible to scale up/down and create that kind of plan for these jobs? (Maybe that's ok, if the user can then destroy one of the allocations out of band?)
| // ensure correct removal. This path is most common when a node goes down | ||
| // and a plan is generated to replace the failed allocations. |
There was a problem hiding this comment.
Or any time we reschedule, right? (ex. allocation failure)
tgross
dismissed
their stale review
probably shouldn't have approved; needs more discussion around livelock
|
From a slack discussion: I don't think we can take this approach. #10727 makes it appear like Nomad repeatedly selects overlapping alloc indexes over multiple deployments. While this could just be "bad luck," it could also be an indication that the scheduler is deterministically choosing overlapping indexes due to a bug. If so, we need to fix the bug in the scheduler before we add this check to the plan applier. If we add the plan applier check first we risk creating an unschedulable job which is far worse than overlapping indexes (which most users probably don't even notice). This is effectively a livelock for one scheduler where it keeps doing the same work over and over but is unable to make progress because this plan applier check would reject the buggy index selection. |
|
I am closing this PR out as the approach was deemed unsuitable to more forward with. I will link to this PR with any future PR that attempts to close the linked issue. |
Nomad allocation indexes are detailed as being unique across all allocations within a single version of a job. This is not enforced at any level and users have reported seeing duplicate indexes used for allocations running the same job version.
In order to ensure this cannot happen, the plan applier will now perform a check and reject any plans which include or would cause duplication of indexes.
closes #10727
Test trigger with count 100