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

[RFC] client: honor last transaction IDs #283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dcbw
Copy link
Contributor

@dcbw dcbw commented Jan 19, 2022

Maybe fixes #250

@dave-tucker is this kinda what you were thinking of, albeit not very clean?

@dcbw dcbw force-pushed the monitor-cond-since branch 2 times, most recently from fc527d8 to fa20454 Compare January 19, 2022 05:43
@coveralls
Copy link

coveralls commented Jan 19, 2022

Pull Request Test Coverage Report for Build 1790135334

  • 49 of 81 (60.49%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 73.522%

Changes Missing Coverage Covered Lines Changed/Added Lines %
util/errors.go 5 11 45.45%
cache/cache.go 14 27 51.85%
client/client.go 30 43 69.77%
Totals Coverage Status
Change from base Build 1754146115: 0.09%
Covered Lines: 4465
Relevant Lines: 6073

💛 - Coveralls

Copy link
Collaborator

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

This is a step in the right direction...

The missing piece is here:

if err == nil && reply.Found {

You need to purge the cache only if reply.found != true, otherwise the results from the monitor RPC can be populated on top of the cache as-is (since it was correct up to lastTransactionId). To do this, we probably need to move purge() to happen inside this function.

client/client.go Outdated
@@ -243,6 +295,10 @@ func (o *ovsdbClient) connect(ctx context.Context, reconnect bool) error {
for dbName, db := range o.databases {
db.monitorsMutex.Lock()
defer db.monitorsMutex.Unlock()

if err := db.purge(dbName); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move this to the monitor() function...

client/client.go Outdated
@@ -199,6 +200,65 @@ func (o *ovsdbClient) Connect(ctx context.Context) error {
return nil
}

func (db *database) purge(name string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is along the lines of what I had in mind, but I think we can simplify...

Have monitor() call this function while reconnecting (it already has reconnecting bool) to put the cache in the correct state. It will need to be re-written to be scoped only to the monitor that is calling it... this would happen after we get the RPC reply but before we populate updates.

@dave-tucker
Copy link
Collaborator

@dcbw there are also some edge cases that we might need to document for... (or have the API prevent)

  1. What happens if we have 2 monitors on the same table?
  2. What happens if we have 2 conditional monitors on the same table with different conditions
  3. What happens if we have 2 conditional monitors on the same table with overlapping conditions

In all likelihood I think we'll be fine as I would hope that we'd get consistent results from ovsdb-server (as in both monitors would get found == false and the cache would be purged and rebuilt... and we'd be eventually consistent.

@dave-tucker
Copy link
Collaborator

Oh also, you'll want to reset lastTransactioId to be empty if we do clear the cache...

@dcbw
Copy link
Contributor Author

dcbw commented Jan 19, 2022

@dave-tucker I thought I tried to account for these:

  • What happens if we have 2 monitors on the same table?

purge() tries to handle this by building up the map of tables to purge. If any of the monitors are not CondSince then it purges the entire table.

  • What happens if we have 2 conditional monitors on the same table with different conditions

purge() adds all the conditions from all monitors for this table to a list and purges the cache of all rows that match any of the conditions.

  • What happens if we have 2 conditional monitors on the same table with overlapping conditions

Would that work different from the second case? If they have overlapping conditions, don't we just want to blow away any row that matches any of the conditions of any monitors on the table?

In any case, I'm not sure we can do this all in monitor() itself, since that deals with just a single monitor. But we really need a view of all monitors to know how to purge each table and/or its rows.

@dcbw
Copy link
Contributor Author

dcbw commented Jan 20, 2022

Thought about this more. Do I have the stuff backwards? Is the following correct?

  1. for CondSince we want to purge everything except rows that match any of the conditions. Becuase when we get the monitor reply we'll get updates for those rows based on their current state (which is the LastTransID).

  2. But for !Since we do want to purge everything since we'll get full updates of current state for the monitored rows so we can purge them.

And just to confirm; we are OK purging rows that dont' match any of the monitors/conditions?

@dave-tucker
Copy link
Collaborator

dave-tucker commented Jan 24, 2022

@dcbw

In any case, I'm not sure we can do this all in monitor() itself, since that deals with just a single monitor. But we really need a view of all monitors to know how to purge each table and/or its rows.

Right, the trouble then becomes how you clear the cache if ovsdb-server says "i don't have that lastTransactionID".
We'd probably need to disconnect/reconnect and start from scratch across all monitors I guess...

Thought about this more. Do I have the stuff backwards? Is the following correct?

  1. for CondSince we want to purge everything except rows that match any of the conditions. Becuase when we get the monitor reply we'll get updates for those rows based on their current state (which is the LastTransID).

Partially correct. When we get the monitor reply we MAY get updates for those rows based on updates since the lastTransactioID. It's also possible that we get the full db state if found = false so we'd need to drop everything. It's this behaviour that makes me think we need to have a per-monitor purge() function that clears ONLY the tables/rows that it's monitoring.

In the case of overlapping monitor tables/conditions we'd end up doing multiple purges... but the end state would always be the same... although I honestly think that overlapping monitor conditions is a terrible idea and we should just plain up state that it's not supported.

The only other option I can see is to have a cache.Populate3 which does as cache.Populate2 but also propagates the found bool.
And we'd just have the cache overwrite everything without any existence checks if found=false

  1. But for !Since we do want to purge everything since we'll get full updates of current state for the monitored rows so we can purge them.

This is correct.

And just to confirm; we are OK purging rows that dont' match any of the monitors/conditions?

Yep. It's essentially a noop so we could skip it.

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.

Use MonitorCondSince Properly
3 participants