Merge pull request #163 from enj/enj/i/remove_degraded_4.1

[release-4.1] Bug 1725936: Split handling of degraded across multiple conditions

glide.yaml

@@ -16,11 +16,11 @@ import:
 - package: k8s.io/code-generator
   version: kubernetes-1.13.4
 - package: github.com/openshift/api
-  version: master
+  version: release-4.1
 - package: github.com/openshift/client-go
-  version: master
+  version: release-4.1
 - package: github.com/openshift/library-go
-  version: master
+  version: release-4.1
 
 # bindata
 - package: github.com/jteeuwen/go-bindata

pkg/operator2/oauth.go

@@ -82,9 +82,7 @@ func (c *authOperator) handleOAuthConfig(
 			},
 		)
 	}
-	if err := v1helpers.NewMultiLineAggregate(errsIDP); err != nil {
-		setDegradedTrue(operatorConfig, "IdentityProviderConfigError", err.Error())
-	}
+	handleDegraded(operatorConfig, "IdentityProviderConfig", v1helpers.NewMultiLineAggregate(errsIDP))
 
 	assetPublicURL, corsAllowedOrigins := consoleToDeploymentData(consoleConfig)
 

pkg/operator2/operator.go

@@ -244,20 +244,19 @@ func (c *authOperator) Key() (metav1.Object, error) {
 func (c *authOperator) Sync(obj metav1.Object) error {
 	operatorConfig := obj.(*operatorv1.Authentication)
 
-	// TODO bump and use IsOperatorManaged
 	if operatorConfig.Spec.ManagementState != operatorv1.Managed {
 		return nil // TODO do something better for all states
 	}
 
 	operatorConfigCopy := operatorConfig.DeepCopy()
 
-	// clear degraded status
-	setDegradedFalse(operatorConfigCopy)
-
 	syncErr := c.handleSync(operatorConfigCopy)
-	if syncErr != nil {
-		setDegradedTrue(operatorConfigCopy, "OperatorSyncLoopError", syncErr.Error())
+	// this is a catch all degraded state that we only set when we are otherwise not degraded
+	globalDegradedErr := syncErr
+	if isDegraded(operatorConfigCopy) {
+		globalDegradedErr = nil // unset because we are already degraded for some other reason
 	}
+	handleDegraded(operatorConfigCopy, "OperatorSync", globalDegradedErr)
 
 	if _, _, err := v1helpers.UpdateStatus(c.authOperatorConfigClient, func(status *operatorv1.OperatorStatus) error {
 		// store a copy of our starting conditions, we need to preserve last transition time
@@ -312,6 +311,7 @@ func (c *authOperator) handleSync(operatorConfig *operatorv1.Authentication) err
 	resourceVersions = append(resourceVersions, ingress.GetResourceVersion())
 
 	route, routerSecret, err := c.handleRoute(ingress)
+	handleDegraded(operatorConfig, "RouteStatus", err)
 	if err != nil {
 		return fmt.Errorf("failed handling the route: %v", err)
 	}
@@ -446,6 +446,7 @@ func (c *authOperator) handleVersion(
 	// but we do NOT want to go to the next version until all OAuth server pods are at that version
 
 	routeReady, routeMsg, err := c.checkRouteHealthy(route, routerSecret, ingress)
+	handleDegraded(operatorConfig, "RouteHealth", err)
 	if err != nil {
 		return fmt.Errorf("unable to check route health: %v", err)
 	}
@@ -455,6 +456,7 @@ func (c *authOperator) handleVersion(
 	}
 
 	wellknownReady, wellknownMsg, err := c.checkWellknownEndpointsReady(authConfig, route)
+	handleDegraded(operatorConfig, "WellKnownEndpoint", err)
 	if err != nil {
 		return fmt.Errorf("unable to check the .well-known endpoint: %v", err)
 	}
@@ -464,6 +466,7 @@ func (c *authOperator) handleVersion(
 	}
 
 	oauthClientsReady, oauthClientsMsg, err := c.oauthClientsReady(route)
+	handleDegraded(operatorConfig, "OAuthClients", err)
 	if err != nil {
 		return fmt.Errorf("unable to check OAuth clients' readiness: %v", err)
 	}

pkg/operator2/route.go

@@ -23,17 +23,20 @@ func (c *authOperator) handleRoute(ingress *configv1.Ingress) (*routev1.Route, *
 		return nil, nil, err
 	}
 
+	host := getCanonicalHost(route, ingress)
+	if len(host) == 0 {
+		// be careful not to print route.spec as it many contain secrets
+		return nil, nil, fmt.Errorf("route is not available at canonical host %s: %+v", ingressToHost(ingress), route.Status.Ingress)
+	}
+
 	// assume it is unsafe to mutate route in case we go to a shared informer in the future
 	// this way everything else can just assume route.Spec.Host is correct
 	// note that we are not updating route.Spec.Host in the API - that value is nonsense to us
 	route = route.DeepCopy()
-	route.Spec.Host = getCanonicalHost(route, ingress)
-
-	if len(route.Spec.Host) == 0 {
-		return nil, nil, fmt.Errorf("route has no host: %#v", route)
-	}
+	route.Spec.Host = host
 
 	if err := isValidRoute(route, ingress); err != nil {
+		// TODO remove this delete so that we do not lose the early creation timestamp of our route
 		// delete the route so that it is replaced with the proper one in next reconcile loop
 		klog.Infof("deleting invalid route: %#v", route)
 		opts := &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &route.UID}}
@@ -48,7 +51,8 @@ func (c *authOperator) handleRoute(ingress *configv1.Ingress) (*routev1.Route, *
 		return nil, nil, err
 	}
 	if len(routerSecret.Data) == 0 {
-		return nil, nil, fmt.Errorf("router secret is empty: %#v", routerSecret)
+		// be careful not to print the routerSecret even when it is empty
+		return nil, nil, fmt.Errorf("router secret %s/%s is empty", routerSecret.Namespace, routerSecret.Name)
 	}
 
 	return route, routerSecret, nil

pkg/operator2/starter.go

@@ -19,8 +19,12 @@ import (
 	routeclient "github.com/openshift/client-go/route/clientset/versioned"
 	routeinformer "github.com/openshift/client-go/route/informers/externalversions"
 	"github.com/openshift/library-go/pkg/controller/controllercmd"
+	"github.com/openshift/library-go/pkg/operator/loglevel"
+	"github.com/openshift/library-go/pkg/operator/management"
 	"github.com/openshift/library-go/pkg/operator/resourcesynccontroller"
+	"github.com/openshift/library-go/pkg/operator/staleconditions"
 	"github.com/openshift/library-go/pkg/operator/status"
+	"github.com/openshift/library-go/pkg/operator/unsupportedconfigoverridescontroller"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 )
 
@@ -153,6 +157,23 @@ func RunOperator(ctx *controllercmd.ControllerContext) error {
 		ctx.EventRecorder,
 	)
 
+	staleConditions := staleconditions.NewRemoveStaleConditions(
+		[]string{
+			// in 4.1.0 this was accidentally in the list.  This can be removed in 4.3.
+			"Degraded",
+		},
+		operatorClient,
+		ctx.EventRecorder,
+	)
+
+	configOverridesController := unsupportedconfigoverridescontroller.NewUnsupportedConfigOverridesController(operatorClient, ctx.EventRecorder)
+	logLevelController := loglevel.NewClusterOperatorLoggingController(operatorClient, ctx.EventRecorder)
+	// TODO remove this controller once we support Removed
+	managementStateController := management.NewOperatorManagementStateController(clusterOperatorName, operatorClient, ctx.EventRecorder)
+	management.SetOperatorNotRemovable()
+	// TODO move to config observers
+	// configobserver.NewConfigObserver(...)
+
 	for _, informer := range []interface {
 		Start(stopCh <-chan struct{})
 	}{
@@ -165,9 +186,20 @@ func RunOperator(ctx *controllercmd.ControllerContext) error {
 		informer.Start(ctx.Done())
 	}
 
+	for _, controller := range []interface {
+		Run(workers int, stopCh <-chan struct{})
+	}{
+		resourceSyncer,
+		clusterOperatorStatus,
+		configOverridesController,
+		logLevelController,
+		managementStateController,
+	} {
+		go controller.Run(1, ctx.Done())
+	}
+
 	go operator.Run(ctx.Done())
-	go resourceSyncer.Run(1, ctx.Done())
-	go clusterOperatorStatus.Run(1, ctx.Done())
+	go staleConditions.Run(1, ctx.Done())
 
 	<-ctx.Done()
 

pkg/operator2/status.go

@@ -1,26 +1,38 @@
 package operator2
 
 import (
+	"strings"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 )
 
-func setDegradedTrue(operatorConfig *operatorv1.Authentication, reason, message string) {
+func handleDegraded(operatorConfig *operatorv1.Authentication, prefix string, err error) {
+	if err != nil {
+		v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
+			operatorv1.OperatorCondition{
+				Type:    prefix + operatorv1.OperatorStatusTypeDegraded,
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "Error",
+				Message: err.Error(),
+			})
+		return
+	}
 	v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
 		operatorv1.OperatorCondition{
-			Type:    operatorv1.OperatorStatusTypeDegraded,
-			Status:  operatorv1.ConditionTrue,
-			Reason:  reason,
-			Message: message,
+			Type:   prefix + operatorv1.OperatorStatusTypeDegraded,
+			Status: operatorv1.ConditionFalse,
 		})
 }
 
-func setDegradedFalse(operatorConfig *operatorv1.Authentication) {
-	v1helpers.SetOperatorCondition(&operatorConfig.Status.Conditions,
-		operatorv1.OperatorCondition{
-			Type:   operatorv1.OperatorStatusTypeDegraded,
-			Status: operatorv1.ConditionFalse,
-		})
+func isDegraded(operatorConfig *operatorv1.Authentication) bool {
+	for _, condition := range operatorConfig.Status.Conditions {
+		if strings.HasSuffix(condition.Type, operatorv1.OperatorStatusTypeDegraded) &&
+			condition.Status == operatorv1.ConditionTrue {
+			return true
+		}
+	}
+	return false
 }
 
 func setProgressingTrue(operatorConfig *operatorv1.Authentication, reason, message string) {