From 4a03265dc2af6ac39fa78674e2a7f6bc991c4d49 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Mon, 19 Jun 2023 12:31:07 +0200 Subject: [PATCH 1/2] Fix force flag not working for bundle destroy (#434) ## Changes `--force` flag did not exist for `bundle destroy`. This PR adds that in. ## Tests manually tested. Now adding the `--force` flag hijacks the deploy lock on the target directory. --- cmd/bundle/deploy.go | 6 +++--- cmd/bundle/destroy.go | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/bundle/deploy.go b/cmd/bundle/deploy.go index 5ecc72d2..7dee32da 100644 --- a/cmd/bundle/deploy.go +++ b/cmd/bundle/deploy.go @@ -15,7 +15,7 @@ var deployCmd = &cobra.Command{ b := bundle.Get(cmd.Context()) // If `--force` is specified, force acquisition of the deployment lock. - b.Config.Bundle.Lock.Force = force + b.Config.Bundle.Lock.Force = forceDeploy return bundle.Apply(cmd.Context(), b, bundle.Seq( phases.Initialize(), @@ -25,9 +25,9 @@ var deployCmd = &cobra.Command{ }, } -var force bool +var forceDeploy bool func init() { AddCommand(deployCmd) - deployCmd.Flags().BoolVar(&force, "force", false, "Force acquisition of deployment lock.") + deployCmd.Flags().BoolVar(&forceDeploy, "force", false, "Force acquisition of deployment lock.") } diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index 979f01a9..d0fe699a 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -22,7 +22,7 @@ var destroyCmd = &cobra.Command{ b := bundle.Get(ctx) // If `--force` is specified, force acquisition of the deployment lock. - b.Config.Bundle.Lock.Force = force + b.Config.Bundle.Lock.Force = forceDestroy // If `--auto-approve`` is specified, we skip confirmation checks b.AutoApprove = autoApprove @@ -51,8 +51,10 @@ var destroyCmd = &cobra.Command{ } var autoApprove bool +var forceDestroy bool func init() { AddCommand(destroyCmd) destroyCmd.Flags().BoolVar(&autoApprove, "auto-approve", false, "Skip interactive approvals for deleting resources and files") + destroyCmd.Flags().BoolVar(&forceDestroy, "force", false, "Force acquisition of deployment lock.") } From 5d036ab6b8295a607867fe565579442ae390cca6 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Mon, 19 Jun 2023 15:57:25 +0200 Subject: [PATCH 2/2] Fix locker unlock for destroy (#492) ## Changes Adds ability for allowing unlock to succeed even if the deploy file is missing. ## Tests Using integration tests and manually --- bundle/deploy/lock/release.go | 30 +++++++++++----- bundle/deployer/deployer.go | 1 + bundle/phases/deploy.go | 2 +- bundle/phases/destroy.go | 2 +- internal/locker_test.go | 66 +++++++++++++++++++++++++++++++++++ libs/locker/locker.go | 36 +++++++++++++------ 6 files changed, 115 insertions(+), 22 deletions(-) diff --git a/bundle/deploy/lock/release.go b/bundle/deploy/lock/release.go index 9b6976fe..52d27194 100644 --- a/bundle/deploy/lock/release.go +++ b/bundle/deploy/lock/release.go @@ -2,15 +2,26 @@ package lock import ( "context" + "fmt" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" ) -type release struct{} +type Goal string -func Release() bundle.Mutator { - return &release{} +const ( + GoalDeploy = Goal("deploy") + GoalDestroy = Goal("destroy") +) + +type release struct { + goal Goal +} + +func Release(goal Goal) bundle.Mutator { + return &release{goal} } func (m *release) Name() string { @@ -32,11 +43,12 @@ func (m *release) Apply(ctx context.Context, b *bundle.Bundle) error { } log.Infof(ctx, "Releasing deployment lock") - err := b.Locker.Unlock(ctx) - if err != nil { - log.Errorf(ctx, "Failed to release deployment lock: %v", err) - return err + switch m.goal { + case GoalDeploy: + return b.Locker.Unlock(ctx) + case GoalDestroy: + return b.Locker.Unlock(ctx, locker.AllowLockFileNotExist) + default: + return fmt.Errorf("unknown goal for lock release: %s", m.goal) } - - return nil } diff --git a/bundle/deployer/deployer.go b/bundle/deployer/deployer.go index 1d780f72..f4f71897 100644 --- a/bundle/deployer/deployer.go +++ b/bundle/deployer/deployer.go @@ -108,6 +108,7 @@ func (d *Deployer) LoadTerraformState(ctx context.Context) error { if err != nil { return err } + defer r.Close() err = os.MkdirAll(d.DefaultTerraformRoot(), os.ModeDir) if err != nil { return err diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 050b5e56..f2692ea9 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -22,7 +22,7 @@ func Deploy() bundle.Mutator { terraform.Apply(), terraform.StatePush(), ), - lock.Release(), + lock.Release(lock.GoalDeploy), ), ) diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 5aaccdd2..b0ed5d62 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -20,7 +20,7 @@ func Destroy() bundle.Mutator { terraform.StatePush(), files.Delete(), ), - lock.Release(), + lock.Release(lock.GoalDestroy), ), ) diff --git a/internal/locker_test.go b/internal/locker_test.go index f3e026d6..2c7e7aa8 100644 --- a/internal/locker_test.go +++ b/internal/locker_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/databricks/cli/libs/filer" lockpkg "github.com/databricks/cli/libs/locker" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/service/workspace" @@ -126,6 +127,7 @@ func TestAccLock(t *testing.T) { // read active locker file r, err := lockers[indexOfActiveLocker].Read(ctx, "foo.json") require.NoError(t, err) + defer r.Close() b, err := io.ReadAll(r) require.NoError(t, err) @@ -159,3 +161,67 @@ func TestAccLock(t *testing.T) { assert.NoError(t, err) assert.True(t, lockers[indexOfAnInactiveLocker].Active) } + +func setupLockerTest(ctx context.Context, t *testing.T) (*lockpkg.Locker, filer.Filer) { + t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV")) + + w, err := databricks.NewWorkspaceClient() + require.NoError(t, err) + + // create temp wsfs dir + tmpDir := temporaryWorkspaceDir(t, w) + f, err := filer.NewWorkspaceFilesClient(w, tmpDir) + require.NoError(t, err) + + // create locker + locker, err := lockpkg.CreateLocker("redfoo@databricks.com", tmpDir, w) + require.NoError(t, err) + + return locker, f +} + +func TestAccLockUnlockWithoutAllowsLockFileNotExist(t *testing.T) { + ctx := context.Background() + locker, f := setupLockerTest(ctx, t) + var err error + + // Acquire lock on tmp directory + err = locker.Lock(ctx, false) + require.NoError(t, err) + + // Assert lock file is created + _, err = f.Stat(ctx, "deploy.lock") + assert.NoError(t, err) + + // Manually delete lock file + err = f.Delete(ctx, "deploy.lock") + assert.NoError(t, err) + + // Assert error, because lock file does not exist + err = locker.Unlock(ctx) + assert.ErrorIs(t, err, fs.ErrNotExist) +} + +func TestAccLockUnlockWithAllowsLockFileNotExist(t *testing.T) { + ctx := context.Background() + locker, f := setupLockerTest(ctx, t) + var err error + + // Acquire lock on tmp directory + err = locker.Lock(ctx, false) + require.NoError(t, err) + assert.True(t, locker.Active) + + // Assert lock file is created + _, err = f.Stat(ctx, "deploy.lock") + assert.NoError(t, err) + + // Manually delete lock file + err = f.Delete(ctx, "deploy.lock") + assert.NoError(t, err) + + // Assert error, because lock file does not exist + err = locker.Unlock(ctx, lockpkg.AllowLockFileNotExist) + assert.NoError(t, err) + assert.False(t, locker.Active) +} diff --git a/libs/locker/locker.go b/libs/locker/locker.go index 7c23f40e..bb95b784 100644 --- a/libs/locker/locker.go +++ b/libs/locker/locker.go @@ -13,8 +13,17 @@ import ( "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go" "github.com/google/uuid" + "golang.org/x/exp/slices" ) +type UnlockOption int + +const ( + AllowLockFileNotExist UnlockOption = iota +) + +const LockFileName = "deploy.lock" + // Locker object enables exclusive access to TargetDir's scope for a client. This // enables multiple clients to deploy to the same scope (ie TargetDir) in an atomic // manner @@ -65,10 +74,11 @@ type LockState struct { // GetActiveLockState returns current lock state, irrespective of us holding it. func (locker *Locker) GetActiveLockState(ctx context.Context) (*LockState, error) { - reader, err := locker.filer.Read(ctx, locker.RemotePath()) + reader, err := locker.filer.Read(ctx, LockFileName) if err != nil { return nil, err } + defer reader.Close() bytes, err := io.ReadAll(reader) if err != nil { @@ -89,7 +99,7 @@ func (locker *Locker) GetActiveLockState(ctx context.Context) (*LockState, error func (locker *Locker) assertLockHeld(ctx context.Context) error { activeLockState, err := locker.GetActiveLockState(ctx) if errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("no active lock on target dir: %s", err) + return fmt.Errorf("no active lock on target dir: %w", err) } if err != nil { return err @@ -140,7 +150,7 @@ func (locker *Locker) Lock(ctx context.Context, isForced bool) error { modes = append(modes, filer.OverwriteIfExists) } - err = locker.filer.Write(ctx, locker.RemotePath(), bytes.NewReader(buf), modes...) + err = locker.filer.Write(ctx, LockFileName, bytes.NewReader(buf), modes...) if err != nil { // If the write failed because the lock file already exists, don't return // the error and instead fall through to [assertLockHeld] below. @@ -161,15 +171,24 @@ func (locker *Locker) Lock(ctx context.Context, isForced bool) error { return nil } -func (locker *Locker) Unlock(ctx context.Context) error { +func (locker *Locker) Unlock(ctx context.Context, opts ...UnlockOption) error { if !locker.Active { return fmt.Errorf("unlock called when lock is not held") } + + // if allowLockFileNotExist is set, do not throw an error if the lock file does + // not exist. This is helpful when destroying a bundle in which case the lock + // file will be deleted before we have a chance to unlock + if _, err := locker.filer.Stat(ctx, LockFileName); errors.Is(err, fs.ErrNotExist) && slices.Contains(opts, AllowLockFileNotExist) { + locker.Active = false + return nil + } + err := locker.assertLockHeld(ctx) if err != nil { - return fmt.Errorf("unlock called when lock is not held: %s", err) + return fmt.Errorf("unlock called when lock is not held: %w", err) } - err = locker.filer.Delete(ctx, locker.RemotePath()) + err = locker.filer.Delete(ctx, LockFileName) if err != nil { return err } @@ -177,11 +196,6 @@ func (locker *Locker) Unlock(ctx context.Context) error { return nil } -func (locker *Locker) RemotePath() string { - // Note: remote paths are scoped to `targetDir`. Also see [CreateLocker]. - return "deploy.lock" -} - func CreateLocker(user string, targetDir string, w *databricks.WorkspaceClient) (*Locker, error) { filer, err := filer.NewWorkspaceFilesClient(w, targetDir) if err != nil {