-
Notifications
You must be signed in to change notification settings - Fork 338
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
Remove topology fallback logic for nodes < 1.14 #448
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msau42 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 |
/assign @verult |
// * allowedTopologies had empty entries. | ||
// | ||
// Either way, we fallback to the "topology disabled" behavior. | ||
// We fallback to the "topology disabled" behavior. | ||
if len(requisiteTerms) == 0 { |
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.
In the if len(allowedTopologies) != 0
clause, allowedTopologies can't be empty. Reaching here means flatten()
is broken
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 believe this comment is referring to the case where you specify an allowedtopology term struct but leave it empty.
The API validation logic allows for this: https://github.com/kubernetes/kubernetes/blob/7ceac2baf0820fd354259aa8b7f0e37b0bc6b814/pkg/apis/core/validation/validation.go#L3474
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.
ACK
/lgtm |
…go_modules/k8s.io/api-0.27.3 Bump k8s.io/api from 0.27.2 to 0.27.3
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We now require the API server to be on at least 1.17, which means we can remove node support for < 1.15.
Another major change in behavior made here is that if the topology feature gate is enabled, then we assume that the driver on the nodes are correctly reporting topology. We no longer fallback to "no topology" behavior. This means that if you are a driver that is going to upgrade to support topology, then it's a two step process. First, upgrade the driver on the nodes to start reporting topology, then flip the feature gate on the controller.
Which issue(s) this PR fixes:
Fixes #257
Special notes for your reviewer:
Does this PR introduce a user-facing change?: