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

Re-work API to support full object modifications #92

Open
alexanderConstantinescu opened this issue Jul 16, 2020 · 3 comments
Open

Re-work API to support full object modifications #92

alexanderConstantinescu opened this issue Jul 16, 2020 · 3 comments

Comments

@alexanderConstantinescu

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.

@vtolstov
Copy link
Contributor

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 #59 but not have time to finish. And as i saw create all options blows code

@hzhou8
Copy link
Contributor

hzhou8 commented Jul 17, 2020

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

@alexanderConstantinescu
Copy link
Author

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.

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

No branches or pull requests

3 participants