go-ovn icon indicating copy to clipboard operation
go-ovn copied to clipboard

Re-work API to support full object modifications

Open alexanderConstantinescu opened this issue 4 years ago • 3 comments

Currently this library supports modifying a small aspect of all OVN objects, which makes it quite unusable in certain use cases. Ex:

Logical Routers in OVN currently have these fields:

_uuid               : 
enabled             : []
external_ids        : {}
load_balancer       : []
name                : 
nat                 : []
options             : {}
policies            : []
ports               : []
static_routes   : []

Now, these fields are internally modifiable and supported by the library as we can see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L26

However, the CRUD operations exposed by the API do not allow the modification of most of these fields, as we see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L40

This means that a user of this library can effectively only set external_ids on any Logical Router, and moreover: also renders some functional operations completely impossible, such as setting options on the Logical Router.

I would expect to see the API allow the following operations

func (odbi *ovndb) lrAddImp(logicalRouter LogicalRouter) (*OvnCommand, error) 

and

func (odbi *ovndb) lrUpdateImp(logicalRouter LogicalRouter) (*OvnCommand, error) 

Depending on the time available to me, I might file a PR with this. But I would like to get some input on if these changes are valid. They directly modify the API, so it would be an important change.

alexanderConstantinescu avatar Jul 16 '20 14:07 alexanderConstantinescu

if you start and provide simple one object changing to new interface we can check it. I'm try sometimes ago to do something usable in this pr https://github.com/eBay/go-ovn/pull/59 but not have time to finish. And as i saw create all options blows code

vtolstov avatar Jul 16 '20 15:07 vtolstov

@alexanderConstantinescu Thanks for bringing this up. Currently the APIs are modeled close to what ovn-nbctl provides, and missing parts were added gradually on-demand, and some of them are still missing, such as setting options for routers, as you mentioned. I agree that in some cases it is better to support full object operations with all the properties, but at this point it is ok to combine multiple API calls into a single transaction to execute, if we are able to add the missing parts.

In the longer term I would expect to have a code generator similar to what C IDL does, to generate all the CRUD APIs according to schema. However, this is not a priority yet.

Before this is ready, if you want to add new APIs manually that takes full object as input, I am totally ok with it, but we need to figure out a good naming convention, to distinguish from existing CRUD APIs (I'd avoid replacing existing APIs, but just adding new ones). Or, would it work for you by just adding the missing part of "Set" APIs, e.g. to set the options for LR?

hzhou8 avatar Jul 17 '20 17:07 hzhou8

Before this is ready, if you want to add new APIs manually that takes full object as input, I am totally ok with it, but we need to figure out a good naming convention, to distinguish from existing CRUD APIs (I'd avoid replacing existing APIs, but just adding new ones). Or, would it work for you by just adding the missing part of "Set" APIs, e.g. to set the options for LR?

Yes, to not break compatibility we would definitely need to either add individual "setters" or add new methods allowing full object modification. I will try to look into doing this, given the time available to me. But for anyone following along: in case I don't post any updates here or file a PR, feel free to pick this task up.

alexanderConstantinescu avatar Jul 20 '20 08:07 alexanderConstantinescu