From 06646253f62e6bb397ebeb91458c4f9dc8e0488d Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 12 Oct 2017 13:33:55 -0700 Subject: [PATCH 1/2] Fix sorting of job versions Fixes an issue in which the versions were improperly sorted which would cause pruning of the wrong job version. This essentially meant that job versions above 255 would be dropped from the job version table (note this was due to the prefix walk crossing from the 1-byte to 2-byte threshold). Fixes https://github.com/hashicorp/nomad/issues/3357 --- nomad/state/state_store.go | 9 +++++---- nomad/state/state_store_test.go | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 9056841d67d1..076b34104c04 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log" + "sort" "github.com/hashicorp/go-memdb" multierror "github.com/hashicorp/go-multierror" @@ -1017,10 +1018,10 @@ func (s *StateStore) jobVersionByID(txn *memdb.Txn, ws *memdb.WatchSet, namespac all = append(all, j) } - // Reverse so that highest versions first - for i, j := 0, len(all)-1; i < j; i, j = i+1, j-1 { - all[i], all[j] = all[j], all[i] - } + // Sort in reverse order so that the highest version is first + sort.Slice(all, func(i, j int) bool { + return all[i].Version > all[j].Version + }) return all, nil } diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 0d62ff78b7cb..dde4b2a96841 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -1226,7 +1226,7 @@ func TestStateStore_UpdateUpsertJob_JobVersion(t *testing.T) { // Create a job and mark it as stable job := mock.Job() job.Stable = true - job.Priority = 0 + job.Name = "0" // Create a watchset so we can test that upsert fires the watch ws := memdb.NewWatchSet() @@ -1244,10 +1244,10 @@ func TestStateStore_UpdateUpsertJob_JobVersion(t *testing.T) { } var finalJob *structs.Job - for i := 1; i < 20; i++ { + for i := 1; i < 300; i++ { finalJob = mock.Job() finalJob.ID = job.ID - finalJob.Priority = i + finalJob.Name = fmt.Sprintf("%d", i) err = state.UpsertJob(uint64(1000+i), finalJob) if err != nil { t.Fatalf("err: %v", err) @@ -1267,10 +1267,10 @@ func TestStateStore_UpdateUpsertJob_JobVersion(t *testing.T) { if out.CreateIndex != 1000 { t.Fatalf("bad: %#v", out) } - if out.ModifyIndex != 1019 { + if out.ModifyIndex != 1299 { t.Fatalf("bad: %#v", out) } - if out.Version != 19 { + if out.Version != 299 { t.Fatalf("bad: %#v", out) } @@ -1278,7 +1278,7 @@ func TestStateStore_UpdateUpsertJob_JobVersion(t *testing.T) { if err != nil { t.Fatalf("err: %v", err) } - if index != 1019 { + if index != 1299 { t.Fatalf("bad: %d", index) } @@ -1288,19 +1288,19 @@ func TestStateStore_UpdateUpsertJob_JobVersion(t *testing.T) { t.Fatalf("err: %v", err) } if len(allVersions) != structs.JobTrackedVersions { - t.Fatalf("got %d; want 1", len(allVersions)) + t.Fatalf("got %d; want %d", len(allVersions), structs.JobTrackedVersions) } - if a := allVersions[0]; a.ID != job.ID || a.Version != 19 || a.Priority != 19 { + if a := allVersions[0]; a.ID != job.ID || a.Version != 299 || a.Name != "299" { t.Fatalf("bad: %+v", a) } - if a := allVersions[1]; a.ID != job.ID || a.Version != 18 || a.Priority != 18 { + if a := allVersions[1]; a.ID != job.ID || a.Version != 298 || a.Name != "298" { t.Fatalf("bad: %+v", a) } // Ensure we didn't delete the stable job if a := allVersions[structs.JobTrackedVersions-1]; a.ID != job.ID || - a.Version != 0 || a.Priority != 0 || !a.Stable { + a.Version != 0 || a.Name != "0" || !a.Stable { t.Fatalf("bad: %+v", a) } From 3dd28105b96b3d9a6e91f28976bcf16245afa1a1 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Thu, 12 Oct 2017 13:36:46 -0700 Subject: [PATCH 2/2] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 638252a84d4d..8a3bd4e57096 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ IMPROVEMENTS: BUG FIXES: * core: Fix restoration of stopped periodic jobs [GH-3201] * core: Run deployment garbage collector on an interval [GH-3267] + * core: Fix issue in which job versions above a threshold potentially wouldn't + be stored [GH-3372] * core: Fix issue where node-drain with complete batch allocation would create replacement [GH-3217] * core: Fix issue in which batch allocations from previous job versions may not