From 9002930d65c47db2002969fe3f30f7e1908b1c23 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 12 Sep 2022 10:30:52 -0400 Subject: [PATCH] Adapt shared lock skip logic to dbswap branch (#2437) --- internal/cmd/commands/server/server.go | 29 +++++++++++---- internal/db/schema/manager.go | 51 +++++++++++++++++++------- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/internal/cmd/commands/server/server.go b/internal/cmd/commands/server/server.go index b90f4272a5..93a24cc8ac 100644 --- a/internal/cmd/commands/server/server.go +++ b/internal/cmd/commands/server/server.go @@ -402,7 +402,7 @@ func (c *Command) Run(args []string) int { return base.CommandCliError } - sm, err := acquireDatabaseSharedLock(c.Context, c.Server.Database) + sm, err := acquireSchemaManager(c.Context, c.Server.Database, c.Config.Controller.Database.SkipSharedLockAcquisition) if err != nil { c.UI.Error(fmt.Errorf("Failed to acquire database shared lock: %w", err).Error()) return base.CommandCliError @@ -433,6 +433,13 @@ func (c *Command) Run(args []string) int { c.UI.Error(err.Error()) return base.CommandCliError } + + if c.Config.Controller.Database.SkipSharedLockAcquisition { + if err := c.schemaManager.Close(c.Context); err != nil { + c.UI.Error(fmt.Errorf("Unable to release shared lock to the database: %w", err).Error()) + return base.CommandCliError + } + } } if c.Config.Controller != nil { @@ -874,7 +881,7 @@ func (c *Command) reloadControllerDatabase(newConfig *config.Config) error { } // Acquire new lock on the new database and verify that it's in a good state to be used. - newDbSchemaManager, err := acquireDatabaseSharedLock(c.Context, newDb) + newDbSchemaManager, err := acquireSchemaManager(c.Context, newDb, c.Config.Controller.Database.SkipSharedLockAcquisition) if err != nil { _ = newDb.Close(c.Context) return fmt.Errorf("failed to acquire shared lock on new database: %w", err) @@ -887,6 +894,12 @@ func (c *Command) reloadControllerDatabase(newConfig *config.Config) error { return fmt.Errorf("invalid new database state: %w", err) } + if newConfig.Controller.Database.SkipSharedLockAcquisition { + if err := newDbSchemaManager.Close(c.Context); err != nil { + return fmt.Errorf("unable to release shared lock to the database for new schema manager: %w", err) + } + } + oldDbSchemaManager := c.schemaManager // Swap underlying database with new one and update application state. @@ -907,10 +920,10 @@ func (c *Command) reloadControllerDatabase(newConfig *config.Config) error { return nil } -// acquireDatabaseSharedLock uses the schema manager to acquire a shared lock on +// acquireSchemaManager returns a schema manager and generally acquires a shared lock on // the database. This is done as a mechanism to disallow running migration commands // while the database is in use. -func acquireDatabaseSharedLock(ctx context.Context, db *db.DB) (*schema.Manager, error) { +func acquireSchemaManager(ctx context.Context, db *db.DB, skipSharedLock bool) (*schema.Manager, error) { if db == nil { return nil, fmt.Errorf("nil database") } @@ -926,9 +939,11 @@ func acquireDatabaseSharedLock(ctx context.Context, db *db.DB) (*schema.Manager, } // This is an advisory locks on the DB which is released when the db session ends. - err = manager.SharedLock(ctx) - if err != nil { - return nil, fmt.Errorf("failed to gain shared access to the database: %w", err) + if !skipSharedLock { + err = manager.SharedLock(ctx) + if err != nil { + return nil, fmt.Errorf("failed to gain shared access to the database: %w", err) + } } return manager, nil diff --git a/internal/db/schema/manager.go b/internal/db/schema/manager.go index 95d042540e..aab1158339 100644 --- a/internal/db/schema/manager.go +++ b/internal/db/schema/manager.go @@ -6,6 +6,7 @@ import ( "database/sql" "fmt" "io" + "sync" "github.com/hashicorp/boundary/internal/db/schema/internal/edition" "github.com/hashicorp/boundary/internal/db/schema/internal/log" @@ -13,6 +14,7 @@ import ( "github.com/hashicorp/boundary/internal/db/schema/internal/provider" "github.com/hashicorp/boundary/internal/db/schema/migration" "github.com/hashicorp/boundary/internal/errors" + "github.com/hashicorp/boundary/internal/observability/event" ) // driver provides functionality to a database. @@ -59,11 +61,13 @@ type driver interface { // the underlying boundary database schema. // Manager is not thread safe. type Manager struct { - db *sql.DB - driver driver - dialect string - editions edition.Editions - selectedRepairs RepairMigrations + db *sql.DB + driver driver + dialect string + editions edition.Editions + selectedRepairs RepairMigrations + sharedLockAcquired bool + sharedLockMutex *sync.Mutex } // NewManager creates a new schema manager. An error is returned @@ -79,6 +83,7 @@ func NewManager(ctx context.Context, dialect Dialect, db *sql.DB, opt ...Option) db: db, dialect: string(dialect), selectedRepairs: opts.withRepairMigrations, + sharedLockMutex: new(sync.Mutex), } if opts.withEditions != nil { dbM.editions = opts.withEditions @@ -128,12 +133,16 @@ func (b *Manager) CurrentState(ctx context.Context) (*State, error) { // database connection afterwards. func (b *Manager) Close(ctx context.Context) error { const op = "schema.(Manager).Close" - err := b.SharedUnlock(ctx) - if err != nil { - return errors.Wrap(ctx, err, op) + + if err := b.SharedUnlock(ctx); err != nil { + event.WriteError(ctx, op, fmt.Errorf("error unlocking shared database lock: %w", err)) + } + + if err := b.driver.Close(); err != sql.ErrConnDone { + return err } - return b.driver.Close() + return nil } // SharedLock attempts to obtain a shared lock on the database. This can fail @@ -141,9 +150,17 @@ func (b *Manager) Close(ctx context.Context) error { // error is returned. func (b *Manager) SharedLock(ctx context.Context) error { const op = "schema.(Manager).SharedLock" - if err := b.driver.TrySharedLock(ctx); err != nil { - return errors.Wrap(ctx, err, op) + + b.sharedLockMutex.Lock() + defer b.sharedLockMutex.Unlock() + + if !b.sharedLockAcquired { + if err := b.driver.TrySharedLock(ctx); err != nil { + return errors.Wrap(ctx, err, op) + } + b.sharedLockAcquired = true } + return nil } @@ -152,9 +169,17 @@ func (b *Manager) SharedLock(ctx context.Context) error { // that is not held is not an error. func (b *Manager) SharedUnlock(ctx context.Context) error { const op = "schema.(Manager).SharedUnlock" - if err := b.driver.UnlockShared(ctx); err != nil { - return errors.Wrap(ctx, err, op) + + b.sharedLockMutex.Lock() + defer b.sharedLockMutex.Unlock() + + if b.sharedLockAcquired { + if err := b.driver.UnlockShared(ctx); err != nil { + return errors.Wrap(ctx, err, op) + } + b.sharedLockAcquired = false } + return nil }