From e39b485c4b769ec3c89d2490eee67fcc07b7ae88 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 4 Aug 2023 23:05:01 -0300 Subject: [PATCH] Fail vs. fatal on store startup (#24170) If the store fails to initialize (e.g. run a migration), it would `log.Fatal` and then `os.Exit`. Unfortunately, this trips up `TestMain`, which happily keeps running tests, now guaranteed to fail. Avoid this by instead returning an error from the store initialization, handling appropriately at the layer above. --- server/channels/app/platform/service.go | 6 ++- .../plugin_api_tests/test_db_driver/main.go | 5 ++- .../store/localcachelayer/layer_test.go | 23 ++++++---- .../channels/store/searchlayer/layer_test.go | 4 +- .../channels/store/sqlstore/migrate_test.go | 4 +- server/channels/store/sqlstore/store.go | 18 ++++---- server/channels/store/sqlstore/store_test.go | 43 +++++++++++++------ server/channels/testlib/helper.go | 22 ++++++++-- server/cmd/mattermost/commands/db.go | 5 ++- server/cmd/mattermost/commands/init.go | 2 +- .../searchengine/bleveengine/bleve_test.go | 7 ++- 11 files changed, 98 insertions(+), 41 deletions(-) diff --git a/server/channels/app/platform/service.go b/server/channels/app/platform/service.go index 430d6e75cf..8869f5ea40 100644 --- a/server/channels/app/platform/service.go +++ b/server/channels/app/platform/service.go @@ -204,7 +204,11 @@ func New(sc ServiceConfig, options ...Option) (*PlatformService, error) { // Timer layer // | // Cache layer - ps.sqlStore = sqlstore.New(ps.Config().SqlSettings, ps.metricsIFace) + var err error + ps.sqlStore, err = sqlstore.New(ps.Config().SqlSettings, ps.metricsIFace) + if err != nil { + return nil, err + } searchStore := searchlayer.NewSearchLayer( retrylayer.New(ps.sqlStore), diff --git a/server/channels/app/plugin_api_tests/test_db_driver/main.go b/server/channels/app/plugin_api_tests/test_db_driver/main.go index 195f03fc34..731fb0fc74 100644 --- a/server/channels/app/plugin_api_tests/test_db_driver/main.go +++ b/server/channels/app/plugin_api_tests/test_db_driver/main.go @@ -31,7 +31,10 @@ func (p *MyPlugin) OnConfigurationChange() error { func (p *MyPlugin) MessageWillBePosted(_ *plugin.Context, _ *model.Post) (*model.Post, string) { settings := p.API.GetUnsanitizedConfig().SqlSettings settings.Trace = model.NewBool(false) - store := sqlstore.New(settings, nil) + store, err := sqlstore.New(settings, nil) + if err != nil { + panic(err) + } store.GetMasterX().Close() for _, isMaster := range []bool{true, false} { diff --git a/server/channels/store/localcachelayer/layer_test.go b/server/channels/store/localcachelayer/layer_test.go index 040f915d24..ddbf6686ab 100644 --- a/server/channels/store/localcachelayer/layer_test.go +++ b/server/channels/store/localcachelayer/layer_test.go @@ -12,6 +12,7 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/channels/store/sqlstore" "github.com/mattermost/mattermost/server/v8/channels/store/storetest" + "golang.org/x/sync/errgroup" ) type storeType struct { @@ -91,23 +92,29 @@ func initStores() { panic(err) } }() - var wg sync.WaitGroup + var eg errgroup.Group for _, st := range storeTypes { st := st - wg.Add(1) - go func() { + eg.Go(func() error { var err error - defer wg.Done() - st.SqlStore = sqlstore.New(*st.SqlSettings, nil) + + st.SqlStore, err = sqlstore.New(*st.SqlSettings, nil) + if err != nil { + return err + } st.Store, err = NewLocalCacheLayer(st.SqlStore, nil, nil, getMockCacheProvider()) if err != nil { - panic(err) + return err } st.Store.DropAllTables() st.Store.MarkSystemRanUnitTests() - }() + + return nil + }) + } + if err := eg.Wait(); err != nil { + panic(err) } - wg.Wait() } var tearDownStoresOnce sync.Once diff --git a/server/channels/store/searchlayer/layer_test.go b/server/channels/store/searchlayer/layer_test.go index ac4fd3737f..069e86d584 100644 --- a/server/channels/store/searchlayer/layer_test.go +++ b/server/channels/store/searchlayer/layer_test.go @@ -14,6 +14,7 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/store/storetest" "github.com/mattermost/mattermost/server/v8/channels/testlib" "github.com/mattermost/mattermost/server/v8/platform/services/searchengine" + "github.com/stretchr/testify/require" ) // Test to verify race condition on UpdateConfig. The test must run with -race flag in order to verify @@ -24,7 +25,8 @@ func TestUpdateConfigRace(t *testing.T) { driverName = model.DatabaseDriverPostgres } settings := storetest.MakeSqlSettings(driverName, false) - store := sqlstore.New(*settings, nil) + store, err := sqlstore.New(*settings, nil) + require.NoError(t, err) cfg := &model.Config{} cfg.SetDefaults() diff --git a/server/channels/store/sqlstore/migrate_test.go b/server/channels/store/sqlstore/migrate_test.go index b5749c3e8f..487c95a257 100644 --- a/server/channels/store/sqlstore/migrate_test.go +++ b/server/channels/store/sqlstore/migrate_test.go @@ -8,6 +8,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUpAndDownMigrations(t *testing.T) { @@ -23,7 +24,8 @@ func TestUpAndDownMigrations(t *testing.T) { t.Skip(err) } - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) defer store.Close() err = store.migrate(migrationsDirectionDown, false) diff --git a/server/channels/store/sqlstore/store.go b/server/channels/store/sqlstore/store.go index bd33da1ded..6c1a465945 100644 --- a/server/channels/store/sqlstore/store.go +++ b/server/channels/store/sqlstore/store.go @@ -135,7 +135,7 @@ type SqlStore struct { wgMonitor *sync.WaitGroup } -func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlStore { +func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) (*SqlStore, error) { store := &SqlStore{ rrCounter: 0, srCounter: 0, @@ -147,7 +147,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS err := store.initConnection() if err != nil { - mlog.Fatal("Error setting up connections", mlog.Err(err)) + return nil, errors.Wrap(err, "error setting up connections") } store.wgMonitor.Add(1) @@ -155,32 +155,32 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS ver, err := store.GetDbVersion(true) if err != nil { - mlog.Fatal("Error while getting DB version.", mlog.Err(err)) + return nil, errors.Wrap(err, "error while getting DB version") } ok, err := store.ensureMinimumDBVersion(ver) if !ok { - mlog.Fatal("Error while checking DB version.", mlog.Err(err)) + return nil, errors.Wrap(err, "error while checking DB version") } err = store.ensureDatabaseCollation() if err != nil { - mlog.Fatal("Error while checking DB collation.", mlog.Err(err)) + return nil, errors.Wrap(err, "error while checking DB collation") } err = store.migrate(migrationsDirectionUp, false) if err != nil { - mlog.Fatal("Failed to apply database migrations.", mlog.Err(err)) + return nil, errors.Wrap(err, "failed to apply database migrations") } store.isBinaryParam, err = store.computeBinaryParam() if err != nil { - mlog.Fatal("Failed to compute binary param", mlog.Err(err)) + return nil, errors.Wrap(err, "failed to compute binary param") } store.pgDefaultTextSearchConfig, err = store.computeDefaultTextSearchConfig() if err != nil { - mlog.Fatal("Failed to compute default text search config", mlog.Err(err)) + return nil, errors.Wrap(err, "failed to compute default text search config") } store.stores.team = newSqlTeamStore(store) @@ -229,7 +229,7 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS store.stores.preference.(*SqlPreferenceStore).deleteUnusedFeatures() - return store + return store, nil } // SetupConnection sets up the connection to the database and pings it to make sure it's alive. diff --git a/server/channels/store/sqlstore/store_test.go b/server/channels/store/sqlstore/store_test.go index 8e319d4abd..0ccc001dd7 100644 --- a/server/channels/store/sqlstore/store_test.go +++ b/server/channels/store/sqlstore/store_test.go @@ -20,6 +20,7 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin/plugintest/mock" @@ -126,19 +127,26 @@ func initStores() { panic(err) } }() - var wg sync.WaitGroup + + var eg errgroup.Group for _, st := range storeTypes { st := st - wg.Add(1) - go func() { - defer wg.Done() - st.SqlStore = New(*st.SqlSettings, nil) + eg.Go(func() error { + var err error + st.SqlStore, err = New(*st.SqlSettings, nil) + if err != nil { + return err + } st.Store = st.SqlStore st.Store.DropAllTables() st.Store.MarkSystemRanUnitTests() - }() + + return nil + }) + } + if err := eg.Wait(); err != nil { + panic(err) } - wg.Wait() } var tearDownStoresOnce sync.Once @@ -175,7 +183,8 @@ func TestStoreLicenseRace(t *testing.T) { t.Skip(err) } - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) defer func() { store.Close() storetest.CleanupSqlSettings(settings) @@ -275,7 +284,8 @@ func TestGetReplica(t *testing.T) { settings.DataSourceReplicas = dataSourceReplicas settings.DataSourceSearchReplicas = dataSourceSearchReplicas - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) defer func() { store.Close() storetest.CleanupSqlSettings(settings) @@ -348,7 +358,8 @@ func TestGetReplica(t *testing.T) { settings.DataSourceReplicas = dataSourceReplicas settings.DataSourceSearchReplicas = dataSourceSearchReplicas - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) defer func() { store.Close() storetest.CleanupSqlSettings(settings) @@ -417,7 +428,8 @@ func TestGetDbVersion(t *testing.T) { t.Skip(err) } - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) version, err := store.GetDbVersion(false) require.NoError(t, err) @@ -641,7 +653,8 @@ func TestGetAllConns(t *testing.T) { settings.DataSourceReplicas = dataSourceReplicas settings.DataSourceSearchReplicas = dataSourceSearchReplicas - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) defer func() { store.Close() storetest.CleanupSqlSettings(settings) @@ -853,7 +866,8 @@ func TestGetDBSchemaVersion(t *testing.T) { if err != nil { t.Skip(err) } - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) assetsList, err := assets.ReadDir(filepath.Join("migrations", driver)) require.NoError(t, err) @@ -891,7 +905,8 @@ func TestGetAppliedMigrations(t *testing.T) { if err != nil { t.Skip(err) } - store := New(*settings, nil) + store, err := New(*settings, nil) + require.NoError(t, err) assetsList, err := assets.ReadDir(filepath.Join("migrations", driver)) require.NoError(t, err) diff --git a/server/channels/testlib/helper.go b/server/channels/testlib/helper.go index e3deae4c4b..b6f1596198 100644 --- a/server/channels/testlib/helper.go +++ b/server/channels/testlib/helper.go @@ -118,7 +118,12 @@ func (h *MainHelper) setupStore(withReadReplica bool) { h.SearchEngine = searchengine.NewBroker(config) h.ClusterInterface = &FakeClusterInterface{} - h.SQLStore = sqlstore.New(*h.Settings, nil) + + var err error + h.SQLStore, err = sqlstore.New(*h.Settings, nil) + if err != nil { + panic(err) + } h.Store = searchlayer.NewSearchLayer(&TestStore{ h.SQLStore, }, h.SearchEngine, config) @@ -130,7 +135,12 @@ func (h *MainHelper) ToggleReplicasOff() { } h.Settings.DataSourceReplicas = []string{} lic := h.SQLStore.GetLicense() - h.SQLStore = sqlstore.New(*h.Settings, nil) + + var err error + h.SQLStore, err = sqlstore.New(*h.Settings, nil) + if err != nil { + panic(err) + } h.SQLStore.UpdateLicense(lic) } @@ -140,7 +150,13 @@ func (h *MainHelper) ToggleReplicasOn() { } h.Settings.DataSourceReplicas = h.replicas lic := h.SQLStore.GetLicense() - h.SQLStore = sqlstore.New(*h.Settings, nil) + + var err error + h.SQLStore, err = sqlstore.New(*h.Settings, nil) + if err != nil { + panic(err) + } + h.SQLStore.UpdateLicense(lic) } diff --git a/server/cmd/mattermost/commands/db.go b/server/cmd/mattermost/commands/db.go index 7a9db36362..3f4bb5a2ef 100644 --- a/server/cmd/mattermost/commands/db.go +++ b/server/cmd/mattermost/commands/db.go @@ -113,7 +113,10 @@ func initDbCmdF(command *cobra.Command, _ []string) error { } defer configStore.Close() - sqlStore := sqlstore.New(configStore.Get().SqlSettings, nil) + sqlStore, err := sqlstore.New(configStore.Get().SqlSettings, nil) + if err != nil { + return errors.Wrap(err, "failed to initialize store") + } defer sqlStore.Close() CommandPrettyPrintln("Database store correctly initialised") diff --git a/server/cmd/mattermost/commands/init.go b/server/cmd/mattermost/commands/init.go index 3d9f416114..fc1e07b6a1 100644 --- a/server/cmd/mattermost/commands/init.go +++ b/server/cmd/mattermost/commands/init.go @@ -64,5 +64,5 @@ func initStoreCommandContextCobra(command *cobra.Command) (store.Store, error) { } config := cfgStore.Get() - return sqlstore.New(config.SqlSettings, nil), nil + return sqlstore.New(config.SqlSettings, nil) } diff --git a/server/platform/services/searchengine/bleveengine/bleve_test.go b/server/platform/services/searchengine/bleveengine/bleve_test.go index 3e166723ec..46b3ea8b66 100644 --- a/server/platform/services/searchengine/bleveengine/bleve_test.go +++ b/server/platform/services/searchengine/bleveengine/bleve_test.go @@ -49,7 +49,12 @@ func (s *BleveEngineTestSuite) setupStore() { driverName = model.DatabaseDriverPostgres } s.SQLSettings = storetest.MakeSqlSettings(driverName, false) - s.SQLStore = sqlstore.New(*s.SQLSettings, nil) + + var err error + s.SQLStore, err = sqlstore.New(*s.SQLSettings, nil) + if err != nil { + s.Require().FailNow("Cannot initialize store: %s", err.Error()) + } cfg := &model.Config{} cfg.SetDefaults()