Skip to content

Commit 61cdbdc

Browse files
AnnaYueclaude
andcommitted
fix(resourcetopo): fix lint issues — misspell 'cancelled'→'canceled', shadow builtin 'min', and gofmt formatting
Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 04bdb91 commit 61cdbdc

3 files changed

Lines changed: 74 additions & 60 deletions

File tree

resourcetopo/doc.go

Lines changed: 40 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
// resources. It maintains relationships between different resource types and notifies
1919
// handlers when resources or their relationships change.
2020
//
21-
// Lock Hierarchy
21+
// # Lock Hierarchy
2222
//
2323
// To prevent deadlocks, locks must be acquired in the following order:
2424
//
@@ -50,55 +50,54 @@
5050
// - Protects: nodeUpdateHandler, relationUpdateHandler
5151
// - Independent - can be acquired anytime
5252
//
53-
// Threading Model
53+
// # Threading Model
5454
//
5555
// This package uses multiple goroutines for concurrent processing. Understanding the
5656
// threading model is essential for avoiding race conditions and deadlocks.
5757
//
5858
// 1. Informer Threads (External):
59-
// - Source: Kubernetes informers (one per resource type)
60-
// - Entry points: nodeStorage.OnAdd, OnUpdate, OnDelete
61-
// - Purpose: Receives resource change events from API server
62-
// - Lock behavior: Acquires metaLock.Lock() during updateNodeMeta(), relationsLock during relation updates
63-
// - Note: These are the ONLY threads that write to metaLock (labels, ownerRefs, objectExisted)
59+
// - Source: Kubernetes informers (one per resource type)
60+
// - Entry points: nodeStorage.OnAdd, OnUpdate, OnDelete
61+
// - Purpose: Receives resource change events from API server
62+
// - Lock behavior: Acquires metaLock.Lock() during updateNodeMeta(), relationsLock during relation updates
63+
// - Note: These are the ONLY threads that write to metaLock (labels, ownerRefs, objectExisted)
6464
//
6565
// 2. Event Processor Threads (Internal):
66-
// - Source: Created by Start() -> startHandleEvent()
67-
// - Count: 2 goroutines
68-
// - handleNodeEvent(): Processes node add/update/delete/relatedUpdate events
69-
// - handleRelationEvent(): Processes relation add/delete events
70-
// - Trigger: newNodeEvent(), newRelationEvent() queue events via workqueue
71-
// - Lock behavior: Read-only access to handlers via RLock, no direct node locking
72-
// - Note: Reads handlers while holding no locks - relies on handlersLock for registration
66+
// - Source: Created by Start() -> startHandleEvent()
67+
// - Count: 2 goroutines
68+
// - handleNodeEvent(): Processes node add/update/delete/relatedUpdate events
69+
// - handleRelationEvent(): Processes relation add/delete events
70+
// - Trigger: newNodeEvent(), newRelationEvent() queue events via workqueue
71+
// - Lock behavior: Read-only access to handlers via RLock, no direct node locking
72+
// - Note: Reads handlers while holding no locks - relies on handlersLock for registration
7373
//
7474
// 3. User/Client Threads (External):
75-
// - Source: User code calling Manager methods
76-
// - Examples: GetNode(), GetTopoNodeStorage(), AddNodeHandler()
77-
// - Lock behavior: Uses storagesLock.RLock() for reads
78-
// - Note: These are typically short-lived operations
75+
// - Source: User code calling Manager methods
76+
// - Examples: GetNode(), GetTopoNodeStorage(), AddNodeHandler()
77+
// - Lock behavior: Uses storagesLock.RLock() for reads
78+
// - Note: These are typically short-lived operations
7979
//
8080
// 4. Callback Threads (External - via Event Processors):
81-
// - Source: User-provided NodeHandler and RelationHandler callbacks
82-
// - Entry: Called by handleNodeEvent/handleRelationEvent
83-
// - Execution: Each callback runs in its own goroutine with a configurable timeout
84-
// (ManagerConfig.EventHandlerTimeout, default 1s). If a callback does not return
85-
// within the timeout, a warning is logged and all remaining handlers for that
86-
// event are skipped.
87-
// - Lock behavior: No resourcetopo locks held during callback execution
88-
// - Concurrency: Multiple callbacks (across different handlers or events) may run
89-
// concurrently. Handler implementations that share mutable state must be
90-
// thread-safe.
91-
// - Safety: Handlers receive node references but should not cache them long-term
92-
//
93-
// Thread Safety Guidelines
94-
//
95-
// - NodeInfo references returned by GetNode() are safe for concurrent reads
96-
// - Do NOT call AddTopologyConfig() after Start() - not thread safe
97-
// - Handler registration (AddNodeHandler, AddRelationHandler) is thread safe
98-
// - Callbacks (OnAdd, OnUpdate, etc.) run in goroutines with a timeout.
99-
// Do not block indefinitely — timed-out callbacks cause remaining handlers
100-
// to be skipped. Implementations must be thread-safe if they share state.
101-
// - nodeInfo.lock can be held for extended periods during relation changes
102-
// - metaLock is never held during callbacks or cross-node operations
103-
//
81+
// - Source: User-provided NodeHandler and RelationHandler callbacks
82+
// - Entry: Called by handleNodeEvent/handleRelationEvent
83+
// - Execution: Each callback runs in its own goroutine with a configurable timeout
84+
// (ManagerConfig.EventHandlerTimeout, default 1s). If a callback does not return
85+
// within the timeout, a warning is logged and all remaining handlers for that
86+
// event are skipped.
87+
// - Lock behavior: No resourcetopo locks held during callback execution
88+
// - Concurrency: Multiple callbacks (across different handlers or events) may run
89+
// concurrently. Handler implementations that share mutable state must be
90+
// thread-safe.
91+
// - Safety: Handlers receive node references but should not cache them long-term
92+
//
93+
// # Thread Safety Guidelines
94+
//
95+
// - NodeInfo references returned by GetNode() are safe for concurrent reads
96+
// - Do NOT call AddTopologyConfig() after Start() - not thread safe
97+
// - Handler registration (AddNodeHandler, AddRelationHandler) is thread safe
98+
// - Callbacks (OnAdd, OnUpdate, etc.) run in goroutines with a timeout.
99+
// Do not block indefinitely — timed-out callbacks cause remaining handlers
100+
// to be skipped. Implementations must be thread-safe if they share state.
101+
// - nodeInfo.lock can be held for extended periods during relation changes
102+
// - metaLock is never held during callbacks or cross-node operations
104103
package resourcetopo

