diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index c93f3d135a0..f2df6f29b27 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -28,3 +29,70 @@ type Policy interface { // and a Pod Admit Handler Response based on hints and policy type Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) } + +// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND +// of their affinity masks. The hint shall be preferred if all hits in the permutation +// are preferred. +func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint { + // Get the NUMANodeAffinity from each hint in the permutation and see if any + // of them encode unpreferred allocations. + preferred := true + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + var numaAffinities []bitmask.BitMask + for _, hint := range permutation { + // Only consider hints that have an actual NUMANodeAffinity set. + if hint.NUMANodeAffinity == nil { + numaAffinities = append(numaAffinities, defaultAffinity) + } else { + numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) + } + + if !hint.Preferred { + preferred = false + } + } + + // Merge the affinities using a bitwise-and operation. + mergedAffinity := bitmask.And(defaultAffinity, numaAffinities...) + // Build a mergedHint from the merged affinity mask, indicating if an + // preferred allocation was used to generate the affinity mask or not. + return TopologyHint{mergedAffinity, preferred} +} + +// Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. +// +// This procedure is implemented as a recursive function over the set of hints +// in 'allproviderHints[i]'. It applies the function 'callback' to each +// permutation as it is found. It is the equivalent of: +// +// for i := 0; i < len(providerHints[0]); i++ +// for j := 0; j < len(providerHints[1]); j++ +// for k := 0; k < len(providerHints[2]); k++ +// ... +// for z := 0; z < len(providerHints[-1]); z++ +// permutation := []TopologyHint{ +// providerHints[0][i], +// providerHints[1][j], +// providerHints[2][k], +// ... +// providerHints[-1][z] +// } +// callback(permutation) +func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { + // Internal helper function to accumulate the permutation before calling the callback. + var iterate func(i int, accum []TopologyHint) + iterate = func(i int, accum []TopologyHint) { + // Base case: we have looped through all providers and have a full permutation. + if i == len(allProviderHints) { + callback(accum) + return + } + + // Loop through all hints for provider 'i', and recurse to build the + // the permutation of this hint with all hints from providers 'i++'. + for j := range allProviderHints[i] { + iterate(i+1, append(accum, allProviderHints[i][j])) + } + } + iterate(0, []TopologyHint{}) +} diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 2a0d9f1d927..fdcf3a55087 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -48,54 +48,16 @@ func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAd } func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(providersHints) admit := p.canAdmitPodResult(&hint) return hint, admit } -// Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. -// -// This procedure is implemented as a recursive function over the set of hints -// in 'allproviderHints[i]'. It applies the function 'callback' to each -// permutation as it is found. It is the equivalent of: -// -// for i := 0; i < len(providerHints[0]); i++ -// for j := 0; j < len(providerHints[1]); j++ -// for k := 0; k < len(providerHints[2]); k++ -// ... -// for z := 0; z < len(providerHints[-1]); z++ -// permutation := []TopologyHint{ -// providerHints[0][i], -// providerHints[1][j], -// providerHints[2][k], -// ... -// providerHints[-1][z] -// } -// callback(permutation) -func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { - // Internal helper function to accumulate the permutation before calling the callback. - var iterate func(i int, accum []TopologyHint) - iterate = func(i int, accum []TopologyHint) { - // Base case: we have looped through all providers and have a full permutation. - if i == len(allProviderHints) { - callback(accum) - return - } - - // Loop through all hints for provider 'i', and recurse to build the - // the permutation of this hint with all hints from providers 'i++'. - for j := range allProviderHints[i] { - iterate(i+1, append(accum, allProviderHints[i][j])) - } - } - iterate(0, []TopologyHint{}) -} - // Merge the hints from all hint providers to find the best one. -func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[string][]TopologyHint) TopologyHint { +func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { // Set the default affinity as an any-numa affinity containing the list // of NUMA Nodes available on this machine. - defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) // Loop through all hint providers and save an accumulated list of the // hints returned by each hint provider. If no hints are provided, assume @@ -136,33 +98,7 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. - preferred := true - var numaAffinities []bitmask.BitMask - for _, hint := range permutation { - if hint.NUMANodeAffinity == nil { - numaAffinities = append(numaAffinities, defaultAffinity) - } else { - numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) - } - - if !hint.Preferred { - preferred = false - } - - // Special case PolicySingleNumaNode to only prefer hints where - // all providers have a single NUMA affinity set. - if policy != nil && policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 { - preferred = false - } - } - - // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) - mergedAffinity.And(numaAffinities...) - - // Build a mergedHintfrom the merged affinity mask, indicating if an - // preferred allocation was used to generate the affinity mask or not. - mergedHint := TopologyHint{mergedAffinity, preferred} + mergedHint := mergePermutation(p.numaNodes, permutation) // Only consider mergedHints that result in a NUMANodeAffinity > 0 to // replace the current bestHint. diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 2ae7de0ecd8..b661516be01 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -49,7 +49,7 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { } } -func TestBestEffortPolicyMerge(t *testing.T) { +func TestPolicyBestEffortMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewBestEffortPolicy(numaNodes) diff --git a/pkg/kubelet/cm/topologymanager/policy_none_test.go b/pkg/kubelet/cm/topologymanager/policy_none_test.go index cafcaf044b6..2c3263cec06 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -17,10 +17,11 @@ limitations under the License. package topologymanager import ( + "k8s.io/kubernetes/pkg/kubelet/lifecycle" "testing" ) -func TestName(t *testing.T) { +func TestPolicyNoneName(t *testing.T) { tcases := []struct { name string expected string @@ -65,3 +66,47 @@ func TestPolicyNoneCanAdmitPodResult(t *testing.T) { } } } + +func TestPolicyNoneMerge(t *testing.T) { + tcases := []struct { + name string + providersHints []map[string][]TopologyHint + expectedHint TopologyHint + expectedAdmit lifecycle.PodAdmitResult + }{ + { + name: "merged empty providers hints", + providersHints: []map[string][]TopologyHint{}, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, + }, + { + name: "merge with a single provider with a single preferred resource", + providersHints: []map[string][]TopologyHint{ + { + "resource": {{NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: true}}, + }, + }, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, + }, + { + name: "merge with a single provider with a single non-preferred resource", + providersHints: []map[string][]TopologyHint{ + { + "resource": {{NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}}, + }, + }, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, + }, + } + + for _, tc := range tcases { + policy := NewNonePolicy() + result, admit := policy.Merge(tc.providersHints) + if !result.IsEqual(tc.expectedHint) || admit.Admit != tc.expectedAdmit.Admit { + t.Errorf("Test Case: %s: Expected merge hint to be %v, got %v", tc.name, tc.expectedHint, result) + } + } +} diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index cc522980c0e..34e3fb3c247 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -52,7 +52,7 @@ func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAd } func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(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 f9cf840708c..d9463ea08a3 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -20,6 +20,24 @@ import ( "testing" ) +func TestPolicyRestrictedName(t *testing.T) { + tcases := []struct { + name string + expected string + }{ + { + name: "New Restricted Policy", + expected: "restricted", + }, + } + for _, tc := range tcases { + policy := NewRestrictedPolicy([]int{0, 1}) + if policy.Name() != tc.expected { + t.Errorf("Expected Policy Name to be %s, got %s", tc.expected, policy.Name()) + } + } +} + func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { tcases := []struct { name string @@ -58,7 +76,7 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { } } -func TestRestrictedPolicyMerge(t *testing.T) { +func TestPolicyRestrictedMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewRestrictedPolicy(numaNodes) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 6f29e1c27b7..ccbbe213aec 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -17,6 +17,8 @@ limitations under the License. package topologymanager import ( + "k8s.io/klog" + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -52,8 +54,111 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } +// Return hints that have valid bitmasks with exactly one bit set. +func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [][]TopologyHint { + var filteredResourcesHints [][]TopologyHint + for _, oneResourceHints := range allResourcesHints { + var filtered []TopologyHint + for _, hint := range oneResourceHints { + if hint.NUMANodeAffinity == nil && hint.Preferred == true { + filtered = append(filtered, hint) + } + if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 && hint.Preferred == true { + filtered = append(filtered, hint) + } + } + filteredResourcesHints = append(filteredResourcesHints, filtered) + } + return filteredResourcesHints +} + +func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { + // Set the default affinity as an any-numa affinity containing the list + // of NUMA Nodes available on this machine. + defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) + + // Loop through all provider hints and save an accumulated list of the + // hints returned by each hint provider. If no hints are provided, assume + // that provider has no preference for topology-aware allocation. + var allProviderHints [][]TopologyHint + for _, hints := range providersHints { + // 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}}) + continue + } + + // Otherwise, accumulate the hints for each resource type into allProviderHints. + for resource := range hints { + if hints[resource] == nil { + klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with resource '%s'", resource) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) + continue + } + + if len(hints[resource]) == 0 { + klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}}) + continue + } + + allProviderHints = append(allProviderHints, hints[resource]) + } + } + + // Filter to only include don't cares and hints with a single NUMA node. + allProviderHints = p.filterHints(allProviderHints) + + // Set the bestHint to return from this function as {nil false}. + // This will only be returned if no better hint can be found when + // merging hints from each hint provider. + bestHint := TopologyHint{defaultAffinity, false} + iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { + // Get the NUMANodeAffinity from each hint in the permutation and see if any + // of them encode unpreferred allocations. + mergedHint := mergePermutation(p.numaNodes, permutation) + + // Only consider mergedHints that result in a NUMANodeAffinity > 0 to + // replace the current bestHint. + if mergedHint.NUMANodeAffinity.Count() == 0 { + return + } + + // If the current bestHint is non-preferred and the new mergedHint is + // preferred, always choose the preferred hint over the non-preferred one. + if mergedHint.Preferred && !bestHint.Preferred { + bestHint = mergedHint + return + } + + // If the current bestHint is preferred and the new mergedHint is + // non-preferred, never update bestHint, regardless of mergedHint's + // narowness. + if !mergedHint.Preferred && bestHint.Preferred { + return + } + + // If mergedHint and bestHint has the same preference, only consider + // mergedHints that have a narrower NUMANodeAffinity than the + // NUMANodeAffinity in the current bestHint. + if !mergedHint.NUMANodeAffinity.IsNarrowerThan(bestHint.NUMANodeAffinity) { + return + } + + // In all other cases, update bestHint to the current mergedHint + bestHint = mergedHint + }) + + if bestHint.NUMANodeAffinity.IsEqual(defaultAffinity) { + bestHint = TopologyHint{nil, bestHint.Preferred} + } + + return bestHint +} + func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(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 a85862ba916..d8664e46b5c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "reflect" "testing" ) @@ -53,7 +54,119 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { } } -func TestSingleNumaNodePolicyMerge(t *testing.T) { +func TestPolicySingleNumaNodeFilterHints(t *testing.T) { + tcases := []struct { + name string + allResources [][]TopologyHint + expectedResources [][]TopologyHint + }{ + { + name: "filter empty resources", + allResources: [][]TopologyHint{}, + expectedResources: [][]TopologyHint(nil), + }, + { + name: "filter hints with nil socket mask 1/2", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + { + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + }, + { + name: "filter hints with nil socket mask 2/2", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + }, + { + name: "filter hints with empty resource socket mask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + {}, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + }, + []TopologyHint(nil), + }, + }, + { + name: "filter hints with wide sockemask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 2), Preferred: false}, + }, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + []TopologyHint(nil), + []TopologyHint(nil), + }, + }, + } + + numaNodes := []int{0, 1, 2, 3} + for _, tc := range tcases { + policy := NewSingleNumaNodePolicy(numaNodes) + actual := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) + if !reflect.DeepEqual(tc.expectedResources, actual) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected result to be %v, got %v", tc.expectedResources, actual) + } + } +} + +func TestPolicySingleNumaNodeMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewSingleNumaNodePolicy(numaNodes) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 5e8a61b6394..c8aa12bbaf9 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "reflect" "testing" "k8s.io/api/core/v1" @@ -30,92 +31,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "TopologyHint not set", - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": nil, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": {}, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, { name: "Two providers, 1 hint each, same mask, both preferred 1/2", hp: []HintProvider{ @@ -174,93 +89,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 hint each, no common mask", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: false, - }, - }, { name: "Two providers, 1 no hints, 1 single hint preferred 1/2", hp: []HintProvider{ @@ -367,39 +195,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, { name: "Two providers, both with 2 hints, matching narrower preferred hint from both", hp: []HintProvider{ @@ -520,6 +315,178 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, no common mask", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -549,6 +516,39 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -583,6 +583,211 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, no common mask", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ @@ -612,7 +817,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), + NUMANodeAffinity: nil, Preferred: false, }, }, @@ -658,11 +863,8 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) } actual, _ := policy.Merge(providersHints) - if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { - t.Errorf("%v: Expected NUMANodeAffinity in result to be %v, got %v", tc.name, tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) - } - if actual.Preferred != tc.expected.Preferred { - t.Errorf("%v: Expected Affinity preference in result to be %v, got %v", tc.name, tc.expected.Preferred, actual.Preferred) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("%v: Expected Topology Hint to be %v, got %v:", tc.name, tc.expected, actual) } } } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 9fc760413da..cd5b4aaf56f 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -104,6 +104,253 @@ func TestGetAffinity(t *testing.T) { } } +func TestAccumulateProvidersHints(t *testing.T) { + tcases := []struct { + name string + hp []HintProvider + expected []map[string][]TopologyHint + }{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: nil, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + {}, + }, + }, + { + name: "HintProvider returns - nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource": nil, + }, + }, + }, + { + name: "2 HintProviders with 1 resource returns hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": {TopologyHint{}}, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + { + "resource2": {TopologyHint{}}, + }, + }, + }, + { + name: "2 HintProviders 1 with 1 resource 1 with nil hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{nil}, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + nil, + }, + }, + { + name: "2 HintProviders 1 with 1 resource 1 empty hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + {}, + }, + }, + { + name: "HintProvider with 2 resources returns hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + "resource2": {TopologyHint{}}, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + "resource2": {TopologyHint{}}, + }, + }, + }, + } + + for _, tc := range tcases { + mngr := manager{ + hintProviders: tc.hp, + } + actual := mngr.accumulateProvidersHints(v1.Pod{}, v1.Container{}) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("Test Case %s: Expected NUMANodeAffinity in result to be %v, got %v", tc.name, tc.expected, actual) + } + } +} + +type mockPolicy struct { + nonePolicy + ph []map[string][]TopologyHint +} + +func (p *mockPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + p.ph = providersHints + return TopologyHint{}, lifecycle.PodAdmitResult{} +} + +func TestCalculateAffinity(t *testing.T) { + tcases := []struct { + name string + hp []HintProvider + expected []map[string][]TopologyHint + }{ + { + name: "No hint providers", + hp: []HintProvider{}, + expected: ([]map[string][]TopologyHint)(nil), + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + {}, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource": nil, + }, + }, + }, + { + name: "Assorted HintProviders", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-1/A": { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}, + }, + "resource-1/B": { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-2/A": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + "resource-2/B": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-3": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource-1/A": { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}, + }, + "resource-1/B": { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + }, + }, + { + "resource-2/A": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + "resource-2/B": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + }, + { + "resource-3": nil, + }, + }, + }, + } + + for _, tc := range tcases { + mngr := manager{} + mngr.policy = &mockPolicy{} + mngr.hintProviders = tc.hp + mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + actual := mngr.policy.(*mockPolicy).ph + if !reflect.DeepEqual(tc.expected, actual) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected result to be %v, got %v", tc.expected, actual) + } + } +} + func TestAddContainer(t *testing.T) { testCases := []struct { name string