Fix Service object remove from resource map from route controller#549
Conversation
This commit fetches the services that were referenced by the route which has now been deleted, and removes those objects from the resource map. In order to facilitate this, the commit introduces a provider cache for kubernetes that stores mappings between kubernetes objects - for now it stores the map between [tls/http]route -> backend service references. So that once the [tls/http]route is deleted we have the services that it referenced. Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Codecov Report
@@ Coverage Diff @@
## main #549 +/- ##
==========================================
- Coverage 60.98% 60.61% -0.37%
==========================================
Files 45 47 +2
Lines 5564 5721 +157
==========================================
+ Hits 3393 3468 +75
- Misses 1968 2034 +66
- Partials 203 219 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
thanks for working on this ! |
|
Will there not be a need to store Service objects any more in the resource map post 413? I'm looking at gateway/internal/gatewayapi/translator.go Line 66 in 0be1aef The |
I was referring to the fact that we won't need to handle
and update the watchable map with the same key, so we dont need to deal with deletes in the provider anymore
|
LukeShu
left a comment
There was a problem hiding this comment.
There are some things I think should be changed, but if we want to get this in to 0.2.0, I'm OK merging as-is and cleaning it up later.
| // routeToServicesMappings maintains a mapping of a Route object, | ||
| // and the Services it references. For instance | ||
| // HTTPRoute/ns1/route1 -> { ns1/svc1, ns1/svc2, ns2/svc1 } | ||
| // TLSRoute/ns1/route1 -> { ns1/svc1, ns2/svc2 } | ||
| routeToServicesMappings map[string]sets.String |
There was a problem hiding this comment.
Can we use typed structs, instead of having to wrangle strings?
type routeIdentifier struct {
Kind string
Namespace string
Name string
}
routeToServicesMappings map[routeIdentifier]Set[types.NamespacedName](it's easy enough to write a type-parametarized Set[T] (example) but if we don't want to do that, map[types.NamespacedName]struct{} would do in a pinch)
| // providerCache maintains additional mappings related to Kubernetes provider | ||
| // resources. The mappings are regularly updated from the reconcilers based | ||
| // on the existence of the object in the Kubernetes datastore. | ||
| type providerCache struct { |
There was a problem hiding this comment.
Calling this a "cache" is misleading. It isn't really a cache, it's a reference-counting machine.
| func (p *providerCache) isServiceReferredByRoutes(service string) bool { | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
|
|
||
| for _, svcs := range p.routeToServicesMappings { | ||
| if svcs.Has(service) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
This is O(n), and that makes me uneasy. Would it make sense to have the primary key be the service, rather than the route?
There was a problem hiding this comment.
It will probably get uglier if we use the reverse mapping with the service as P.K.
As we need to query this primarily - Find all services for a Route r1 which was deleted, while reconciling the deleted Route r1. With service as the P.K. we'll have to traverse all the Services (and find a route match) which will be more than the number of TLS/HTTP Routes. (Note that with route as the P.K we are filtering based on kind too)
I started off by adding multiple mappings like route-to-services and service-to-routes, but seemed like we're optimizing at the cost of double the memory, so stuck with the route-to-services mapping only.
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
|
hey @chauhanshubham can you rm the |
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
Signed-off-by: Shubham Chauhan <shubham@tetrate.io>
This commit fetches the services that were referenced by the route which has now been deleted, and removes those objects from the resource map. In order to facilitate this, the commit introduces a provider cache for kubernetes that stores mappings between kubernetes objects - for now it stores the map between [tls/http]route -> backend service references.
So that once the [tls/http]route is deleted we have the services that it referenced.
Fixes: #536
Signed-off-by: Shubham Chauhan shubham@tetrate.io