Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_storage_account - fix hanging destroy caused by multiple network rules #5565

Merged
merged 4 commits into from
Feb 7, 2020
Merged

azurerm_storage_account - fix hanging destroy caused by multiple network rules #5565

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
261a08a
Providing a fix for issue 5434
krispetkov Jan 30, 2020
91a6815
More generic fix for duplicated lock values
krispetkov Feb 4, 2020
03a0bf4
Remove duplicated strings and use unique strings for locking
krispetkov Feb 5, 2020
c2ea114
Fix boolean check after travis-ci check failed
krispetkov Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions azurerm/internal/locks/lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package locks

import "github.com/hashicorp/terraform-plugin-sdk/helper/mutexkv"
import (
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/helper/mutexkv"
)

// armMutexKV is the instance of MutexKV for ARM resources
var armMutexKV = mutexkv.NewMutexKV()
Expand All @@ -16,8 +20,11 @@ func ByName(name string, resourceType string) {
}

func MultipleByName(names *[]string, resourceType string) {
for _, name := range *names {
ByName(name, resourceType)
for i, name := range *names {
// at the end of every array item add its index. This way we guarantee that this item will be unique (no duplicates are possible)
uniqueValue := fmt.Sprintf("%s-arrIdx-%d", name, i)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same issue as the previous solution, if two resources call this with resource N, but they have different indexes we could deadlock. What we should do is just ensure there are no duplicates:

func SliceUniqString(elements []string) []sring {
	seen := make(map[int]bool, len(s))
	j := 0
	for _, v := range s {
		if _, ok := seen[v]; ok {
			continue
		}
		seen[v] = true
		elements[j] = v
		j++
	}
	return elements[:j]
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also abstracts out the actual lock names, which could introduce hard to debug issues

Copy link
Contributor Author

@krispetkov krispetkov Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I got your idea. I had prepared similar solution. I was trying to avoid the deletion/remove of duplicated strings and that's why I proposed my previous solution but maybe you are right and this is the right way. I have committed the new changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katbyte I know that you are busy working on v2.0.0 but just wanted to ask if I need to make some other changes or the last ones are OK? And do you think that this fix will be merged in the next v1.44? I'm asking because it is an essential bug for our team and currently blocking some of our tasks and I just want to have some more information.


ByName(uniqueValue, resourceType)
}
}

Expand All @@ -31,7 +38,10 @@ func UnlockByName(name string, resourceType string) {
}

func UnlockMultipleByName(names *[]string, resourceType string) {
for _, name := range *names {
UnlockByName(name, resourceType)
for i, name := range *names {
// at the end of every array item add its index. We need to add this sufix because it was added during the lock process
uniqueValue := fmt.Sprintf("%s-arrIdx-%d", name, i)
katbyte marked this conversation as resolved.
Show resolved Hide resolved

UnlockByName(uniqueValue, resourceType)
}
}