-
Notifications
You must be signed in to change notification settings - Fork 2k
dynamic host volumes: fix Windows compatibility #27147
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
base: main
Are you sure you want to change the base?
Conversation
867f9fe to
b8d4028
Compare
b8d4028 to
5282e74
Compare
5282e74 to
de3b143
Compare
|
Rebased just in case. @tgross PTAL |
| if err := os.RemoveAll(path); err != nil { | ||
| log.Error("failed to remove directory after create failure", | ||
| "error", err) | ||
| if params.Uid != -1 || params.Gid != -1 { |
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.
If os.Chown always returns an error on windows, should we just wrap the chown in a if runtime.GOOS != "windows"?
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.
That would works too but isn't that bad that user might expect to set permissions which is not actually done?
Based on code -1 means change nothing so current logic would works exactly same way in both Linux and Windows as long user do not configure permissions but any other value would still give error not supported by windows
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 point! Also, I apologize for belaboring this, but I was curious how this was handled elsewhere in the codebase (it's not the only place we use os.Chown(). It looks we only chown when the euid from syscall.Geteuid() is 0. I wonder if, for consistency sake, we should do something similar here? This syscall returns -1 on Windows. See here for reference.
Either way, I think it's probably useful for someone on the Nomad team (I can do this) to update the mkdir plugin docs to let Windows users know that ownership changing is not available on Windows.
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.
I wonder if, for consistency sake, we should do something similar here?
I'd like to get away from doing that where we can, in hopes of being able to support rootless Nomad at some point. And for this PR I think it makes sense to just fix the Windows issue directly and not introduce a new constraint.
Description
Revisit logic added by #25533 and make sure that it does not break Windows compatibility.
Without this volume creation targeting to Windows node failed to error:
Testing & Reproduction steps
nomad volume create volume.hclLinks
Contributor Checklist
changelog entry using the
make clcommand.ensure regressions will be caught.
and job configuration, please update the Nomad product documentation, which is stored in the
web-unified-docsrepo. Refer to theweb-unified-docscontributor guide for docs guidelines.Please also consider whether the change requires notes within the upgrade
guide. If you would like help with the docs, tag the
nomad-docsteam in this PR.Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.