-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add stage_publish_dir field to csi_plugin stanza of a job #13919
Conversation
api/tasks.go
Outdated
// e.g. staging_target_path = {StagePublishDir}/staging/{volume-id}/{usage-mode} | ||
// e.g. target_path = {StagePublishDir}/per-alloc/{alloc-id}/{volume-id}/{usage-mode} | ||
// Default is /local/csi. | ||
StagePublishDir string `mapstructure:"stage_publish_dir" hcl:"stage_publish_dir,optional"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming may or may not be controversial (as a new part of Nomad's public API). I tried to make it as descriptive as possible, but I'm definitely open to other ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah seems like a tricky name to get right. Maybe StagePublishBaseDir
and stage_publish_base_dir
?
Thanks @ejweber! I'm going to review this but just FYI we've got a bit of disruption going on this week with a team conference. I'll do my best to look at this early next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @ejweber! I've pulled this down locally and run my usual tests with a NFS plugin and everything seems to work as expected.
I've left a few comments, mostly around docs and naming. If you want, you can also add a changelog entry at .changelog/13919.txt
, similar to ones like .changelog/12193.txt
@@ -103,6 +103,16 @@ func newCSIPluginSupervisorHook(config *csiPluginSupervisorHookConfig) *csiPlugi | |||
socketMountPoint := filepath.Join(config.clientStateDirPath, "csi", | |||
"plugins", config.runner.Alloc().ID) | |||
|
|||
// In v1.3.0, Nomad started instructing CSI plugins to stage and publish | |||
// within /csi/local. Plugins deployed after the introduction of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is right but this comment has swapped the path ordering. It's /local/csi
.
HostPath: h.mountPoint, | ||
Readonly: false, | ||
PropagationMode: "bidirectional", | ||
} | ||
h.logger.Info("", "volumeStagingMounts", volumeStagingMounts) // TODO: Remove this before merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove this! 😀
api/tasks.go
Outdated
// e.g. staging_target_path = {StagePublishDir}/staging/{volume-id}/{usage-mode} | ||
// e.g. target_path = {StagePublishDir}/per-alloc/{alloc-id}/{volume-id}/{usage-mode} | ||
// Default is /local/csi. | ||
StagePublishDir string `mapstructure:"stage_publish_dir" hcl:"stage_publish_dir,optional"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah seems like a tricky name to get right. Maybe StagePublishBaseDir
and stage_publish_base_dir
?
command/agent/job_endpoint.go
Outdated
@@ -1263,6 +1263,7 @@ func ApiCSIPluginConfigToStructsCSIPluginConfig(apiConfig *api.TaskCSIPluginConf | |||
sc.ID = apiConfig.ID | |||
sc.Type = structs.CSIPluginType(apiConfig.Type) | |||
sc.MountDir = apiConfig.MountDir | |||
sc.StagePublishDir = apiConfig.StagePublishDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍 I've still got #10471 open to remove this whole gross blob of code but we'll need it for now.
LGTM. I'll drop a changelog entry here in a sec and once CI is green we'll get this merged. |
Thanks for the changelog entry @tgross. I ran out of time last night or I would have taken a stab at it. |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Resolves #13263.
This PR adds a new field to the csi_plugin stanza of a job that determines the base (inside a CSI plugin's container) of CSI staging_target_path and target_path directories.
This is my first PR to Nomad. I have attempted to go through the bullets in the checklist below. Some of the bullets are (I think) not applicable. For the docs, I tried to find all locations within /docs where CSI is mentioned and add the field as appropriate.
I updated comments on a couple of fields that I found to be confusing while trying to figure out where and how to get this implemented. I'm obviously open to reverting anything that the maintainers think is not an improvement.
Code
api/
packageapi/
structs usually have Canonicalize and Copy methodsnomad/structs
packagestructs/
structs usually have Copy, Equals, and Validate methodsapi/
packageapi/
andnomad/structs/
incommand/agent/job_endpoint.go
nomad/structs/
toapi/
conversions if necessary and write testsnomad/structs/diff.go
FieldDiff
slices innomad/structs/diff_test.go
scheduler/util.go/tasksUpdated
.Equals
but might not be, check.Docs