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

Add generated code using libovsdb #2217

Merged
merged 4 commits into from
Jun 22, 2021
Merged

Conversation

dave-tucker
Copy link
Collaborator

@dave-tucker dave-tucker commented May 12, 2021

- What this PR does and why is it needed

Checks in generated code that can be used to communicate with OVN via ovn-org/libovsdb.
Version 20.12 of the nb/db schemas were used for generation.
There is documentation on how to use the library on https://github.com/ovn-org/libovsdb

The intention is for this to be used in place of go-ovn
The benefit to using libovsdb is that enables generic CRUD (and mutate) operations using the generated structs.
If at any point you need a new field, you can quickly regenerate code using a newer schema version.

- Special notes for reviewers

This is code-generation only.
No usage of this library yet.

Also, we avoid import conflicts with ebay/go-ovn as it's using ebaylibovsdb.

- How to verify it

CI will pass. You can see the testing that has been done over at https://github.com/ovn-org/libovsdb

- Description for the changelog

@dave-tucker
Copy link
Collaborator Author

dave-tucker commented May 12, 2021

Testing wise, I'm looking at the one or all of the following options:

a) a FakeOvsdbClient that simply puts everything directly in to it's cache. You can then assert in tests that rows exist in the db that look like you want them to.

b) a FakeOvsdbServer implementation that might be a little smarter, and produce realistic(ish) error messages

c) using two real ovsdb servers

@fedepaol
Copy link
Collaborator

c) using two real ovsdb servers

What I think it would be really cool is using something like https://github.com/ory/dockertest to setup the databases to interact with.

go-controller/Makefile Outdated Show resolved Hide resolved
@aojea
Copy link
Contributor

aojea commented May 25, 2021

@dave-tucker I've checked out this PR and the code doesn't compile because you have to do go mod vendor after the go mod tidy and checkout the vendor folder, at least that is what is happening to me.

Another thing, is that I think that we shouldn't try to do this in one shot, it is too complex to replace all the go-ovn bindings, is there any incompatibility if we checkout first the support for libovsdb and we start to migrate things one by one?

@dave-tucker
Copy link
Collaborator Author

dave-tucker commented May 25, 2021

@aojea there is no incompatibility, but I wouldn't recommend releasing or using a version that has both go-ovn and libovsdb in parallel. Both will retain an in-memory copy of all tables monitored in the nbdb and sbdb. Effectively we'll have a lot higher memory usage (especially on large dbs) until we migrate away from go-ovn.

I can certainly make this PR just to add generation for now so it's smaller. Then I could look at porting one or two usages over to the new bindings.

@aojea
Copy link
Contributor

aojea commented May 25, 2021

Both will retain an in-memory copy of all tables monitored in the nbdb and sbdb. Effectively we'll have a lot higher memory usage (especially on large dbs) until we migrate away from go-ovn.

ah, ok, right, I forgot that

I can certainly make this PR just to add generation for now so it's smaller. Then I could look at porting one or two usages over to the new bindings.

also I think we can spread the load and we can start playing/learning

@astoycos
Copy link
Collaborator

I am happy to try and help here as well if we would like to divide and conquer :)

@coveralls
Copy link

coveralls commented May 27, 2021

Coverage Status

Coverage decreased (-0.4%) to 53.576% when pulling 2db262a on dave-tucker:libovsdb into 7ec1ad1 on ovn-org:master.

@dave-tucker dave-tucker changed the title [WIP] Migrate from go-ovn to libovsdb Add generated code using libovsdb May 27, 2021
@dave-tucker dave-tucker marked this pull request as ready for review May 27, 2021 16:56
@dave-tucker
Copy link
Collaborator Author

@aojea @squeed @astoycos thanks for the comments. I've re-scoped this to just check in the generated bindings for the nb and sb schemas.

go-controller/go.mod Outdated Show resolved Hide resolved
go-controller/go.mod Outdated Show resolved Hide resolved
@dave-tucker
Copy link
Collaborator Author

/retest

@dave-tucker
Copy link
Collaborator Author

Updated to tip of libovsdb's main branch. This is a release candidate for 0.4.0 - we'll be tagging shortly and then I'll push again, removing my last #FIXME before this is merge-worthy

@dave-tucker
Copy link
Collaborator Author

v0.4.0 tagged, updated PR with appropriate versions

@dave-tucker
Copy link
Collaborator Author

/retest

@dave-tucker
Copy link
Collaborator Author

Update: I found a few issues when doing #2262 so I'll merge some fixes in libovsdb, tag a new version and update this PR shortly

This adds a gen.go in each package that will use
modelgen from libovsdb to populate the packages based
on the content of the schema

Signed-off-by: Dave Tucker <[email protected]>
To reproduce, run ./hack/update-codegen

Signed-off-by: Dave Tucker <[email protected]>
@dave-tucker
Copy link
Collaborator Author

@trozet this should be good to go

@dave-tucker
Copy link
Collaborator Author

Update: Included the util functions from #2262 the client module gets included in the vendor dir

Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

thanks @dave-tucker !

cd "${builddir}"
GO111MODULE=on go install github.com/ovn-org/libovsdb/cmd/[email protected]
cd "${olddir}"
if [[ "${builddir}" == /tmp/* ]]; then #paranoia
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

# generate ovsdb bindings
if ! ( command -v modelgen > /dev/null ); then
echo "modelgen not found, installing github.com/ovn-org/libovsdb/cmd/modelgen"
olddir="${PWD}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushd/popd is easier to use, no need to change this though

@trozet trozet merged commit 41c79f4 into ovn-org:master Jun 22, 2021
andreaskaris pushed a commit to andreaskaris/ovn-kubernetes that referenced this pull request Jul 11, 2024
OCPBUGS-33758,OCPBUGS-35347,SDN-4919,OCPBUGS-35367,OCPBUGS-33758,SDN-5011: Downstream Merge: July 2nd
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

Successfully merging this pull request may close these issues.

8 participants