Skip to content

Commit 998d96c

Browse files
committed
Move sync.Once into nodeInformerController controller level
As a result each nodeInformerController instance will execute the function once which makes more sense than doing it once grobally because what we do there is initialization of the caches that are associated with the instance. For the core code, it is not a big deal since we have only ONE instance of the controller. However, it matters for unit tests where there are many controllers.
1 parent a81ec69 commit 998d96c

File tree

2 files changed

+26
-23
lines changed

2 files changed

+26
-23
lines changed

pkg/updatestatus/nodeinformer.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ type nodeInformerController struct {
4040
// sendInsight should be called to send produced insights to the update status controller
4141
sendInsight sendInsightFn
4242

43+
// once does the tasks that need to be executed once and only once at the beginning of its sync function
44+
// for each nodeInformerController instance, e.g., initializing caches.
45+
once sync.Once
46+
4347
// mcpSelectors caches the label selectors converted from the node selectors of the machine config pools by their names.
4448
mcpSelectors machineConfigPoolSelectorCache
4549

@@ -88,15 +92,13 @@ func newNodeInformerController(
8892
return controller
8993
}
9094

91-
var once sync.Once
92-
9395
func (c *nodeInformerController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
9496
// Warm up controller's caches.
9597
// This has to be called after informers caches have been synced and before the first event comes in.
9698
// The existing openshift-library does not provide such a hook.
9799
// In case any error occurs during cache initialization, we can still proceed which leads to stale insights that
98100
// will be corrected on the reconciliation when the caches are warmed up.
99-
once.Do(func() {
101+
c.once.Do(func() {
100102
if err := c.initializeMachineConfigPools(); err != nil {
101103
klog.Errorf("Failed to initialize machineConfigPoolSelectorCache: %v", err)
102104
}

pkg/updatestatus/nodeinformer_test.go

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,9 @@ func Test_whichMCP(t *testing.T) {
210210

211211
c := &nodeInformerController{machineConfigs: mcLister, machineConfigPools: mcpLister}
212212

213-
if err := initializeCaches(c); err != nil {
214-
t.Errorf("Failed to initialize caches: %v", err)
213+
// This is to initialize the caches
214+
if err := syncOnceWithAnUnrecognizableKey(c); err == nil {
215+
t.Fatalf("Expected sync error did not occur")
215216
}
216217

217218
if diff := cmp.Diff(tc.expected, c.mcpSelectors.whichMCP(tc.labels)); diff != "" {
@@ -222,11 +223,9 @@ func Test_whichMCP(t *testing.T) {
222223
}
223224
}
224225

225-
func initializeCaches(c *nodeInformerController) error {
226-
if err := c.initializeMachineConfigVersions(); err != nil {
227-
return err
228-
}
229-
return c.initializeMachineConfigPools()
226+
func syncOnceWithAnUnrecognizableKey(c *nodeInformerController) error {
227+
syncOnceContext := newTestSyncContext("sync/once")
228+
return c.sync(context.TODO(), syncOnceContext)
230229
}
231230

232231
func Test_assessNode(t *testing.T) {
@@ -1078,10 +1077,6 @@ func Test_sync_with_node(t *testing.T) {
10781077
now: func() metav1.Time { return now },
10791078
}
10801079

1081-
if err := initializeCaches(controller); err != nil {
1082-
t.Errorf("Failed to initialize caches: %v", err)
1083-
}
1084-
10851080
queueKey := nodeInformerControllerQueueKeys(tc.node)[0]
10861081

10871082
actualErr := controller.sync(context.TODO(), newTestSyncContext(queueKey))
@@ -1136,6 +1131,12 @@ func Test_sync_with_event(t *testing.T) {
11361131
}
11371132
nodeLister := corelistersv1.NewNodeLister(nodeIndexer)
11381133

1134+
mcIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
1135+
mcLister := machineconfigv1listers.NewMachineConfigLister(mcIndexer)
1136+
1137+
mcpIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
1138+
mcpLister := machineconfigv1listers.NewMachineConfigPoolLister(mcpIndexer)
1139+
11391140
testCases := []struct {
11401141
name string
11411142

@@ -1172,8 +1173,10 @@ func Test_sync_with_event(t *testing.T) {
11721173
}
11731174

11741175
controller := &nodeInformerController{
1175-
nodes: nodeLister,
1176-
sendInsight: sendInsight,
1176+
nodes: nodeLister,
1177+
machineConfigs: mcLister,
1178+
machineConfigPools: mcpLister,
1179+
sendInsight: sendInsight,
11771180
}
11781181
queueKey := nodeInformerControllerQueueKeys(tc.object)[0]
11791182

@@ -1205,11 +1208,6 @@ func Test_sync_with_mcp(t *testing.T) {
12051208
now := metav1.Now()
12061209

12071210
mcIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
1208-
for _, o := range []metav1.Object{} {
1209-
if err := mcIndexer.Add(o); err != nil {
1210-
t.Fatalf("Failed to add object to indexer: %v", err)
1211-
}
1212-
}
12131211
mcLister := machineconfigv1listers.NewMachineConfigLister(mcIndexer)
12141212

12151213
nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
@@ -1297,8 +1295,11 @@ func Test_sync_with_mcp(t *testing.T) {
12971295
now: func() metav1.Time { return now },
12981296
}
12991297

1300-
if err := initializeCaches(controller); err != nil {
1301-
t.Errorf("Failed to initialize caches: %v", err)
1298+
if tc.mcpToRemove != "" || tc.mcpToAdd != "" {
1299+
// This is to initialize the caches
1300+
if err := syncOnceWithAnUnrecognizableKey(controller); err == nil {
1301+
t.Fatalf("Expected sync error did not occur")
1302+
}
13021303
}
13031304

13041305
if tc.mcpToRemove != "" {

0 commit comments

Comments
 (0)