Skip to content

Commit

Permalink
refactor: volume request modes to be generic between DHV/CSI
Browse files Browse the repository at this point in the history 
When we implemented CSI, the types of the fields for access mode and attachment
mode on volume requests were defined with a prefix "CSI". This gets confusing
now that we have dynamic host volumes using the same fields. Fortunately the
original was a typedef on string, and the Go API in the `api` package just uses
strings directly, so we can change the name of the type without breaking
backwards compatibility for the msgpack wire format.

Update the names to `VolumeAccessMode` and `VolumeAttachmentMode`. Keep the CSI
and DHV specific value constant names for these fields (they aren't currently
1:1), so that we can easily differentiate in a given bit of code which values
are valid.

Ref: #24881 (comment)
  • Loading branch information
@tgross
tgross committed Jan 17, 2025
1 parent 44d9c2a commit b0a7f9a
Show file tree
Hide file tree
Showing 12 changed files with 65 additions and 69 deletions.
12 changes: 6 additions & 6 deletions client/csi_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func TestCSIController_AttachVolume(t *testing.T) {
VolumeID: "1234-4321-1234-4321",
ClientCSINodeID: "abcde",
AttachmentMode: nstructs.CSIVolumeAttachmentModeFilesystem,
AccessMode: nstructs.CSIVolumeAccessMode("foo"),
AccessMode: nstructs.VolumeAccessMode("foo"),
},
ExpectedErr: errors.New("CSI.ControllerAttachVolume: unknown volume access mode: foo"),
},
Expand All @@ -93,7 +93,7 @@ func TestCSIController_AttachVolume(t *testing.T) {
VolumeID: "1234-4321-1234-4321",
ClientCSINodeID: "abcde",
AccessMode: nstructs.CSIVolumeAccessModeMultiNodeReader,
AttachmentMode: nstructs.CSIVolumeAttachmentMode("bar"),
AttachmentMode: nstructs.VolumeAttachmentMode("bar"),
},
ExpectedErr: errors.New("CSI.ControllerAttachVolume: unknown volume attachment mode: bar"),
},
Expand Down Expand Up @@ -217,7 +217,7 @@ func TestCSIController_ValidateVolume(t *testing.T) {
},
VolumeID: "1234-4321-1234-4321",
VolumeCapabilities: []*nstructs.CSIVolumeCapability{{
AttachmentMode: nstructs.CSIVolumeAttachmentMode("bar"),
AttachmentMode: nstructs.VolumeAttachmentMode("bar"),
AccessMode: nstructs.CSIVolumeAccessModeMultiNodeReader,
}},
},
Expand All @@ -232,7 +232,7 @@ func TestCSIController_ValidateVolume(t *testing.T) {
VolumeID: "1234-4321-1234-4321",
VolumeCapabilities: []*nstructs.CSIVolumeCapability{{
AttachmentMode: nstructs.CSIVolumeAttachmentModeFilesystem,
AccessMode: nstructs.CSIVolumeAccessMode("foo"),
AccessMode: nstructs.VolumeAccessMode("foo"),
}},
},
ExpectedErr: errors.New("CSI.ControllerValidateVolume: unknown volume access mode: foo"),
Expand Down Expand Up @@ -395,7 +395,7 @@ func TestCSIController_CreateVolume(t *testing.T) {
VolumeCapabilities: []*nstructs.CSIVolumeCapability{
{
AttachmentMode: nstructs.CSIVolumeAttachmentModeFilesystem,
AccessMode: nstructs.CSIVolumeAccessMode("foo"),
AccessMode: nstructs.VolumeAccessMode("foo"),
},
},
},
Expand All @@ -411,7 +411,7 @@ func TestCSIController_CreateVolume(t *testing.T) {
VolumeCapabilities: []*nstructs.CSIVolumeCapability{
{
AccessMode: nstructs.CSIVolumeAccessModeMultiNodeReader,
AttachmentMode: nstructs.CSIVolumeAttachmentMode("bar"),
AttachmentMode: nstructs.VolumeAttachmentMode("bar"),
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions client/pluginmanager/csimanager/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ func (mi *MountInfo) Copy() *MountInfo {

type UsageOptions struct {
ReadOnly bool
AttachmentMode structs.CSIVolumeAttachmentMode
AccessMode structs.CSIVolumeAccessMode
AttachmentMode structs.VolumeAttachmentMode
AccessMode structs.VolumeAccessMode
MountOptions *structs.CSIMountOptions
}

Expand Down
12 changes: 6 additions & 6 deletions client/structs/csi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ type ClientCSIControllerValidateVolumeRequest struct {

// COMPAT(1.1.1): the AttachmentMode and AccessMode fields are deprecated
// and replaced by the VolumeCapabilities field above
AttachmentMode structs.CSIVolumeAttachmentMode
AccessMode structs.CSIVolumeAccessMode
AttachmentMode structs.VolumeAttachmentMode
AccessMode structs.VolumeAccessMode

// Parameters as returned by storage provider in CreateVolumeResponse.
// This field is optional.
Expand Down Expand Up @@ -117,10 +117,10 @@ type ClientCSIControllerAttachVolumeRequest struct {

// AttachmentMode indicates how the volume should be attached and mounted into
// a task.
AttachmentMode structs.CSIVolumeAttachmentMode
AttachmentMode structs.VolumeAttachmentMode

// AccessMode indicates the desired concurrent access model for the volume
AccessMode structs.CSIVolumeAccessMode
AccessMode structs.VolumeAccessMode

// MountOptions is an optional field that contains additional configuration
// when providing an AttachmentMode of CSIVolumeAttachmentModeFilesystem
Expand Down Expand Up @@ -449,8 +449,8 @@ type ClientCSINodeDetachVolumeRequest struct {

// These fields should match the original volume request so that
// we can find the mount points on the client
AttachmentMode structs.CSIVolumeAttachmentMode
AccessMode structs.CSIVolumeAccessMode
AttachmentMode structs.VolumeAttachmentMode
AccessMode structs.VolumeAccessMode
ReadOnly bool
}

Expand Down
4 changes: 2 additions & 2 deletions command/agent/job_endpoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1336,8 +1336,8 @@ func ApiTgToStructsTG(job *structs.Job, taskGroup *api.TaskGroup, tg *structs.Ta
ReadOnly: v.ReadOnly,
Sticky: v.Sticky,
Source: v.Source,
AttachmentMode: structs.CSIVolumeAttachmentMode(v.AttachmentMode),
AccessMode: structs.CSIVolumeAccessMode(v.AccessMode),
AttachmentMode: structs.VolumeAttachmentMode(v.AttachmentMode),
AccessMode: structs.VolumeAccessMode(v.AccessMode),
PerAlloc: v.PerAlloc,
}

Expand Down
46 changes: 23 additions & 23 deletions nomad/structs/csi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,33 +117,33 @@ func (t *TaskCSIPluginConfig) Copy() *TaskCSIPluginConfig {
// CSIVolumeCapability is the requested attachment and access mode for a
// volume
type CSIVolumeCapability struct {
AttachmentMode CSIVolumeAttachmentMode
AccessMode CSIVolumeAccessMode
AttachmentMode VolumeAttachmentMode
AccessMode VolumeAccessMode
}

// CSIVolumeAttachmentMode chooses the type of storage api that will be used to
// VolumeAttachmentMode chooses the type of storage api that will be used to
// interact with the device.
type CSIVolumeAttachmentMode string
type VolumeAttachmentMode string

const (
CSIVolumeAttachmentModeUnknown CSIVolumeAttachmentMode = ""
CSIVolumeAttachmentModeBlockDevice CSIVolumeAttachmentMode = "block-device"
CSIVolumeAttachmentModeFilesystem CSIVolumeAttachmentMode = "file-system"
CSIVolumeAttachmentModeUnknown VolumeAttachmentMode = ""
CSIVolumeAttachmentModeBlockDevice VolumeAttachmentMode = "block-device"
CSIVolumeAttachmentModeFilesystem VolumeAttachmentMode = "file-system"
)

// CSIVolumeAccessMode indicates how a volume should be used in a storage topology
// VolumeAccessMode indicates how a volume should be used in a storage topology
// e.g whether the provider should make the volume available concurrently.
type CSIVolumeAccessMode string
type VolumeAccessMode string

const (
CSIVolumeAccessModeUnknown CSIVolumeAccessMode = ""
CSIVolumeAccessModeUnknown VolumeAccessMode = ""

CSIVolumeAccessModeSingleNodeReader CSIVolumeAccessMode = "single-node-reader-only"
CSIVolumeAccessModeSingleNodeWriter CSIVolumeAccessMode = "single-node-writer"
CSIVolumeAccessModeSingleNodeReader VolumeAccessMode = "single-node-reader-only"
CSIVolumeAccessModeSingleNodeWriter VolumeAccessMode = "single-node-writer"

CSIVolumeAccessModeMultiNodeReader CSIVolumeAccessMode = "multi-node-reader-only"
CSIVolumeAccessModeMultiNodeSingleWriter CSIVolumeAccessMode = "multi-node-single-writer"
CSIVolumeAccessModeMultiNodeMultiWriter CSIVolumeAccessMode = "multi-node-multi-writer"
CSIVolumeAccessModeMultiNodeReader VolumeAccessMode = "multi-node-reader-only"
CSIVolumeAccessModeMultiNodeSingleWriter VolumeAccessMode = "multi-node-single-writer"
CSIVolumeAccessModeMultiNodeMultiWriter VolumeAccessMode = "multi-node-multi-writer"
)

// CSIMountOptions contain optional additional configuration that can be used
Expand Down Expand Up @@ -238,8 +238,8 @@ type CSIVolumeClaim struct {
NodeID string
ExternalNodeID string
Mode CSIVolumeClaimMode
AccessMode CSIVolumeAccessMode
AttachmentMode CSIVolumeAttachmentMode
AccessMode VolumeAccessMode
AttachmentMode VolumeAttachmentMode
State CSIVolumeClaimState
}

Expand Down Expand Up @@ -273,8 +273,8 @@ type CSIVolume struct {
// could support. This value cannot be set by the user.
Topologies []*CSITopology

AccessMode CSIVolumeAccessMode // *current* access mode
AttachmentMode CSIVolumeAttachmentMode // *current* attachment mode
AccessMode VolumeAccessMode // *current* access mode
AttachmentMode VolumeAttachmentMode // *current* attachment mode
MountOptions *CSIMountOptions

Secrets CSISecrets
Expand Down Expand Up @@ -352,8 +352,8 @@ type CSIVolListStub struct {
Name string
ExternalID string
Topologies []*CSITopology
AccessMode CSIVolumeAccessMode
AttachmentMode CSIVolumeAttachmentMode
AccessMode VolumeAccessMode
AttachmentMode VolumeAttachmentMode
CurrentReaders int
CurrentWriters int
Schedulable bool
Expand Down Expand Up @@ -929,8 +929,8 @@ type CSIVolumeClaimRequest struct {
NodeID string
ExternalNodeID string
Claim CSIVolumeClaimMode
AccessMode CSIVolumeAccessMode
AttachmentMode CSIVolumeAttachmentMode
AccessMode VolumeAccessMode
AttachmentMode VolumeAttachmentMode
State CSIVolumeClaimState
Timestamp int64 // UnixNano
WriteRequest
Expand Down
2 changes: 1 addition & 1 deletion nomad/structs/funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func AllocsFit(node *Node, allocs []*Allocation, netIdx *NetworkIndex, checkDevi
for _, volReq := range group.Volumes {
hostVolumeClaims[volReq.Source]++
if volReq.AccessMode ==
CSIVolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter) {
VolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter) {

Check failure on line 176 in nomad/structs/funcs.go

View workflow job for this annotation

GitHub Actions / checks / checks 

unnecessary conversion (unconvert)
exclusiveHostVolumeClaims = append(exclusiveHostVolumeClaims, volReq.Source)
}
}
Expand Down
4 changes: 2 additions & 2 deletions nomad/structs/funcs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ func TestAllocsFit_ExclusiveVolumes(t *testing.T) {
Job: &Job{TaskGroups: []*TaskGroup{{Name: "group", Volumes: map[string]*VolumeRequest{
"foo": {
Source: "example",
AccessMode: CSIVolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter),
AccessMode: HostVolumeAccessModeSingleNodeSingleWriter,
},
}}}},
AllocatedResources: &AllocatedResources{
Expand All @@ -566,7 +566,7 @@ func TestAllocsFit_ExclusiveVolumes(t *testing.T) {
Cpu: AllocatedCpuResources{CpuShares: 500},
Memory: AllocatedMemoryResources{MemoryMB: 500},
}
a2.Job.TaskGroups[0].Volumes["foo"].AccessMode = CSIVolumeAccessModeMultiNodeReader
a2.Job.TaskGroups[0].Volumes["foo"].AccessMode = HostVolumeAccessModeSingleNodeMultiWriter

// Should fit one allocation
fit, _, _, err := AllocsFit(n, []*Allocation{a1}, nil, true)
Expand Down
28 changes: 12 additions & 16 deletions nomad/structs/host_volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,8 @@ func (hv *HostVolume) GetID() string {

// HostVolumeCapability is the requested attachment and access mode for a volume
type HostVolumeCapability struct {
AttachmentMode HostVolumeAttachmentMode
AccessMode HostVolumeAccessMode
AttachmentMode VolumeAttachmentMode
AccessMode VolumeAccessMode
}

func (hvc *HostVolumeCapability) Copy() *HostVolumeCapability {
Expand Down Expand Up @@ -326,27 +326,23 @@ func (hvc *HostVolumeCapability) Validate() error {
return nil
}

// HostVolumeAttachmentMode chooses the type of storage API that will be used to
// HostVolumeAttachmentModes choose the type of storage API that will be used to
// interact with the device.
type HostVolumeAttachmentMode string

const (
HostVolumeAttachmentModeUnknown HostVolumeAttachmentMode = ""
HostVolumeAttachmentModeBlockDevice HostVolumeAttachmentMode = "block-device"
HostVolumeAttachmentModeFilesystem HostVolumeAttachmentMode = "file-system"
HostVolumeAttachmentModeUnknown VolumeAttachmentMode = ""
HostVolumeAttachmentModeBlockDevice VolumeAttachmentMode = "block-device"
HostVolumeAttachmentModeFilesystem VolumeAttachmentMode = "file-system"
)

// HostVolumeAccessMode indicates how Nomad should make the volume available to
// HostVolumeAccessModes indicate how Nomad should make the volume available to
// concurrent allocations.
type HostVolumeAccessMode string

const (
HostVolumeAccessModeUnknown HostVolumeAccessMode = ""
HostVolumeAccessModeUnknown VolumeAccessMode = ""

HostVolumeAccessModeSingleNodeReader HostVolumeAccessMode = "single-node-reader-only"
HostVolumeAccessModeSingleNodeWriter HostVolumeAccessMode = "single-node-writer"
HostVolumeAccessModeSingleNodeSingleWriter HostVolumeAccessMode = "single-node-single-writer"
HostVolumeAccessModeSingleNodeMultiWriter HostVolumeAccessMode = "single-node-multi-writer"
HostVolumeAccessModeSingleNodeReader VolumeAccessMode = "single-node-reader-only"
HostVolumeAccessModeSingleNodeWriter VolumeAccessMode = "single-node-writer"
HostVolumeAccessModeSingleNodeSingleWriter VolumeAccessMode = "single-node-single-writer"
HostVolumeAccessModeSingleNodeMultiWriter VolumeAccessMode = "single-node-multi-writer"
)

// HostVolumeStub is used for responses for the list volumes endpoint
Expand Down
4 changes: 2 additions & 2 deletions nomad/structs/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,8 @@ type VolumeRequest struct {
Source string
ReadOnly bool
Sticky bool
AccessMode CSIVolumeAccessMode
AttachmentMode CSIVolumeAttachmentMode
AccessMode VolumeAccessMode
AttachmentMode VolumeAttachmentMode
MountOptions *CSIMountOptions
PerAlloc bool
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/csi/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ type VolumeCapability struct {
MountVolume *structs.CSIMountOptions
}

func VolumeCapabilityFromStructs(sAccessType structs.CSIVolumeAttachmentMode, sAccessMode structs.CSIVolumeAccessMode, sMountOptions *structs.CSIMountOptions) (*VolumeCapability, error) {
func VolumeCapabilityFromStructs(sAccessType structs.VolumeAttachmentMode, sAccessMode structs.VolumeAccessMode, sMountOptions *structs.CSIMountOptions) (*VolumeCapability, error) {
var accessType VolumeAccessType
switch sAccessType {
case structs.CSIVolumeAttachmentModeBlockDevice:
Expand Down
8 changes: 4 additions & 4 deletions scheduler/feasible.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ func (h *HostVolumeChecker) hasVolumes(n *structs.Node) bool {
return false
}
if !h.hostVolumeIsAvailable(vol,
structs.HostVolumeAccessMode(req.AccessMode),
structs.HostVolumeAttachmentMode(req.AttachmentMode),
req.AccessMode,
req.AttachmentMode,
req.ReadOnly,
proposed,
) {
Expand Down Expand Up @@ -245,8 +245,8 @@ func (h *HostVolumeChecker) hasVolumes(n *structs.Node) bool {
// hostVolumeIsAvailable determines if a dynamic host volume is available for a request
func (h *HostVolumeChecker) hostVolumeIsAvailable(
vol *structs.HostVolume,
reqAccess structs.HostVolumeAccessMode,
reqAttach structs.HostVolumeAttachmentMode,
reqAccess structs.VolumeAccessMode,
reqAttach structs.VolumeAttachmentMode,
readOnly bool,
proposed []*structs.Allocation) bool {

Expand Down
8 changes: 4 additions & 4 deletions scheduler/feasible_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,13 +484,13 @@ func TestDynamicHostVolumeIsAvailable(t *testing.T) {

allCaps := []*structs.HostVolumeCapability{}

for _, accessMode := range []structs.HostVolumeAccessMode{
for _, accessMode := range []structs.VolumeAccessMode{
structs.HostVolumeAccessModeSingleNodeReader,
structs.HostVolumeAccessModeSingleNodeWriter,
structs.HostVolumeAccessModeSingleNodeSingleWriter,
structs.HostVolumeAccessModeSingleNodeMultiWriter,
} {
for _, attachMode := range []structs.HostVolumeAttachmentMode{
for _, attachMode := range []structs.VolumeAttachmentMode{
structs.HostVolumeAttachmentModeFilesystem,
structs.HostVolumeAttachmentModeBlockDevice,
} {
Expand Down Expand Up @@ -537,8 +537,8 @@ func TestDynamicHostVolumeIsAvailable(t *testing.T) {
name string
hasProposed []*structs.Allocation
hasCaps []*structs.HostVolumeCapability
wantAccess structs.HostVolumeAccessMode
wantAttach structs.HostVolumeAttachmentMode
wantAccess structs.VolumeAccessMode
wantAttach structs.VolumeAttachmentMode
readOnly bool
expect bool
}{
Expand Down

0 comments on commit b0a7f9a

Please sign in to comment.