Fixes for RDS modify behavior

services/rds/broker.go

@@ -274,8 +274,7 @@ func (broker *rdsBroker) ModifyInstance(c *catalog.Catalog, id string, modifyReq
 		return response.NewErrorResponse(http.StatusInternalServerError, fmt.Sprintf("Error modifying the instance: %s", err))
 	case base.InstanceInProgress:
 		// Update the existing instance in the broker.
-		existingInstance.State = status
-		err = broker.brokerDB.Save(existingInstance).Error
+		err = broker.brokerDB.Model(RDSInstance{}).Where("uuid", existingInstance.Uuid).Update("state", status).Error
 		if err != nil {
 			return response.NewErrorResponse(http.StatusInternalServerError, err.Error())
 		}

services/rds/broker_test.go

@@ -311,6 +311,7 @@ func TestModify(t *testing.T) {
 		catalog              *catalog.Catalog
 		modifyRequest        request.Request
 		expectedDbInstance   *RDSInstance
+		dbAdapter            dbAdapter
 	}{
 		"success": {
 			catalog: &catalog.Catalog{
@@ -422,7 +423,9 @@ func TestModify(t *testing.T) {
 				brokerDB:   brokerDB,
 				settings:   test.settings,
 				tagManager: test.tagManager,
-				dbAdapter:  &mockDBAdapter{},
+				dbAdapter: &mockDBAdapter{
+					db: brokerDB,
+				},
 			}
 
 			err = brokerDB.Create(test.dbInstance).Error

services/rds/rds.go

@@ -2,6 +2,7 @@ package rds
 
 import (
 	"context"
+	"math"
 	"time"
 
 	"code.cloudfoundry.org/lager"
@@ -36,7 +37,9 @@ type dbAdapter interface {
 func initializeAdapter(s *config.Settings, db *gorm.DB, logger lager.Logger) (dbAdapter, error) {
 	// For test environments, use a mock broker.dbAdapter.
 	if s.Environment == "test" {
-		return &mockDBAdapter{}, nil
+		return &mockDBAdapter{
+			db: db,
+		}, nil
 	}
 
 	cfg, err := awsConfig.LoadDefaultConfig(
@@ -69,6 +72,7 @@ func NewRdsDedicatedDBAdapter(s *config.Settings, db *gorm.DB, rdsClient RDSClie
 // It is only here because *_test.go files are only compiled during "go test"
 // and it's referenced in non *_test.go code eg. InitializeAdapter in main.go.
 type mockDBAdapter struct {
+	db            *gorm.DB
 	createDBState *base.InstanceState
 }
 
@@ -81,7 +85,8 @@ func (d *mockDBAdapter) createDB(i *RDSInstance, plan catalog.RDSPlan, password
 }
 
 func (d *mockDBAdapter) modifyDB(i *RDSInstance, plan catalog.RDSPlan) (base.InstanceState, error) {
-	return base.InstanceInProgress, nil
+	err := d.db.Save(i).Error
+	return base.InstanceInProgress, err
 }
 
 func (d *mockDBAdapter) checkDBStatus(database string) (base.InstanceState, error) {
@@ -244,7 +249,9 @@ func (d *dedicatedDBAdapter) waitForDbReady(operation base.Operation, i *RDSInst
 	var dbState base.InstanceState
 	var err error
 
-	for attempt <= int(d.settings.PollAwsMaxRetries) {
+	maxRetries := getMaxCheckDBStatusRetries(i.AllocatedStorage, d.settings.PollAwsMaxRetries)
+
+	for attempt <= int(maxRetries) {
 		dbState, err = d.checkDBStatus(database)
 		if err != nil {
 			updateErr := jobs.WriteAsyncJobMessage(d.db, i.ServiceID, i.Uuid, operation, base.InstanceNotCreated, fmt.Sprintf("Failed to get database status: %s", err))
@@ -258,7 +265,7 @@ func (d *dedicatedDBAdapter) waitForDbReady(operation base.Operation, i *RDSInst
 			return nil
 		}
 
-		err := jobs.WriteAsyncJobMessage(d.db, i.ServiceID, i.Uuid, operation, base.InstanceInProgress, fmt.Sprintf("Waiting for database to be available. Current status: %s (attempt %d of %d)", dbState, attempt, d.settings.PollAwsMaxRetries))
+		err := jobs.WriteAsyncJobMessage(d.db, i.ServiceID, i.Uuid, operation, base.InstanceInProgress, fmt.Sprintf("Waiting for database to be available. Current status: %s (attempt %d of %d)", dbState, attempt, maxRetries))
 		if err != nil {
 			return fmt.Errorf("waitForDbReady: %w", err)
 		}
@@ -675,3 +682,11 @@ func isDatabaseInstanceNotFoundError(err error) bool {
 	var notFoundException *rdsTypes.DBInstanceNotFoundFault
 	return errors.As(err, &notFoundException)
 }
+
+func getMaxCheckDBStatusRetries(storageSize int64, defaultMaxRetries int64) int64 {
+	// Scale the number of retries in proportion to the database
+	// storage size
+	retryMultiplier := math.Ceil(float64(storageSize) / 200)
+	maxRetries := defaultMaxRetries * int64(retryMultiplier)
+	return max(defaultMaxRetries, maxRetries)
+}

services/rds/rds_test.go

@@ -2211,3 +2211,41 @@ func TestDeleteDb(t *testing.T) {
 		})
 	}
 }
+
+func TestGetMaxCheckDBStatusRetries(t *testing.T) {
+	testCases := map[string]struct {
+		storageSize        int64
+		defaultMaxRetries  int64
+		expectedMaxRetries int64
+	}{
+		"storage = 0": {
+			storageSize:        0,
+			defaultMaxRetries:  1,
+			expectedMaxRetries: 1,
+		},
+		"storage = 1": {
+			storageSize:        1,
+			defaultMaxRetries:  1,
+			expectedMaxRetries: 1,
+		},
+		"storage = 201": {
+			storageSize:        201,
+			defaultMaxRetries:  1,
+			expectedMaxRetries: 2,
+		},
+		"storage = 1000": {
+			storageSize:        1000,
+			defaultMaxRetries:  1,
+			expectedMaxRetries: 5,
+		},
+	}
+
+	for name, test := range testCases {
+		t.Run(name, func(t *testing.T) {
+			retries := getMaxCheckDBStatusRetries(test.storageSize, test.defaultMaxRetries)
+			if retries != test.expectedMaxRetries {
+				t.Fatalf("expected %d, got %d", test.expectedMaxRetries, retries)
+			}
+		})
+	}
+}

services/rds/rdsinstance.go

@@ -77,7 +77,7 @@ func (i *RDSInstance) generateCredentials(settings *config.Settings) error {
 	return nil
 }
 
-func (i *RDSInstance) modify(options Options, currentPlan catalog.RDSPlan, newPlan catalog.RDSPlan, settings *config.Settings, tags map[string]string) (*RDSInstance, error) {
+func (i RDSInstance) modify(options Options, currentPlan catalog.RDSPlan, newPlan catalog.RDSPlan, settings *config.Settings, tags map[string]string) (*RDSInstance, error) {
 	// Copy the existing instance so that we can return a modified instance rather than mutating the instance
 	modifiedInstance := i
 	modifiedInstance.PlanID = newPlan.ID
@@ -156,9 +156,9 @@ func (i *RDSInstance) modify(options Options, currentPlan catalog.RDSPlan, newPl
 		modifiedInstance.DeleteReadReplica = true
 	}
 
-	i.setTags(newPlan, tags)
+	modifiedInstance.setTags(newPlan, tags)
 
-	return modifiedInstance, nil
+	return &modifiedInstance, nil
 }
 
 func (i *RDSInstance) generateDatabaseReplicaName() string {

services/rds/rdsinstance_test.go

@@ -737,8 +737,11 @@ func TestModifyInstance(t *testing.T) {
 			if test.expectErr && err == nil {
 				t.Errorf("expected error, got nil")
 			}
+			if diff := deep.Equal(test.existingInstance, test.expectedInstance); diff == nil {
+				t.Fatal("Expected no modifications to existing instance")
+			}
 			if diff := deep.Equal(modifiedInstance, test.expectedInstance); diff != nil {
-				t.Error(diff)
+				t.Fatal(diff)
 			}
 		})
 	}