-
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
Conversation
867f9fe to
b8d4028
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.
de3b143 to
759c692
Compare
759c692 to
a8fa4c3
Compare
|
Updated, "test-windows" failure looks like build server issue for me: Not sure if that happens often in here but if it does, my recommendation would be switch to Windows Server 2025 based build servers. There is no official release notes about this but based on my stress testing it seems that Microsoft has improved HNS stability in Win 2025. |
Yes, it's super flaky! Thanks for the recommendation! |
tgross
left a comment
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.
LGTM!
@mismithhisler is going to follow-up with docs updates.
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.