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()
This commit is contained in:
Adrian Chiris 2019-11-04 12:38:43 +01:00 committed by Kevin Klues
parent 78d7856288
commit d95464645c
11 changed files with 81 additions and 43 deletions

View File

@ -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)
}

View File

@ -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
}

View File

@ -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)

View File

@ -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)
}

View File

@ -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)

View File

@ -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
}

View File

@ -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)

View File

@ -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
}

View File

@ -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)

View File

@ -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
}

View File

@ -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{