Merge pull request #1830 from bertinatto/try-apply-workload

API-1835: Migrate WorkloadController to SSA

Author: openshift-merge-bot[bot]

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

@@ -20,10 +20,10 @@ import (
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	openshiftconfigclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
+	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
 	"github.com/openshift/library-go/pkg/apps/deployment"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
-	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
 	"github.com/openshift/library-go/pkg/operator/status"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 )
@@ -49,6 +49,7 @@ type Delegate interface {
 // Callers must provide a sync function for delegation. It should bring the desired workload into operation.
 // The returned state along with errors will be converted into conditions and persisted in the status field.
 type Controller struct {
+	controllerInstanceName string
 	// conditionsPrefix an optional prefix that will be used as operator's condition type field for example APIServerDeploymentDegraded where APIServer indicates the prefix
 	conditionsPrefix     string
 	operatorNamespace    string
@@ -71,13 +72,13 @@ type Controller struct {
 
 // NewController creates a brand new Controller instance.
 //
-// the "name" param will be used to set conditions in the status field. It will be suffixed with "WorkloadController",
+// the "instanceName" param will be used to set conditions in the status field. It will be suffixed with "WorkloadController",
 // so it can end up in the condition in the form of "OAuthAPIWorkloadControllerDeploymentAvailable"
 //
 // the "operatorNamespace" is used to set "version-mapping" in the correct namespace
 //
 // the "targetNamespace" represent the namespace for the managed resource (DaemonSet)
-func NewController(name, operatorNamespace, targetNamespace, targetOperandVersion, operandNamePrefix, conditionsPrefix string,
+func NewController(instanceName, operatorNamespace, targetNamespace, targetOperandVersion, operandNamePrefix, conditionsPrefix string,
 	operatorClient v1helpers.OperatorClient,
 	kubeClient kubernetes.Interface,
 	podLister corev1listers.PodLister,
@@ -89,6 +90,7 @@ func NewController(name, operatorNamespace, targetNamespace, targetOperandVersio
 	versionRecorder status.VersionGetter,
 ) factory.Controller {
 	controllerRef := &Controller{
+		controllerInstanceName:       factory.ControllerInstanceName(instanceName, "Workload"),
 		operatorNamespace:            operatorNamespace,
 		targetNamespace:              targetNamespace,
 		targetOperandVersion:         targetOperandVersion,
@@ -100,7 +102,7 @@ func NewController(name, operatorNamespace, targetNamespace, targetOperandVersio
 		delegate:                     delegate,
 		openshiftClusterConfigClient: openshiftClusterConfigClient,
 		versionRecorder:              versionRecorder,
-		queue:                        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), name),
+		queue:                        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), instanceName),
 	}
 
 	c := factory.New()
@@ -111,7 +113,7 @@ func NewController(name, operatorNamespace, targetNamespace, targetOperandVersio
 	return c.WithSync(controllerRef.sync).
 		WithInformers(informers...).
 		ToController(
-			fmt.Sprintf("%sWorkloadController", name), // don't change what is passed here unless you also remove the old FooDegraded condition
+			fmt.Sprintf("%sWorkloadController", controllerRef.controllerInstanceName),
 			eventRecorder,
 		)
 }
@@ -161,40 +163,40 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 		errs = []error{}
 	}
 
-	deploymentAvailableCondition := operatorv1.OperatorCondition{
-		Type:   fmt.Sprintf("%sDeployment%s", c.conditionsPrefix, operatorv1.OperatorStatusTypeAvailable),
-		Status: operatorv1.ConditionTrue,
-	}
+	deploymentAvailableCondition := applyoperatorv1.OperatorCondition().
+		WithType(fmt.Sprintf("%sDeployment%s", c.conditionsPrefix, operatorv1.OperatorStatusTypeAvailable))
 
-	workloadDegradedCondition := operatorv1.OperatorCondition{
-		Type:   fmt.Sprintf("%sWorkloadDegraded", c.conditionsPrefix),
-		Status: operatorv1.ConditionFalse,
-	}
+	workloadDegradedCondition := applyoperatorv1.OperatorCondition().
+		WithType(fmt.Sprintf("%sWorkloadDegraded", c.conditionsPrefix))
 
-	deploymentDegradedCondition := operatorv1.OperatorCondition{
-		Type:   fmt.Sprintf("%sDeploymentDegraded", c.conditionsPrefix),
-		Status: operatorv1.ConditionFalse,
-	}
+	deploymentDegradedCondition := applyoperatorv1.OperatorCondition().
+		WithType(fmt.Sprintf("%sDeploymentDegraded", c.conditionsPrefix))
 
-	deploymentProgressingCondition := operatorv1.OperatorCondition{
-		Type:   fmt.Sprintf("%sDeployment%s", c.conditionsPrefix, operatorv1.OperatorStatusTypeProgressing),
-		Status: operatorv1.ConditionFalse,
+	deploymentProgressingCondition := applyoperatorv1.OperatorCondition().
+		WithType(fmt.Sprintf("%sDeployment%s", c.conditionsPrefix, operatorv1.OperatorStatusTypeProgressing))
+
+	status := applyoperatorv1.OperatorStatus()
+	if workload != nil {
+		// The Hash field is not required since the LastGeneration field is enough to uniquely identify a Deployment's desired state
+		status = status.WithGenerations(applyoperatorv1.GenerationStatus().
+			WithGroup("apps").
+			WithResource("deployments").
+			WithNamespace(workload.Namespace).
+			WithName(workload.Name).
+			WithLastGeneration(workload.Generation),
+		)
 	}
 
-	// only set updateGenerationFn to update the observed generation if everything is available
-	var updateGenerationFn func(newStatus *operatorv1.OperatorStatus) error
 	defer func() {
-		updates := []v1helpers.UpdateStatusFunc{
-			v1helpers.UpdateConditionFn(deploymentAvailableCondition),
-			v1helpers.UpdateConditionFn(deploymentDegradedCondition),
-			v1helpers.UpdateConditionFn(deploymentProgressingCondition),
-			v1helpers.UpdateConditionFn(workloadDegradedCondition),
-		}
-		if updateGenerationFn != nil {
-			updates = append(updates, updateGenerationFn)
-		}
-		if _, _, updateError := v1helpers.UpdateStatus(ctx, c.operatorClient, updates...); updateError != nil {
-			err = updateError
+		status = status.WithConditions(
+			deploymentAvailableCondition,
+			deploymentDegradedCondition,
+			deploymentProgressingCondition,
+			workloadDegradedCondition,
+		)
+
+		if applyError := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, status); applyError != nil {
+			err = applyError
 		}
 	}()
 
@@ -209,15 +211,23 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 
 		// we are degraded, not available and we are not progressing
 
-		deploymentDegradedCondition.Status = operatorv1.ConditionTrue
-		deploymentDegradedCondition.Reason = "PreconditionNotFulfilled"
-		deploymentDegradedCondition.Message = message
+		deploymentDegradedCondition = deploymentDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("PreconditionNotFulfilled").
+			WithMessage(message)
 
-		deploymentAvailableCondition.Status = operatorv1.ConditionFalse
-		deploymentAvailableCondition.Reason = "PreconditionNotFulfilled"
+		deploymentAvailableCondition = deploymentAvailableCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("PreconditionNotFulfilled")
 
-		deploymentProgressingCondition.Status = operatorv1.ConditionFalse
-		deploymentProgressingCondition.Reason = "PreconditionNotFulfilled"
+		deploymentProgressingCondition = deploymentProgressingCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("PreconditionNotFulfilled")
+
+		workloadDegradedCondition = workloadDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("PreconditionNotFulfilled").
+			WithMessage(message)
 
 		return kerrors.NewAggregate(errs)
 	}
@@ -227,37 +237,49 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 		for _, err := range errs {
 			message = message + err.Error() + "\n"
 		}
-		workloadDegradedCondition.Status = operatorv1.ConditionTrue
-		workloadDegradedCondition.Reason = "SyncError"
-		workloadDegradedCondition.Message = message
+		workloadDegradedCondition = workloadDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("SyncError").
+			WithMessage(message)
+	} else if workload == nil {
+		workloadDegradedCondition = workloadDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("NoDeployment").
+			WithMessage(fmt.Sprintf("deployment/%s: could not be retrieved", c.targetNamespace))
 	} else {
-		workloadDegradedCondition.Status = operatorv1.ConditionFalse
+		workloadDegradedCondition = workloadDegradedCondition.
+			WithStatus(operatorv1.ConditionFalse)
 	}
 
 	if workload == nil {
 		message := fmt.Sprintf("deployment/%s: could not be retrieved", c.targetNamespace)
-		deploymentAvailableCondition.Status = operatorv1.ConditionFalse
-		deploymentAvailableCondition.Reason = "NoDeployment"
-		deploymentAvailableCondition.Message = message
+		deploymentAvailableCondition = deploymentAvailableCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("NoDeployment").
+			WithMessage(message)
 
-		deploymentProgressingCondition.Status = operatorv1.ConditionTrue
-		deploymentProgressingCondition.Reason = "NoDeployment"
-		deploymentProgressingCondition.Message = message
+		deploymentProgressingCondition = deploymentProgressingCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("NoDeployment").
+			WithMessage(message)
 
-		deploymentDegradedCondition.Status = operatorv1.ConditionTrue
-		deploymentDegradedCondition.Reason = "NoDeployment"
-		deploymentDegradedCondition.Message = message
+		deploymentDegradedCondition = deploymentDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("NoDeployment").
+			WithMessage(message)
 
 		return kerrors.NewAggregate(errs)
 	}
 
 	if workload.Status.AvailableReplicas == 0 {
-		deploymentAvailableCondition.Status = operatorv1.ConditionFalse
-		deploymentAvailableCondition.Reason = "NoPod"
-		deploymentAvailableCondition.Message = fmt.Sprintf("no %s.%s pods available on any node.", workload.Name, c.targetNamespace)
+		deploymentAvailableCondition = deploymentAvailableCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("NoPod").
+			WithMessage(fmt.Sprintf("no %s.%s pods available on any node.", workload.Name, c.targetNamespace))
 	} else {
-		deploymentAvailableCondition.Status = operatorv1.ConditionTrue
-		deploymentAvailableCondition.Reason = "AsExpected"
+		deploymentAvailableCondition = deploymentAvailableCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("AsExpected")
 	}
 
 	desiredReplicas := int32(1)
@@ -268,18 +290,21 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 	// If the workload is up to date, then we are no longer progressing
 	workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration
 	workloadIsBeingUpdated := workload.Status.UpdatedReplicas < desiredReplicas
-	workloadIsBeingUpdatedTooLong, err := isUpdatingTooLong(previousStatus, deploymentProgressingCondition.Type)
+	workloadIsBeingUpdatedTooLong, err := isUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type)
 	if !workloadAtHighestGeneration {
-		deploymentProgressingCondition.Status = operatorv1.ConditionTrue
-		deploymentProgressingCondition.Reason = "NewGeneration"
-		deploymentProgressingCondition.Message = fmt.Sprintf("deployment/%s.%s: observed generation is %d, desired generation is %d.", workload.Name, c.targetNamespace, workload.Status.ObservedGeneration, workload.ObjectMeta.Generation)
+		deploymentProgressingCondition = deploymentProgressingCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("NewGeneration").
+			WithMessage(fmt.Sprintf("deployment/%s.%s: observed generation is %d, desired generation is %d.", workload.Name, c.targetNamespace, workload.Status.ObservedGeneration, workload.ObjectMeta.Generation))
 	} else if workloadIsBeingUpdated {
-		deploymentProgressingCondition.Status = operatorv1.ConditionTrue
-		deploymentProgressingCondition.Reason = "PodsUpdating"
-		deploymentProgressingCondition.Message = fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas)
+		deploymentProgressingCondition = deploymentProgressingCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("PodsUpdating").
+			WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas))
 	} else {
-		deploymentProgressingCondition.Status = operatorv1.ConditionFalse
-		deploymentProgressingCondition.Reason = "AsExpected"
+		deploymentProgressingCondition = deploymentProgressingCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("AsExpected")
 	}
 
 	// During a rollout the default maxSurge (25%) will allow the available
