Resolve various data races in translation and validation #7207

kdorosh · 2022-09-21T15:48:05Z

Description

Ran Gloo in a large environment under race detector, which identified some races which are fixed in this PR:

Updates validation server to use a shared translator that is synchronized which prevents data races across translation
Updates locking in validation server to prevent concurrent access to last snapshot
Updates RateLimitConfig client construction to happen during init() so it cannot race with global scheme access by k8s shared informers

Checklist:

I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
I followed guidelines laid out in the Gloo Edge contribution guide
I opened a draft PR or added the work in progress label if my PR is not ready for review
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves solo-io#7202
resolves solo-io/skv2#375
resolves solo-io#7213

kdorosh · 2022-09-21T15:48:26Z

/skip-changelog for now

kdorosh · 2022-09-21T15:57:09Z

/kick docker build flake? https://storage.googleapis.com/solo-public-build-logs/logs.html?buildid=3283b0be-0b04-4280-a5f1-bbc82ce7b19d

elcasteel · 2022-09-21T16:02:13Z

This will definitely fix the problem with concurrent writes in plugins. I'm pretty confident that it won't deadlock. Are we also planning to share a single translator between validation and translation?

kdorosh · 2022-09-21T16:12:08Z

@elcasteel I think that's a good idea and the right direction forward; torn on whether we want to make that specific change in our backport however. Still reasoning about the shared memory here

github-actions · 2022-09-21T16:15:53Z

Visit the preview URL for this PR (updated for commit 1ebcd00):

https://gloo-edge--pr7207-lock-translation-vsvuq7kv.web.app

