Replace the global lock by the SubnetSet lock for Subnet creation (#931…

…) (#935)

Signed-off-by: Yanjun Zhou <[email protected]>

pkg/controllers/common/utils.go

@@ -23,14 +23,14 @@ import (
 )
 
 var (
-	log  = &logger.Log
-	lock = &sync.Mutex{}
+	log            = &logger.Log
+	SubnetSetLocks sync.Map
 )
 
 func AllocateSubnetFromSubnetSet(subnetSet *v1alpha1.SubnetSet, vpcService servicecommon.VPCServiceProvider, subnetService servicecommon.SubnetServiceProvider, subnetPortService servicecommon.SubnetPortServiceProvider) (string, error) {
-	// TODO: For now, this is a global lock. In the future, we need to narrow its scope down to improve the performance.
-	lock.Lock()
-	defer lock.Unlock()
+	// Use SubnetSet uuid lock to make sure when multiple ports are created on the same SubnetSet, only one Subnet will be created
+	lockSubnetSet(subnetSet.GetUID())
+	defer unlockSubnetSet(subnetSet.GetUID())
 	subnetList := subnetService.GetSubnetsByIndex(servicecommon.TagScopeSubnetSetCRUID, string(subnetSet.GetUID()))
 	for _, nsxSubnet := range subnetList {
 		portNums := len(subnetPortService.GetPortsOfSubnet(*nsxSubnet.Id))
@@ -240,3 +240,17 @@ func NewStatusUpdater(client k8sclient.Client, nsxConfig *config.NSXOperatorConf
 		ResourceType:    resourceType,
 	}
 }
+
+func lockSubnetSet(uuid types.UID) {
+	lock := sync.Mutex{}
+	subnetSetLock, _ := SubnetSetLocks.LoadOrStore(uuid, &lock)
+	log.V(1).Info("Lock SubnetSet", "uuid", uuid)
+	subnetSetLock.(*sync.Mutex).Lock()
+}
+
+func unlockSubnetSet(uuid types.UID) {
+	if subnetSetLock, existed := SubnetSetLocks.Load(uuid); existed {
+		log.V(1).Info("Unlock SubnetSet", "uuid", uuid)
+		subnetSetLock.(*sync.Mutex).Unlock()
+	}
+}

pkg/controllers/subnetset/subnetset_controller.go

@@ -12,6 +12,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -260,6 +261,15 @@ func (r *SubnetSetReconciler) CollectGarbage(ctx context.Context) {
 			r.StatusUpdater.IncreaseDeleteSuccessTotal()
 		}
 	}
+
+	// clean the SubnetSet lock used to create Subnet
+	common.SubnetSetLocks.Range(func(key, value interface{}) bool {
+		uuid := key.(types.UID)
+		if !crdSubnetSetIDsSet.Has(string(uuid)) {
+			common.SubnetSetLocks.Delete(key)
+		}
+		return true
+	})
 }
 
 func (r *SubnetSetReconciler) deleteSubnetBySubnetSetName(ctx context.Context, subnetSetName, ns string) error {

pkg/controllers/subnetset/subnetset_controller_test.go

@@ -6,10 +6,12 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/agiledragon/gomonkey/v2"
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model"
 	v12 "k8s.io/api/core/v1"
@@ -603,7 +605,15 @@ func TestSubnetSetReconciler_CollectGarbage(t *testing.T) {
 		}
 	})
 
+	// fake SubnetSetLocks
+	lock := sync.Mutex{}
+	subnetSetId := types.UID(uuid.NewString())
+	ctlcommon.SubnetSetLocks.LoadOrStore(subnetSetId, &lock)
+
 	r.CollectGarbage(ctx)
+	// the lock for should be deleted
+	_, ok := ctlcommon.SubnetSetLocks.Load(subnetSetId)
+	assert.False(t, ok)
 }
 
 type MockManager struct {