Skip to content

Commit 9f8a417

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #679 from ffromani/nrt-cache-get-fixes
nodetopologymatch: cache: clarify and test the get semantics
2 parents 63e08c7 + 12e4ab7 commit 9f8a417

File tree

6 files changed

+303
-144
lines changed

6 files changed

+303
-144
lines changed

‎pkg/noderesourcetopology/cache/cache.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ type Interface interface {
3030
// Over-reserved resources are the resources consumed by pods scheduled to that node after the last update
3131
// of NRT pertaining to the same node, pessimistically overallocated on ALL the NUMA zones of the node.
3232
// The pod argument is used only for logging purposes.
33-
// Returns a boolean to signal the caller if the NRT data is clean. If false, then the node has foreign
34-
// Pods detected - so it should be ignored or handled differently by the caller.
33+
// Returns nil if there is no NRT data available for the node named `nodeName`.
34+
// Returns a boolean to signal the caller if the NRT data is fresh.
35+
// If true, the data is fresh and ready to be consumed.
36+
// If false, the data is stale and the caller need to wait for a future refresh.
3537
GetCachedNRTCopy(ctx context.Context, nodeName string, pod *corev1.Pod) (*topologyv1alpha2.NodeResourceTopology, bool)
3638

3739
// NodeMaybeOverReserved declares a node was filtered out for not enough resources available.

‎pkg/noderesourcetopology/cache/cache_test.go

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
/*
2+
Copyright 2023 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package cache
18+
19+
import (
20+
"context"
21+
"encoding/json"
22+
"testing"
23+
24+
topologyv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
25+
26+
corev1 "k8s.io/api/core/v1"
27+
"k8s.io/apimachinery/pkg/api/resource"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29+
"k8s.io/apimachinery/pkg/runtime"
30+
podlisterv1 "k8s.io/client-go/listers/core/v1"
31+
32+
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
33+
tu "sigs.k8s.io/scheduler-plugins/test/util"
34+
)
35+
36+
type testCaseGetCachedNRTCopy struct {
37+
name string
38+
nodeTopologies []*topologyv1alpha2.NodeResourceTopology
39+
nodeName string
40+
hasForeignPods bool
41+
expectedNRT *topologyv1alpha2.NodeResourceTopology
42+
expectedOK bool
43+
}
44+
45+
func checkGetCachedNRTCopy(t *testing.T, makeCache func(client ctrlclient.Client, podLister podlisterv1.PodLister) (Interface, error), extraCases ...testCaseGetCachedNRTCopy) {
46+
t.Helper()
47+
48+
testNodeName := "worker-node-1"
49+
nrt := makeTestNRT(testNodeName)
50+
pod := &corev1.Pod{} // API placeholder
51+
ctx := context.Background()
52+
fakePodLister := &fakePodLister{}
53+
54+
testCases := []testCaseGetCachedNRTCopy{
55+
{
56+
name: "empty",
57+
nodeName: testNodeName,
58+
expectedNRT: nil,
59+
expectedOK: true, // because there's no data, and the information is fresh
60+
},
61+
{
62+
name: "data present",
63+
nodeTopologies: []*topologyv1alpha2.NodeResourceTopology{
64+
nrt,
65+
},
66+
nodeName: testNodeName,
67+
expectedNRT: nrt,
68+
expectedOK: true,
69+
},
70+
{
71+
name: "data missing for node",
72+
nodeTopologies: []*topologyv1alpha2.NodeResourceTopology{
73+
nrt,
74+
},
75+
nodeName: "invalid-node",
76+
expectedNRT: nil,
77+
expectedOK: true, // because there's no data, and the information is fresh
78+
},
79+
}
80+
testCases = append(testCases, extraCases...)
81+
82+
for _, tc := range testCases {
83+
t.Run(tc.name, func(t *testing.T) {
84+
objs := make([]runtime.Object, 0, len(tc.nodeTopologies))
85+
for _, nrt := range tc.nodeTopologies {
86+
objs = append(objs, nrt)
87+
}
88+
89+
fakeClient, err := tu.NewFakeClient(objs...)
90+
if err != nil {
91+
t.Fatal(err)
92+
}
93+
94+
nrtCache, err := makeCache(fakeClient, fakePodLister)
95+
if err != nil {
96+
t.Fatalf("unexpected error creating cache: %v", err)
97+
}
98+
99+
if tc.hasForeignPods {
100+
nrtCache.NodeHasForeignPods(tc.nodeName, pod)
101+
}
102+
103+
gotNRT, gotOK := nrtCache.GetCachedNRTCopy(ctx, tc.nodeName, pod)
104+
105+
if gotOK != tc.expectedOK {
106+
t.Fatalf("unexpected object status from cache: got: %v expected: %v", gotOK, tc.expectedOK)
107+
}
108+
if gotNRT != nil && tc.expectedNRT == nil {
109+
t.Fatalf("object from cache not nil but expected nil")
110+
}
111+
if gotNRT == nil && tc.expectedNRT != nil {
112+
t.Fatalf("object from cache nil but expected not nil")
113+
}
114+
115+
gotJSON := dumpNRT(gotNRT)
116+
expJSON := dumpNRT(tc.expectedNRT)
117+
if gotJSON != expJSON {
118+
t.Fatalf("unexpected object from cache\ngot: %s\nexpected: %s\n", gotJSON, expJSON)
119+
}
120+
})
121+
}
122+
}
123+
124+
func makeTestNRT(nodeName string) *topologyv1alpha2.NodeResourceTopology {
125+
return &topologyv1alpha2.NodeResourceTopology{
126+
TypeMeta: metav1.TypeMeta{
127+
Kind: "NodeResourceTopology",
128+
APIVersion: "topology.node.k8s.io/v1alpha2",
129+
},
130+
ObjectMeta: metav1.ObjectMeta{
131+
Name: nodeName,
132+
},
133+
Attributes: topologyv1alpha2.AttributeList{
134+
{
135+
Name: "topologyManagerPolicy",
136+
Value: "single-numa-node",
137+
},
138+
{
139+
Name: "topologyManagerScope",
140+
Value: "container",
141+
},
142+
},
143+
Zones: topologyv1alpha2.ZoneList{
144+
{
145+
Name: "node-0",
146+
Type: "Node",
147+
Resources: topologyv1alpha2.ResourceInfoList{
148+
MakeTopologyResInfo(cpu, "32", "30"),
149+
MakeTopologyResInfo(memory, "32Gi", "32Gi"),
150+
MakeTopologyResInfo(nicResourceName, "8", "8"),
151+
},
152+
},
153+
{
154+
Name: "node-1",
155+
Type: "Node",
156+
Resources: topologyv1alpha2.ResourceInfoList{
157+
MakeTopologyResInfo(cpu, "32", "30"),
158+
MakeTopologyResInfo(memory, "32Gi", "32Gi"),
159+
MakeTopologyResInfo(nicResourceName, "8", "8"),
160+
},
161+
},
162+
},
163+
}
164+
}
165+
166+
func dumpNRT(nrtObj *topologyv1alpha2.NodeResourceTopology) string {
167+
nrtJson, err := json.MarshalIndent(nrtObj, "", " ")
168+
if err != nil {
169+
return "marshallingError"
170+
}
171+
return string(nrtJson)
172+
}
173+
174+
func MakeTopologyResInfo(name, capacity, available string) topologyv1alpha2.ResourceInfo {
175+
return topologyv1alpha2.ResourceInfo{
176+
Name: name,
177+
Capacity: resource.MustParse(capacity),
178+
Available: resource.MustParse(available),
179+
}
180+
}
181+
182+
func makeDefaultTestTopology() []*topologyv1alpha2.NodeResourceTopology {
183+
return []*topologyv1alpha2.NodeResourceTopology{
184+
{
185+
ObjectMeta: metav1.ObjectMeta{Name: "node1"},
186+
TopologyPolicies: []string{string(topologyv1alpha2.SingleNUMANodeContainerLevel)},
187+
Zones: topologyv1alpha2.ZoneList{
188+
{
189+
Name: "node-0",
190+
Type: "Node",
191+
Resources: topologyv1alpha2.ResourceInfoList{
192+
MakeTopologyResInfo(cpu, "32", "30"),
193+
MakeTopologyResInfo(memory, "64Gi", "60Gi"),
194+
MakeTopologyResInfo(nicResourceName, "16", "16"),
195+
},
196+
},
197+
{
198+
Name: "node-1",
199+
Type: "Node",
200+
Resources: topologyv1alpha2.ResourceInfoList{
201+
MakeTopologyResInfo(cpu, "32", "30"),
202+
MakeTopologyResInfo(memory, "64Gi", "60Gi"),
203+
MakeTopologyResInfo(nicResourceName, "16", "16"),
204+
},
205+
},
206+
},
207+
},
208+
}
209+
}

‎pkg/noderesourcetopology/cache/discardreserved.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func (pt *DiscardReserved) GetCachedNRTCopy(ctx context.Context, nodeName string
6565

6666
nrt := &topologyv1alpha2.NodeResourceTopology{}
6767
if err := pt.client.Get(ctx, types.NamespacedName{Name: nodeName}, nrt); err != nil {
68-
return nil, false
68+
return nil, true
6969
}
7070
return nrt, true
7171
}

‎pkg/noderesourcetopology/cache/discardreserved_test.go

Lines changed: 20 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -25,65 +25,35 @@ import (
2525
corev1 "k8s.io/api/core/v1"
2626
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2727
"k8s.io/apimachinery/pkg/types"
28+
podlisterv1 "k8s.io/client-go/listers/core/v1"
2829

29-
tu "sigs.k8s.io/scheduler-plugins/test/util"
30+
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
3031
)
3132

32-
func TestDiscardReservedNodesGetNRTCopy(t *testing.T) {
33-
fakeClient, err := tu.NewFakeClient()
34-
if err != nil {
35-
t.Fatal(err)
36-
}
33+
func TestDiscardReservedNodesGetCachedNRTCopy(t *testing.T) {
34+
testNodeName := "worker-node-1"
35+
nrt := makeTestNRT(testNodeName)
3736

38-
ctx := context.Background()
39-
nrtCache := NewDiscardReserved(fakeClient)
40-
var nrtObj *topologyv1alpha2.NodeResourceTopology
41-
nrtObj, _ = nrtCache.GetCachedNRTCopy(ctx, "node1", &corev1.Pod{})
42-
if nrtObj != nil {
43-
t.Fatalf("non-empty object from empty cache")
44-
}
45-
46-
nodeTopologies := []*topologyv1alpha2.NodeResourceTopology{
37+
testCases := []testCaseGetCachedNRTCopy{
4738
{
48-
ObjectMeta: metav1.ObjectMeta{Name: "node1"},
49-
TopologyPolicies: []string{string(topologyv1alpha2.SingleNUMANodeContainerLevel)},
50-
Zones: topologyv1alpha2.ZoneList{
51-
{
52-
Name: "node-0",
53-
Type: "Node",
54-
Resources: topologyv1alpha2.ResourceInfoList{
55-
MakeTopologyResInfo(cpu, "20", "4"),
56-
MakeTopologyResInfo(memory, "8Gi", "8Gi"),
57-
MakeTopologyResInfo(nicResourceName, "30", "10"),
58-
},
59-
},
60-
{
61-
Name: "node-1",
62-
Type: "Node",
63-
Resources: topologyv1alpha2.ResourceInfoList{
64-
MakeTopologyResInfo(cpu, "30", "8"),
65-
MakeTopologyResInfo(memory, "8Gi", "8Gi"),
66-
MakeTopologyResInfo(nicResourceName, "30", "10"),
67-
},
68-
},
39+
name: "data present with foreign pods",
40+
nodeTopologies: []*topologyv1alpha2.NodeResourceTopology{
41+
nrt,
6942
},
43+
nodeName: testNodeName,
44+
hasForeignPods: true,
45+
expectedNRT: nrt,
46+
expectedOK: true,
7047
},
7148
}
72-
for _, obj := range nodeTopologies {
73-
if err := fakeClient.Create(ctx, obj.DeepCopy()); err != nil {
74-
t.Fatal(err)
75-
}
76-
}
7749

78-
nrtObj, ok := nrtCache.GetCachedNRTCopy(ctx, "node1", &corev1.Pod{})
79-
if !isNRTEqual(nrtObj, nodeTopologies[0]) {
80-
t.Fatalf("unexpected object from cache\ngot: %s\nexpected: %s\n",
81-
dumpNRT(nrtObj), dumpNRT(nodeTopologies[0]))
82-
}
83-
84-
if !ok {
85-
t.Fatalf("expecting GetCachedNRTCopy to return true not false")
86-
}
50+
checkGetCachedNRTCopy(
51+
t,
52+
func(client ctrlclient.Client, _ podlisterv1.PodLister) (Interface, error) {
53+
return NewDiscardReserved(client), nil
54+
},
55+
testCases...,
56+
)
8757
}
8858

8959
func TestDiscardReservedNodesGetNRTCopyFails(t *testing.T) {

0 commit comments

Comments
 (0)