change the router eventqueue key function #13494
Conversation
|
[test] |
|
@knobunc @openshift/networking PTAL |
openshift-bot
added
the
needs-rebase
|
Hm. But can't I crash the router as an unprivileged user by repeatedly re-sending the same route object on a create with the UID set? |
|
@smarterclayton the networking team is concerned that if they make the key |
|
@ncdc @smarterclayton @danwinship @marun @dcbw -- I just tried the test of: So, with routes at least, the uids are being updated. I know Andy, Dan (Winship), and I worked on debugging a networking problem that ended up being caused by the namespaces having the same uid when the user dumped them and edited them to create new ones. So perhaps the problem is just with certain objects? Or perhaps the problem is now solved somewhere sytstemically. Anyway, this LGTM now. I have to revert the other patch from @ramr to crash the router (#13502). |
| @@ -56,25 +57,39 @@ func NewDefaultRouterControllerFactory(oc osclient.RoutesNamespacer, kc kclients | |||
| } | |||
| } | |||
|
|
|||
| func routerKeyFn(obj interface{}) (string, error) { | |||
There was a problem hiding this comment.
Can you just put a comment in here explaining where this came from and why we pulled it in here.
JacobTanenbaum
force-pushed
the
BZ1429823
branch
from
101ab9d to
066995f
Compare
|
Looks ok, as long as we can reproduce reliably before and ensure it's fixed after |
|
[test] |
|
@JacobTanenbaum failed with: |
openshift-bot
removed
the
needs-rebase
changing the router eventqueue key function so that there is a higher chance that each item will have a unique key so the router does not panic. originally the thought was to add the creation timestamp because it was not user editable but the accessor function meta.CreationTimestamp() only gives the timestamp to the second and since these actions need to occur quickly a second is too long. Only adding creation timestamp I was able to observe the panic with the test script. I decided to use UID because it is much more likely that the UID is unique. Bug: 1429823 changelog: added a note explaining why routerKeyFn was added
JacobTanenbaum
force-pushed
the
BZ1429823
branch
from
41d0f9b to
fd723fd
Compare
|
Evaluated for origin test up to fd723fd |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/448/) (Base Commit: 367a8d3) |
|
[merge] |
|
Do we have a reproducer for before and after and confirmation of a fix?
…On Fri, Mar 24, 2017 at 11:08 AM, Ben Bennett ***@***.***> wrote:
[merge]
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13494 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_pzJEONbE0lxBOMvcUu1emZmDzR40ks5ro9xegaJpZM4Mkn-t>
.
|
|
Evaluated for origin merge up to fd723fd |
|
@smarterclayton yes there is a reproducer The caveat being our routers recover from the panic while the customers is not able too... After applying this PR the router no longer panics |
|
I'd like a test in our tree that causes the panic and is shown to be fixed
with this change
…On Fri, Mar 24, 2017 at 11:34 AM, Jacob Tanenbaum ***@***.***> wrote:
@smarterclayton <https://github.com/smarterclayton> yes there is a
reproducer
https://gist.github.com/ramr/58dbdc3c5982db7b3c3154eb4bca60c8
$ ./reproduce-eq-panic.sh [<route-json-yaml-file>]
The caveat being our routers recover from the panic while the customers is
not able too... After applying the this PR the router no longer panics
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13494 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p6mApKZup5G689cwwJhdY5NQEgO9ks5ro-J-gaJpZM4Mkn-t>
.
|
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/199/) (Base Commit: ac3b692) (Image: devenv-rhel7_6100) |
|
@smarterclayton Ok, we'll provide that. Does the test case need to be back-ported too? |
changing the router eventqueue key function so that there is a higher chance that
each item will have a unique key so the router does not panic.
originally the thought was to add the creation timestamp because it was not user
editable but the accessor function meta.CreationTimestamp() only gives the timestamp
to the second and since these actions need to occur quickly a second is too long. Only
adding creation timestamp I was able to observe the panic with the test script. I
decided to use UID because it is much more likely that the UID is unique.
Bug: 1429823 Link