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

Auto reconnect #176

Merged
merged 1 commit into from
Jul 1, 2021
Merged

Auto reconnect #176

merged 1 commit into from
Jul 1, 2021

Conversation

dave-tucker
Copy link
Collaborator

@dave-tucker dave-tucker commented Jun 23, 2021

This commit builds on the work released in v0.5.0 to provide disconnect notifications.
It adds the WithReconnect option that provides the ability for the library to handle automatically reconnecting then client when it is disconnected. This includes re-establishing any Monitors that a user may have established.

Normal Operation

  • Disconnect() and Close() API calls do the same thing - clear all connection state and set the cache to nil
  • In both cases, you'll get a notification on the DisconnectNotify channel

WithReconnect

  • Disconnect() (or anything that closes the underlying rpcClient) will trigger reconnection. You will not get a notification on the DisconnectNotify channel. The cache content is cleared BUT any event handlers will be retained.
  • Close() will do a full shutdown of the client. All connection state is cleared and the cache is set to nil. You will get a notification on the DisocnnectNotify channel

WithReconnect includes a timeout (for the call to DialWithContext and a user-provided backoff function, which can handle, ZeroBackoff, ExponentialBackoff and more.

API Changes

  • Monitor and MonitorAll do not accept a jsonContext argument any more. We instead generate a unique ID for every monitor and return this to the caller. This can then be used to MonitorCancel if they wish. This is required to enable the auto-reconnect feature because we want to ensure the unique-ness of monitor id's across connection hiccups.
  • Cache gains a Purge method to drop the contents of the cache but leave any eventHandlers intact so we can ensure we don't have stale entries in the cache on re-establishing a monitor.

** Misc **
This also cleans adds locking of key state in the client ensure that things don't break catastrophically when the automatic reconnection is happening in a separate goroutine. This should also make the client safe to be shared across by more than one goroutine.

Fixes #172

@coveralls
Copy link

coveralls commented Jun 23, 2021

Pull Request Test Coverage Report for Build 989870243

  • 144 of 173 (83.24%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 76.681%

Changes Missing Coverage Covered Lines Changed/Added Lines %
client/client.go 116 145 80.0%
Totals Coverage Status
Change from base Build 989822161: 0.8%
Covered Lines: 3193
Relevant Lines: 4164

💛 - Coveralls

o.monitorsMutex.Lock()
defer o.monitorsMutex.Unlock()
for id, request := range o.monitors {
err = o.monitor(id, reconnect, request...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

calling o.monitor will re-establish the monitors.
the reponse to the monitor rpc call is the the contents of the tables we're monitoring which gets fed to cache.Populate()

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so ovsdb takes care of re-syncing the monitor on reconnect? nice, etcd doesn't do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, the downside is you get all the things, not just the delta from when you were last connected... and with a large database it can take a while to process.

@jcaamano
Copy link
Collaborator

jcaamano commented Jul 1, 2021

/lgtm as far as I can tell

This commit adds the WithReconnect option that provides the ability for
the library to handle automatically reconnecting then client when it is
disconnected.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker dave-tucker merged commit 3040e12 into ovn-org:main Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client: Support Multiple Monitor Requests (and maybe resuming Monitor on reconnect)
4 participants