Skip to content

Commit

Permalink
ovsdb: don't create sized arrays for OVS Sets
Browse files Browse the repository at this point in the history 
OVS Sets must be unique, but modelgen created sized arrays
to represent OVS Sets, which when marshalled are filled with
duplicate default values.

For a column schema like:

   "cvlans": {
     "type": {"key": {"type": "integer",
                      "minInteger": 0,
                      "maxInteger": 4095},
              "min": 0, "max": 4096}},

modelgen would create:

   CVLANs [4096]int

which when marshalled and sent to ovsdb-server becomes:

cvlans:{GoSet:[0 0 0 0 0 0 0 <repeat many times>]}

and is rejected by ovsdb-server with:

{Count:0 Error:ovsdb error Details:set contains duplicate UUID:{GoUUID:} Rows:[]}] and errors [ovsdb error: set contains duplicate]: 1 ovsdb operations failed

Instead, generate these fields as slices instead of sized
arrays and enforce the size when marshalling the set.

Signed-off-by: Dan Williams <[email protected]>
  • Loading branch information
@dcbw
dcbw committed Sep 1, 2022
1 parent e4d2d86 commit 4457d31
Show file tree
Hide file tree
Showing 21 changed files with 298 additions and 177 deletions.
35 changes: 27 additions & 8 deletions cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1951,6 +1951,24 @@ func BenchmarkIndexExists(b *testing.B) {
}
}

func testOvsSet(t assert.TestingT, keyType string, set interface{}) ovsdb.OvsSet {
colSchema := fmt.Sprintf(`{
"type": {
"key": {"type": "%s"},
"min": 0,
"max": "unlimited"
}
}`, keyType)

var column ovsdb.ColumnSchema
err := json.Unmarshal([]byte(colSchema), &column)
assert.NoError(t, err)

oSet, err := ovsdb.NewOvsSet(&column, set)
assert.NoError(t, err)
return oSet
}

