Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-02-07
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
17:09:57 sean-k-mooney its a property of the neutron subnet
17:10:14 sean-k-mooney i would not expect this to be reported by os-vif
17:10:48 openstackgerrit Balazs Gibizer proposed openstack/nova master: Improve assertJsonEqual error reporting https://review.openstack.org/428199
17:10:53 sean-k-mooney sfinucan: also ^^
17:11:21 sean-k-mooney sfinucan: i should have mentioned you in the reply
17:12:07 sfinucan sean-k-mooney: OK, cool
17:12:13 mriedem dansmith: you know what might be nice is to put the service version in the os-services output
17:12:25 mriedem dansmith: now that we have all of this stuff that depends on having computes at a certain version
17:12:25 mdbooth lyarwood mriedem: rebuild also disconnects twice. Once explicitly in compute manager, once again in the driver during destroy.
17:12:34 sfinucan So there's merit in keeping around the use_ipv6 option for building VIF filters in libvirt
17:12:43 sfinucan e.g. https://libvirt.org/formatnwfilter.html
17:12:44 sfinucan ?
17:13:08 sean-k-mooney sfinucan: am im not sure but i dont we use that and i dont think we should
17:13:09 dansmith mriedem: might be good to abstract it a bit if we do.. so report "LATEST" and "OLD" or something
17:13:33 sean-k-mooney sfinucan: i would like to completely remove libvirt form our port binding if we can
17:13:50 mriedem dansmith: yeah, was just thinking about a doc that armax is working on for get me a network and we don't have a way to tell you if your computes are new enough to use that
17:14:14 sfinucan sean-k-mooney: I don't think we can, assuming we want to keep the interface firewalls https://github.com/openstack/nova/blob/master/nova/virt/libvirt/firewall.py#L138-L179
17:14:30 dansmith mriedem: aye, although they log the service version on startup I think so we could also just rely on that for the short term
17:14:31 sean-k-mooney sfinucan: the only place we use libvirt for port binding currently is the non hybrid plug case for ovs. that really should be done by os-vif
17:15:12 sean-k-mooney sfinucan: no we can we just need to not add the interfaces as an openvswithc interface in the libvirt xml. just add it as a generic tap
17:15:13 lyarwood mdbooth: ewww, we can't really remove the compute layer call as other virt drivers might not do that right?
17:15:30 sean-k-mooney sfinucan: then have os-vif add it to the ovs bridge
17:15:35 mdbooth lyarwood: Presumably not
17:15:42 lyarwood mdbooth: as mriedem said before I think it's just another case of us assuming that nothing bad will happen if you disconnect_volume twice :|
17:16:01 mriedem i said that?
17:16:02 sean-k-mooney sfinucan: where is _get_instance_filter_parameters used
17:16:18 lyarwood mriedem: something along the lines of that if I understood correctly, maybe I didn't
17:16:38 sfinucan sean-k-mooney: Here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/firewall.py#L181-L193
17:16:39 mriedem if it was smart, then yes i said it
17:16:45 mdbooth hehe
17:16:59 sfinucan sean-k-mooney: Which is in turn called here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/firewall.py#L112-L136
17:17:43 sean-k-mooney yep tracking back now. im trying to figure out if this code is still executed with os-vif
17:18:06 sfinucan sean-k-mooney: I think it is. It's called in a few places in the libvirt driver.py
17:18:09 mdbooth mriedem: So, I've been through the ERRORs generated against my NFS patch with a fine toothed comb. There are 2, and they're both existing errors where we're double-calling disconnect from higher up. They cause no ill effects other than the log message.
17:18:10 sean-k-mooney basically i think we could move this to os-vif if it was still needed
17:18:21 cburgess mriedem Sorry missed your messages yesterday, yes we use live_migration_progress_timeout why?
17:18:43 mriedem cburgess: https://review.openstack.org/#/c/429798
17:18:46 mdbooth I originally made them ERROR because I didn't expect the errors higher up, which with hindsight perhaps I should have.

Earlier   Later