Fix MCD status reporting, retry loop and max unavailable progress

manifests/machineconfigpool.crd.yaml

@@ -16,10 +16,6 @@ spec:
     description: When at least one of machine is not either not updated or is in the process of updating to the desired machine config.
     name: Updating
     type: string
-  - JSONPath: .status.conditions[?(@.type=="Degraded")].status
-    description: When the update for one of the machine is not progressig due to an error.
-    name: Degraded
-    type: string
   # group name to use for REST API: /apis/<group>/<version>
   group: machineconfiguration.openshift.io
   # list of versions supported by this CustomResourceDefinition

pkg/apis/machineconfiguration.openshift.io/v1/helpers_test.go

@@ -26,17 +26,9 @@ var (
 		}
 	}
 
-	condDegraded = func() MachineConfigPoolCondition {
-		return MachineConfigPoolCondition{
-			Type:   MachineConfigPoolDegraded,
-			Status: corev1.ConditionTrue,
-			Reason: "ForSomeReason",
-		}
-	}
-
 	status = func() *MachineConfigPoolStatus {
 		return &MachineConfigPoolStatus{
-			Conditions: []MachineConfigPoolCondition{condUpdatedFalse(), condDegraded()},
+			Conditions: []MachineConfigPoolCondition{condUpdatedFalse()},
 		}
 	}
 )
@@ -82,11 +74,6 @@ func TestSetMachineConfigPoolCondition(t *testing.T) {
 		cond:   condUpdatedTrue(),
 
 		expectedStatus: &MachineConfigPoolStatus{Conditions: []MachineConfigPoolCondition{condUpdatedTrue()}},
-	}, {
-		status: &MachineConfigPoolStatus{Conditions: []MachineConfigPoolCondition{condUpdatedFalse()}},
-		cond:   condDegraded(),
-
-		expectedStatus: status(),
 	}, {
 		status: &MachineConfigPoolStatus{Conditions: []MachineConfigPoolCondition{condUpdatedFalse()}},
 		cond:   condUpdatedTrue(),
@@ -111,12 +98,6 @@ func TestRemoveMachineConfigPoolCondition(t *testing.T) {
 
 		expectedStatus *MachineConfigPoolStatus
 	}{
-		{
-			status:   &MachineConfigPoolStatus{},
-			condType: MachineConfigPoolDegraded,
-
-			expectedStatus: &MachineConfigPoolStatus{},
-		},
 		{
 			status:   &MachineConfigPoolStatus{Conditions: []MachineConfigPoolCondition{condUpdatedTrue()}},
 			condType: MachineConfigPoolUpdated,

pkg/apis/machineconfiguration.openshift.io/v1/types.go

@@ -324,10 +324,6 @@ type MachineConfigPoolStatus struct {
 	// A node is marked unavailable if it is in updating state or NodeReady condition is false.
 	UnavailableMachineCount int32 `json:"unavailableMachineCount"`
 
-	// Total number of machines marked degraded.
-	// A node is marked degraded if applying a configuration failed..
-	DegradedMachineCount int32 `json:"degradedMachineCount"`
-
 	// Represents the latest available observations of current state.
 	Conditions []MachineConfigPoolCondition `json:"conditions"`
 }
@@ -374,9 +370,6 @@ const (
 	// When at least one of machine is not either not updated or is in the process of updating
 	// to the desired machine config.
 	MachineConfigPoolUpdating MachineConfigPoolConditionType = "Updating"
-	// MachineConfigPoolDegraded means the update for one of the machine is not progressing
-	// due to an error
-	MachineConfigPoolDegraded MachineConfigPoolConditionType = "Degraded"
 )
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/controller/node/node_controller.go

@@ -138,13 +138,15 @@ func (ctrl *Controller) addMachineConfigPool(obj interface{}) {
 	ctrl.enqueueMachineConfigPool(pool)
 
 }
+
 func (ctrl *Controller) updateMachineConfigPool(old, cur interface{}) {
 	oldPool := old.(*mcfgv1.MachineConfigPool)
 	curPool := cur.(*mcfgv1.MachineConfigPool)
 
 	glog.V(4).Infof("Updating MachineConfigPool %s", oldPool.Name)
 	ctrl.enqueueMachineConfigPool(curPool)
 }
+
 func (ctrl *Controller) deleteMachineConfigPool(obj interface{}) {
 	pool, ok := obj.(*mcfgv1.MachineConfigPool)
 	if !ok {
@@ -245,6 +247,10 @@ func nodeChanged(old, cur *corev1.Node) bool {
 		return true
 	}
 
+	if old.Annotations[daemonconsts.MachineConfigDaemonStateAnnotationKey] != cur.Annotations[daemonconsts.MachineConfigDaemonStateAnnotationKey] {
+		return true
+	}
+
 	return false
 }
 
@@ -376,12 +382,11 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 	}
 
 	if machineconfigpool.Status.Configuration.Name == "" {
-		delay := 5 * time.Second
 		// Previously we spammed the logs about empty pools.
 		// Let's just pause for a bit here to let the renderer
 		// initialize them.
-		glog.Infof("Pool %s is unconfigured, pausing %v for renderer to initialize", name, delay)
-		time.Sleep(delay)
+		glog.Infof("Pool %s is unconfigured, pausing %v for renderer to initialize", name, updateDelay)
+		time.Sleep(updateDelay)
 		return nil
 	}
 

pkg/controller/node/status.go

@@ -3,6 +3,7 @@ package node
 import (
 	"fmt"
 
+	"github.com/golang/glog"
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
 	daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants"
 	corev1 "k8s.io/api/core/v1"
@@ -43,16 +44,12 @@ func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv
 	unavailableMachines := getUnavailableMachines(pool.Status.Configuration.Name, nodes)
 	unavailableMachineCount := int32(len(unavailableMachines))
 
-	degradedMachines := getDegradedMachines(pool.Status.Configuration.Name, nodes)
-	degradedMachineCount := int32(len(degradedMachines))
-
 	status := mcfgv1.MachineConfigPoolStatus{
 		ObservedGeneration:      pool.Generation,
 		MachineCount:            machineCount,
 		UpdatedMachineCount:     updatedMachineCount,
 		ReadyMachineCount:       readyMachineCount,
 		UnavailableMachineCount: unavailableMachineCount,
-		DegradedMachineCount:    degradedMachineCount,
 	}
 
 	status.Configuration = pool.Status.Configuration
@@ -76,14 +73,6 @@ func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv
 		supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionTrue, fmt.Sprintf("All nodes are updating to %s", pool.Status.Configuration.Name), "")
 		mcfgv1.SetMachineConfigPoolCondition(&status, *supdating)
 	}
-
-	if len(degradedMachines) > 0 {
-		sdegraded := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolDegraded, corev1.ConditionTrue, fmt.Sprintf("%d nodes are reporting degraded status on update. Cannot proceed.", len(degradedMachines)), "")
-		mcfgv1.SetMachineConfigPoolCondition(&status, *sdegraded)
-	} else {
-		sdegraded := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolDegraded, corev1.ConditionFalse, "", "")
-		mcfgv1.SetMachineConfigPoolCondition(&status, *sdegraded)
-	}
 	return status
 }
 
