| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-02-07 | |||
| 15:52:51 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: handle uninited fields in notification payload https://review.openstack.org/415857 | |
| 16:06:43 | pkholkin | johnthetubaguy: mriedem: jaypipes: baoli | |
| 16:07:15 | pkholkin | oops | |
| 16:08:46 | pkholkin | johnthetubaguy: mriedem: jaypipes: bauzas: dansmith: alex_xu: and all nova folks, I decided to leave OpenStack, thank you for your work in community and your support! maybe we will meet once again, wish you all the best and good luck! :) | |
| 16:09:19 | dansmith | pkholkin: bummer, but good luck on your next adventure! | |
| 16:09:43 | mriedem | pkholkin: thanks for your contributions and good luck in the new position | |
| 16:09:57 | pkholkin | dansmith: sorry) thank you ! | |
| 16:10:38 | pkholkin | mriedem: thanks! it was a great pleasure to work in our community! | |
| 16:15:31 | rpodolyaka | pkholkin: good luck, Pasha! really sad to see you go :( | |
| 16:16:38 | markus_z | pkholkin: too bad, thanks for your time | |
| 16:17:18 | pkholkin | rpodolyaka: me too( thank you very much, Roman ! | |
| 16:17:26 | pkholkin | markus_z: thanks! | |
| 16:19:39 | mdbooth | ARGH! | |
| 16:20:01 | mdbooth | The detach/delete race isn't fixable on the compute host | |
| 16:20:20 | mdbooth | Because api gets the bdms and sends them to the compute host, which then acts on them | |
| 16:20:26 | mdbooth | Even though they might be out of date | |
| 16:20:37 | mdbooth | Why? | |
| 16:21:11 | mriedem | mdbooth: did you find my comment in the compute manager code about that already? | |
| 16:21:34 | mdbooth | mriedem: Not yet. I have spelunked significantly, but not everywhere. | |
| 16:21:46 | mriedem | https://github.com/openstack/nova/blob/46c778efafa3ff3c7bb99d74656bb2d2869ceb16/nova/compute/manager.py#L2413 | |
| 16:22:00 | mdbooth | mriedem: Incidentally, while I'd like to fix this race, I think it's orthogonal to the NFS thing. | |
| 16:22:08 | mdbooth | Because the NFS change is safe either way. | |
| 16:22:22 | mdbooth | It's just ugly complaining about removing things which no longer exist. | |
| 16:23:10 | mdbooth | mriedem: Why not just ignore what api gave us and refresh anyway? | |
| 16:23:43 | cfriesen | mdbooth: couldn't they still be out of date, just less likely? | |
| 16:24:02 | mdbooth | cfriesen: Once we're on the compute host we can hold a lock | |
| 16:24:22 | mdbooth | But obviously only against other thing which mutate bdms on the compute host | |
| 16:24:44 | mdbooth | It would fix *this* race, for eg | |
| 16:25:08 | cfriesen | mdbooth: coming from the linux kernel I was surprised (shocked, appalled) at the locking in nova. :) | |
| 16:25:20 | mdbooth | cfriesen: locking in nova? | |
| 16:25:35 | mdbooth | cfriesen: Yeah, I'm right there with you. | |
| 16:25:42 | pumaranikar | cdent, I have updated json formatter patch. Please have a look when you get time https://review.openstack.org/#/c/395194/ | |
| 16:27:33 | mdbooth | mriedem: What's the significance of refresh if volume_id isn't set there? | |
| 16:27:58 | cdent | pumaranikar: yup, will do, is definitely on my list for today | |
| 16:28:02 | mriedem | to get the volume_id so we can delete the volume | |
| 16:28:15 | mriedem | if we're deleting the instance while it's spawning during a boot from volume where nova creates the volume | |
| 16:28:41 | pumaranikar | cdent, thanks. | |
| 16:29:37 | mdbooth | mriedem: The race here is between detach and delete | |
| 16:30:02 | mdbooth | So the issue is that when delete runs, it has a bdm in its list which has already been deleted by detach | |
| 16:30:19 | mriedem | mdbooth: there is where that comment and refresh came from https://review.openstack.org/#/c/258118/ | |
| 16:31:23 | mdbooth | mriedem: Fun. Any reason not to refresh unconditionally? | |
| 16:31:50 | mdbooth | It's slightly more expensive, but it's unlikely to add a great deal to a delete operation. | |
| 16:32:03 | mriedem | mdbooth: no, i had a targeted refresh for the specific thing i was fixing to avoid round tripping and pulling the bdm list unconditionally when i didn't need to | |
| 16:32:12 | mriedem | mdbooth: that doesn't fix the race though right? | |
| 16:32:14 | mdbooth | Critically, refresh while holding the instance lock on the compute host | |
| 16:32:30 | mriedem | so lock in delete and lock in detach | |
| 16:32:33 | mriedem | you're saying | |
| 16:32:42 | mdbooth | Yeah. delete already locks. | |
| 16:32:47 | mdbooth | I added a patch to lock in detach. | |
| 16:33:48 | mdbooth | https://review.openstack.org/#/c/430213/ | |
| 16:34:13 | mdbooth | detach currently races with itself, so I don't understand why that's not already there | |
| 16:34:47 | mriedem | what do you mean by races with itself? | |
| 16:35:07 | mdbooth | Detach the same volume multiple times in quick succession | |
| 16:35:28 | mdbooth | They'll all run concurrently. | |
| 16:38:34 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Run api sample tests against 2.latest https://review.openstack.org/430352 | |
| 16:39:19 | mriedem | mdbooth: i imagine it's not there because in that scenario, eventually the first one wins and the user doesn't see any errors in the compute but they see the detach eventually complete, so they are happy | |
| 16:39:56 | mriedem | the admin might see an error in the logs if they are monitoring | |
| 16:40:16 | mriedem | anyway, left a comment in your chnage | |
| 16:40:17 | mdbooth | mriedem: But meanwhile the backend is busy hassling cinder about it, and doing multiple cleanups simultaneously, and we've got our fingers crossed there are no side effects we haven't thought of. | |
| 16:40:18 | mriedem | *chnage | |
| 16:40:24 | mdbooth | Thanks | |
| 16:40:30 | mriedem | crap, *change* | |
| 16:40:35 | mdbooth | hehe | |
| 16:40:52 | mriedem | well like all bugs, it's not a bug until it is and someone cares about it | |
| 16:40:55 | mriedem | so shrug | |
| 16:42:30 | mdbooth | mriedem: TBH, I only care about it because I wrote a patch which causes it to be noticed :) | |
| 16:43:28 | mriedem | just put out a TC resolution to drop support for NFS and then you don't need to worry about this | |
| 16:44:11 | mdbooth | mriedem: Well it would have to be *any* filesystem based volume backend. My patch supports them all :) | |
| 16:44:34 | cdent | pumaranikar: nice work! I've left some questions within on that but they are mostly for johnthetubaguy and sdague | |
| 16:46:33 | openstackgerrit | Dan Smith proposed openstack/nova master: Add an update_cell command to nova-manage https://review.openstack.org/430354 | |
| 16:48:46 | pumaranikar | cdent, thanks for quick review :) | |
| 16:51:14 | lyarwood | mdbooth: speaking of something that end users don't really care about, shouldn't all of that logic be in the DriverVolumeBlockDevice class like it is for attach_volume? | |
| 16:51:34 | mriedem | armax: thanks for https://review.openstack.org/#/c/429984/ - i left some comments in there | |
| 16:51:57 | mdbooth | lyarwood: Nope. See code comment. | |
| 16:51:59 | armax | mriedem: thanks, I’ll have another pass | |
| 16:52:29 | mdbooth | lyarwood: It's only relevant to backends which use it. Ideally that would be all of them, but I chose not to do them all in 1 patch. | |
| 16:53:02 | dansmith | mriedem: this might be good to get in https://review.openstack.org/430354 | |
| 16:53:15 | dansmith | mriedem: it's non-runtime and gives us yet another way to tweak those records if they get created incorrectly | |
| 16:53:17 | sdague | cdent: for https://review.openstack.org/#/c/395194/11/nova/api/openstack/placement/wsgi_wrapper.py - is that always going to be a 5xx error, or are 4xx in that mix as well | |
| 16:53:34 | dansmith | mriedem: I've now encountered two people that created their records and needed to change them | |
| 16:53:48 | cdent | sdague that always not 5xx at that stage, only intentionally raised 4xx | |
| 16:53:49 | mriedem | dansmith: we have create_cell and delete_cell | |
| 16:54:22 | dansmith | mriedem: yeah, but you can't do that once you've created instances | |
| 16:54:45 | mriedem | oh yeah | |
| 16:54:48 | mriedem | fudge | |
| 16:54:57 | lyarwood | mdbooth: sorry which comment? | |
| 16:55:04 | mriedem | dansmith: let me contemplate this in the men's room | |
| 16:55:14 | cdent | sdague: the goal there is to only add the formatting. there's middleware much further "out" which deals with trapping unexpected exceptions | |
| 16:55:28 | sdague | cdent: right, so I wouldn't add the logging there then | |
| 16:55:51 | sdague | because we don't want to be logging the same error multiple times if it's already gathered elsewhere | |
| 16:58:16 | cdent | sdague: these particular excpeptions will not leave the app (into middleware) as exceptions. webob turns them into proper responses. so any logging that is happening is either right there, or when the original exception is raised | |
| 16:59:05 | cdent | the need for this superclass is precisely because of the way webob turns exceptions within wsgify decorated functions into responses | |
| 16:59:14 | mdbooth | lyarwood: https://review.openstack.org/#/c/383859/13/nova/virt/libvirt/volume/fs.py | |
| 17:00:34 | lyarwood | mdbooth: ah sorry, I'm talking about moving the _detach_volume logic from the compute layer into https://github.com/openstack/nova/blob/master/nova/virt/block_device.py#L205 like we did for attach | |
| 17:02:20 | mdbooth | lyarwood: I guess. You'd still need to resolve the locking issues in compute manager, though | |
| 17:03:38 | lyarwood | mdbooth: yeah, I just thought of it again when I saw your change, it's something Nikola wanted to do before he left but never got around to. Anyway, ignore me, sorry for the noise. | |
| 17:09:09 | sfinucan | sean-k-mooney, jaypipes: Do you know if os-vif publishes whether a VIF should support IPv6 or not? | |
| 17:09:33 | sfinucan | i.e. as part of the VIF model | |
| 17:09:37 | sean-k-mooney | no | |
| 17:09:46 | sean-k-mooney | it is not a property of the VIF | |