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

perf: Update frameworks to use compiler sharding #1900

Merged
merged 31 commits into from Apr 1, 2022
Merged

perf: Update frameworks to use compiler sharding #1900

merged 31 commits into from Apr 1, 2022

Conversation

willbeason
Copy link
Member

@willbeason willbeason commented Mar 7, 2022 •
edited

Integrate Gatekeeper with the compiler sharding frameworks change.

I'm working on measuring performance changes in real clusters.

@willbeason willbeason force-pushed the compiler-sharding branch 2 times, most recently from cb066f9 to bc3b47b Compare March 8, 2022 18:30
@willbeason willbeason marked this pull request as ready for review March 17, 2022 19:43
@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2022 •
edited

Codecov Report

Merging #1900 (e10d761) into master (6c58cad) will increase coverage by 0.24%.
The diff coverage is 52.91%.

@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
+ Coverage   53.23%   53.48%   +0.24%     
==========================================
  Files         100      103       +3     
  Lines        8986     9092     +106     
==========================================
+ Hits         4784     4863      +79     
- Misses       3809     3852      +43     
+ Partials      393      377      -16
Flag Coverage Δ
unittests 53.48% <52.91%> (+0.24%) ⬆️

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

Impacted Files Coverage Δ
pkg/audit/audit_cache_lister.go 0.00% <0.00%> (ø)
pkg/audit/controller.go 0.00% <0.00%> (ø)
pkg/audit/manager.go 0.00% <0.00%> (ø)
pkg/audit/result.go 0.00% <0.00%> (ø)
pkg/controller/config/process/excluder.go 12.76% <0.00%> (+1.00%) ⬆️
pkg/controller/constraint/constraint_controller.go 5.66% <0.00%> (ø)
pkg/controller/sync/opadataclient.go 0.00% <0.00%> (ø)
pkg/controller/sync/sync_controller.go 0.00% <0.00%> (ø)
pkg/watch/set.go 0.00% <0.00%> (ø)
pkg/controller/config/config_controller.go 64.25% <28.57%> (-0.79%) ⬇️
... and 17 more

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 97e9dd8...e10d761. Read the comment docs.

@willbeason
Copy link
Member Author

Fixes #1916

@willbeason willbeason marked this pull request as draft March 17, 2022 21:49
@willbeason
Copy link
Member Author

Fixes #1916

@willbeason willbeason marked this pull request as ready for review March 21, 2022 16:11
sozercan
sozercan reviewed Mar 21, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
maxsmythe
maxsmythe reviewed Mar 22, 2022
View reviewed changes
pkg/controller/config/config_controller.go Outdated Show resolved Hide resolved
pkg/webhook/policy.go Show resolved Hide resolved
pkg/target/target.go Outdated Show resolved Hide resolved
@maxsmythe
Copy link
Contributor

It looks like the External Data tests are failing due to problems calling Match()... unexpected nil values?

Will Beason added 5 commits March 22, 2022 07:18
Start frameworks upgrade
913932f 
Still need to implement audit-from-cache

Signed-off-by: Will Beason <willbeason@google.com>
Start fixing tests
ffdfca0 
Signed-off-by: Will Beason <willbeason@google.com>
Fix all but one test
ce1bb6e 
Signed-off-by: Will Beason <willbeason@google.com>
Update frameworks
bbb41b8 
Change-Id: I862a247815c1dbe7fde90200d4a8ef1e4e27a2c7

Signed-off-by: Will Beason <willbeason@google.com>
Start adding hook to list objects
d370ac6 
Signed-off-by: Will Beason <willbeason@google.com>
Will Beason added 2 commits March 23, 2022 10:23
Fix matching when Namespace is not specified
9916ae6 
Cover many previously-untested cases for matching when Namespace is not
available, or when the object to be matched is a Namespace.

Also clarify that typos in scope match criteria match everything.

Now there is no need for Target to get a cached Namespace for
Namespaces.

Signed-off-by: Will Beason <willbeason@google.com>
Merge branch 'main' into compiler-sharding
d1cbbcd
Will Beason added 3 commits March 23, 2022 11:10
Fix linter errors
e916382 
Signed-off-by: Will Beason <willbeason@google.com>
Remove Rego library
1a3bc5f 
Since changes to frameworks make gatekeeper-specific Rego library
unnecessary.

Also add many tests for Target.

Signed-off-by: Will Beason <willbeason@google.com>
Add back switch case types
e10d761 
Signed-off-by: Will Beason <willbeason@google.com>
maxsmythe
maxsmythe reviewed Mar 24, 2022
View reviewed changes
pkg/target/target.go Show resolved Hide resolved
pkg/mutation/match/match.go Outdated Show resolved Hide resolved
Will Beason added 2 commits March 24, 2022 09:31
Start fixing race condition
7026ebd 
Signed-off-by: Will Beason <willbeason@google.com>
Fix race condition
0903c81 
Also add race condition test for Set replacement.

Signed-off-by: Will Beason <willbeason@google.com>
maxsmythe
maxsmythe requested changes Mar 25, 2022
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.

This LGTM after we get a read lock for AssignedStringList in operations.go

pkg/operations/operations.go Outdated Show resolved Hide resolved
Use two-level RW locking for assignedOperations
616043e 
Signed-off-by: Will Beason <willbeason@google.com>
maxsmythe
maxsmythe approved these changes Mar 25, 2022
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!!! 🎊

ritazh
ritazh reviewed Mar 28, 2022
View reviewed changes
go.mod
@@ -24,7 +24,8 @@ require (
github.com/onsi/ginkgo v1.16.5
github.com/onsi/gomega v1.17.0
github.com/open-policy-agent/cert-controller v0.2.0
github.com/open-policy-agent/frameworks/constraint v0.0.0-20220218180203-c2a0d8cdf85a
github.com/open-policy-agent/frameworks/constraint v0.0.0-20220319020711-acd776435a23
github.com/open-policy-agent/opa v0.37.2
Copy link
Member

Choose a reason for hiding this comment

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

Where does GK still reference opa directly? I thought everything is moving to framework?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like in two places (target and target test):

pkg/target/target.go:	"github.com/open-policy-agent/opa/storage"
pkg/target/target_test.go:	"github.com/open-policy-agent/opa/storage"

This is because the TargetHandler interface is currently expecting an OPA path for the storage key. IIRC this is b/c the path object has some formatting methods that are nice, but essentially it's a list of strings otherwise.

If we want to fully remove this dependency again, maybe a follow-on piece of work to remove this from the TargetHandler interface?

https://github.com/open-policy-agent/frameworks/blob/00eadf686a2876291b032335b6993dc41d770398/constraint/pkg/handler/handler.go#L19-L30

Copy link
Member

Choose a reason for hiding this comment

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

ah now I remember. it makes sense to address this as a follow up work.

ritazh
ritazh reviewed Mar 29, 2022
View reviewed changes
pkg/audit/manager.go Outdated Show resolved Hide resolved
ritazh
ritazh reviewed Mar 29, 2022
View reviewed changes
pkg/audit/manager.go Outdated Show resolved Hide resolved
Resolve reviewer comments
f1f66e6 
Signed-off-by: Will Beason <willbeason@google.com>
@willbeason willbeason requested a review from ritazh March 29, 2022 15:54
ritazh
ritazh approved these changes Mar 30, 2022
View reviewed changes
Copy link
Member

@ritazh ritazh left a comment •
edited

Choose a reason for hiding this comment

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

LGTM
Thanks @willbeason! 🎉
pending load test results from @sozercan

@willbeason
Copy link
Member Author

LGTM Thanks @willbeason! 🎉 pending load test results from @sozercan

Yay!

sozercan
sozercan approved these changes Apr 1, 2022
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.

LGTM, seeing around ~1.5x-4x decrease in webhook cpu and memory usage (depending on # of template/constraints)

@ritazh ritazh merged commit 2c07f12 into open-policy-agent:master Apr 1, 2022
@sozercan sozercan mentioned this pull request Apr 15, 2022
Merged
@willbeason willbeason mentioned this pull request Jun 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsmythe maxsmythe maxsmythe approved these changes

@sozercan sozercan sozercan approved these changes

@ritazh ritazh ritazh approved these changes

@shomron shomron Awaiting requested review from shomron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@willbeason @codecov-commenter @maxsmythe @sozercan @ritazh