| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-02-28 | |||
| 14:26:25 | johnthetubaguy | we pass the cinder driver details from os-brick in the connector | |
| 14:26:39 | mdbooth | lyarwood: But it can't tell you if the driver needs to perform a global action or not | |
| 14:26:43 | johnthetubaguy | cinder folks seemed to think that could be specific enough for our needs | |
| 14:26:59 | mdbooth | e.g. connecting a gluster volume directly to a domain via a monitor call | |
| 14:27:26 | mdbooth | vs mounting a gluster volume on the host and passing a file to the domain | |
| 14:27:49 | mdbooth | The gluster driver supports both of those things currently, and only it knows which of them it's doing | |
| 14:28:07 | johnthetubaguy | so the cinder backend needs to tell cinder API what happens, if that needs to know which one we are doing, it needs to go into the connector, I assume? | |
| 14:28:09 | mdbooth | So no higher level can decide whether or not to call disconnect | |
| 14:28:16 | lyarwood | ah it's not configured locally on the host is it? | |
| 14:28:17 | johnthetubaguy | there is no higher level deciding here | |
| 14:28:18 | johnthetubaguy | its the driver | |
| 14:28:25 | lyarwood | connection_info isn't telling it which one to do? | |
| 14:28:34 | mdbooth | lyarwood: No | |
| 14:28:40 | johnthetubaguy | so it needs to add that now | |
| 14:28:48 | lyarwood | yeah | |
| 14:28:51 | johnthetubaguy | and default to a global lock if that might be possibe | |
| 14:28:55 | mdbooth | It literally can't | |
| 14:28:57 | johnthetubaguy | and it doesn't know which | |
| 14:29:01 | mdbooth | And it's none of its business anyway | |
| 14:29:07 | johnthetubaguy | it is now | |
| 14:29:37 | mdbooth | In the gluster case it's based on the qemu and libvirt versions on the host | |
| 14:29:42 | mdbooth | cinder doesn't know that | |
| 14:29:51 | johnthetubaguy | so what we are saying, I think, is the cinder driver, on the cinder side, needs to tell us if we need the lock or not | |
| 14:29:53 | mriedem | we can tell cinder via the connector dict | |
| 14:29:55 | smcginnis | There is no gluster driver, but anyway... | |
| 14:29:59 | mdbooth | As it happens we can safely remove it, but it's still an example of how cinder cannot know this | |
| 14:30:05 | johnthetubaguy | to make that possible cinder always stores the connector I think | |
| 14:30:09 | mdbooth | It's a host-side driver implementation detail | |
| 14:30:23 | mdbooth | All cinder knows is that there will be a connection from the host | |
| 14:30:30 | smcginnis | I'm trying to read the scrollback, but not sure what the issue is here. | |
| 14:30:38 | johnthetubaguy | my preference is to always call os-brick, and tell it what we know | |
| 14:30:42 | mdbooth | It's not cinder's business where that comes from | |
| 14:30:48 | johnthetubaguy | would that make things happier? | |
| 14:31:11 | mdbooth | johnthetubaguy: Yup. That's also entirely consistent with my patch. | |
| 14:31:40 | johnthetubaguy | mdbooth: I am totally fine with that approach, but I guess we will have to version that somehow, but thats fine | |
| 14:31:49 | mdbooth | johnthetubaguy: version what? | |
| 14:32:00 | johnthetubaguy | deciding if we always call detach | |
| 14:32:12 | mriedem | mdbooth: the nfs volume driver in nova today doesn't call os-brick right | |
| 14:32:13 | johnthetubaguy | although maybe thats what we always do today anyways? | |
| 14:32:13 | mriedem | ? | |
| 14:32:30 | mriedem | johnthetubaguy: we always call disconnect_volume on the volume driver today | |
| 14:32:32 | mdbooth | mriedem: No, it doesn't. None of the fs backed drivers do, I think. | |
| 14:32:42 | lyarwood | yeah correct | |
| 14:32:52 | mriedem | johnthetubaguy: then the nfs vol driver just handles a "device is busy" error and ignores it on umount | |
| 14:33:04 | johnthetubaguy | mriedem: ah, fair enough | |
| 14:33:12 | mdbooth | mriedem: ... which is broken for a few reasons. | |
| 14:33:20 | mriedem | mdbooth's patch https://review.openstack.org/#/c/383859/ is adding ref counting | |
| 14:33:26 | mriedem | so we don't blindly do that | |
| 14:33:29 | johnthetubaguy | so all this shared connection stuff is the very last bit of the cinder API changes | |
| 14:33:38 | mdbooth | Yeah | |
| 14:33:47 | mdbooth | Well "ref counting" | |
| 14:33:56 | johnthetubaguy | if we don't need it, thats cool | |
| 14:34:03 | mdbooth | Because our connect and disconnect calls don't actually match up in practise | |
| 14:34:22 | jdandrea | Can someone please clarify? From Placement API doc: "The placement service tracks the inventory and usage of each provider." Does the Placement service *perform* placement, or is it a means to that end, or is it something else? | |
| 14:34:25 | mriedem | "ref counting" is how i'm summarizing with wavy high-level hands a 1200 LOC patch :) | |
| 14:34:42 | mdbooth | It's not that big, is it? | |
| 14:34:51 | mriedem | mdbooth: massive | |
| 14:34:56 | mdbooth | Eurgh, it bloody is | |
| 14:35:10 | mriedem | jdandrea: nova-scheduler performs the actual placement, | |
| 14:35:14 | mdbooth | Anyway, have a look at how big glusterfs is using it :) | |
| 14:35:14 | mriedem | the placement service is the means to the end | |
| 14:35:50 | jdandrea | mriedem Thank you! My brain was running in circles, confused as to why it isn't called an Inventory API, f'rinstance. | |
| 14:35:55 | mriedem | jdandrea: the placement service holds the global view on resources in the deployment, and nova-scheduler asks it which resource providers it knows about can fit a given instance build/move request | |
| 14:35:55 | mdbooth | It's all about error handling, and it can be reused without modification by all the other filesystem drivers | |
| 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 | |