Skip to content

Commit

Permalink
libovsdbops: only one wait per txn
Browse files Browse the repository at this point in the history 
Transactions is an all or nothing thing so we just need one wait per
transaction to check against duplicates.

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
  • Loading branch information
@jcaamano
jcaamano committed May 3, 2022
1 parent b40ae90 commit 1aaad50
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 23 deletions.
32 changes: 24 additions & 8 deletions go-controller/pkg/libovsdbops/model_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ func (m *modelClient) CreateOrUpdateOps(ops []ovsdb.Operation, opModels ...opera
}

func (m *modelClient) createOrUpdateOps(ops []ovsdb.Operation, opModels ...operationModel) (interface{}, []ovsdb.Operation, error) {
hasGuardOp := len(ops) > 0 && isGuardOp(&ops[0])
guardOp := []ovsdb.Operation{}
doWhenFound := func(model interface{}, opModel *operationModel) ([]ovsdb.Operation, error) {
if opModel.OnModelUpdates != nil {
return m.update(model, opModel)
Expand All @@ -204,9 +206,25 @@ func (m *modelClient) createOrUpdateOps(ops []ovsdb.Operation, opModels ...opera
return nil, nil
}
doWhenNotFound := func(model interface{}, opModel *operationModel) ([]ovsdb.Operation, error) {
if !hasGuardOp {
// for the first insert of certain models, build a wait operation
// that checks for duplicates as a guard op to prevent against
// duplicate transactions
var err error
guardOp, err = buildFailOnDuplicateOps(m.client, opModel.Model)
if err != nil {
return nil, err
}
hasGuardOp = len(guardOp) > 0
}
return m.create(opModel)
}
return m.buildOps(ops, doWhenFound, doWhenNotFound, opModels...)
created, ops, err := m.buildOps(ops, doWhenFound, doWhenNotFound, opModels...)
if len(guardOp) > 0 {
// set the guard op as the first of the list
ops = append(guardOp, ops...)
}
return created, ops, err
}

/*
Expand Down Expand Up @@ -318,17 +336,11 @@ func (m *modelClient) create(opModel *operationModel) ([]ovsdb.Operation, error)
setUUID(opModel.Model, buildNamedUUID())
}

ops, err := buildFailOnDuplicateOps(m.client, opModel.Model)
ops, err := m.client.Create(opModel.Model)
if err != nil {
return nil, fmt.Errorf("unable to create model, err: %v", err)
}

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...)

klog.V(5).Infof("Create operations generated as: %+v", ops)
return ops, nil
}
Expand Down Expand Up @@ -468,3 +480,7 @@ func addToExistingResult(model interface{}, existingResult interface{}) error {

return nil
}

func isGuardOp(op *ovsdb.Operation) bool {
return op != nil && op.Op == ovsdb.OperationWait && op.Timeout != nil && *op.Timeout == types.OVSDBWaitTimeout
}
79 changes: 64 additions & 15 deletions go-controller/pkg/libovsdbops/model_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ var (
)

type OperationModelTestCase struct {
name string
op string
generateOp func() []operationModel
initialDB []libovsdbtest.TestData
expectedDB []libovsdbtest.TestData
expectedRes [][]libovsdbtest.TestData
expectedErr error
name string
op string
generateOp func() []operationModel
interleaveOp bool
initialDB []libovsdbtest.TestData
expectedDB []libovsdbtest.TestData
expectedRes [][]libovsdbtest.TestData
expectedOpsErr error
expectedTxnErr bool
}

func runTestCase(t *testing.T, tCase OperationModelTestCase) error {
Expand All @@ -52,19 +54,32 @@ func runTestCase(t *testing.T, tCase OperationModelTestCase) error {

opModels := tCase.generateOp()

ops := []ovsdb.Operation{}
switch tCase.op {
case "Lookup":
err = modelClient.Lookup(opModels...)
case "CreateOrUpdate":
_, err = modelClient.CreateOrUpdate(opModels...)
ops, err = modelClient.CreateOrUpdateOps(ops, opModels...)
case "Delete":
err = modelClient.Delete(opModels...)
ops, err = modelClient.DeleteOps(ops, opModels...)
default:
return fmt.Errorf("test \"%s\": unknown op %s", tCase.name, tCase.op)
}

if err != tCase.expectedErr {
return fmt.Errorf("test \"%s\": unexpected error generating %s operations, got %v, expected %v", tCase.name, tCase.op, err, tCase.expectedErr)
if err != tCase.expectedOpsErr {
return fmt.Errorf("test \"%s\": unexpected error generating %s operations, got %v, expected %v", tCase.name, tCase.op, err, tCase.expectedOpsErr)
}

if tCase.interleaveOp {
_, err = modelClient.CreateOrUpdate(opModels...)
if err != nil {
return fmt.Errorf("test \"%s\": unexpected error executing interleave operations: %v", tCase.name, err)
}
}

_, err = TransactAndCheck(nbClient, ops)
if err != nil && !tCase.expectedTxnErr {
return fmt.Errorf("test \"%s\": unexpected error transacting operations: %v", tCase.name, err)
}

var matcher types.GomegaMatcher
Expand Down Expand Up @@ -1152,7 +1167,7 @@ func TestLookup(t *testing.T) {
Addresses: []string{adressSetTestAdress},
},
},
expectedErr: client.ErrNotFound,
expectedOpsErr: client.ErrNotFound,
},
{
name: "Test lookup by index not found no error",
Expand Down Expand Up @@ -1257,7 +1272,7 @@ func TestLookup(t *testing.T) {
Addresses: []string{adressSetTestAdress},
},
},
expectedErr: client.ErrNotFound,
expectedOpsErr: client.ErrNotFound,
},
{
name: "Test lookup by predicate not found no error",
Expand Down Expand Up @@ -1303,7 +1318,7 @@ func TestLookup(t *testing.T) {
Addresses: []string{adressSetTestAdress},
},
},
expectedErr: client.ErrNotFound,
expectedOpsErr: client.ErrNotFound,
},
{
name: "Test lookup by predicate bulk op multiple results",
Expand Down Expand Up @@ -1367,7 +1382,7 @@ func TestLookup(t *testing.T) {
Addresses: []string{adressSetTestAdress + "-2"},
},
},
expectedErr: errMultipleResults,
expectedOpsErr: errMultipleResults,
},
}

Expand Down Expand Up @@ -1490,3 +1505,37 @@ func TestBuildMutationsFromFields(t *testing.T) {
}
}
}

func TestWaitForDuplicates(t *testing.T) {
tt := []OperationModelTestCase{
{
name: "Test non-root model transaction fails when duplicate",
op: "CreateOrUpdate",
generateOp: func() []operationModel {
return []operationModel{
{
Model: &nbdb.LogicalSwitch{
Name: logicalSwitchTestName,
},
},
}
},
interleaveOp: true,
initialDB: []libovsdbtest.TestData{},
expectedDB: []libovsdbtest.TestData{
&nbdb.LogicalSwitch{
UUID: logicalSwitchTestUUID,
Name: logicalSwitchTestName,
},
},
expectedTxnErr: true,
},
}

for _, tCase := range tt {
if err := runTestCase(t, tCase); err != nil {
t.Fatal(err)
}
}

}

0 comments on commit 1aaad50

Please sign in to comment.