Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dynamic host volumes: enforce exclusive access in plan apply #24881

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 16, 2025

Some dynamic host volumes are claimed by allocations with the capability we borrowed from CSI called single-node-single-writer, which says only one allocation can use the volume, and it can use it in read/write mode. We enforce this in the scheduler, but if evaluations for different jobs were to be processed concurrently by the scheduler, it's possible to get plans that would fail to enforce this requirement. Add a check in the plan applier to ensure that non-terminal allocations have exclusive access when requested.

Ref: https://hashicorp.atlassian.net/browse/NET-11549

Some dynamic host volumes are claimed by allocations with the capability we
borrowed from CSI called `single-node-single-writer`, which says only one
allocation can use the volume, and it can use it in read/write mode. We enforce
this in the scheduler, but if evaluations for different jobs were to be
processed concurrently by the scheduler, it's possible to get plans that would
fail to enforce this requirement. Add a check in the plan applier to ensure that
non-terminal allocations have exclusive access when requested.
@tgross tgross added this to the 1.10.0 milestone Jan 16, 2025
@tgross tgross marked this pull request as ready for review January 16, 2025 21:11
@tgross tgross requested review from a team as code owners January 16, 2025 21:11
for _, volReq := range group.Volumes {
hostVolumeClaims[volReq.Source]++
if volReq.AccessMode ==
CSIVolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a mental hiccup seeing CSIVolume...(HostVolume...) -- I'd think it should be

Suggested change
CSIVolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter) {
HostVolumeAccessMode(HostVolumeAccessModeSingleNodeSingleWriter) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not (ref volumes.go#L100-L101) because the type for VolumeRequest is named for the CSI mode.

// VolumeRequest is a representation of a storage volume that a TaskGroup wishes
// to use.
type VolumeRequest struct {
	Name           string
	Type           string
	Source         string
	ReadOnly       bool
	Sticky         bool
	AccessMode     CSIVolumeAccessMode
	AttachmentMode CSIVolumeAttachmentMode
	MountOptions   *CSIMountOptions
	PerAlloc       bool
}

But both are "defined types" on string, which msgpack serializes to the same thing on the wire. So we could swap out for a new shared VolumeAccessMode / VolumeAttachmentMode without breaking backcompat. Especially because in this case we didn't define types in the api package version of these fields. They're just strings there!

I'll do that, but under a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgross tgross merged commit 44d9c2a into main Jan 17, 2025
36 checks passed
@tgross tgross deleted the dhv-plan-applier branch January 17, 2025 20:38
tgross added a commit that referenced this pull request Jan 17, 2025
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)
tgross added a commit that referenced this pull request Jan 17, 2025
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants