-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wip] ovsdb: don't create sized arrays for OVS Sets #333
base: main
Are you sure you want to change the base?
Conversation
4457d31
to
28ce0d0
Compare
Pull Request Test Coverage Report for Build 3060832130
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want some integration test with the real ovsdb server with enum sets and non-enum sets.
modelgen/table.go
Outdated
// 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering:
- Why would it be any different with enums? The set is still a set.
- Why did we use arrays in the first place. Kinda only makes sense to me when
1 > min == max
and even then we can just use a slice and validate the size if needs be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think there's a good reason to keep them arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we used slices. Then we changed it to arrays, but I don't quite remember all the reasoning behind the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved enums to slices as well; they'll still get length enforced because they're sets.
ovsdb/set.go
Outdated
type OvsSet struct { | ||
GoSet []interface{} | ||
GoSet []interface{} | ||
maxSize int // <0 means unlimited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need checks on a minSize
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up: we decided that we don't actually need to check minSize right now because nothing uses a minSize that's > 1
ovsdb/set.go
Outdated
if o.maxSize >= 0 && len(o.GoSet) > o.maxSize { | ||
return nil, fmt.Errorf("OvsSet max size is %d but has %d elements", o.maxSize, len(o.GoSet)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering, given that
- when un-marshaling a set, max size is an assumed value
- there is nothing preventing a set being un-marshaled, modified and marshaled again and the max check being done on an assumed value instead of the proper schema value (which is not necessarily bad, but looking at ways it might work better).
if we should un-export GoSet
in OvsSet
to prevent changes on un-marshaled sets, and probably do the size check early on newSetRaw
.
And this might work better also if a min size check is required.
ovsdb/bindings.go
Outdated
var ovsSet OvsSet | ||
if column.TypeObj.Key.Type == TypeUUID { | ||
ovsSlice := []interface{}{} | ||
if _, ok := rawElem.([]string); ok { | ||
for _, v := range rawElem.([]string) { | ||
uuid := UUID{GoUUID: v} | ||
ovsSlice = append(ovsSlice, uuid) | ||
} | ||
} else if _, ok := rawElem.(*string); ok { | ||
v := rawElem.(*string) | ||
if v != nil { | ||
uuid := UUID{GoUUID: *v} | ||
ovsSlice = append(ovsSlice, uuid) | ||
} | ||
} else { | ||
return nil, fmt.Errorf("uuid slice was neither []string or *string") | ||
} | ||
ovsSet = OvsSet{GoSet: ovsSlice} | ||
|
||
} else { | ||
var err error | ||
ovsSet, err = NewOvsSet(rawElem) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
return ovsSet, nil | ||
return NewOvsSet(column, rawElem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
76e5895
to
f71dfa8
Compare
@jcaamano so we now have size checking in a couple of places:
Unmarshal of the OvsSet can't know the column maxSize, but whenever we unmarshall we're going to use that set to update our table cache, and that uses ovs-to-native (which is size-checked) and SetField (which is also size checked). So client/server will receive a Set as a row update, and then attempt to Merge/Update/etc that too-large row update into its existing row-cache, and then that will fail. eg, fail when applying to the cache, not fail-when-unmarshalling. |
I have the feeling that we are not implementing the size check in the correct place (and I probably misguided you here, sorry for that) and is a bit too spread over the place so as to ensure we are not making any mistake, or at least for me is difficult to track this down right now. So what I can see is that I hope that this size check can be contained in a single place for simplicity and for this right now I am looking at the Mapper methods:
Would it be reasonable to divide the PR in two:
I can take the second and you can get unblocked on the first. The cost is a server that misbehaves in a different way, but I don't think is working correctly as it is now. |
@jcaamano we can do that, it's not hard. But note that since the server doesn't actually implement transactions, if you have a problem halfway through an update, your cache is now inconsistent and we can't roll it back. |
Hmm in my eyes it does, but not sure if we are talking about the same thing or if I am missing something. |
What do you think about
Implementing size checks can be assumed under this effort: #338 with actually something we are going to work in sooner rather than later |
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: Jaime Caamaño Ruiz <jcaamano@redhat.com> Co-authored-by: Dan Williams dcbw@redhat.com
Signed-off-by: Dan Williams <dcbw@redhat.com>
Which means we need to pass the expected element type into the set creation functions. Signed-off-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Dan Williams <dcbw@redhat.com>
Convert usage of OvsSet{} in most places to accessors. The accessors now also enforce set length restrictions. Signed-off-by: Dan Williams <dcbw@redhat.com>
Signed-off-by: Dan Williams <dcbw@redhat.com>
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:
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 dcbw@redhat.com