-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
scheduler: fix klog.KObjSlice when applied to []*NodeInfo #125699
scheduler: fix klog.KObjSlice when applied to []*NodeInfo #125699
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
pkg/scheduler/framework/types.go
Outdated
// when nodes is a []*NodeInfo. | ||
var _ klog.KMetadata = &NodeInfo{} | ||
|
||
func (n *NodeInfo) GetName() string { return n.node.Name } |
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.
should we do if n != nil && n.node != nil
just in case?
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.
Nil pointer is handled by klog, but I had to write a test to be sure 😁
It also gets handled a bit unexpectedly by klog.KObj under the hood, resulting in an empty string instead of some kind of nil indicator. This predates my involvement, I might need to go back and check that.
In the meantime, let's add a unit test. The only downside is that it encodes the current behavior of klog. If I change that behavior, I'll have to update the test.
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 don't know whether NodeInfo may have a nil node pointer, but that definitely looks worth checking. Added.
Pushed, please take another look.
e8e23e0
to
a818a9f
Compare
a818a9f
to
e9e8209
Compare
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.
/retest
test/utils/ktesting/run.go
Outdated
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.
Why is it necessary?
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.
Darn. It isn't. Should have gone into a different PR. Fixed.
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.
Okay, I thought there was some tricks around go testing that I didn't know of 😅
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 can copy you on the PR where this will come back if you are curious 😄
But it's not a priority, so might take a while...
This is a feature of the underlying k8s.io/klog/v2/ktesting which is useful also when using the Kubernetes ktesting.
The DRA plugin does that. It didn't actually work and only printed an error message about NodeInfo not implementing klog.KMetata. That's not a compile-time check due to limitations with Go generics and had been missed earlier.
e9e8209
to
719a49c
Compare
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
/approve
LGTM label has been added. Git tree hash: 7ba66f7b5cb9576e35280e586d59c7bb7a8bfc14
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, sanposhiho The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/test pull-kubernetes-unit |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The DRA plugin uses klog.KObjSlice on a NodeInfo slice. It didn't actually work and only printed an error message about NodeInfo not implementing klog.KMetata. That's not a compile-time check due to limitations with Go generics and had been missed earlier.
Does this PR introduce a user-facing change?
/assign @sanposhiho