From d95464645cdad87a104080bb36a0a1e5a833ebf1 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 12:38:43 +0100 Subject: [PATCH] Add Merge() API to TopologyManager Policy abstraction This abstraction moves the responsibility of merging topology hints to the individual policies themselves. As part of this, it removes the CanAdmitPodResult() API from the policy abstraction, and rolls it into a second return value from Merge() --- pkg/kubelet/cm/topologymanager/policy.go | 5 +- .../cm/topologymanager/policy_best_effort.go | 8 ++- .../policy_best_effort_test.go | 2 +- pkg/kubelet/cm/topologymanager/policy_none.go | 6 +- .../cm/topologymanager/policy_none_test.go | 2 +- .../cm/topologymanager/policy_restricted.go | 8 ++- .../topologymanager/policy_restricted_test.go | 2 +- .../policy_single_numa_node.go | 8 ++- .../policy_single_numa_node_test.go | 2 +- .../cm/topologymanager/topology_manager.go | 12 ++-- .../topologymanager/topology_manager_test.go | 69 ++++++++++++------- 11 files changed, 81 insertions(+), 43 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 7ffa0a79f10..05f46714eab 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -24,6 +24,7 @@ import ( type Policy interface { //Returns Policy Name Name() string - //Returns Pod Admit Handler Response based on hints and policy type - CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult + // Returns a merged TopologyHint based on input from hint providers + // and a Pod Admit Handler Response based on hints and policy type + Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) } diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index c0611d31a10..97c61f031da 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -39,8 +39,14 @@ func (p *bestEffortPolicy) Name() string { return PolicyBestEffort } -func (p *bestEffortPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { return lifecycle.PodAdmitResult{ Admit: true, } } + +func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 9505c72f89f..1459a823bc3 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -41,7 +41,7 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewBestEffortPolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + result := policy.(*bestEffortPolicy).canAdmitPodResult(&tc.hint) if result.Admit != tc.expected { t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) diff --git a/pkg/kubelet/cm/topologymanager/policy_none.go b/pkg/kubelet/cm/topologymanager/policy_none.go index aacdf19aa10..7e5010a765d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none.go +++ b/pkg/kubelet/cm/topologymanager/policy_none.go @@ -36,8 +36,12 @@ func (p *nonePolicy) Name() string { return PolicyNone } -func (p *nonePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *nonePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { return lifecycle.PodAdmitResult{ Admit: true, } } + +func (p *nonePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + return TopologyHint{}, p.canAdmitPodResult(nil) +} diff --git a/pkg/kubelet/cm/topologymanager/policy_none_test.go b/pkg/kubelet/cm/topologymanager/policy_none_test.go index 76f46e490b4..cafcaf044b6 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -58,7 +58,7 @@ func TestPolicyNoneCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { policy := NewNonePolicy() - result := policy.CanAdmitPodResult(&tc.hint) + result := policy.(*nonePolicy).canAdmitPodResult(&tc.hint) if result.Admit != tc.expected { t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index 02e188a35b8..cc522980c0e 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -38,7 +38,7 @@ func (p *restrictedPolicy) Name() string { return PolicyRestricted } -func (p *restrictedPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { if !hint.Preferred { return lifecycle.PodAdmitResult{ Admit: false, @@ -50,3 +50,9 @@ func (p *restrictedPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAd Admit: true, } } + +func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index 2543d8af0c3..11119a0400c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -41,7 +41,7 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewRestrictedPolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + result := policy.(*restrictedPolicy).canAdmitPodResult(&tc.hint) if result.Admit != tc.expected { t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 7173ef94c5d..6f29e1c27b7 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -39,7 +39,7 @@ func (p *singleNumaNodePolicy) Name() string { return PolicySingleNumaNode } -func (p *singleNumaNodePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { if !hint.Preferred { return lifecycle.PodAdmitResult{ Admit: false, @@ -51,3 +51,9 @@ func (p *singleNumaNodePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.P Admit: true, } } + +func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go index 594fef71c6b..cdc895295fc 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -36,7 +36,7 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewSingleNumaNodePolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + result := policy.(*singleNumaNodePolicy).canAdmitPodResult(&tc.hint) if result.Admit != tc.expected { t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index f8288d8cef8..3600d3700db 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -63,8 +63,6 @@ type manager struct { podMap map[string]string //Topology Manager Policy policy Policy - //List of NUMA Nodes available on the underlying machine - numaNodes []int } // HintProvider is an interface for components that want to collaborate to @@ -157,7 +155,6 @@ func NewManager(numaNodeInfo cputopology.NUMANodeInfo, topologyPolicyName string podTopologyHints: pth, podMap: pm, policy: policy, - numaNodes: numaNodes, } return manager, nil @@ -325,11 +322,11 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st } // Collect Hints from hint providers and pass to policy to retrieve the best one. -func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) TopologyHint { +func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) (TopologyHint, lifecycle.PodAdmitResult) { providersHints := m.accumulateProvidersHints(pod, container) - bestHint := mergeProvidersHints(m.policy, m.numaNodes, providersHints) + bestHint, admit := m.policy.Merge(providersHints) klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint) - return bestHint + return bestHint, admit } func (m *manager) AddHintProvider(h HintProvider) { @@ -361,8 +358,7 @@ func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitR c := make(map[string]TopologyHint) for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { - result := m.calculateAffinity(*pod, container) - admitPod := m.policy.CanAdmitPodResult(&result) + result, admitPod := m.calculateAffinity(*pod, container) if !admitPod.Admit { return admitPod } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 0a79d10d38f..49c4e1cea88 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -114,15 +114,17 @@ func TestCalculateAffinity(t *testing.T) { policy Policy }{ { - name: "TopologyHint not set", - hp: []HintProvider{}, + name: "TopologyHint not set", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{}, expected: TopologyHint{ NUMANodeAffinity: NewTestBitMask(numaNodes...), Preferred: true, }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{}, @@ -134,7 +136,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -148,7 +151,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -162,7 +166,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -181,7 +186,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -200,7 +206,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 1/2", + name: "Two providers, 1 hint each, same mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -229,7 +236,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 2/2", + name: "Two providers, 1 hint each, same mask, both preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -258,7 +266,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -287,7 +296,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -316,7 +326,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, no common mask", + name: "Two providers, 1 hint each, no common mask", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -345,7 +356,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -374,7 +386,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -403,7 +416,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 1/2", + name: "Two providers, 1 no hints, 1 single hint preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -423,7 +437,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 2/2", + name: "Two providers, 1 no hints, 1 single hint preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -443,7 +458,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", + name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -476,7 +492,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", + name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -509,7 +526,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -542,7 +560,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, both with 2 hints, matching narrower preferred hint from both", + name: "Two providers, both with 2 hints, matching narrower preferred hint from both", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -579,7 +598,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", + name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -620,7 +640,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Multiple resources, same provider", + name: "Multiple resources, same provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -727,9 +748,8 @@ func TestCalculateAffinity(t *testing.T) { mngr := manager{ policy: tc.policy, hintProviders: tc.hp, - numaNodes: numaNodes, } - actual := mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + actual, _ := mngr.calculateAffinity(v1.Pod{}, v1.Container{}) if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) } @@ -1107,7 +1127,6 @@ func TestAdmit(t *testing.T) { policy: tc.policy, podTopologyHints: make(map[string]map[string]TopologyHint), hintProviders: tc.hp, - numaNodes: numaNodes, } pod := &v1.Pod{