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

csi: NodePublish should not create target_path, only its parent dir #8505

Merged
merged 2 commits into from
Jul 23, 2020

Conversation

tgross
Copy link
Member

@tgross tgross commented Jul 22, 2020

Fixes #8358

The NodePublish workflow currently creates the target path and its parent
directory. However, the CSI specification says that the CO shall ensure the
parent directory of the target path exists, and that the SP shall place the
block device or mounted directory at the target path. Much of our testing has
been with CSI plugins that are more forgiving, but our behavior breaks
spec-compliant CSI plugins.

This changeset ensures we only create the parent directory.

The NodePublish workflow currently creates the target path and its parent
directory. However, the CSI specification says that the CO shall ensure the
parent directory of the target path exists, and that the SP shall place the
block device or mounted directory at the target path. Much of our testing has
been with CSI plugins that are more forgiving, but our behavior breaks
spec-compliant CSI plugins.

This changeset ensures we only create the parent directory.
@tgross tgross force-pushed the b-csi-nodepublish-target-path branch from 985b7c2 to a016d06 Compare July 22, 2020 20:09
@tgross
Copy link
Member Author

tgross commented Jul 22, 2020

Edit: fixed in e5bdcda

This seems to be "working" but has an unpleasant ergonomic result that also breaks backwards compat with existing jobspecs. With a volume mount like:

volume_mount {
  volume      = "test"
  destination = "${NOMAD_TASK_DIR}/test"
  read_only   = false
}

We end up with the mount showing up in the task at /local/test/rw-file-system-single-node-writer. So I need to correct how we pass the final binding into the container.

Copy link
Contributor

@langmartin langmartin left a comment

Choose a reason for hiding this comment

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

looks fine

@tgross
Copy link
Member Author

tgross commented Jul 23, 2020

I've tested on our usual set of regression targets as well as the DO plugin from the original report in #8358. Going to merge this and it'll ship in 0.12.2

@tgross tgross merged commit 1cb9e75 into master Jul 23, 2020
@tgross tgross deleted the b-csi-nodepublish-target-path branch July 23, 2020 19:52
@tgross tgross added this to the 0.12.2 milestone Jul 23, 2020
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nomad should not create the target_path passed into NodePublishVolume
2 participants