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
WIP: DRA structured parameters: scheduler fixes #123903
Closed
pohly
wants to merge
2
commits into
kubernetes:master
from
pohly:dra-structured-parameters-scheduler-fixes
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 remember this coming up in the past, but I don't remember. When might
handle.DriverName == ""
?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.
It's simply how we defined our API:
Perhaps we should have made it required to avoid if checks like this one here. Instead, we made it optional to allow avoiding redundant values in the claim status.
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.
Actually... validation requires it:
kubernetes/pkg/apis/resource/validation/validation.go
Line 201 in 634fc1b
That'll complain if the name is empty.
All questions about whether we could change validation aside (implies API break), I'm leaning towards keeping the validation as-is and fixing the API definition. This would make this PR unnecessary for 1.30 because only a small typo fix remains and also avoids this potential pitfall in the future.
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.
+1 on fixing API definition.
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 remember now why this is necessary. In "traditional" DRA, the driver's controller is responsible for populating the ResourceHandle. If the driver doesn't actually use the ResourceHandle to communicate information to the kubelet plugin (as is the case with the NVIDIA driver), then it shouldn't have to instantiate one just to set this DriverName field in it.
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.
It's the reason we have this in the kubelet:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cm/dra/manager.go#L135-L140
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.
Then there is no
ResourceHandle
to validate. That's unrelated to whetherDriverName
must be set when there is aResourceHandle
.Here I don't quite follow. That code only gets called if there is a
ResourceHandle
. Or is that using a fakeResourceHandle
inclaimInfo.ResourceHandles
that didn't actually from from the claim status? If that is so, then why not copy in the rightDriverName
when faking the handle?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.
See #124075 for the API fix.
/close