From 95299be52d51eb4f6a17345b8c2b0729f4f2bd6c Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Thu, 13 Jun 2024 13:39:19 +0300 Subject: [PATCH] Deprecate FileStore engine support (#2119) * Removejsonfile' from test matrix in workflows * Remove sqlite to json migration command * Refactor store engine implementation to remove JSON file store support The codebase has been refactored to remove support for JSON file store storage engine, with SQLite serving as the default store engine. New functions have been added to handle unsupported store engines and to migrate data from file store to SQLite. * Remove 'downCmd' from migration commands * Refactoring * Add sqlite cleanup * Remove comment --- .github/workflows/golang-test-darwin.yml | 2 +- .github/workflows/golang-test-linux.yml | 2 +- client/internal/engine_test.go | 10 +- management/cmd/migration_down.go | 67 ----------- management/cmd/migration_up.go | 34 +----- management/cmd/root.go | 1 - management/server/sql_store.go | 18 ++- management/server/store.go | 142 ++++++++++++++++------- 8 files changed, 126 insertions(+), 150 deletions(-) delete mode 100644 management/cmd/migration_down.go diff --git a/.github/workflows/golang-test-darwin.yml b/.github/workflows/golang-test-darwin.yml index d7007c860..2b4c43cb4 100644 --- a/.github/workflows/golang-test-darwin.yml +++ b/.github/workflows/golang-test-darwin.yml @@ -14,7 +14,7 @@ jobs: test: strategy: matrix: - store: ['jsonfile', 'sqlite'] + store: ['sqlite'] runs-on: macos-latest steps: - name: Install Go diff --git a/.github/workflows/golang-test-linux.yml b/.github/workflows/golang-test-linux.yml index 4259f1b3e..c30d08eed 100644 --- a/.github/workflows/golang-test-linux.yml +++ b/.github/workflows/golang-test-linux.yml @@ -15,7 +15,7 @@ jobs: strategy: matrix: arch: [ '386','amd64' ] - store: [ 'jsonfile', 'sqlite', 'postgres'] + store: [ 'sqlite', 'postgres'] runs-on: ubuntu-latest steps: - name: Install Go diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index a6461e24b..176d65459 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -817,7 +817,7 @@ func TestEngine_MultiplePeers(t *testing.T) { return } defer sigServer.Stop() - mgmtServer, mgmtAddr, err := startManagement(dir) + mgmtServer, mgmtAddr, err := startManagement(t, dir) if err != nil { t.Fatal(err) return @@ -1037,7 +1037,9 @@ func startSignal(t *testing.T) (*grpc.Server, string, error) { return s, lis.Addr().String(), nil } -func startManagement(dataDir string) (*grpc.Server, string, error) { +func startManagement(t *testing.T, dataDir string) (*grpc.Server, string, error) { + t.Helper() + config := &server.Config{ Stuns: []*server.Host{}, TURNConfig: &server.TURNConfig{}, @@ -1054,10 +1056,12 @@ func startManagement(dataDir string) (*grpc.Server, string, error) { return nil, "", err } s := grpc.NewServer(grpc.KeepaliveEnforcementPolicy(kaep), grpc.KeepaliveParams(kasp)) - store, _, err := server.NewTestStoreFromJson(config.Datadir) + + store, cleanUp, err := server.NewTestStoreFromJson(config.Datadir) if err != nil { return nil, "", err } + t.Cleanup(cleanUp) peersUpdateManager := server.NewPeersUpdateManager(nil) eventStore := &activity.InMemoryEventStore{} diff --git a/management/cmd/migration_down.go b/management/cmd/migration_down.go deleted file mode 100644 index 81ef93a6c..000000000 --- a/management/cmd/migration_down.go +++ /dev/null @@ -1,67 +0,0 @@ -package cmd - -import ( - "errors" - "flag" - "fmt" - "os" - "path" - - log "github.com/sirupsen/logrus" - "github.com/spf13/cobra" - - "github.com/netbirdio/netbird/management/server" - "github.com/netbirdio/netbird/util" -) - -var shortDown = "Rollback SQLite store to JSON file store. Please make a backup of the SQLite file before running this command." - -var downCmd = &cobra.Command{ - Use: "downgrade [--datadir directory] [--log-file console]", - Aliases: []string{"down"}, - Short: shortDown, - Long: shortDown + - "\n\n" + - "This command reads the content of {datadir}/store.db and migrates it to {datadir}/store.json that can be used by File store driver.", - RunE: func(cmd *cobra.Command, args []string) error { - flag.Parse() - err := util.InitLog(logLevel, logFile) - if err != nil { - return fmt.Errorf("failed initializing log %v", err) - } - - sqliteStorePath := path.Join(mgmtDataDir, "store.db") - if _, err := os.Stat(sqliteStorePath); errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("%s doesn't exist, couldn't continue the operation", sqliteStorePath) - } - - fileStorePath := path.Join(mgmtDataDir, "store.json") - if _, err := os.Stat(fileStorePath); err == nil { - return fmt.Errorf("%s already exists, couldn't continue the operation", fileStorePath) - } - - sqlStore, err := server.NewSqliteStore(mgmtDataDir, nil) - if err != nil { - return fmt.Errorf("failed creating file store: %s: %v", mgmtDataDir, err) - } - - sqliteStoreAccounts := len(sqlStore.GetAllAccounts()) - log.Infof("%d account will be migrated from sqlite store %s to file store %s", - sqliteStoreAccounts, sqliteStorePath, fileStorePath) - - store, err := server.NewFilestoreFromSqliteStore(sqlStore, mgmtDataDir, nil) - if err != nil { - return fmt.Errorf("failed creating file store: %s: %v", mgmtDataDir, err) - } - - fsStoreAccounts := len(store.GetAllAccounts()) - if fsStoreAccounts != sqliteStoreAccounts { - return fmt.Errorf("failed to migrate accounts from sqlite to file[]. Expected accounts: %d, got: %d", - sqliteStoreAccounts, fsStoreAccounts) - } - - log.Info("Migration finished successfully") - - return nil - }, -} diff --git a/management/cmd/migration_up.go b/management/cmd/migration_up.go index 5c7505cfc..89adfce55 100644 --- a/management/cmd/migration_up.go +++ b/management/cmd/migration_up.go @@ -1,11 +1,8 @@ package cmd import ( - "errors" "flag" "fmt" - "os" - "path" "github.com/netbirdio/netbird/management/server" "github.com/netbirdio/netbird/util" @@ -29,36 +26,9 @@ var upCmd = &cobra.Command{ return fmt.Errorf("failed initializing log %v", err) } - fileStorePath := path.Join(mgmtDataDir, "store.json") - if _, err := os.Stat(fileStorePath); errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("%s doesn't exist, couldn't continue the operation", fileStorePath) + if err := server.MigrateFileStoreToSqlite(mgmtDataDir); err != nil { + return err } - - sqlStorePath := path.Join(mgmtDataDir, "store.db") - if _, err := os.Stat(sqlStorePath); err == nil { - return fmt.Errorf("%s already exists, couldn't continue the operation", sqlStorePath) - } - - fstore, err := server.NewFileStore(mgmtDataDir, nil) - if err != nil { - return fmt.Errorf("failed creating file store: %s: %v", mgmtDataDir, err) - } - - fsStoreAccounts := len(fstore.GetAllAccounts()) - log.Infof("%d account will be migrated from file store %s to sqlite store %s", - fsStoreAccounts, fileStorePath, sqlStorePath) - - store, err := server.NewSqliteStoreFromFileStore(fstore, mgmtDataDir, nil) - if err != nil { - return fmt.Errorf("failed creating file store: %s: %v", mgmtDataDir, err) - } - - sqliteStoreAccounts := len(store.GetAllAccounts()) - if fsStoreAccounts != sqliteStoreAccounts { - return fmt.Errorf("failed to migrate accounts from file to sqlite. Expected accounts: %d, got: %d", - fsStoreAccounts, sqliteStoreAccounts) - } - log.Info("Migration finished successfully") return nil diff --git a/management/cmd/root.go b/management/cmd/root.go index f5c533969..32c044a56 100644 --- a/management/cmd/root.go +++ b/management/cmd/root.go @@ -75,7 +75,6 @@ func init() { migrationCmd.MarkFlagRequired("datadir") //nolint migrationCmd.AddCommand(upCmd) - migrationCmd.AddCommand(downCmd) rootCmd.AddCommand(migrationCmd) } diff --git a/management/server/sql_store.go b/management/server/sql_store.go index 56136327a..2153a011e 100644 --- a/management/server/sql_store.go +++ b/management/server/sql_store.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "os" "path/filepath" "runtime" "strings" @@ -27,6 +28,10 @@ import ( "github.com/netbirdio/netbird/route" ) +const ( + storeSqliteFileName = "store.db" +) + // SqlStore represents an account storage backed by a Sql DB persisted to disk type SqlStore struct { db *gorm.DB @@ -623,10 +628,10 @@ func (s *SqlStore) GetStoreEngine() StoreEngine { // NewSqliteStore creates a new SQLite store. func NewSqliteStore(dataDir string, metrics telemetry.AppMetrics) (*SqlStore, error) { - storeStr := "store.db?cache=shared" + storeStr := fmt.Sprintf("%s?cache=shared", storeSqliteFileName) if runtime.GOOS == "windows" { // Vo avoid `The process cannot access the file because it is being used by another process` on Windows - storeStr = "store.db" + storeStr = storeSqliteFileName } file := filepath.Join(dataDir, storeStr) @@ -655,6 +660,15 @@ func NewPostgresqlStore(dsn string, metrics telemetry.AppMetrics) (*SqlStore, er return NewSqlStore(db, PostgresStoreEngine, metrics) } +// newPostgresStore initializes a new Postgres store. +func newPostgresStore(metrics telemetry.AppMetrics) (Store, error) { + dsn, ok := os.LookupEnv(postgresDsnEnv) + if !ok { + return nil, fmt.Errorf("%s is not set", postgresDsnEnv) + } + return NewPostgresqlStore(dsn, metrics) +} + // NewSqliteStoreFromFileStore restores a store from FileStore and stores SQLite DB in the file located in datadir. func NewSqliteStoreFromFileStore(fileStore *FileStore, dataDir string, metrics telemetry.AppMetrics) (*SqlStore, error) { store, err := NewSqliteStore(dataDir, metrics) diff --git a/management/server/store.go b/management/server/store.go index 5210f1210..8a4fc4553 100644 --- a/management/server/store.go +++ b/management/server/store.go @@ -1,21 +1,24 @@ package server import ( + "errors" "fmt" "net" "net/netip" "os" + "path" "path/filepath" "strings" "time" + "github.com/netbirdio/netbird/management/server/telemetry" + "github.com/netbirdio/netbird/util" log "github.com/sirupsen/logrus" "gorm.io/gorm" "github.com/netbirdio/netbird/management/server/migration" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/posture" - "github.com/netbirdio/netbird/management/server/telemetry" "github.com/netbirdio/netbird/management/server/testutil" "github.com/netbirdio/netbird/route" ) @@ -76,50 +79,74 @@ func getStoreEngineFromEnv() StoreEngine { } value := StoreEngine(strings.ToLower(kind)) - if value == FileStoreEngine || value == SqliteStoreEngine || value == PostgresStoreEngine { + if value == SqliteStoreEngine || value == PostgresStoreEngine { return value } return SqliteStoreEngine } -func getStoreEngineFromDatadir(dataDir string) StoreEngine { - storeFile := filepath.Join(dataDir, storeFileName) - if _, err := os.Stat(storeFile); err != nil { - // json file not found then use sqlite as default - return SqliteStoreEngine - } - return FileStoreEngine -} - -func NewStore(kind StoreEngine, dataDir string, metrics telemetry.AppMetrics) (Store, error) { +// getStoreEngine determines the store engine to use +func getStoreEngine(kind StoreEngine) StoreEngine { if kind == "" { - // if store engine is not set in the config we first try to evaluate NETBIRD_STORE_ENGINE kind = getStoreEngineFromEnv() if kind == "" { - // NETBIRD_STORE_ENGINE is not set we evaluate default based on dataDir - kind = getStoreEngineFromDatadir(dataDir) + kind = SqliteStoreEngine } } + return kind +} + +// NewStore creates a new store based on the provided engine type, data directory, and telemetry metrics +func NewStore(kind StoreEngine, dataDir string, metrics telemetry.AppMetrics) (Store, error) { + kind = getStoreEngine(kind) + + if err := checkFileStoreEngine(kind, dataDir); err != nil { + return nil, err + } + switch kind { - case FileStoreEngine: - log.Info("using JSON file store engine") - return NewFileStore(dataDir, metrics) case SqliteStoreEngine: log.Info("using SQLite store engine") return NewSqliteStore(dataDir, metrics) case PostgresStoreEngine: log.Info("using Postgres store engine") - dsn, ok := os.LookupEnv(postgresDsnEnv) - if !ok { - return nil, fmt.Errorf("%s is not set", postgresDsnEnv) - } - return NewPostgresqlStore(dsn, metrics) + return newPostgresStore(metrics) default: - return nil, fmt.Errorf("unsupported kind of store %s", kind) + return handleUnsupportedStoreEngine(kind, dataDir, metrics) } } +func checkFileStoreEngine(kind StoreEngine, dataDir string) error { + if kind == FileStoreEngine { + storeFile := filepath.Join(dataDir, storeFileName) + if util.FileExists(storeFile) { + return fmt.Errorf("%s is not supported. Please refer to the documentation for migrating to SQLite: "+ + "https://docs.netbird.io/selfhosted/sqlite-store#migrating-from-json-store-to-sq-lite-store", FileStoreEngine) + } + } + return nil +} + +// handleUnsupportedStoreEngine handles cases where the store engine is unsupported +func handleUnsupportedStoreEngine(kind StoreEngine, dataDir string, metrics telemetry.AppMetrics) (Store, error) { + jsonStoreFile := filepath.Join(dataDir, storeFileName) + sqliteStoreFile := filepath.Join(dataDir, storeSqliteFileName) + + if util.FileExists(jsonStoreFile) && !util.FileExists(sqliteStoreFile) { + log.Warnf("unsupported store engine, but found %s. Automatically migrating to SQLite.", jsonStoreFile) + + if err := MigrateFileStoreToSqlite(dataDir); err != nil { + return nil, fmt.Errorf("failed to migrate data to SQLite store: %w", err) + } + + log.Info("using SQLite store engine") + return NewSqliteStore(dataDir, metrics) + } + + return nil, fmt.Errorf("unsupported kind of store: %s", kind) +} + // migrate migrates the SQLite database to the latest schema func migrate(db *gorm.DB) error { migrations := getMigrations() @@ -160,25 +187,18 @@ func NewTestStoreFromJson(dataDir string) (Store, func(), error) { return nil, nil, err } - cleanUp := func() {} - // if store engine is not set in the config we first try to evaluate NETBIRD_STORE_ENGINE kind := getStoreEngineFromEnv() if kind == "" { - // NETBIRD_STORE_ENGINE is not set we evaluate default based on dataDir - kind = getStoreEngineFromDatadir(dataDir) + kind = SqliteStoreEngine } - switch kind { - case FileStoreEngine: - return fstore, cleanUp, nil - case SqliteStoreEngine: - store, err := NewSqliteStoreFromFileStore(fstore, dataDir, nil) - if err != nil { - return nil, nil, err - } - return store, cleanUp, nil - case PostgresStoreEngine: + var ( + store Store + cleanUp func() + ) + + if kind == PostgresStoreEngine { cleanUp, err = testutil.CreatePGDB() if err != nil { return nil, nil, err @@ -189,16 +209,52 @@ func NewTestStoreFromJson(dataDir string) (Store, func(), error) { return nil, nil, fmt.Errorf("%s is not set", postgresDsnEnv) } - store, err := NewPostgresqlStoreFromFileStore(fstore, dsn, nil) + store, err = NewPostgresqlStoreFromFileStore(fstore, dsn, nil) if err != nil { return nil, nil, err } - return store, cleanUp, nil - default: - store, err := NewSqliteStoreFromFileStore(fstore, dataDir, nil) + } else { + store, err = NewSqliteStoreFromFileStore(fstore, dataDir, nil) if err != nil { return nil, nil, err } - return store, cleanUp, nil + cleanUp = func() { store.Close() } } + + return store, cleanUp, nil +} + +// MigrateFileStoreToSqlite migrates the file store to the SQLite store. +func MigrateFileStoreToSqlite(dataDir string) error { + fileStorePath := path.Join(dataDir, storeFileName) + if _, err := os.Stat(fileStorePath); errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("%s doesn't exist, couldn't continue the operation", fileStorePath) + } + + sqlStorePath := path.Join(dataDir, storeSqliteFileName) + if _, err := os.Stat(sqlStorePath); err == nil { + return fmt.Errorf("%s already exists, couldn't continue the operation", sqlStorePath) + } + + fstore, err := NewFileStore(dataDir, nil) + if err != nil { + return fmt.Errorf("failed creating file store: %s: %v", dataDir, err) + } + + fsStoreAccounts := len(fstore.GetAllAccounts()) + log.Infof("%d account will be migrated from file store %s to sqlite store %s", + fsStoreAccounts, fileStorePath, sqlStorePath) + + store, err := NewSqliteStoreFromFileStore(fstore, dataDir, nil) + if err != nil { + return fmt.Errorf("failed creating file store: %s: %v", dataDir, err) + } + + sqliteStoreAccounts := len(store.GetAllAccounts()) + if fsStoreAccounts != sqliteStoreAccounts { + return fmt.Errorf("failed to migrate accounts from file to sqlite. Expected accounts: %d, got: %d", + fsStoreAccounts, sqliteStoreAccounts) + } + + return nil }