From 729421a1ee456e8ece4dc91f9b187db5788ce677 Mon Sep 17 00:00:00 2001
From: James Rasell <jrasell@hashicorp.com>
Date: Thu, 28 Sep 2023 08:48:44 +0100
Subject: [PATCH] updates based on review.

---
 nomad/plan_apply.go      | 33 +++++++++++++++++---------
 nomad/plan_apply_test.go | 50 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 11 deletions(-)

diff --git a/nomad/plan_apply.go b/nomad/plan_apply.go
index 4ba9c2e1dc17..95076e6fce88 100644
--- a/nomad/plan_apply.go
+++ b/nomad/plan_apply.go
@@ -808,22 +808,33 @@ func isValidForDisconnectedNode(plan *structs.Plan, nodeID string) bool {
 // ensure this, and will return an error when a duplicate is found.
 func evaluatePlanAllocIndexes(stateSnap *state.StateSnapshot, plan *structs.Plan) error {
 
+	switch plan.Job.Type {
+	case structs.JobTypeSystem, structs.JobTypeSysBatch:
+		// system: all allocation indexes are zero, so we cannot perform this
+		// check.
+		//
+		// sysbatch: allocation indexes are unique across each set of node
+		// allocations. They are not unique across a version of a job,
+		// therefore this will require new functionality and testing.
+		// TODO(jrasell): add this.
+		return nil
+	default:
+	}
+
+	// In the case there are no node allocations, the plan will only be
+	// destructive actions which will not result in new alloc indexes being
+	// generated. Therefore, exit early.
+	if len(plan.NodeAllocation) == 0 {
+		return nil
+	}
+
 	// This map will be used to track what allocation names we have. The
 	// allocation name is formed using the jobID, taskgroup name, and alloc
 	// index. We can therefore use this to identify alloc index duplicates and
 	// save the computational overhead of extracting the index from the name.
-	var allocIndexMap map[string]string
-
-	// If the plan includes node allocations, instantiate the set. In the case
-	// there are no node allocations, the plan will only be destructive actions
-	// which will not result in new alloc indexes being generated.
-	if len(plan.NodeAllocation) > 0 {
-		allocIndexMap = make(map[string]string)
-	} else {
-		return nil
-	}
+	allocIndexMap := make(map[string]string)
 
-	stateAllocs, err := stateSnap.AllocsByJob(nil, plan.Job.Namespace, plan.Job.ID, false)
+	stateAllocs, err := stateSnap.AllocsByJob(nil, plan.Job.Namespace, plan.Job.ID, true)
 	if err != nil {
 		return err
 	}
diff --git a/nomad/plan_apply_test.go b/nomad/plan_apply_test.go
index aafded99cd91..9959df43a9be 100644
--- a/nomad/plan_apply_test.go
+++ b/nomad/plan_apply_test.go
@@ -1559,4 +1559,54 @@ func Test_evaluatePlanAllocIndexes(t *testing.T) {
 			evaluatePlanAllocIndexes(testStateSnapshotFn(t, testState), &testPlan),
 			duplicateAllocIndexErrorString)
 	})
+
+	// All allocations within a job of type system have zero index. Therefore,
+	// this check is not run.
+	t.Run("system job", func(t *testing.T) {
+		testPlan := structs.Plan{
+			Job: &structs.Job{
+				ID:        testAllocs[0].JobID,
+				Namespace: testAllocs[0].Namespace,
+				Version:   0,
+				Type:      structs.JobTypeSystem,
+			},
+			NodeUpdate: nil,
+			NodeAllocation: map[string][]*structs.Allocation{
+				"8bdb21db-9445-3650-ca9d-0d7883cc8a73": {
+					{Name: "example.cache[0]", ID: "bd8cf1bc-5ded-5942-5b39-078878e74e15"},
+				},
+				"52b3508a-c88e-fc83-74c9-829d5ef1103a": {
+					{Name: "example.cache[0]", ID: "2a8243d9-c54b-ec78-8b51-e99ff3579d72"},
+				},
+			},
+		}
+		must.NoError(t, evaluatePlanAllocIndexes(testStateSnapshotFn(t, testState), &testPlan))
+	})
+
+	// Allocation indexes are unique across each set of node allocations. They
+	// are not unique across a version of a job, therefore this will require
+	// new functionality and testing. Once the additional functionality is
+	// added, this test should be removed/updated.
+	t.Run("sysbatch job", func(t *testing.T) {
+		testPlan := structs.Plan{
+			Job: &structs.Job{
+				ID:        testAllocs[0].JobID,
+				Namespace: testAllocs[0].Namespace,
+				Version:   0,
+				Type:      structs.JobTypeSysBatch,
+			},
+			NodeUpdate: nil,
+			NodeAllocation: map[string][]*structs.Allocation{
+				"8bdb21db-9445-3650-ca9d-0d7883cc8a73": {
+					{Name: "example.cache[0]", ID: "bd8cf1bc-5ded-5942-5b39-078878e74e15"},
+					{Name: "example.cache[1]", ID: "1097ce2b-f131-4065-9ed1-86795b738e7a"},
+				},
+				"52b3508a-c88e-fc83-74c9-829d5ef1103a": {
+					{Name: "example.cache[0]", ID: "2a8243d9-c54b-ec78-8b51-e99ff3579d72"},
+					{Name: "example.cache[1]", ID: "cc7460e0-a02d-47ef-9041-7b0e681f37fd"},
+				},
+			},
+		}
+		must.NoError(t, evaluatePlanAllocIndexes(testStateSnapshotFn(t, testState), &testPlan))
+	})
 }