controller: update is node done check to handle image mode disabling case

Author: isabella-janssen

pkg/controller/common/layered_node_state.go

@@ -26,17 +26,16 @@ func NewLayeredNodeState(n *corev1.Node) *LayeredNodeState {
 	return &LayeredNodeState{node: n}
 }
 
-// Checks if the node is done "working" and that the node is targeting is MachineConfig
-// determined by the pool. If in layered mode, the image annotations are also checked
-// checked against the OCL objects.
+// Checks if the node is done "working." For a node in both layered and non-layered MCPs, the
+// node's state annotation must be "Done" and it must be targeting the correct MachineConfig. For
+// `layered` MCPs, the node's desired image annotation must match the image defined in the
+// MachineOsConfig and the desired MachineConfig must match the config version defined in the
+// MachineOSBuild. For non-layered MCPs, the node must not have a desired image annotation value.
 func (l *LayeredNodeState) IsDone(mcp *mcfgv1.MachineConfigPool, layered bool, mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild) bool {
-	// if layered {
-	// 	klog.Errorf("in IsDone and layered")
-	// 	return l.IsNodeDone() && l.IsDesiredMachineConfigEqualToPool(mcp) && l.IsDesiredEqualToBuild(mosc, mosb)
-	// }
-	// klog.Errorf("in IsDone and NOT layered")
-	// return l.IsNodeDone() && l.IsDesiredMachineConfigEqualToPool(mcp)
-	return l.IsNodeDone() && l.IsDesiredMachineConfigEqualToPool(mcp) && l.IsDesiredEqualToBuild(mosc, mosb)
+	if layered {
+		return l.IsNodeDone() && l.IsDesiredMachineConfigEqualToPool(mcp) && l.IsDesiredEqualToBuild(mosc, mosb)
+	}
+	return l.IsNodeDone() && l.IsDesiredMachineConfigEqualToPool(mcp) && !l.IsDesiredImageAnnotationPresentOnNode()
 }
 
 // The original behavior of getUnavailableMachines is: getUnavailableMachines
@@ -70,28 +69,19 @@ func (l *LayeredNodeState) IsUnavailableForUpdate() bool {
 // node's desired image annotation and compares the MachineConfig specified by the
 // MachineOSBuild to the one specified by the node's desired MachineConfig annotation.
 func (l *LayeredNodeState) IsDesiredEqualToBuild(mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild) bool {
-	klog.Errorf("in IsDesiredEqualToBuild")
-	return l.isDesiredImageEqualToMachineOSConfig(mosc) && l.isDesiredMachineConfigEqualToBuild(mosb)
+	return (mosc != nil && l.isDesiredImageEqualToMachineOSConfig(mosc)) && (mosb != nil && l.isDesiredMachineConfigEqualToBuild(mosb))
 }
 
 // Compares the desired image annotation on the node against the CurrentImagePullSpec
 // specified by the MachineOSConfig object.
 func (l *LayeredNodeState) isDesiredImageEqualToMachineOSConfig(mosc *mcfgv1.MachineOSConfig) bool {
-	klog.Errorf("in isDesiredImageEqualToMachineOSConfig")
 	return l.isImageAnnotationEqualToMachineOSConfig(daemonconsts.DesiredImageAnnotationKey, mosc)
 }
 
 // Compares the MachineConfig specified by the MachineConfigPool to the MachineConfig
 // specified by the node's desired MachineConfig annotation.
 func (l *LayeredNodeState) isDesiredMachineConfigEqualToBuild(mosb *mcfgv1.MachineOSBuild) bool {
-	mosbName := ""
-	// Handle case when MOSB is nil (image mode is being disabled)
-	if mosb != nil {
-		// TODO: check if this comes back as nil or empty string
-		mosbName = mosb.Spec.MachineConfig.Name
-	}
-
-	return l.node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] == mosbName
+	return l.node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] == mosb.Spec.MachineConfig.Name
 
 }
 
@@ -104,9 +94,7 @@ func (l *LayeredNodeState) IsCurrentMachineConfigEqualToPool(mcp *mcfgv1.Machine
 // Compares the MachineConfig specified by the MachineConfigPool to the one
 // specified by the node's desired MachineConfig annotation.
 func (l *LayeredNodeState) IsDesiredMachineConfigEqualToPool(mcp *mcfgv1.MachineConfigPool) bool {
-	ret := l.node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] == mcp.Spec.Configuration.Name
-	klog.Errorf("in IsDesiredMachineConfigEqualToPool, returning %v", ret)
-	return ret
+	return l.node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] == mcp.Spec.Configuration.Name
 }
 
 // Checks if both the current and desired image annotation are present on the node.
