diff --git a/nomad/structs/csi_test.go b/nomad/structs/csi_test.go index 9ef5a7f8b679..5da73a78b688 100644 --- a/nomad/structs/csi_test.go +++ b/nomad/structs/csi_test.go @@ -1002,7 +1002,7 @@ func TestDeleteNodeForType_Monolith_NilController(t *testing.T) { func TestDeleteNodeForType_Monolith_NilNode(t *testing.T) { ci.Parallel(t) - + plug := NewCSIPlugin("foo", 1000) plug.Nodes["n0"] = nil diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 325c07f76222..cdf1f76696da 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6384,7 +6384,7 @@ func (tg *TaskGroup) Validate(j *Job) error { canaries = tg.Update.Canary } for name, volReq := range tg.Volumes { - if err := volReq.Validate(canaries); err != nil { + if err := volReq.Validate(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 new file mode 100644 index 000000000000..170c5498f98d --- /dev/null +++ b/nomad/structs/volume_test.go @@ -0,0 +1,81 @@ +package structs + +import ( + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/stretchr/testify/require" +) + +func TestVolumeRequest_Validate(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + expected []string + canariesCount int + taskGroupCount int + req *VolumeRequest + }{ + { + name: "host volume with empty source", + expected: []string{"volume has an empty source"}, + req: &VolumeRequest{ + Type: VolumeTypeHost, + }, + }, + { + name: "host volume with CSI volume config", + expected: []string{ + "host volumes cannot have an access mode", + "host volumes cannot have an attachment mode", + "host volumes cannot have mount options", + "host volumes do not support per_alloc", + }, + req: &VolumeRequest{ + Type: VolumeTypeHost, + ReadOnly: false, + AccessMode: CSIVolumeAccessModeSingleNodeReader, + AttachmentMode: CSIVolumeAttachmentModeBlockDevice, + MountOptions: &CSIMountOptions{ + FSType: "ext4", + MountFlags: []string{"ro"}, + }, + PerAlloc: true, + }, + }, + { + name: "CSI volume bad single access mode", + expected: []string{ + "volume cannot have multi-node-single-writer access mode with count > 1 unless per_alloc = true", + "multi-node-single-writer volumes must not be read-only", + }, + taskGroupCount: 2, + req: &VolumeRequest{ + Type: VolumeTypeCSI, + AccessMode: CSIVolumeAccessModeMultiNodeSingleWriter, + ReadOnly: true, + }, + }, + { + name: "CSI volume per-alloc with canaries", + expected: []string{"volume cannot be per_alloc when canaries are in use"}, + canariesCount: 1, + req: &VolumeRequest{ + Type: VolumeTypeCSI, + PerAlloc: true, + }, + }, + } + + for _, tc := range testCases { + tc = tc + t.Run(tc.name, func(t *testing.T) { + err := tc.req.Validate(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 5fe5238732d3..3433deeb887b 100644 --- a/nomad/structs/volumes.go +++ b/nomad/structs/volumes.go @@ -102,54 +102,81 @@ type VolumeRequest struct { PerAlloc bool } -func (v *VolumeRequest) Validate(canaries int) error { +func (v *VolumeRequest) Validate(taskGroupCount, canaries int) error { if !(v.Type == VolumeTypeHost || v.Type == VolumeTypeCSI) { return fmt.Errorf("volume has unrecognized type %s", v.Type) } var mErr multierror.Error - if v.Type == VolumeTypeHost && v.AttachmentMode != CSIVolumeAttachmentModeUnknown { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("host volumes cannot have an attachment mode")) + addErr := func(msg string, args ...interface{}) { + mErr.Errors = append(mErr.Errors, fmt.Errorf(msg, args...)) } - if v.Type == VolumeTypeHost && v.AccessMode != CSIVolumeAccessModeUnknown { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("host volumes cannot have an access mode")) - } - if v.Type == VolumeTypeHost && v.MountOptions != nil { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("host volumes cannot have mount options")) - } - if v.Type == VolumeTypeCSI && v.AttachmentMode == CSIVolumeAttachmentModeUnknown { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("CSI volumes must have an attachment mode")) - } - if v.Type == VolumeTypeCSI && v.AccessMode == CSIVolumeAccessModeUnknown { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("CSI volumes must have an access mode")) + + if v.Source == "" { + addErr("volume has an empty source") } - if v.AccessMode == CSIVolumeAccessModeSingleNodeReader || v.AccessMode == CSIVolumeAccessModeMultiNodeReader { - if !v.ReadOnly { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("%s volumes must be read-only", v.AccessMode)) + switch v.Type { + + case VolumeTypeHost: + if v.AttachmentMode != CSIVolumeAttachmentModeUnknown { + addErr("host volumes cannot have an attachment mode") + } + if v.AccessMode != CSIVolumeAccessModeUnknown { + addErr("host volumes cannot have an access mode") + } + if v.MountOptions != nil { + addErr("host volumes cannot have mount options") + } + if v.PerAlloc { + addErr("host volumes do not support per_alloc") } - } - if v.AttachmentMode == CSIVolumeAttachmentModeBlockDevice && v.MountOptions != nil { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("block devices cannot have mount options")) - } + case VolumeTypeCSI: - if v.PerAlloc && canaries > 0 { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("volume cannot be per_alloc when canaries are in use")) - } + switch v.AttachmentMode { + case CSIVolumeAttachmentModeUnknown: + addErr("CSI volumes must have an attachment mode") + case CSIVolumeAttachmentModeBlockDevice: + if v.MountOptions != nil { + addErr("block devices cannot have mount options") + } + } + + switch v.AccessMode { + case CSIVolumeAccessModeUnknown: + addErr("CSI volumes must have an access mode") + case CSIVolumeAccessModeSingleNodeReader: + if !v.ReadOnly { + addErr("%s volumes must be read-only", v.AccessMode) + } + if taskGroupCount > 1 && !v.PerAlloc { + addErr("volume cannot have %s access mode with count > 1 unless per_alloc = true", v.AccessMode) + } + case CSIVolumeAccessModeSingleNodeWriter, CSIVolumeAccessModeMultiNodeSingleWriter: + if v.ReadOnly { + addErr("%s volumes must not be read-only", v.AccessMode) + } + if taskGroupCount > 1 && !v.PerAlloc { + addErr("volume cannot have %s access mode with count > 1 unless per_alloc = true", v.AccessMode) + } + case CSIVolumeAccessModeMultiNodeReader: + if !v.ReadOnly { + addErr("%s volumes must be read-only", v.AccessMode) + } + case CSIVolumeAccessModeMultiNodeMultiWriter: + if v.ReadOnly { + addErr("%s volumes must not be read-only", v.AccessMode) + } + } + + if v.PerAlloc && canaries > 0 { + addErr("volume cannot be per_alloc when canaries are in use") + } - if v.Source == "" { - mErr.Errors = append(mErr.Errors, fmt.Errorf("volume has an empty source")) } + return mErr.ErrorOrNil() }