Add Spec.Configuration to MachineConfigPool, render controller writes it

See #765 (comment)

MachineConfigPool needs a `Spec.Configuration` and `Status.Configuration`
[just like other objects][1] so that we can properly detect state.
Currently there's a race because the render controller may set `Status.Configuration`
while the pool's `Status` still has `Updated`, so one can't reliably check whether the
pool is at a given config.

With this, ownership is clear: the render controller sets the spec, and the node controller
updates the status.

[1] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status)

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

@@ -299,6 +299,9 @@ type MachineConfigPoolSpec struct {
 	// MaxUnavailable specifies the percentage or constant number of machines that can be updating at any given time.
 	// default is 1.
 	MaxUnavailable *intstr.IntOrString `json:"maxUnavailable"`
+
+	// The targeted MachineConfig object for the machine config pool.
+	Configuration MachineConfigPoolStatusConfiguration `json:"configuration"`
 }
 
 // MachineConfigPoolStatus is the status for MachineConfigPool resource.

pkg/controller/container-runtime-config/container_runtime_config_controller_test.go

@@ -118,6 +118,7 @@ func newMachineConfigPool(name string, labels map[string]string, selector *metav
 		ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels, UID: types.UID(utilrand.String(5))},
 		Spec: mcfgv1.MachineConfigPoolSpec{
 			MachineConfigSelector: selector,
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
 		},
 		Status: mcfgv1.MachineConfigPoolStatus{
 			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},

pkg/controller/kubelet-config/kubelet_config_controller_test.go

@@ -128,6 +128,7 @@ func newMachineConfigPool(name string, labels map[string]string, selector *metav
 		ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels, UID: types.UID(utilrand.String(5))},
 		Spec: mcfgv1.MachineConfigPoolSpec{
 			MachineConfigSelector: selector,
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
 		},
 		Status: mcfgv1.MachineConfigPoolStatus{
 			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},

pkg/controller/node/node_controller.go

@@ -432,7 +432,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 		return err
 	}
 