_{(expires Fri, 30 Sep 2022 17:47:18 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

elcasteel · 2022-09-21T16:25:38Z

@elcasteel I think that's a good idea and the right direction forward; torn on whether we want to make that specific change in our backport however. Still reasoning about the shared memory here

We shared the translator before we started reusing plugins so I don't believe there are any other issues with shared memory.
I think if we are trying to avoid waiting on locks the best set up would be to give the validation webhook its own translator (and own validator) and then to have a separate validator for the proxy reconciler that can share a translator with translation.
I think it's worth scale testing with a single translator for everything.

projects/gloo/pkg/syncer/sanitizer/route_replacing_sanitizer.go

projects/gloo/pkg/translator/translator.go

solo-changelog-bot · 2022-09-21T19:22:35Z

Issues linked to changelog:
solo-io#7202

yuval-k · 2022-09-21T20:54:22Z

projects/gloo/pkg/translator/translator.go

@@ -68,6 +71,8 @@ func (t *translatorInstance) Translate(
 	proxy *v1.Proxy,
 ) (envoycache.Snapshot, reporter.ResourceReports, *validationapi.ProxyReport) {
 	// setup tracing, logging
+	t.lock.Lock()


how much time does translation take? could this be a problem if this take a long time?

that and potential scaling concerns are why I don't want to rush this change out. I'd like to run this in your environment and benchmark more thoroughly before merging.

tldr; i don't know

If we see a big slowdown after this, we can give the validation webhook one translator and translation+proxy_reconciler another translator which will separate the two truly independent uses of the lock.

Agreed, I thought that's what translatorInstance was for, to instantiate one for each.

Yes; although I think it's a bit better to have validation and core translation use the same if possible. For example, think about two plugins resolving DNS async .. imagine if one copy of translator has DNS entries and the other does not, or something to this effect. Validation translation may succeed using its cached DNS data but then fail on the real translation. This type of bug is why I think they should be the same.

kdorosh · 2022-09-22T06:29:52Z

/kick look into https://storage.googleapis.com/solo-public-build-logs/logs.html?buildid=25a10be6-8fc4-4a45-92ba-e3bad5e9d94c

elcasteel · 2022-09-22T13:29:42Z

projects/gloo/pkg/translator/translator.go

-		routesProto = append(routesProto, resource.NewEnvoyResource(routeCfg))
+		// important to clone, ownership will be transferred to goroutine serving xds snapshot.
+		// marshalling a proto to serve over grpc xds can mutate it, which can race with other reads (e.g. hashing last seen snapshot)
+		routesProto = append(routesProto, resource.NewEnvoyResource(proto.Clone(routeCfg)))


This is called from two places and I don't think we need the clone when it's called from route_replacing_sanitizer so we could pass in a bool to decide whether or not to clone (had this in a previous iteration of the clone removal change).

@yuval-k is considering another solution to this that updates hashstructure to be read-only so that we can keep these as they are, rather than add clones

…translation

This reverts commit 256767e.

This reverts commit 7df16e1.

solo-changelog-bot · 2022-09-23T15:49:45Z

Issues linked to changelog:
solo-io#7202
solo-io/skv2#375
solo-io#7213

sam-heilbron · 2022-09-23T18:22:33Z

projects/gloo/pkg/syncer/sanitizer/route_replacing_sanitizer.go

@@ -49,6 +49,8 @@ var (
 )

 type RouteReplacingSanitizer struct {
+	// note to devs: this can be called in parallel by the validation webhook and main translation loops at the same time


sam-heilbron

LGTM on first pass!

* Add lock for translation * Share translator to reuse caches * Add lock for sanitizers * Add note for devs * No need to be too safe before we need it * No need for RWMutex * Add changelog * Avoid global scheme in broken skv2 function * Need to use global scheme, but register it before race can happen * Import the right scheme * Fix data race in with serving xds * Fix data race with status reporter * Adding changelog file to new location * Deleting changelog file from old location * Lock on validator sync * Remove extra lock * Add more locks * Need write lock * Copy write namespaces to avoid race * Update changelog * Revert "Fix data race with status reporter" This reverts commit 256767e. * Revert "Fix data race in with serving xds" This reverts commit 7df16e1. Co-authored-by: changelog-bot <changelog-bot>

* Resolve various data races in translation and validation (#7207) * Add lock for translation * Share translator to reuse caches * Add lock for sanitizers * Add note for devs * No need to be too safe before we need it * No need for RWMutex * Add changelog * Avoid global scheme in broken skv2 function * Need to use global scheme, but register it before race can happen * Import the right scheme * Fix data race in with serving xds * Fix data race with status reporter * Adding changelog file to new location * Deleting changelog file from old location * Lock on validator sync * Remove extra lock * Add more locks * Need write lock * Copy write namespaces to avoid race * Update changelog * Revert "Fix data race with status reporter" This reverts commit 256767e. * Revert "Fix data race in with serving xds" This reverts commit 7df16e1. Co-authored-by: changelog-bot <changelog-bot> * Fix merge * Optimize rate limit hash calculation (#7198) * gencode with unrelease solokit * update solokit * codegen * changelog * empty * Adding changelog file to new location * Deleting changelog file from old location Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot> Co-authored-by: Kevin Dorosh <[email protected]> * Use new hashstructure (#7227) * update protoc-gen-ext * code gen * remove remaining usages * make update-licenses * changelog * update solo-apis 9746539fc625fed45d1d146c4e0c1359e70f8036 - gloo-edge-safe-hasher * go mod tidy * fix merge solo kit v0.30.4 * Go mod tidy Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Kevin Dorosh <[email protected]> * Codegen * Consolidate changelogs * Prefer fix changelog type Co-authored-by: Yuval Kohavi <[email protected]> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

Add lock for translation

a0f4c7d

kdorosh added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Sep 21, 2022

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Sep 21, 2022

kdorosh removed the keep pr updated signals bulldozer to keep pr up to date with base branch label Sep 21, 2022

Share translator to reuse caches

52d0603

Add lock for sanitizers

eed90e8

Add note for devs

e7fac96

elcasteel reviewed Sep 21, 2022

View reviewed changes

projects/gloo/pkg/syncer/sanitizer/route_replacing_sanitizer.go Outdated Show resolved Hide resolved

No need to be too safe before we need it

3fd22c3

yuval-k reviewed Sep 21, 2022

View reviewed changes

projects/gloo/pkg/translator/translator.go Outdated Show resolved Hide resolved

Kevin Dorosh added 3 commits September 21, 2022 15:19

No need for RWMutex

d4fd937

Merge branch 'master' into lock_translation

1fc2e0d

Add changelog

a9508d8

yuval-k reviewed Sep 21, 2022

View reviewed changes

yuval-k added a commit that referenced this pull request Sep 21, 2022

adding #7207

f103506

Kevin Dorosh added 4 commits September 21, 2022 17:31

Avoid global scheme in broken skv2 function

35676f4

Need to use global scheme, but register it before race can happen

6845a5e

Import the right scheme

bccff71

Fix data race in with serving xds

e9650a8

kdorosh mentioned this pull request Sep 22, 2022

Data Race marshalling while streaming and ??? #7210

Closed

kdorosh force-pushed the lock_translation branch from 6bb3b35 to 3989982 Compare September 22, 2022 05:55

Fix data race with status reporter

51b4f7b

kdorosh force-pushed the lock_translation branch from 3989982 to 51b4f7b Compare September 22, 2022 05:59

elcasteel reviewed Sep 22, 2022

View reviewed changes

kdorosh mentioned this pull request Sep 22, 2022

fix scheme data race solo-io/skv2#375

Open

changelog-bot and others added 11 commits September 22, 2022 15:35

Adding changelog file to new location

f43d6c1

Deleting changelog file from old location

a1530a4

Lock on validator sync

d484bbe

Merge branch 'lock_translation' of github.com:solo-io/gloo into lock_…

292c7c7

…translation

Remove extra lock

dfef0d5

Add more locks

5c20304

Need write lock

c087cab

Copy write namespaces to avoid race

e3eb8cd

Update changelog

014122d

Revert "Fix data race with status reporter"

0933bdd

This reverts commit 256767e.

Revert "Fix data race in with serving xds"

87f30be

This reverts commit 7df16e1.

kdorosh mentioned this pull request Sep 23, 2022

data race validator and event loop #7215

Closed

kdorosh changed the title ~~Add lock for translation~~ Resolve various data races in translation and validation Sep 23, 2022

Merge branch 'master' into lock_translation

1ebcd00

yuval-k approved these changes Sep 23, 2022

View reviewed changes

kdorosh added keep pr updated signals bulldozer to keep pr up to date with base branch and removed work in progress signals bulldozer to keep pr open (don't auto-merge) labels Sep 23, 2022

sam-heilbron reviewed Sep 23, 2022

View reviewed changes

sam-heilbron approved these changes Sep 23, 2022

View reviewed changes

soloio-bulldozer bot merged commit c780bcc into master Sep 23, 2022

soloio-bulldozer bot deleted the lock_translation branch September 23, 2022 18:53

kdorosh mentioned this pull request Sep 23, 2022

[v1.12] Backport data race and speed optimizations #7230

Merged

8 tasks

github-actions bot mentioned this pull request Nov 14, 2024

[Migrated] Resolve various data races in translation and validation solo-io/gloo#7207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve various data races in translation and validation #7207

Resolve various data races in translation and validation #7207

kdorosh commented Sep 21, 2022 •

edited by solo-changelog-bot bot

Loading

kdorosh commented Sep 21, 2022 •

edited

Loading

kdorosh commented Sep 21, 2022

elcasteel commented Sep 21, 2022

kdorosh commented Sep 21, 2022

github-actions bot commented Sep 21, 2022 •

edited

Loading

elcasteel commented Sep 21, 2022

solo-changelog-bot bot commented Sep 21, 2022

yuval-k Sep 21, 2022

kdorosh Sep 21, 2022

elcasteel Sep 21, 2022

EItanya Sep 21, 2022

kdorosh Sep 21, 2022

kdorosh commented Sep 22, 2022

elcasteel Sep 22, 2022

kdorosh Sep 22, 2022

solo-changelog-bot bot commented Sep 23, 2022

sam-heilbron Sep 23, 2022

sam-heilbron left a comment

Resolve various data races in translation and validation #7207

Resolve various data races in translation and validation #7207

Conversation

kdorosh commented Sep 21, 2022 • edited by solo-changelog-bot bot Loading

Description

Checklist:

kdorosh commented Sep 21, 2022 • edited Loading

kdorosh commented Sep 21, 2022

elcasteel commented Sep 21, 2022

kdorosh commented Sep 21, 2022

github-actions bot commented Sep 21, 2022 • edited Loading

elcasteel commented Sep 21, 2022

solo-changelog-bot bot commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdorosh commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solo-changelog-bot bot commented Sep 23, 2022

Choose a reason for hiding this comment

sam-heilbron left a comment

Choose a reason for hiding this comment

kdorosh commented Sep 21, 2022 •

edited by solo-changelog-bot bot

Loading

kdorosh commented Sep 21, 2022 •

edited

Loading

github-actions bot commented Sep 21, 2022 •

edited

Loading