[3.1.10 backport] CBG-4107 CBG-4173 : remove database even if removeCorruptConfigIfExists fails (#7060)

* CBG-4088: If we error in removeCorruptConfigIfExists we don't unload/remove database (#7014)

* CBG-4087: remove configs from invalid tracking that are not found in config poll (#7020)

* CBG-4087: remove configs from invalid tracking that are not found in config poll

* update test

---------

Co-authored-by: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com>

Author: torcolvin

rest/admin_api.go

@@ -135,6 +135,9 @@ func (h *handler) handleCreateDB() error {
 		// if it used to be corrupt we need to remove it from the invalid database map on server context and remove the old corrupt config from the bucket
 		err = h.removeCorruptConfigIfExists(contextNoCancel.Ctx, bucket, h.server.Config.Bootstrap.ConfigGroupID, dbName)
 		if err != nil {
+			// we cannot continue on with database creation with possibility of the corrupt database config in the bucket for this db
+			// thus we need to unload the requested database config to prevent the cluster being in an inconsistent state
+			h.server._removeDatabase(contextNoCancel.Ctx, dbName)
 			return err
 		}
 		cas, err := h.server.BootstrapContext.InsertConfig(contextNoCancel.Ctx, bucket, h.server.Config.Bootstrap.ConfigGroupID, &persistedConfig)

rest/config.go

@@ -366,6 +366,18 @@ func (d *invalidDatabaseConfigs) remove(dbname string) {
 	delete(d.dbNames, dbname)
 }
 
+// removeNonExistingConfigs will remove any configs from invalid config tracking map that aren't present in fetched configs
+func (d *invalidDatabaseConfigs) removeNonExistingConfigs(fetchedConfigs map[string]bool) {
+	d.m.Lock()
+	defer d.m.Unlock()
+	for dbName := range d.dbNames {
+		if ok := fetchedConfigs[dbName]; !ok {
+			// this invalid db config was not found in config polling, so lets remove
+			delete(d.dbNames, dbName)
+		}
+	}
+}
+
 // inheritFromBootstrap sets any empty Couchbase Server values from the given bootstrap config.
 func (dbc *DbConfig) inheritFromBootstrap(b BootstrapConfig) {
 	if dbc.Username == "" {
@@ -1762,6 +1774,7 @@ func (sc *ServerContext) FetchConfigs(ctx context.Context, isInitialStartup bool
 		return nil, err
 	}
 
+	allConfigsFound := make(map[string]bool)
 	fetchedConfigs := make(map[string]DatabaseConfig, len(buckets))
 	for _, bucket := range buckets {
 		ctx := base.BucketNameCtx(ctx, bucket)
@@ -1782,6 +1795,7 @@ func (sc *ServerContext) FetchConfigs(ctx context.Context, isInitialStartup bool
 			continue
 		}
 		for _, cnf := range configs {
+			allConfigsFound[cnf.Name] = true
 			// Handle invalid database registry entries. Either:
 			// - CBG-3292: Bucket in config doesn't match the actual bucket
 			// - CBG-3742: Registry entry marked invalid (due to rollback causing collection conflict)
@@ -1815,6 +1829,10 @@ func (sc *ServerContext) FetchConfigs(ctx context.Context, isInitialStartup bool
 		}
 	}
 
+	// remove any invalid databases from the tracking map if config poll above didn't
+	// pick up that configs from the bucket. This means the config is no longer present in the bucket.
+	sc.invalidDatabaseConfigTracking.removeNonExistingConfigs(allConfigsFound)
+
 	return fetchedConfigs, nil
 }
 

rest/config_test.go

@@ -30,6 +30,7 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+	"time"
 
 	"golang.org/x/crypto/bcrypt"
 	"gopkg.in/square/go-jose.v2"
@@ -2925,3 +2926,146 @@ func makeScopesConfigWithDefault(scopeName string, collections []string) *Scopes
 	scopesConfig := makeScopesConfig(scopeName, collections)
 	return &scopesConfig
 }
+
+// TestInvalidDbConfigNoLongerPresentInBucket:
+//   - Create rest tester with large config poll interval
+//   - Create valid db
+//   - Alter config in bucket to make it invalid
+//   - Force config poll, assert it is picked up as invalid db config
+//   - Delete the invalid db config form the bucket
+//   - Force config poll reload and assert the invalid db is cleared
+func TestInvalidDbConfigNoLongerPresentInBucket(t *testing.T) {
+	if base.UnitTestUrlIsWalrus() {
+		t.Skip("test only works with CBS, requires bootstrap connection")
+	}
+	rt := NewRestTester(t, &RestTesterConfig{
+		CustomTestBucket: base.GetTestBucket(t),
+		PersistentConfig: true,
+		MutateStartupConfig: func(config *StartupConfig) {
+			// configure the interval time to not run
+			config.Bootstrap.ConfigUpdateFrequency = base.NewConfigDuration(10 * time.Minute)
+		},
+		DatabaseConfig: nil,
+	})
+	defer rt.Close()
+	realBucketName := rt.CustomTestBucket.GetName()
+	ctx := base.TestCtx(t)
+	const dbName = "db1"
+
+	// create db with correct config
+	dbConfig := rt.NewDbConfig()
+	resp := rt.CreateDatabase(dbName, dbConfig)
+	RequireStatus(t, resp, http.StatusCreated)
+
+	// wait for db to come online
+	require.NoError(t, rt.WaitForDBOnline())
+
+	// grab the persisted db config from the bucket
+	databaseConfig := DatabaseConfig{}
+	_, err := rt.ServerContext().BootstrapContext.GetConfig(rt.Context(), realBucketName, rt.ServerContext().Config.Bootstrap.ConfigGroupID, "db1", &databaseConfig)
+	require.NoError(t, err)
+
+	// update the persisted config to a fake bucket name
+	newBucketName := "fakeBucket"
+	_, err = rt.UpdatePersistedBucketName(&databaseConfig, &newBucketName)
+	require.NoError(t, err)
+
+	// force reload of configs from bucket
+	rt.ServerContext().ForceDbConfigsReload(t, ctx)
+
+	// assert the config is picked as invalid db config
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		invalidDatabases := rt.ServerContext().AllInvalidDatabaseNames(t)
+		assert.Equal(c, 1, len(invalidDatabases))
+		assert.Equal(c, 0, len(rt.ServerContext().dbConfigs))
+	}, time.Second*10, time.Millisecond*100)
+
+	// remove the invalid config from the bucket
+	rt.RemoveDbConfigFromBucket(dbName, realBucketName)
+
+	// force reload of configs from bucket
+	rt.ServerContext().ForceDbConfigsReload(t, ctx)
+
+	// assert the config is removed from tracking
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		invalidDatabases := rt.ServerContext().AllInvalidDatabaseNames(t)
+		assert.Equal(c, 0, len(invalidDatabases))
+		assert.Equal(c, 0, len(rt.ServerContext().dbConfigs))
+	}, time.Second*10, time.Millisecond*100)
+
+	// create db again, should succeed
+	resp = rt.CreateDatabase(dbName, dbConfig)
+	RequireStatus(t, resp, http.StatusCreated)
+}
+
+// TestNotFoundOnInvalidDatabase:
+//   - Create rest tester with large config polling interval
+//   - Insert a bad dbConfig into the bucket
+//   - Manually fetch and load db from buckets
+//   - Assert that the bad config is tracked as invalid config
+//   - Delete the bad config manually and attempt to correct the db config through create db endpoint
+//   - Assert db is removed form invalid db's and is now a running database on server context
+func TestNotFoundOnInvalidDatabase(t *testing.T) {
+	if base.UnitTestUrlIsWalrus() {
+		t.Skip("test only works with CBS, requires bootstrap connection")
+	}
+	rt := NewRestTester(t, &RestTesterConfig{
+		CustomTestBucket: base.GetTestBucket(t),
+		PersistentConfig: true,
+		MutateStartupConfig: func(config *StartupConfig) {
+			// configure the interval time to not run
+			config.Bootstrap.ConfigUpdateFrequency = base.NewConfigDuration(100 * time.Second)
+		},
+		DatabaseConfig: nil,
+	})
+	defer rt.Close()
+	realBucketName := rt.CustomTestBucket.GetName()
+
+	// create a new invalid db config and persist to bucket
+	badName := "badBucketName"
+	dbConfig := rt.NewDbConfig()
+	dbConfig.Name = "db1"
+
+	version, err := GenerateDatabaseConfigVersionID(rt.Context(), "", &dbConfig)
+	require.NoError(t, err)
+	metadataID, metadataIDError := rt.ServerContext().BootstrapContext.ComputeMetadataIDForDbConfig(base.TestCtx(t), &dbConfig)
+	require.NoError(t, metadataIDError)
+
+	// insert the db config with bad bucket name
+	dbConfig.Bucket = &badName
+	persistedConfig := DatabaseConfig{
+		Version:    version,
+		MetadataID: metadataID,
+		DbConfig:   dbConfig,
+		SGVersion:  base.ProductVersion.String(),
+	}
+	rt.InsertDbConfigToBucket(&persistedConfig, rt.CustomTestBucket.GetName())
+
+	// manually fetch and load db configs from bucket
+	_, err = rt.ServerContext().fetchAndLoadConfigs(rt.Context(), false)
+	require.NoError(t, err)
+
+	// assert the config is picked as invalid db config
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		invalidDatabases := rt.ServerContext().AllInvalidDatabaseNames(t)
+		assert.Equal(c, 1, len(invalidDatabases))
+	}, time.Second*10, time.Millisecond*100)
+
+	resp := rt.SendAdminRequest(http.MethodGet, "/db1/", "")
+	RequireStatus(t, resp, http.StatusNotFound)
+	assert.Contains(t, resp.Body.String(), "You must update database config immediately")
+
+	// delete the invalid db config to force the not found error
+	rt.RemoveDbConfigFromBucket(dbConfig.Name, realBucketName)
+
+	// fix the bucket name and try fix corrupt db through create db endpoint
+	dbConfig.Bucket = &realBucketName
+	RequireStatus(t, rt.CreateDatabase(dbConfig.Name, dbConfig), http.StatusCreated)
+
+	// assert the config is remove the invalid config and we have a running db
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		invalidDatabases := rt.ServerContext().AllInvalidDatabaseNames(t)
+		assert.Equal(c, 0, len(invalidDatabases))
+		assert.Equal(c, 1, len(rt.ServerContext().dbConfigs))
+	}, time.Second*10, time.Millisecond*100)
+}

