From cbb985a31002db39f921c296650d191e9f8853a5 Mon Sep 17 00:00:00 2001 From: "Ian K. Coolidge" Date: Tue, 8 Nov 2022 14:57:03 +0000 Subject: [PATCH] cpuset: Delete 'builder' methods All usage of builder pattern is convertible to cpuset.New() with the same or fewer lines of code. Migrate Builder.Add to a private method of CPUSet, with a comment that it is only intended for internal use to preserve immutable propoerty of the exported interface. This also removes 'require' library dependency, which avoids non-standard library usage. --- .../cm/cpumanager/policy_static_test.go | 18 ++-- .../cm/cpumanager/topology/topology.go | 66 +++++++-------- pkg/kubelet/cm/cpuset/cpuset.go | 84 +++++++------------ pkg/kubelet/cm/cpuset/cpuset_test.go | 22 ----- test/e2e_node/cpu_manager_test.go | 5 +- test/e2e_node/podresources_test.go | 6 +- 6 files changed, 77 insertions(+), 124 deletions(-) diff --git a/pkg/kubelet/cm/cpumanager/policy_static_test.go b/pkg/kubelet/cm/cpumanager/policy_static_test.go index 25de6d25d9e..09ca6145606 100644 --- a/pkg/kubelet/cm/cpumanager/policy_static_test.go +++ b/pkg/kubelet/cm/cpumanager/policy_static_test.go @@ -176,21 +176,21 @@ func TestStaticPolicyStart(t *testing.T) { } func TestStaticPolicyAdd(t *testing.T) { - largeTopoBuilder := cpuset.NewBuilder() - largeTopoSock0Builder := cpuset.NewBuilder() - largeTopoSock1Builder := cpuset.NewBuilder() + var largeTopoCPUids []int + var largeTopoSock0CPUids []int + var largeTopoSock1CPUids []int largeTopo := *topoQuadSocketFourWayHT for cpuid, val := range largeTopo.CPUDetails { - largeTopoBuilder.Add(cpuid) + largeTopoCPUids = append(largeTopoCPUids, cpuid) if val.SocketID == 0 { - largeTopoSock0Builder.Add(cpuid) + largeTopoSock0CPUids = append(largeTopoSock0CPUids, cpuid) } else if val.SocketID == 1 { - largeTopoSock1Builder.Add(cpuid) + largeTopoSock1CPUids = append(largeTopoSock1CPUids, cpuid) } } - largeTopoCPUSet := largeTopoBuilder.Result() - largeTopoSock0CPUSet := largeTopoSock0Builder.Result() - largeTopoSock1CPUSet := largeTopoSock1Builder.Result() + largeTopoCPUSet := cpuset.New(largeTopoCPUids...) + largeTopoSock0CPUSet := cpuset.New(largeTopoSock0CPUids...) + largeTopoSock1CPUSet := cpuset.New(largeTopoSock1CPUids...) // these are the cases which must behave the same regardless the policy options. // So we will permutate the options to ensure this holds true. diff --git a/pkg/kubelet/cm/cpumanager/topology/topology.go b/pkg/kubelet/cm/cpumanager/topology/topology.go index c2b3ba8ad2b..7defe3400d2 100644 --- a/pkg/kubelet/cm/cpumanager/topology/topology.go +++ b/pkg/kubelet/cm/cpumanager/topology/topology.go @@ -83,138 +83,138 @@ func (d CPUDetails) KeepOnly(cpus cpuset.CPUSet) CPUDetails { // NUMANodes returns all of the NUMANode IDs associated with the CPUs in this // CPUDetails. func (d CPUDetails) NUMANodes() cpuset.CPUSet { - b := cpuset.NewBuilder() + var numaNodeIDs []int for _, info := range d { - b.Add(info.NUMANodeID) + numaNodeIDs = append(numaNodeIDs, info.NUMANodeID) } - return b.Result() + return cpuset.New(numaNodeIDs...) } // NUMANodesInSockets returns all of the logical NUMANode IDs associated with // the given socket IDs in this CPUDetails. func (d CPUDetails) NUMANodesInSockets(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var numaNodeIDs []int for _, id := range ids { for _, info := range d { if info.SocketID == id { - b.Add(info.NUMANodeID) + numaNodeIDs = append(numaNodeIDs, info.NUMANodeID) } } } - return b.Result() + return cpuset.New(numaNodeIDs...) } // Sockets returns all of the socket IDs associated with the CPUs in this // CPUDetails. func (d CPUDetails) Sockets() cpuset.CPUSet { - b := cpuset.NewBuilder() + var socketIDs []int for _, info := range d { - b.Add(info.SocketID) + socketIDs = append(socketIDs, info.SocketID) } - return b.Result() + return cpuset.New(socketIDs...) } // CPUsInSockets returns all of the logical CPU IDs associated with the given // socket IDs in this CPUDetails. func (d CPUDetails) CPUsInSockets(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var cpuIDs []int for _, id := range ids { for cpu, info := range d { if info.SocketID == id { - b.Add(cpu) + cpuIDs = append(cpuIDs, cpu) } } } - return b.Result() + return cpuset.New(cpuIDs...) } // SocketsInNUMANodes returns all of the logical Socket IDs associated with the // given NUMANode IDs in this CPUDetails. func (d CPUDetails) SocketsInNUMANodes(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var socketIDs []int for _, id := range ids { for _, info := range d { if info.NUMANodeID == id { - b.Add(info.SocketID) + socketIDs = append(socketIDs, info.SocketID) } } } - return b.Result() + return cpuset.New(socketIDs...) } // Cores returns all of the core IDs associated with the CPUs in this // CPUDetails. func (d CPUDetails) Cores() cpuset.CPUSet { - b := cpuset.NewBuilder() + var coreIDs []int for _, info := range d { - b.Add(info.CoreID) + coreIDs = append(coreIDs, info.CoreID) } - return b.Result() + return cpuset.New(coreIDs...) } // CoresInNUMANodes returns all of the core IDs associated with the given // NUMANode IDs in this CPUDetails. func (d CPUDetails) CoresInNUMANodes(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var coreIDs []int for _, id := range ids { for _, info := range d { if info.NUMANodeID == id { - b.Add(info.CoreID) + coreIDs = append(coreIDs, info.CoreID) } } } - return b.Result() + return cpuset.New(coreIDs...) } // CoresInSockets returns all of the core IDs associated with the given socket // IDs in this CPUDetails. func (d CPUDetails) CoresInSockets(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var coreIDs []int for _, id := range ids { for _, info := range d { if info.SocketID == id { - b.Add(info.CoreID) + coreIDs = append(coreIDs, info.CoreID) } } } - return b.Result() + return cpuset.New(coreIDs...) } // CPUs returns all of the logical CPU IDs in this CPUDetails. func (d CPUDetails) CPUs() cpuset.CPUSet { - b := cpuset.NewBuilder() + var cpuIDs []int for cpuID := range d { - b.Add(cpuID) + cpuIDs = append(cpuIDs, cpuID) } - return b.Result() + return cpuset.New(cpuIDs...) } // CPUsInNUMANodes returns all of the logical CPU IDs associated with the given // NUMANode IDs in this CPUDetails. func (d CPUDetails) CPUsInNUMANodes(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var cpuIDs []int for _, id := range ids { for cpu, info := range d { if info.NUMANodeID == id { - b.Add(cpu) + cpuIDs = append(cpuIDs, cpu) } } } - return b.Result() + return cpuset.New(cpuIDs...) } // CPUsInCores returns all of the logical CPU IDs associated with the given // core IDs in this CPUDetails. func (d CPUDetails) CPUsInCores(ids ...int) cpuset.CPUSet { - b := cpuset.NewBuilder() + var cpuIDs []int for _, id := range ids { for cpu, info := range d { if info.CoreID == id { - b.Add(cpu) + cpuIDs = append(cpuIDs, cpu) } } } - return b.Result() + return cpuset.New(cpuIDs...) } // Discover returns CPUTopology based on cadvisor node info diff --git a/pkg/kubelet/cm/cpuset/cpuset.go b/pkg/kubelet/cm/cpuset/cpuset.go index ab436731d86..a0f6a78361c 100644 --- a/pkg/kubelet/cm/cpuset/cpuset.go +++ b/pkg/kubelet/cm/cpuset/cpuset.go @@ -25,40 +25,6 @@ import ( "strings" ) -// Builder is a mutable builder for CPUSet. Functions that mutate instances -// of this type are not thread-safe. -type Builder struct { - result CPUSet - done bool -} - -// NewBuilder returns a mutable CPUSet builder. -func NewBuilder() *Builder { - return &Builder{ - result: CPUSet{ - elems: map[int]struct{}{}, - }, - } -} - -// Add adds the supplied elements to the result. Calling Add after calling -// Result has no effect. -func (b *Builder) Add(elems ...int) { - if b.done { - return - } - for _, elem := range elems { - b.result.elems[elem] = struct{}{} - } -} - -// Result returns the result CPUSet containing all elements that were -// previously added to this builder. Subsequent calls to Add have no effect. -func (b *Builder) Result() CPUSet { - b.done = true - return b.result -} - // CPUSet is a thread-safe, immutable set-like data structure for CPU IDs. type CPUSet struct { elems map[int]struct{} @@ -66,11 +32,21 @@ type CPUSet struct { // New returns a new CPUSet containing the supplied elements. func New(cpus ...int) CPUSet { - b := NewBuilder() - for _, c := range cpus { - b.Add(c) + s := CPUSet{ + elems: map[int]struct{}{}, + } + for _, c := range cpus { + s.add(c) + } + return s +} + +// add adds the supplied elements to the CPUSet. +// It is intended for internal use only, since it mutates the CPUSet. +func (s CPUSet) add(elems ...int) { + for _, elem := range elems { + s.elems[elem] = struct{}{} } - return b.Result() } // Size returns the number of elements in this set. @@ -98,13 +74,13 @@ func (s CPUSet) Equals(s2 CPUSet) bool { // filter returns a new CPU set that contains all of the elements from this // set that match the supplied predicate, without mutating the source set. func (s CPUSet) filter(predicate func(int) bool) CPUSet { - b := NewBuilder() + r := New() for cpu := range s.elems { if predicate(cpu) { - b.Add(cpu) + r.add(cpu) } } - return b.Result() + return r } // IsSubsetOf returns true if the supplied set contains all the elements @@ -123,16 +99,16 @@ func (s CPUSet) IsSubsetOf(s2 CPUSet) bool { // set and all of the elements from the supplied sets, without mutating // either source set. func (s CPUSet) Union(s2 ...CPUSet) CPUSet { - b := NewBuilder() + r := New() for cpu := range s.elems { - b.Add(cpu) + r.add(cpu) } for _, cs := range s2 { for cpu := range cs.elems { - b.Add(cpu) + r.add(cpu) } } - return b.Result() + return r } // Intersection returns a new CPU set that contains all of the elements @@ -214,13 +190,13 @@ func (s CPUSet) String() string { // // See: http://man7.org/linux/man-pages/man7/cpuset.7.html#FORMATS func Parse(s string) (CPUSet, error) { - b := NewBuilder() - // Handle empty string. if s == "" { - return b.Result(), nil + return New(), nil } + result := New() + // Split CPU list string: // "0-5,34,46-48" => ["0-5", "34", "46-48"] ranges := strings.Split(s, ",") @@ -233,7 +209,7 @@ func Parse(s string) (CPUSet, error) { if err != nil { return New(), err } - b.Add(elem) + result.add(elem) } else if len(boundaries) == 2 { // Handle multi-element ranges like "0-5". start, err := strconv.Atoi(boundaries[0]) @@ -252,18 +228,18 @@ func Parse(s string) (CPUSet, error) { // Add all elements to the result. // e.g. "0-5", "46-48" => [0, 1, 2, 3, 4, 5, 46, 47, 48]. for e := start; e <= end; e++ { - b.Add(e) + result.add(e) } } } - return b.Result(), nil + return result, nil } // Clone returns a copy of this CPU set. func (s CPUSet) Clone() CPUSet { - b := NewBuilder() + r := New() for elem := range s.elems { - b.Add(elem) + r.add(elem) } - return b.Result() + return r } diff --git a/pkg/kubelet/cm/cpuset/cpuset_test.go b/pkg/kubelet/cm/cpuset/cpuset_test.go index d8dcab373b1..ac4d32134b9 100644 --- a/pkg/kubelet/cm/cpuset/cpuset_test.go +++ b/pkg/kubelet/cm/cpuset/cpuset_test.go @@ -19,30 +19,8 @@ package cpuset import ( "reflect" "testing" - - "github.com/stretchr/testify/require" ) -func TestCPUSetBuilder(t *testing.T) { - b := NewBuilder() - elems := []int{1, 2, 3, 4, 5} - for _, elem := range elems { - b.Add(elem) - } - result := b.Result() - for _, elem := range elems { - if !result.Contains(elem) { - t.Fatalf("expected cpuset to contain element %d: [%v]", elem, result) - } - } - if len(elems) != result.Size() { - t.Fatalf("expected cpuset %s to have the same size as %v", result, elems) - } - - b.Add(6) - require.False(t, result.Contains(6), "expected calls to Add after calling Result() to have no effect") -} - func TestCPUSetSize(t *testing.T) { testCases := []struct { cpuset CPUSet diff --git a/test/e2e_node/cpu_manager_test.go b/test/e2e_node/cpu_manager_test.go index f868c547fe7..f604ea8a009 100644 --- a/test/e2e_node/cpu_manager_test.go +++ b/test/e2e_node/cpu_manager_test.go @@ -733,13 +733,12 @@ func validateSMTAlignment(cpus cpuset.CPUSet, smtLevel int, pod *v1.Pod, cnt *v1 // now check all the given cpus are thread siblings. // to do so the easiest way is to rebuild the expected set of siblings from all the cpus we got. // if the expected set matches the given set, the given set was good. - b := cpuset.NewBuilder() + siblingsCPUs := cpuset.New() for _, cpuID := range cpus.UnsortedList() { threadSiblings, err := cpuset.Parse(strings.TrimSpace(getCPUSiblingList(int64(cpuID)))) framework.ExpectNoError(err, "parsing cpuset from logs for [%s] of pod [%s]", cnt.Name, pod.Name) - b.Add(threadSiblings.UnsortedList()...) + siblingsCPUs = siblingsCPUs.Union(threadSiblings) } - siblingsCPUs := b.Result() framework.Logf("siblings cpus: %v", siblingsCPUs) if !siblingsCPUs.Equals(cpus) { diff --git a/test/e2e_node/podresources_test.go b/test/e2e_node/podresources_test.go index 09fc9419de4..0625995dcad 100644 --- a/test/e2e_node/podresources_test.go +++ b/test/e2e_node/podresources_test.go @@ -522,11 +522,11 @@ func podresourcesGetAllocatableResourcesTests(ctx context.Context, cli kubeletpo resp, err := cli.GetAllocatableResources(ctx, &kubeletpodresourcesv1.AllocatableResourcesRequest{}) framework.ExpectNoErrorWithOffset(1, err) devs := resp.GetDevices() - b := cpuset.NewBuilder() + var cpus []int for _, cpuid := range resp.GetCpuIds() { - b.Add(int(cpuid)) + cpus = append(cpus, int(cpuid)) } - allocatableCPUs := b.Result() + allocatableCPUs := cpuset.New(cpus...) if onlineCPUs.Size() == 0 { ginkgo.By("expecting no CPUs reported")