perf: optimize activation of bundles with no inter-bundle path overlap #7155
Conversation
ast/compile.go
Outdated
| // WithPathConflictsCheckRoot enables checking path conflicts from the specified root instead | ||
| // of the top root node. This would enable optimizting path conflict checks during bundle | ||
| // activation if a bundle already defines its own root paths. | ||
| func (c *Compiler) WithPathConflictsCheckRoot(rootPaths []string) *Compiler { |
There was a problem hiding this comment.
Any reason to use singular "root" for the name while the args and the property says "roots"?
There was a problem hiding this comment.
Good catch! Will change to use the plural form.
Signed-off-by: Shiqi Yang <[email protected]>
Signed-off-by: Ashutosh Narkar <[email protected]>
Signed-off-by: Ashutosh Narkar <[email protected]>
…-policy-agent#7153) Fixes open-policy-agent#7151 Signed-off-by: Anders Eknert <[email protected]>
Signed-off-by: Peter Macdonald <[email protected]>
Signed-off-by: Shiqi Yang <[email protected]>
Signed-off-by: Shiqi Yang <[email protected]>
Signed-off-by: Shiqi Yang <[email protected]>
sqyang94
force-pushed
the
check-conflict-from-child-node
branch
from
179ecc8 to
5cd286b
Compare
|
Force pushing to sign off my commits... |
Signed-off-by: Shiqi Yang <[email protected]>
ast/compile.go
Outdated
| // WithPathConflictsCheckRoots enables checking path conflicts from the specified root instead | ||
| // of the top root node. This would enable optimizting path conflict checks during bundle | ||
| // activation if a bundle already defines its own root paths. |
There was a problem hiding this comment.
The compiler is agnostic to concepts such as bundle activation. I suggest we drop the second sentence, or reformulate it to say something general about performance optimization. E.g.
| // WithPathConflictsCheckRoots enables checking path conflicts from the specified root instead | |
| // of the top root node. This would enable optimizting path conflict checks during bundle | |
| // activation if a bundle already defines its own root paths. | |
| // WithPathConflictsCheckRoots enables checking path conflicts from the specified root instead | |
| // of the top root node. Limiting conflict checks to a known set of roots, such as bundle roots, | |
| // improves performance. |
There was a problem hiding this comment.
Maybe we should also mention the expected format of the paths: /-separated, data root document excluded.
ast/compile_test.go
Outdated
| @@ -1963,7 +1966,7 @@ p[r] := 2 if { r := "foo" }`, | |||
| return false, fmt.Errorf("unexpected error") | |||
| } | |||
| return false, nil | |||
| }) | |||
| }).WithPathConflictsCheckRoots([]string{"badrules"}) | |||
There was a problem hiding this comment.
Perhaps this is deserving of its own test, so that we assert the behavior of WithPathConflictsCheck() and WithPathConflictsCheckRoots() in isolation from each other.
There was a problem hiding this comment.
Will do. I'll add a new unit test for WithPathConflictsCheckRoots specifically.
ast/conflicts.go
Outdated
| node = child | ||
| } | ||
|
|
||
| if nodeNotFound { |
There was a problem hiding this comment.
Nit: we could just do node = node.Child(String(key)) in the above loop and check node == nil here instead?
ast/conflicts.go
Outdated
| if nodeNotFound { | ||
| // could not find the node from the AST (e.g. `path` is from a data file) | ||
| // then no conflict is possible | ||
| continue |
There was a problem hiding this comment.
Do we have a test case that covers this scenario? If not, to be exhaustive in our testing, we should add one.
There was a problem hiding this comment.
Yes. It's implicitly tested with the WithPathConflictsCheckRoots test. Since I'm going to add a brand new test case, I'll make sure to comment on how this is tested.
plugins/bundle/plugin_test.go
Outdated
| if status.Code != errCode { | ||
| t.Fatalf("Expected status code to be %s, found %s", errCode, status.Code) | ||
| } | ||
| if !strings.Contains(status.Message, "detected overlapping") { |
There was a problem hiding this comment.
Nit: but we say "detected overlapping roots" in the error message below.
One way to make sure the check and error message always are in agreement across code changes is to extract the expected value into a var, e.g.:
if exp := "detected overlapping roots"; !strings.Contains(status.Message, exp) {
t.Fatalf(`Expected status message to contain "%s", found %s`, exp, status.Message)
}Signed-off-by: Shiqi Yang <[email protected]>
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I think we can merge this? Provided that it's rebased and the conflicts resolved. |
|
@sqyang94 can you rebase this on top of latest main and resolve the conflicts? Once that's done we can merge this 🙂 |
Why the changes in this PR are needed?
When multiple bundles are loaded into OPA using the bundle plugin, it takes a long time to active the bundles according to the logs. Profiling results has shown that most of the activation time is spent in checking path conflicts.
This PR avoids unnecessary path conflict checking when no root overlaps are guaranteed. This will reduce the number of expensive deep copies and hence improve bundle activation time and CPU utilization.
Resolves #7144.
What are the changes in this PR?
If a bundle has a manifest file with
rootsdefined, then during path conflict checking, only check the sub-AST from the root path instead of the entire AST.Notes to assist PR review:
#7144 includes a detailed explanation and context on this change.
Further comments:
I wasn't able to find a good way of directly testing the change (i.e. testing that only subtree was checked when roots are defined in the manifest). Instead, I added some bundle plugin tests with different scenarios to make sure conflicts are correctly checked still.
Ideas on how to directly test this approach are welcome :)