Add stage_publish_dir field to csi_plugin stanza of a job #13919

ejweber · 2022-07-25T19:39:09Z

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

Consider similar features in Consul, Kubernetes, and other tools. Is there prior art we should match? Terminology, structure, etc?
Add structs/fields to api/ package
- api/ structs usually have Canonicalize and Copy methods
- New fields should be added to existing Canonicalize, Copy methods
- Test the structs/fields via methods mentioned above
Add structs/fields to nomad/structs package
- structs/ structs usually have Copy, Equals, and Validate methods
- Validation happens in this package and must be implemented
- Note that analogous struct field names should match with api/ package
- Test the structs/fields via methods mentioned above
- Implement and test other logical methods
Add conversion between api/ and nomad/structs/ in command/agent/job_endpoint.go
- Add test for conversion
Determine JSON encoding strategy for responses from RPC (see "JSON Encoding" below)
- Write nomad/structs/ to api/ conversions if necessary and write tests
Implement diff logic for new structs/fields in nomad/structs/diff.go
- Note that fields must be listed in alphabetical order in FieldDiff slices in nomad/structs/diff_test.go
- Add test for diff of new structs/fields
Add change detection for new structs/fields in scheduler/util.go/tasksUpdated
- Might be covered by .Equals but might not be, check.
- Should return true if the task must be replaced as a result of the change.

Docs

Changelog
Jobspec entry https://www.nomadproject.io/docs/job-specification/index.html
Jobspec sidebar entry https://github.com/hashicorp/nomad/blob/main/website/data/docs-navigation.js
Job JSON API entry https://www.nomadproject.io/api/json-jobs.html
Sample Response output in API https://www.nomadproject.io/api/jobs.html
Consider if it needs a guide https://www.nomadproject.io/guides/index.html

…n.mount_dir

…ublishDir

hashicorp-cla · 2022-07-25T19:39:12Z

All committers have signed the CLA.

ejweber · 2022-07-25T19:45:53Z

api/tasks.go

+	// 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"`


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.

Yeah seems like a tricky name to get right. Maybe StagePublishBaseDir and stage_publish_base_dir?

client/allocrunner/taskrunner/plugin_supervisor_hook.go

tgross · 2022-07-27T19:15:32Z

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.

tgross

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

client/allocrunner/taskrunner/plugin_supervisor_hook.go

tgross · 2022-08-01T13:50:47Z

client/allocrunner/taskrunner/plugin_supervisor_hook.go

@@ -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


The code is right but this comment has swapped the path ordering. It's /local/csi.

tgross · 2022-08-01T13:51:12Z

client/allocrunner/taskrunner/plugin_supervisor_hook.go

 		HostPath:        h.mountPoint,
 		Readonly:        false,
 		PropagationMode: "bidirectional",
 	}
+	h.logger.Info("", "volumeStagingMounts", volumeStagingMounts) // TODO: Remove this before merge.


Don't forget to remove this! 😀

tgross · 2022-08-01T13:52:33Z

api/tasks.go

+	// 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"`


Yeah seems like a tricky name to get right. Maybe StagePublishBaseDir and stage_publish_base_dir?

tgross · 2022-08-01T13:59:39Z

command/agent/job_endpoint.go

@@ -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


Good catch 👍 I've still got #10471 open to remove this whole gross blob of code but we'll need it for now.

website/content/docs/concepts/plugins/csi.mdx

website/content/docs/job-specification/csi_plugin.mdx

tgross · 2022-08-02T13:00:12Z

LGTM. I'll drop a changelog entry here in a sec and once CI is green we'll get this merged.

ejweber · 2022-08-02T13:09:37Z

Thanks for the changelog entry @tgross. I ran out of time last night or I would have taken a stab at it.

github-actions · 2022-12-01T02:29:04Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ejweber added 4 commits July 25, 2022 13:35

Allow specification of CSI staging and publishing directory path

ca3c585

Add website documentation for stage_publish_dir

c8010c4

Replace erroneous reference to csi_plugin.mount_config with csi_plugi…

1eab7f5

…n.mount_dir

