Stop reporting unavailable when progressing and add support for multiple operands status

pkg/operator/operator.go

@@ -4,9 +4,12 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/openshift/machine-api-operator/pkg/version"
+
 	"github.com/golang/glog"
 	osclientset "github.com/openshift/client-go/config/clientset/versioned"
 
+	osconfigv1 "github.com/openshift/api/config/v1"
 	v1 "k8s.io/api/core/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -50,7 +53,8 @@ type Operator struct {
 	deployListerSynced cache.InformerSynced
 
 	// queue only ever has one item, but it has nice error handling backoff/retry semantics
-	queue workqueue.RateLimitingInterface
+	queue           workqueue.RateLimitingInterface
+	operandVersions []osconfigv1.OperandVersion
 }
 
 // New returns a new machine config operator.
@@ -80,6 +84,12 @@ func New(
 		osClient:      osClient,
 		eventRecorder: eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "machineapioperator"}),
 		queue:         workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "machineapioperator"),
+		operandVersions: []osconfigv1.OperandVersion{
+			{
+				Name:    "operator",
+				Version: version.Raw,
+			},
+		},
 	}
 
 	serviceAccountInfomer.Informer().AddEventHandler(optr.eventHandler())

pkg/operator/status.go

@@ -1,13 +1,16 @@
 package operator
 
 import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	"github.com/golang/glog"
+
 	osconfigv1 "github.com/openshift/api/config/v1"
-	osclientsetv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 	cvoresourcemerge "github.com/openshift/cluster-version-operator/lib/resourcemerge"
-	"github.com/openshift/machine-api-operator/pkg/version"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/pointer"
 )
 
 // StatusReason is a MixedCaps string representing the reason for a
