Skip to content

Commit 7a506ff

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#87696 from liggitt/node2
Switch node authorizer indexes to reference counts, add fastpath edge removal
2 parents bb3cddc + 8a3f587 commit 7a506ff

File tree

4 files changed

+115
-94
lines changed

4 files changed

+115
-94
lines changed

‎plugin/pkg/auth/authorizer/node/graph.go‎

Lines changed: 41 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ func (g *Graph) deleteVertex_locked(vertexType vertexType, namespace, name strin
175175

176176
// find existing neighbors with a single edge (meaning we are their only neighbor)
177177
neighborsToRemove := []graph.Node{}
178-
neighborsToRecompute := []graph.Node{}
178+
edgesToRemoveFromIndexes := []graph.Edge{}
179179
g.graph.VisitFrom(vertex, func(neighbor graph.Node) bool {
180180
// this downstream neighbor has only one edge (which must be from us), so remove them as well
181181
if g.graph.Degree(neighbor) == 1 {
@@ -188,8 +188,8 @@ func (g *Graph) deleteVertex_locked(vertexType vertexType, namespace, name strin
188188
// this upstream neighbor has only one edge (which must be to us), so remove them as well
189189
neighborsToRemove = append(neighborsToRemove, neighbor)
190190
} else {
191-
// recompute the destination edge index on this neighbor
192-
neighborsToRecompute = append(neighborsToRecompute, neighbor)
191+
// decrement the destination edge index on this neighbor if the edge between us was a destination edge
192+
edgesToRemoveFromIndexes = append(edgesToRemoveFromIndexes, g.graph.EdgeBetween(vertex, neighbor))
193193
}
194194
return true
195195
})
@@ -202,9 +202,9 @@ func (g *Graph) deleteVertex_locked(vertexType vertexType, namespace, name strin
202202
g.removeVertex_locked(neighbor.(*namedVertex))
203203
}
204204

205-
// recompute destination indexes for neighbors that dropped outbound edges
206-
for _, neighbor := range neighborsToRecompute {
207-
g.recomputeDestinationIndex_locked(neighbor)
205+
// remove edges from destination indexes for neighbors that dropped outbound edges
206+
for _, edge := range edgesToRemoveFromIndexes {
207+
g.removeEdgeFromDestinationIndex_locked(edge)
208208
}
209209
}
210210

@@ -220,19 +220,17 @@ func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toN
220220

221221
// delete all edges between vertices of fromType and toVert
222222
neighborsToRemove := []*namedVertex{}
223-
neighborsToRecompute := []*namedVertex{}
223+
edgesToRemove := []graph.Edge{}
224224
g.graph.VisitTo(toVert, func(from graph.Node) bool {
225225
fromVert := from.(*namedVertex)
226226
if fromVert.vertexType != fromType {
227227
return true
228228
}
229-
// remove the edge
230-
g.graph.RemoveEdge(simple.Edge{F: fromVert, T: toVert})
231-
// track vertexes that changed edges
232-
if g.graph.Degree(fromVert) == 0 {
229+
// this neighbor has only one edge (which must be to us), so remove them as well
230+
if g.graph.Degree(fromVert) == 1 {
233231
neighborsToRemove = append(neighborsToRemove, fromVert)
234232
} else {
235-
neighborsToRecompute = append(neighborsToRecompute, fromVert)
233+
edgesToRemove = append(edgesToRemove, g.graph.EdgeBetween(from, toVert))
236234
}
237235
return true
238236
})
@@ -242,9 +240,30 @@ func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toN
242240
g.removeVertex_locked(v)
243241
}
244242

245-
// recompute destination indexes for neighbors that dropped outbound edges
246-
for _, v := range neighborsToRecompute {
247-
g.recomputeDestinationIndex_locked(v)
243+
// remove edges and decrement destination indexes for neighbors that dropped outbound edges
244+
for _, edge := range edgesToRemove {
245+
g.graph.RemoveEdge(edge)
246+
g.removeEdgeFromDestinationIndex_locked(edge)
247+
}
248+
}
249+
250+
// A fastpath for recomputeDestinationIndex_locked for "removing edge" case.
251+
func (g *Graph) removeEdgeFromDestinationIndex_locked(e graph.Edge) {
252+
n := e.From()
253+
// don't maintain indices for nodes with few edges
254+
edgeCount := g.graph.Degree(n)
255+
if edgeCount < g.destinationEdgeThreshold {
256+
delete(g.destinationEdgeIndex, n.ID())
257+
return
258+
}
259+
260+
// decrement the nodeID->destinationID refcount in the index, if the index exists
261+
index := g.destinationEdgeIndex[n.ID()]
262+
if index == nil {
263+
return
264+
}
265+
if destinationEdge, ok := e.(*destinationEdge); ok {
266+
index.decrement(destinationEdge.DestinationID())
248267
}
249268
}
250269

@@ -259,7 +278,7 @@ func (g *Graph) addEdgeToDestinationIndex_locked(e graph.Edge) {
259278
}
260279
// fast-add the new edge to an existing index
261280
if destinationEdge, ok := e.(*destinationEdge); ok {
262-
index.mark(destinationEdge.DestinationID())
281+
index.increment(destinationEdge.DestinationID())
263282
}
264283
}
265284

@@ -290,25 +309,17 @@ func (g *Graph) recomputeDestinationIndex_locked(n graph.Node) {
290309
if index == nil {
291310
index = newIntSet()
292311
} else {
293-
index.startNewGeneration()
312+
index.reset()
294313
}
295314

296315
// populate the index
297316
g.graph.VisitFrom(n, func(dest graph.Node) bool {
298317
if destinationEdge, ok := g.graph.EdgeBetween(n, dest).(*destinationEdge); ok {
299-
index.mark(destinationEdge.DestinationID())
318+
index.increment(destinationEdge.DestinationID())
300319
}
301320
return true
302321
})
303-
304-
// remove existing items no longer in the list
305-
index.sweep()
306-
307-
if len(index.members) < g.destinationEdgeThreshold {
308-
delete(g.destinationEdgeIndex, n.ID())
309-
} else {
310-
g.destinationEdgeIndex[n.ID()] = index
311-
}
322+
g.destinationEdgeIndex[n.ID()] = index
312323
}
313324

314325
// AddPod should only be called once spec.NodeName is populated.
@@ -451,7 +462,9 @@ func (g *Graph) SetNodeConfigMap(nodeName, configMapName, configMapNamespace str
451462
if len(configMapName) > 0 && len(configMapNamespace) > 0 {
452463
configmapVertex := g.getOrCreateVertex_locked(configMapVertexType, configMapNamespace, configMapName)
453464
nodeVertex := g.getOrCreateVertex_locked(nodeVertexType, "", nodeName)
454-
g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex, nodeVertex))
465+
e := newDestinationEdge(configmapVertex, nodeVertex, nodeVertex)
466+
g.graph.SetEdge(e)
467+
g.addEdgeToDestinationIndex_locked(e)
455468
}
456469

