Merge pull request #1977 from liouk/AUTH-543

AUTH-543: Add workload deletion condition method and clean-up

Author: openshift-merge-bot[bot]

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

@@ -4,6 +4,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
+
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -14,7 +16,6 @@ import (
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
-	"strings"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	openshiftconfigclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
@@ -41,6 +42,12 @@ type Delegate interface {
 	// operator will be degraded, not available and not progressing
 	// returned errors (if any) will be added to the Message field
 	PreconditionFulfilled(ctx context.Context) (bool, error)
+
+	// WorkloadDeleted indicates whether the delegate workload has been deleted or not. It returns a bool
+	// flag to indicate this, a string representing the workload's name and an error. When true, this
+	// controller will remove any fields from the operator status that the controller is managing, and
+	// will also remove the version field.
+	WorkloadDeleted(ctx context.Context) (bool, string, error)
 }
 
 // Controller is a generic workload controller that deals with Deployment resource.
@@ -133,6 +140,20 @@ func (c *Controller) sync(ctx context.Context, controllerContext factory.SyncCon
 		return c.updateOperatorStatus(ctx, operatorStatus, nil, false, false, nil)
 	}
 
+	if deleted, operandName, err := c.delegate.WorkloadDeleted(ctx); err != nil {
+		return c.updateOperatorStatus(ctx, operatorStatus, nil, false, false, []error{err})
+	} else if deleted {
+		// Server-Side-Apply with an empty operator status for the specific field manager will effectively
+		// remove any conditions and generations owned by it, because the respective API fields have 'map'
+		// as the list type where field managers can be list element-specific
+		if err := c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, applyoperatorv1.OperatorStatus()); err != nil {
+			return err
+		}
+
+		c.versionRecorder.UnsetVersion(c.constructOperandNameFor(operandName))
+		return nil
+	}
+
 	workload, operatorConfigAtHighestGeneration, errs := c.delegate.Sync(ctx, controllerContext)
 
 	return c.updateOperatorStatus(ctx, operatorStatus, workload, operatorConfigAtHighestGeneration, true, errs)
@@ -341,11 +362,7 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 	// which should immediately result in a deployment generation diff, which should cause this block to be skipped until it is ready.
 	workloadHasAllPodsUpdated := workload.Status.UpdatedReplicas == desiredReplicas
 	if workloadAtHighestGeneration && workloadHasAllPodsAvailable && workloadHasAllPodsUpdated && operatorConfigAtHighestGeneration {
-		operandName := workload.Name
-		if len(c.operandNamePrefix) > 0 {
-			operandName = fmt.Sprintf("%s-%s", c.operandNamePrefix, workload.Name)
-		}
-		c.versionRecorder.SetVersion(operandName, c.targetOperandVersion)
+		c.versionRecorder.SetVersion(c.constructOperandNameFor(workload.Name), c.targetOperandVersion)
 	}
 
 	if len(errs) > 0 {
@@ -354,6 +371,14 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 	return nil
 }
 
