MESH-5878: Admiral does not create envoy filter on client onboardings #362
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #362 +/- ##
==========================================
+ Coverage 63.25% 63.74% +0.49%
==========================================
Files 91 91
Lines 12158 12218 +60
==========================================
+ Hits 7690 7788 +98
+ Misses 4053 4016 -37
+ Partials 415 414 -1 ☔ View full report in Codecov by Sentry. |
adilfulara
force-pushed
the
MESH-5878
branch
4 times, most recently
from
a538076 to
d2ffc07
Compare
- Introduce RoutingPolicyProcessor interface - Inject RoutingPolicyService into DependencyHandler Tech-Debt: - Increase coverage for routingpolicy_handler.go Signed-off-by: Adil Fulara <[email protected]>
adilfulara
force-pushed
the
MESH-5878
branch
from
d2ffc07 to
d2b5b9b
Compare
- Add unit tests for RoutingPolicyService.DeltaUpdate() api Signed-off-by: Adil Fulara <[email protected]>
- Rename Signed-off-by: Adil Fulara <[email protected]>
- Coverage Signed-off-by: Adil Fulara <[email protected]>
| if oldRP != nil { | ||
| oldEnv := common.GetRoutingPolicyEnv(oldRP) | ||
| r.RemoteRegistry.AdmiralCache.RoutingPolicyCache.Delete(id, oldEnv, oldRP.Name) | ||
| } |
There was a problem hiding this comment.
Why are we deleting from the cache?
There was a problem hiding this comment.
We are deleting the old resource as part of the update event. Should we be retaining older resources?
There was a problem hiding this comment.
Since we dont have routing policies in ppd or production we may not need this. Plus cache ( in memory) gets populated every time admiral restarts.
There was a problem hiding this comment.
the reason i am deleting is if the env label is changed the cache will be corrupted which could cause problems for subsequent updates. ( ex: change is admiral.io/env label value )
There was a problem hiding this comment.
@adilfulara is it ok to refresh the map if there are error in the above loop? For example, what if all the calls to createOrUpdateEnvoyFilter return failure. Is it ok to mutate the map?
| // for each destination, identify the newRP. | ||
| for _, dependent := range newDestinations { | ||
| // if the dependent is in the mesh, then get the newRP | ||
| policies := r.RemoteRegistry.AdmiralCache.RoutingPolicyCache.GetForIdentity(dependent) |
There was a problem hiding this comment.
sorry please correct me if I'm wrong, but why are we trying to get routingPolicies of the destination/client?
There was a problem hiding this comment.
newDestinations is the new service that the client onboarded to. So we need to get all routing policies associated with the destination so we can apply it at the client side.
- clean up commented code Signed-off-by: Adil Fulara <[email protected]>
| log.Warnf(LogFormat, eventType, "routingpolicy", newRP.Name, remoteController.ClusterID, "RoutingPolicy disabled for cluster") | ||
| continue | ||
| } | ||
| for _, dependent := range dependents { |
There was a problem hiding this comment.
Do you think multi threading will help here, assuming there are 100's of dependents for services like QBO
There was a problem hiding this comment.
We could definitely benefit from usage of go-routines but i chose not do this for following reasons
- currently there are no such use cases.
- each client to service mapping will come in as a separate update due to the DependencyRecord CRD, there wont be much benefit from usage of goroutine usage.
- only for enabled clusters.
LMK what you prefer.
| if oldRP != nil { | ||
| oldEnv := common.GetRoutingPolicyEnv(oldRP) | ||
| r.RemoteRegistry.AdmiralCache.RoutingPolicyCache.Delete(id, oldEnv, oldRP.Name) | ||
| } |
There was a problem hiding this comment.
Since we dont have routing policies in ppd or production we may not need this. Plus cache ( in memory) gets populated every time admiral restarts.
| log.Infof(LogFormat, eventType, "routingpolicy", rp.Name, "", | ||
| fmt.Sprintf("finished processing routing policy for new destination=%s in delta update", dependent)) | ||
| } | ||
| //} |
There was a problem hiding this comment.
nitpick, can this be removed?
| log.Errorf(LogErrFormat, eventType, "envoyfilter", filter, rc.ClusterID, err1) | ||
| err = common.AppendError(err, err1) | ||
| } else { | ||
| log.Infof(LogFormat, eventType, "envoyfilter", filter, rc.ClusterID, "deleting from cache") |
There was a problem hiding this comment.
this log line message might be inaccurate?
Task
Description
What does this change do and why?
Thank you!