Modify GetTopologyHints() for per-resource hints in the TopologyManager #1131
Conversation
At present, there is no way for a hint provider to return distinct hints for different resource types via a call to `GetTopologyHints()`. This means that hint providers that govern multiple resource types (e.g. the devicemanager) must do some sort of "pre-merge" on the hints it generates for each resource type before passing them back to the `TopologyManager`. This seems counter-intuitive, since there is no practical reason that a "pre-merge" should be necessary -- it just happens to be necessary because of the way the current interface is designed. It would be better to allow a hint provider to pass back raw hints for each resource type, and allow the `TopologyManager` to merge them using a single unified strategy. This patch makes a simple change to the `GetTopologyHints()` interface to allow this to occur. Moreover, this change allows the `TopologyManager` to recognize which resource type a set of hints originated from, should this information become useful in the future.
k8s-ci-robot
added
cncf-cla: yes
|
Hi @klueska. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
kind/kep
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: klueska The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@klueska: GitHub didn't allow me to request PR reviews from the following users: cbf123, lmdaly, moshe010. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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/test-infra repository. |
| // resource types that it governs; otherwise returns `nil` to indicate "don't | ||
| // care". Hints for different resource types are indexed by their name in the | ||
| // returned map. | ||
| GetTopologyHints(pod v1.Pod, containerName string) map[string][]TopologyHint |
|
LGTM! Are we planning on using the resource name in the pod spec as their name in the map? |
|
That’s what I would expect, yes. |
|
I now have a patch for this based on the current state of the outstanding PRs for the They are the top 6 commits in: |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/assign @dchen1107 @derekwaynecarr |
|
/assign @ConnorDoyle |
|
Closing in favor of #1222 which also includes this change. |
At present, there is no way for a hint provider to return distinct hints
for different resource types via a call to
GetTopologyHints(). Thismeans that hint providers that govern multiple resource types (e.g. the
devicemanager) must do some sort of "pre-merge" on the hints it
generates for each resource type before passing them back to the
TopologyManager.This seems counter-intuitive, since there is no practical reason that a
"pre-merge" should be necessary -- it just happens to be necessary
because of the way the current interface is designed.
It would be better to allow a hint provider to pass back raw hints for
each resource type, and allow the
TopologyManagerto merge them usinga single unified strategy.
This patch makes a simple change to the
GetTopologyHints()interfaceto allow this to occur.
Moreover, this change allows the
TopologyManagerto recognize whichresource type a set of hints originated from, should this information
become useful in the future.