Merge pull request #442 from runcom/clear-failing

MCO: clear out failing status on success and add tests

Gopkg.toml

@@ -39,6 +39,10 @@ required = [
     go-tests = false
     unused-packages = false
 
+[[constraint]]
+  name = "github.com/stretchr/testify"
+  version = "v1.3.0"
+
 [[constraint]]
   name = "github.com/apparentlymart/go-cidr"
   version = "1.0.0"

cmd/common/client_builder.go

@@ -4,8 +4,10 @@ import (
 	"os"
 
 	"github.com/golang/glog"
+	configv1 "github.com/openshift/api/config/v1"
 	configclientset "github.com/openshift/client-go/config/clientset/versioned"
 	apiext "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
@@ -19,23 +21,58 @@ type ClientBuilder struct {
 	config *rest.Config
 }
 
-// ClientOrDie returns the kubernetes client interface for machine config.
+// MachineConfigClientOrDie returns the kubernetes client interface for machine config.
 func (cb *ClientBuilder) MachineConfigClientOrDie(name string) mcfgclientset.Interface {
 	return mcfgclientset.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
 }
 
-// ClientOrDie returns the kubernetes client interface for general kubernetes objects.
+// KubeClientOrDie returns the kubernetes client interface for general kubernetes objects.
 func (cb *ClientBuilder) KubeClientOrDie(name string) kubernetes.Interface {
 	return kubernetes.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
 }
 
-// ClientOrDie returns the kubernetes client interface for security related kubernetes objects
+// clusterOperatorsClient is a wrapper around the ClusterOperators client
+// implementing the ClusterOperatorsClientInterface
+type clusterOperatorsClient struct {
+	innerConfigClient configclientset.Interface
+}
+
+// Create creates the cluster operator and returns it
+func (c *clusterOperatorsClient) Create(co *configv1.ClusterOperator) (*configv1.ClusterOperator, error) {
+	return c.innerConfigClient.ConfigV1().ClusterOperators().Create(co)
+}
+
+// UpdateStatus updates the status of the cluster operator and returns it
+func (c *clusterOperatorsClient) UpdateStatus(co *configv1.ClusterOperator) (*configv1.ClusterOperator, error) {
+	return c.innerConfigClient.ConfigV1().ClusterOperators().UpdateStatus(co)
+}
+
+// Get returns the cluster operator by name
+func (c *clusterOperatorsClient) Get(name string, options metav1.GetOptions) (*configv1.ClusterOperator, error) {
+	return c.innerConfigClient.ConfigV1().ClusterOperators().Get(name, options)
+}
+
+// ClusterOperatorsClientInterface is a controlled ClusterOperators client which is used
+// by the operator and allows us to mock it in tests.
+type ClusterOperatorsClientInterface interface {
+	Create(*configv1.ClusterOperator) (*configv1.ClusterOperator, error)
+	UpdateStatus(*configv1.ClusterOperator) (*configv1.ClusterOperator, error)
+	Get(name string, options metav1.GetOptions) (*configv1.ClusterOperator, error)
+}
+
+// ClusterOperatorsClientOrDie returns the controlleed ClusterOperators client used by the operator
+func (cb *ClientBuilder) ClusterOperatorsClientOrDie(name string) ClusterOperatorsClientInterface {
+	cc := configclientset.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
+	return &clusterOperatorsClient{innerConfigClient: cc}
+}
+
+// ConfigClientOrDie returns the kubernetes client interface for security related kubernetes objects
 // such as pod security policy, security context.
 func (cb *ClientBuilder) ConfigClientOrDie(name string) configclientset.Interface {
 	return configclientset.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
 }
 
-// ClientOrDie returns the kubernetes client interface for extended kubernetes objects.
+// APIExtClientOrDie returns the kubernetes client interface for extended kubernetes objects.
 func (cb *ClientBuilder) APIExtClientOrDie(name string) apiext.Interface {
 	return apiext.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
 }

cmd/machine-config-operator/start.go

@@ -99,7 +99,7 @@ func startControllers(ctx *common.ControllerContext) error {
 		ctx.ClientBuilder.MachineConfigClientOrDie(componentName),
 		ctx.ClientBuilder.KubeClientOrDie(componentName),
 		ctx.ClientBuilder.APIExtClientOrDie(componentName),
-		ctx.ClientBuilder.ConfigClientOrDie(componentName),
+		ctx.ClientBuilder.ClusterOperatorsClientOrDie(componentName),
 	).Run(2, ctx.Stop)
 
 	return nil

pkg/operator/operator.go

@@ -29,11 +29,12 @@ import (
 	"k8s.io/client-go/util/workqueue"
 
 	configv1 "github.com/openshift/api/config/v1"
-	configclientset "github.com/openshift/client-go/config/clientset/versioned"
 	configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
 	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
 	installertypes "github.com/openshift/installer/pkg/types"
 
+	// TODO(runcom): move to pkg
+	"github.com/openshift/machine-config-operator/cmd/common"
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
 	templatectrl "github.com/openshift/machine-config-operator/pkg/controller/template"
 	mcfgclientset "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
@@ -68,7 +69,7 @@ type Operator struct {
 	client        mcfgclientset.Interface
 	kubeClient    kubernetes.Interface
 	apiExtClient  apiextclientset.Interface
-	configClient  configclientset.Interface
+	configClient  common.ClusterOperatorsClientInterface
 	eventRecorder record.EventRecorder
 
 	syncHandler func(ic string) error
@@ -115,7 +116,7 @@ func New(
 	client mcfgclientset.Interface,
 	kubeClient kubernetes.Interface,
 	apiExtClient apiextclientset.Interface,
-	configClient configclientset.Interface,
+	configClient common.ClusterOperatorsClientInterface,
 ) *Operator {
 	eventBroadcaster := record.NewBroadcaster()
 	eventBroadcaster.StartLogging(glog.Infof)
@@ -329,7 +330,16 @@ func (optr *Operator) sync(key string) error {
 
 	// create renderConfig
 	rc := getRenderConfig(namespace, spec, imgs, infra.Status.APIServerURL)
-	return optr.syncAll(rc)
+	// syncFuncs is the list of sync functions that are executed in order.
+	// any error marks sync as failure but continues to next syncFunc
+	var syncFuncs = []syncFunc{
+		{"pools", optr.syncMachineConfigPools},
+		{"mcc", optr.syncMachineConfigController},
+		{"mcs", optr.syncMachineConfigServer},
+		{"mcd", optr.syncMachineConfigDaemon},
+		{"required-pools", optr.syncRequiredMachineConfigPools},
+	}
+	return optr.syncAll(rc, syncFuncs)
 }
 
 func (optr *Operator) getOsImageURL(namespace string) (string, error) {

pkg/operator/status.go

@@ -28,13 +28,12 @@ func (optr *Operator) syncAvailableStatus() error {
 	}
 
 	optrVersion, _ := optr.vStore.Get("operator")
-	progressing := cov1helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorProgressing)
 	failing := cov1helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorFailing)
 	message := fmt.Sprintf("Cluster has deployed %s", optrVersion)
 
 	available := configv1.ConditionTrue
 
-	if failing && !progressing {
+	if failing {
 		available = configv1.ConditionFalse
 		message = fmt.Sprintf("Cluster not available for %s", optrVersion)
 	}
@@ -47,7 +46,7 @@ func (optr *Operator) syncAvailableStatus() error {
 
 	co.Status.Versions = optr.vStore.GetAll()
 	optr.setMachineConfigPoolStatuses(&co.Status)
-	_, err = optr.configClient.ConfigV1().ClusterOperators().UpdateStatus(co)
+	_, err = optr.configClient.UpdateStatus(co)
 	return err
 }
 
@@ -67,6 +66,7 @@ func (optr *Operator) syncProgressingStatus() error {
 
 	if optr.vStore.Equal(co.Status.Versions) {
 		if optr.inClusterBringup {
+			message = fmt.Sprintf("Cluster is bootstrapping %s", optrVersion)
 			progressing = configv1.ConditionTrue
 		}
 	} else {
@@ -80,15 +80,12 @@ func (optr *Operator) syncProgressingStatus() error {
 	})
 
 	optr.setMachineConfigPoolStatuses(&co.Status)
-	_, err = optr.configClient.ConfigV1().ClusterOperators().UpdateStatus(co)
+	_, err = optr.configClient.UpdateStatus(co)
 	return err
 }
 
 // syncFailingStatus applies the new condition to the mco's ClusterOperator object.
-func (optr *Operator) syncFailingStatus(ierr error) error {
-	if ierr == nil {
-		return nil
-	}
+func (optr *Operator) syncFailingStatus(ierr error) (err error) {
 	co, err := optr.fetchClusterOperator()
 	if err != nil {
 		return err
@@ -98,34 +95,40 @@ func (optr *Operator) syncFailingStatus(ierr error) error {
 	}
 
 	optrVersion, _ := optr.vStore.Get("operator")
-	var message string
-	if optr.vStore.Equal(co.Status.Versions) {
-		// syncing the state to exiting version.
-		message = fmt.Sprintf("Failed to resync %s because: %v", optrVersion, ierr.Error())
+	failing := configv1.ConditionTrue
+	var message, reason string
+	if ierr == nil {
+		failing = configv1.ConditionFalse
 	} else {
-		message = fmt.Sprintf("Unable to apply %s: %v", optrVersion, ierr.Error())
+		if optr.vStore.Equal(co.Status.Versions) {
+			// syncing the state to exiting version.
+			message = fmt.Sprintf("Failed to resync %s because: %v", optrVersion, ierr.Error())
+		} else {
+			message = fmt.Sprintf("Unable to apply %s: %v", optrVersion, ierr.Error())
+		}
+		reason = ierr.Error()
+
+		// set progressing
+		if cov1helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorProgressing) {
+			cov1helpers.SetStatusCondition(&co.Status.Conditions, configv1.ClusterOperatorStatusCondition{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: fmt.Sprintf("Unable to apply %s", version.Version.String())})
+		} else {
+			cov1helpers.SetStatusCondition(&co.Status.Conditions, configv1.ClusterOperatorStatusCondition{Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Message: fmt.Sprintf("Error while reconciling %s", version.Version.String())})
+		}
 	}
 	// set failing condition
 	cov1helpers.SetStatusCondition(&co.Status.Conditions, configv1.ClusterOperatorStatusCondition{
-		Type: configv1.OperatorFailing, Status: configv1.ConditionTrue,
+		Type: configv1.OperatorFailing, Status: failing,
 		Message: message,
-		Reason:  ierr.Error(),
+		Reason:  reason,
 	})
 
-	// set progressing
-	if cov1helpers.IsStatusConditionTrue(co.Status.Conditions, configv1.OperatorProgressing) {
-		cov1helpers.SetStatusCondition(&co.Status.Conditions, configv1.ClusterOperatorStatusCondition{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: fmt.Sprintf("Unable to apply %s", version.Version.String())})
-	} else {
-		cov1helpers.SetStatusCondition(&co.Status.Conditions, configv1.ClusterOperatorStatusCondition{Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Message: fmt.Sprintf("Error while reconciling %s", version.Version.String())})
-	}
-
 	optr.setMachineConfigPoolStatuses(&co.Status)
-	_, err = optr.configClient.ConfigV1().ClusterOperators().UpdateStatus(co)
+	_, err = optr.configClient.UpdateStatus(co)
 	return err
 }
 
 func (optr *Operator) fetchClusterOperator() (*configv1.ClusterOperator, error) {
-	co, err := optr.configClient.ConfigV1().ClusterOperators().Get(optr.name, metav1.GetOptions{})
+	co, err := optr.configClient.Get(optr.name, metav1.GetOptions{})
 	if meta.IsNoMatchError(err) {
 		return nil, nil
 	}
@@ -139,7 +142,7 @@ func (optr *Operator) fetchClusterOperator() (*configv1.ClusterOperator, error)
 }
 
 func (optr *Operator) initializeClusterOperator() (*configv1.ClusterOperator, error) {
-	co, err := optr.configClient.ConfigV1().ClusterOperators().Create(&configv1.ClusterOperator{
+	co, err := optr.configClient.Create(&configv1.ClusterOperator{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: optr.name,
 		},
@@ -154,7 +157,7 @@ func (optr *Operator) initializeClusterOperator() (*configv1.ClusterOperator, er
 	co.Status.RelatedObjects = []configv1.ObjectReference{
 		{Resource: "namespaces", Name: "openshift-machine-config-operator"},
 	}
-	return optr.configClient.ConfigV1().ClusterOperators().UpdateStatus(co)
+	return optr.configClient.UpdateStatus(co)
 }
 
 func (optr *Operator) setMachineConfigPoolStatuses(status *configv1.ClusterOperatorStatus) {