Avoid requiring CSI plugins to be redeployed after introducing StageP…

3448143

…ublishDir

tgross added this to Needs Triage in Nomad - Community Issues Triage via automation Jul 25, 2022

vercel bot deployed to Preview – nomad-storybook-and-ui July 25, 2022 19:43 View deployment

ejweber mentioned this pull request Jul 25, 2022

Make the base of CSI staging_target_paths and target_paths configurable #13263

Closed

ejweber commented Jul 25, 2022

View reviewed changes

client/allocrunner/taskrunner/plugin_supervisor_hook.go Show resolved Hide resolved

Fix gofmt -s failure

27e95c1

vercel bot deployed to Preview – nomad-storybook-and-ui July 25, 2022 20:03 View deployment

tgross self-requested a review July 25, 2022 20:14

ejweber mentioned this pull request Jul 26, 2022

Feedback on Nomad CSI ThinkParQ/beegfs-csi-driver#7

Closed

tgross requested changes Aug 1, 2022

View reviewed changes

tgross added the stage/waiting-reply label Aug 1, 2022

tgross moved this from Needs Triage to In Progress in Nomad - Community Issues Triage Aug 1, 2022

tgross added the theme/storage label Aug 1, 2022

Respond to PR feedback

b5d29a3

vercel bot deployed to Preview – nomad-storybook-and-ui August 1, 2022 23:14 View deployment

tgross approved these changes Aug 2, 2022

View reviewed changes

tgross removed the stage/waiting-reply label Aug 2, 2022

changelog entry

64d48fc

vercel bot deployed to Preview – nomad-storybook-and-ui August 2, 2022 13:06 View deployment

tgross merged commit 07bbf1f into hashicorp:main Aug 2, 2022

Nomad - Community Issues Triage automation moved this from In Progress to Done Aug 2, 2022

tgross added this to the 1.3.3 milestone Aug 2, 2022

tgross added backport/1.3.x backport to 1.3.x release line and removed backport/1.3.x backport to 1.3.x release line labels Aug 2, 2022

hc-github-team-nomad-core mentioned this pull request Aug 2, 2022

Backport of Add stage_publish_dir field to csi_plugin stanza of a job into release/1.3.x #13965

Merged

14 tasks

tgross added the backport/1.3.x backport to 1.3.x release line label Aug 2, 2022

hc-github-team-nomad-core mentioned this pull request Aug 2, 2022

Backport of Add stage_publish_dir field to csi_plugin stanza of a job into release/1.3.x #13966

Closed

14 tasks

github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stage_publish_dir field to csi_plugin stanza of a job #13919

Add stage_publish_dir field to csi_plugin stanza of a job #13919

ejweber commented Jul 25, 2022

hashicorp-cla commented Jul 25, 2022 •

edited

Loading

ejweber Jul 25, 2022

tgross Aug 1, 2022

tgross commented Jul 27, 2022

tgross left a comment

tgross Aug 1, 2022

tgross Aug 1, 2022

tgross Aug 1, 2022

tgross Aug 1, 2022

tgross commented Aug 2, 2022

ejweber commented Aug 2, 2022

github-actions bot commented Dec 1, 2022

Add stage_publish_dir field to csi_plugin stanza of a job #13919

Add stage_publish_dir field to csi_plugin stanza of a job #13919

Conversation

ejweber commented Jul 25, 2022

Code

Docs

hashicorp-cla commented Jul 25, 2022 • edited Loading

ejweber Jul 25, 2022

Choose a reason for hiding this comment

tgross Aug 1, 2022

Choose a reason for hiding this comment

tgross commented Jul 27, 2022

tgross left a comment

Choose a reason for hiding this comment

tgross Aug 1, 2022

Choose a reason for hiding this comment

tgross Aug 1, 2022

Choose a reason for hiding this comment

tgross Aug 1, 2022

Choose a reason for hiding this comment

tgross Aug 1, 2022

Choose a reason for hiding this comment

tgross commented Aug 2, 2022

ejweber commented Aug 2, 2022

github-actions bot commented Dec 1, 2022

hashicorp-cla commented Jul 25, 2022 •

edited

Loading