+func (c *Controller) constructOperandNameFor(name string) string {
+	if len(c.operandNamePrefix) > 0 {
+		return fmt.Sprintf("%s-%s", c.operandNamePrefix, name)
+	}
+
+	return name
+}
+
 // hasDeploymentProgressed returns true if the deployment reports NewReplicaSetAvailable
 // via the DeploymentProgressing condition
 func hasDeploymentProgressed(status appsv1.DeploymentStatus) bool {

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

@@ -3,13 +3,13 @@ package workload
 import (
 	"context"
 	"fmt"
-	clocktesting "k8s.io/utils/clock/testing"
 	"testing"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	corev1listers "k8s.io/client-go/listers/core/v1"
+	clocktesting "k8s.io/utils/clock/testing"
 	"k8s.io/utils/ptr"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -39,6 +39,10 @@ type testDelegate struct {
 	syncErrrors             []error
 }
 
+func (d *testDelegate) WorkloadDeleted(_ context.Context) (bool, string, error) {
+	return false, "", nil
+}
+
 func (d *testDelegate) PreconditionFulfilled(_ context.Context) (bool, error) {
 	return d.preconditionReady, d.preconditionErr
 }

pkg/operator/status/status_controller.go

@@ -2,22 +2,21 @@ package status
 
 import (
 	"context"
-	"k8s.io/utils/clock"
 	"strings"
 	"time"
 
+	"k8s.io/apimachinery/pkg/api/equality"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/klog/v2"
+	"k8s.io/utils/clock"
 
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 	configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1"
 	configv1listers "github.com/openshift/client-go/config/listers/config/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-
 	configv1helpers "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
@@ -29,6 +28,8 @@ import (
 type VersionGetter interface {
 	// SetVersion is a way to set the version for an operand.  It must be thread-safe
 	SetVersion(operandName, version string)
+	// UnsetVersion removes a version for an operand if it exists; it is a no-op otherwise.  It must be thread-safe
+	UnsetVersion(operandName string)
 	// GetVersion is way to get the versions for all operands.  It must be thread-safe and return an object that doesn't mutate
 	GetVersions() map[string]string
 	// VersionChangedChannel is a channel that will get an item whenever SetVersion has been called

pkg/operator/status/version.go

@@ -35,14 +35,19 @@ func (v *versionGetter) SetVersion(operandName, version string) {
 	defer v.lock.Unlock()
 
 	v.versions[operandName] = version
+	v.notifyChannelsLocked()
+}
 
-	for i := range v.notificationChannels {
-		ch := v.notificationChannels[i]
-		// don't let a slow consumer block the rest
-		go func() {
-			ch <- struct{}{}
-		}()
+func (v *versionGetter) UnsetVersion(operandName string) {
+	v.lock.Lock()
+	defer v.lock.Unlock()
+
+	if _, exists := v.versions[operandName]; !exists {
+		return
 	}
+
+	delete(v.versions, operandName)
+	v.notifyChannelsLocked()
 }
 
 func (v *versionGetter) GetVersions() map[string]string {
@@ -65,6 +70,17 @@ func (v *versionGetter) VersionChangedChannel() <-chan struct{} {
 	return channel
 }
 
+// notifyChannelsLocked must be called under a locked v.lock
+func (v *versionGetter) notifyChannelsLocked() {
+	for i := range v.notificationChannels {
+		ch := v.notificationChannels[i]
+		// don't let a slow consumer block the rest
+		go func() {
+			ch <- struct{}{}
+		}()
+	}
+}
+
 func ImageForOperandFromEnv() string {
 	return os.Getenv(operandImageEnvVarName)
 }

pkg/operator/status/version_test.go

@@ -33,3 +33,45 @@ func TestVersionGetterBasic(t *testing.T) {
 	}
 
 }
+
+func TestVersionGetterUnset(t *testing.T) {
+	versionGetter := NewVersionGetter()
+
+	versionGetter.UnsetVersion("nonexistent")
+	expected := map[string]string{}
+	versions := versionGetter.GetVersions()
+	if !reflect.DeepEqual(expected, versions) {
+		t.Fatalf("Expected %v, got %v", expected, versions)
+	}
+
+	versionGetter.SetVersion("foo", "1.0.0")
+	versionGetter.SetVersion("bar", "2.0.0")
+
+	versions = versionGetter.GetVersions()
+	expected = map[string]string{"foo": "1.0.0", "bar": "2.0.0"}
+	if !reflect.DeepEqual(expected, versions) {
+		t.Fatalf("wanted: %v; got: %v", expected, versions)
+	}
+
+	ch := versionGetter.VersionChangedChannel()
+	versionGetter.UnsetVersion("foo")
+
+	select {
+	case <-ch:
+		// we got notified
+	case <-time.After(5 * time.Second):
+		t.Fatal("expected change notification after UnsetVersion but didn't get any")
+	}
+
+	versions = versionGetter.GetVersions()
+	expected = map[string]string{"bar": "2.0.0"}
+	if !reflect.DeepEqual(expected, versions) {
+		t.Fatalf("Expected %v, got %v", expected, versions)
+	}
+
+	versionGetter.UnsetVersion("nonexistent")
+	versions = versionGetter.GetVersions()
+	if !reflect.DeepEqual(expected, versions) {
+		t.Fatalf("Expected %v, got %v", expected, versions)
+	}
+}