fix: race condition (#50) #51

d-kuro · 2020-06-03T05:46:59Z

refs: #50

If you import the code from #52, the race condition will be gone.

# refs: https://github.com/argoproj/gitops-engine/pull/52
$ git status
On branch feature/fix-race-condition
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   pkg/sync/sync_context_test.go
        modified:   pkg/utils/kube/kubetest/mock.go

no changes added to commit (use "git add" and/or "git commit -a")

$ go test ./... --race
?       github.com/argoproj/gitops-engine       [no test files]
?       github.com/argoproj/gitops-engine/agent [no test files]
ok      github.com/argoproj/gitops-engine/pkg/cache     (cached)
?       github.com/argoproj/gitops-engine/pkg/cache/mocks       [no test files]
ok      github.com/argoproj/gitops-engine/pkg/diff      (cached)
?       github.com/argoproj/gitops-engine/pkg/engine    [no test files]
ok      github.com/argoproj/gitops-engine/pkg/health    (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync      (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/common       (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/hook (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/hook/helm    (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/ignore       (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/resource     (cached)
ok      github.com/argoproj/gitops-engine/pkg/sync/syncwaves    (cached)
?       github.com/argoproj/gitops-engine/pkg/utils/errors      [no test files]
ok      github.com/argoproj/gitops-engine/pkg/utils/exec        (cached)
?       github.com/argoproj/gitops-engine/pkg/utils/io  [no test files]
?       github.com/argoproj/gitops-engine/pkg/utils/json        [no test files]
ok      github.com/argoproj/gitops-engine/pkg/utils/kube        (cached)
?       github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest       [no test files]
?       github.com/argoproj/gitops-engine/pkg/utils/testing     [no test files]
?       github.com/argoproj/gitops-engine/pkg/utils/text        [no test files]
ok      github.com/argoproj/gitops-engine/pkg/utils/tracing     (cached)

Among many functions, using sync.Mutex to secure a lock was complex and difficult.
I took the approach of using sync.Map.
I don't think this is the best response, but I think it's good that race conditions are gone.
If you have any comments, please feel free to do so!

codecov · 2020-06-03T05:51:13Z

Codecov Report

Merging #51 into master will decrease coverage by 0.09%.
The diff coverage is 57.33%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   52.49%   52.40%   -0.10%     
==========================================
  Files          25       25              
  Lines        2585     2727     +142     
==========================================
+ Hits         1357     1429      +72     
- Misses       1100     1146      +46     
- Partials      128      152      +24

Impacted Files	Coverage Δ
pkg/utils/kube/ctl.go	`5.78% <0.00%> (+0.09%)`	⬆️
pkg/cache/cluster.go	`50.89% <51.76%> (-2.68%)`	⬇️
pkg/cache/resource.go	`72.97% <53.84%> (-12.22%)`	⬇️
pkg/sync/sync_context.go	`64.38% <86.27%> (+1.45%)`	⬆️

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 16598d5...01379a4. Read the comment docs.

ash2k · 2020-06-06T11:53:40Z

Hey, you beat me to it! I was also working on fixing these race conditions. I think your branch does more and does it better as I haven't tried to simplify things as much - I'm not that familiar with the codebase. You can see what I've done here if you are interested master...ash2k:ash2k/fixes

ash2k · 2020-06-06T11:54:22Z

pkg/cache/cluster.go

 type apiMeta struct {
+	lock            sync.Mutex
 	namespaced      bool
 	resourceVersion string


nit: this could be just a sync.Value to simplify things.

Fixed
8c8713d

ash2k · 2020-06-06T11:55:54Z

pkg/cache/cluster.go

 }

+type Resources struct {
+	sync.Map


It would probably be better to make it a field rather than embed - currently all the map's methods are added to Resources type and perhaps this is not what you want?

ash2k · 2020-06-06T11:57:01Z

pkg/cache/cluster.go

-func runSynced(lock sync.Locker, action func() error) error {
-	lock.Lock()
-	defer lock.Unlock()
+func runSynced(action func() error) error {


This can be just removed

Thanks!
29224aa

ash2k · 2020-06-06T12:02:24Z

pkg/sync/sync_context.go

 						// if there is anything that needs deleting, we are at best now in pending and
 						// want to return and wait for sync to be invoked again
+						runStateMutex.Lock()
 						runState = pending


I think apart from the data race that you are fixing here, there is also a logic error. One goroutine may set runState to failed but then another one to pending or the other way around. This non-deterministic behavior is probably not right, or is it? Please see 1e0e792 for a different approach.

I cherry-picked from your commit.
Thank you!

6a762b3
c5d445f

alexmt · 2020-06-06T23:24:13Z

pkg/sync/sync_context.go

 				sc.setResourceResult(task, "", common.OperationFailed, fmt.Sprintf("Failed to delete: %v", err))
 				terminateSuccessful = false
 			} else {
-				sc.setResourceResult(task, "", common.OperationSucceeded, fmt.Sprintf("Deleted"))


alexmt · 2020-06-06T23:24:39Z

pkg/sync/sync_context.go

 	{
 		var wg sync.WaitGroup
 		for _, task := range pruneTasks {
+			task := task


This line is not needed as task is passed to the closure (line 737/742) and it uses t instead.

I think that's been fixed with this commit.
c5d445f

alexmt

@ash2k , @d-kuro Thanks a lot for fixing the race conditions and improving the quality! The GitOps engine code is lifting and shifting of Argo CD code and requires polishing.

Changes in sync_context.go are fixing real bugs. Great catch!

I apologize in advance for the delay in review. The updated code is very performance sensitive and requires careful testing. Before merging it please let us fix two issues that block 1.6 GA release:

Next, we will need to run performance testing on internal Argo CD instances. So it might take a few days.

d-kuro · 2020-06-10T02:21:38Z

@ash2k @alexmt
Thank you for your comment!
I'm going to work on the code fix that was pointed out to me over the weekend.

ash2k · 2020-06-10T02:56:56Z

@d-kuro 👍

I suggest to always run go test -race. ArgoCD's tests too!

alexmt · 2020-06-11T04:38:40Z

v1.6 testing looks good so far. We just want to run it internally a few more days and create 1.6 GA release. As soon as 1.6 is out I will test this PR's performance.

ash2k · 2020-06-11T10:56:27Z

pkg/sync/sync_context.go

 	if runState == successful && createTasks.Any(func(t *syncTask) bool { return t.needsDeleting() }) {
 		var wg sync.WaitGroup
 		for _, task := range createTasks.Filter(func(t *syncTask) bool { return t.needsDeleting() }) {
+			task := task


This line is not needed, same as above.

I understand.
Thank you comments.

I think that's been fixed with this commit.
c5d445f

ash2k · 2020-06-11T10:57:17Z

pkg/sync/sync_context.go

 	var pruneTasks syncTasks

 	for _, task := range tasks {
+		task := task


I wonder why is this needed?

I think that's been fixed with this commit.
c5d445f

Make runState a new type rather than a type alias

================== WARNING: DATA RACE Write at 0x00c0000d0b40 by goroutine 83: github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks.func5.1() /gitops-engine/pkg/sync/sync_context.go:786 +0x68c Previous write at 0x00c0000d0b40 by goroutine 84: github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks.func5.1() /gitops-engine/pkg/sync/sync_context.go:786 +0x68c Goroutine 83 (running) created at: github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks.func5() /gitops-engine/pkg/sync/sync_context.go:778 +0x165 github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks() /gitops-engine/pkg/sync/sync_context.go:807 +0xb32 github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).Sync() /gitops-engine/pkg/sync/sync_context.go:265 +0x1b3d github.com/argoproj/gitops-engine/pkg/sync.TestSyncFailureHookWithFailedSync() /gitops-engine/pkg/sync/sync_context_test.go:532 +0x4e5 testing.tRunner() /usr/local/Cellar/go/1.14.3/libexec/src/testing/testing.go:991 +0x1eb Goroutine 84 (running) created at: github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks.func5() /gitops-engine/pkg/sync/sync_context.go:778 +0x165 github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).runTasks() /gitops-engine/pkg/sync/sync_context.go:807 +0xb32 github.com/argoproj/gitops-engine/pkg/sync.(*syncContext).Sync() /gitops-engine/pkg/sync/sync_context.go:265 +0x1b3d github.com/argoproj/gitops-engine/pkg/sync.TestSyncFailureHookWithFailedSync() /gitops-engine/pkg/sync/sync_context_test.go:532 +0x4e5 testing.tRunner() /usr/local/Cellar/go/1.14.3/libexec/src/testing/testing.go:991 +0x1eb ==================

ash2k

I've left a few more comments. I think it would be very helpful to document which object/method is thread safe and which is not.

I think it might be not the best idea to use sync.Map because it makes it harder to write code:

all those nil checks all over the place
lost type safety in loops (.Range() vs for)
introduces a chance of "logical" data races where two method don't exclude each other but both work on the same data concurrently (e.g. iterate over a map and mutate it) and that leads to unforeseen results.

It might be better to just use a mutex and a normal map. WDYT? Just my two cents.

ash2k · 2020-06-15T23:20:09Z

pkg/cache/cluster.go

+		return version, nil
+	}
+
+	return "", fmt.Errorf("stored type is invalid in resourceVersion")


This cannot happen and if it does it should be ok to panic because... it should never happen?
I'd do this instead

str, _ := a.resourceVersion.Load().(string) return str, nil

This will not error out and it also preserves current semantics where the field is an empty string if it was never set. It's also nicer to not need to check the error in every place where this method is called.

ash2k · 2020-06-15T23:24:12Z

pkg/cache/cluster.go

+	r.Store(key, value)
+}
+
+func (r *Resources) LoadResources(key kube.ResourceKey) (*Resource, error) {


Same as above - panicking should be ok if something unexpected is in this private field. And the error is not checked in a few places below. Why return it then? :)

ash2k · 2020-06-15T23:24:52Z

pkg/cache/cluster.go

+	c.apisMeta.Store(key, value)
+}
+
+func (c *clusterCache) LoadApisMeta(key schema.GroupKind) (*apiMeta, error) {


Same as above - panicking should be ok if something unexpected is in this private field.

ash2k · 2020-06-15T23:25:14Z

pkg/cache/cluster.go

+	c.nsIndex.Store(key, value)
+}
+
+func (c *clusterCache) LoadNsIndex(key string) (*Resources, error) {


Same as above - panicking should be ok if something unexpected is in this private field.

ash2k · 2020-06-15T23:26:54Z

pkg/cache/cluster.go

+	if err != nil {
+		return err
+	}
+	if info != nil {


This if can be inverted to avoid so much indentation. https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

ash2k · 2020-06-15T23:30:45Z

pkg/cache/cluster.go

-	c.lock.Lock()
-	defer c.lock.Unlock()
 	// before doing any work, check once again now that we have the lock, to see if it got
 	// synced between the first check and now


This comment and code above can be deleted now.

ash2k · 2020-06-15T23:33:11Z

pkg/cache/cluster.go

-		if len(res.OwnerRefs) == 0 {
-			resources[res.ResourceKey()] = res
+func (c *clusterCache) GetNamespaceTopLevelResources(namespace string) *Resources {
+	res, _ := c.LoadNsIndex(namespace)


No error check here too. That's why I'm suggesting to just let it panic if the field contents is corrupted. The next line will panic anyway.

d-kuro · 2020-06-19T01:49:34Z

@ash2k

I think it might be not the best idea to use sync.Map because it makes it harder to write code:

I understand.
I too think the Range() of the sync map is not good and should be improved.
Initially, we decided to use sync.Map because of the complexity of locking on the map, but the Now that we've cleaned up the code to access it, we can use the mutex and map access approach with I think we can switch it up.

I'm going to try to change it back from sync map to map once.
Thanks!

alexmt · 2020-06-23T19:02:09Z

pkg/cache/cluster.go

-			if len(ns) == 0 {
-				delete(c.nsIndex, key.Namespace)
+func (c *clusterCache) onNodeRemoved(key kube.ResourceKey) error {
+	existing, err := c.resources.LoadResources(key)


I agree with @ash2k regarding sync.Map usage.

I think we still need to have a lock here to make sure resources map and namespace index are synchronized.

alexmt · 2020-06-23T19:03:54Z

pkg/cache/cluster.go

+	c.nsIndex.Store(key, value)
+}
+
+func (c *clusterCache) LoadNsIndex(key string) (*Resources, error) {


I think LoadNsIndex is a private method. Please rename to loadNsIndex.

alexmt · 2020-06-23T19:04:37Z

pkg/cache/cluster.go

+	return cnt
+}
+
+func (c *clusterCache) StoreNsIndex(key string, value *Resources) {


Same as LoadNsIndex. Please rename to storeNsIndex.

alexmt · 2020-06-24T15:22:02Z

Hello @d-kuro . I'm very interested in completing and merging this PR. Please, let me know if you have time to continue working on it.
I can help. E.g. I can send PR to your fork that addresses sync.Map related comments. let me know

d-kuro · 2020-06-27T14:07:14Z

@alexmt
Sorry for the late reply.

I'm going to try migrating from SyncMap to map, as I wrote in my comment below.
#51 (comment)

I may ask you to follow up if you try and get stuck, but I'll try to do it myself for now.
Thanks for your concern!

sonarqubecloud · 2020-06-27T16:27:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

No Coverage information
0.0% Duplication

d-kuro · 2020-06-27T17:35:37Z

@ash2k cc: @alexmt

I'm trying to work on migration from sync.Map to map.
but it suffers from the race condition of map range access and writing.

fatal error: concurrent map iteration and map write

Using RLock for map range access and Lock for write is simple but for example the following code.
https://github.com/argoproj/gitops-engine/blob/master/pkg/cache/cluster.go#L241-L249

Deadlocked this function, because we are deleting the map in range access that requires a read lock.
Is there a good way to write this without using SyncMap?

// c.resources is "map[kube.ResourceKey]*Resource"

// Need RWMutex.RLock()
for key := range c.resources {
	if key.Kind != gk.Kind || key.Group != gk.Group || ns != "" && key.Namespace != ns {
		continue
	}

	if _, ok := objByKey[key]; !ok {
		// Need RWMutex.Lock()
		// "delete(c.resources, key)" in a function
		c.onNodeRemoved(key)
	}
}

ash2k · 2020-06-28T05:37:26Z

@d-kuro Perhaps replaceResourceCache() can be left as is but then watchEvents() which calls it can grab the write lock before calling it?

I'd like to suggest to split this PR as it's getting quite big. Perhaps make all the changes to pkg/cache/cluster.go in a separate PR?

I've also looked at the clusterCache more closely and I'm wondering why was it written using list+watch+manual cache (resources field) and not using informers? It might be a good idea to refactor clusterCache to use informers instead - they support notifications, indexes and are thoroughly tested by kubernetes itself. WDYT?

alexmt · 2020-06-29T19:16:50Z

Thanks a lot for continue working on it @d-kuro !

I've looked only more time where replaceResourceCache is used. The method is executed in two places and both have write lock:

gitops-engine/pkg/cache/cluster.go

Line 443 in ce9616a

c.replaceResourceCache(api.GroupKind, info.resourceVersion, items, ns)

gitops-engine/pkg/cache/cluster.go

Line 371 in ce9616a

c.replaceResourceCache(gk, "", []unstructured.Unstructured{}, ns)

I suspect race condition happens in tests only: TestNamespaceModeReplace. I hope I'm not missing anything. If this is the case we just need to use lock in TestNamespaceModeReplace test.

Agree with @ash2k about moving cluster.go changes into separate PRs.

alexmt · 2020-06-29T19:20:38Z

@ash2k we could not use informers framework because of informers cache the whole resource manifest. We need only resources reference and parent links. Switching to informers would increase memory consumption. That was the main reason to manually use List/Watch APIs.
But I agree that we are partially re-implementing functionality that exists in informers framework.

ash2k · 2020-06-30T00:08:28Z

@alexmt

we could not use informers framework because of informers cache the whole resource manifest.

This is the default behavior, but it is configurable. If only certain fields are required, a custom Go type can be crafted to use for any watched resource type. That way only the needed fields are unmarshaled and nothing else. For example, there is a whole package just for working with metadata of objects. Tuned client and informer are available there.

However, memory consumption will still be higher in the above case vs how things currently work because e.g. all annotations will be stored in the informer's cache. Thank you for answering the question :)

alexmt · 2020-08-28T20:33:03Z

Changes of this PR got split into several other PRs and race detection is now enabled in the project. @d-kuro , @ash2k thanks a lot for driving it!

alswl · 2021-10-28T09:04:16Z

@alexmt Hi, is there any progress? The master branch still not contains the commits in this PR.

alswl · 2021-11-03T08:44:06Z

c.lock and c.listSemaphore had two deadlock situation in big clusters(more than 500 api-resources).

c.lock holds too big scope in EnsureSyncd() function, we should remove c.lock, using sync.Map.
semaphore in c.listSemaphore using defer caused lock callback() function body, and in watchEvents() the body contains runSync().

d-kuro force-pushed the feature/fix-race-condition branch from 2f102ac to d78cf09 Compare June 4, 2020 16:44

d-kuro changed the title ~~[WIP] Fix race condition~~ fix: race condition Jun 4, 2020

d-kuro changed the title ~~fix: race condition~~ fix: race condition (#50) Jun 4, 2020

d-kuro marked this pull request as ready for review June 4, 2020 16:54

ash2k reviewed Jun 6, 2020

View reviewed changes

alexmt reviewed Jun 6, 2020

View reviewed changes

ash2k reviewed Jun 11, 2020

View reviewed changes

d-kuro added 8 commits June 15, 2020 15:14

fix variable scope

744380b

migrate apisMeta map to sync.map

819c075

gosimple

308d10f

migrate resources map to sync.map

4708ea1

refactor resources map

4d5fda8

migrate nsIndex map to sync.map

7763c02

fix resourceVersion race confition

ccf2f92

remove unused lock

7d7a436

d-kuro force-pushed the feature/fix-race-condition branch from d78cf09 to 7d7a436 Compare June 15, 2020 06:15

d-kuro and others added 5 commits June 15, 2020 15:16

add race option for make test

3760c4b

Using atomic.Value

8c8713d

remove runSynced func

29224aa

More type safety

6a762b3

Make runState a new type rather than a type alias

ash2k reviewed Jun 15, 2020

View reviewed changes

alexmt reviewed Jun 23, 2020

View reviewed changes

d-kuro added 2 commits June 27, 2020 23:41

Stopping the export of functions

cb9927f

migrate sync.map to map for apisMeta

01379a4

alexmt closed this Aug 28, 2020

alswl mentioned this pull request Nov 15, 2021

fix: 0.4.1 Reduce lock contention, narrowing lock space, pprof, watch retry timeout #350

Closed

fix: race condition (#50) #51

fix: race condition (#50) #51

Uh oh!

Conversation

d-kuro commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ash2k commented Jun 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-kuro Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmt left a comment

Choose a reason for hiding this comment

Uh oh!

d-kuro commented Jun 10, 2020

Uh oh!

ash2k commented Jun 10, 2020

Uh oh!

alexmt commented Jun 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-kuro Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash2k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-kuro commented Jun 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

d-kuro commented Jun 3, 2020 •

edited

Loading

codecov bot commented Jun 3, 2020 •

edited

Loading

d-kuro Jun 15, 2020 •

edited

Loading

d-kuro Jun 15, 2020 •

edited

Loading

ash2k commented Jun 28, 2020 •

edited

Loading

alexmt commented Jun 29, 2020 •

edited

Loading

alexmt commented Jun 29, 2020 •

edited

Loading

alswl commented Nov 3, 2021 •

edited

Loading