[wip] ovsdb: don't create sized arrays for OVS Sets

[dcbw]

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 dcbw@redhat.com

[coveralls]

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
Change from base Build 2968537322:	-0.3%
Covered Lines:	5050
Relevant Lines:	6763

---

💛 - Coveralls

[jcaamano]

I think we might want some integration test with the real ovsdb server with enum sets and non-enum sets.

[jcaamano]

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.

[dcbw]

Yeah, I don't think there's a good reason to keep them arrays.

[amorenoz]

Originally we used slices. Then we changed it to arrays, but I don't quite remember all the reasoning behind the change.

[dcbw]

I moved enums to slices as well; they'll still get length enforced because they're sets.

[jcaamano]

Do we need checks on a minSize as well?

[dcbw]

Follow-up: we decided that we don't actually need to check minSize right now because nothing uses a minSize that's > 1

[jcaamano]

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.

[jcaamano]

great!

[amorenoz]

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

[dcbw]

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

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]

• 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]

• 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]

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