Merge pull request #1702 from rastislavszabo/master

Fix ID release logic in idalloc plugin

plugins/idalloc/idalloc.go

@@ -51,8 +51,9 @@ type IDAllocator struct {
 type Deps struct {
 	infra.PluginDeps
 
-	ContivConf contivconf.API
-	RemoteDB   nodesync.KVDBWithAtomic
+	ServiceLabel servicelabel.ReaderAPI
+	ContivConf   contivconf.API
+	RemoteDB     nodesync.KVDBWithAtomic
 }
 
 // poolMetadata contains metadata of a pool used for faster ID allocation.
@@ -153,7 +154,7 @@ func (a *IDAllocator) InitPool(name string, poolRange *idallocation.AllocationPo
 	pool := &idallocation.AllocationPool{
 		Name:          name,
 		Range:         poolRange,
-		IdAllocations: map[string]uint32{},
+		IdAllocations: map[string]*idallocation.AllocationPool_Allocation{},
 	}
 
 	// save the pool in db
@@ -252,7 +253,7 @@ func (a *IDAllocator) ReleaseID(poolName string, idLabel string) (err error) {
 	if poolMeta == nil {
 		return
 	}
-	id := pool.IdAllocations[idLabel]
+	alloc := pool.IdAllocations[idLabel]
 
 	succeeded := false
 	for i := 0; i < maxIDAllocationAttempts; i++ {
@@ -262,7 +263,7 @@ func (a *IDAllocator) ReleaseID(poolName string, idLabel string) (err error) {
 		}
 		if succeeded {
 			// successfully released an ID
-			delete(poolMeta.allocatedIDs, id)
+			delete(poolMeta.allocatedIDs, alloc.Id)
 			break
 		} else {
 			// pool changed in db, re-read from db and retry
@@ -281,7 +282,7 @@ func (a *IDAllocator) ReleaseID(poolName string, idLabel string) (err error) {
 		a.Log.Errorf("Error by releasing ID: %v", err)
 	}
 
-	a.Log.Debugf("Released ID for label '%s' in pool %s: %d", idLabel, poolName, id)
+	a.Log.Debugf("Released ID for label '%s' in pool %s: %d", idLabel, poolName, alloc.Id)
 
 	return nil
 }
@@ -291,8 +292,8 @@ func (a *IDAllocator) tryToAllocateID(pool *idallocation.AllocationPool, poolMet
 	id uint32, succeeded bool, err error) {
 
 	// step 0, try to get already allocated ID number
-	if id, exists := pool.IdAllocations[idLabel]; exists {
-		return id, true, nil
+	if alloc, exists := pool.IdAllocations[idLabel]; exists {
+		return alloc.Id, true, nil
 	}
 
 	// step 1, find a free ID number
@@ -316,7 +317,10 @@ func (a *IDAllocator) tryToAllocateID(pool *idallocation.AllocationPool, poolMet
 	if err != nil {
 		return 0, false, err
 	}
-	pool.IdAllocations[idLabel] = id
+	pool.IdAllocations[idLabel] = &idallocation.AllocationPool_Allocation{
+		Id:    id,
+		Owner: a.ServiceLabel.GetAgentLabel(),
+	}
 	newData, err := a.serializer.Marshal(pool)
 	if err != nil {
 		return 0, false, err
@@ -335,21 +339,33 @@ func (a *IDAllocator) tryToAllocateID(pool *idallocation.AllocationPool, poolMet
 func (a *IDAllocator) tryToReleaseID(pool *idallocation.AllocationPool, poolMeta *poolMetadata, idLabel string) (
 	succeeded bool, err error) {
 
-	// check if it is not already released
-	if _, exists := pool.IdAllocations[idLabel]; !exists {
+	alloc, exists := pool.IdAllocations[idLabel]
+	if !exists {
+		// already released
+		return true, nil
+	}
+	if alloc.Owner != a.ServiceLabel.GetAgentLabel() {
+		// we do not own this allocation, do not write into db
+		delete(pool.IdAllocations, idLabel)
 		return true, nil
 	}
 
-	// try to write into db
+	// marshal the old state
 	prevData, err := a.serializer.Marshal(pool)
 	if err != nil {
 		return false, err
 	}
+
+	// delete the allocation from cache
 	delete(pool.IdAllocations, idLabel)
+
+	// marshal the new state
 	newData, err := a.serializer.Marshal(pool)
 	if err != nil {
 		return false, err
 	}
+
+	// delete from db
 	db, err := a.getDBBroker()
 	if err != nil {
 		a.Log.Error(err)
@@ -392,8 +408,8 @@ func (a *IDAllocator) buildPoolMetadata(pool *idallocation.AllocationPool) *pool
 	for _, id := range pool.Range.Reserved {
 		meta.reservedIDs[id] = true
 	}
-	for label, id := range pool.IdAllocations {
-		meta.allocatedIDs[id] = label
+	for label, alloc := range pool.IdAllocations {
+		meta.allocatedIDs[alloc.Id] = label
 	}
 	return meta
 }

plugins/idalloc/idallocation/idallocation.proto

@@ -27,7 +27,12 @@ message AllocationPool {
     }
     Range range = 2; // specification of the pool range
 
+    message Allocation {
+        uint32 id = 1;    // allocated ID
+        string owner = 2; // owner of the allocation (only owner can release the allocation)
+    }
+
     // map of all allocations, key is the allocation "label"
-    // describing its purpose (e.g. nework name for vrf pool, etc.)
-    map<string, uint32> id_allocations = 3;
+    // describing its purpose (e.g. network name for vrf pool, etc.)
+    map<string, Allocation> id_allocations = 3;
 }

plugins/idalloc/options.go

@@ -17,6 +17,7 @@ package idalloc
 import (
 	"github.com/contiv/vpp/plugins/contivconf"
 	"github.com/ligato/cn-infra/logging"
+	"github.com/ligato/cn-infra/servicelabel"
 )
 
 // DefaultPlugin is a default instance of IDAlloc plugin.
@@ -28,6 +29,7 @@ func NewPlugin(opts ...Option) *IDAllocator {
 
 	p.PluginName = "idalloc"
 	p.ContivConf = &contivconf.DefaultPlugin
+	p.ServiceLabel = &servicelabel.DefaultPlugin
 
 	for _, o := range opts {
 		o(p)

plugins/ipnet/pod.go

@@ -260,6 +260,7 @@ func (n *IPNet) podCustomIfsConfig(pod *podmanager.LocalPod, eventType configEve
 		switch customIf.ifType {
 		case memifIfType:
 			// handle custom memif interface
+			memifID++
 			if memifInfo == nil {
 				memifInfo, err = n.DeviceManager.GetPodMemifInfo(pod.ID)
 				if err != nil || memifInfo == nil {
@@ -270,7 +271,6 @@ func (n *IPNet) podCustomIfsConfig(pod *podmanager.LocalPod, eventType configEve
 			// VPP side of the memif
 			k, v := n.podVPPMemif(pod, podIP, customIf.ifName, customIf.ifNet, memifInfo, memifID)
 			config[k] = v
-			memifID++
 
 		case tapIfType:
 			// handle custom tap interface