libovsdb: refactor duplicate detection in modelClient

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>

go-controller/pkg/libovsdbops/loadbalancer.go

@@ -66,7 +66,6 @@ func CreateOrUpdateLoadBalancersOps(nbClient libovsdbclient.Client, ops []libovs
 		// can't use i in the predicate, for loop replaces it in-memory
 		lb := lbs[i]
 		opModel := operationModel{
-			Name:           &lb.Name,
 			Model:          lb,
 			ModelPredicate: func(item *nbdb.LoadBalancer) bool { return item.Name == lb.Name },
 			OnModelUpdates: getNonZeroLoadBalancerMutableFields(lb),

go-controller/pkg/libovsdbops/model.go

@@ -4,9 +4,12 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/ovn-org/libovsdb/client"
 	"github.com/ovn-org/libovsdb/model"
+	"github.com/ovn-org/libovsdb/ovsdb"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/sbdb"
+	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
 )
 
 func getUUID(model model.Model) string {
@@ -309,3 +312,41 @@ func onModels(models interface{}, do func(interface{}) error) error {
 	}
 	return nil
 }
+
+// buildFailOnDuplicateOps builds a wait operation on a condition that will fail
+// if a duplicate to the provided model is considered to be found. We use this
+// to avoid duplicates on certain unknown scenarios that are still to be tracked
+// down. See: https://bugzilla.redhat.com/show_bug.cgi?id=2042001.
+// When no specific operation is required for the provided model, returns an empty
+// array for convenience.
+func buildFailOnDuplicateOps(c client.Client, m model.Model) ([]ovsdb.Operation, error) {
+	// Right now we only consider models with a "Name" field that is not an
+	// index for which we don't expect duplicate names.
+	// A duplicate Name field that is an index will fail without the
+	// need of this wait operation.
+	// Models that require a complex condition to detect duplicates are not
+	// considered for the time being due to the performance hit (i.e ACLs).
+	var field interface{}
+	var value string
+	switch t := m.(type) {
+	case *nbdb.LoadBalancer:
+		field = &t.Name
+		value = t.Name
+	case *nbdb.LogicalRouter:
+		field = &t.Name
+		value = t.Name
+	case *nbdb.LogicalSwitch:
+		field = &t.Name
+		value = t.Name
+	default:
+		return []ovsdb.Operation{}, nil
+	}
+
+	timeout := types.OVSDBWaitTimeout
+	cond := model.Condition{
+		Field:    field,
+		Function: ovsdb.ConditionEqual,
+		Value:    value,
+	}
+	return c.Where(m, cond).Wait(ovsdb.WaitConditionNotEqual, &timeout, m, field)
+}

go-controller/pkg/libovsdbops/model_client.go

@@ -116,12 +116,6 @@ type operationModel struct {
 	// DoAfter is invoked at the end of the operation and allows to setup a
 	// subsequent operation with values obtained from this one.
 	DoAfter func()
-	// Name is used to signify if this model has a name being used. Typically
-	// corresponds to Name field used on ovsdb objects. Using a non-empty
-	// Name indicates that during a Create the model will have a predicate
-	// operation to ensure a duplicate txn will not occur. See:
-	// https://bugzilla.redhat.com/show_bug.cgi?id=2042001
-	Name interface{}
 }
 
 func onModelUpdatesNone() []interface{} {
@@ -289,41 +283,18 @@ func (m *modelClient) create(opModel *operationModel) ([]ovsdb.Operation, error)
 	if uuid == "" {
 		setUUID(opModel.Model, buildNamedUUID())
 	}
-	ops := make([]ovsdb.Operation, 0, 1)
-	o, err := m.client.Create(opModel.Model)
+
+	ops, err := buildFailOnDuplicateOps(m.client, opModel.Model)
 	if err != nil {
 		return nil, fmt.Errorf("unable to create model, err: %v", err)
 	}
 
-	// Add wait methods accordingly
-	// ACL we would have to use external_ids + name for unique match
-	// However external_ids would be a performance hit, and in one case we use
-	// an empty name and external_ids for addAllowACLFromNode
-	if opModel.Name != nil && o[0].Table != "ACL" {
-		timeout := types.OVSDBWaitTimeout
-		condition := model.Condition{
-			Field:    opModel.Name,
-			Function: ovsdb.ConditionEqual,
-			Value:    getString(opModel.Name),
-		}
-		waitOps, err := m.client.Where(opModel.Model, condition).Wait(ovsdb.WaitConditionNotEqual, &timeout, opModel.Model, opModel.Name)
-		if err != nil {
-			return nil, err
-		}
-		ops = append(ops, waitOps...)
-	} else if cache := m.client.Cache(); cache != nil { // cache might be nil if disconnected
-		if info, err := cache.DatabaseModel().NewModelInfo(opModel.Model); err == nil {
-			if name, err := info.FieldByColumn("name"); err == nil {
-				objName := getString(name)
-				if len(objName) > 0 {
-					klog.Warningf("OVSDB Create operation detected without setting opModel Name. Name: %s, %#v",
-						objName, info)
-				}
-			}
-		}
+	op, err := m.client.Create(opModel.Model)
+	if err != nil {
+		return nil, fmt.Errorf("unable to create model, err: %v", err)
 	}
+	ops = append(ops, op...)
 
-	ops = append(ops, o...)
 	klog.V(5).Infof("Create operations generated as: %+v", ops)
 	return ops, nil
 }
@@ -463,15 +434,3 @@ func addToExistingResult(model interface{}, existingResult interface{}) error {
 
 	return nil
 }
-
-func getString(field interface{}) string {
-	objName, ok := field.(string)
-	if !ok {
-		if strPtr, ok := field.(*string); ok {
-			if strPtr != nil {
-				objName = *strPtr
-			}
-		}
-	}
-	return objName
-}

go-controller/pkg/libovsdbops/router.go

@@ -49,7 +49,6 @@ func FindLogicalRoutersWithPredicate(nbClient libovsdbclient.Client, p logicalRo
 // CreateOrUpdateLogicalRouter creates or updates the provided logical router
 func CreateOrUpdateLogicalRouter(nbClient libovsdbclient.Client, router *nbdb.LogicalRouter) error {
 	opModel := operationModel{
-		Name:           &router.Name,
 		Model:          router,
 		ModelPredicate: func(item *nbdb.LogicalRouter) bool { return item.Name == router.Name },
 		OnModelUpdates: onModelUpdatesAll(),

go-controller/pkg/libovsdbops/switch.go

@@ -48,7 +48,6 @@ func GetLogicalSwitch(nbClient libovsdbclient.Client, sw *nbdb.LogicalSwitch) (*
 // CreateOrUpdateLogicalRouter creates or updates the provided logical switch
 func CreateOrUpdateLogicalSwitch(nbClient libovsdbclient.Client, sw *nbdb.LogicalSwitch) error {
 	opModel := operationModel{
-		Name:           &sw.Name,
 		Model:          sw,
 		ModelPredicate: func(item *nbdb.LogicalSwitch) bool { return item.Name == sw.Name },
 		OnModelUpdates: onModelUpdatesAll(),
@@ -234,7 +233,6 @@ func createOrUpdateLogicalSwitchPortsOps(nbClient libovsdbclient.Client, ops []l
 		opModels = append(opModels, opModel)
 	}
 	opModel := operationModel{
-		Name:             &sw.Name,
 		Model:            sw,
 		ModelPredicate:   func(item *nbdb.LogicalSwitch) bool { return item.Name == sw.Name },
 		OnModelMutations: []interface{}{&sw.Ports},