basic version selection test suite

backend/pkg/app/backend.go

@@ -700,11 +700,11 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context,
 		unionKubeApplierInformers,
 	)
 	nodePoolVersionController := upgradecontrollers.NewNodePoolVersionController(
+		b.clock,
 		b.options.ResourcesDBClient,
 		subscriptionLister,
 		backendInformers,
 		unionKubeApplierInformers,
-		unionReadDesireLister,
 	)
 	nodePoolActiveVersionController := upgradecontrollers.NewNodePoolActiveVersionController(
 		b.options.ResourcesDBClient,

backend/pkg/controllers/upgradecontrollers/nodepool_version_controller.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/blang/semver/v4"
@@ -26,17 +27,16 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/api/operation"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilsclock "k8s.io/utils/clock"
 
 	cvocincinnati "github.com/openshift/cluster-version-operator/pkg/cincinnati"
 
 	"github.com/Azure/ARO-HCP/backend/pkg/controllers/controllerutils"
 	"github.com/Azure/ARO-HCP/backend/pkg/informers"
-	"github.com/Azure/ARO-HCP/backend/pkg/kubeapplierhelpers"
 	"github.com/Azure/ARO-HCP/backend/pkg/listers"
 	"github.com/Azure/ARO-HCP/internal/api"
 	"github.com/Azure/ARO-HCP/internal/cincinnati"
 	"github.com/Azure/ARO-HCP/internal/database"
-	dblisters "github.com/Azure/ARO-HCP/internal/database/listers"
 	unionkubeapplierinformers "github.com/Azure/ARO-HCP/internal/database/unioninformers/kubeapplier"
 	"github.com/Azure/ARO-HCP/internal/utils"
 	"github.com/Azure/ARO-HCP/internal/utils/apihelpers"
@@ -55,22 +55,21 @@ type nodePoolVersionSyncer struct {
 	serviceProviderNodePoolLister listers.ServiceProviderNodePoolLister
 	serviceProviderClusterLister  listers.ServiceProviderClusterLister
 	subscriptionLister            listers.SubscriptionLister
-	readDesireLister              dblisters.ReadDesireLister
 	resourcesDBClient             database.ResourcesDBClient
 
-	cincinnatiClientCache cincinnati.ClientCache
+	cincinnatiClient cincinnati.Client
 }
 
 var _ controllerutils.NodePoolSyncer = (*nodePoolVersionSyncer)(nil)
 
 // NewNodePoolVersionController creates a new syncer that validates and persists
 // the customer's desired NodePool version on the ServiceProviderNodePool.
 func NewNodePoolVersionController(
+	clock utilsclock.PassiveClock,
 	resourcesDBClient database.ResourcesDBClient,
 	subscriptionLister listers.SubscriptionLister,
 	informers informers.BackendInformers,
 	kubeApplierInformers *unionkubeapplierinformers.UnionKubeApplierInformers,
-	readDesireLister dblisters.ReadDesireLister,
 ) controllerutils.Controller {
 	_, nodePoolLister := informers.NodePools()
 	_, serviceProviderNodePoolLister := informers.ServiceProviderNodePools()
@@ -80,9 +79,11 @@ func NewNodePoolVersionController(
 		serviceProviderNodePoolLister: serviceProviderNodePoolLister,
 		serviceProviderClusterLister:  serviceProviderClusterLister,
 		subscriptionLister:            subscriptionLister,
-		readDesireLister:              readDesireLister,
 		resourcesDBClient:             resourcesDBClient,
-		cincinnatiClientCache:         cincinnati.NewClientCache(),
+		cincinnatiClient: cincinnati.NewCachingClient(
+			cvocincinnati.NewClient(uuid.Nil, http.DefaultTransport.(*http.Transport).Clone(), "ARO-HCP", cincinnati.NewAcceptAllConditionRegistry()),
+			clock, 1*time.Hour,
+		),
 	}
 
 	resyncDuration := 1 * time.Minute
@@ -204,22 +205,12 @@ func (c *nodePoolVersionSyncer) SyncOnce(ctx context.Context, key controllerutil
 		return utils.TrackError(fmt.Errorf("failed to get Subscription from cache: %w", err))
 	}
 
-	// Resolve the cluster UUID from the cached HostedCluster so we can build the Cincinnati client.
-	// Use it as best effort.  If we cannot find use, use an empty value to make progress without a specific value.
-	clusterUUID, found, err := kubeapplierhelpers.GetCachedHostedClusterUUIDForCluster(ctx, c.readDesireLister, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)
-	if err != nil {
-		logger.Info("error getting cluster UUID, continuing with empty", "err", err.Error())
-	}
-	if !found {
-		logger.Info("missing cluster UUID, continuing with empty")
-	}
-
 	op := operation.Operation{
 		Options: validation.AFECsToValidationOptions(subscription.GetRegisteredFeatures()),
 	}
 
 	// Validate the customer's desired version before setting it
-	err = c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, cachedServiceProviderNodePool, cachedServiceProviderCluster, cachedNodePool.Properties.Version.ChannelGroup, clusterUUID,
+	err = c.validateDesiredNodePoolVersion(ctx, &customerDesiredVersion, cachedServiceProviderNodePool, cachedServiceProviderCluster, cachedNodePool.Properties.Version.ChannelGroup,
 		op.HasOption(api.FeatureExperimentalReleaseFeatures))
 	if err != nil {
 		// Persist IntentFailed on the controller document for Cincinnati VersionNotFound or any non-Cincinnati resolution error.
@@ -291,7 +282,7 @@ func (c *nodePoolVersionSyncer) SyncOnce(ctx context.Context, key controllerutil
 //
 // Returns nil if the desired version is valid, or an error describing why it's invalid.
 func (c *nodePoolVersionSyncer) validateDesiredNodePoolVersion(ctx context.Context, desiredVersion *semver.Version, spNodePool *api.ServiceProviderNodePool, spCluster *api.ServiceProviderCluster,
-	channelGroup string, clusterUUID uuid.UUID, allowExperimentalReleaseFeatures bool) error {
+	channelGroup string, allowExperimentalReleaseFeatures bool) error {
 	if desiredVersion == nil {
 		return fmt.Errorf("customerDesiredVersion is nil, cannot evaluate upgrade")
 	}
@@ -308,7 +299,7 @@ func (c *nodePoolVersionSyncer) validateDesiredNodePoolVersion(ctx context.Conte
 	}
 
 	// Validate the desired version exists in Cincinnati (not that an edge exists from the current version).
-	if err := c.validateVersionExistsInCincinnati(ctx, desiredVersion, channelGroup, clusterUUID); err != nil {
+	if err := c.validateVersionExistsInCincinnati(ctx, desiredVersion, channelGroup); err != nil {
 		return err
 	}
 
@@ -321,18 +312,15 @@ func (c *nodePoolVersionSyncer) validateVersionExistsInCincinnati(
 	ctx context.Context,
 	version *semver.Version,
 	channelGroup string,
-	clusterUUID uuid.UUID,
 ) error {
 	cincinnatiURI, err := cincinnati.GetCincinnatiURI(channelGroup)
 	if err != nil {
 		return fmt.Errorf("failed to get Cincinnati URI: %w", err)
 	}
 
 	cincinnatiChannel := fmt.Sprintf("%s-%d.%d", channelGroup, version.Major, version.Minor)
-	cincinnatiClient := c.cincinnatiClientCache.GetOrCreateClient(clusterUUID)
 
-	// GetUpdates returns VersionNotFound if the version doesn't exist in the channel.
-	_, _, _, err = cincinnatiClient.GetUpdates(ctx, cincinnatiURI, "multi", "multi", cincinnatiChannel, *version)
+	_, _, _, err = c.cincinnatiClient.GetUpdates(ctx, cincinnatiURI, "multi", "multi", cincinnatiChannel, *version)
 	if err != nil {
 		if cincinnati.IsCincinnatiVersionNotFoundError(err) {
 			return utils.TrackError(fmt.Errorf("version %s not found in Cincinnati channel %s", version, cincinnatiChannel))

backend/pkg/controllers/upgradecontrollers/nodepool_version_controller_test.go

@@ -204,7 +204,6 @@ func TestNodePoolVersionSyncer_SyncOnce(t *testing.T) {
 	tests := []struct {
 		name                  string
 		seedDB                func(t *testing.T, ctx context.Context, mockResourcesDBClient *databasetesting.MockResourcesDBClient)
-		readDesireLister      func(t *testing.T) dblisters.ReadDesireLister
 		expectedError         bool
 		expectedErrorContains string
 	}{
@@ -254,24 +253,16 @@ func TestNodePoolVersionSyncer_SyncOnce(t *testing.T) {
 			ctrl := gomock.NewController(t)
 
 			mockResourcesDBClient := databasetesting.NewMockResourcesDBClient()
-			mockClientCache := cincinnati.NewMockClientCache(ctrl)
-			mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(cincinnati.NewMockClient(ctrl)).AnyTimes()
 
 			tt.seedDB(t, ctx, mockResourcesDBClient)
 
-			contentLister := newValidHostedClusterReadDesireLister(t)
-			if tt.readDesireLister != nil {
-				contentLister = tt.readDesireLister(t)
-			}
-
 			syncer := &nodePoolVersionSyncer{
 				nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 				serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 				serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 				subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-				readDesireLister:              contentLister,
 				resourcesDBClient:             mockResourcesDBClient,
-				cincinnatiClientCache:         mockClientCache,
+				cincinnatiClient:              cincinnati.NewMockClient(ctrl),
 			}
 
 			ctx = utils.ContextWithLogger(ctx, logr.Discard())
@@ -479,17 +470,13 @@ func TestNodePoolVersionSyncer_SyncOnce_IntentFailed(t *testing.T) {
 			mockCincinnati := cincinnati.NewMockClient(ctrl)
 			tt.setupCincinnati(mockCincinnati)
 
-			mockClientCache := cincinnati.NewMockClientCache(ctrl)
-			mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 			syncer := &nodePoolVersionSyncer{
 				nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 				serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 				serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 				subscriptionLister:            subscriptionLister,
-				readDesireLister:              newValidHostedClusterReadDesireLister(t),
 				resourcesDBClient:             mockResourcesDBClient,
-				cincinnatiClientCache:         mockClientCache,
+				cincinnatiClient:              mockCincinnati,
 			}
 
 			err := syncer.SyncOnce(ctx, testKey)
@@ -943,11 +930,8 @@ func TestNodePoolVersionSyncer_ValidateDesiredNodePoolVersion(t *testing.T) {
 				Return(configv1.Release{}, []configv1.Release{{Version: tt.desiredVersion}}, nil, nil).
 				AnyTimes()
 
-			mockClientCache := cincinnati.NewMockClientCache(ctrl)
-			mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnatiClient).AnyTimes()
-
 			syncer := &nodePoolVersionSyncer{
-				cincinnatiClientCache: mockClientCache,
+				cincinnatiClient: mockCincinnatiClient,
 			}
 
 			err := syncer.validateDesiredNodePoolVersion(
@@ -956,7 +940,6 @@ func TestNodePoolVersionSyncer_ValidateDesiredNodePoolVersion(t *testing.T) {
 				spNodePool,
 				spCluster,
 				"stable",
-				[16]byte{}, // dummy UUID
 				tt.allowMajorUpgrades,
 			)
 
@@ -975,8 +958,6 @@ func TestNodePoolVersionSyncer_SyncOnce_DesiredExceedsControlPlaneFails(t *testi
 	ctrl := gomock.NewController(t)
 
 	mockResourcesDBClient := databasetesting.NewMockResourcesDBClient()
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(cincinnati.NewMockClient(ctrl)).AnyTimes()
 
 	// Create node pool with desired version 4.19.15 (exceeds control plane 4.19.10)
 	createTestNodePoolWithVersion(t, ctx, mockResourcesDBClient, "4.19.15")
@@ -992,9 +973,8 @@ func TestNodePoolVersionSyncer_SyncOnce_DesiredExceedsControlPlaneFails(t *testi
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              cincinnati.NewMockClient(ctrl),
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1042,17 +1022,13 @@ func TestNodePoolVersionSyncer_SyncOnce_SucceedsWithoutCincinnatiEdge(t *testing
 		).
 		Times(1)
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1103,17 +1079,13 @@ func TestNodePoolVersionSyncer_SyncOnce_VersionNotInCincinnatiFails(t *testing.T
 		).
 		Times(1)
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1161,17 +1133,13 @@ func TestNodePoolVersionSyncer_SyncOnce_DowngradeVersionNotInCincinnatiFails(t *
 		).
 		Times(1)
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1219,17 +1187,13 @@ func TestNodePoolVersionSyncer_SyncOnce_DowngradeWithinSkewSucceeds(t *testing.T
 		).
 		Times(1)
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1280,17 +1244,13 @@ func TestNodePoolVersionSyncer_SyncOnce_ValidUpgradeSucceeds(t *testing.T) {
 		).
 		Times(1)
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).AnyTimes()
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1339,17 +1299,13 @@ func TestNodePoolVersionSyncer_SyncOnce_DesiredVersionUnchangedOnFailure_Changed
 		).
 		AnyTimes()
 
-	mockClientCache := cincinnati.NewMockClientCache(ctrl)
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnati).Times(1)
-
 	syncer := &nodePoolVersionSyncer{
 		nodePoolLister:                &listertesting.DBNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderNodePoolLister: &listertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockResourcesDBClient},
 		serviceProviderClusterLister:  &listertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockResourcesDBClient},
 		subscriptionLister:            &listertesting.DBSubscriptionLister{ResourcesDBClient: mockResourcesDBClient},
-		readDesireLister:              newValidHostedClusterReadDesireLister(t),
 		resourcesDBClient:             mockResourcesDBClient,
-		cincinnatiClientCache:         mockClientCache,
+		cincinnatiClient:              mockCincinnati,
 	}
 
 	testKey := controllerutils.HCPNodePoolKey{
@@ -1400,7 +1356,7 @@ func TestNodePoolVersionSyncer_SyncOnce_DesiredVersionUnchangedOnFailure_Changed
 			&cvocincinnati.Error{Reason: "VersionNotFound", Message: "version 4.19.99 not found"},
 		).
 		AnyTimes()
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnatiFailing).Times(1)
+	syncer.cincinnatiClient = mockCincinnatiFailing
 
 	// SyncOnce should succeed but persist IntentFailed (VersionNotFound is a user error, not transient)
 	err = syncer.SyncOnce(ctx, testKey)
@@ -1440,7 +1396,7 @@ func TestNodePoolVersionSyncer_SyncOnce_DesiredVersionUnchangedOnFailure_Changed
 			nil,
 		).
 		AnyTimes()
-	mockClientCache.EXPECT().GetOrCreateClient(gomock.Any()).Return(mockCincinnatiSucceeding).Times(1)
+	syncer.cincinnatiClient = mockCincinnatiSucceeding
 
 	// SyncOnce should succeed now
 	err = syncer.SyncOnce(ctx, testKey)