457470
}

‎plugin/pkg/auth/authorizer/node/graph_test.go‎

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,8 @@ func TestIndex(t *testing.T) {
247247
actual := map[string][]string{}
248248
for from, to := range g.destinationEdgeIndex {
249249
sortedValues := []string{}
250-
for member := range to.members {
251-
sortedValues = append(sortedValues, toString(member))
250+
for member, count := range to.members {
251+
sortedValues = append(sortedValues, fmt.Sprintf("%s=%d", toString(member), count))
252252
}
253253
sort.Strings(sortedValues)
254254
actual[toString(from)] = sortedValues
@@ -280,10 +280,10 @@ func TestIndex(t *testing.T) {
280280
"serviceAccount:ns/sa1": {"pod:ns/pod1", "pod:ns/pod2", "pod:ns/pod3"},
281281
})
282282
expectIndex(map[string][]string{
283-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
284-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
285-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
286-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
283+
"configmap:ns/cm1": {"node:node1=1", "node:node2=1", "node:node3=1"},
284+
"configmap:ns/cm2": {"node:node1=1", "node:node2=1", "node:node3=1"},
285+
"configmap:ns/cm3": {"node:node1=1", "node:node2=1", "node:node3=1"},
286+
"serviceAccount:ns/sa1": {"node:node1=1", "node:node2=1", "node:node3=1"},
287287
})
288288

289289
// delete one to drop below the threshold
@@ -317,10 +317,10 @@ func TestIndex(t *testing.T) {
317317
"serviceAccount:ns/sa1": {"pod:ns/pod1", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
318318
})
319319
expectIndex(map[string][]string{
320-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
321-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
322-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
323-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
320+
"configmap:ns/cm1": {"node:node1=2", "node:node2=1", "node:node3=1"},
321+
"configmap:ns/cm2": {"node:node1=2", "node:node2=1", "node:node3=1"},
322+
"configmap:ns/cm3": {"node:node1=2", "node:node2=1", "node:node3=1"},
323+
"serviceAccount:ns/sa1": {"node:node1=2", "node:node2=1", "node:node3=1"},
324324
})
325325

326326
// delete one to remain above the threshold
@@ -338,33 +338,35 @@ func TestIndex(t *testing.T) {
338338
"serviceAccount:ns/sa1": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
339339
})
340340
expectIndex(map[string][]string{
341-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
342-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
343-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
344-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
341+
"configmap:ns/cm1": {"node:node1=1", "node:node2=1", "node:node3=1"},
342+
"configmap:ns/cm2": {"node:node1=1", "node:node2=1", "node:node3=1"},
343+
"configmap:ns/cm3": {"node:node1=1", "node:node2=1", "node:node3=1"},
344+
"serviceAccount:ns/sa1": {"node:node1=1", "node:node2=1", "node:node3=1"},
345345
})
346346

347347
// Set node->configmap references
348348
g.SetNodeConfigMap("node1", "cm1", "ns")
349349
g.SetNodeConfigMap("node2", "cm1", "ns")
350350
g.SetNodeConfigMap("node3", "cm1", "ns")
351+
g.SetNodeConfigMap("node4", "cm1", "ns")
351352
expectGraph(map[string][]string{
352353
"node:node1": {},
353354
"node:node2": {},
354355
"node:node3": {},
356+
"node:node4": {},
355357
"pod:ns/pod2": {"node:node2"},
356358
"pod:ns/pod3": {"node:node3"},
357359
"pod:ns/pod4": {"node:node1"},
358-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
360+
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3", "node:node4", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
359361
"configmap:ns/cm2": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
360362
"configmap:ns/cm3": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
361363
"serviceAccount:ns/sa1": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
362364
})
363365
expectIndex(map[string][]string{
364-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
365-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
366-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
367-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
366+
"configmap:ns/cm1": {"node:node1=2", "node:node2=2", "node:node3=2", "node:node4=1"},
367+
"configmap:ns/cm2": {"node:node1=1", "node:node2=1", "node:node3=1"},
368+
"configmap:ns/cm3": {"node:node1=1", "node:node2=1", "node:node3=1"},
369+
"serviceAccount:ns/sa1": {"node:node1=1", "node:node2=1", "node:node3=1"},
368370
})
369371

370372
// Update node->configmap reference
@@ -373,27 +375,30 @@ func TestIndex(t *testing.T) {
373375
"node:node1": {},
374376
"node:node2": {},
375377
"node:node3": {},
378+
"node:node4": {},
376379
"pod:ns/pod2": {"node:node2"},
377380
"pod:ns/pod3": {"node:node3"},
378381
"pod:ns/pod4": {"node:node1"},
379-
"configmap:ns/cm1": {"node:node2", "node:node3", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
382+
"configmap:ns/cm1": {"node:node2", "node:node3", "node:node4", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
380383
"configmap:ns/cm2": {"node:node1", "pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
381384
"configmap:ns/cm3": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
382385
"serviceAccount:ns/sa1": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
383386
})
384387
expectIndex(map[string][]string{
385-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
386-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
387-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
388-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
388+
"configmap:ns/cm1": {"node:node1=1", "node:node2=2", "node:node3=2", "node:node4=1"},
389+
"configmap:ns/cm2": {"node:node1=2", "node:node2=1", "node:node3=1"},
390+
"configmap:ns/cm3": {"node:node1=1", "node:node2=1", "node:node3=1"},
391+
"serviceAccount:ns/sa1": {"node:node1=1", "node:node2=1", "node:node3=1"},
389392
})
390393

391394
// Remove node->configmap reference
392395
g.SetNodeConfigMap("node1", "", "")
396+
g.SetNodeConfigMap("node4", "", "")
393397
expectGraph(map[string][]string{
394398
"node:node1": {},
395399
"node:node2": {},
396400
"node:node3": {},
401+
"node:node4": {},
397402
"pod:ns/pod2": {"node:node2"},
398403
"pod:ns/pod3": {"node:node3"},
399404
"pod:ns/pod4": {"node:node1"},
@@ -403,9 +408,9 @@ func TestIndex(t *testing.T) {
403408
"serviceAccount:ns/sa1": {"pod:ns/pod2", "pod:ns/pod3", "pod:ns/pod4"},
404409
})
405410
expectIndex(map[string][]string{
406-
"configmap:ns/cm1": {"node:node1", "node:node2", "node:node3"},
407-
"configmap:ns/cm2": {"node:node1", "node:node2", "node:node3"},
408-
"configmap:ns/cm3": {"node:node1", "node:node2", "node:node3"},
409-
"serviceAccount:ns/sa1": {"node:node1", "node:node2", "node:node3"},
411+
"configmap:ns/cm1": {"node:node1=1", "node:node2=2", "node:node3=2"},
412+
"configmap:ns/cm2": {"node:node1=1", "node:node2=1", "node:node3=1"},
413+
"configmap:ns/cm3": {"node:node1=1", "node:node2=1", "node:node3=1"},
414+
"serviceAccount:ns/sa1": {"node:node1=1", "node:node2=1", "node:node3=1"},
410415
})
411416
}

‎plugin/pkg/auth/authorizer/node/intset.go‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,47 +16,47 @@ limitations under the License.
1616

1717
package node
1818

19-
// intSet maintains a set of ints, and supports promoting and culling the previous generation.
20-
// this allows tracking a large, mostly-stable set without constantly reallocating the entire set.
19+
// intSet maintains a map of id to refcounts
2120
type intSet struct {
22-
currentGeneration byte
23-
members map[int]byte
21+
// members is a map of id to refcounts
22+
members map[int]int
2423
}
2524

2625
func newIntSet() *intSet {
27-
return &intSet{members: map[int]byte{}}
26+
return &intSet{members: map[int]int{}}
2827
}
2928

30-
// has returns true if the specified int is in the set.
29+
// has returns true if the specified id has a positive refcount.
3130
// it is safe to call concurrently, but must not be called concurrently with any of the other methods.
3231
func (s *intSet) has(i int) bool {
3332
if s == nil {
3433
return false
3534
}
36-
_, present := s.members[i]
37-
return present
35+
return s.members[i] > 0
3836
}
3937

40-
// startNewGeneration begins a new generation.
41-
// it must be followed by a call to mark() for every member of the generation,
42-
// then a call to sweep() to remove members not present in the generation.
38+
// reset removes all ids, effectively setting their refcounts to 0.
4339
// it is not thread-safe.
44-
func (s *intSet) startNewGeneration() {
45-
s.currentGeneration++
40+
func (s *intSet) reset() {
41+
for k := range s.members {
42+
delete(s.members, k)
43+
}
4644
}
4745

48-
// mark indicates the specified int belongs to the current generation.
46+
// increment adds one to the refcount of the specified id.
4947
// it is not thread-safe.
50-
func (s *intSet) mark(i int) {
51-
s.members[i] = s.currentGeneration
48+
func (s *intSet) increment(i int) {
49+
s.members[i]++
5250
}
5351

54-
// sweep removes items not in the current generation.
52+
// decrement removes one from the refcount of the specified id,
53+
// and removes the id if the resulting refcount is <= 0.
54+
// it will not track refcounts lower than zero.
5555
// it is not thread-safe.
56-
func (s *intSet) sweep() {
57-
for k, v := range s.members {
58-
if v != s.currentGeneration {
59-
delete(s.members, k)
60-
}
56+
func (s *intSet) decrement(i int) {
57+
if s.members[i] <= 1 {
58+
delete(s.members, i)
59+
} else {
60+
s.members[i]--
6161
}
6262
}

‎plugin/pkg/auth/authorizer/node/intset_test.go‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,33 +30,36 @@ func TestIntSet(t *testing.T) {
3030
assert.False(t, i.has(3))
3131
assert.False(t, i.has(4))
3232

33-
i.startNewGeneration()
34-
i.mark(1)
35-
i.mark(2)
36-
i.sweep()
33+
i.reset()
34+
i.increment(1) // to 1
35+
i.increment(2) // to 1
3736

3837
assert.True(t, i.has(1))
3938
assert.True(t, i.has(2))
4039
assert.False(t, i.has(3))
4140
assert.False(t, i.has(4))
4241

43-
i.startNewGeneration()
44-
i.mark(2)
45-
i.mark(3)
46-
i.sweep()
42+
i.decrement(1) // to 0
43+
i.increment(3) // to 1
4744

48-
assert.False(t, i.has(1))
49-
assert.True(t, i.has(2))
50-
assert.True(t, i.has(3))
51-
assert.False(t, i.has(4))
45+
assert.False(t, i.has(1)) // removed
46+
assert.True(t, i.has(2)) // still present
47+
assert.True(t, i.has(3)) // added
48+
assert.False(t, i.has(4)) // not yet present
5249

53-
i.startNewGeneration()
54-
i.mark(3)
55-
i.mark(4)
56-
i.sweep()
50+
i.decrement(2) // to 0
51+
i.increment(3) // to 2
52+
i.decrement(3) // to 1
53+
i.increment(4) // to 1
5754

5855
assert.False(t, i.has(1))
5956
assert.False(t, i.has(2))
6057
assert.True(t, i.has(3))
6158
assert.True(t, i.has(4))
59+
60+
i.reset()
61+
assert.False(t, i.has(1))
62+
assert.False(t, i.has(2))
63+
assert.False(t, i.has(3))
64+
assert.False(t, i.has(4))
6265
}

0 commit comments

Comments
 (0)