Skip to content

refactor: introduce CacheManager#2785

Merged
sozercan merged 24 commits into
open-policy-agent:masterfrom
acpana:acpana/cmt-synccontroller
Jun 15, 2023
Merged

refactor: introduce CacheManager#2785
sozercan merged 24 commits into
open-policy-agent:masterfrom
acpana:acpana/cmt-synccontroller

Conversation

@acpana
Copy link
Copy Markdown
Contributor

@acpana acpana commented May 22, 2023 •
edited
Loading

meta

  • in support of the sync set proposal
  • CacheManager (CM) will be built on to encapsulate the replay of the cache in upcoming PRs

Factors out data mgmt & replication from the sync_controller.

acpana added 8 commits May 18, 2023 22:09
@acpana
start with an inplace refactor
27e8d13 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
make pure func
4a50836 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
pull data ops out of sync controller
a07928e 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
inject tracker in cmt
60cd403 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
inject process excluder in cmt
c11c39b 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
pass tracker, excluder from config controller
ba4317d 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
add glog to cmt
dc78ba6 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
polish
768c2bf 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested review from julianKatz and maxsmythe May 22, 2023 18:00
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 22, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@2413cf8). Click here to learn what that means.
Patch coverage: 48.33% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master    #2785   +/-   ##
=========================================
  Coverage          ?   53.58%           
=========================================
  Files             ?      132           
  Lines             ?    11550           
  Branches          ?        0           
=========================================
  Hits              ?     6189           
  Misses            ?     4882           
  Partials          ?      479
Flag Coverage Δ
unittests 53.58% <48.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/constraint/constraint_controller.go 5.68% <ø> (ø)
...onstrainttemplate/constrainttemplate_controller.go 59.23% <ø> (ø)
pkg/controller/expansion/expansion_controller.go 52.29% <ø> (ø)
pkg/syncutil/opadataclient.go 0.00% <ø> (ø)
pkg/syncutil/stats_reporter.go 21.25% <5.08%> (ø)
pkg/syncutil/cachemanager/cachemanager.go 89.58% <89.58%> (ø)
pkg/controller/config/config_controller.go 63.51% <90.90%> (ø)
pkg/readiness/object_tracker.go 84.09% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
julianKatz
julianKatz reviewed May 22, 2023
View reviewed changes
Comment thread pkg/syncutil/cmt/cmt.go Outdated
@maxsmythe
Copy link
Copy Markdown
Contributor

maxsmythe commented May 23, 2023 •
edited
Loading

The PR title should avoid using an abbreviation for a net-new concept (CMT) as it will make release notes harder to read

@acpana
Copy link
Copy Markdown
Contributor Author

acpana commented May 24, 2023

@anlandu fyi

@acpana acpana changed the title refactor: introduce cmt refactor: introduce CacheManager May 24, 2023
acpana added 4 commits May 24, 2023 17:58
@acpana
rename cmt to cachemanager
afab5d1 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
rename funcs, no locks for now now
ffd8d01 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
return err on process exclusion err
a7e477a 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
just return err
b8e7bd3 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from julianKatz May 24, 2023 18:23
julianKatz
julianKatz reviewed May 30, 2023
View reviewed changes
Comment thread pkg/syncutil/cachemanager/cachemanager.go Outdated
acpana added 3 commits June 5, 2023 23:50
@acpana
refactor: export FakeOpa
b04a59f 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
add cm tests
f056ec5 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
add err cases
2431c9d 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
acpana
acpana commented Jun 6, 2023
View reviewed changes
Comment thread pkg/controller/sync/sync_controller.go Outdated
acpana
acpana commented Jun 6, 2023
View reviewed changes
Comment thread pkg/controller/sync/sync_controller.go Outdated
acpana added 2 commits June 6, 2023 13:04
@acpana
Apply suggestions from code review
28b4466 
removes todos

Signed-off-by: alex <8968914+acpana@users.noreply.github.com>
@acpana
lint
4858f5d 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
acpana
acpana commented Jun 6, 2023
View reviewed changes
Comment thread pkg/syncutil/cachemanager/cachemanager_test.go
@@ -0,0 +1,138 @@
package cachemanager
Copy link
Copy Markdown
Contributor Author

@acpana acpana Jun 6, 2023

Choose a reason for hiding this comment

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

Let's remember that config_controller_test, by definition, tests the sync controller, which will test that the CM handles the current uses cases as well.

So the new tests here are meant to only target the cachemanager in as much isolation as possible from controllers.

Another point to remember is that as we build the background replay process more into the cachemanager, the tests will change to cover the new functionality.

julianKatz
julianKatz reviewed Jun 6, 2023
View reviewed changes
Comment thread pkg/syncutil/cachemanager/cachemanager.go Outdated
julianKatz
julianKatz reviewed Jun 6, 2023
View reviewed changes
Comment thread pkg/syncutil/cachemanager/cachemanager.go Outdated
@acpana
review feedback
b4b8bcf 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from julianKatz June 6, 2023 22:57
@acpana acpana marked this pull request as ready for review June 6, 2023 23:35
@acpana acpana requested review from ritazh and sozercan June 6, 2023 23:35
@acpana
Copy link
Copy Markdown
Contributor Author

acpana commented Jun 6, 2023

@maxsmythe @ritazh @sozercan PTAL when y'all have some time! 🙏🏼

@julianKatz
Copy link
Copy Markdown
Contributor

julianKatz commented Jun 8, 2023

@maxsmythe @ritazh @sozercan PTAL when y'all have some time! 🙏🏼

ping :)

maxsmythe
maxsmythe requested changes Jun 9, 2023
View reviewed changes
Comment thread pkg/controller/sync/sync_controller.go Outdated
Comment thread pkg/syncutil/cachemanager/cachemanager.go Outdated
Comment thread pkg/syncutil/stats_reporter.go
Comment thread pkg/syncutil/stats_reporter.go Outdated
acpana added 4 commits June 9, 2023 21:20
@acpana
log in Observe()
f04af8a 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
review: don't pass in log
4aea7fa 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
review: don't pass in reporter
e816124 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana
review: rename reportMetrics
0ff4b54 
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from maxsmythe June 9, 2023 22:06
maxsmythe
maxsmythe approved these changes Jun 10, 2023
View reviewed changes
Copy link
Copy Markdown
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

@acpana
Copy link
Copy Markdown
Contributor Author

acpana commented Jun 12, 2023

@ritazh or @sozercan kindly let me know if y'all have any questions or concerns.

sozercan
sozercan approved these changes Jun 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, thanks! LGTM

@sozercan sozercan merged commit 2835519 into open-policy-agent:master Jun 15, 2023
This was referenced Nov 22, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sozercan sozercan sozercan approved these changes

@julianKatz julianKatz julianKatz approved these changes

@maxsmythe maxsmythe maxsmythe approved these changes

@ritazh ritazh Awaiting requested review from ritazh

Assignees

No one assigned

Labels

SyncSet Related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@acpana @codecov-commenter @maxsmythe @julianKatz @sozercan