resourcetopo/resourcetopo_test.go

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package resourcetopo
1919
import (
2020
"context"
2121
"fmt"
22+
"runtime"
2223
"sync"
2324
"testing"
2425
"time"
@@ -30,7 +31,6 @@ import (
3031
"k8s.io/apimachinery/pkg/types"
3132
"k8s.io/client-go/informers"
3233
"k8s.io/client-go/kubernetes/fake"
33-
"runtime"
3434
"k8s.io/klog/v2"
3535
)
3636

@@ -2023,8 +2023,8 @@ var _ = Describe("test suite for multi routine", func() {
20232023
g.Expect(len(rsNode.GetPostOrders())).To(Equal(totalNum / len(rsNames)))
20242024
}
20252025
}, "10s").Should(Succeed())
2026-
})
20272026
})
2027+
})
20282028

20292029
var _ = Describe("test handler timeout protection", func() {
20302030
var manager Manager
@@ -2127,18 +2127,21 @@ func (h *blockingNodeHandler) OnAdd(ctx context.Context, info NodeInfo) {
21272127
case <-ctx.Done():
21282128
}
21292129
}
2130+
21302131
func (h *blockingNodeHandler) OnUpdate(ctx context.Context, info NodeInfo) {
21312132
select {
21322133
case <-h.blockCh:
21332134
case <-ctx.Done():
21342135
}
21352136
}
2137+
21362138
func (h *blockingNodeHandler) OnDelete(ctx context.Context, info NodeInfo) {
21372139
select {
21382140
case <-h.blockCh:
21392141
case <-ctx.Done():
21402142
}
21412143
}
2144+
21422145
func (h *blockingNodeHandler) OnRelatedUpdate(ctx context.Context, info NodeInfo) {
21432146
select {
21442147
case <-h.blockCh:
@@ -2159,6 +2162,7 @@ func (h *blockingRelationHandler) OnAdd(ctx context.Context, preNode, postNode N
21592162
case <-ctx.Done():
21602163
}
21612164
}
2165+
21622166
func (h *blockingRelationHandler) OnDelete(ctx context.Context, preNode, postNode NodeInfo) {
21632167
select {
21642168
case <-h.blockCh:
@@ -2441,7 +2445,7 @@ var _ = Describe("test goroutine leak and panic handling", func() {
24412445
crName := "crNoLeak"
24422446

24432447
// Register a cooperative blocking handler for ClusterRole
2444-
// (selects on ctx.Done() so it returns when context is cancelled)
2448+
// (selects on ctx.Done() so it returns when context is canceled)
24452449
blockCh := make(chan struct{})
24462450
blockingHandler := &blockingNodeHandler{blockCh: blockCh}
24472451
Expect(manager.AddNodeHandler(ClusterRoleMeta, blockingHandler)).To(Succeed())
@@ -2458,7 +2462,7 @@ var _ = Describe("test goroutine leak and panic handling", func() {
24582462
// Count baseline goroutines
24592463
baseline := countStableGoroutines(3, 100*time.Millisecond)
24602464

2461-
// Trigger handler — ctx will be cancelled after timeout, handler returns
2465+
// Trigger handler — ctx will be canceled after timeout, handler returns
24622466
Expect(fakeClient.RbacV1().ClusterRoles().Create(ctx, newClusterRole(crName), metav1.CreateOptions{})).NotTo(BeNil())
24632467

24642468
// Wait well past timeout for handler to return and goroutine to exit
@@ -2512,24 +2516,35 @@ var _ = Describe("test goroutine leak and panic handling", func() {
25122516
// countStableGoroutines samples NumGoroutine multiple times and returns
25132517
// the minimum observed count (to filter out transient goroutines).
25142518
func countStableGoroutines(samples int, interval time.Duration) int {
2515-
min := runtime.NumGoroutine()
2519+
minGoroutines := runtime.NumGoroutine()
25162520
for i := 1; i < samples; i++ {
25172521
time.Sleep(interval)
25182522
n := runtime.NumGoroutine()
2519-
if n < min {
2520-
min = n
2523+
if n < minGoroutines {
2524+
minGoroutines = n
25212525
}
25222526
}
2523-
return min
2527+
return minGoroutines
25242528
}
25252529

25262530
// panickingNodeHandler panics on every callback, used to test panic absorption.
25272531
type panickingNodeHandler struct{}
25282532

2529-
func (h *panickingNodeHandler) OnAdd(ctx context.Context, info NodeInfo) { panic("intentional panic in OnAdd") }
2530-
func (h *panickingNodeHandler) OnUpdate(ctx context.Context, info NodeInfo) { panic("intentional panic in OnUpdate") }
2531-
func (h *panickingNodeHandler) OnDelete(ctx context.Context, info NodeInfo) { panic("intentional panic in OnDelete") }
2532-
func (h *panickingNodeHandler) OnRelatedUpdate(ctx context.Context, info NodeInfo) { panic("intentional panic in OnRelatedUpdate") }
2533+
func (h *panickingNodeHandler) OnAdd(ctx context.Context, info NodeInfo) {
2534+
panic("intentional panic in OnAdd")
2535+
}
2536+
2537+
func (h *panickingNodeHandler) OnUpdate(ctx context.Context, info NodeInfo) {
2538+
panic("intentional panic in OnUpdate")
2539+
}
2540+
2541+
func (h *panickingNodeHandler) OnDelete(ctx context.Context, info NodeInfo) {
2542+
panic("intentional panic in OnDelete")
2543+
}
2544+
2545+
func (h *panickingNodeHandler) OnRelatedUpdate(ctx context.Context, info NodeInfo) {
2546+
panic("intentional panic in OnRelatedUpdate")
2547+
}
25332548

25342549
var _ NodeHandler = &panickingNodeHandler{}
25352550

resourcetopo/types.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ type ManagerConfig struct {
5252
RelationEventHandleRateMaxDelay time.Duration
5353

5454
// EventHandlerTimeout is the timeout for each event handler callback.
55-
// If a handler takes longer than this timeout, it will be cancelled and
55+
// If a handler takes longer than this timeout, it will be canceled and
5656
// remaining handlers will be skipped. A warning will be logged.
5757
// Default: 1 second
5858
EventHandlerTimeout time.Duration
@@ -176,30 +176,30 @@ type ResourceRelation struct {
176176

177177
type RelationHandler interface {
178178
// OnAdd is called after relation newly added between preOrder and postOrder.
179-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
179+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
180180
OnAdd(ctx context.Context, preOrder, postOrder NodeInfo)
181181

182182
// OnDelete is called after relation deleted between preOrder and postOrder.
183-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
183+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
184184
OnDelete(ctx context.Context, preOrder, postOrder NodeInfo)
185185
}
186186

187187
type NodeHandler interface {
188188
// OnAdd is called when the node related resource object is added.
189-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
189+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
190190
OnAdd(ctx context.Context, info NodeInfo)
191191

192192
// OnUpdate is called when the node related resource object is updated.
193-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
193+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
194194
OnUpdate(ctx context.Context, info NodeInfo)
195195

196196
// OnDelete is called when the node related resource object is deleted.
197-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
197+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
198198
OnDelete(ctx context.Context, info NodeInfo)
199199

200200
// OnRelatedUpdate is called when the nodes'
201201
// post-order(or pre-order if ReverseNotice configured) object is updated.
202-
// The ctx is cancelled when the handler exceeds EventHandlerTimeout.
202+
// The ctx is canceled when the handler exceeds EventHandlerTimeout.
203203
OnRelatedUpdate(ctx context.Context, info NodeInfo)
204204
}
205205

0 commit comments

Comments
 (0)