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.

Sign up for GitHub

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

Jump to bottom

feat: support multiple sync sources #2852

Merged
merged 60 commits into from
Aug 30, 2023
Merged

feat: support multiple sync sources #2852

merged 60 commits into from
Aug 30, 2023

Conversation

acpana
Copy link
Contributor

@acpana acpana commented Jun 29, 2023 •
edited

This patch introduces a CacheManager type to manage any constraint framework data caching needs.

@acpana acpana requested a review from julianKatz June 29, 2023 18:50
julianKatz
julianKatz reviewed Jun 29, 2023
View reviewed changes
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/syncutil/aggregator/aggregator.go Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
julianKatz
julianKatz reviewed Jul 6, 2023
View reviewed changes
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager_test.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager_test.go Outdated Show resolved Hide resolved
pkg/controller/cachemanager/cachemanager_test.go Outdated Show resolved Hide resolved
@acpana acpana force-pushed the acpana/cmt-replay-6-r-clean branch from 447c40d to 57ca58d Compare July 6, 2023 22:43
acpana added 15 commits July 6, 2023 23:13
@acpana
use cm
11bb964 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
replace process excluder in cm
5314889 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
nit: dont pass target
c382961 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
use cm.WipeData
ba56451 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
inject cm
b746411 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
move cm, make syncc submodule
a455772 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
introduce cm config
4750a94 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
refactor: make cm watch aware
c3745a9 
dirty, refactor: make cm watch aware

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty: frontload with gvk aggregator

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty: use gvk aggregator for config

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, refactor: replayData is async

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, refactor: wipe and replay are async

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, refactor: dual controllers

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, refactor: nil check props

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, refactor: cm.Start

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

dirty, fix: mark gvks to sync

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

tests

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
review: push all cache mgmt in bckgr
8e70e79 
dirty, squash: config_c changes

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
add cacheManager as a runnable
1cd478a 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
rename makeUpdates, comments
9eb8f35 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
pass reader for cm
7fbb1c4 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
review: consumer defines source, others
ba4981d 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
lint fixes & more
cf11ef1 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
after origin/master rebase
21b92e9 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana force-pushed the acpana/cmt-replay-6-r-clean branch from 57ca58d to 21b92e9 Compare July 6, 2023 23:24
@acpana
Copy link
Contributor Author

acpana commented Jul 10, 2023

@anlandu FYI

@acpana
spli tests into unit, e2e
29d747f 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
julianKatz
julianKatz reviewed Aug 18, 2023
View reviewed changes
main.go Outdated Show resolved Hide resolved
@acpana
review: naming, comments
505de58 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from maxsmythe August 23, 2023 00:15
maxsmythe
maxsmythe reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

1 naming nit and a few write lock acquisitions that should be read lock acquisitions, after that LGTM

pkg/audit/audit_cache_lister.go Outdated Show resolved Hide resolved
pkg/cachemanager/aggregator/aggregator.go Outdated Show resolved Hide resolved
pkg/cachemanager/aggregator/aggregator.go Outdated Show resolved Hide resolved
pkg/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/cachemanager/cachemanager.go Outdated Show resolved Hide resolved
pkg/syncutil/stats_reporter.go Outdated Show resolved Hide resolved
pkg/syncutil/stats_reporter.go Outdated Show resolved Hide resolved
@acpana
review: use read locks
21f7d57 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from maxsmythe August 23, 2023 21:49
@acpana
use failure injector in config_c test
10e62bf 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested review from ritazh and sozercan August 23, 2023 22:59
maxsmythe
maxsmythe approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for sticking with it!

@acpana
Copy link
Contributor Author

acpana commented Aug 28, 2023

@sozercan @ritazh PTAL when you have a chance. Happy to walk y'all thru if needed.

AFAIK, there are no concerns from @anlandu 💯 .

@anlandu
Copy link
Member

anlandu commented Aug 28, 2023

✅

@acpana
remove stale expectations
0637f6e 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
sozercan
sozercan approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@sozercan sozercan left a comment •
edited

Choose a reason for hiding this comment

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

Thank you! LGTM pending tests

@acpana
fix flaky test
6ca3fa5 
AFAICT, the root cause is an internal controller race in config_c
that triggers an additional reconcile request to be sent down the
previous wrapped channel which will block as it's not being read.

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
ritazh
ritazh reviewed Aug 30, 2023
View reviewed changes
pkg/cachemanager/cachemanager.go
c.needToList = false
relistStopChan = make(chan struct{})

go c.replayGVKs(ctx, gvksToRelist, relistStopChan)
Copy link
Member

@ritazh ritazh Aug 30, 2023 •
edited

Choose a reason for hiding this comment

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

This for loop can be spawning a lot of new goroutines. If the new goroutine blocks indefinitely or takes a long time to execute, you could run into a goroutine leak and/or lock contention. is this a concern?

Copy link
Member

Choose a reason for hiding this comment

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

no error handling for c.replayGVKs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goroutine should stop when the relistStopChan is closed or ctx is Done. For now, we log the errors if there's a replay issue with a GVK but we also have a retry mechanism in the func to protect against transient errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge this in for now but please feel free to reach out if things don't seem quite right.

ritazh
ritazh reviewed Aug 30, 2023
View reviewed changes
pkg/cachemanager/cachemanager.go

// stop any goroutines that were relisting before
// as we may no longer be interested in those gvks
close(relistStopChan)
Copy link
Member

Choose a reason for hiding this comment

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

we are closing relistStopChan without checking whether it has already been closed, which could result in a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the invariant here should be that this codesite is the only place that opens/ closes the channel here so we should be good IIRC.

@acpana acpana merged commit a7e3b7c into open-policy-agent:master Aug 30, 2023
15 checks passed
This was referenced Aug 31, 2023
Closed
Merged
Merged
salaxander pushed a commit to salaxander/gatekeeper that referenced this pull request Sep 13, 2023
@acpana @salaxander
feat: support multiple sync sources (open-policy-agent#2852)
0e46b19 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Mattes83 pushed a commit to Mattes83/gatekeeper that referenced this pull request Oct 25, 2023
@acpana @Mattes83
feat: support multiple sync sources (open-policy-agent#2852)
6375217 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritazh ritazh ritazh left review comments

@julianKatz julianKatz julianKatz approved these changes

@sozercan sozercan sozercan approved these changes

@maxsmythe maxsmythe maxsmythe approved these changes

Assignees
No one assigned
Labels
SyncSet Related
Projects
Gatekeeper Workboard
Status: ✅ Done
Milestone
v3.14.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@acpana @codecov-commenter @anlandu @sozercan @ritazh @julianKatz @maxsmythe