@@ -288,17 +313,19 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 	workloadHasAllPodsAvailable := workload.Status.AvailableReplicas >= desiredReplicas
 	if !workloadHasAllPodsAvailable && (!workloadIsBeingUpdated || workloadIsBeingUpdatedTooLong) {
 		numNonAvailablePods := desiredReplicas - workload.Status.AvailableReplicas
-		deploymentDegradedCondition.Status = operatorv1.ConditionTrue
-		deploymentDegradedCondition.Reason = "UnavailablePod"
+		deploymentDegradedCondition = deploymentDegradedCondition.
+			WithStatus(operatorv1.ConditionTrue).
+			WithReason("UnavailablePod")
 		podContainersStatus, err := deployment.PodContainersStatus(workload, c.podsLister)
 		if err != nil {
 			podContainersStatus = []string{fmt.Sprintf("failed to get pod containers details: %v", err)}
 		}
-		deploymentDegradedCondition.Message = fmt.Sprintf("%v of %v requested instances are unavailable for %s.%s (%s)", numNonAvailablePods, desiredReplicas, workload.Name, c.targetNamespace,
-			strings.Join(podContainersStatus, ", "))
+		deploymentDegradedCondition = deploymentDegradedCondition.
+			WithMessage(fmt.Sprintf("%v of %v requested instances are unavailable for %s.%s (%s)", numNonAvailablePods, desiredReplicas, workload.Name, c.targetNamespace, strings.Join(podContainersStatus, ", ")))
 	} else {
-		deploymentDegradedCondition.Status = operatorv1.ConditionFalse
-		deploymentDegradedCondition.Reason = "AsExpected"
+		deploymentDegradedCondition = deploymentDegradedCondition.
+			WithStatus(operatorv1.ConditionFalse).
+			WithReason("AsExpected")
 	}
 
 	// if the deployment is all available and at the expected generation, then update the version to the latest