rest/handler.go

@@ -572,7 +572,7 @@ func (h *handler) removeCorruptConfigIfExists(ctx context.Context, bucket, confi
 	}
 	// remove the bad config from the bucket
 	err := h.server.BootstrapContext.DeleteConfig(ctx, bucket, configGroupID, dbName)
-	if err != nil {
+	if err != nil && !base.IsDocNotFoundError(err) {
 		return err
 	}
 	// delete the database name form the invalid database map on server context

rest/utilities_testing.go

@@ -2629,6 +2629,23 @@ func (sc *ServerContext) RequireInvalidDatabaseConfigNames(t *testing.T, expecte
 	require.ElementsMatch(t, expectedDbNames, dbNames)
 }
 
+// ForceDbConfigsReload forces the reload db config from bucket process (like the ConfigUpdate background process)
+func (sc *ServerContext) ForceDbConfigsReload(t *testing.T, ctx context.Context) {
+	_, err := sc.fetchAndLoadConfigs(ctx, false)
+	require.NoError(t, err)
+}
+
+// AllInvalidDatabaseNames returns the names of all the databases that have invalid configs. Testing only since this locks the database context.
+func (sc *ServerContext) AllInvalidDatabaseNames(_ *testing.T) []string {
+	sc.invalidDatabaseConfigTracking.m.RLock()
+	defer sc.invalidDatabaseConfigTracking.m.RUnlock()
+	dbs := make([]string, 0, len(sc.invalidDatabaseConfigTracking.dbNames))
+	for db := range sc.invalidDatabaseConfigTracking.dbNames {
+		dbs = append(dbs, db)
+	}
+	return dbs
+}
+
 // Calls DropAllIndexes to remove all indexes, then restores the primary index for TestBucketPool readier requirements
 func dropAllNonPrimaryIndexes(t *testing.T, dataStore base.DataStore) {
 

rest/utilities_testing_resttester.go

@@ -290,6 +290,11 @@ func (rt *RestTester) InsertDbConfigToBucket(config *DatabaseConfig, bucketName
 	require.NoError(rt.TB, insertErr)
 }
 
+func (rt *RestTester) RemoveDbConfigFromBucket(dbName string, bucketName string) {
+	deleteErr := rt.ServerContext().BootstrapContext.DeleteConfig(base.TestCtx(rt.TB), bucketName, rt.ServerContext().Config.Bootstrap.ConfigGroupID, dbName)
+	require.NoError(rt.TB, deleteErr)
+}
+
 func (rt *RestTester) PersistDbConfigToBucket(dbConfig DbConfig, bucketName string) {
 	version, err := GenerateDatabaseConfigVersionID(rt.Context(), "", &dbConfig)
 	require.NoError(rt.TB, err)