Proposed api interface changes

[vtolstov]

I want to propose interface changes, Firstly to follow go interface naming, api creates Client interface.
All methods have args with native types, so for adding port to logical switch we can provide
*LogicalSwitch struct to function, that have needed name. Also in this struct we can pass ExternalIDs, so LSPAdd can find needed ls by name, uuid, or external_ids. This is already handled with getRowUUIDs.

as in example of LSPAdd second arg is *LogicalSwitchPort with filled needed data. So function can receive only 2 args. We can easy handle optional params and default values.

Inside function we can use https://godoc.org/github.com/mitchellh/mapstructure to convert from struct to map[string]interface{}.

I think that this is more universal that current approach, and easy deduplicate many identical pieces of our code.

[vtolstov]

@hzhou8 @softwarejl i need you approve and suggestions. This is not small task, but it don't need too much time to provide WIP for this.

[hzhou8]

@vtolstov I support this idea! @softwarejl and I had discussed about having more generic API interface many times, just didn't get bandwidth to work on it. Maybe you could have a POC first. @softwarejl what do you think?

[softwarejl]

Yes, we need to improve on that part, beside the interface, we need a way to define the data structure which aligns with ovn table structure. we need a poc firstly.

[vtolstov]

first change in this series #42

[vtolstov]

@hzhou8 this is first part of storing in cache needed struct https://github.com/eBay/go-ovn/pull/44/files#diff-48d527baa1a7dc53e558c5548785cf1bR277

[vtolstov]

i think that if we store needed type inside cache all thing can be very simple.
next sep is write helper to check if some struct holds in another (so we can pass LogicalSwitch with only uuid setted and get back full row from cache if it exists)

[vtolstov]

if somebody want helps me , i'l be happy =)

[hzhou8]

@vtolstov Thanks for the effort. Could you complete the POC with just one end-to-end example, e.g. LogicalSwitchPort create/read/update/delete with the new approach? That would be very helpful for me to understand, since I am not sure yet if I understand what the helper function means exactly.

[vtolstov]

@hzhou8 check ovnnbimp.go and acl.go
main thing - i'm parse map[string]interface{} and convert it to needed struct and store it in cache,
when i need to find needed row i'm check via reflection all fields in passed row and check such row in cache.
now this is POC so not all fine, but main parts ready.

[vtolstov]

@hzhou8 what can you say?

[hzhou8]

@vtolstov this looks awesome! It simplifies a lot for the conversions. I think most of the code is generic for ovsdb, so it can actually be in libovsdb. For OVN part, it would be even better if we can generate the data types from schema, instead of manually write it (but it is totally ok now as the first step).

[hzhou8]

i think that if we store needed type inside cache all thing can be very simple.
next sep is write helper to check if some struct holds in another (so we can pass LogicalSwitch with only uuid setted and get back full row from cache if it exists)

I think we should have two sets of "get" methods. One is get by uuid, another is "find" by any combination of column values.

[vtolstov]

Ok, so for now may be i can complete rewrite via ovn repo? And after that when all works fine we can move needed parts of code to libovsdb and change some parts of go-ovn?
Generation types from /usr/share/openvswitch/ovn-nb.ovsschema is fine for me, this is error prone and cam be used via go generate step.
About two get method -thanks, this is good.

[vtolstov]

last question, does we need to support nested values in ExternalIDs?
As i know for neutron case this is needed, so i also need to create some recursive function to check nested values... (but for POC now needed now).

[hzhou8]

last question, does we need to support nested values in ExternalIDs?
As i know for neutron case this is needed, so i also need to create some recursive function to check nested values... (but for POC now needed now).

Do you have an example? I think we should support whatever column types can appear in OVSDB schema. (Of course it doesn't have to be complete in the first version)

[vtolstov]

I can't find example now, but as i see in libovsdb map represented as map[string]interface{} , but ovn-nb says:

external_ids: map of string-string pairs
                     Key-value pairs for use by the CMS.  The  CMS  might  use
                     certain  pairs,  for example, to identify entities in its
                     own configuration that correspond to those in this  data‐
                     base.

so may be recursion is not needed

[hzhou8]

Yes, the schema defines as:
"external_ids": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
So the value must be string.
In fact, checking OVSDB RFC7047, it is not possible to have nested values:
<type>
The type of a database column. Either an <atomic-type> or a JSON
object that describes the type of a database column, with the
following members:
"key": <base-type> required
"value": <base-type> optional
"min": <integer> optional
"max": <integer> or "unlimited" optional

The "value" has to be base-type, i.e. string, integer, etc.

[amorenoz]

@vtolstov , @hzhou8 What is the status of this enhancement?
I think I'd be able to allocate some cycles to help on this effort if needed.

[vtolstov]

i don't have time to finish it. may be the best - generate needed struct from schema and replace own handmade stuff
after that rewrite api interfaces

[amorenoz]

I have coded a small PoC based on @vtolstov's and some old code I had. The result is here:
https://github.com/amorenoz/go-ovn/commits/poc/crud-api
(used a new table: Datapath_Binding from SouthBound)

Since there is no proposal for an API (let's discuss this!), I've implemented an API that resembles a ORM:
https://github.com/amorenoz/go-ovn/blob/be88c59e1074b54956d632c38165123ee6644c36/datapath_binding_test.go

Taking @vtolstov's idea, the encoding/decoding is done based on struct tags:
https://github.com/amorenoz/go-ovn/blob/be88c59e1074b54956d632c38165123ee6644c36/datapath_binding.go

It's still very early, but it could serve as a discussion trigger. @hzhou8 WDYT?

Some issues that I would like feedback on:

• Is there a proposal of how the API should look like? I think that would be a good start.
• Should we keep expose a "clean" Add() function that performs the operation instead of returning it?
• Should this be exposed in the same Client or in another interface?
• Should we auto-generate the definition of the structs and their tags based on the ovn schema? if so, how schema changes should be handled? should we keep updating the source code or keep compatibility with older schemas?

[vtolstov]

sorry i need more time to dig.
Firstly i don't like

var datas []DataPathBinding
err = ovndbapi.List(&datas)

usually List, returns slice and err like datas, err := ovndbapi.List()

Same thing for Get...

[amorenoz]

sorry i need more time to dig.
Thanks for taking some time to look into it!

Firstly i don't like

var datas []DataPathBinding
err = ovndbapi.List(&datas)

I took inspiration from a well-known, widely-used Golang ORM
It also resembles the widely used json {un,}marshal functions where the information of how to translate from json to struct is encoded in the stuct's 'tags' (credit's are yours @vtolstov for this)
This also allows the API to be expanded without requiring library recompilation. If a new user-specific table is created, the user could create a new struct specifying "ovn" tags and the library would be able to read / write from that Table without recompiling the library.

usually List, returns slice and err like datas, err := ovndbapi.List()

If ovndbapi.List() is used, how will List() know what Table to list? (the current PoC uses reflect to inspect the type of the given argument). We would en up having ListLogicalRouter(), ListLogicalSwitch, etc... and this should be automatically generated. Is that what you prefer?

@hzhou8 WDYT?

[vtolstov]

i'm prefer this style:

type Client struct {
  LogicalRouter LogicalRouter
  LogicalSwitch LogicalSwitch
  ...
}

type LogicalSwitch interface {
  List() ([]schema.LogicalSwitch, error)
}

so list of switches looks like

cli, err := NewClient(xxx)

sws, err := cli.LogicalSwitch.List()

[hzhou8]

@amorenoz Sorry for the slow response. The POC looks promising. I need more time to study the Golang ORM example. Feel free to send a PR to the repo. (In fact I think ebay/libovsdb is where the major part of change should go)

My answers to some of your questions:

Should we keep expose a "clean" Add() function that performs the operation instead of returning it?

I think it is important to return a command, so that it can be combined and executed together with other commands as a single transaction.

Should we auto-generate the definition of the structs and their tags based on the ovn schema? if so, how schema changes should be handled? should we keep updating the source code or keep compatibility with older schemas?

Yes we should auto-generate. I don't think we need to maitain the compatibility in the Go library. It is OVN's responsibility to keep the DB schema compatibility. We can follow the approach of the C IDL.

[amorenoz]

@amorenoz Sorry for the slow response. The POC looks promising. I need more time to study the Golang ORM example. Feel free to send a PR to the repo. (In fact I think ebay/libovsdb is where the major part of change should go)

Thanks @hzhou8, once there is consensus on the high level API, I'll send a PR.

My answers to some of your questions:

Should we keep expose a "clean" Add() function that performs the operation instead of returning it?

I think it is important to return a command, so that it can be combined and executed together with other commands as a single transaction.

Should "reading" functions (such as List(), Get()) be the same?

Should we auto-generate the definition of the structs and their tags based on the ovn schema? if so, how schema changes should be handled? should we keep updating the source code or keep compatibility with older schemas?

Yes we should auto-generate. I don't think we need to maitain the compatibility in the Go library. It is OVN's responsibility to keep the DB schema compatibility. We can follow the approach of the C IDL.

Right, but the IDL generation is at compile time, right?
So go-ovn should use a specific version of the schema to generate it's code and it's the user's responsibility to use a version of go-ovn compatible with it's ovn DB schema?
Or, alternatively, is the schema going to be provided dynamically? in which case, the API has to be 100% generic

[hzhou8]

@amorenoz Sorry for the slow response. The POC looks promising. I need more time to study the Golang ORM example. Feel free to send a PR to the repo. (In fact I think ebay/libovsdb is where the major part of change should go)

Thanks @hzhou8, once there is consensus on the high level API, I'll send a PR.

My answers to some of your questions:

Should we keep expose a "clean" Add() function that performs the operation instead of returning it?

I think it is important to return a command, so that it can be combined and executed together with other commands as a single transaction.

Should "reading" functions (such as List(), Get()) be the same?

"reading" functions should directly return the results. In addition, for efficiency, we should provide an interface to iterate the local cache instead of duplicate the whole result list by a List() function.

Should we auto-generate the definition of the structs and their tags based on the ovn schema? if so, how schema changes should be handled? should we keep updating the source code or keep compatibility with older schemas?

Yes we should auto-generate. I don't think we need to maitain the compatibility in the Go library. It is OVN's responsibility to keep the DB schema compatibility. We can follow the approach of the C IDL.

Right, but the IDL generation is at compile time, right?

Yes.

So go-ovn should use a specific version of the schema to generate it's code and it's the user's responsibility to use a version of go-ovn compatible with it's ovn DB schema?

Yes. Do you think this is reasonable?

Or, alternatively, is the schema going to be provided dynamically? in which case, the API has to be 100% generic

I didn't thought about this. Too generic API may be harder to use, but I didn't think deeply on this option.

[amorenoz]

@amorenoz Sorry for the slow response. The POC looks promising. I need more time to study the Golang ORM example. Feel free to send a PR to the repo. (In fact I think ebay/libovsdb is where the major part of change should go)

Thanks @hzhou8, once there is consensus on the high level API, I'll send a PR.

My answers to some of your questions:

Should we keep expose a "clean" Add() function that performs the operation instead of returning it?

I think it is important to return a command, so that it can be combined and executed together with other commands as a single transaction.

Should "reading" functions (such as List(), Get()) be the same?

"reading" functions should directly return the results. In addition, for efficiency, we should provide an interface to iterate the local cache instead of duplicate the whole result list by a List() function.

Good idea!

Should we auto-generate the definition of the structs and their tags based on the ovn schema? if so, how schema changes should be handled? should we keep updating the source code or keep compatibility with older schemas?

Yes we should auto-generate. I don't think we need to maitain the compatibility in the Go library. It is OVN's responsibility to keep the DB schema compatibility. We can follow the approach of the C IDL.

Right, but the IDL generation is at compile time, right?

Yes.

So go-ovn should use a specific version of the schema to generate it's code and it's the user's responsibility to use a version of go-ovn compatible with it's ovn DB schema?

Yes. Do you think this is reasonable?

It might be complicated. Layered projects might want to stick with an old version of go-ovn feature-wise while supporting newer ovn schemas. That would put some pressure into go-ovn for being very conservative in the changes it introduces. The alternative would be to clone the entire repository per supported ovn version.

Plus, you have the problem of guaranteeing what version of ovn you work with. Projects that work with a pre-defined version of go-ovn (such as ovn-kubernetes) might be able to cope with it, but others like skydive might not have it that easy.
CC'ing some of the relevant contributors for feedback: @lebauce @abhat

Or, alternatively, is the schema going to be provided dynamically? in which case, the API has to be 100% generic

I didn't thought about this. Too generic API may be harder to use, but I didn't think deeply on this option.

The API in my last PoC is generic. The columns to read are determined by the tags of object the client passes to the function, e.g:

type DataPathBinding struct {
	UUID        string            `ovn:",uuid"`
	TunnelKey   int               `ovn:"tunnel_key"`
	ExternalIDs map[string]string `ovn:"external_ids"`
}
var datas []DataPathBinding
err = ovndbapi.List(&datas)

This would allow for us to provide a set of pre-defined structures per ovn version, place them in different directories and allow users to use the same version of go-ovn with multiple versions of ovn.

Even more, it would allow developers to define their own structs (e.g: only partial readings, or out-of-tree / unmerged ovn schemas).

[vtolstov]

we can generate structs for all needed ovn versions in different dirs. libovsdb may check ovn version on connect and return needed structs to go-ovn. and go-ovn may return needed data depending on ovn version i think

[amorenoz]

we can generate structs for all needed ovn versions in different dirs. libovsdb may check ovn version on connect and return needed structs to go-ovn. and go-ovn may return needed data depending on ovn version i think

OK, sounds good. How about the following for the API:

import api github.com/ebay/go-ovn/apis/nb/20.14.0

nbdb := api.newClient(cfg)

# list
var logical_routers []api.LogicalRouter
logical_routers, err = nbdb.LogicalRouter.List()

# get by UUID
var logical_router api.LogicalRouter
logical_router, err = nbdb.LogicalRouter.Get(uuid)

# get by index
var logical_router api.LogicalRouter
logical_router, err = nbdb.LogicalRouter.GetByIndex("name", name)

# add/update
logical_router := api.LogicalRouter {
...
}
cmd, err = nbdb.LogicalRouter.Add(logical_router)
cmd, err = nbdb.LogicalRouter.Update(logical_router)

How does that look to you?
Some questions:

• In order to implement a zero-copy iterator, the internal cache should be changed to store interface{} (already decoded according to the ovndb schema). Right?
• Technically, the dependency is not with the version of OVN but with the version of the ovn-nb.ovsschema and ovn-sb.ovsschema, which have different versions. So we will have separate sb and nb api paths, objects and SignalCB interfaces. Does it sound reasonable?
• As it is a non-written policy from the ovn team not to delete fields in the schema, a specific set of api objects might work with more than one version. We could specify the range of known compatible schema versions per api and, if the installed version is not within the range, print a warning and try (best effort) to decode the data. The client must know that fields might be empty or missing.
• A tool could be written to automatically generate the api objects based on a given schema file, that way we will be able to track future changes without too much work

[amorenoz]

More questions:

• This API is more generic than the existing one, meaning: we would be replacing the existing "concrete" API that has functions such as LSPGetDHCPv4Options() or LinkSwithToRouter with this one. Right?
  If we wanted to still have these concrete functions we would have to, either generate them per schema version (and that's quite a lot of generated code) or have it do some reflex magic. What do you think?
• It seems to me that the 100% generic API I drafted in my PoC (where the decoding information is not autogenerated but derived from the struct's tags) can still be kept while sharing most of the code. Do you think it's worth trying to keep it for flexibility?

[amorenoz]

I have updated my PoC to expose the API @vtolstov suggests:
The API is exposed in a new struct
The client using the API can be seen here

We would need to create a tool that generates the types and API structs per Table in the schema and combine them all into a DatabaseAPI object

[vtolstov]

client using the api looks good to me.
@hzhou8 what you think?

[amorenoz]

@hzhou8, @vtolstov Any chance you could give your opinion on the questions above? I'd like to start implementing, but I would need to know there is an agreement on the general approach and API first.
Thanks

[amorenoz]

@vtolstov @hzhou8 Any updates? I'm waiting for some feedback from you to proceed.
I opened an issue in libovsdb to start exposing a somewhat native API there (eBay/libovsdb#29) as well as a PR to support schema parsing (eBay/libovsdb#28).
Are we OK with the approach?