@@ -313,12 +340,6 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 		c.versionRecorder.SetVersion(operandName, c.targetOperandVersion)
 	}
 
-	// set updateGenerationFn so that it is invoked in defer
-	updateGenerationFn = func(newStatus *operatorv1.OperatorStatus) error {
-		resourcemerge.SetDeploymentGeneration(&newStatus.Generations, workload)
-		return nil
-	}
-
 	if len(errs) > 0 {
 		return kerrors.NewAggregate(errs)
 	}

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

@@ -71,8 +71,10 @@ func TestUpdateOperatorStatus(t *testing.T) {
 						Message: "deployment/: could not be retrieved",
 					},
 					{
-						Type:   fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
-						Status: operatorv1.ConditionFalse,
+						Type:    fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
+						Status:  operatorv1.ConditionTrue,
+						Reason:  "NoDeployment",
+						Message: "deployment/: could not be retrieved",
 					},
 					{
 						Type:    fmt.Sprintf("%sDeploymentDegraded", defaultControllerName),
@@ -441,8 +443,10 @@ func TestUpdateOperatorStatus(t *testing.T) {
 						Message: "",
 					},
 					{
-						Type:   fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
-						Status: operatorv1.ConditionFalse,
+						Type:    fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
+						Status:  operatorv1.ConditionTrue,
+						Reason:  "PreconditionNotFulfilled",
+						Message: "the operator didn't specify what preconditions are missing",
 					},
 				}
 				return areCondidtionsEqual(expectedConditions, actualStatus.Conditions)