Add native bindings and API

[amorenoz]

Following the idea described in #29, this PR tries to add schema-based support for native bindings in libovsdb.

Why is this desired?

• Because currently the users of libovsdb have to write a lot of code to translate the incoming information, that contains internal types such as OvsSet, OvsMapandOvsUUID`, to native golang types.
• Because currently the users of libovsdb have to deal with the little quirks in the RFC. For instance, you might expect a set of strings, however, the RFC states that you could get a single string if the set contains exactly one element. Thus you end up with a lot of boilerplate code such as this.
• Because currently there is no way to perform any (or very limited) degree of type checking before sending the RPC call.

Why schema-based?

• Because it allows you to really expose a clean API. Without it, how would you distinguish if a string is a UUID or not?
• Because without it you cannot dynamically infer the type of an empty slice.
• Because we can provide type verification based on the schema.

How does the API look now?

For instance, when you get the TableUpdates, after performing the transformations of the Row, you still have a map[string] interface{}. However, the client does not have to do any type assertions. If column foo holds a list of uuids, you can safely cast it result["foo"].([]string) and use it. No more GoMap or GoSets either.

An example of how to use it can be seen in the examples/play_with_ovs
As another example, I have adapted go-ovn/logical_router.go to use this API here. It reduces ~50 LOC. Many more could be reduced if the rest of the library is updated and if the ovs->native translation is performed before adding the objects to the cache.

What's next?

Based on this PR, we can very easily add dynamic ORM-like API based on struct tags (PoC to come) as described in #29

Pushing this PR early on to get feedback on the approach and implementation.
It is based on #28

[vtolstov]

looks good, but i have some questions and suggestions

[vtolstov]

i'm comment only on this pr and not on #28 because this pr contain full picture how the overall code works.
@hzhou8 may be we need to drop #28 and accumulate all changes in this pr? do you have time to check this pr ?

[vtolstov]

please fix conflict with client.go

[vtolstov]

after resolving conflict for LGTM, but i think that we need to have fresh look of @hzhou8 and @noah8713

[hzhou8]

Thanks @amorenoz @girishmg @vtolstov for working on this. Sorry for being slow on reviews. This one is big (and important) but I am busy on something else this month. I will get some time to review it in detail in March. I hope this is ok and sorry for the delays.

[noah8713]

Thanks @vtolstov for major review and looping us.
thanks @amorenoz for the big change and detailed commit message explaining the need. +1 as overall looks good from functionality point with minor comments. Please do run go fmt once as I saw on couple places where it might have been missed.

[amorenoz]

@noah8713 thanks for the comments and review.
Maybe you're also interested in taking a look at #31? I'm not asking for a review (although I would appreciate it ;)), but for your opinion on the general idea of using an object-based relational model to access the DB.

[vtolstov]

As I'm understand now its ready for merge? @noah8713 ?

[noah8713]

Thanks @amorenoz for addressing the comments.

LGTM from my side @vtolstov .

Since this is a big change @hzhou8 if you have no other pointers, feel free to merge

[hzhou8]

Thanks @vtolstov @noah8713 for the review. Sorry for the delay but I am still reviewing. Please give me some time.

[hzhou8]

I am still reviewing and submit the comments I have so far. Please also fold my comments from #28 if relevant.

In addition, each commit should be independently correct and tested. I'd suggest to squash the unit tests to the commits they belong to.

[vtolstov]

@hzhou8 why you prefer panic and not error?
I dont like panic stack traces because it looks weird in ci systems that wants structured logging

[hzhou8]

@hzhou8 why you prefer panic and not error?
I dont like panic stack traces because it looks weird in ci systems that wants structured logging

It depends on whether we are doing error handling or assertion. For my understanding panic is the way we do assertion in Go (Please correct me if I am wrong). In the above switch-case scenario it is determined by code in this library itself that the "default" branch is impossible, so "assertion" should be used instead of error handling.

[vtolstov]

You right, but as i say panic is harder to catch and convert to structure log, so I'm prefer to avoid it.

[hzhou8]

You right, but as i say panic is harder to catch and convert to structure log, so I'm prefer to avoid it.

In this case you don't need to catch/convert. It is not about error handling. It just logically shouldn't happen. So if it happens we don't really want to do error handling, and assert/panic is for this situation. A more extreme example is:

switch <a boolean var>: {
case true:
...
case false:
...
default:
return fmt.Errorf("unexpected value of boolean")
}

In this example, the error return is completely unnecessary.

In our case, the function nativeTypeFromExtended() doesn't need to return any error, which also saves the unnecessary burden for the caller to do error handling.

Could you propose an alternative if panic is not preferred?

[vtolstov]

You right, but as i say panic is harder to catch and convert to structure log, so I'm prefer to avoid it.

In this case you don't need to catch/convert. It is not about error handling. It just logically shouldn't happen. So if it happens we don't really want to do error handling, and assert/panic is for this situation. A more extreme example is:

switch : {
case true:
...
case false:
...
default:
return fmt.Errorf("unexpected value of boolean")
}

In this example, the error return is completely unnecessary.

In our case, the function nativeTypeFromExtended() doesn't need to return any error, which also saves the unnecessary burden for the caller to do error handling.

Could you propose an alternative if panic is not preferred?

In case of you extreme example - i can't =)
But from user-programmer perspective:

1. i'm write microservice that works with ovn
2. i'm init logger , some other stuff and connect to ovn
3. run it in under kubernetes/nomad/etc... and all logs passed to elastic
4. after some time/after some upgrades my services panics and i don't have normal logs in my logging system because it panics
5. more extreme example if i'm run commands inside goroutine and panic must be recovered inside goroutine...

so from this point of view i'm prefer not have panic inside library code because you must specify for all methods that can be panic info about it. And users of the library must always use recovery to display some useful info about it.

[hzhou8]

In case of you extreme example - i can't =)
But from user-programmer perspective:

1. i'm write microservice that works with ovn

2. i'm init logger , some other stuff and connect to ovn

3. run it in under kubernetes/nomad/etc... and all logs passed to elastic

4. after some time/after some upgrades my services panics and i don't have normal logs in my logging system because it panics

5. more extreme example if i'm run commands inside goroutine and panic must be recovered inside goroutine...

so from this point of view i'm prefer not have panic inside library code because you must specify for all methods that can be panic info about it. And users of the library must always use recovery to display some useful info about it.

@vtolstov thanks for sharing your use case. Your concern is valid, but I think the assumption of "no panic inside library" is unrealistic. Even if library code doesn't call panic, it can still panic due to runtime errors such as "index out of range". Of course one can argue that the code should ensure "out of range" and other runtime error never happens. Well, this is true as long as we write the code correctly. I think same principle just applies to the context here - we should ensure nativeTypeFromExtended() handles all atomic types and ensuring the callers of it never pass in an undefined atomic type. This is pretty straightforwardly doable because they are all internal functions.

1. If we know it is possible that the callers could have an undefined type passed in, we should fix it now. (just like spotting a "out of range" problem in the code and fix it).
2. If we confirmed it won't happen, the default branch is never going to be hit. (And if it is hit somehow, let it panic, just like when "index out of range" happens)

And now in fact I am surprised that we are in case 1), and I added a comment here: https://github.com/eBay/libovsdb/pull/30/files/7d3749a59bf564e0c452c754500aae683c64dccb#r600761873

With this comment addressed, we are in case 2), and we don't need to do error handling at: e235c30#diff-e29a67e14ba2eeb4cd617c06383912da10f348f70e1643c6189b2f1a20d02eb1R68

@amorenoz what do you think?

[hzhou8]

Hi @amorenoz I have put all my comments for this PR. This looks great overall. Please let me know when you address the comments. I am reviewing #31 , which get even more interesting.

[amorenoz]

Hi @hzhou8, thanks for the comments and review. I'll address them right away. Note #31 was based on this PR but I have not rebased it recently, I'll do it once I address the comments.

[amorenoz]

Latest update (to help focus the reviews):

• schema.go:
  • added more tests (schema_tests.go)
  • ensured Type fields are one of the allowed types (basicTypes)
  • replace error handling in String() with panics since the errors should have been taken care of at parsing time
• bindings.go:
  • unexport some unneeded functions as per @hzhou8's comments
  • replace schema errors with panics

[hzhou8]

Thanks @amorenoz and all for the great work and reviews.

[vtolstov]

@amorenoz thanks for great work!

[amorenoz]

Thanks @vtolstov and @hzhou8 for the reviews and guidance. I'll rebase #31 and update the message to keep the discussion going