Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-02-28
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
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.

Earlier   Later