| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-21 | |||
| 21:19:24 | mriedem | i don't think refresh_connection_info would be getting called during spawn | |
| 21:19:40 | mnaser | the only place i noticed calls it is refresh_conn_infos | |
| 21:19:43 | mriedem | but yeah that update_db decorator is what could be doing it | |
| 21:20:18 | sean-k-mooney | efried: .. we pin our lower constating for warlock to one explcitly blacklisted by python-glance client... | |
| 21:20:22 | mnaser | ah | |
| 21:20:23 | sean-k-mooney | the fun continues | |
| 21:20:31 | mriedem | mnaser: i think that's only called here https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/compute/manager.py#L1779 | |
| 21:20:33 | mriedem | on like move ops and such | |
| 21:20:44 | mnaser | indeed thats the only reference to it that i find | |
| 21:21:23 | efried | sean-k-mooney: Presumably it doesn't hurt to elevate the warlock l-c to... whatever glanclient is using | |
| 21:21:41 | sean-k-mooney | it is useing an older version and there is no newer version | |
| 21:21:57 | mnaser | ah shit | |
| 21:21:59 | sean-k-mooney | we pin to 1.3.0 which was relwased in 2016 | |
| 21:22:11 | sean-k-mooney | they use 1.2.0 | |
| 21:22:16 | mriedem | mnaser: which release is this? | |
| 21:22:21 | mnaser | rocky | |
| 21:23:02 | mriedem | it could be somewhere in here that the changes to connection_info get saved https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/compute/manager.py#L2554 | |
| 21:23:26 | sean-k-mooney | efried: im goint to try lowerering it | |
| 21:23:42 | sean-k-mooney | the constratin we have is from when we intoduced lower-constratits | |
| 21:23:56 | sean-k-mooney | so im not sure its actully the min version we need | |
| 21:24:12 | mnaser | terminate_instance seems to get bdms | |
| 21:24:39 | mriedem | mnaser: you wouldn't get there when the server is deleted, | |
| 21:24:42 | mriedem | because the instance.host is None | |
| 21:24:44 | mnaser | yeah | |
| 21:24:49 | sean-k-mooney | efried: actully looking at requirements.txt we dont actully depend on it directly | |
| 21:25:45 | efried | Not surprising. | |
| 21:28:23 | mriedem | mnaser: anyway, it's probably not worth trying to figure out where in that pile of spaghetting the bdm.save is getting called, | |
| 21:28:32 | mriedem | we shouldn't save 'null' in the connection_info | |
| 21:28:48 | mriedem | so your change is probably sane on it's own | |
| 21:29:00 | mnaser | mriedem: yeah.. i was just trying to find a way to have some sort of tests but yeah | |
| 21:29:10 | mnaser | also i just realized that wouldn't even fix it | |
| 21:29:12 | mriedem | get_stashed_volume_connector is very old so i'm kind of surprised someone is just hitting this now | |
| 21:29:40 | mnaser | https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/virt/block_device.py#L284-L288 | |
| 21:30:16 | mnaser | where i thought we were getting 'None' because .get("connection_info") was returning an empty field, it was actually set, but to 'null'.. so yeah | |
| 21:30:19 | mriedem | i'm not sure what that has to do with it - that's code that runs before things blow up | |
| 21:30:46 | mnaser | the assumption in my fix that self['connection_info'] doesn't exist and the fact we get None is because there is no 'connection_info' key | |
| 21:31:04 | mriedem | _transform is called to convert the BlockDeviceMapping object into a DriverVolImageBlockDevice object before the compute manager calls attach() on it | |
| 21:31:08 | mriedem | from _prep_block_device | |
| 21:31:11 | mriedem | it's extremely confusing | |
| 21:31:21 | mnaser | but it seems that 'connection_info' key always exists, but it can be set to None. so the 'None' value comes from the fact that it's *actually* equal to 'None' | |
| 21:31:54 | mriedem | loads(None) should raise the TypeError though | |
| 21:33:24 | mnaser | ok so the ideal fix would be | |
| 21:33:28 | mnaser | https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/virt/block_device.py#L288 change that default to {} | |
| 21:33:49 | mnaser | so if its null, it blows up and returns {}, if its an actual object, we keep it | |
| 21:33:55 | sean-k-mooney | dumps(None) should return 'null' loads('null) shoudl return None | |
| 21:34:08 | sean-k-mooney | loads(None) shoudl be a type error yes | |
| 21:34:15 | mriedem | _validate_existing_attachment_ids | |
| 21:34:17 | mriedem | oops | |
| 21:34:39 | mriedem | mnaser: connection_info can be None, and get_stashed_volume_connector handles it, so i'm not following you | |
| 21:34:58 | mnaser | mriedem: but if update_db happened, connection_info now is a literally 'null' (the string) | |
| 21:34:59 | mriedem | i though https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/virt/block_device.py#L681 was the problem | |
| 21:35:09 | mnaser | and json loads converts 'null' to 'None' | |
| 21:35:35 | mriedem | but we can avoid the 'null' by changing https://github.com/openstack/nova/blob/67ac57b3258d4d7476fa544b3dfef7296e220ad2/nova/virt/block_device.py#L682 to default to {} yeah? | |
| 21:35:42 | sean-k-mooney | mnaser: it convets the sting 'null' to the object None | |
| 21:35:43 | mnaser | yes | |
| 21:35:45 | mriedem | so you'd get '{}' | |
| 21:35:51 | mnaser | yep, and that would fix it | |
| 21:36:13 | mriedem | right, hence me saying "anyway, it's probably not worth trying to figure out where in that pile of spaghetting the bdm.save is getting called" | |
| 21:36:20 | mriedem | "we shouldn't save 'null' in the connection_info" | |
| 21:36:25 | sean-k-mooney | {} is an empty json object which is technically not the same thing but it will fix this case | |
| 21:36:37 | mnaser | mriedem: ah because my original 'fix' didn't actually fix it -- https://review.openstack.org/#/c/645352/1/nova/virt/block_device.py | |
| 21:36:44 | mnaser | so i was referring to that, but, i will fix that now | |
| 21:36:48 | mnaser | and will try to have some unit test for _transform | |
| 21:37:04 | mnaser | to make sure it doesn't output connection_info = None | |
| 21:37:17 | mriedem | i would think this could be easily reproducible if we just hit overquota when booting from volume and then try to delete the server | |
| 21:38:02 | mnaser | probably, but not sure if i can write that level of functional tests from my skillset so far | |
| 21:38:20 | mnaser | unless you wanna a similar functional test that i can read and implement similar to | |
| 21:38:22 | mriedem | i could write one | |
| 21:38:44 | mnaser | sure, let me push up this with a unit test too | |
| 21:41:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Pre-filter hosts based on multiattach volume support https://review.openstack.org/645316 | |
| 21:44:16 | mriedem | efried: i went through and marked the deferred train blueprints that dont have specs re-approved as pending approval https://blueprints.launchpad.net/nova/train | |
| 21:44:22 | mriedem | i forgot to do that when deferring from stein | |
| 21:44:31 | mriedem | so we re-approve in LP once the spec is re-approved | |
| 21:44:59 | efried | mriedem: ack, thank you. I can see you saw I approved the quota one. | |
| 21:45:14 | mriedem | yeah, i thought about it because you said on the review: | |
| 21:45:15 | mriedem | "Blueprint in approved state [3] ✔" | |
| 21:45:26 | mriedem | technically the blueprint shouldn't have been approved before the spec | |
| 21:45:31 | mriedem | but i forgot to change those when deferring | |
| 21:47:40 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Trivial: remove unused var from policies.base.py https://review.openstack.org/645380 | |
| 21:49:29 | mriedem | dansmith: i've got this pretty much done, without using a pre-request filter https://review.openstack.org/#/c/645316/ but not sure if i need a blueprint or not, since i'm not adding a configurable filter, it's just going to always be required if you're booting from volume | |
| 21:50:01 | mriedem | cuz it's kind of dumb to even try to boot with a multiattach volume if the scheduler isn't going to filter out hosts that won't support them | |
| 21:51:03 | efried | does that mean they would break further on down the line? | |
| 21:51:13 | efried | I mean, would initial spawn succeed or fail? | |
| 21:51:23 | mriedem | it would fail, | |
| 21:51:25 | mriedem | it's arguably a bug fix | |
| 21:51:49 | mriedem | the bare minimum we do today is check if the computes are new enough, but that was minimum of queens | |
| 21:52:01 | mriedem | that doesn't say bfv wo'nt work for vmware for exapmle | |
| 21:52:17 | mriedem | if you're using vmware exclusively, you'd just have to never enable multiattach volumes on the cinder side via policy | |
| 21:52:30 | mriedem | that's a pain if you're doing multi-hypervisor though | |
| 21:52:55 | mriedem | you'd have to setup aggregates and filter using some flavor extra spec or something | |
| 21:55:38 | mriedem | anywho, idk, it's janky, i suppose you can't reasonably do bfv with vmware and libvirt mixed anyway because you have different volume types, and the libvirt driver isn't going to support vmware volumes and vice versa, so i'm not sure what if anything people do for mixed hypervisor - definitely at least aggregates/AZs | |
| 21:56:39 | sean-k-mooney | efried: ... this change https://github.com/openstack/nova/commit/6b844af57eebda6317b1ed3bf5a2f85dcba0bc13 does not work after i fix the lower constrating so im going to have to change it | |
| 21:57:39 | efried | mriedem: I say no bp, bug good, reno optional | |
| 21:58:33 | efried | sean-k-mooney: I'm on board with that. Long as you don't have to go changing the prod code, I say do what you gotta do to get this fixed. | |
| 21:59:38 | sean-k-mooney | im actully not sure why this is even working upstream honestly | |
| 22:00:34 | sean-k-mooney | i think it is depending on something having previously loaded messaging._drivers.impl_fake | |
| 22:00:50 | sean-k-mooney | *imported | |
| 22:02:35 | sean-k-mooney | ya it is | |
| 22:02:38 | sean-k-mooney | >>> import oslo_messaging as messaging | |
| 22:02:40 | sean-k-mooney | >>> messaging._drivers.impl_fake.FakeExchangeManager._exchanges | |