Merge pull request #1794 from deads2k/apply-02-more-loops

API-1835: Apply 02 more loops

Author: openshift-merge-bot[bot]

pkg/controller/factory/interfaces.go

@@ -50,3 +50,7 @@ type SyncFunc func(ctx context.Context, controllerContext SyncContext) error
 func ControllerFieldManager(controllerName, usageName string) string {
 	return fmt.Sprintf("%s-%s", controllerName, usageName)
 }
+
+func ControllerInstanceName(instanceName, controllerName string) string {
+	return fmt.Sprintf("%s-%s", instanceName, controllerName)
+}

pkg/operator/README.md

@@ -4,13 +4,43 @@
 
 Creates and maintains an `apiservices.apiregistration.k8s.io`.
 
-### ResourceSyncController
+### AuditPolicy
+**Location** [apiserver/controller/audit](https://github.com/openshift/library-go/tree/master/pkg/operator/apiserver/controller/audit)
+
+Reads the `apiserver.config.openshift.io#.spec.audit` and creates a ConfigMap with the desired audit configuration.
+
+### ConfigObserver
+**Location** [configobserver](https://github.com/openshift/library-go/tree/master/pkg/operator/configobserver)
+
+Runs a series of configured observers to produce a RawExtension to set at `.spec.observedConfiguration`.
+This allows independently watching many different inputs to produce a single configuration with a `.metadata.generation`.
+Every field in the RawExtension must be set by a single observer.
+A single observer may set many fields.
+
+### LatencyProfile
+**Location** [latencyprofile](https://github.com/openshift/library-go/tree/master/pkg/operator/latencyprofile)
+
+Reads the `node.config.openshift.io` resource to determine a latency profile for deciding about node lifecycle 
+controller and related timing.
+This controller appears to be trying to indicate whether a level has been reached, but to be honest I cannot
+figure out how it's actually driving the change, which probably means that there is a configobserver somewhere.
+
+### ResourceSync
 **Location** [resourcesynccontroller](https://github.com/openshift/library-go/tree/master/pkg/operator/resourcesynccontroller)
 
 Copies a ConfigMap or Secret from one location to another.
 Can copy partial ConfigMaps or Secrets.
 
-### StaticResourceController
+### StaleConditions
+**Location** [staleconditions](https://github.com/openshift/library-go/tree/master/pkg/operator/staleconditions)
+
+Takes a list of conditions and their corresponding fieldManagers so that the condition can be removed on the 
+`<foo>.operator.openshift.io` resource.
+This is useful when old control loops are no longer needed.
+If you forget to do this and just delete the control loop, then on upgrade there is a "stuck" condition that 
+cannot be cleared and can impact the summary of status on `clusteroperator.config.openshift.io`.
+
+### StaticResources
 **Location** [staticresourcecontroller](https://github.com/openshift/library-go/tree/master/pkg/operator/staticresourcecontroller)
 
 Creates, maintains, and deletes resources that need little to no customization.

pkg/operator/apiserver/controller/apiservice/apiservice_controller.go

@@ -34,7 +34,7 @@ type GetAPIServicesToMangeFunc func() (enabled []*apiregistrationv1.APIService,
 type apiServicesPreconditionFuncType func([]*apiregistrationv1.APIService) (bool, error)
 
 type APIServiceController struct {
-	name                     string
+	controllerInstanceName   string
 	getAPIServicesToManageFn GetAPIServicesToMangeFunc
 	// preconditionsForEnabledAPIServices must return true before the apiservices will be created
 	preconditionsForEnabledAPIServices apiServicesPreconditionFuncType
@@ -46,7 +46,7 @@ type APIServiceController struct {
 }
 
 func NewAPIServiceController(
-	name, targetNamespace string,
+	instanceName, targetNamespace string,
 	getAPIServicesToManageFunc GetAPIServicesToMangeFunc,
 	operatorClient v1helpers.OperatorClient,
 	apiregistrationInformers apiregistrationinformers.SharedInformerFactory,
@@ -57,7 +57,7 @@ func NewAPIServiceController(
 	informers ...factory.Informer,
 ) factory.Controller {
 	c := &APIServiceController{
-		name: "APIServiceController_" + name,
+		controllerInstanceName: factory.ControllerInstanceName(instanceName, "APIService"),
 		preconditionsForEnabledAPIServices: preconditionsForEnabledAPIServices(
 			kubeInformersForNamespaces.InformersFor(targetNamespace).Core().V1().Endpoints().Lister(),
 			kubeInformersForNamespaces.InformersFor(metav1.NamespaceSystem).Core().V1().ConfigMaps().Lister(),
@@ -77,7 +77,7 @@ func NewAPIServiceController(
 			kubeInformersForNamespaces.InformersFor(metav1.NamespaceSystem).Core().V1().ConfigMaps().Informer(),
 			apiregistrationInformers.Apiregistration().V1().APIServices().Informer(),
 		)...,
-	).ToController(c.name, eventRecorder.WithComponentSuffix("apiservice-"+name+"-controller"))
+	).ToController(c.controllerInstanceName, eventRecorder.WithComponentSuffix("apiservice-"+instanceName+"-controller"))
 }
 
 func (c *APIServiceController) updateOperatorStatus(
@@ -121,7 +121,7 @@ func (c *APIServiceController) updateOperatorStatus(
 	defer func() {
 		status := applyoperatorv1.OperatorStatus().
 			WithConditions(conditionAPIServicesDegraded, conditionAPIServicesAvailable)
-		updateError := c.operatorClient.ApplyOperatorStatus(ctx, factory.ControllerFieldManager(c.name, "updateOperatorStatus"), status)
+		updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
 		if updateError != nil {
 			// overrides error returned through 'return <ERROR>' statement
 			err = updateError

pkg/operator/apiserver/controller/auditpolicy/auditpolicy_controller.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"time"
 
+	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
+
 	configv1 "github.com/openshift/api/config/v1"
 	operatorsv1 "github.com/openshift/api/operator/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -23,6 +25,7 @@ import (
 )
 
 type auditPolicyController struct {
+	controllerInstanceName               string
 	apiserverConfigLister                configv1listers.APIServerLister
 	kubeClient                           kubernetes.Interface
 	operatorClient                       v1helpers.OperatorClient
@@ -32,6 +35,7 @@ type auditPolicyController struct {
 // NewAuditPolicyController create a controller that watches the config.openshift.io/v1 APIServer object
 // and reconciles a ConfigMap in the target namespace with the audit.k8s.io/v1 policy.yaml file.
 func NewAuditPolicyController(
+	name string,
 	targetNamespace string,
 	targetConfigMapName string,
 	apiserverConfigLister configv1listers.APIServerLister,
@@ -42,18 +46,22 @@ func NewAuditPolicyController(
 	eventRecorder events.Recorder,
 ) factory.Controller {
 	c := &auditPolicyController{
-		operatorClient:        operatorClient,
-		apiserverConfigLister: apiserverConfigLister,
-		kubeClient:            kubeClient,
-		targetNamespace:       targetNamespace,
-		targetConfigMapName:   targetConfigMapName,
+		controllerInstanceName: factory.ControllerInstanceName(name, "AuditPolicy"),
+		operatorClient:         operatorClient,
+		apiserverConfigLister:  apiserverConfigLister,
+		kubeClient:             kubeClient,
+		targetNamespace:        targetNamespace,
+		targetConfigMapName:    targetConfigMapName,
 	}
 
 	return factory.New().WithSync(c.sync).ResyncEvery(10*time.Second).WithInformers(
 		configInformers.Config().V1().APIServers().Informer(),
 		kubeInformersForTargetNamesace.Core().V1().ConfigMaps().Informer(),
 		operatorClient.Informer(),
-	).ToController("auditPolicyController", eventRecorder.WithComponentSuffix("audit-policy-controller"))
+	).ToController(
+		"auditPolicyController", // don't change what is passed here unless you also remove the old FooDegraded condition
+		eventRecorder.WithComponentSuffix("audit-policy-controller"),
+	)
 }
 
 func (c *auditPolicyController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
@@ -81,19 +89,19 @@ func (c *auditPolicyController) sync(ctx context.Context, syncCtx factory.SyncCo
 	err = c.syncAuditPolicy(ctx, config.Spec.Audit, syncCtx.Recorder())
 
 	// update failing condition
-	cond := operatorv1.OperatorCondition{
-		Type:   "AuditPolicyDegraded",
-		Status: operatorv1.ConditionFalse,
-	}
+	condition := applyoperatorv1.OperatorCondition().
+		WithType("AuditPolicyDegraded").
+		WithStatus(operatorv1.ConditionFalse)
 	if err != nil {
-		cond.Status = operatorv1.ConditionTrue
-		cond.Reason = "Error"
-		cond.Message = err.Error()
+		condition = condition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("Error").
+			WithMessage(err.Error())
 	}
-	if _, _, updateError := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(cond)); updateError != nil {
-		if err == nil {
-			return updateError
-		}
+	status := applyoperatorv1.OperatorStatus().WithConditions(condition)
+	updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
+	if updateError != nil {
+		return updateError
 	}
 
 	return err

pkg/operator/apiserver/controller/nsfinalizer/finalizer_controller.go

@@ -53,7 +53,10 @@ func NewFinalizerController(
 	return factory.New().ResyncEvery(time.Minute).WithSync(c.sync).WithInformers(
 		kubeInformersForTargetNamespace.Core().V1().Pods().Informer(),
 		kubeInformersForTargetNamespace.Apps().V1().DaemonSets().Informer(),
-	).ToController(fullname, eventRecorder.WithComponentSuffix("finalizer-controller"))
+	).ToController(
+		fullname, // don't change what is passed here unless you also remove the old FooDegraded condition
+		eventRecorder.WithComponentSuffix("finalizer-controller"),
+	)
 }
 
 func (c finalizerController) sync(ctx context.Context, syncCtx factory.SyncContext) error {

pkg/operator/apiserver/controller/workload/workload.go

@@ -110,7 +110,10 @@ func NewController(name, operatorNamespace, targetNamespace, targetOperandVersio
 
 	return c.WithSync(controllerRef.sync).
 		WithInformers(informers...).
-		ToController(fmt.Sprintf("%sWorkloadController", name), eventRecorder)
+		ToController(
+			fmt.Sprintf("%sWorkloadController", name), // don't change what is passed here unless you also remove the old FooDegraded condition
+			eventRecorder,
+		)
 }
 
 func (c *Controller) sync(ctx context.Context, controllerContext factory.SyncContext) error {

pkg/operator/apiserver/controllerset/apiservercontrollerset.go

@@ -71,6 +71,7 @@ func (cw *controllerWrapper) prepare() (controller, error) {
 
 // APIServerControllerSet is a set of controllers that maintain a deployment of an API server and the namespace it's running in
 type APIServerControllerSet struct {
+	name           string
 	operatorClient v1helpers.OperatorClient
 	eventRecorder  events.Recorder
 
@@ -88,10 +89,12 @@ type APIServerControllerSet struct {
 }
 
 func NewAPIServerControllerSet(
+	name string,
 	operatorClient v1helpers.OperatorClient,
 	eventRecorder events.Recorder,
 ) *APIServerControllerSet {
 	apiServerControllerSet := &APIServerControllerSet{
+		name:           name,
 		operatorClient: operatorClient,
 		eventRecorder:  eventRecorder,
 	}
@@ -405,6 +408,7 @@ func (cs *APIServerControllerSet) WithAuditPolicyController(
 	kubeClient kubernetes.Interface,
 ) *APIServerControllerSet {
 	cs.auditPolicyController.controller = auditpolicy.NewAuditPolicyController(
+		cs.name,
 		targetNamespace,
 		targetConfigMapName,
 		apiserverConfigLister,

pkg/operator/certrotation/client_cert_rotation_controller.go

@@ -98,7 +98,10 @@ func NewCertRotationController(
 		WithPostStartHooks(
 			c.targetCertRecheckerPostRunHook,
 		).
-		ToController("CertRotationController", recorder.WithComponentSuffix("cert-rotation-controller").WithComponentSuffix(name))
+		ToController(
+			"CertRotationController", // don't change what is passed here unless you also remove the old FooDegraded condition
+			recorder.WithComponentSuffix("cert-rotation-controller").WithComponentSuffix(name),
+		)
 }
 
 func (c CertRotationController) Sync(ctx context.Context, syncCtx factory.SyncContext) error {

pkg/operator/configobserver/config_observer_controller.go

@@ -9,6 +9,8 @@ import (
 	"strings"
 	"time"
 
+	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
+
 	"github.com/imdario/mergo"
 	"k8s.io/klog/v2"
 
@@ -44,6 +46,8 @@ type Listers interface {
 type ObserveConfigFunc func(listers Listers, recorder events.Recorder, existingConfig map[string]interface{}) (observedConfig map[string]interface{}, errs []error)
 
 type ConfigObserver struct {
+	controllerInstanceName string
+
 	// observers are called in an undefined order and their results are merged to
 	// determine the observed configuration.
 	observers []ObserveConfigFunc
@@ -58,13 +62,15 @@ type ConfigObserver struct {
 }
 
 func NewConfigObserver(
+	name string,
 	operatorClient v1helpers.OperatorClient,
 	eventRecorder events.Recorder,
 	listers Listers,
 	informers []factory.Informer,
 	observers ...ObserveConfigFunc,
 ) factory.Controller {
 	return NewNestedConfigObserver(
+		name,
 		operatorClient,
 		eventRecorder,
 		listers,
@@ -95,6 +101,7 @@ func NewConfigObserver(
 // oauthAPIController    := NewNestedConfigObserver(..., []string{"oauthAPIServer"}
 // oauthServerController := NewNestedConfigObserver(..., []string{"oauthServer"}
 func NewNestedConfigObserver(
+	name string,
 	operatorClient v1helpers.OperatorClient,
 	eventRecorder events.Recorder,
 	listers Listers,
@@ -104,14 +111,22 @@ func NewNestedConfigObserver(
 	observers ...ObserveConfigFunc,
 ) factory.Controller {
 	c := &ConfigObserver{
-		operatorClient:        operatorClient,
-		observers:             observers,
-		listers:               listers,
-		nestedConfigPath:      nestedConfigPath,
-		degradedConditionType: degradedConditionPrefix + condition.ConfigObservationDegradedConditionType,
+		controllerInstanceName: factory.ControllerInstanceName(name, "ConfigObserver"),
+		operatorClient:         operatorClient,
+		observers:              observers,
+		listers:                listers,
+		nestedConfigPath:       nestedConfigPath,
+		degradedConditionType:  degradedConditionPrefix + condition.ConfigObservationDegradedConditionType,
 	}
 
-	return factory.New().ResyncEvery(time.Minute).WithSync(c.sync).WithInformers(append(informers, listersToInformer(listers)...)...).ToController("ConfigObserver", eventRecorder.WithComponentSuffix("config-observer"))
+	return factory.New().
+		ResyncEvery(time.Minute).
+		WithSync(c.sync).
+		WithInformers(append(informers, listersToInformer(listers)...)...).
+		ToController(
+			"ConfigObserver", // don't change what is passed here unless you also remove the old FooDegraded condition
+			eventRecorder.WithComponentSuffix("config-observer"),
+		)
 }
 
 // sync reacts to a change in prereqs by finding information that is required to match another value in the cluster. This
@@ -165,16 +180,18 @@ func (c ConfigObserver) sync(ctx context.Context, syncCtx factory.SyncContext) e
 	configError := v1helpers.NewMultiLineAggregate(errs)
 
 	// update failing condition
-	cond := operatorv1.OperatorCondition{
-		Type:   c.degradedConditionType,
-		Status: operatorv1.ConditionFalse,
-	}
+	condition := applyoperatorv1.OperatorCondition().
+		WithType(c.degradedConditionType).
+		WithStatus(operatorv1.ConditionFalse)
 	if configError != nil {
-		cond.Status = operatorv1.ConditionTrue
-		cond.Reason = "Error"
-		cond.Message = configError.Error()
+		condition = condition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("Error").
+			WithMessage(configError.Error())
 	}
-	if _, _, updateError := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(cond)); updateError != nil {
+	status := applyoperatorv1.OperatorStatus().WithConditions(condition)
+	updateError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status)
+	if updateError != nil {
 		return updateError
 	}
 

pkg/operator/configobserver/config_observer_controller_test.go

@@ -3,6 +3,7 @@ package configobserver
 import (
 	"context"
 	"fmt"
+	"k8s.io/apimachinery/pkg/util/json"
 	"reflect"
 	"strings"
 	"testing"
@@ -70,6 +71,16 @@ func (c *fakeOperatorClient) ApplyOperatorSpec(ctx context.Context, fieldManager
 }
 
 func (c *fakeOperatorClient) ApplyOperatorStatus(ctx context.Context, fieldManager string, applyConfiguration *applyoperatorv1.OperatorStatusApplyConfiguration) (err error) {
+	applyJSON, err := json.Marshal(applyConfiguration)
+	if err != nil {
+		return fmt.Errorf("marshal failure: %w", err)
+	}
+	status := &operatorv1.OperatorStatus{}
+	if err := json.Unmarshal(applyJSON, status); err != nil {
+		return fmt.Errorf("unmarshal failure: %w", err)
+	}
+	c.status = status
+
 	return nil
 }