New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-16707: UniqueHost: Fix incorrect identification of conflicting route #508
Conversation
@frobware: This pull request references Jira Issue OCPBUGS-16707, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions 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. |
3abac27
to
9229a22
Compare
@frobware: This pull request references Jira Issue OCPBUGS-16707, which is invalid:
Comment In response to this:
Instructions 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. |
9229a22
to
606ce07
Compare
@frobware: This pull request references Jira Issue OCPBUGS-16707, which is invalid:
Comment In response to this:
Instructions 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. |
There are no nodes that your pod can schedule to - check your requests, tolerations, and node selectors (0/16 nodes are available /test all |
/assign @rfredette |
Too many days, ... |
sort.SliceStable(old, func(i, j int) bool { | ||
return !routeapihelpers.RouteLessThan(old[i], old[j]) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure this fixes the issue. old
here seems to be all the routes with the same host, but I think we also need to take path into account to decide which route conflicts. Sorting all the routes works for the reproducer script from your haproxy-hacks repo, but if I edit the script to create the routes in the order route2 > route1 > route3, then route3 reports that route1 is the conflicting route, even though it's actually route2 that's the problem:
route.route.openshift.io/route2 created
route.route.openshift.io/route1 created
route.route.openshift.io/route3 created
Name: route3
Namespace: ocpbugs16707
Created: 3 seconds ago
Labels: <none>
Annotations: <none>
Requested Host: httpd-example-path-based-routes.apps.firstcluster.lab.upshift.rdu2.redhat.com
rejected by router default: (host router-default.apps.ci-ln-xxi6972-72292.gcp-2.ci.openshift.org)HostAlreadyClaimed (3 seconds ago)
route route1 already exposes httpd-example-path-based-routes.apps.firstcluster.lab.upshift.rdu2.redhat.com and is older
Path: /api
TLS Termination: <none>
Insecure Policy: <none>
Endpoint Port: http
Service: api-service
Weight: 100 (100%)
Endpoints: <error: endpoints "api-service" not found>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update in ca357ed.
When a route is rejected due to the host already being claimed, it was always selecting the first route but the first route is not guaranteed to being the conflicting route. All routes for the host are retrieved using the 'RoutesForHost' function and that function returns a slice of routes derived from a map in the 'hostIndex' structure. Since maps do not guarantee order, the first route was not necessarily the oldest. The fix is to first sort the conflicting routes based on their submission time, and then pick the newest. Fixes: https://issues.redhat.com/browse/OCPBUGS-16707
606ce07
to
ca357ed
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rfredette The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/jira refresh |
@frobware: This pull request references Jira Issue OCPBUGS-16707, which is invalid:
Comment In response to this:
Instructions 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. |
/jira refresh |
@frobware: This pull request references Jira Issue OCPBUGS-16707, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions 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. |
/retest |
@frobware: all tests passed! Full PR test history. Your PR dashboard. Instructions 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. |
@frobware: Jira Issue OCPBUGS-16707: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-16707 has been moved to the MODIFIED state. In response to this:
Instructions 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. |
Fix included in accepted release 4.15.0-0.nightly-2023-09-27-073353 |
When a route is rejected due to the host already being claimed, it was
always selecting the first route but the first route is not guaranteed
to being the conflicting route.
All routes for the host are retrieved using the 'RoutesForHost'
function and that function returns a slice of routes derived from a
map in the 'hostIndex' structure. Since maps do not guarantee order,
the first route was not necessarily the oldest.
The fix is to first sort the conflicting routes based on their
submission time, and then pick the newest.
Fixes: https://issues.redhat.com/browse/OCPBUGS-16707