Fix HTTPRoute parentRefs updates#1229
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request updates AddOrUpdateHTTPRoute to clean up old gateway route mappings when an HTTPRoute is updated, and adds a corresponding unit test. The review feedback suggests optimizing this cleanup by retrieving the old route version and removing the route only from its previously referenced gateways, avoiding an inefficient
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for gatewayKey, routeSet := range s.gatewayRoutes { | ||
| routeSet.Delete(key) | ||
| if routeSet.IsEmpty() { | ||
| delete(s.gatewayRoutes, gatewayKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
Efficiency Improvement Opportunity
Currently, when an HTTPRoute is added or updated, the code iterates over all gateways in s.gatewayRoutes to remove the route from any old gateways:
for gatewayKey, routeSet := range s.gatewayRoutes {
routeSet.Delete(key)
if routeSet.IsEmpty() {
delete(s.gatewayRoutes, gatewayKey)
}
}This operation is
Recommended Solution
Instead of iterating over all gateways, we can retrieve the old version of the HTTPRoute before overwriting it, and only remove the route from the gateways that were actually referenced in its previous ParentRefs. This reduces the cleanup complexity to
Here is how the refactored AddOrUpdateHTTPRoute function could look:
func (s *store) AddOrUpdateHTTPRoute(httpRoute *gatewayv1.HTTPRoute) error {
key := fmt.Sprintf("%s/%s", httpRoute.Namespace, httpRoute.Name)
s.httpRouteMutex.Lock()
defer s.httpRouteMutex.Unlock()
// 1. Retrieve the old route before overwriting it
oldRoute := s.httpRoutes[key]
s.httpRoutes[key] = httpRoute
// 2. Only clean up from the old route's parent gateways
if oldRoute != nil {
for _, parentRef := range oldRoute.Spec.ParentRefs {
if parentRef.Kind != nil && *parentRef.Kind == "Gateway" {
gatewayName := string(parentRef.Name)
gatewayNamespace := oldRoute.Namespace
if parentRef.Namespace != nil {
gatewayNamespace = string(*parentRef.Namespace)
}
gatewayKey := fmt.Sprintf("%s/%s", gatewayNamespace, gatewayName)
if routeSet, exists := s.gatewayRoutes[gatewayKey]; exists {
routeSet.Delete(key)
if routeSet.IsEmpty() {
delete(s.gatewayRoutes, gatewayKey)
}
}
}
}
}
// 3. Add to the new parent gateways
for _, parentRef := range httpRoute.Spec.ParentRefs {
if parentRef.Kind != nil && *parentRef.Kind == "Gateway" {
gatewayName := string(parentRef.Name)
gatewayNamespace := httpRoute.Namespace
if parentRef.Namespace != nil {
gatewayNamespace = string(*parentRef.Namespace)
}
gatewayKey := fmt.Sprintf("%s/%s", gatewayNamespace, gatewayName)
if s.gatewayRoutes[gatewayKey] == nil {
s.gatewayRoutes[gatewayKey] = sets.New[string]()
}
s.gatewayRoutes[gatewayKey].Insert(key)
}
}
klog.V(4).Infof("Added or updated HTTPRoute: %s", key)
return nil
}There was a problem hiding this comment.
yeah, but I’d keep the broader cleanup here. This runs on HTTPRoute store updates, not the request path, and it matches the existing DeleteHTTPRoute cleanup pattern while also clearing any stale mapping already present in the store.
This simple cleanup is better
|
Hey @hzxuzhonghu @YaoZengzeng pls take a look, small bug I noticed while looking through the HTTPRoute store path. This PR clears the old mapping before adding the current one. |
hzxuzhonghu
left a comment
There was a problem hiding this comment.
Not only in the store, i think in the https://github.com/volcano-sh/kthena/blob/4864eb71aca9d395e79b52296165a3e67af49111/pkg/kthena-router/controller/httproute_controller.go syncHandler, we need to compare the old parentRef with new one
| s.httpRoutes[key] = httpRoute | ||
|
|
||
| // Update gateway routes mapping | ||
| for gatewayKey, routeSet := range s.gatewayRoutes { |
There was a problem hiding this comment.
can optimize if we know the old parent gateway
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
e1e10e0 to
1d41ff0
Compare
Updated, syncHandler now drops the old stored route when parentRefs changed but the new Gateway is still pending, and AddOrUpdateHTTPRoute only cleans mappings from the old parentRefs. |
|
Hey @hzxuzhonghu thiss failure looks unrelated. The failing router E2E is in scheduler/plugin tests, while this change only touches HTTPRoute parentRef cleanup. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes stale HTTPRoute gateway mapping when
parentRefsis updated.AddOrUpdateHTTPRoutewas adding the route to the new Gateway mapping but not removing it from the old one, so the old Gateway could still see the route after it moved.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Added a small regression test for updating an HTTPRoute from one Gateway to another.
Does this PR introduce a user-facing change?: