OCPEDGE-2088, OCPEDGE-1885: Updated state transitions & tests for TNF setup job

- Setup job now sets Available condition only when ExternalEtcd isn't enabled yet (includes test)
- Fixed jobcontroller & test setup to handle variable operator status conditions
- Added test coverage for updated expectations for job controller condition transitions (Progressing, Degraded, Available)

Author: jaypoulz

pkg/tnf/operator/starter.go

@@ -4,10 +4,11 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
 	"os"
 	"sync"
 
+	operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
+
 	operatorv1 "github.com/openshift/api/operator/v1"
 	configv1informers "github.com/openshift/client-go/config/informers/externalversions"
 	v1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
@@ -61,14 +62,30 @@ func HandleDualReplicaClusters(ctx context.Context,
 	kubeClient kubernetes.Interface,
 	dynamicClient dynamic.Interface) (bool, error) {
 
-	if isDualReplicaTopology, err := isDualReplicaTopoly(ctx, featureGateAccessor, configInformers); err != nil {
+	// Start the informers and wait for them to sync.
+	// The config informer is used to check the control plane topology.
+	// The operator client informer is used to check if CEO is managing etcd, since
+	// we need to set the available status to false during the transition to external etcd.
+	configInformers.Start(ctx.Done())
+	operatorClient.Informer().Run(ctx.Done())
+	if !cache.WaitForCacheSync(ctx.Done(), configInformers.Config().V1().APIServers().Informer().HasSynced, operatorClient.Informer().HasSynced) {
+		klog.Fatal("Failed to sync caches for static pod operator client")
+	}
+
+	if isDualReplicaTopology, err := isDualReplicaTopology(ctx, featureGateAccessor, configInformers); err != nil {
 		return false, err
 	} else if !isDualReplicaTopology {
 		return false, nil
 	}
 
 	klog.Infof("detected DualReplica topology")
 
+	// We only set the CEO available status to false during the initial transition to external etcd.
+	initialTransition, err := isCEOManagingEtcd(operatorClient)
+	if err != nil {
+		return false, fmt.Errorf("could not determine if we are in the initial setup: %w", err)
+	}
+
 	runExternalEtcdSupportController(ctx, controllerContext, operatorClient, envVarGetter, kubeInformersForNamespaces,
 		configInformers, networkInformer, controlPlaneNodeInformer, etcdInformer, kubeClient)
 	runTnfResourceController(ctx, controllerContext, kubeClient, dynamicClient, operatorClient, kubeInformersForNamespaces)
@@ -78,7 +95,7 @@ func HandleDualReplicaClusters(ctx context.Context,
 	// we need node names for assigning auth and after-setup jobs to specific nodes
 	var once sync.Once
 	klog.Infof("watching for nodes...")
-	_, err := controlPlaneNodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+	_, err = controlPlaneNodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			node, ok := obj.(*corev1.Node)
 			if !ok {
@@ -102,11 +119,22 @@ func HandleDualReplicaClusters(ctx context.Context,
 				klog.Infof("found 2 control plane nodes (%q, %q), creating TNF jobs", nodeList[0].GetName(), nodeList[1].GetName())
 				// the order of job creation does not matter, the jobs wait on each other as needed
 				for _, node := range nodeList {
-					runJobController(ctx, tools.JobTypeAuth, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces)
-					runJobController(ctx, tools.JobTypeAfterSetup, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces)
+					runJobController(ctx, tools.JobTypeAuth, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions)
+					runJobController(ctx, tools.JobTypeAfterSetup, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions)
 				}
-				runJobController(ctx, tools.JobTypeSetup, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces)
-				runJobController(ctx, tools.JobTypeFencing, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces)
+
+				// Make a copy of the default conditions and add the available condition
+				conditionsWithAvailable := make([]string, len(jobs.DefaultConditions))
+				copy(conditionsWithAvailable, jobs.DefaultConditions)
+				conditionsWithAvailable = append(conditionsWithAvailable, operatorv1.OperatorStatusTypeAvailable)
+
+				setupConditions := jobs.DefaultConditions
+				if initialTransition {
+					setupConditions = conditionsWithAvailable
+				}
+
+				runJobController(ctx, tools.JobTypeSetup, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, setupConditions)
+				runJobController(ctx, tools.JobTypeFencing, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions)
 			})
 		},
 	})
@@ -138,7 +166,21 @@ func HandleDualReplicaClusters(ctx context.Context,
 	return true, nil
 }
 
-func isDualReplicaTopoly(ctx context.Context, featureGateAccessor featuregates.FeatureGateAccess, configInformers configv1informers.SharedInformerFactory) (bool, error) {
+func isCEOManagingEtcd(operatorClient v1helpers.StaticPodOperatorClient) (bool, error) {
+	// Detect if the cluster is already running in ExternalEtcd mode
+	operatorSpec, _, _, err := operatorClient.GetStaticPodOperatorState()
+	if err != nil {
+		return false, fmt.Errorf("could not get operator spec: %w", err)
+	}
+	externalEtcdMode, err := ceohelpers.IsExternalEtcdSupport(operatorSpec)
+	if err != nil {
+		return false, fmt.Errorf("could not determine if useExternalEtcdSupport config override is set: %w", err)
+	}
+
+	return !externalEtcdMode, nil
+}
+
+func isDualReplicaTopology(ctx context.Context, featureGateAccessor featuregates.FeatureGateAccess, configInformers configv1informers.SharedInformerFactory) (bool, error) {
 	if isDualReplicaTopology, err := ceohelpers.IsDualReplicaTopology(ctx, configInformers.Config().V1().Infrastructures().Lister()); err != nil {
 		return false, fmt.Errorf("could not determine DualReplicaTopology, aborting controller start: %w", err)
 	} else if !isDualReplicaTopology {
@@ -201,7 +243,7 @@ func runTnfResourceController(ctx context.Context, controllerContext *controller
 	go tnfResourceController.Run(ctx, 1)
 }
 
-func runJobController(ctx context.Context, jobType tools.JobType, nodeName *string, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces) {
+func runJobController(ctx context.Context, jobType tools.JobType, nodeName *string, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, conditions []string) {
 	nodeNameForLogs := "n/a"
 	if nodeName != nil {
 		nodeNameForLogs = *nodeName
@@ -214,6 +256,7 @@ func runJobController(ctx context.Context, jobType tools.JobType, nodeName *stri
 		operatorClient,
 		kubeClient,
 		kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Batch().V1().Jobs(),
+		conditions,
 		[]factory.Informer{},
 		[]jobs.JobHookFunc{
 			func(_ *operatorv1.OperatorSpec, job *batchv1.Job) error {

pkg/tnf/operator/starter_test.go

@@ -2,11 +2,12 @@ package operator
 
 import (
 	"context"
-	"github.com/stretchr/testify/require"
 	"maps"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 
@@ -36,8 +37,49 @@ import (
 	"github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers"
 	"github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient"
 	"github.com/openshift/cluster-etcd-operator/pkg/tnf/operator/dualreplicahelpers"
+	"github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs"
 )
 
+// mockSharedIndexInformer implements cache.SharedIndexInformer with proper Run and HasSynced methods
+type mockSharedIndexInformer struct {
+	cache.SharedIndexInformer
+}
+
+func (m *mockSharedIndexInformer) Run(stopCh <-chan struct{}) {
+	// Mock implementation that just waits for the stop channel
+	<-stopCh
+}
+
+func (m *mockSharedIndexInformer) HasSynced() bool {
+	return true
+}
+
+// createMockOperatorClient creates a StaticPodOperatorClient with a mock informer
+func createMockOperatorClient(spec *operatorv1.StaticPodOperatorSpec, status *operatorv1.StaticPodOperatorStatus) v1helpers.StaticPodOperatorClient {
+	client := v1helpers.NewFakeStaticPodOperatorClient(spec, status, nil, nil)
+
+	// Create a mock informer
+	mockInformer := &mockSharedIndexInformer{
+		SharedIndexInformer: client.Informer(),
+	}
+
+	// Return a wrapper that uses our mock informer
+	return &mockOperatorClient{
+		StaticPodOperatorClient: client,
+		mockInformer:            mockInformer,
+	}
+}
+
+// mockOperatorClient wraps the fake client with our mock informer
+type mockOperatorClient struct {
+	v1helpers.StaticPodOperatorClient
+	mockInformer *mockSharedIndexInformer
+}
+
+func (m *mockOperatorClient) Informer() cache.SharedIndexInformer {
+	return m.mockInformer
+}
+
 type args struct {
 	ctx                        context.Context
 	controllerContext          *controllercmd.ControllerContext
@@ -112,6 +154,75 @@ func TestHandleDualReplicaClusters(t *testing.T) {
 	}
 }
 
+func TestSetupJobConditionsBasedOnExternalEtcd(t *testing.T) {
+	tests := []struct {
+		name                     string
+		externalEtcdEnabled      bool
+		expectedAvailableInSetup bool
+	}{
+		{
+			name:                     "Job sets the Available condition during initial setup",
+			externalEtcdEnabled:      false,
+			expectedAvailableInSetup: true,
+		},
+		{
+			name:                     "Job does not set the Available condition once ExternalEtcd is enabled",
+			externalEtcdEnabled:      true,
+			expectedAvailableInSetup: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create operator spec with or without external etcd support
+			operatorSpec := &operatorv1.StaticPodOperatorSpec{}
+			if tt.externalEtcdEnabled {
+				operatorSpec.UnsupportedConfigOverrides = runtime.RawExtension{
+					Raw: []byte(`{"useExternalEtcdSupport": true}`),
+				}
+			}
+
+			// Create the operator client with mock informer
+			fakeOperatorClient := createMockOperatorClient(
+				operatorSpec,
+				&operatorv1.StaticPodOperatorStatus{},
+			)
+
+			// Test initialTransition function
+			initialTransition, err := isCEOManagingEtcd(fakeOperatorClient)
+			require.NoError(t, err)
+
+			// Verify the expected behavior
+			expectedInitialTransition := !tt.externalEtcdEnabled
+			require.Equalf(t, expectedInitialTransition, initialTransition,
+				"initialTransition should return %v when externalEtcdEnabled is %v",
+				expectedInitialTransition, tt.externalEtcdEnabled)
+
+			// Verify setup conditions
+			conditionsWithAvailable := make([]string, len(jobs.DefaultConditions))
+			copy(conditionsWithAvailable, jobs.DefaultConditions)
+			conditionsWithAvailable = append(conditionsWithAvailable, operatorv1.OperatorStatusTypeAvailable)
+
+			setupConditions := jobs.DefaultConditions
+			if initialTransition {
+				setupConditions = conditionsWithAvailable
+			}
+
+			hasAvailableCondition := false
+			for _, condition := range setupConditions {
+				if condition == operatorv1.OperatorStatusTypeAvailable {
+					hasAvailableCondition = true
+					break
+				}
+			}
+
+			require.Equalf(t, tt.expectedAvailableInSetup, hasAvailableCondition,
+				"Setup job should have Available condition: %v, but got: %v",
+				tt.expectedAvailableInSetup, hasAvailableCondition)
+		})
+	}
+}
+
 func getArgs(t *testing.T, dualReplicaControlPlaneEnabled, dualReplicaFeatureGateEnabled bool) args {
 
 	fakeKubeClient := fake.NewSimpleClientset()
@@ -131,11 +242,10 @@ func getArgs(t *testing.T, dualReplicaControlPlaneEnabled, dualReplicaFeatureGat
 	}
 	fakeConfigClient := fakeconfig.NewSimpleClientset([]runtime.Object{infra}...)
 
-	fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient(
+	// Create the operator client with mock informer
+	fakeOperatorClient := createMockOperatorClient(
 		&operatorv1.StaticPodOperatorSpec{},
 		&operatorv1.StaticPodOperatorStatus{},
-		nil,
-		nil,
 	)
 
 	enabledFeatureGates := make([]configv1.FeatureGateName, 0)

pkg/tnf/pkg/jobs/jobcontroller.go

@@ -26,6 +26,11 @@ import (
 	"github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools"
 )
 
+// By default, we only set the progressing and degraded conditions for TNF setup.
+// This is because the operator is only unavailable during the transition of etcd
+// ownership between CEO and pacemaker.
+var DefaultConditions = []string{opv1.OperatorStatusTypeProgressing, opv1.OperatorStatusTypeDegraded}
+
 // TODO This based on DeploymentController in openshift/library-go
 // TODO should be moved there once it proved to be useful
 
@@ -71,6 +76,7 @@ func NewJobController(
 	operatorClient v1helpers.OperatorClient,
 	kubeClient kubernetes.Interface,
 	jobInformer batchinformersv1.JobInformer,
+	conditions []string,
 	optionalInformers []factory.Informer,
 	optionalJobHooks ...JobHookFunc,
 ) factory.Controller {
@@ -82,9 +88,7 @@ func NewJobController(
 		kubeClient,
 		jobInformer,
 	).WithConditions(
-		opv1.OperatorStatusTypeAvailable,
-		opv1.OperatorStatusTypeProgressing,
-		opv1.OperatorStatusTypeDegraded,
+		conditions...,
 	).WithExtraInformers(
 		optionalInformers...,
 	).WithJobHooks(