func BenchmarkPopulate2UpdateArray(b *testing.B) {
const numRows int = 500

Expand All @@ -1977,7 +1995,8 @@ func BenchmarkPopulate2UpdateArray(b *testing.B) {
b.ResetTimer()
for n := 0; n < b.N; n++ {
for i := 0; i < numRows; i++ {
updatedRow := ovsdb.Row(map[string]interface{}{"array": ovsdb.OvsSet{GoSet: updateSet}})
ovsSet := testOvsSet(b, ovsdb.TypeString, updateSet)
updatedRow := ovsdb.Row(map[string]interface{}{"array": ovsSet})
err := tc.Populate2(ovsdb.TableUpdates2{
"Open_vSwitch": {
"foo": &ovsdb.RowUpdate2{
Expand All @@ -1995,7 +2014,7 @@ func BenchmarkTableCacheApplyModificationsSet(b *testing.B) {
UUID string `ovsdb:"_uuid"`
Set []string `ovsdb:"set"`
}
aFooSet, _ := ovsdb.NewOvsSet([]string{"foo"})
aFooSet := testOvsSet(b, ovsdb.TypeString, []string{"foo"})
base := &testDBModel{Set: []string{}}
for i := 0; i < 57000; i++ {
base.Set = append(base.Set, fmt.Sprintf("foo%d", i))
Expand Down Expand Up @@ -2119,20 +2138,20 @@ func TestTableCacheApplyModifications(t *testing.T) {
Ptr *string `ovsdb:"ptr"`
Ptr2 *string `ovsdb:"ptr2"`
}
aEmptySet, _ := ovsdb.NewOvsSet([]string{})
aFooSet, _ := ovsdb.NewOvsSet([]string{"foo"})
aFooBarSet, _ := ovsdb.NewOvsSet([]string{"foo", "bar"})
aFooBarBazQuxSet, _ := ovsdb.NewOvsSet([]string{"foo", "bar", "baz", "qux"})
aEmptySet := testOvsSet(t, ovsdb.TypeString, []string{})
aFooSet := testOvsSet(t, ovsdb.TypeString, []string{"foo"})
aFooBarSet := testOvsSet(t, ovsdb.TypeString, []string{"foo", "bar"})
aFooBarBazQuxSet := testOvsSet(t, ovsdb.TypeString, []string{"foo", "bar", "baz", "qux"})
aFooMap, _ := ovsdb.NewOvsMap(map[string]string{"foo": "bar"})
aBarMap, _ := ovsdb.NewOvsMap(map[string]string{"bar": "baz"})
aBarBazMap, _ := ovsdb.NewOvsMap(map[string]string{
"bar": "baz",
"baz": "quux",
})
wallace := "wallace"
aWallaceSet, _ := ovsdb.NewOvsSet([]string{wallace})
aWallaceSet := testOvsSet(t, ovsdb.TypeString, []string{wallace})
gromit := "gromit"
aWallaceGromitSet, _ := ovsdb.NewOvsSet([]string{wallace, gromit})
aWallaceGromitSet := testOvsSet(t, ovsdb.TypeString, []string{wallace, gromit})
tests := []struct {
name string
update ovsdb.Row
Expand Down
25 changes: 13 additions & 12 deletions client/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@ func TestAPIMutate(t *testing.T) {
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, []string{"1.1.1.1"})}},
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"1.1.1.1"})}},
Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID0}}},
},
},
Expand All @@ -940,19 +940,19 @@ func TestAPIMutate(t *testing.T) {
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, []string{"2.2.2.2"})}},
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"2.2.2.2"})}},
Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID0}}},
},
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, []string{"2.2.2.2"})}},
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"2.2.2.2"})}},
Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID1}}},
},
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, []string{"2.2.2.2"})}},
Mutations: []ovsdb.Mutation{{Column: "addresses", Mutator: ovsdb.MutateOperationInsert, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"2.2.2.2"})}},
Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID2}}},
},
},
Expand All @@ -976,7 +976,7 @@ func TestAPIMutate(t *testing.T) {
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "external_ids", Mutator: ovsdb.MutateOperationDelete, Value: testOvsSet(t, []string{"foo"})}},
Mutations: []ovsdb.Mutation{{Column: "external_ids", Mutator: ovsdb.MutateOperationDelete, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"foo"})}},
Where: []ovsdb.Condition{{Column: "_uuid", Function: ovsdb.ConditionEqual, Value: ovsdb.UUID{GoUUID: aUUID2}}},
},
},
Expand All @@ -1000,7 +1000,7 @@ func TestAPIMutate(t *testing.T) {
{
Op: ovsdb.OperationMutate,
Table: "Logical_Switch_Port",
Mutations: []ovsdb.Mutation{{Column: "external_ids", Mutator: ovsdb.MutateOperationDelete, Value: testOvsSet(t, []string{"foo"})}},
Mutations: []ovsdb.Mutation{{Column: "external_ids", Mutator: ovsdb.MutateOperationDelete, Value: testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"foo"})}},
Where: []ovsdb.Condition{{Column: "name", Function: ovsdb.ConditionEqual, Value: "foo"}},
},
},
Expand Down Expand Up @@ -1136,10 +1136,10 @@ func TestAPIUpdate(t *testing.T) {
tcache := apiTestCache(t, testData)

testObj := testLogicalSwitchPort{}
testRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, []int{6})})
tagRow := ovsdb.Row(map[string]interface{}{"tag": testOvsSet(t, []int{6})})
testRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, ovsdb.TypeInteger, 1, []int{6})})
tagRow := ovsdb.Row(map[string]interface{}{"tag": testOvsSet(t, ovsdb.TypeInteger, 1, []int{6})})
var nilInt *int
testNilRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, nilInt)})
testNilRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse", "tag": testOvsSet(t, ovsdb.TypeInteger, 1, nilInt)})
typeRow := ovsdb.Row(map[string]interface{}{"type": "somethingElse"})
fields := []interface{}{&testObj.Tag, &testObj.Type}

