From 38e1ba1174e436d2529dd06129103730f2bd37cf Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 3 Feb 2023 08:39:48 -0500 Subject: [PATCH] System and sysbatch jobs always have zero index (#16030) (#16041) Service jobs should have unique allocation Names, derived from the Job.ID. System jobs do not have unique allocation Names because the index is intended to indicated the instance out of a desired count size. Because system jobs do not have an explicit count but the results are based on the targeted nodes, the index is less informative and this was intentionally omitted from the original design. Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for system/sysbatch jobs Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs. System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So interpolation via `per_alloc` will not work as soon as there's more than one allocation placed. Validate against this on job submission. --- .changelog/16030.txt | 3 ++ nomad/structs/structs.go | 2 +- nomad/structs/volume_test.go | 9 ++-- nomad/structs/volumes.go | 12 +++-- scheduler/generic_sched_test.go | 11 +++++ scheduler/scheduler_sysbatch_test.go | 11 +++++ scheduler/scheduler_system_test.go | 10 ++++ .../content/docs/job-specification/volume.mdx | 3 ++ website/content/partials/envvars.mdx | 48 +++++++++---------- 9 files changed, 77 insertions(+), 32 deletions(-) create mode 100644 .changelog/16030.txt diff --git a/.changelog/16030.txt b/.changelog/16030.txt new file mode 100644 index 000000000000..4649a508c778 --- /dev/null +++ b/.changelog/16030.txt @@ -0,0 +1,3 @@ +```release-note:bug +volumes: Fixed a bug where `per_alloc` was allowed for volume blocks on system and sysbatch jobs, which do not have an allocation index +``` diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4dd762a7172d..88998dc54e41 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6510,7 +6510,7 @@ func (tg *TaskGroup) Validate(j *Job) error { canaries = tg.Update.Canary } for name, volReq := range tg.Volumes { - if err := volReq.Validate(tg.Count, canaries); err != nil { + if err := volReq.Validate(j.Type, tg.Count, canaries); err != nil { mErr.Errors = append(mErr.Errors, fmt.Errorf( "Task group volume validation for %s failed: %v", name, err)) } diff --git a/nomad/structs/volume_test.go b/nomad/structs/volume_test.go index cbe9ed6fe8ba..6f7ca4657c38 100644 --- a/nomad/structs/volume_test.go +++ b/nomad/structs/volume_test.go @@ -68,8 +68,11 @@ func TestVolumeRequest_Validate(t *testing.T) { }, }, { - name: "CSI volume per-alloc with canaries", - expected: []string{"volume cannot be per_alloc when canaries are in use"}, + name: "CSI volume per-alloc with canaries", + expected: []string{ + "volume cannot be per_alloc for system or sysbatch jobs", + "volume cannot be per_alloc when canaries are in use", + }, canariesCount: 1, req: &VolumeRequest{ Type: VolumeTypeCSI, @@ -81,7 +84,7 @@ func TestVolumeRequest_Validate(t *testing.T) { for _, tc := range testCases { tc = tc t.Run(tc.name, func(t *testing.T) { - err := tc.req.Validate(tc.taskGroupCount, tc.canariesCount) + err := tc.req.Validate(JobTypeSystem, tc.taskGroupCount, tc.canariesCount) for _, expected := range tc.expected { require.Contains(t, err.Error(), expected) } diff --git a/nomad/structs/volumes.go b/nomad/structs/volumes.go index 0f8b040de458..0b16f9b3e7ca 100644 --- a/nomad/structs/volumes.go +++ b/nomad/structs/volumes.go @@ -102,7 +102,7 @@ type VolumeRequest struct { PerAlloc bool } -func (v *VolumeRequest) Validate(taskGroupCount, canaries int) error { +func (v *VolumeRequest) Validate(jobType string, taskGroupCount, canaries int) error { if !(v.Type == VolumeTypeHost || v.Type == VolumeTypeCSI) { return fmt.Errorf("volume has unrecognized type %s", v.Type) @@ -170,9 +170,13 @@ func (v *VolumeRequest) Validate(taskGroupCount, canaries int) error { case CSIVolumeAccessModeMultiNodeMultiWriter: // note: we intentionally allow read-only mount of this mode } - - if v.PerAlloc && canaries > 0 { - addErr("volume cannot be per_alloc when canaries are in use") + if v.PerAlloc { + if jobType == JobTypeSystem || jobType == JobTypeSysBatch { + addErr("volume cannot be per_alloc for system or sysbatch jobs") + } + if canaries > 0 { + addErr("volume cannot be per_alloc when canaries are in use") + } } } diff --git a/scheduler/generic_sched_test.go b/scheduler/generic_sched_test.go index 4b99f9e752dd..ae38c92af986 100644 --- a/scheduler/generic_sched_test.go +++ b/scheduler/generic_sched_test.go @@ -9,11 +9,13 @@ import ( memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/exp/slices" @@ -91,6 +93,15 @@ func TestServiceSched_JobRegister(t *testing.T) { t.Fatalf("bad: %#v", out) } + // Ensure allocations have unique names derived from Job.ID + allocNames := helper.ConvertSlice(out, + func(alloc *structs.Allocation) string { return alloc.Name }) + expectAllocNames := []string{} + for i := 0; i < 10; i++ { + expectAllocNames = append(expectAllocNames, fmt.Sprintf("%s.web[%d]", job.ID, i)) + } + must.SliceContainsAll(t, expectAllocNames, allocNames) + // Ensure different ports were used. used := make(map[int]map[string]struct{}) for _, alloc := range out { diff --git a/scheduler/scheduler_sysbatch_test.go b/scheduler/scheduler_sysbatch_test.go index fac543699491..49ddbfc574d8 100644 --- a/scheduler/scheduler_sysbatch_test.go +++ b/scheduler/scheduler_sysbatch_test.go @@ -7,11 +7,13 @@ import ( "github.com/hashicorp/go-memdb" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/kr/pretty" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -64,6 +66,15 @@ func TestSysBatch_JobRegister(t *testing.T) { // Ensure all allocations placed require.Len(t, out, 10) + // Note that all sysbatch allocations have the same name derived from Job.Name + allocNames := helper.ConvertSlice(out, + func(alloc *structs.Allocation) string { return alloc.Name }) + expectAllocNames := []string{} + for i := 0; i < 10; i++ { + expectAllocNames = append(expectAllocNames, fmt.Sprintf("%s.pinger[0]", job.Name)) + } + must.SliceContainsAll(t, expectAllocNames, allocNames) + // Check the available nodes count, ok := out[0].Metrics.NodesAvailable["dc1"] require.True(t, ok) diff --git a/scheduler/scheduler_system_test.go b/scheduler/scheduler_system_test.go index 05a4ec6a2c62..40f83ceb9770 100644 --- a/scheduler/scheduler_system_test.go +++ b/scheduler/scheduler_system_test.go @@ -9,6 +9,7 @@ import ( memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/mock" @@ -66,6 +67,15 @@ func TestSystemSched_JobRegister(t *testing.T) { // Ensure all allocations placed require.Len(t, out, 10) + // Note that all system allocations have the same name derived from Job.Name + allocNames := helper.ConvertSlice(out, + func(alloc *structs.Allocation) string { return alloc.Name }) + expectAllocNames := []string{} + for i := 0; i < 10; i++ { + expectAllocNames = append(expectAllocNames, fmt.Sprintf("%s.web[0]", job.Name)) + } + must.SliceContainsAll(t, expectAllocNames, allocNames) + // Check the available nodes count, ok := out[0].Metrics.NodesAvailable["dc1"] require.True(t, ok) diff --git a/website/content/docs/job-specification/volume.mdx b/website/content/docs/job-specification/volume.mdx index 0b07be31eabf..c4aaa44e2452 100644 --- a/website/content/docs/job-specification/volume.mdx +++ b/website/content/docs/job-specification/volume.mdx @@ -99,6 +99,9 @@ The following fields are only valid for volumes with `type = "csi"`: = true`, the allocation named `myjob.mygroup.mytask[0]` will require a volume ID `myvolume[0]`. + The `per_alloc` field cannot be true for system jobs, sysbatch jobs, or jobs + that use canaries. + - `mount_options` - Options for mounting CSI volumes that have the `file-system` [attachment mode]. These options override the `mount_options` field from [volume registration]. Consult the documentation for your storage diff --git a/website/content/partials/envvars.mdx b/website/content/partials/envvars.mdx index 64789d7c6c66..1659892dfb16 100644 --- a/website/content/partials/envvars.mdx +++ b/website/content/partials/envvars.mdx @@ -1,29 +1,29 @@ ### Job-related variables -| Variable | Description | -| ------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `NOMAD_ALLOC_DIR` | The path to the shared `alloc/` directory. See [here](/docs/runtime/environment#task-directories) for more information. | -| `NOMAD_TASK_DIR` | The path to the task `local/` directory. See [here](/docs/runtime/environment#task-directories) for more information. | -| `NOMAD_SECRETS_DIR` | Path to the task's secrets directory. See [here](/docs/runtime/environment#task-directories) for more information. | -| `NOMAD_MEMORY_LIMIT` | Memory limit in MB for the task | -| `NOMAD_MEMORY_MAX_LIMIT` | The maximum memory limit the task may use if client has excess memory capacity, in MB. Omitted if task isn't configured with memory oversubscription. | -| `NOMAD_CPU_LIMIT` | CPU limit in MHz for the task | -| `NOMAD_CPU_CORES` | The specific CPU cores reserved for the task in cpuset list notation. Omitted if the task does not request cpu cores. E.g. `0-2,7,12-14` | -| `NOMAD_ALLOC_ID` | Allocation ID of the task | -| `NOMAD_SHORT_ALLOC_ID` | The first 8 characters of the allocation ID of the task | -| `NOMAD_ALLOC_NAME` | Allocation name of the task | -| `NOMAD_ALLOC_INDEX` | Allocation index; useful to distinguish instances of task groups. From 0 to (count - 1). The index is unique within a given version of a job, but canaries or failed tasks in a deployment may reuse the index. | -| `NOMAD_TASK_NAME` | Task's name | -| `NOMAD_GROUP_NAME` | Group's name | -| `NOMAD_JOB_ID` | Job's ID, which is equal to the Job name when submitted through CLI but can be different when using the API | -| `NOMAD_JOB_NAME` | Job's name | -| `NOMAD_JOB_PARENT_ID` | ID of the Job's parent if it has one | -| `NOMAD_DC` | Datacenter in which the allocation is running | -| `NOMAD_PARENT_CGROUP` | The parent cgroup used to contain task cgroups (Linux only) | -| `NOMAD_NAMESPACE` | Namespace in which the allocation is running | -| `NOMAD_REGION` | Region in which the allocation is running | -| `NOMAD_META_` | The metadata value given by `key` on the task's metadata. Note that this is different from [`${meta.}`](/docs/runtime/interpolation#node-variables-) which are keys in the node's metadata. | -| `VAULT_TOKEN` | The task's Vault token. See [Vault Integration](/docs/integrations/vault-integration) for more details | +| Variable | Description | +|--------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `NOMAD_ALLOC_DIR` | The path to the shared `alloc/` directory. See [here](/nomad/docs/runtime/environment#task-directories) for more information. | +| `NOMAD_TASK_DIR` | The path to the task `local/` directory. See [here](/nomad/docs/runtime/environment#task-directories) for more information. | +| `NOMAD_SECRETS_DIR` | Path to the task's secrets directory. See [here](/nomad/docs/runtime/environment#task-directories) for more information. | +| `NOMAD_MEMORY_LIMIT` | Memory limit in MB for the task | +| `NOMAD_MEMORY_MAX_LIMIT` | The maximum memory limit the task may use if client has excess memory capacity, in MB. Omitted if task isn't configured with memory oversubscription. | +| `NOMAD_CPU_LIMIT` | CPU limit in MHz for the task | +| `NOMAD_CPU_CORES` | The specific CPU cores reserved for the task in cpuset list notation. Omitted if the task does not request cpu cores. E.g. `0-2,7,12-14` | +| `NOMAD_ALLOC_ID` | Allocation ID of the task | +| `NOMAD_SHORT_ALLOC_ID` | The first 8 characters of the allocation ID of the task | +| `NOMAD_ALLOC_NAME` | Allocation name of the task. This is derived from the job name, task group name, and allocation index. | +| `NOMAD_ALLOC_INDEX` | Allocation index; useful to distinguish instances of task groups. From 0 to (count - 1). For system jobs and sysbatch jobs, this value will always be 0. The index is unique within a given version of a job, but canaries or failed tasks in a deployment may reuse the index. | +| `NOMAD_TASK_NAME` | Task's name | +| `NOMAD_GROUP_NAME` | Group's name | +| `NOMAD_JOB_ID` | Job's ID, which is equal to the Job name when submitted through CLI but can be different when using the API | +| `NOMAD_JOB_NAME` | Job's name | +| `NOMAD_JOB_PARENT_ID` | ID of the Job's parent if it has one | +| `NOMAD_DC` | Datacenter in which the allocation is running | +| `NOMAD_PARENT_CGROUP` | The parent cgroup used to contain task cgroups (Linux only) | +| `NOMAD_NAMESPACE` | Namespace in which the allocation is running | +| `NOMAD_REGION` | Region in which the allocation is running | +| `NOMAD_META_` | The metadata value given by `key` on the task's metadata. Note that this is different from [`${meta.}`](/nomad/docs/runtime/interpolation#node-variables-) which are keys in the node's metadata. | +| `VAULT_TOKEN` | The task's Vault token. See [Vault Integration](/nomad/docs/integrations/vault-integration) for more details | ### Network-related Variables