libnet: Replace DeleteAtomic in retry loops with DeleteIdempotent

A common pattern in libnetwork is to delete an object using
`DeleteAtomic`, ie. to check the optimistic lock, but put in a retry
loop to refresh the data and the version index used by the optimistic
lock.

This commit introduces a new `Delete` method to delete without
checking the optimistic lock. It focuses only on the few places where
it's obvious the calling code doesn't rely on the side-effects of the
retry loop (ie. refreshing the object to be deleted).

Signed-off-by: Albin Kerouanton <albinker@gmail.com>

libnetwork/datastore/datastore.go

@@ -261,6 +261,29 @@ func (ds *Store) Map(key string, kvObject KVObject) (map[string]KVObject, error)
 	return results, nil
 }
 
+// DeleteObject deletes a kvObject from the on-disk DB and the in-memory cache.
+// Unlike DeleteObjectAtomic, it doesn't check the optimistic lock of the
+// passed kvObject.
+func (ds *Store) DeleteObject(kvObject KVObject) error {
+	ds.mu.Lock()
+	defer ds.mu.Unlock()
+
+	if kvObject == nil {
+		return types.InvalidParameterErrorf("invalid KV Object : nil")
+	}
+
+	if !kvObject.Skip() {
+		if err := ds.store.Delete(Key(kvObject.Key()...)); err != nil {
+			return err
+		}
+	}
+
+	// cleanup the cache only if AtomicDelete went through successfully
+	// If persistent store is skipped, sequencing needs to
+	// happen in cache.
+	return ds.cache.del(kvObject, false)
+}
+
 // DeleteObjectAtomic performs atomic delete on a record.
 func (ds *Store) DeleteObjectAtomic(kvObject KVObject) error {
 	ds.mu.Lock()

libnetwork/datastore/mockstore_test.go

@@ -88,6 +88,16 @@ func (s *MockStore) AtomicDelete(key string, previous *store.KVPair) error {
 	return nil
 }
 
+// Delete deletes a value at "key". Unlike AtomicDelete it doesn't check
+// whether the deleted key is at a specific version before deleting.
+func (s *MockStore) Delete(key string) error {
+	if _, ok := s.db[key]; !ok {
+		return store.ErrKeyNotFound
+	}
+	delete(s.db, key)
+	return nil
+}
+
 // Close closes the client connection
 func (s *MockStore) Close() {
 }

libnetwork/drivers/bridge/bridge_store.go

@@ -114,18 +114,7 @@ func (d *driver) storeDelete(kvObject datastore.KVObject) error {
 		return nil
 	}
 
-retry:
-	if err := d.store.DeleteObjectAtomic(kvObject); err != nil {
-		if err == datastore.ErrKeyModified {
-			if err := d.store.GetObject(kvObject); err != nil {
-				return fmt.Errorf("could not update the kvobject to latest when trying to delete: %v", err)
-			}
-			goto retry
-		}
-		return err
-	}
-
-	return nil
+	return d.store.DeleteObject(kvObject)
 }
 
 func (ncfg *networkConfiguration) MarshalJSON() ([]byte, error) {

libnetwork/drivers/ipvlan/ipvlan_store.go

@@ -129,18 +129,8 @@ func (d *driver) storeDelete(kvObject datastore.KVObject) error {
 		log.G(context.TODO()).Debugf("ipvlan store not initialized. kv object %s is not deleted from store", datastore.Key(kvObject.Key()...))
 		return nil
 	}
-retry:
-	if err := d.store.DeleteObjectAtomic(kvObject); err != nil {
-		if err == datastore.ErrKeyModified {
-			if err := d.store.GetObject(kvObject); err != nil {
-				return fmt.Errorf("could not update the kvobject to latest when trying to delete: %v", err)
-			}
-			goto retry
-		}
-		return err
-	}
 
-	return nil
+	return d.store.DeleteObject(kvObject)
 }
 
 func (config *configuration) MarshalJSON() ([]byte, error) {

libnetwork/drivers/macvlan/macvlan_store.go

@@ -128,18 +128,8 @@ func (d *driver) storeDelete(kvObject datastore.KVObject) error {
 		log.G(context.TODO()).Debugf("macvlan store not initialized. kv object %s is not deleted from store", datastore.Key(kvObject.Key()...))
 		return nil
 	}
-retry:
-	if err := d.store.DeleteObjectAtomic(kvObject); err != nil {
-		if err == datastore.ErrKeyModified {
-			if err := d.store.GetObject(kvObject); err != nil {
-				return fmt.Errorf("could not update the kvobject to latest when trying to delete: %v", err)
-			}
-			goto retry
-		}
-		return err
-	}
 
-	return nil
+	return d.store.DeleteObject(kvObject)
 }
 
 func (config *configuration) MarshalJSON() ([]byte, error) {

libnetwork/drivers/windows/windows_store.go

@@ -114,18 +114,7 @@ func (d *driver) storeDelete(kvObject datastore.KVObject) error {
 		return nil
 	}
 
-retry:
-	if err := d.store.DeleteObjectAtomic(kvObject); err != nil {
-		if err == datastore.ErrKeyModified {
-			if err := d.store.GetObject(kvObject); err != nil {
-				return fmt.Errorf("could not update the kvobject to latest when trying to delete: %v", err)
-			}
-			goto retry
-		}
-		return err
-	}
-
-	return nil
+	return d.store.DeleteObject(kvObject)
 }
 
 func (ncfg *networkConfiguration) MarshalJSON() ([]byte, error) {

libnetwork/internal/kvstore/boltdb/boltdb.go

@@ -180,6 +180,21 @@ func (b *BoltDB) AtomicDelete(key string, previous *store.KVPair) error {
 	})
 }
 
+// Delete deletes a value at "key". Unlike AtomicDelete it doesn't check
+// whether the deleted key is at a specific version before deleting.
+func (b *BoltDB) Delete(key string) error {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+
+	return b.client.Update(func(tx *bolt.Tx) error {
+		bucket := tx.Bucket(b.boltBucket)
+		if bucket == nil || bucket.Get([]byte(key)) == nil {
+			return store.ErrKeyNotFound
+		}
+		return bucket.Delete([]byte(key))
+	})
+}
+
 // AtomicPut puts a value at "key" if the key has not been
 // modified since the last Put, throws an error if this is the case
 func (b *BoltDB) AtomicPut(key string, value []byte, previous *store.KVPair) (*store.KVPair, error) {

libnetwork/internal/kvstore/kvstore.go

@@ -51,6 +51,10 @@ type Store interface {
 	// AtomicDelete performs an atomic delete of a single value.
 	AtomicDelete(key string, previous *KVPair) error
 
+	// Delete deletes a value at "key". Unlike AtomicDelete it doesn't check
+	// whether the deleted key is at a specific version before deleting.
+	Delete(key string) error
+
 	// Close the store connection
 	Close()
 }

libnetwork/sandbox_store.go

@@ -159,11 +159,15 @@ retry:
 }
 
 func (sb *Sandbox) storeDelete() error {
-	return sb.controller.deleteFromStore(&sbState{
+	cs := sb.controller.getStore()
+	if cs == nil {
+		return fmt.Errorf("datastore is not initialized")
+	}
+
+	return cs.DeleteObject(&sbState{
 		c:        sb.controller,
 		ID:       sb.id,
 		Cid:      sb.containerID,
-		dbIndex:  sb.dbIndex,
 		dbExists: sb.dbExists,
 	})
 }