| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-02-28 | |||
| 14:36:06 | jdandrea | mriedem *nodnod* got it | |
| 14:36:44 | mdbooth | mriedem: Look at this: https://review.openstack.org/#/c/423402/ | |
| 14:36:46 | jdandrea | Appreciatd! | |
| 14:36:50 | jdandrea | (and also appreciated) | |
| 14:36:52 | jdandrea | :) | |
| 14:36:52 | mdbooth | I can do that to ever other filesystem driver | |
| 14:37:14 | mriedem | mdbooth: still https://review.openstack.org/#/c/383859/19/nova/virt/libvirt/volume/mount.py@192 | |
| 14:37:19 | mriedem | what am i missing there? | |
| 14:37:31 | mdbooth | mriedem: It's aset | |
| 14:37:34 | mdbooth | a set | |
| 14:37:59 | mdbooth | So you can add (vol_a, instance_a) and (vol_a, instance_b) | |
| 14:38:02 | mdbooth | and they're different things | |
| 14:38:16 | mdbooth | When you remove (vol_a, instance_b), the mountpoint is still in use | |
| 14:38:20 | mdbooth | There's a test for it | |
| 14:38:33 | mriedem | ah because self.attachments is still len == 1 | |
| 14:38:38 | mdbooth | Yeah | |
| 14:40:06 | mdbooth | mriedem: Anyway, don't look at this as a 1200 line nfs driver patch. It's a 1200 line driver for every filesystem volume driver with really good error handling and really good tests. | |
| 14:40:49 | mriedem | yeah i know - you also told me how really good it was right before rc1 and i found an issue in there, so the size and complexity does matter :) | |
| 14:40:53 | mriedem | not trying to knock it, but you know | |
| 14:41:28 | mriedem | i was just trying to sort out why the multiattach stuff was playing into it since the new bottom patch in the series is new | |
| 14:41:31 | johnthetubaguy | mdbooth: is there are reason not to use the usual oslo.concurrency lock thing instead of using threading directly? | |
| 14:41:51 | mdbooth | johnthetubaguy: Probably. Do you have a specific? | |
| 14:42:14 | johnthetubaguy | mdbooth: well I was just expecting to see us generate a string, and use that for the lock, in your locking patch | |
| 14:43:24 | mdbooth | johnthetubaguy: I think my solution is much simpler. I don't think oslo.concurrency locks were really designed for this. | |
| 14:44:02 | johnthetubaguy | mdbooth: https://review.openstack.org/#/c/383859/19/nova/virt/libvirt/volume/mount.py@183 and https://review.openstack.org/#/c/383859/19/nova/virt/libvirt/volume/mount.py@295 and https://review.openstack.org/#/c/383859/19/nova/virt/libvirt/volume/mount.py@62 | |
| 14:44:23 | johnthetubaguy | mdbooth: its just not following the pattern I was expecting that we know is eventlet friendly | |
| 14:44:25 | mdbooth | johnthetubaguy: I don't think you can do conditions with oslo.concurrency for eg, can you? | |
| 14:44:50 | mdbooth | johnthetubaguy: This is all eventletised, as I have discovered | |
| 14:44:56 | mdbooth | It's monkey patched | |
| 14:45:04 | johnthetubaguy | yeah, I figured it had to be | |
| 14:45:27 | mdbooth | I was going to use evently primitives explicitly, in fact I think I had one version where I did that | |
| 14:45:44 | mdbooth | But iirc the api is a bit crippled compared to the python threading api | |
| 14:45:52 | johnthetubaguy | just I know getting this stuff "correct" is surpizingly hard, I was kinda expecting us to use some more higher level construct, but I am probably missing something | |
| 14:47:05 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove mox in nova/tests/unit/compute/test_shelve.py (3) https://review.openstack.org/342648 | |
| 14:47:28 | johnthetubaguy | mdbooth: its probably the need for notify_all that rings alarm bells, I remember that being tricky to get correct | |
| 14:47:34 | mriedem | dansmith: melwitt: sdague: btw, i enjoyed some wonderful high fiber whole grain cereal this morning | |
| 14:47:49 | johnthetubaguy | mdbooth: we could just use a regular lock to protect the counts, and if we did, I am not sure what we would loose | |
| 14:47:53 | mdbooth | johnthetubaguy: Not really, if you ensure all your waits are guarded. | |
| 14:48:15 | dansmith | mriedem: sounds like my morning 18 minutes from now | |
| 14:49:05 | mdbooth | johnthetubaguy: You'd lose condition.wait(), which is a really useful construct | |
| 14:49:42 | johnthetubaguy | mdbooth: but why not guard the count with a regular lock? with eventlet I can't see there being much of a gain between the two, and one is more obviously "OK ish" | |
| 14:49:54 | johnthetubaguy | I am probably missing something big | |
| 14:50:35 | mdbooth | johnthetubaguy: Yeah, so I want to wait until the in-use count reaches zero. | |
| 14:50:46 | mdbooth | I take the lock | |
| 14:50:51 | mdbooth | It's not zero | |
| 14:50:55 | mdbooth | What do I do? | |
| 14:51:12 | mdbooth | This is exactly what the Condition api is for | |
| 14:51:30 | johnthetubaguy | mdbooth: ah, so its because you want to wait rather than just sleep or something dumb like that, thats what I was missing | |
| 14:51:49 | mdbooth | Yeah, otherwise you have to drop the lock and poll | |
| 14:52:09 | mdbooth | That uses more resources, and it's more complicated | |
| 14:52:22 | mdbooth | And there's an existing API which designed for exactly this | |
| 14:52:39 | johnthetubaguy | mdbooth: we could just do callbacks on triggers I guess? | |
| 14:52:51 | mdbooth | That's not sounding simpler :) | |
| 14:53:04 | mdbooth | Condition gives us: wait until X is true | |
| 14:53:10 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: ironic inventory reporting https://review.openstack.org/437602 | |
| 14:54:51 | mdbooth | johnthetubaguy: I also wrote a slightly funky test library for this, btw | |
| 14:54:56 | sdague | mriedem: thank you for sharing | |
| 14:55:08 | mdbooth | It's in that patch, but it's entirely generic | |
| 14:55:30 | mdbooth | It allows you to insert arbitrary wait points in test code so you can force race conditions | |
| 15:03:28 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Fix default_availability_zone docs https://review.openstack.org/438887 | |
| 15:03:45 | sfinucan | mdbooth: The fact that your Red Hat email isn't also mdbooth annoys me | |
| 15:04:00 | sfinucan | mriedem: That run_tests patch you requested is here https://review.openstack.org/#/c/437947 | |
| 15:05:13 | johnthetubaguy | mdbooth: maybe the bit I am not getting is why you have to reset the state when you loose connection to libvirt? | |
| 15:14:28 | mdbooth | johnthetubaguy: That was originally danpb's suggestion. It's a robustness measure. | |
| 15:14:49 | mdbooth | johnthetubaguy: We need to tie initialisation to host_up anyway, because initialisation uses the libvirt connection. | |
| 15:15:01 | johnthetubaguy | yeah, host up makes sense | |
| 15:15:07 | mdbooth | So re-initialising it after host_down is no great complication. | |
| 15:15:08 | johnthetubaguy | just seems odd to drop it | |
| 15:15:28 | mdbooth | johnthetubaguy: Well, if libvirt went down we don't really know what happened. | |
| 15:16:06 | mdbooth | That doesn't really complicate the code all that much, anyway, as it's separate in the manager | |
| 15:17:24 | bauzas | jaypipes: around ? | |
| 15:17:43 | bauzas | jaypipes: I have a question for you before +Wign https://review.openstack.org/#/c/376198/11 | |
| 15:18:24 | mdbooth | The code is especially paranoid as it's reference counting. We want to do our best to be correct, even if operators are doing unexpected things underneath us, or things are crashing. | |
| 15:18:41 | bauzas | jaypipes: ie. we set the collation to latin1 encoding, I guess it's for memory reasons, right? | |
| 15:19:19 | bauzas | I just wonder if that's really worth it in case we would need to add another column needing utf8 encoding, like a description TEXT field | |
| 15:20:34 | jaypipes | bauzas: gimme a bit.. back to back meetings | |
| 15:20:43 | bauzas | heh, no worries | |
| 15:21:10 | mriedem | bauzas: before you go approving that code, | |
| 15:21:11 | mriedem | https://blueprints.launchpad.net/openstack/?searchtext=resource-provider-traits | |
| 15:21:15 | mriedem | can we get the bp actually approved? | |
| 15:21:34 | bauzas | oh very good point | |
| 15:21:49 | bauzas | was there a spec reproposed ? | |
| 15:22:09 | mriedem | https://review.openstack.org/#/c/345138/ | |
| 15:22:12 | bauzas | there it is https://review.openstack.org/#/c/345138/ | |
| 15:22:14 | bauzas | yeah | |
| 15:23:00 | bauzas | here is the thing, given it would take more time for that as we haven't previously approved the spec | |
| 15:23:17 | bauzas | mriedem: I would then wonder if it would be worth decoupling that spec into 2 pieces | |
| 15:23:30 | mriedem | i haven't read the spec, | |
| 15:23:31 | spotz | Hey nova, anyone about who could review the doc we're working on for the placement API and cells and give feedback? We can't build instances so we're missing something https://review.openstack.org/#/c/438328/ | |
| 15:23:33 | bauzas | mriedem: one would be the the basement piece we agreed as the top prio | |
| 15:23:38 | mriedem | if it's the full hog of traits, then yeah probably | |
| 15:23:42 | bauzas | mriedem: the other would be exposing it thru the API | |
| 15:23:48 | mriedem | since we just need the piece to fix can_host | |
| 15:24:14 | mriedem | spotz: have you seen the docs in the nova devref? | |
| 15:24:28 | mriedem | https://docs.openstack.org/developer/nova/cells.html | |
| 15:24:33 | mriedem | https://docs.openstack.org/developer/nova/placement.html | |
| 15:24:59 | dansmith | spotz: also, kinda need to know the error you're hitting to know what you're missing | |
| 15:27:05 | spotz | dansmith: I'm getting the very unhelpful No valid hosts, I'm not sure what Brian got last night though | |
| 15:27:24 | dansmith | spotz: got host_mappings in the apidb? | |
| 15:28:41 | spotz | dansmith. Let me bring up the VM and double check | |