mixerfilter: add protobuf caching#11469
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
| out := &cacheData{ | ||
| serviceConfigAny: make(map[cacheKey]*types.Any), | ||
| } | ||
| // TODO: unclear whether this enumerates all services, including external? |
There was a problem hiding this comment.
All services in the service registry [k8s, service entries, etc.]
rshriram
left a comment
There was a problem hiding this comment.
In general looks okay.
Question: doesn't the mixer attributes encode the source UID or destination UID, which are specific to every pod? If that is not the case, then its actually far more easier to simply cache the LDS/RDS outputs right?
| } | ||
| // TODO: unclear whether this enumerates all services, including external? | ||
| for _, service := range in.Push.ServiceByHostname { | ||
| for _, disable := range []bool{true, false} { |
There was a problem hiding this comment.
instead of looping through true and false and then inbound and outbound, why not just write four calls ?
for _, service := range in.Push.ServiceByHostname {
// policy disabled
out.serviceConfigAny[cacheKey{"inbound", true, ... }] = util.MessageToAny(...)
out.serviceConfigAny[cacheKey{"outbound", true, ... }] = util.MessageToAny(...)
// policy enabled
out.serviceConfigAny[cacheKey{"inbound", false, ... }] = util.MessageToAny(...)
out.serviceConfigAny[cacheKey{"outbound", false, ... }] = util.MessageToAny(...)
}
seems more readable to me.
There was a problem hiding this comment.
Updated it. Does this look more reasonable? I'm trying to provision for more complicated keys (e.g. telemetry on/off options) and seems like a bunch of base cases is easier to maintain (similar to recommendation to use test case structs).
|
@rshriram caching per proxy would lead to a massive cache. It's far better to let the filter decide how to cache. In this case, we compute a cache key from a proxy, and then do a cache lookup. |
|
|
||
| // fallback to regular call | ||
| if config == nil { | ||
| config = util.MessageToAny(buildRouteConfig(direction, istioConfig, push, disablePolicy, hostname)) |
There was a problem hiding this comment.
@mandarjog I am adding a fallback to pass the tests. Something in pilot does not always call OnPrecompute which causes the tests to fail.
|
/test istio-unit-tests |
1 similar comment
|
/test istio-unit-tests |
|
@rshriram any reason for precompute not being called? The fallback if |
|
/test istio-unit-tests There is no concurrent write access issue, since the cache is not written to after precompute. |
|
Friendly ping @douglas-reid @mandarjog . |
|
@kyessenov PR needs rebase |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kyessenov If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mandarjog @douglas-reid Please take another look. Recent changes about client retries do not affect this PR. Those changes affect client configs, while this change affects service configs. |
|
@kyessenov: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Supersedes #11137
Fixes #9800
/assign @mandarjog
/assign @douglas-reid
Signed-off-by: Kuat Yessenov kuat@google.com