| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-21 | |||
| 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 | |
| 14:59:25 | mriedem | if the action fails, check the fault for why | |
| 14:59:42 | dansmith | it's just not what people do I think | |
| 14:59:48 | mriedem | although we also store that in the instance action event record, but you can only see the traceback there (like a fault) if you're an admin | |
| 14:59:49 | dansmith | they do a thing, they expect it to work | |
| 15:00:21 | mriedem | sure, but you see that with this patch, we fail, and return, we don't record a fault or say the action failed via the instance action record | |
| 15:00:24 | mriedem | whereas before we would | |
| 15:00:37 | mriedem | if the bug is just "don't log a traceback because that's not helpful" we can handle that | |
| 15:00:57 | dansmith | no, I get that this removes a fault that was being added before | |
| 15:01:05 | dansmith | I'm not arguing against fixing that, | |
| 15:01:28 | dansmith | I'm just ruminating about two separate support engineers (and me) not even thinking about polling for the action or fault | |
| 15:02:38 | dansmith | I guess this also deletes the bdm if we don't raise | |
| 15:02:50 | mriedem | which then means you can't try to detach it again.. | |
| 15:02:55 | dansmith | yeah | |
| 15:03:02 | mriedem | lyarwood would say you'd be proper fucked | |
| 15:03:04 | mriedem | i assume | |
| 15:04:56 | lyarwood | rather | |
| 15:05:52 | dansmith | hmm, we have no tests for that driver_detach method? | |
| 15:06:10 | mriedem | in DriverVolumeBlockDevice? | |