Make the router data structures remove duplicates cleanly#10747
Make the router data structures remove duplicates cleanly#10747openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
|
@rajatchopra @openshift/networking PTAL |
|
[testextended][extended:core(router)] |
|
LGTM. Ready to merge. |
pkg/router/controller/unique_host.go
Outdated
| // Clean out any old form of this route | ||
| next := []*routeapi.Route{} | ||
| for i := range old { | ||
| if old[i].Name != route.Name { |
There was a problem hiding this comment.
don't we need to check namespace as well?
There was a problem hiding this comment.
We already checked the namespace before it can get in the list. And the delete loop at the bottom does the same check I implemented. However, if you want, I can add the namespace check too.
There was a problem hiding this comment.
ah, I only saw the comparison against the oldest one, but the whole list is supposed to be from a single namespace? if so, that's ok
There was a problem hiding this comment.
Yeah, at line 122 it screens the list by the namespace of the oldest path. I saw that check and thought about it, but I think it is redundant.
There was a problem hiding this comment.
Use routeNameKey(old[i]) != routeNameKey(route) instead of old[i].Name != route.Name
|
@liggitt if it looks ok to you, can you chuck a merge tag on it please? Or approve it, and I'll merge. Thanks |
|
LGTM |
The bug is caused by the modification of the route. In the code it would add a new object to the list of routes by the host and not remove them. When the route was deleted, it would remove all of the routes with the same name, and then the list would be cleared. Unfortunately, the code would then look at the number of items in the list, and either delete it from the map, or clean items out of the list. But since it was possible for there to be multiple duplicate names, we could end up with an empty list. Later code would assume that if there was a key in the map, then the associated list would have items. That caused a panic. This fix changes both places: - If there are duplicates, remove them all, and then if the list is empty, delete the key from the map - When a modification happens, remove the previous entry from the list So, the first duplicate check is now just for protection, but the proper fix is to clean out the lists before adding items to them. Fixes bug 1371826
knobunc
force-pushed
the
bug/bz1371826-panic-in-haproxy
branch
from
8cf4f23 to
7ac6611
Compare
|
Evaluated for origin testextended up to 7ac6611 |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/455/) (Extended Tests: core(router)) |
|
last one flaked [merge] |
|
flake was #10765 |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 7ac6611 |
|
Argh. Wrong button :-( |
|
do we need to say [merge] again? |
|
where's the test? edit: My point is that a clean test result only validates existing assumptions. This code fixes an oversight in the previous coverage, and without additional coverage, there is no guarantee the fix here won't be broken by future changes. |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8626/) |
|
@marun, agreed, and I'm working on the test. But in interests of getting 3.3 out the door I was putting the fix in separately. I manually tested the PR for now. |
|
[merge] flaked on #10765 again |
|
Evaluated for origin merge up to 7ac6611 |
|
[merge] flaked on #10773 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8642/) (Image: devenv-rhel7_4969) |
|
@knobunc I'm still able to reproduce it on v1.3.0-rc1 router image. |
|
After talking to @spinolacastro over IRC. This seems like a new (but similar) issue, so a new bug will be opened. |
8 participants
The bug is caused by the modification of the route. In the code it
would add a new object to the list of routes by the host and not
remove them. When the route was deleted, it would remove all of the
routes with the same name, and then the list would be cleared.
Unfortunately, the code would then look at the number of items in the
list, and either delete it from the map, or clean items out of the
list. But since it was possible for there to be multiple duplicate
names, we could end up with an empty list. Later code would assume
that if there was a key in the map, then the associated list would
have items. That caused a panic.
This fix changes both places:
empty, delete the key from the map
So, the first duplicate check is now just for protection, but the
proper fix is to clean out the lists before adding items to them.
Fixes bug 1371826