| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-02-07 | |||
| 14:40:55 | sfinucan | johnthetubaguy: Yeah, so my reply to the first option also stands for the second one | |
| 14:41:16 | sfinucan | We're overloading the value for libvirt's firewall and that option should be either (a) removed altogether or (b) moved to the libvirt group | |
| 14:41:32 | johnthetubaguy | sfinucan: agreed with the first one, but trying to workout what does the neutron firewalling? isn't it using this code too? https://github.com/openstack/nova/blob/d5f113de34589933c23a7cecc25ca0bf5f47207d/nova/virt/libvirt/firewall.py#L165 | |
| 14:42:01 | johnthetubaguy | sfinucan: I think its shared in virt via this one though: https://github.com/openstack/nova/blob/4f91ed3a547965ed96a22520edcfb783e7936e95/nova/virt/firewall.py#L307 | |
| 14:42:18 | johnthetubaguy | sfinucan: I guess the others maybe don't use it for any neutron drivers yet, unsure | |
| 14:43:24 | sfinucan | johnthetubaguy: Hmm, that's a good point | |
| 14:45:23 | sfinucan | johnthetubaguy: If we're reading that right, the comments for 'use_ipv6' are also wrong https://github.com/openstack/nova/blob/d5f113de34589933c23a7cecc25ca0bf5f47207d/nova/virt/libvirt/firewall.py#L158 | |
| 14:45:49 | sfinucan | https://github.com/openstack/nova/blob/d5f113de34589933c23a7cecc25ca0bf5f47207d/nova/conf/netconf.py#L68-L70 | |
| 14:45:53 | johnthetubaguy | sfinucan: yeah, it looks that way | |
| 14:46:26 | sfinucan | grr, whoever thought it was a good idea to reuse nova-net options for neutron deserves a smack | |
| 14:46:27 | sfinucan | :) | |
| 14:51:57 | sfinucan | And use_ipv6 is *also* used for that injected network template (compute.injected_network_template). Ugh... | |
| 15:00:28 | lyarwood | https://review.openstack.org/#/c/427364/ - I'd appreciate reviews on this swap_volume bugfix if any cores have time today. sfinucan, mdbooth's NFS fix now depends on this btw. | |
| 15:02:47 | sfinucan | lyarwood: I'll take a gawk shortly | |
| 15:02:55 | lyarwood | sfinucan: cheers | |
| 15:05:01 | openstackgerrit | Jordan Pittier proposed openstack/nova master: api-ref: document the 'tenant_id' query parameter https://review.openstack.org/430290 | |
| 15:06:15 | lyarwood | hmmmm there's no #openstack-devstack channel, does anyone know if openstack-infra is the best place to ask for devstack reviews? I'm looking for reviews on https://review.openstack.org/#/c/427602/ so our ceph job can pass again... | |
| 15:06:41 | dansmith | lyarwood: -qa | |
| 15:07:01 | lyarwood | dansmith: thanks | |
| 15:20:53 | sfinucan | johnthetubaguy: Out of curiosity, what's the finger in the air estimate for cells v1 going bye bye? | |
| 15:21:00 | sfinucan | ...and, in turn, nova-network | |
| 15:21:05 | openstackgerrit | Christopher Martin proposed openstack/nova master: Stop using mox in test_compute_cells.py https://review.openstack.org/389750 | |
| 15:22:01 | johnthetubaguy | sfinucan: we need to implement the transition code, then let that cook for a release. | |
| 15:22:12 | johnthetubaguy | sfinucan: after we actually support two cells v2 cells | |
| 15:22:30 | johnthetubaguy | sfinucan: so another two or three cycles till we delete, maybe :( | |
| 15:22:41 | sfinucan | ouch | |
| 15:23:04 | johnthetubaguy | sfinucan: looking after our users is expensive | |
| 15:23:14 | sfinucan | johnthetubaguy: aye, and sadly necessary | |
| 15:32:55 | dansmith | sfinucan: but we should only maintain n-net for cellsv1 users with legit bugs I think, not just anyone | |
| 15:34:10 | sfinucan | dansmith: I was hoping for an easy way out of unravelling the nest of conf options that are shared between nova-net and libvirt/Xen/other things | |
| 15:34:25 | sfinucan | namely, wait until nova-net is gone then remove the ones we don't have references for anymore :) | |
| 15:34:29 | dansmith | heh | |
| 15:37:15 | sfinucan | johnthetubaguy: So I'm figuring out what nova-network options /have been/ repurposed and duplicate them in other groups with UPGRADE renos | |
| 15:37:56 | sfinucan | then I'm going to mass migrate all nova-net opts to a single conf file which we can leave to d̶i̶e̶ mature | |
| 15:38:01 | sfinucan | Sound fair? | |
| 15:38:21 | johnthetubaguy | that sounds like a good plan | |
| 15:40:41 | sfinucan | (y) | |
| 15:46:13 | BobBall | johnthetubaguy: Thanks for the review - are you +2 on the revision to https://review.openstack.org/#/c/424428/ or are further changes needed? | |
| 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* | |