@@ -28,36 +31,55 @@ const (
 // statusProgressing sets the Progressing condition to True, with the given
 // reason and message, and sets both the Available and Failing conditions to
 // False.
-func (optr *Operator) statusProgressing(reason StatusReason, message string) error {
+func (optr *Operator) statusProgressing() error {
+	desiredVersions := optr.operandVersions
+	currentVersions, err := optr.getCurrentVersions()
+	if err != nil {
+		glog.Errorf("error getting current versions: %v", err)
+		return err
+	}
+
+	var message string
+	if !reflect.DeepEqual(desiredVersions, currentVersions) {
+		message = fmt.Sprintf("Progressing towards %s", optr.printOperandVersions())
+	} else {
+		message = fmt.Sprintf("Running resync for %s", optr.printOperandVersions())
+	}
+
 	conds := []osconfigv1.ClusterOperatorStatusCondition{
 		{
 			Type:    osconfigv1.OperatorProgressing,
 			Status:  osconfigv1.ConditionTrue,
-			Reason:  string(reason),
+			Reason:  string(ReasonSyncing),
 			Message: message,
 		},
 		{
 			Type:   osconfigv1.OperatorAvailable,
-			Status: osconfigv1.ConditionFalse,
+			Status: osconfigv1.ConditionTrue,
 		},
 		{
 			Type:   osconfigv1.OperatorFailing,
 			Status: osconfigv1.ConditionFalse,
 		},
 	}
 
-	return optr.syncStatus(conds)
+	co, err := optr.getOrCreateClusterOperator()
+	if err != nil {
+		glog.Errorf("failed to get or create Cluster Operator: %v", err)
+		return err
+	}
+	return optr.syncStatus(co, conds)
 }
 
 // statusAvailable sets the Available condition to True, with the given reason
 // and message, and sets both the Progressing and Failing conditions to False.
-func (optr *Operator) statusAvailable(reason StatusReason, message string) error {
+func (optr *Operator) statusAvailable() error {
 	conds := []osconfigv1.ClusterOperatorStatusCondition{
 		{
 			Type:    osconfigv1.OperatorAvailable,
 			Status:  osconfigv1.ConditionTrue,
-			Reason:  string(reason),
-			Message: message,
+			Reason:  string(ReasonEmpty),
+			Message: fmt.Sprintf("Cluster Machine API Operator is available at %s", optr.printOperandVersions()),
 		},
 		{
 			Type:   osconfigv1.OperatorProgressing,
@@ -70,19 +92,38 @@ func (optr *Operator) statusAvailable(reason StatusReason, message string) error
 		},
 	}
 
-	return optr.syncStatus(conds)
+	co, err := optr.getOrCreateClusterOperator()
+	if err != nil {
+		return err
+	}
+	co.Status.Versions = optr.operandVersions
+	return optr.syncStatus(co, conds)
 }
 
 // statusFailing sets the Failing condition to True, with the given reason and
 // message, and sets the Progressing condition to False, and the Available
 // condition to True.  This indicates that the operator is present and may be
 // partially functioning, but is in a degraded or failing state.
-func (optr *Operator) statusFailing(reason StatusReason, message string) error {
+func (optr *Operator) statusFailing(error string) error {
+	desiredVersions := optr.operandVersions
+	currentVersions, err := optr.getCurrentVersions()
+	if err != nil {
+		glog.Errorf("error getting current versions: %v", err)
+		return err
+	}
+
+	var message string
+	if !reflect.DeepEqual(desiredVersions, currentVersions) {
+		message = fmt.Sprintf("Failed when progressing towards %s because %s", optr.printOperandVersions(), error)
+	} else {
+		message = fmt.Sprintf("Failed to resync for %s because %s", optr.printOperandVersions(), error)
+	}
+
 	conds := []osconfigv1.ClusterOperatorStatusCondition{
 		{
 			Type:    osconfigv1.OperatorFailing,
 			Status:  osconfigv1.ConditionTrue,
-			Reason:  string(reason),
+			Reason:  string(ReasonSyncFailed),
 			Message: message,
 		},
 		{
@@ -95,60 +136,70 @@ func (optr *Operator) statusFailing(reason StatusReason, message string) error {
 		},
 	}
 
-	return optr.syncStatus(conds)
+	co, err := optr.getOrCreateClusterOperator()
+	if err != nil {
+		return err
+	}
+	return optr.syncStatus(co, conds)
 }
 
 //syncStatus applies the new condition to the mao ClusterOperator object.
-func (optr *Operator) syncStatus(conds []osconfigv1.ClusterOperatorStatusCondition) error {
-	// to report the status of all the managed components.
-	// TODO we will report the version of the operands (so our machine api implementation version)
-	// NOTE: related objects lets openshift/must-gather collect diagnostic content
-	clusterOperator := &osconfigv1.ClusterOperator{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: clusterOperatorName,
-		},
-		Status: osconfigv1.ClusterOperatorStatus{
-			Versions: []osconfigv1.OperandVersion{
-				{
-					Name:    "machine-api",
-					Version: version.Raw,
-				},
-			},
-			RelatedObjects: []osconfigv1.ObjectReference{
-				{
-					Group:    "",
-					Resource: "namespaces",
-					Name:     optr.namespace,
-				},
-			},
-		},
-	}
-
+func (optr *Operator) syncStatus(co *osconfigv1.ClusterOperator, conds []osconfigv1.ClusterOperatorStatusCondition) error {
 	for _, c := range conds {
-		cvoresourcemerge.SetOperatorStatusCondition(&clusterOperator.Status.Conditions, c)
+		cvoresourcemerge.SetOperatorStatusCondition(&co.Status.Conditions, c)
 	}
 
-	_, _, err := ApplyClusterOperator(optr.osClient.ConfigV1(), clusterOperator)
+	_, err := optr.osClient.ConfigV1().ClusterOperators().UpdateStatus(co)
 	return err
 }
 
-// ApplyClusterOperator applies the required ClusterOperator
-func ApplyClusterOperator(client osclientsetv1.ClusterOperatorsGetter, required *osconfigv1.ClusterOperator) (*osconfigv1.ClusterOperator, bool, error) {
-	existing, err := client.ClusterOperators().Get(required.Name, metav1.GetOptions{})
+func (optr *Operator) getOrCreateClusterOperator() (*osconfigv1.ClusterOperator, error) {
+	co, err := optr.osClient.ConfigV1().ClusterOperators().Get(clusterOperatorName, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
-		actual, err := client.ClusterOperators().Create(required)
-		return actual, true, err
+		// to report the status of all the managed components.
+		// TODO we will report the version of the operands (so our machine api implementation version)
+		// NOTE: related objects lets openshift/must-gather collect diagnostic content
+		co = &osconfigv1.ClusterOperator{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: clusterOperatorName,
+			},
+			Status: osconfigv1.ClusterOperatorStatus{
+				Versions: optr.operandVersions,
+				RelatedObjects: []osconfigv1.ObjectReference{
+					{
+						Group:    "",
+						Resource: "namespaces",
+						Name:     optr.namespace,
+					},
+				},
+			},
+		}
+
+		glog.Infof("%s clusterOperator status does not exist, creating %v", clusterOperatorName, co)
+		co, err := optr.osClient.ConfigV1().ClusterOperators().Create(co)
+		if err != nil {
+			return nil, err
+		}
+		return co, nil
 	}
 	if err != nil {
-		return nil, false, err
+		return nil, err
 	}
+	return co, nil
+}
 
-	modified := pointer.BoolPtr(false)
-	cvoresourcemerge.EnsureClusterOperatorStatus(modified, existing, *required)
-	if !*modified {
-		return existing, false, nil
+func (optr *Operator) getCurrentVersions() ([]osconfigv1.OperandVersion, error) {
+	co, err := optr.getOrCreateClusterOperator()
+	if err != nil {
+		return nil, err
 	}
+	return co.Status.Versions, nil
+}
 
-	actual, err := client.ClusterOperators().UpdateStatus(existing)
-	return actual, true, err
+func (optr *Operator) printOperandVersions() string {
+	versionsOutput := []string{}
+	for _, operand := range optr.operandVersions {
+		versionsOutput = append(versionsOutput, fmt.Sprintf("%s: %s", operand.Name, operand.Version))
+	}
+	return strings.Join(versionsOutput, ", ")
 }

pkg/operator/status_test.go

@@ -0,0 +1,27 @@
+package operator
+
+import (
+	"testing"
+
+	osconfigv1 "github.com/openshift/api/config/v1"
+)
+
+func TestPrintOperandVersions(t *testing.T) {
+	optr := Operator{
+		operandVersions: []osconfigv1.OperandVersion{
+			{
+				Name:    "operator",
+				Version: "1.0",
+			},
+			{
+				Name:    "controller-manager",
+				Version: "2.0",
+			},
+		},
+	}
+	expectedOutput := "operator: 1.0, controller-manager: 2.0"
+	got := optr.printOperandVersions()
+	if got != expectedOutput {
+		t.Errorf("Expected: %s, got: %s", expectedOutput, got)
+	}
+}

pkg/operator/sync.go

@@ -24,13 +24,13 @@ const (
 func (optr *Operator) syncAll(config OperatorConfig) error {
 	glog.Infof("Syncing ClusterOperatorStatus")
 
-	if err := optr.statusProgressing(ReasonSyncing, "Running sync functions"); err != nil {
+	if err := optr.statusProgressing(); err != nil {
 		glog.Errorf("Error synching ClusterOperatorStatus: %v", err)
 		return fmt.Errorf("error syncing ClusterOperatorStatus: %v", err)
 	}
 
 	if err := optr.syncClusterAPIController(config); err != nil {
-		if err := optr.statusFailing(ReasonSyncFailed, err.Error()); err != nil {
+		if err := optr.statusFailing(err.Error()); err != nil {
 			// Just log the error here.  We still want to
 			// return the outer error.
 			glog.Errorf("Error synching ClusterOperatorStatus: %v", err)
@@ -42,7 +42,7 @@ func (optr *Operator) syncAll(config OperatorConfig) error {
 
 	glog.Info("Synched up cluster api controller")
 
-	if err := optr.statusAvailable(ReasonEmpty, "cluster-api ready"); err != nil {
+	if err := optr.statusAvailable(); err != nil {
 		glog.Errorf("Error synching ClusterOperatorStatus: %v", err)
 		return fmt.Errorf("error syncing ClusterOperatorStatus: %v", err)
 	}

test/integration/main.go

@@ -423,7 +423,8 @@ var rootCmd = &cobra.Command{
 		// TESTS
 		// verify the cluster-api is running
 		err = wait.Poll(pollInterval, timeoutPoolClusterAPIDeploymentInterval, func() (bool, error) {
-			if clusterAPIDeployment, err := testConfig.KubeClient.AppsV1beta2().Deployments(targetNamespace).Get("clusterapi-manager-controllers", metav1.GetOptions{}); err == nil {
+			clusterAPIDeployment, err := testConfig.KubeClient.AppsV1beta2().Deployments(targetNamespace).Get("clusterapi-manager-controllers", metav1.GetOptions{})
+			if err == nil {
 				// Check all the pods are running
 				log.Infof("Waiting for all clusterapi-manager-controllers deployment pods to be ready, have %v, expecting 1", clusterAPIDeployment.Status.ReadyReplicas)
 				if clusterAPIDeployment.Status.ReadyReplicas < 1 {
@@ -432,7 +433,7 @@ var rootCmd = &cobra.Command{
 				return true, nil
 			}
 
-			log.Info("Waiting for clusterapi-manager-controllers deployment to be created")
+			log.Infof("Waiting for clusterapi-manager-controllers deployment to be created: %v", err)
 			return false, nil
 		})
 

test/integration/manifests/status-crd.yaml

@@ -30,8 +30,5 @@ spec:
   subresources:
     status: {}
   version: v1
-  versions:
-  - name: v1
-    served: true