Expand Down Expand Up @@ -1350,7 +1350,7 @@ func TestAPIUpdate(t *testing.T) {
Row: tagRow,
Where: []ovsdb.Condition{
{Column: "type", Function: ovsdb.ConditionEqual, Value: "sometype"},
{Column: "enabled", Function: ovsdb.ConditionIncludes, Value: testOvsSet(t, &trueVal)},
{Column: "enabled", Function: ovsdb.ConditionIncludes, Value: testOvsSet(t, ovsdb.TypeBoolean, 1, &trueVal)},
},
},
},
Expand Down Expand Up @@ -1403,7 +1403,7 @@ func TestAPIUpdate(t *testing.T) {
Op: ovsdb.OperationUpdate,
Table: "Logical_Switch_Port",
Row: tagRow,
Where: []ovsdb.Condition{{Column: "tag", Function: ovsdb.ConditionNotEqual, Value: testOvsSet(t, &one)}},
Where: []ovsdb.Condition{{Column: "tag", Function: ovsdb.ConditionNotEqual, Value: testOvsSet(t, ovsdb.TypeInteger, 1, &one)}},
},
},
err: false,
Expand Down Expand Up @@ -1843,6 +1843,7 @@ func TestAPIWait(t *testing.T) {
tcache := apiTestCache(t, cache.Data{})
timeout0 := 0

trueSet := testOvsSet(t, ovsdb.TypeBoolean, 1, []interface{}{true})
test := []struct {
name string
condition func(API) ConditionalAPI
Expand Down Expand Up @@ -1944,7 +1945,7 @@ func TestAPIWait(t *testing.T) {
{
Column: "up",
Function: ovsdb.ConditionNotEqual,
Value: ovsdb.OvsSet{GoSet: []interface{}{true}},
Value: trueSet,
},
},
Until: string(ovsdb.WaitConditionNotEqual),
Expand Down
20 changes: 18 additions & 2 deletions client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,24 @@ var schema = `{
}
}`

func testOvsSet(t *testing.T, set interface{}) ovsdb.OvsSet {
oSet, err := ovsdb.NewOvsSet(set)
func testOvsSet(t *testing.T, keyType string, maxSize int, set interface{}) ovsdb.OvsSet {
maxStr := `"unlimited"`
if maxSize > 0 {
maxStr = fmt.Sprintf("%d", maxSize)
}
colSchema := fmt.Sprintf(`{
"type": {
"key": {"type": "%s"},
"min": 0,
"max": %s
}
}`, keyType, maxStr)

var column ovsdb.ColumnSchema
err := json.Unmarshal([]byte(colSchema), &column)
assert.Nil(t, err)

oSet, err := ovsdb.NewOvsSet(&column, set)
assert.Nil(t, err)
return oSet
}
Expand Down
6 changes: 3 additions & 3 deletions client/condition_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ func TestExplicitConditionalWithNoCache(t *testing.T) {
{
Column: "enabled",
Function: ovsdb.ConditionEqual,
Value: testOvsSet(t, &trueVal),
Value: testOvsSet(t, ovsdb.TypeBoolean, 1, &trueVal),
}}},
},
{
Expand All @@ -369,7 +369,7 @@ func TestExplicitConditionalWithNoCache(t *testing.T) {
{
Column: "enabled",
Function: ovsdb.ConditionEqual,
Value: testOvsSet(t, &trueVal),
Value: testOvsSet(t, ovsdb.TypeBoolean, 1, &trueVal),
}},
{
{
Expand All @@ -396,7 +396,7 @@ func TestExplicitConditionalWithNoCache(t *testing.T) {
{
Column: "enabled",
Function: ovsdb.ConditionEqual,
Value: testOvsSet(t, &trueVal),
Value: testOvsSet(t, ovsdb.TypeBoolean, 1, &trueVal),
},
{
Column: "name",
Expand Down
2 changes: 1 addition & 1 deletion mapper/mapper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (m Mapper) NewMutation(data *Info, column string, mutator ovsdb.Mutator, va
// keys (rfc7047 5.1). Handle this special case here.
if mutator == "delete" && columnSchema.Type == ovsdb.TypeMap && reflect.TypeOf(value).Kind() != reflect.Map {
// It's OK to cast the value to a list of elements because validation has passed
ovsSet, err := ovsdb.NewOvsSet(value)
ovsSet, err := ovsdb.NewOvsSet(columnSchema, value)
if err != nil {
return nil, err
}
Expand Down
48 changes: 32 additions & 16 deletions mapper/mapper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,25 +157,25 @@ var testSchema = []byte(`{
func getOvsTestRow(t *testing.T) ovsdb.Row {
ovsRow := ovsdb.NewRow()
ovsRow["aString"] = aString
ovsRow["aSet"] = testOvsSet(t, aSet)
ovsRow["aSet"] = testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, aSet)
// Set's can hold the value if they have len == 1
ovsRow["aSingleSet"] = aString

us := make([]ovsdb.UUID, 0)
for _, u := range aUUIDSet {
us = append(us, ovsdb.UUID{GoUUID: u})
}
ovsRow["aUUIDSet"] = testOvsSet(t, us)
ovsRow["aUUIDSet"] = testOvsSet(t, ovsdb.TypeUUID, ovsdb.Unlimited, us)

ovsRow["aUUID"] = ovsdb.UUID{GoUUID: aUUID0}

ovsRow["aIntSet"] = testOvsSet(t, aIntSet)
ovsRow["aIntSet"] = testOvsSet(t, ovsdb.TypeInteger, ovsdb.Unlimited, aIntSet)

ovsRow["aFloat"] = aFloat

ovsRow["aFloatSet"] = testOvsSet(t, aFloatSet)
ovsRow["aFloatSet"] = testOvsSet(t, ovsdb.TypeReal, 10, aFloatSet)

ovsRow["aEmptySet"] = testOvsSet(t, []string{})
ovsRow["aEmptySet"] = testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{})

ovsRow["aEnum"] = aEnum

Expand Down Expand Up @@ -265,7 +265,7 @@ func TestMapperNewRow(t *testing.T) {
}{
SomeSet: aSet,
},
expectedRow: ovsdb.Row(map[string]interface{}{"aSet": testOvsSet(t, aSet)}),
expectedRow: ovsdb.Row(map[string]interface{}{"aSet": testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, aSet)}),
}, {
name: "emptySet with no column specification",
objInput: &struct {
Expand All @@ -289,15 +289,15 @@ func TestMapperNewRow(t *testing.T) {
}{
MyUUIDSet: []string{aUUID0, aUUID1},
},
expectedRow: ovsdb.Row(map[string]interface{}{"aUUIDSet": testOvsSet(t, []ovsdb.UUID{{GoUUID: aUUID0}, {GoUUID: aUUID1}})}),
expectedRow: ovsdb.Row(map[string]interface{}{"aUUIDSet": testOvsSet(t, ovsdb.TypeUUID, ovsdb.Unlimited, []ovsdb.UUID{{GoUUID: aUUID0}, {GoUUID: aUUID1}})}),
}, {
name: "aIntSet",
objInput: &struct {
MyIntSet []int `ovsdb:"aIntSet"`
}{
MyIntSet: []int{0, 42},
},
expectedRow: ovsdb.Row(map[string]interface{}{"aIntSet": testOvsSet(t, []int{0, 42})}),
expectedRow: ovsdb.Row(map[string]interface{}{"aIntSet": testOvsSet(t, ovsdb.TypeInteger, ovsdb.Unlimited, []int{0, 42})}),
}, {
name: "aFloat",
objInput: &struct {
Expand All @@ -313,7 +313,7 @@ func TestMapperNewRow(t *testing.T) {
}{
MyFloatSet: aFloatSet,
},
expectedRow: ovsdb.Row(map[string]interface{}{"aFloatSet": testOvsSet(t, aFloatSet)}),
expectedRow: ovsdb.Row(map[string]interface{}{"aFloatSet": testOvsSet(t, ovsdb.TypeReal, 10, aFloatSet)}),
}, {
name: "Enum",
objInput: &struct {
Expand Down Expand Up @@ -401,7 +401,7 @@ func TestMapperNewRowFields(t *testing.T) {
prepare: func(o *obj) {
},
fields: []interface{}{&testObj.MySet},
expectedRow: ovsdb.Row(map[string]interface{}{"aSet": testOvsSet(t, []string{})}),
expectedRow: ovsdb.Row(map[string]interface{}{"aSet": testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{})}),
}, {
name: "empty maps",
prepare: func(o *obj) {
Expand All @@ -424,7 +424,7 @@ func TestMapperNewRowFields(t *testing.T) {
o.MyFloat = aFloat
},
fields: []interface{}{&testObj.MyMap, &testObj.MySet},
expectedRow: ovsdb.Row(map[string]interface{}{"aMap": testOvsMap(t, aMap), "aSet": testOvsSet(t, aSet)}),
expectedRow: ovsdb.Row(map[string]interface{}{"aMap": testOvsMap(t, aMap), "aSet": testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, aSet)}),
},
}

Expand Down Expand Up @@ -989,7 +989,7 @@ func TestMapperMutation(t *testing.T) {
obj: testType{},
mutator: ovsdb.MutateOperationInsert,
value: []string{"foo"},
expected: ovsdb.NewMutation("set", ovsdb.MutateOperationInsert, testOvsSet(t, []string{"foo"})),
expected: ovsdb.NewMutation("set", ovsdb.MutateOperationInsert, testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"foo"})),
err: false,
},
{
Expand All @@ -998,7 +998,7 @@ func TestMapperMutation(t *testing.T) {
obj: testType{},
mutator: ovsdb.MutateOperationDelete,
value: []string{"foo"},
expected: ovsdb.NewMutation("set", ovsdb.MutateOperationDelete, testOvsSet(t, []string{"foo"})),
expected: ovsdb.NewMutation("set", ovsdb.MutateOperationDelete, testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"foo"})),
err: false,
},
{
Expand All @@ -1007,7 +1007,7 @@ func TestMapperMutation(t *testing.T) {
obj: testType{},
mutator: ovsdb.MutateOperationDelete,
value: []string{"foo", "bar"},
expected: ovsdb.NewMutation("map", ovsdb.MutateOperationDelete, testOvsSet(t, []string{"foo", "bar"})),
expected: ovsdb.NewMutation("map", ovsdb.MutateOperationDelete, testOvsSet(t, ovsdb.TypeString, ovsdb.Unlimited, []string{"foo", "bar"})),
err: false,
},
{
Expand Down Expand Up @@ -1050,8 +1050,24 @@ func TestMapperMutation(t *testing.T) {
}
}

func testOvsSet(t *testing.T, set interface{}) ovsdb.OvsSet {
oSet, err := ovsdb.NewOvsSet(set)
func testOvsSet(t *testing.T, keyType string, maxSize int, set interface{}) ovsdb.OvsSet {
maxStr := `"unlimited"`
if maxSize > 0 {
maxStr = fmt.Sprintf("%d", maxSize)
}
colSchema := fmt.Sprintf(`{
"type": {
"key": {"type": "%s"},
"min": 0,
"max": %s
}
}`, keyType, maxStr)

var column ovsdb.ColumnSchema
err := json.Unmarshal([]byte(colSchema), &column)
assert.Nil(t, err)

oSet, err := ovsdb.NewOvsSet(&column, set)
assert.Nil(t, err)
return oSet
}
Expand Down
9 changes: 3 additions & 6 deletions modelgen/table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,12 +347,9 @@ func fieldType(tableName, columnName string, column *ovsdb.ColumnSchema, enumTyp
}
return AtomicType(column.TypeObj.Key.Type)
}
// use array for columns with max > 1
if column.TypeObj.Max() > 1 {
if enumTypes && FieldEnum(tableName, columnName, column) != nil {
return fmt.Sprintf("[%d]%s", column.TypeObj.Max(), enumName(tableName, columnName))
}
return fmt.Sprintf("[%d]%s", column.TypeObj.Max(), AtomicType(column.TypeObj.Key.Type))
// use array for enums with max > 1
if column.TypeObj.Max() > 1 && enumTypes && FieldEnum(tableName, columnName, column) != nil {
return fmt.Sprintf("[%d]%s", column.TypeObj.Max(), enumName(tableName, columnName))
}
// use a slice
if enumTypes && FieldEnum(tableName, columnName, column) != nil {
Expand Down
2 changes: 1 addition & 1 deletion modelgen/table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@ func buildTestBridge() *vswitchd.Bridge {
DatapathVersion: *buildRandStr(),
ExternalIDs: map[string]string{*buildRandStr(): *buildRandStr(), *buildRandStr(): *buildRandStr()},
FailMode: &vswitchd.BridgeFailModeSecure,
FloodVLANs: [4096]int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
FloodVLANs: []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
FlowTables: map[int]string{1: *buildRandStr(), 2: *buildRandStr()},
IPFIX: buildRandStr(),
McastSnoopingEnable: false,
Expand Down
Loading

0 comments on commit 4457d31

Please sign in to comment.