@@ -97,8 +86,17 @@ func getUpdatedMachines(currentConfig string, nodes []*corev1.Node) []*corev1.No
 		if !ok || cconfig == "" {
 			continue
 		}
+		dconfig, ok := node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey]
+		if !ok || cconfig == "" {
+			continue
+		}
 
-		if cconfig == currentConfig {
+		dstate, ok := node.Annotations[daemonconsts.MachineConfigDaemonStateAnnotationKey]
+		if !ok || dstate == "" {
+			continue
+		}
+
+		if cconfig == currentConfig && dconfig == currentConfig && dstate == daemonconsts.MachineConfigDaemonStateDone {
 			updated = append(updated, node)
 		}
 	}
@@ -155,31 +153,11 @@ func getUnavailableMachines(currentConfig string, nodes []*corev1.Node) []*corev
 			continue
 		}
 
-		if dconfig == currentConfig && (dconfig != cconfig || !isNodeReady(node)) {
+		nodeNotReady := !isNodeReady(node)
+		if dconfig == currentConfig && (dconfig != cconfig || nodeNotReady) {
 			unavail = append(unavail, node)
+			glog.V(2).Infof("Node %s unavailable: different configs %v or node not ready %v", node.Name, dconfig != cconfig, nodeNotReady)
 		}
 	}
 	return unavail
 }
-
-func getDegradedMachines(currentConfig string, nodes []*corev1.Node) []*corev1.Node {
-	var degraded []*corev1.Node
-	for _, node := range nodes {
-		if node.Annotations == nil {
-			continue
-		}
-		dconfig, ok := node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey]
-		if !ok || dconfig == "" {
-			continue
-		}
-		dstate, ok := node.Annotations[daemonconsts.MachineConfigDaemonStateAnnotationKey]
-		if !ok || dstate == "" {
-			continue
-		}
-
-		if dconfig == currentConfig && dstate == daemonconsts.MachineConfigDaemonStateDegraded {
-			degraded = append(degraded, node)
-		}
-	}
-	return degraded
-}

pkg/controller/node/status_test.go

@@ -59,6 +59,7 @@ func newNode(name string, currentConfig, desiredConfig string) *corev1.Node {
 		annos = map[string]string{}
 		annos[daemonconsts.CurrentMachineConfigAnnotationKey] = currentConfig
 		annos[daemonconsts.DesiredMachineConfigAnnotationKey] = desiredConfig
+		annos[daemonconsts.MachineConfigDaemonStateAnnotationKey] = daemonconsts.MachineConfigDaemonStateDone
 	}
 
 	return &corev1.Node{
@@ -318,22 +319,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -369,22 +361,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -420,22 +403,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -471,22 +445,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionTrue; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -522,22 +487,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -573,22 +529,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}, {
 		nodes: []*corev1.Node{
@@ -624,22 +571,13 @@ func TestCalculateStatus(t *testing.T) {
 				t.Fatal("updated condition not found")
 			}
 
-			conddegrad := mcfgv1.GetMachineConfigPoolCondition(status, mcfgv1.MachineConfigPoolDegraded)
-			if condupdating == nil {
-				t.Fatal("updated condition not found")
-			}
-
 			if got, want := condupdated.Status, corev1.ConditionTrue; got != want {
 				t.Fatalf("mismatch condupdated.Status: got %s want: %s", got, want)
 			}
 
 			if got, want := condupdating.Status, corev1.ConditionFalse; got != want {
 				t.Fatalf("mismatch condupdating.Status: got %s want: %s", got, want)
 			}
-
-			if got, want := conddegrad.Status, corev1.ConditionFalse; got != want {
-				t.Fatalf("mismatch conddegrad.Status: got %s want: %s", got, want)
-			}
 		},
 	}}
 	for idx, test := range tests {