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 28ce0d0
Show file tree
Hide file tree
Showing 21 changed files with 302 additions and 185 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
22 changes: 19 additions & 3 deletions client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ type Bridge struct {
DatapathVersion string `ovsdb:"datapath_version"`
ExternalIDs map[string]string `ovsdb:"external_ids"`
FailMode *BridgeFailMode `ovsdb:"fail_mode"`
FloodVLANs [4096]int `ovsdb:"flood_vlans"`
FloodVLANs []int `ovsdb:"flood_vlans"`
FlowTables map[int]string `ovsdb:"flow_tables"`
IPFIX *string `ovsdb:"ipfix"`
McastSnoopingEnable bool `ovsdb:"mcast_snooping_enable"`
Expand Down 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
Loading

0 comments on commit 28ce0d0

Please sign in to comment.