-	if machineconfigpool.Status.Configuration.Name == "" {
+	if machineconfigpool.Spec.Configuration.Name == "" {
 		// Previously we spammed the logs about empty pools.
 		// Let's just pause for a bit here to let the renderer
 		// initialize them.
@@ -475,7 +475,7 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 
 	candidates := getCandidateMachines(pool, nodes, maxunavail)
 	for _, node := range candidates {
-		if err := ctrl.setDesiredMachineConfigAnnotation(node.Name, pool.Status.Configuration.Name); err != nil {
+		if err := ctrl.setDesiredMachineConfigAnnotation(node.Name, pool.Spec.Configuration.Name); err != nil {
 			return err
 		}
 	}
@@ -517,7 +517,7 @@ func (ctrl *Controller) setDesiredMachineConfigAnnotation(nodeName, currentConfi
 }
 
 func getCandidateMachines(pool *mcfgv1.MachineConfigPool, nodesInPool []*corev1.Node, maxUnavailable int) []*corev1.Node {
-	targetConfig := pool.Status.Configuration.Name
+	targetConfig := pool.Spec.Configuration.Name
 
 	unavail := getUnavailableMachines(nodesInPool)
 	// If we're at capacity, there's nothing to do.

pkg/controller/node/node_controller_test.go

@@ -63,6 +63,7 @@ func newMachineConfigPool(name string, selector *metav1.LabelSelector, maxUnavai
 		Spec: mcfgv1.MachineConfigPoolSpec{
 			NodeSelector:   selector,
 			MaxUnavailable: maxUnavail,
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
 		},
 		Status: mcfgv1.MachineConfigPoolStatus{
 			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
@@ -502,7 +503,7 @@ func TestGetCandidateMachines(t *testing.T) {
 	for idx, test := range tests {
 		t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) {
 			pool := &mcfgv1.MachineConfigPool{
-				Status: mcfgv1.MachineConfigPoolStatus{
+				Spec: mcfgv1.MachineConfigPoolSpec{
 					Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: "v1"}},
 				},
 			}

pkg/controller/node/status.go

@@ -34,14 +34,12 @@ func (ctrl *Controller) syncStatusOnly(pool *mcfgv1.MachineConfigPool) error {
 }
 
 func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv1.MachineConfigPoolStatus {
-	previousUpdated := mcfgv1.IsMachineConfigPoolConditionTrue(pool.Status.Conditions, mcfgv1.MachineConfigPoolUpdated)
-
 	machineCount := int32(len(nodes))
 
-	updatedMachines := getUpdatedMachines(pool.Status.Configuration.Name, nodes)
+	updatedMachines := getUpdatedMachines(pool.Spec.Configuration.Name, nodes)
 	updatedMachineCount := int32(len(updatedMachines))
 
-	readyMachines := getReadyMachines(pool.Status.Configuration.Name, nodes)
+	readyMachines := getReadyMachines(pool.Spec.Configuration.Name, nodes)
 	readyMachineCount := int32(len(readyMachines))
 
 	unavailableMachines := getUnavailableMachines(nodes)
@@ -73,22 +71,26 @@ func calculateStatus(pool *mcfgv1.MachineConfigPool, nodes []*corev1.Node) mcfgv
 		status.Conditions = append(status.Conditions, conditions[i])
 	}
 
-	if updatedMachineCount == machineCount &&
+	allUpdated := updatedMachineCount == machineCount &&
 		readyMachineCount == machineCount &&
-		unavailableMachineCount == 0 {
+		unavailableMachineCount == 0
+
+	if allUpdated {
 		//TODO: update api to only have one condition regarding status of update.
-		updatedMsg := fmt.Sprintf("All nodes are updated with %s", pool.Status.Configuration.Name)
+		updatedMsg := fmt.Sprintf("All nodes are updated with %s", pool.Spec.Configuration.Name)
 		supdated := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdated, corev1.ConditionTrue, updatedMsg, "")
 		mcfgv1.SetMachineConfigPoolCondition(&status, *supdated)
-		if !previousUpdated {
-			glog.Infof("Pool %s: %s", pool.Name, updatedMsg)
-		}
+
 		supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionFalse, "", "")
 		mcfgv1.SetMachineConfigPoolCondition(&status, *supdating)
+		if status.Configuration.Name != pool.Spec.Configuration.Name {
+			glog.Infof("Pool %s: %s", pool.Name, updatedMsg)
+			status.Configuration = pool.Spec.Configuration
+		}
 	} else {
 		supdated := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdated, corev1.ConditionFalse, "", "")
 		mcfgv1.SetMachineConfigPoolCondition(&status, *supdated)
-		supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionTrue, fmt.Sprintf("All nodes are updating to %s", pool.Status.Configuration.Name), "")
+		supdating := mcfgv1.NewMachineConfigPoolCondition(mcfgv1.MachineConfigPoolUpdating, corev1.ConditionTrue, fmt.Sprintf("All nodes are updating to %s", pool.Spec.Configuration.Name), "")
 		mcfgv1.SetMachineConfigPoolCondition(&status, *supdating)
 	}
 

pkg/controller/node/status_test.go

@@ -598,7 +598,7 @@ func TestCalculateStatus(t *testing.T) {
 	for idx, test := range tests {
 		t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) {
 			pool := &mcfgv1.MachineConfigPool{
-				Status: mcfgv1.MachineConfigPoolStatus{
+				Spec: mcfgv1.MachineConfigPoolSpec{
 					Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.currentConfig}},
 				},
 			}

pkg/controller/render/render_controller.go

@@ -500,18 +500,20 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo
 		return err
 	}
 
-	if pool.Status.Configuration.Name == generated.Name {
+	if pool.Spec.Configuration.Name == generated.Name {
 		_, _, err = resourceapply.ApplyMachineConfig(ctrl.client.MachineconfigurationV1(), generated)
 		return err
 	}
 
-	glog.V(2).Infof("Pool %s is now targeting: %s", pool.Name, generated.Name)
-	pool.Status.Configuration.Name = generated.Name
-	pool.Status.Configuration.Source = source
-	_, err = ctrl.client.MachineconfigurationV1().MachineConfigPools().UpdateStatus(pool)
+	newPool := pool.DeepCopy()
+	newPool.Spec.Configuration.Name = generated.Name
+	newPool.Spec.Configuration.Source = source
+	// TODO(walters) Use subresource or JSON patch, but the latter isn't supported by the unit test mocks
+	pool, err = ctrl.client.MachineconfigurationV1().MachineConfigPools().Update(newPool)
 	if err != nil {
 		return err
 	}
+	glog.V(2).Infof("Pool %s: now targeting: %s", pool.Name, generated.Name)
 
 	if err := ctrl.garbageCollectRenderedConfigs(pool); err != nil {
 		return err
@@ -565,6 +567,7 @@ func RunBootstrap(pools []*mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineCo
 			return nil, nil, err
 		}
 
+		pool.Spec.Configuration.Name = generated.Name
 		pool.Status.Configuration.Name = generated.Name
 		opools = append(opools, pool)
 		oconfigs = append(oconfigs, generated)

pkg/controller/render/render_controller_test.go

@@ -59,6 +59,7 @@ func newMachineConfigPool(name string, selector *metav1.LabelSelector, currentMa
 		ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(utilrand.String(5))},
 		Spec: mcfgv1.MachineConfigPoolSpec{
 			MachineConfigSelector: selector,
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
 		},
 		Status: mcfgv1.MachineConfigPoolStatus{
 			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: currentMachineConfig}},
@@ -235,6 +236,10 @@ func (f *fixture) expectUpdateMachineConfigAction(config *mcfgv1.MachineConfig)
 	f.actions = append(f.actions, core.NewRootUpdateAction(schema.GroupVersionResource{Resource: "machineconfigs"}, config))
 }
 
+func (f *fixture) expectUpdateMachineConfigPoolSpec(pool *mcfgv1.MachineConfigPool) {
+	f.actions = append(f.actions, core.NewRootUpdateSubresourceAction(schema.GroupVersionResource{Resource: "machineconfigpools"}, "spec", pool))
+}
+
 func (f *fixture) expectUpdateMachineConfigPoolStatus(pool *mcfgv1.MachineConfigPool) {
 	f.actions = append(f.actions, core.NewRootUpdateSubresourceAction(schema.GroupVersionResource{Resource: "machineconfigpools"}, "status", pool))
 }
@@ -339,6 +344,7 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) {
 		t.Fatal(err)
 	}
 	gmc.Spec.OSImageURL = "why-did-you-change-it"
+	mcp.Spec.Configuration.Name = gmc.Name
 	mcp.Status.Configuration.Name = gmc.Name
 
 	f.ccLister = append(f.ccLister, cc)
@@ -401,6 +407,7 @@ func TestDoNothing(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	mcp.Spec.Configuration.Name = gmc.Name
 	mcp.Status.Configuration.Name = gmc.Name
 
 	f.ccLister = append(f.ccLister, cc)

pkg/operator/status.go

@@ -258,8 +258,11 @@ func (optr *Operator) allMachineConfigPoolStatus() (map[string]string, error) {
 // isMachineConfigPoolConfigurationValid returns nil error when the configuration of a `pool` is created by the controller at version `version`.
 func isMachineConfigPoolConfigurationValid(pool *mcfgv1.MachineConfigPool, version string, machineConfigGetter func(string) (*mcfgv1.MachineConfig, error)) error {
 	// both .status.configuration.name and .status.configuration.source must be set.
+	if len(pool.Spec.Configuration.Name) == 0 {
+		return fmt.Errorf("configuration spec for pool %s is empty", pool.GetName())
+	}
 	if len(pool.Status.Configuration.Name) == 0 {
-		return fmt.Errorf("configuration for pool %s is empty", pool.GetName())
+		return fmt.Errorf("configuration status for pool %s is empty", pool.GetName())
 	}
 	if len(pool.Status.Configuration.Source) == 0 {
 		return fmt.Errorf("list of MachineConfigs that were used to generate configuration for pool %s is empty", pool.GetName())

pkg/operator/status_test.go

@@ -35,7 +35,7 @@ func TestIsMachineConfigPoolConfigurationValid(t *testing.T) {
 
 		err error
 	}{{
-		err: errors.New("configuration for pool dummy-pool is empty"),
+		err: errors.New("configuration spec for pool dummy-pool is empty"),
 	}, {
 		generated: "g",
 		err:       errors.New("list of MachineConfigs that were used to generate configuration for pool dummy-pool is empty"),
@@ -116,6 +116,9 @@ func TestIsMachineConfigPoolConfigurationValid(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "dummy-pool",
 				},
+				Spec: mcfgv1.MachineConfigPoolSpec{
+					Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.generated}, Source: source},
+				},
 				Status: mcfgv1.MachineConfigPoolStatus{
 					Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{ObjectReference: corev1.ObjectReference{Name: test.generated}, Source: source},
 				},

test/e2e/mcd_test.go

@@ -93,6 +93,7 @@ func createMCToAddFile(name, filename, data, fs string) *mcv1.MachineConfig {
 			},
 		},
 	}
+
 	return mcadd
 }
 
@@ -107,9 +108,9 @@ func waitForRenderedConfig(t *testing.T, cs *framework.ClientSet, pool, mcName s
 		if err != nil {
 			return false, err
 		}
-		for _, mc := range mcp.Status.Configuration.Source {
+		for _, mc := range mcp.Spec.Configuration.Source {
 			if mc.Name == mcName {
-				renderedConfig = mcp.Status.Configuration.Name
+				renderedConfig = mcp.Spec.Configuration.Name
 				return true, nil
 			}
 		}
@@ -123,8 +124,6 @@ func waitForRenderedConfig(t *testing.T, cs *framework.ClientSet, pool, mcName s
 
 // waitForPoolComplete polls a pool until it has completed an update to target
 func waitForPoolComplete(t *testing.T, cs *framework.ClientSet, pool, target string) error {
-	// We need a spec and status separation https://github.com/openshift/machine-config-operator/pull/765#issuecomment-493136113
-	time.Sleep(time.Second * 13)
 	startTime := time.Now()
 	if err := wait.Poll(2*time.Second, 10*time.Minute, func() (bool, error) {
 		mcp, err := cs.MachineConfigPools().Get(pool, metav1.GetOptions{})