Merge pull request #13430 from hashicorp/jbardin/consul-delete-locks

cleanup consul lock entries

backend/remote-state/consul/client.go

@@ -121,16 +121,15 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 	default:
 		if c.lockCh != nil {
 			// we have an active lock already
-			return "", nil
+			return "", fmt.Errorf("state %q already locked", c.Path)
 		}
 	}
 
 	if c.consulLock == nil {
 		opts := &consulapi.LockOptions{
 			Key: c.Path + lockSuffix,
-			// We currently don't procide any options to block terraform and
-			// retry lock acquisition, but we can wait briefly in case the
-			// lock is about to be freed.
+			// only wait briefly, so terraform has the choice to fail fast or
+			// retry as needed.
 			LockWaitTime: time.Second,
 			LockTryOnce:  true,
 		}
@@ -191,6 +190,10 @@ func (c *RemoteClient) Unlock(id string) error {
 	err := c.consulLock.Unlock()
 	c.lockCh = nil
 
+	// This is only cleanup, and will fail if the lock was immediately taken by
+	// another client, so we don't report an error to the user here.
+	c.consulLock.Destroy()
+
 	kv := c.Client.KV()
 	_, delErr := kv.Delete(c.Path+lockInfoSuffix, nil)
 	if delErr != nil {

backend/remote-state/consul/client_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/state/remote"
 )
 
@@ -98,3 +99,43 @@ func TestConsul_stateLock(t *testing.T) {
 
 	remote.TestRemoteLocks(t, sA.(*remote.State).Client, sB.(*remote.State).Client)
 }
+
+func TestConsul_destroyLock(t *testing.T) {
+	srv := newConsulTestServer(t)
+	defer srv.Stop()
+
+	// Get the backend
+	b := backend.TestBackendConfig(t, New(), map[string]interface{}{
+		"address": srv.HTTPAddr,
+		"path":    fmt.Sprintf("tf-unit/%s", time.Now().String()),
+	})
+
+	// Grab the client
+	s, err := b.State(backend.DefaultStateName)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	c := s.(*remote.State).Client.(*RemoteClient)
+
+	info := state.NewLockInfo()
+	id, err := c.Lock(info)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	lockPath := c.Path + lockSuffix
+
+	if err := c.Unlock(id); err != nil {
+		t.Fatal(err)
+	}
+
+	// get the lock val
+	pair, _, err := c.Client.KV().Get(lockPath, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if pair != nil {
+		t.Fatalf("lock key not cleaned up at: %s", pair.Key)
+	}
+}