Address comments from reviews

Author: hongkailiu

pkg/updatestatus/controlplaneinformer.go

@@ -62,9 +62,9 @@ func newControlPlaneInformerController(
 
 	controller := factory.New().
 		// call sync on ClusterVersion changes
-		WithInformersQueueKeysFunc(configApiQueueKeys, cvInformer).
+		WithInformersQueueKeysFunc(controlPlaneInformerQueueKeys, cvInformer).
 		// call sync on ClusterOperator changes with a filter
-		WithFilteredEventsInformersQueueKeysFunc(configApiQueueKeys, clusterOperatorEventFilterFunc, coInformer).
+		WithFilteredEventsInformersQueueKeysFunc(controlPlaneInformerQueueKeys, clusterOperatorEventFilterFunc, coInformer).
 		WithSync(c.sync).
 		ToController("ControlPlaneInformer", c.recorder)
 
@@ -97,7 +97,7 @@ const (
 func (c *controlPlaneInformerController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
 	queueKey := syncCtx.QueueKey()
 
-	t, name, err := parseQueueKey(queueKey)
+	t, name, err := parseControlPlaneInformerQueueKey(queueKey)
 	if err != nil {
 		return fmt.Errorf("failed to parse queue key: %w", err)
 	}
@@ -470,15 +470,15 @@ func versionsFromHistory(history []configv1.UpdateHistory) ControlPlaneUpdateVer
 	return versions
 }
 
-func parseQueueKey(queueKey string) (string, string, error) {
+func parseControlPlaneInformerQueueKey(queueKey string) (string, string, error) {
 	splits := strings.Split(queueKey, "/")
 	if len(splits) != 2 {
 		return "", "", fmt.Errorf("invalid queue key: %s", queueKey)
 	}
 	return splits[0], splits[1], nil
 }
 
-func configApiQueueKeys(object runtime.Object) []string {
+func controlPlaneInformerQueueKeys(object runtime.Object) []string {
 	if object == nil {
 		return nil
 	}

pkg/updatestatus/controlplaneinformer_test.go

@@ -215,7 +215,7 @@ func Test_sync_with_cv(t *testing.T) {
 				now:             func() metav1.Time { return now },
 			}
 
-			queueKey := configApiQueueKeys(cv)[0]
+			queueKey := controlPlaneInformerQueueKeys(cv)[0]
 
 			err := controller.sync(context.Background(), newTestSyncContext(queueKey))
 			if err != nil {
@@ -391,14 +391,14 @@ func Test_configApiQueueKeys(t *testing.T) {
 				}
 			}()
 
-			actual := configApiQueueKeys(tc.object)
+			actual := controlPlaneInformerQueueKeys(tc.object)
 
 			if diff := cmp.Diff(tc.expected, actual); diff != "" {
 				t.Errorf("%s: key differs from expected:\n%s", tc.name, diff)
 			}
 
 			if !tc.expectedPanic && len(actual) > 0 {
-				kind, name, err := parseQueueKey(actual[0])
+				kind, name, err := parseControlPlaneInformerQueueKey(actual[0])
 				if err != nil {
 					t.Errorf("%s: unexpected error raised:\n%v", tc.name, err)
 				}
@@ -504,7 +504,7 @@ func Test_sync_with_co(t *testing.T) {
 				now:              func() metav1.Time { return now },
 			}
 
-			queueKey := configApiQueueKeys(co)[0]
+			queueKey := controlPlaneInformerQueueKeys(co)[0]
 
 			err := controller.sync(context.Background(), newTestSyncContext(queueKey))
 			if err != nil {

pkg/updatestatus/nodeinformer.go

@@ -77,7 +77,7 @@ func newNodeInformerController(
 func (c *nodeInformerController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
 	queueKey := syncCtx.QueueKey()
 
-	t, name, err := parseQueueKey(queueKey)
+	t, name, err := parseNodeInformerControllerQueueKey(queueKey)
 	if err != nil {
 		return fmt.Errorf("failed to parse queue key: %w", err)
 	}
@@ -135,7 +135,7 @@ func (c *nodeInformerController) sync(ctx context.Context, syncCtx factory.SyncC
 	if klog.V(4).Enabled() {
 		msgForLog = fmt.Sprintf(" | msg=%s", string(msg.insight))
 	}
-	klog.V(2).Infof("CPI :: Syncing %s %s%s", t, name, msgForLog)
+	klog.V(2).Infof("NI :: Syncing %s %s%s", t, name, msgForLog)
 	c.sendInsight(msg)
 	return nil
 }
@@ -328,7 +328,9 @@ func assessNode(node *corev1.Node, mcp *machineconfigv1.MachineConfigPool, machi
 	}
 
 	desiredConfig, ok := node.Annotations[mco.DesiredMachineConfigAnnotationKey]
-	currentVersion, foundCurrent := machineConfigVersions[node.Annotations[mco.CurrentMachineConfigAnnotationKey]]
+	noDesiredOnNode := !ok
+	currentConfig := node.Annotations[mco.CurrentMachineConfigAnnotationKey]
+	currentVersion, foundCurrent := machineConfigVersions[currentConfig]
 	desiredVersion, foundDesired := machineConfigVersions[desiredConfig]
 
 	lns := mco.NewLayeredNodeState(node)
@@ -337,7 +339,7 @@ func assessNode(node *corev1.Node, mcp *machineconfigv1.MachineConfigPool, machi
 	isDegraded := isNodeDegraded(node)
 	isUpdated := foundCurrent && mostRecentVersionInCVHistory == currentVersion &&
 		// The following condition is to handle the multi-arch migration because the version number stays the same there
-		(!ok || node.Annotations[mco.CurrentMachineConfigAnnotationKey] == desiredConfig)
+		(noDesiredOnNode || currentConfig == desiredConfig)
 
 	// foundCurrent makes sure we don't blip phase "updating" for nodes that we are not sure
 	// of their actual phase, even though the conservative assumption is that the node is
@@ -377,6 +379,14 @@ const (
 	nodeKindName = "Node"
 )
 
+func parseNodeInformerControllerQueueKey(queueKey string) (string, string, error) {
+	splits := strings.Split(queueKey, "/")
+	if len(splits) != 2 {
+		return "", "", fmt.Errorf("invalid queue key: %s", queueKey)
+	}
+	return splits[0], splits[1], nil
+}
+
 func nodeInformerControllerQueueKeys(object runtime.Object) []string {
 	if object == nil {
 		return nil

pkg/updatestatus/nodeinformer_test.go

@@ -70,7 +70,7 @@ func Test_nodeInformerControllerQueueKeys(t *testing.T) {
 			}
 
 			if !tc.expectedPanic && len(actual) > 0 {
-				kind, name, err := parseQueueKey(actual[0])
+				kind, name, err := parseNodeInformerControllerQueueKey(actual[0])
 				if err != nil {
 					t.Errorf("%s: unexpected error raised:\n%v", tc.name, err)
 				}