| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-02-27 | |||
| 14:45:33 | efried | If there's now real sdk impl (rather than just REST primitive passthrough) that we could switch to, so much the better, though I would argue that should be done later, since it doesn't involve a problem we need to solve immediately. | |
| 14:45:40 | sean-k-mooney | in that case i might just put a testing patch on the top of the chain that swaps the function calls and runs my multi node job to see if it fixes it | |
| 14:46:22 | sean-k-mooney | ya i was thinkig of just doint the s/get_ksa_adapter/get_sdk_adapter/ | |
| 14:46:32 | efried | yeah | |
| 14:46:36 | sean-k-mooney | and see if that works | |
| 14:46:36 | efried | worth a try | |
| 14:46:46 | efried | I don't remember for sure whether that's all it takes... | |
| 14:47:03 | efried | but easy enough to sniff out. | |
| 14:47:18 | sean-k-mooney | if we are basically doing raw request i think it is | |
| 14:47:20 | efried | If you have a live env handy it might make sense to try it out to winnow out any easy problems. | |
| 14:47:30 | sean-k-mooney | if we wanted to use the new proxy function that would be more involed | |
| 14:47:32 | efried | yeah, it wouldn't be wrt the requests, it would be with the setup. | |
| 14:49:17 | sean-k-mooney | did you see i updated https://review.opendev.org/#/c/673341/ yesterday by the way | |
| 14:49:33 | sean-k-mooney | im going to try and do the rest of the cain later today | |
| 14:51:16 | efried | sean-k-mooney: yeah, I was going to wait until all updated to start reviewing, unless you think I should look sooner. | |
| 14:51:48 | sean-k-mooney | am whatever works for you. most of the comments were gibis so i think he is the one that is more pressing | |
| 14:52:03 | sean-k-mooney | that said waiting till they are all done is proably for the best | |
| 14:52:27 | sean-k-mooney | that way you will have less context switching | |
| 14:53:02 | efried | mordred: I think there's not too much context to be missed here. In proposed patch series, nova is talking to cyborg using a raw ksa adapter. I insisted we make that client a singleton, and we're seeing tokens expire after a while (I guess ksa doesn't take care of keeping them fresh). We want to cut over to using sdk anyway, so if sdk handles expiration under the covers, two birds. | |
| 14:53:08 | sean-k-mooney | so ya im just going to grab lunch. when i get back ill quickly do the test patch for the cyborg series and then ill start on the provider.yaml patches | |
| 14:53:18 | efried | ++ | |
| 14:57:53 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Always provide the size in bytes when calling virDomainBlockResize https://review.opendev.org/707590 | |
| 14:58:01 | openstackgerrit | Stephen Finucane proposed openstack/nova master: images: Remove Libvirt specific configurable use from qemu_img_info https://review.opendev.org/707591 | |
| 15:03:38 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove MIN_LIBVIRT_MULTIATTACH https://review.opendev.org/710238 | |
| 15:03:39 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove QEMU_VERSION_REQ_SHARED https://review.opendev.org/710239 | |
| 15:05:53 | openstackgerrit | Stephen Finucane proposed openstack/os-traits master: setup.cfg: Configure 'python-requires' https://review.opendev.org/704534 | |
| 15:05:54 | openstackgerrit | Stephen Finucane proposed openstack/os-traits master: setup.cfg: Remove unused 'build_sphinx' configuration https://review.opendev.org/704535 | |
| 15:05:55 | openstackgerrit | Stephen Finucane proposed openstack/os-traits master: Remove use of six https://review.opendev.org/710274 | |
| 15:05:56 | openstackgerrit | Stephen Finucane proposed openstack/os-traits master: Switch to hacking 2.x https://review.opendev.org/710275 | |
| 15:08:38 | stephenfin | lyarwood: I've a question left at https://review.opendev.org/#/c/706898/ btw | |
| 15:08:56 | stephenfin | regarding how we're going to backport this fix if depends on a new oslo.utils feature | |
| 15:09:07 | stephenfin | not sure if you've thought through that yet or no | |
| 15:12:38 | mordred | efried: ok. that's something I think we should still look in to ... ksa should be auto-renewing tokens ... but sdk should _certainly_ be doing it | |
| 15:13:03 | mordred | efried: and I'd consider it a bug in sdk if it's not - even if it's something we should be getting ksa to do for us - so that sounds like a good plan | |
| 15:13:20 | efried | ack, thx | |
| 15:13:44 | mordred | efried: are these tokens created from credentials nova has? or tokens received from the user that nova is re-using? | |
| 15:14:13 | efried | mordred: from conf, via our soon-to-be-famous oslo.config loader | |
| 15:14:25 | efried | um, I think | |
| 15:14:45 | efried | Now I can't remember whether we decided to do cyborg-ness as user or admin... | |
| 15:15:20 | efried | ...user | |
| 15:15:45 | efried | mordred: so you can conf the session & adapter opts, but the auth should be the nova user's. | |
| 15:15:54 | efried | Which means it makes even less sense that the token is expiring. | |
| 15:16:38 | lyarwood | stephenfin: sorry just back from a call | |
| 15:17:19 | lyarwood | stephenfin: yeah I was talking of backporting this more in a downstream context where we control everything within a given container | |
| 15:17:36 | lyarwood | stephenfin: and then only to stable/train | |
| 15:17:48 | lyarwood | stephenfin: for upstream, what do you mean in terms of vendoring? | |
| 15:20:07 | stephenfin | I mean copying the newly updated contents of 'oslo_utils.imageutils' into the nova tree | |
| 15:21:17 | openstackgerrit | GEET JAIN proposed openstack/nova master: Fix bug where aggregate api works with invalid id https://review.opendev.org/710290 | |
| 15:22:33 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove QEMU_VERSION_REQ_SHARED https://review.opendev.org/710239 | |
| 15:22:52 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove MIN_LIBVIRT_MULTIATTACH https://review.opendev.org/710238 | |
| 15:22:56 | lyarwood | stephenfin: ah wait, so in the context of the bugfix I just need to bump requirements in https://review.opendev.org/#/c/706900/ | |
| 15:24:01 | stephenfin | For the master patch, yup | |
| 15:24:11 | stephenfin | Assuming a new oslo.utils patch has been cut since | |
| 15:25:15 | lyarwood | https://review.opendev.org/#/c/710078/ - cut but not released | |
| 15:29:50 | mordred | efried: yeah - it should *really* be renewing | |
| 15:30:47 | openstackgerrit | Lee Yarwood proposed openstack/nova master: workarounds: Add option to disable native LUKSv1 decryption by QEMU https://review.opendev.org/708030 | |
| 15:30:48 | openstackgerrit | Lee Yarwood proposed openstack/nova master: workarounds: Connect RBD volumes to the compute host as block devices https://review.opendev.org/708029 | |
| 15:31:01 | sean-k-mooney | efried: we should be useing the token form the api request not form the config | |
| 15:31:42 | sean-k-mooney | we are not adding setting up the service user in the config and we are not adding a cyborg section to the nova.conf | |
| 15:32:15 | sean-k-mooney | so i dont think the issue is with renewing the token in the nova code | |
| 15:32:43 | sean-k-mooney | i think with the singelton we create the client onece capturing the context and token at that point | |
| 15:32:51 | sean-k-mooney | and then it never gets updated again | |
| 15:34:35 | sean-k-mooney | mordred: ^ that is also relevent to you i guess | |
| 15:37:34 | mordred | yeah. that - if we're using the api request token, then when we get the singleton sdk connection object, we need to call conn.connect_as on it to get a new connection with the credentials we've been given, otherwise we're making all of the requests with the creds of the original user :) | |
| 15:38:09 | sean-k-mooney | ya we are not doing that | |
| 15:38:19 | sean-k-mooney | so that is likely the bug | |
| 15:38:35 | mordred | conn.connect_as lets us keep discovery cache appropriately and all of that - but will construct a new underlying Auth construct and a new Adapter | |
| 15:38:59 | mordred | sean-k-mooney: computers are hard | |
| 15:39:18 | sean-k-mooney | mordred: we would do that here right https://review.opendev.org/#/c/631242/51/nova/accelerator/cyborg.py@32 just before the retrun? | |
| 15:39:32 | sean-k-mooney | although im not sure that would be safe | |
| 15:39:47 | sean-k-mooney | we could context switch at any point so we would have to take the lock | |
| 15:39:58 | sean-k-mooney | and do it before making any call | |
| 15:41:49 | mordred | sean-k-mooney: sdk is threadsafe | |
| 15:41:55 | mordred | you shouldn't need a lock | |
| 15:42:12 | sean-k-mooney | is it eventlet safe | |
| 15:42:22 | mordred | _should_ be | |
| 15:42:25 | mordred | if it's not it's a bug | |
| 15:42:38 | sean-k-mooney | if the con.connect_as returns a new object that we then make new calls on it should work | |
| 15:42:42 | mordred | I mean - but that said - with eventlet who the hell knows | |
| 15:42:53 | mordred | sean-k-mooney: yes - connect_as returns a new object | |
| 15:43:29 | mordred | it returns a whole new Connection with all the settings of the old one but with just the auth bits replaced | |
| 15:43:32 | sean-k-mooney | and that object basicaly poxies the requeqes on it to the adapter it was created form but with the update creds | |
| 15:44:21 | mordred | it has to create a new adapter - because an adapter is a container of session+auth+endpoint-config - you can't use the old adapter if you have new auth, you need a new adapter | |
| 15:44:36 | sean-k-mooney | is so then we just need to change "return _CYBORG_CLIENT" to "return _CYBORG_CLIENT.connect_as(context) | |
| 15:44:36 | mordred | but it does keep/share the things that are safe to keep/share | |
| 15:44:43 | mordred | yeah | |
| 15:45:13 | mordred | with a little hand wavey, because we'll want to make sure we're doing the right things with context ... but pseudo-code-wide yes | |
| 15:45:36 | sean-k-mooney | ok so ill swap to the sdk adapter and add the connect_as | |
| 15:46:49 | sean-k-mooney | mordred: ya, this isnt my code and i havent realy review it im just testing it but i can relyably repoduce that the current code is borked so tring to fine the minimal correct fix and test that in a follow up patch | |
| 15:47:09 | sean-k-mooney | if it fixes the issue then we can squash it in and move on to the next thing | |
| 15:47:41 | mordred | ++ | |
| 15:48:00 | mordred | let me know if you have issues and want me to dive in further | |
| 15:48:11 | mordred | I'm sure I could find some lovely new ways to curse | |
| 15:56:00 | efried | If we have to build a new adapter anyway, then I suppose we might as well just do as dansmith suggests and un-singleton for now. We'll want to do the SDK cutover later, but there's no way we'll forget to do that. | |
| 15:56:14 | dansmith | ++ | |
| 15:57:48 | sean-k-mooney | im not sure if the connect_as is cheaper then building the adapter from scratch but ya i just want to see this working and then iterate on it later to not loose the momentem we have while people are around to ask questions | |
| 15:58:15 | sean-k-mooney | *answer | |
| 15:58:49 | openstackgerrit | Stephen Finucane proposed openstack/os-resource-classes master: Switch to hacking 2.x https://review.opendev.org/710276 | |
| 16:34:55 | openstackgerrit | Merged openstack/os-traits master: setup.cfg: Configure 'python-requires' https://review.opendev.org/704534 | |
| 16:34:56 | openstackgerrit | Merged openstack/os-traits master: setup.cfg: Remove unused 'build_sphinx' configuration https://review.opendev.org/704535 | |