Skip to content
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] Updated - topologymanager: Add Merge method to Policy #85798

Open
wants to merge 7 commits into
base: master
from

Conversation

@nolancon
Copy link
Contributor

nolancon commented Dec 2, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR is an updated version of #81586:
"In the current design the Topology Manager generates the merged
hints and select the best hint and the policy indicates whether
or not to admit the pod.

This patch adds Merge abstraction to Topology Manager policies which
allow the them to control on how to merge the permutation hints.
It also update the strict policy to admit only if all resources
aligned to same NUMA."

Now that #84721 has been merged, the changes proposed here are the original commits by @adrianchiris, but rebased to accommodate easier reviewing.

Related Issue: #83478

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nolancon
To complete the pull request process, please assign connordoyle
You can assign the PR to them by writing /assign @connordoyle in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from krmayankk and thockin Dec 2, 2019
@nolancon

This comment has been minimized.

Copy link
Contributor Author

nolancon commented Dec 2, 2019

@k8s-ci-robot k8s-ci-robot requested review from klueska and lmdaly Dec 2, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

@nolancon: GitHub didn't allow me to request PR reviews from the following users: adrianchiris.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @adrianchiris @klueska @lmdaly

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.

pkg/kubelet/cm/topologymanager/topology_manager.go Outdated Show resolved Hide resolved
@@ -105,21 +124,21 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st
// If hints is nil, insert a single, preferred any-numa hint into allProviderHints.
if len(hints) == 0 {
klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with any resource")
allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}})
allProviderHints = append(allProviderHints, []TopologyHint{{defaultAffinity, true}})

This comment has been minimized.

Copy link
@adrianchiris

adrianchiris Dec 2, 2019

Contributor

A bit of history here:
The reason we put defaultAffinity here instead of nil is simply because nil was introduced when adding SingleNumaNode policy since we had one merge logic that dealt with all policies.

since Single NUMA policy has now its own logic, this is no longer needed, thus we restore the original value.
Same applies for L#135,141

pkg/kubelet/cm/topologymanager/policy_best_effort.go Outdated Show resolved Hide resolved
pkg/kubelet/cm/topologymanager/policy_best_effort.go Outdated Show resolved Hide resolved
pkg/kubelet/cm/topologymanager/policy_restricted.go Outdated Show resolved Hide resolved
@klueska

This comment has been minimized.

Copy link
Contributor

klueska commented Dec 4, 2019

Can you please break this PR into small incremental commits , each with a single, logical change in them? The main problem with the original PR was that it was too hard to review because lots of changes were smashed into a single commit.

To help with this, I took the original PR and broke (most of it) it into these logical changes:
https://github.com/kubernetes/kubernetes/pull/84721/commits

Derek was then able to merge them the same day I put the PR up because it was much easier to review: #84721 (comment)

It would be good to do the same for these remaining changes.

adrianchiris added 7 commits Dec 5, 2019
- Modularize code with mergePermutation method
Update unit tests for none_policy
Add Name test for policy_restricted
- Remove need to pass policy and numaNodes as arguments
- Remove PolicySingleNUMANode special case check in policy_best_effort
- Add mergeProviderHints base to policy_single_numa_node for upcoming
commit
Now that PolicySingleNUMANode is not considered here, return
defaultAffinity as was the original case before previous bug fix
Explanation taken from original commit:
- Change the current method of finding the best hint.
  Instead of going over all permutations, sort the hints and find
  the narrowest hint common to all resources.
- Break out early when merging to a preferred hint is not possible
@nolancon nolancon changed the title Updated - topologymanager: Add Merge method to Policy [WIP] Updated - topologymanager: Add Merge method to Policy Dec 6, 2019
@nolancon nolancon force-pushed the nolancon:merge-policy-rebase branch from 54e5b08 to 181abd0 Dec 6, 2019
@nolancon nolancon force-pushed the nolancon:merge-policy-rebase branch from 181abd0 to 3f25ea5 Dec 6, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 6, 2019

@nolancon: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test 3f25ea5 link /test pull-kubernetes-bazel-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@nolancon

This comment has been minimized.

Copy link
Contributor Author

nolancon commented Dec 6, 2019

I've broken this PR into smaller commits to accommodate the review process based on the advice of @klueska.
I've also marked it as WIP - there are some unit tests failing so it will need additional work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.