| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-21 | |||
| 12:36:13 | sean-k-mooney | for dpdkvhostuserclient the unix socket is created by qemu so it is definetly not deleted | |
| 12:36:25 | sahid | so this issue does not exist for dpdkvhostuserclient | |
| 12:37:03 | sean-k-mooney | sahid: i cant say 100% with out testing but i think it would be significatly less likely to happen | |
| 12:37:22 | sahid | that is also my thinking | |
| 12:37:36 | sahid | so i don't know whether we really want to fix that | |
| 12:37:46 | sahid | starts to ocata we are using dpdkvhostuserclient | |
| 12:37:56 | sahid | and i think dpdkvhostuser is deprecated, right? | |
| 12:38:09 | sean-k-mooney | sahid: well we are using dpdkvhostuserclient if ovs is new enough | |
| 12:38:25 | sean-k-mooney | dpdkvhostuser is not but dpdkvhostcuse was and has been removed | |
| 12:38:57 | sean-k-mooney | dpdkvhostuser is just not recommended but no one has pushed for removal/deprecation yet as far as i know | |
| 12:39:01 | sahid | oh yes we are looking at the capabilities to know which one to user | |
| 12:39:03 | sahid | use | |
| 12:39:23 | sahid | ok so let fix it | |
| 12:39:58 | sean-k-mooney | for ovs-vsctl its should be a 2 line fix. want me to trow up a patch | |
| 12:40:57 | sahid | no worries i will do i have also a env to test it | |
| 12:41:27 | sahid | sean-k-mooney: will ping you when it's sent upstrea | |
| 12:42:19 | sean-k-mooney | cool it should just be changing "'--if-exists', 'del-port', dev, '--','add-port'" -> "'add-port','--may-exist'" https://github.com/openstack/os-vif/blob/master/vif_plug_ovs/linux_net.py#L68 and maybe updating some unit tests | |
| 12:43:02 | sean-k-mooney | actully the --may-exists possibly goes before add-port but youll figure it out | |
| 12:44:40 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Unmap compute nodes when deleting host mappings in delete cell operation https://review.openstack.org/542964 | |
| 13:15:13 | openstackgerrit | sahid proposed openstack/os-vif master: ovs: do not delete port if already exists https://review.openstack.org/546588 | |
| 13:20:19 | sahid | sean-k-mooney: ^ i still need to test it but if you want to have a llok | |
| 13:46:53 | openstackgerrit | Shoham Peller proposed openstack/nova master: Handle spawning error on unshelving https://review.openstack.org/378009 | |
| 14:15:15 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: [WIP] libvirt: Allow to specify granular CPU feature flags https://review.openstack.org/534384 | |
| 14:20:52 | mriedem | mnaser: some comments / thoughts in https://review.openstack.org/#/c/546398/ | |
| 14:24:06 | mnaser | mriedem: if i bdm.attachment_id = None .. that means that i can drop 'attachment_id' in bdm ? | |
| 14:24:25 | mnaser | and i can change the test to make sure that i always get an attachment that is =None i guess? | |
| 14:25:10 | mnaser | that does feel like a much cleaner fix because then we won't carry the extra cruft of an issue that exists in the old flow of attachment which will one day disappear | |
| 14:26:30 | mriedem | mnaser: i think that is true... | |
| 14:26:54 | mnaser | mriedem: i have a functional test so ill make the change and see if it does address it | |
| 14:27:06 | mriedem | the only time we do a local delete in the api and the bdm.attachment_id wouldn't be set is this pre-scheduling thing where we delete the instance via the build_request | |
| 14:27:42 | mnaser | mriedem: and if i understand correctly, the bdms we get from the build_request are not persisted in db so they don't have an attachment_id | |
| 14:27:44 | mriedem | if the bdm does exist in a cell database, when the api loads it out, it will set bdm.attachment_id b/c of whatever is in the db record | |
| 14:28:03 | mriedem | they aren't persisted in a cell block_device_mappings table, | |
| 14:28:20 | mriedem | they are serialized as a json blob (like the instance) and stored in the nova_api.build_requests table | |
| 14:28:24 | mnaser | gotcha | |
| 14:28:33 | mriedem | which means, when we load them into the object, we don't set any default field values from the actual db record | |
| 14:28:41 | sahid | mriedem: any chance you ack this: https://review.openstack.org/#/c/515008/ ? | |
| 14:28:54 | mriedem | like if we load up a bdm record created 3 years ago, the attachment_id entry will be null and we'll set that on the object | |
| 14:29:17 | mnaser | mriedem: ok, so but if we did change the behaviour to set a default value, i assume that would involve bumping the object version which wouldn't make it very backportable | |
| 14:29:34 | mriedem | mnaser: no i'm not suggesting we change the object at all | |
| 14:29:45 | mnaser | mriedem: oh i know, i'm just thinking out loud | |
| 14:29:48 | mriedem | ok | |
| 14:29:50 | mnaser | trying to understand nova's codebase a tad bit more | |
| 14:30:14 | mnaser | anyways im going to test out your idea and see if it passes locally | |
| 14:34:26 | efried_omalley | Ye gods, even reading just the *titles* of 882 summit talks is a labor of hours. | |
| 14:35:59 | sean-k-mooney | sahid: im not sure if the intel nfv ci is triggering on os-vif. it should be triggering on any change to nova,neutron,os-vif,networking-ovs-dpdk and devstack but if it is that will do some validation of https://review.openstack.org/#/c/546588/ | |
| 14:37:14 | openstackgerrit | Rajesh Tailor proposed openstack/nova master: Reset instance status on invalid host https://review.openstack.org/401009 | |
| 14:37:46 | sean-k-mooney | sahid: zuul seams to be happy with it so that is a good sign. | |
| 14:38:33 | mriedem | lyarwood: since sahid left, question in https://review.openstack.org/#/c/515008/ | |
| 14:40:07 | mriedem | lyarwood: i mean, i guess the patch is ok in that if one thing fails the user has to retry (either updating the guest xml or the disconnect from the host) and either thing is now idempotent | |
| 14:40:19 | mriedem | just seems like you'd want to try the cleanup thing that's more likely to fail first | |
| 14:41:50 | dansmith | mriedem: trivial review to correct a stack trace in logs that confused multiple support people: https://review.openstack.org/#/c/546423/ | |
| 14:42:19 | jaypipes | efried_omalley: I can shortcut that for you. "Edge", "NFV", "Hyperconverged", "Kubernetes". Rinse and repeat. | |
| 14:42:31 | mriedem | dansmith: timely given i'm asking about https://review.openstack.org/#/c/515008/ | |
| 14:42:58 | dansmith | hah | |
| 14:43:04 | efried_omalley | jaypipes: Yeah, would be nice to have a list of checkboxes like Amazon does, where I could exclude those terms and filter down to the 20 or so talks that might interest me. | |
| 14:44:03 | dansmith | jaypipes: lol | |
| 14:44:10 | dansmith | jaypipes: early or late? | |
| 14:44:30 | jaypipes | dansmith: early of course. | |
| 14:44:38 | dansmith | jaypipes: good, just checking. | |
| 14:44:59 | jaypipes | dansmith: you know... pre-cloud. | |
| 14:45:16 | mnaser | wow i just realized this bug has 3 paths, < BFV_RESERVE_MIN_COMPUTE_VERSION --> _check_attach (reserves), >= BFV_RESERVE_MIN_COMPUTE_VERSION -> _check_attach_and_reserve_volume (can reserve if cinder version is too old), >= CINDER_V3_ATTACH_MIN_COMPUTE_VERSION -> _check_attach_and_reserve_volume (will create attachment) | |
| 14:45:18 | dansmith | jaypipes: even the ultra-modern starship days were pre-cloud, so I had to ask | |
| 14:45:29 | sean-k-mooney | jaypipes: im trying to think of something you missed but yep i think those 4 buzz words some it up well maybe add "serverless" of "FaaS" | |
| 14:45:38 | mnaser | so we reserve a volume in the _check_attach_and_reserve_volume path too | |
| 14:45:42 | mnaser | (or we could) | |
| 14:46:14 | mnaser | OH and also if the cinder api doesn't support attachments it will reserve too | |
| 14:46:27 | jaypipes | sean-k-mooney: I think you meant "FaaD". | |
| 14:51:32 | dansmith | mriedem: I think I answered your question on that patch | |
| 14:52:42 | lyarwood | mriedem: https://review.openstack.org/#/c/515008/13/nova/virt/libvirt/driver.py@1555 - otherwise we'd likely see QEMU getting into D state if the instance attempts to write to the underlying storage after we've disconnected it from the compute | |
| 14:53:05 | dansmith | lyarwood: if it even lets us disconnect | |
| 14:53:10 | lyarwood | right | |
| 14:53:17 | dansmith | I would expect it to always fail, | |
| 14:53:25 | dansmith | unless we're echoing into sdX/delete in sysfs | |
| 14:53:50 | lyarwood | yeah there are some backends in os-brick that force things to disconnect like that | |
| 14:53:55 | dansmith | ack | |
| 14:54:00 | dansmith | then that would be like bad and stuff :D | |
| 14:54:13 | mriedem | ok +2 on sahid's | |
| 14:54:15 | mriedem | -1 on dansmith's | |
| 14:55:14 | dansmith | mriedem: raising from there isn't putting the instance into error state, | |
| 14:55:19 | dansmith | but you're saying it does get a fault? | |
| 14:55:26 | mriedem | i never said it would be put into error state | |
| 14:55:36 | mriedem | but yes there is (1) a fault recorded and (2) the instance action event record is marked as failed | |
| 14:55:43 | mriedem | which are ways for an api user to track the detach volume operation in nova | |
| 14:55:47 | dansmith | I know you didn't, but I thought we didn't record a fault unless we put it into error state | |
| 14:55:52 | mriedem | nope | |
| 14:56:03 | mriedem | it's anytime a compute manager method raises an unhandled exception | |
| 14:56:12 | dansmith | okay, so the two people that hit this yesterday weren't looking at faults | |
| 14:56:18 | dansmith | it just never happened, | |
| 14:56:25 | dansmith | and then they saw the trace in the log | |
| 14:57:13 | openstackgerrit | sahid proposed openstack/nova stable/queens: libvirt: disconnect volume from host during detach https://review.openstack.org/546655 | |
| 14:57:16 | mriedem | from what i'm reading in the bug, they are just saying, 'don't traceback, give me a useful message' | |
| 14:57:31 | dansmith | right | |
| 14:57:36 | mriedem | your patch is doing the useful message, but then saying the detach was successful | |
| 14:57:38 | mriedem | by returning | |
| 14:58:21 | dansmith | mriedem: returning isn't saying it's successful because the volume is still attached, but I see your point | |
| 14:58:39 | mriedem | well, success as in masking the failure | |
| 14:58:45 | dansmith | having to poll the volume status and then poll instance faults if it doesn't detach in a while is obscure | |
| 14:59:10 | mriedem | they can poll the action record | |
| 14:59:15 | mriedem | more than the fault | |