Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-21
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
22:02:42 sean-k-mooney Traceback (most recent call last):
22:02:47 sean-k-mooney AttributeError: 'module' object has no attribute 'impl_fake'
22:02:47 sean-k-mooney File "<stdin>", line 1, in <module>
22:02:48 sean-k-mooney >>> import oslo_messaging._drivers.impl_fake
22:02:50 sean-k-mooney >>> messaging._drivers.impl_fake.FakeExchangeManager._exchanges
22:02:52 sean-k-mooney {}
22:03:40 sean-k-mooney so there is a race based on the execution order of the tests
22:16:14 openstackgerrit Sundar Nadathur proposed openstack/nova master: ksa auth conf and client for cyborg access https://review.openstack.org/631242
22:16:15 openstackgerrit Sundar Nadathur proposed openstack/nova master: WIP: Create and bind Cyborg ARQs. https://review.openstack.org/631244
22:16:15 openstackgerrit Sundar Nadathur proposed openstack/nova master: WIP: Add Cyborg device profile groups to request spec. https://review.openstack.org/631243
22:16:16 openstackgerrit Sundar Nadathur proposed openstack/nova master: WIP: Get resolved Cyborg ARQs and add PCI BDFs to VM's domain XML. https://review.openstack.org/631245
22:26:57 dansmith mriedem: why is that not in request_filter.py, just without a conf flag?
22:27:11 dansmith that way all these "check a thing about the request and add a trait requirement" things are in one place
22:27:17 sean-k-mooney efried: so just as an fyi we have other dodgy test too that are wrong upstream :(
22:27:29 efried sean-k-mooney: I am not surprised at all.
22:27:44 efried sean-k-mooney: I wonder if we need to file a bug for this.
22:27:51 sean-k-mooney oh we do
22:28:00 sean-k-mooney but that is not what im concerned about

Earlier   Later