-
Notifications
You must be signed in to change notification settings - Fork 151
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
Cluster support #221
Cluster support #221
Conversation
8707553
to
2ea7818
Compare
@dave-tucker PTAL. I still need to implement the critical bits, but this is the first start. |
client/client.go
Outdated
db.monitorsMutex.Lock() | ||
defer db.monitorsMutex.Unlock() | ||
for id, request := range db.monitors { | ||
// TODO: should err here just be treated as failure to connect? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've not encountered a situation where a monitor
rpc failed, but it's plausible it could if we were disconnected due to a network hiccup and ovsdb-server hadn't yet closed the monitor session completely - the error would be that there is already a monitor with the same id. in which case, trying again once the state has been torn down would be the best way to fix it I think....
tl;dr i think it's ok to treat this error the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far @squeed. Just a couple of comments so far
152beaa
to
375a0f1
Compare
@dave-tucker thanks for the review; I'll work on the wording changes. I've implemented the _Server watch, so that should be good to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have a typo to fix s/betwen/between/g
and the server_model
package should be renamed serverdb
and then the lint should pass 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had another look through. As db.schema
gets updated on re-connection any read access to it needs to hold an RLock
or the race detector gets upset.
FWIW, trying to do this downstream in go-ovn via openshift/ovn-kubernetes#695 |
454041d
to
3ad9102
Compare
FYI, logger support is currently stuck until klog takes a new release, since it moves from logr v0.4 to v1.0. So that will have to wait. otherwise, @dave-tucker, this is ready to go. |
As a part of ovn-org#129, we need to monitor the special "_Server" database so we can disconnect if a server loses leader. That means we need (internal) support for multple databases / schemas / models on the same endpoint. This implements that, but doesn't expose it to the end-user. Signed-off-by: Casey Callendrello <cdc@redhat.com>
This table is used by ovsdb itself to report internal status. Signed-off-by: Casey Callendrello <cdc@redhat.com>
This adds an additional check when testing a potential endpoint: if the endpoint reports itself as not a leader, then reject it and continue. This needs to be explicitly requested via a specific Option. Note that this does not monitor for leadership changes. Signed-off-by: Casey Callendrello <cdc@redhat.com>
This sets up a monitor on the _Server/Database meta-table, and disconnects if leadership is lost. It also adds some useful locking. Signed-off-by: Casey Callendrello <cdc@redhat.com>
args is an arbitrary json cookie, so we need to handle that rather than assuming it's a string. Signed-off-by: Casey Callendrello <cdc@redhat.com>
1c9cd01
to
3d13c37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @squeed
Pull Request Test Coverage Report for Build 1182187084
💛 - Coveralls |
Fixes #129
We would like to support ovn raft clusters. This means we need:
_Server
database