| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-21 | |||
| 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? | |
| 15:06:19 | dansmith | yeah | |
| 15:06:22 | mriedem | they'd likely be in nova.tests.unit.virt.test_block_device, which is mox city | |
| 15:06:31 | dansmith | right, but I don't see any | |
| 15:06:56 | mriedem | could also be some tests in the old nova.tests.unit.compute.test_compute that didn't get ported, | |
| 15:07:04 | mriedem | that driver_detach code used to live in the compute manager | |
| 15:07:07 | mriedem | lyarwood moved it over | |
| 15:07:11 | mriedem | not sure all the tests made the journey | |
| 15:07:15 | dansmith | grep driver_detach -r nova/tests/unit/virt/ | |
| 15:07:17 | dansmith | returns nothing | |
| 15:07:29 | mriedem | b/c it's a compute manager thing | |
| 15:07:32 | mriedem | it's not a virt driver thing | |
| 15:07:40 | dansmith | eh? | |
| 15:07:43 | mriedem | it's like everything else in nova.virt.block_device, | |
| 15:07:47 | mriedem | it's an extension of the compute manager | |
| 15:07:47 | dansmith | test_block_device is in there | |
| 15:08:22 | dansmith | # TODO(lyarwood): Test DriverVolumeBlockDevice.driver_detach in | |
| 15:08:23 | mriedem | i know, it's called that because that's what ndipanov called the driver-view bdm wrapper object things or whatever | |
| 15:08:28 | mriedem | bingo | |
| 15:08:34 | dansmith | I see no unmocked calls of it in compute/ either | |
| 15:09:52 | mriedem | you can be the first one | |
| 15:10:56 | dansmith | and I shall | |
| 15:12:39 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Purge comp_node and res_prvdr records during deletion of cells/hosts https://review.openstack.org/546660 | |
| 15:14:17 | mriedem | jaypipes: dansmith: bauzas: i assume you all would be very much -1 on this also https://review.openstack.org/#/c/546482/ | |
| 15:14:29 | mriedem | i'm actually pretty annoyed that's being proposed given it was already discussed in queens and we said no | |