@@ -127,42 +115,23 @@ func (l *LayeredNodeState) IsCurrentImageAnnotationPresentOnNode() bool {
 // Compares the CurrentImagePullSpec specified by the MachineOSConfig object against the
 // image specified by the annotation passed in.
 func (l *LayeredNodeState) isImageAnnotationEqualToMachineOSConfig(anno string, mosc *mcfgv1.MachineOSConfig) bool {
-	moscImage := ""
-
-	// // Handle case when MOSC is nil (image mode is being disabled)
-	// if mosc != nil {
-	// 	// TOOD: check if this comes back as nil or empty string
-	// 	return moscName =
-	// }
+	moscs := NewMachineOSConfigState(mosc)
 
-	// Get the image annotation from the node
 	val, ok := l.node.Annotations[anno]
-	klog.Errorf("in isImageAnnotationEqualToMachineOSConfig, val: %v, ok: %v", val, ok)
-
-	// // Handle case when MOSC is nil (image mode is being disabled)
-	// if mosc == nil {
-	// 	// TOOD: check if this comes back as nil or empty string
-	// 	return val == ""
-	// }
 
 	// If a layered node does not have an image annotation and has a valid MOSC, then the image is being built
 	if val == "" || !ok {
-		klog.Errorf("in isImageAnnotationEqualToMachineOSConfig returning false cause val == '' or !ok")
 		return false
 	}
 
-	// Get the MachineOSConfigState
-	moscs := NewMachineOSConfigState(mosc)
-	klog.Errorf("in isImageAnnotationEqualToMachineOSConfig with moscs")
-
 	if moscs.HasOSImage() {
 		// If the MOSC image has an image, the image annotation on the node can be directly compared.
-		moscImage = moscs.GetOSImage()
+		return moscs.GetOSImage() == val
 	}
 
 	// If the MOSC does not have an image, but the node has an older image annotation, the image is still likely
 	// being built.
-	return moscImage == val
+	return false
 }
 
 // Sets the desired MachineConfig annotations from the MachineConfigPool.
@@ -208,26 +177,21 @@ func (l *LayeredNodeState) Node() *corev1.Node {
 // isNodeDone returns true if the current == desired and the MCD has marked done.
 func (l *LayeredNodeState) IsNodeDone() bool {
 	if l.node.Annotations == nil {
-		klog.Error("in IsNodeDone, returning false, 1")
 		return false
 	}
 
 	if !l.isNodeConfigDone() {
-		klog.Error("in IsNodeDone, returning false, 2")
 		return false
 	}
 
 	if !l.isNodeImageDone() {
-		klog.Error("in IsNodeDone, returning false, 3")
 		return false
 	}
 
 	if !l.isNodeMCDState(daemonconsts.MachineConfigDaemonStateDone) {
-		klog.Error("in IsNodeDone, returning false, 4")
 		return false
 	}
 
-	klog.Error("in IsNodeDone, returning true")
 	return true
 }
 

pkg/controller/node/status.go

@@ -206,7 +206,6 @@ func (ctrl *Controller) calculateStatus(mcs []*mcfgv1.MachineConfigNode, cconfig
 
 	// this is # 1 priority, get the upgrade states actually reporting
 	if degradedMachineCount+readyMachineCount+unavailableMachineCount+updatingMachineCount != int32(len(nodes)) {
-		klog.Errorf("In degraded + ready + unavailable + updating != nodes...")
 
 		updatedMachines = getUpdatedMachines(pool, nodes, mosc, mosb, isLayeredPool)
 		updatedMachineCount = int32(len(updatedMachines))
@@ -219,8 +218,6 @@ func (ctrl *Controller) calculateStatus(mcs []*mcfgv1.MachineConfigNode, cconfig
 
 		degradedMachines = getDegradedMachines(nodes)
 		degradedMachineCount = int32(len(degradedMachines))
-	} else {
-		klog.Errorf("In degraded + ready + unavailable + updating == nodes... Not where we want to be.")
 	}
 
 	for _, n := range degradedMachines {