Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dcbw
Copy link
Contributor

@dcbw dcbw commented Aug 31, 2022

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]

@dcbw dcbw force-pushed the sets-as-slice branch 3 times, most recently from 4457d31 to 28ce0d0 Compare September 1, 2022 02:42
@coveralls
Copy link

coveralls commented Sep 1, 2022

Pull Request Test Coverage Report for Build 3060832130

  • 153 of 242 (63.22%) changed or added relevant lines in 9 files are covered.
  • 9 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.3%) to 74.671%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mapper/info.go 10 14 71.43%
ovsdb/bindings.go 11 17 64.71%
ovsdb/updates2.go 8 26 30.77%
server/server.go 19 38 50.0%
server/transact.go 35 54 64.81%
ovsdb/set.go 64 87 73.56%
Files with Coverage Reduction New Missed Lines %
ovsdb/updates2.go 1 61.43%
server/server.go 1 65.45%
cache/cache.go 2 79.21%
ovsdb/bindings.go 2 78.96%
server/transact.go 3 67.71%
Totals Coverage Status
Change from base Build 2968537322: -0.3%
Covered Lines: 5050
Relevant Lines: 6763

💛 - Coveralls

Copy link
Collaborator

@jcaamano jcaamano left a 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.

Comment on lines 350 to 352
// 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))
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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
Comment on lines 106 to 108
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))
}
Copy link
Collaborator

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/set.go Outdated Show resolved Hide resolved
ovsdb/bindings.go Outdated Show resolved Hide resolved
Comment on lines 258 to 239
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

@amorenoz
Copy link
Collaborator

amorenoz commented Sep 8, 2022

Thanks @dcbw for this. I've been wanting to do it since #259

@dcbw dcbw force-pushed the sets-as-slice branch 3 times, most recently from 76e5895 to f71dfa8 Compare September 15, 2022 13:27
@dcbw
Copy link
Contributor Author

dcbw commented Sep 15, 2022

@jcaamano so we now have size checking in a couple of places:

  1. ovs-to-native, native-to-ovs
  2. SetField()

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.

@jcaamano
Copy link
Collaborator

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 OvsToNative, NativeToOvs and OvsSet can be used for things that should not be schema sized constrained in the same way as column values, like mutation or condition values. So I would just ignore these for size checks.

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:

  • GetRowData or whereabouts for the OVS to Native Path, which happens at the server and at the client processing server updates. While it is initially reasonable to think that doing size checks in client side of this path is not problematic, we might consider not doing so if a client and a server can run on different schema versions and an increase in size could be a backward compatible schema change. Perhaps we can have a flag to avoid doing it on the client
  • NewRow or SetField or whereabouts for the Native to OVS path which happens at the client and the server when sending updates to the client. If using SetField, we should take care that it is not used to incrementally update the value of a column as it is done in ApplyModifications as it can temporarily violate the size constrain:

    libovsdb/cache/cache.go

    Lines 1314 to 1344 in 999ca19

    for i := 0; i < nv.Len(); i++ {
    // search for match in base values
    baseValue, err := info.FieldByColumn(k)
    if err != nil {
    return modified, err
    }
    bv := reflect.ValueOf(baseValue)
    var found bool
    newVal := nv.Index(i).Interface()
    for j := 0; j < bv.Len(); j++ {
    if bv.Index(j).Interface() == newVal {
    // found a match, delete from slice
    found = true
    newValue := reflect.AppendSlice(bv.Slice(0, j), bv.Slice(j+1, bv.Len()))
    err = info.SetField(k, newValue.Interface())
    if err != nil {
    return modified, err
    }
    modified = true
    break
    }
    }
    if !found {
    newValue := reflect.Append(bv, nv.Index(i))
    err = info.SetField(k, newValue.Interface())
    if err != nil {
    return modified, err
    }
    modified = true
    }
    }

    Also we probably need to return proper ovsdb errors in transact rather than panicking.

Would it be reasonable to divide the PR in two:

  • change arrays to slices
  • implement size checks

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.

@dcbw
Copy link
Contributor Author

dcbw commented Sep 28, 2022

  • Also we probably need to return proper ovsdb errors in transact rather than panicking.

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

@jcaamano
Copy link
Collaborator

  • Also we probably need to return proper ovsdb errors in transact rather than panicking.

@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.
In the server side, we have transact.go that basically generates a set of updates (which we can validate). These updates (if validation is ok) are then committed to the server database (a operation that can't fail) and sent to the client.

@jcaamano
Copy link
Collaborator

What do you think about

Would it be reasonable to divide the PR in two:

* change arrays to slices
* implement size checks
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.

Implementing size checks can be assumed under this effort: #338 with actually something we are going to work in sooner rather than later

dcbw and others added 8 commits May 11, 2023 14:40
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 <[email protected]>
Co-authored-by: Dan Williams [email protected]
Which means we need to pass the expected element type into
the set creation functions.

Signed-off-by: Dan Williams <[email protected]>
Convert usage of OvsSet{} in most places to accessors. The
accessors now also enforce set length restrictions.

Signed-off-by: Dan Williams <[email protected]>
@dcbw dcbw changed the title ovsdb: don't create sized arrays for OVS Sets [wip] ovsdb: don't create sized arrays for OVS Sets May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants