Fix State.SyncChanges mutex Lock/Unlock #6619

PumpkinSeed · 2019-10-13T13:31:27Z

This PR wants to resolve the #6616

I try to accomplish as minimal lists about the necessary fields as I could, so I created two function scope map.

hashicorp-cla · 2019-10-13T13:31:29Z

All committers have signed the CLA.

ShimmerGlass · 2019-10-14T10:28:01Z

If I'm not mistaken this introduces a race for both service and checks.

When deleting a service :

SyncChanges() is called, the diff is computed, lock is released
deleteService() is called and makes a Catalog.Deregister RPC
A service with the same id is registered
The RPC returns and the service is removed from the map when it should not have been.

When updating a service :

SyncChanges() is called, the diff is computed, lock is released
syncService() is called and makes a Catalog.Register RPC
The service is updated
The RPC returns and the service is marked as "InSync" when its in fact not and the update is delayed until next anti-antropy run.

I'm not sure what the best way to fix this would be, but one way would be to maintain a state version number : the version is incremented on each state change and is stored in the ServiceState/CheckState structs. The {sync,delete}{Service,Check} funcs are called with the version their respecting entities had when the diff was computed (and the lock was held). If when the RPCs return the entity version is not what was passed as arguments the entity is not marked as in sync nor deleted from state.

Additionally, delete{Service,Check} currently expect the state lock to be held when called. This introduces a crash due to the concurrent writes to State.{services,checks}.

PumpkinSeed · 2019-10-14T10:51:45Z

@Aestek I see the complexity behind this problem. Let me have an other run with it.

PumpkinSeed · 2019-10-14T14:20:21Z

@Aestek I think it get fixed. So I'm checking the {ServiceState,CheckState}.Deleted, because in case of AddService these will be false, so after the RPC response arrives and the Deleted still true, it should mean there is no new service under the same id.

ShimmerGlass · 2019-10-14T14:39:08Z

@PumpkinSeed this fixes the "delete" scenario, but not the "update"

PumpkinSeed · 2019-10-14T14:47:58Z

@Aestek yes, it's a bit tricky. I would like to have a bit more time to investigate about.

PumpkinSeed · 2019-10-14T19:18:15Z

@Aestek I tried it with your suggestion of versioning.

…s-lock

jacksontj · 2019-10-29T23:26:51Z

I just ran into this same issue (actually ran into it because of disk space issues on the master nodes which caused API slowdown -- #866). While debugging this I set up a repro environment locally (master + agent with tc adding 3s latency in the middle). I have pulled down the PR branch and am running this build and it seems to resolve the issue.

I've also read through the PR and it seems like a reasonable fix to the problem (versioning works since its single-source data -- from the agent -- that needs to eventually be pushed to the masters).

So I am very interested in getting this merged in, so if there is anything I can do to help this along let me know :)

ShimmerGlass · 2019-10-30T09:39:41Z

This looks good to me as well

PumpkinSeed · 2019-12-11T16:59:48Z

May I ask what is the status of the PR? I have seen there is conflict. I will check it out tonight.

…ul into I6616-sync-changes-lock

codecov · 2019-12-13T18:12:10Z

Codecov Report

❗ No coverage uploaded for pull request base (master@35a6279). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #6619   +/-   ##
=========================================
  Coverage          ?   65.62%           
=========================================
  Files             ?      443           
  Lines             ?    53359           
  Branches          ?        0           
=========================================
  Hits              ?    35017           
  Misses            ?    14111           
  Partials          ?     4231

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35a6279...ddccf8d. Read the comment docs.

jacksontj · 2020-01-06T17:32:12Z

@freddygv any update on this PR? This fixes a huge liability for users of consul agent services.

hanshasselberg

Thank you for your work. @freddygv and I left a bunch of comments, I hope it helps.

We also discovered a problem with how semantics have changed for SyncChanges:

ServiceA is updated and RPC is sent
ServiceB is updated and RPC is sent (because the response for ServiceA is not back, this time SyncChanges sends out two RPCs: ServiceA and ServiceB)
ServiceC is updated and RPC is sent (because the response for ServiceA and ServiceB are not back, this time SyncChanges sends out three RPCs: ServiceA, ServiceB and ServiceC)
and so on

We are looking to hear what you think. We think this is an important issue, but it is also pretty tricky to get right. Thanks again.

agent/local/state.go

hanshasselberg · 2020-01-15T21:05:27Z

agent/local/state.go

+			var checkKey structs.CheckID
+			for _, check := range checks {
+				checkKey.Init(check.CheckID, &check.EnterpriseMeta)
+				l.checks[checkKey].InSync = true


We don't know it is in sync, because we did not check the version. Checks are all send along with Service registrations and if we want to mark them as in sync, we need to version them all as well. And check the version here.

agent/local/state.go

PumpkinSeed · 2020-01-24T13:29:34Z

I fixed nearly all of the suggested things. But I already lost my domain knowledge since I created the PR and also it's started to get a hell of changes and I can't oversee it properly.
So what do you think about a bit of restructure? @i0rek @freddygv
Let me explain my suggestion:

ServiceState and CheckState should get their own file line service_state.go and check_state.go
They should have methods for changing their InSync, because there are more places where we want to call it, and I think it make sense to have a general solution which checks everything properly before we want to perform that.
I prefer to move the related State struct's methods along with their main struct. ex.: func (l *State) CheckState(id structs.CheckID) *CheckState goes to check_state.go

Then when I oversee the problem again properly we can get back to the bigger issue what you told me. Or if you don't want to have this restructure I can get back my domain on the current implementation. But I recommend that, because it's really hard to manage it right now I guess.

Let me know what you think.

freddygv · 2020-02-06T21:23:16Z

@PumpkinSeed, after having a team-wide discussion today, we would like to suggest that our team makes the remaining changes to this PR.

Since the anti-entropy sync is so fundamental in the operation of Consul, we need to spend a significant amount of additional time making sure that:

Remaining corner-cases are addressed
Everything works as expected
There is appropriate test coverage

Due to the time commitment needed we also need to decide how to prioritize this work relative to other features on our roadmap. So the enhancement could end up on hold again because of that.

Lastly, please note that we would like to preserve the changes that you have made, and if we do, we will ensure that you are credited in the git history.

Thanks again for your contribution. We really appreciate the time and effort you have put into this problem.

PumpkinSeed · 2020-02-14T11:57:25Z

@freddygv
Probably this is the best for all of us. Because I'm totally out of context. Thank you for that help.

jacksontj · 2020-04-15T16:46:34Z

@freddygv any updated ETA on this fix? I've bee hit by this issue a couple of times and am anxious to get a fix in place.

freddygv · 2020-04-23T22:09:13Z

Hi @jacksontj, we picked this back up earlier this week. It should make its way into the next release as long as we feel comfortable with the changes.

jacksontj · 2020-06-10T18:10:33Z

@freddygv Just checking in, any update on this issue?

jacksontj · 2020-07-10T20:33:57Z

@freddygv any update on this issue?

freddygv · 2020-07-10T22:31:06Z

Hi @jacksontj we have been working on and off on the PR, but didn't have much time due to the 1.8.0 release. Going to take another pass at it next week and should have a more concrete update then.

jacksontj · 2020-07-29T19:09:45Z

@freddygv just checking in again here, really excited to get this fix in :)

freddygv · 2020-08-11T20:13:29Z

@jacksontj we're still working on an implementation on a new branch. I'll post here when we have PRs out.

jacksontj · 2021-04-21T16:47:24Z

@freddygv checking in now thats it been another 8 months :)

hanshasselberg · 2021-04-22T10:43:44Z

@jacksontj thanks for reaching out! We worked on this change quite a bit but don't have anything to show so far. This is a really challenging task because it changes Consul behaviour significantly. And while we still think this is a great idea, we no longer think it can be easily done.
We decided to stop working on this until we have the time to properly think about the implications of this change in advance and then decided on how to implement it so that it doesn't break the current behaviour.

dnephin · 2021-05-10T21:17:03Z

Thank you @PumpkinSeed for your work on this issue! Also thank you to everyone else who has helped or expressed interest in getting this fixed.

This is a difficult issue to solve and will likely require a substantial change to the agent/local package. It seems like this PR won't be able to merge, so I'm going to close it.

We have an issue (#6616) which describes the problem, and #8778 which is a potential first step for solving it. Please subscribe to those to be notified of any updates.

Fix State.SyncChanges mutex Lock/Unlock

ada6152

Call make format for contribution

bf308aa

Solve race in case of same id in delete

8226eb0

Add versions for state of {service,check}

04682d5

PumpkinSeed added 3 commits October 14, 2019 21:26

Merge branch 'master' into I6616-sync-changes-lock

268a6f1

Remove unecessary locks

9a1798c

Merge remote-tracking branch 'upstream/master' into I6616-sync-change…

08f3e39

…s-lock

freddygv self-assigned this Oct 22, 2019

PumpkinSeed added 5 commits December 12, 2019 08:52

Merge branch 'master' into I6616-sync-changes-lock

df59a3b

Merge branch 'I6616-sync-changes-lock' of github.com:PumpkinSeed/cons…

b622bbc

…ul into I6616-sync-changes-lock

Merge branch 'master' into I6616-sync-changes-lock

7fcb75e

Add fixes for dereference error of the service

2f67b16

Fix return vet error

2bef858

PumpkinSeed mentioned this pull request Dec 18, 2019

Catalog.Deregister rpc error making call: Unknown check #6947

Closed

hanshasselberg requested changes Jan 15, 2020

View reviewed changes

hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 15, 2020

stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 15, 2020

Merge branch 'master' into I6616-sync-changes-lock

a8fe5e1

PumpkinSeed added 4 commits January 23, 2020 15:44

Add requested changes

35a6279

Merge branch 'master' into I6616-sync-changes-lock

780f7fc

Resolve ACL version check

4e276eb

Service sync fix for return whether version mismatch

ddccf8d

dnephin closed this May 10, 2021

jacksontj mentioned this pull request Oct 5, 2021

Release state lock before catalog operations in State.SyncChanges #6616

Open

wjordan mentioned this pull request Mar 2, 2022

Release state lock before catalog operations in State.SyncChanges #12485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix State.SyncChanges mutex Lock/Unlock #6619

Fix State.SyncChanges mutex Lock/Unlock #6619

PumpkinSeed commented Oct 13, 2019 •

edited

Loading

hashicorp-cla commented Oct 13, 2019 •

edited

Loading

ShimmerGlass commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

ShimmerGlass commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

jacksontj commented Oct 29, 2019

ShimmerGlass commented Oct 30, 2019

PumpkinSeed commented Dec 11, 2019

codecov bot commented Dec 13, 2019 •

edited

Loading

jacksontj commented Jan 6, 2020 •

edited

Loading

hanshasselberg left a comment

hanshasselberg Jan 15, 2020

PumpkinSeed commented Jan 24, 2020

freddygv commented Feb 6, 2020

PumpkinSeed commented Feb 14, 2020 •

edited

Loading

jacksontj commented Apr 15, 2020

freddygv commented Apr 23, 2020

jacksontj commented Jun 10, 2020

jacksontj commented Jul 10, 2020

freddygv commented Jul 10, 2020

jacksontj commented Jul 29, 2020

freddygv commented Aug 11, 2020 •

edited

Loading

jacksontj commented Apr 21, 2021

hanshasselberg commented Apr 22, 2021

dnephin commented May 10, 2021

Fix State.SyncChanges mutex Lock/Unlock #6619

Fix State.SyncChanges mutex Lock/Unlock #6619

Conversation

PumpkinSeed commented Oct 13, 2019 • edited Loading

hashicorp-cla commented Oct 13, 2019 • edited Loading

ShimmerGlass commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

ShimmerGlass commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

PumpkinSeed commented Oct 14, 2019

jacksontj commented Oct 29, 2019

ShimmerGlass commented Oct 30, 2019

PumpkinSeed commented Dec 11, 2019

codecov bot commented Dec 13, 2019 • edited Loading

Codecov Report

jacksontj commented Jan 6, 2020 • edited Loading

hanshasselberg left a comment

Choose a reason for hiding this comment

hanshasselberg Jan 15, 2020

Choose a reason for hiding this comment

PumpkinSeed commented Jan 24, 2020

freddygv commented Feb 6, 2020

PumpkinSeed commented Feb 14, 2020 • edited Loading

jacksontj commented Apr 15, 2020

freddygv commented Apr 23, 2020

jacksontj commented Jun 10, 2020

jacksontj commented Jul 10, 2020

freddygv commented Jul 10, 2020

jacksontj commented Jul 29, 2020

freddygv commented Aug 11, 2020 • edited Loading

jacksontj commented Apr 21, 2021

hanshasselberg commented Apr 22, 2021

dnephin commented May 10, 2021

PumpkinSeed commented Oct 13, 2019 •

edited

Loading

hashicorp-cla commented Oct 13, 2019 •

edited

Loading

codecov bot commented Dec 13, 2019 •

edited

Loading

jacksontj commented Jan 6, 2020 •

edited

Loading

PumpkinSeed commented Feb 14, 2020 •

edited

Loading

freddygv commented Aug 11, 2020 •

edited

Loading