| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-01 | |||
| 19:32:35 | mnaser | ill welcome all help :P | |
| 19:32:44 | mnaser | *novncproxy | |
| 19:34:15 | mnaser | think it relies on upstream this https://github.com/novnc/websockify/blob/master/websockify/websocketproxy.py#L267 | |
| 19:34:26 | mnaser | self.token_plugin = kwargs.pop('token_plugin', None) | |
| 19:34:28 | mnaser | oh I wonder what that is | |
| 19:35:39 | melwitt | mnaser: I looked at that and it doesn't seem like something that would help us. but I could be wrong https://github.com/novnc/websockify/wiki/Token-based-target-selection | |
| 19:37:42 | mnaser | https://github.com/novnc/websockify/blob/master/websockify/websocketserver.py#L35-L52 | |
| 19:37:42 | mnaser | maybe we somehow override the html portion | |
| 19:37:48 | melwitt | mnaser: I think handling of HTTP (GET) requests might be this https://github.com/novnc/websockify/blob/6e09ec25486f431e2acc463f994c4eb4bebc6ddd/websockify/websockifyserver.py#L268 | |
| 19:37:57 | mnaser | oh that's even better | |
| 19:38:26 | melwitt | one of those, I don't really understand it yet | |
| 19:39:08 | efried | This one came across my desk again just now, can I please get a couple of pairs of core eyeballs on it? https://review.openstack.org/#/c/635533/ | |
| 19:41:10 | mnaser | I'm thinking https://github.com/novnc/websockify/wiki/Token-based-target-selection#custom-plugins | |
| 19:43:08 | melwitt | mnaser: hm. so instead of validating the token in new_websocket_client we'd do it in a new lookup method? | |
| 19:43:24 | melwitt | 'lookup' | |
| 19:43:29 | mnaser | melwitt: yeah, if we write a custom plugin, we can validate the token in it and then give it back to novnc | |
| 19:43:33 | efried | mriedem: This looks like a deadlock starting n-api? http://logs.openstack.org/36/649036/1/check/tempest-full-py3/a8c7223/controller/logs/screen-n-api.txt.gz | |
| 19:43:53 | melwitt | mnaser: it says lookup has to return a host:port tuple? | |
| 19:44:31 | mnaser | or none, none (if we don't validate I guess) | |
| 19:44:43 | efried | (maybe not, I guess the locks are being released quickly) | |
| 19:44:55 | mnaser | and the host:port tuple business can be grabbed from the db | |
| 19:46:27 | mnaser | the example module there seems to be reasonable | |
| 19:47:20 | mnaser | ugh but even then | |
| 19:47:26 | mnaser | I don't think anything will pass ?token=foo | |
| 19:48:08 | melwitt | mnaser: ok, I was trying to say when you said "give the token back to novnc" I don't think you can do that in lookup() | |
| 19:48:51 | mnaser | oooo | |
| 19:48:58 | melwitt | mnaser: earlier I was saying maybe we could do the entire validation in lookup() and then no longer need the token after that? | |
| 19:49:02 | mnaser | melwitt: https://github.com/novnc/websockify/blob/6e09ec25486f431e2acc463f994c4eb4bebc6ddd/websockify/websocketproxy.py#L164-L175 | |
| 19:49:16 | melwitt | yeah | |
| 19:49:47 | mnaser | so we get lookup called with the token, grab the stuff from the db, and return it back, and we can probably drop all that extra code | |
| 19:49:59 | melwitt | what do you mean by return it back? | |
| 19:50:23 | mnaser | so in the token plugin, we check the db, see if the var is still valid, and return (host, port) | |
| 19:50:36 | mnaser | and technically... I think none will just do the rest of the issues | |
| 19:50:43 | melwitt | ok. I thought you meant return the token | |
| 19:50:45 | melwitt | ok. that's what I was trying to say earlier | |
| 19:50:53 | mnaser | ah no I meant return the host/port, sorry distracted | |
| 19:51:00 | mnaser | people keep walking into my office >:( | |
| 19:51:09 | melwitt | how dare they! | |
| 19:52:39 | mnaser | let me try to hack at this | |
| 19:54:12 | melwitt | sounds good. I will review it when it's ready :) | |
| 19:54:27 | mnaser | honestly I think this could completely eliminate the websocketproxy.py | |
| 19:54:53 | mnaser | a lot of that stuff exists in webproxify but I dunno how much of this can be back portable at this point.. | |
| 19:55:28 | melwitt | maybe. there is some extra stuff in there though | |
| 20:19:18 | mnaser | melwitt: well, I got a nice refactor out of it, but still I think it expects I to be there | |
| 20:21:16 | melwitt | mnaser: yeah, you'd have to change https://github.com/openstack/nova/blob/master/nova/cmd/baseproxy.py#L68 too | |
| 20:23:32 | mnaser | ok well to get it just to work I found a gross fix.. I think | |
| 20:23:38 | mnaser | let me test it against the original code | |
| 20:23:49 | mnaser | vnc_lite.html?path=?token=<token> seems to be calling my token plugin | |
| 20:25:14 | mnaser | But I think then the original code works | |
| 20:25:22 | mnaser | We would just need to tweak the default value :/ | |
| 20:25:49 | melwitt | yeah, shouldn't need path= right | |
| 20:25:58 | melwitt | default value of what? | |
| 20:27:45 | mnaser | The nova configuration value | |
| 20:27:51 | mnaser | The one pointing at vnc_lite or whatever | |
| 20:28:19 | mnaser | To not longer suggest ?token=foo is the way but ?path=?token=foo | |
| 20:28:39 | melwitt | I thought earlier you told me including path= wasn't as clean :P | |
| 20:28:51 | mnaser | Yeah it’s just BLAH | |
| 20:28:57 | mnaser | why people gotta rip code out | |
| 20:29:14 | melwitt | I guess, I thought the token plugin would let us use the old default url | |
| 20:29:19 | mnaser | Same. | |
| 20:29:34 | mnaser | But once it was done I realized the web socket thing was only checking it at the web socket stage | |
| 20:29:37 | melwitt | because if we include path in the default, we could fix this without the token plugin, I think, as I mentioned earlier | |
| 20:29:55 | mnaser | Yeah then we won’t be falling back to cookies every time | |
| 20:30:02 | melwitt | right | |
| 20:31:38 | mnaser | I guess we can propose a revert and then I’ll just pin on the old commit :/ | |
| 20:31:50 | mriedem | efried: you're talking about get_or_set_cached_cell_and_set_connections right? | |
| 20:32:13 | efried | mriedem: yeah | |
| 20:32:23 | efried | I didn't go back and look whether that's "normal". | |
| 20:32:25 | mriedem | i think that's nova.compute.api.API being constructed per controller per worker | |
| 20:32:30 | efried | k | |
| 20:32:35 | mriedem | which is a lot since it's not a singleton | |
| 20:33:02 | efried | Last week I noticed pretty frequent failures sprayed across the long-running jobs. Never more than one per patch, and never the same one twice in a row. | |
| 20:33:08 | efried | but I didn't dig into any of them. | |
| 20:33:10 | mriedem | i assume the get_or_set_cached_cell_and_set_connections target_cell is due to a query on the services table when constructing the compute RPCAPI with [upgrade_levels]/compute=auto | |
| 20:33:39 | mriedem | https://bugs.launchpad.net/nova/+bug/1815697 | |
| 20:33:40 | mnaser | I honestly don’t have that much time to dig more into this. I can report a bug and add a known issue (if needed) but I don’t have time to deal with getting this merged into novnc :/ | |
| 20:33:40 | openstack | Launchpad bug 1815697 in OpenStack Compute (nova) "[upgrade_levels]compute=auto grinds the API response times when a cell is down" [Medium,Confirmed] | |
| 20:33:58 | mriedem | efried: also https://bugs.launchpad.net/nova/+bug/1815692 | |
| 20:34:00 | openstack | Launchpad bug 1815692 in OpenStack Compute (nova) "minimum nova-compute service version is not cached in nova-api when running under wsgi" [Low,Triaged] | |
| 20:34:58 | melwitt | mnaser: or we could change our default nova conf value? or do you think asking for a revert is a better route? | |
| 20:35:25 | mnaser | melwitt: I think the amount of downstream stuff that will break (because everyone configures this in their compute nodes) is gonna be .. a lot | |
| 20:35:37 | mnaser | cause now tripleo/osa/etc all have to change our paths | |
| 20:35:47 | mnaser | to something sketchy none the less :P | |
| 20:35:58 | melwitt | mnaser: yeah, I see. ok | |
| 20:36:06 | mnaser | I wouldn't revert it all | |
| 20:36:11 | mnaser | maybe just the small part to set the cookie | |
| 20:36:12 | melwitt | yeah | |
| 20:41:34 | mriedem | efried: i guess bug 1815692 won't help because we don't cache the minimum compute service version until we get one, meaning until a nova-compute starts up, which in devstack is after the api | |
| 20:41:36 | openstack | bug 1815692 in OpenStack Compute (nova) "minimum nova-compute service version is not cached in nova-api when running under wsgi" [Low,Triaged] https://launchpad.net/bugs/1815692 | |
| 20:42:12 | mriedem | making nova.compute.api.API or nova.compute.rpcapi.ComputeAPI a singleton might help | |
| 20:42:12 | efried | is making the thing a singleton an option? | |
| 20:42:27 | efried | Is there any evidence that that's actually a problem though? | |
| 20:42:31 | mnaser | melwitt: https://bugs.launchpad.net/nova/+bug/1822676 | |
| 20:42:32 | openstack | Launchpad bug 1822676 in OpenStack Compute (nova) "novnc no longer sets token inside cookie" [Undecided,New] | |
| 20:42:38 | melwitt | mnaser: you know, while I was looking at the websockify source, I was thinking maybe in code we could append to our self.path earlier in the call path. I can try something out | |
| 20:42:42 | efried | the devstack log just says n-api doesn't start. | |
| 20:42:51 | efried | I guess that means "within a certain amount of time" | |
| 20:42:53 | melwitt | mnaser: thanks for the bug report | |
| 20:42:55 | mriedem | efried: it's a timeout | |
| 20:42:58 | mnaser | melwitt: all yours :) I don't think I can take more time on this :X | |