proxy/hybrid: add locking around userspace map #13847
Conversation
|
I'm not actually sure how to easily test this; can somebody either test it out for me, or describe how I would do so? Thanks! |
|
LGTM |
pkg/proxy/hybrid/proxy.go
Outdated
| p.lock.Lock() | ||
| defer p.lock.Unlock() | ||
|
|
||
| p.usingUserspace = make(map[types.NamespacedName]bool) |
There was a problem hiding this comment.
Is this already set up in line 43?
There was a problem hiding this comment.
we swap out a new map every time (in response to @pecameron)
pkg/proxy/hybrid/proxy.go
Outdated
| @@ -24,7 +25,8 @@ type HybridProxier struct { | |||
|
|
|||
| // TODO(directxman12): figure out a good way to avoid duplicating this information | |||
| // (it's saved in the individual proxies as well) | |||
| usingUserspace map[types.NamespacedName]struct{} | |||
| usingUserspace map[types.NamespacedName]bool | |||
| lock sync.Mutex | |||
There was a problem hiding this comment.
What data are you locking with this lock?
pkg/proxy/hybrid/proxy.go
Outdated
| @@ -24,7 +25,8 @@ type HybridProxier struct { | |||
|
|
|||
| // TODO(directxman12): figure out a good way to avoid duplicating this information | |||
| // (it's saved in the individual proxies as well) | |||
| usingUserspace map[types.NamespacedName]struct{} | |||
| usingUserspace map[types.NamespacedName]bool | |||
There was a problem hiding this comment.
please leave this as struct{} -- it's the recommended method doing sets (it takes up no space for the value).
pkg/proxy/hybrid/proxy.go
Outdated
| Name: endpoint.Name, | ||
| } | ||
| if _, ok := p.usingUserspace[svcName]; !ok { | ||
| if !p.isUsingUserspace(endpoint.Namespace, endpoint.Name) { |
There was a problem hiding this comment.
I would just lock around this entire block, instead of locking on each operation like this.
pkg/proxy/hybrid/proxy.go
Outdated
| p.lock.Lock() | ||
| defer p.lock.Unlock() | ||
|
|
||
| p.usingUserspace = make(map[types.NamespacedName]bool) |
There was a problem hiding this comment.
we swap out a new map every time (in response to @pecameron)
dcbw
force-pushed
the
hybrid-proxy-locking
branch
from
bfea7fa
to
6563222
Compare
|
@DirectXMan12 updated; now with just the locking and went back to struct{} maps. PTAL thanks! |
|
[test] |
|
[merge] |
dcbw
force-pushed
the
hybrid-proxy-locking
branch
from
6563222
to
c0ee5dd
Compare
|
re-[test] this issue #13067 |
openshift-bot
added
the
needs-rebase
The map may be accessed concurrently from different goroutines so we need to lock it. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1444125
dcbw
force-pushed
the
hybrid-proxy-locking
branch
from
c0ee5dd
to
826b674
Compare
|
Evaluated for origin test up to 826b674 |
|
[merge] |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1004/) (Base Commit: b6b92db) |
openshift-bot
removed
the
needs-rebase
|
[merge] flaked on #13942 (logs: https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1004/) |
|
[merge] #13977 |
|
Evaluated for origin merge up to 826b674 |
|
continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/526/) (Base Commit: 09ea247) |
|
[merge] flake is #13942 |
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1444125
@openshift/networking @knobunc @DirectXMan12 @eparis