From ee9e346039ebe5f60782be24c82dd0c36fc1a783 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 17 Sep 2021 15:08:29 -0700 Subject: [PATCH] refactoring: ApplyMoves skips moving when destination address occupied Per our rule that the content of the state can never make a move statement invalid, our behavior for two objects trying to occupy the same address will be to just ignore that and let the object already at the address take priority. For the moment this is silent from an end-user perspective and appears only in our internal logs. However, I'm hoping that our future planned adjustment to the interface of this function will include some way to allow reporting these collisions in some end-user-visible way, either as a separate warning per collision or as a single warning that collects together all of the collisions into a single message somehow. This situation can arise both because the previous run state already contained an object at the target address of a move and because more than one move ends up trying to target the same location. In the latter case, which one "wins" is decided by our depth-first traversal order, which is in turn derived from our chaining and nesting rules and is therefore arbitrary but deterministic. --- internal/refactoring/move_execute.go | 36 +++++++++ internal/refactoring/move_execute_test.go | 96 +++++++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 178a336af..66c0d6c0a 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -79,6 +79,18 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // directly. if newAddr, matches := modAddr.MoveDestination(stmt.From, stmt.To); matches { log.Printf("[TRACE] refactoring.ApplyMoves: %s has moved to %s", modAddr, newAddr) + + // If we already have a module at the new address then + // we'll skip this move and let the existing object take + // priority. + // TODO: This should probably generate a user-visible + // warning, but we'd need to rethink the signature of this + // function to achieve that. + if ms := state.Module(newAddr); ms != nil { + log.Printf("[WARN] Skipped moving %s to %s, because there's already another module instance at the destination", modAddr, newAddr) + continue + } + // We need to visit all of the resource instances in the // module and record them individually as results. for _, rs := range ms.Resources { @@ -105,6 +117,18 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] rAddr := rs.Addr if newAddr, matches := rAddr.MoveDestination(stmt.From, stmt.To); matches { log.Printf("[TRACE] refactoring.ApplyMoves: resource %s has moved to %s", rAddr, newAddr) + + // If we already have a resource at the new address then + // we'll skip this move and let the existing object take + // priority. + // TODO: This should probably generate a user-visible + // warning, but we'd need to rethink the signature of this + // function to achieve that. + if rs := state.Resource(newAddr); rs != nil { + log.Printf("[WARN] Skipped moving %s to %s, because there's already another resource at the destination", rAddr, newAddr) + continue + } + for key := range rs.Instances { oldInst := rAddr.Instance(key) newInst := newAddr.Instance(key) @@ -122,6 +146,18 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] iAddr := rAddr.Instance(key) if newAddr, matches := iAddr.MoveDestination(stmt.From, stmt.To); matches { log.Printf("[TRACE] refactoring.ApplyMoves: resource instance %s has moved to %s", iAddr, newAddr) + + // If we already have a resource instance at the new + // address then we'll skip this move and let the existing + // object take priority. + // TODO: This should probably generate a user-visible + // warning, but we'd need to rethink the signature of this + // function to achieve that. + if is := state.ResourceInstance(newAddr); is != nil { + log.Printf("[WARN] Skipped moving %s to %s, because there's already another resource instance at the destination", iAddr, newAddr) + continue + } + result := MoveResult{From: iAddr, To: newAddr} results[iAddr.UniqueKey()] = result results[newAddr.UniqueKey()] = result diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 63c54a7ad..f3ab1ec1e 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -391,6 +391,102 @@ func TestApplyMoves(t *testing.T) { `module.bar[0].foo.to[0]`, }, }, + + "move module instance to already-existing module instance": { + []MoveStatement{ + testMoveStatement(t, "", "module.bar[0]", "module.boo"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + instAddrs["module.bar[0].foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + instAddrs["module.boo.foo.to[0]"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + map[addrs.UniqueKey]MoveResult{ + // Nothing moved, because the module.b address is already + // occupied by another module. + }, + []string{ + `module.bar[0].foo.from`, + `module.boo.foo.to[0]`, + }, + }, + + "move resource to already-existing resource": { + []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + instAddrs["foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + instAddrs["foo.to"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + map[addrs.UniqueKey]MoveResult{ + // Nothing moved, because the module.b address is already + // occupied by another module. + }, + []string{ + `foo.from`, + `foo.to`, + }, + }, + + "move resource instance to already-existing resource instance": { + []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to[0]"), + }, + states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + instAddrs["foo.from"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + s.SetResourceInstanceCurrent( + instAddrs["foo.to[0]"], + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + providerAddr, + ) + }), + map[addrs.UniqueKey]MoveResult{ + // Nothing moved, because the module.b address is already + // occupied by another module. + }, + []string{ + `foo.from`, + `foo.to[0]`, + }, + }, } for name, test := range tests {