-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
fix: verify metadata is non-nil in resource allocation #77845
fix: verify metadata is non-nil in resource allocation #77845
Conversation
/assign @k82cn @Huang-Wei |
b4c4772
to
b05beec
Compare
/retest |
} | ||
} | ||
list, err := priorityFunction(BalancedResourceAllocationMap, nil, nil)(test.pod, nodeNameToInfo, test.nodes) |
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.
we are passing nil already in the test, was the test crashing all the time?
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.
Let me try to explain here: "non-typed" nil and "typed" nil (i.e. "var foo *meta") behave differently in the type assertion.
Current testcase and code base are always using "non-typed" nil, so it works fine all the time. In this PR, @draveness introduced a potential "typed" nil (when hasMeta=false), so it can pass type assertion. It's a Golang gotcha which IMO should be aware of, but shouldn't practice as a pattern in code.
@draveness my suggestion is:
- avoid introducing "typed" nil as it can confuse people
- eliminate the
&& meta != nil
check in this PR as well as feat: cache pod limits as part of metadata in priority functions #77498
Thoughts?
BTW: a write-up on typed nil.
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.
SGTM, updated as your suggestion.
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.
sure, but in the new code, if you pass a typed nil, wouldn't that cause a crash? shouldn't we make sure that does not happen?
9d3c1fd
to
0ad4624
Compare
0ad4624
to
7336725
Compare
/retest |
1 similar comment
/retest |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: draveness, Huang-Wei 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 |
/kind bug
What this PR does / why we need it:
Fix a bug when metadata is nil in resource allocation.
It crashes when
meta
is of type*priorityMetadata
but equals tonil
Related to #77498 (review)
Does this PR introduce a user-facing change?: