Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-13
16:10:10 jgwentworth I thought I've asked him about that before. maybe he's holding out on me
16:10:56 mriedem not in the UI that i know of,
16:10:59 mriedem via the REST API yes
16:11:09 superdan I think LP has a bunch of inaccessible query strings you can use
16:11:13 mriedem right
16:11:32 mriedem https://github.com/openstack/nova-specs/blob/master/tools/move_implemented_specs.py
16:12:40 sahid superdan: i saw your comments and questioning on the tx/rx queue size patch. I tried to give more details I hope that is going to help
16:15:31 superdan sahid: okay the thing I'm worried about is I thought that the guest had to support vhost (or is it vhostuser?) in a specific way, and I haven't gone to chase down all the places where we choose between those different types of virtio backends
16:15:31 jgwentworth mriedem: cool script
16:18:22 cfriesen mriedem: here's the bug:
16:18:23 cfriesen LOG.info("Device %s not found in instance.",
16:18:25 cfriesen disk_dev, instance=instance
16:18:30 cfriesen grr......https://bugs.launchpad.net/nova/+bug/1763766
16:18:30 openstack Launchpad bug 1763766 in OpenStack Compute (nova) "nova needs to disallow topology changes on image rebuild" [Undecided,New]
16:20:14 superdan sahid: okay re-reading this with your context I have a little better idea on what to look at
16:20:20 sahid superdan: the driver are on the guest OS. that is your question?
16:20:23 superdan sahid: I'll try to do that while you wor on your unit test
16:20:32 superdan sahid: yeah, the guest
16:21:02 superdan sahid: I thought that if we specify one of those (I think vhostuser) the guest _has_ to have something new enough to support it?
16:21:15 superdan although maybe that doesn't matter since you're only overriding back to vhost/
16:22:51 superdan sahid: I guess it would be less confusing if you did your refactor of get_base_config() first
16:23:09 mriedem jgwentworth: one thing i've always meant to do with that is move the validation that the spec filename matches the blueprint in LP into the doc/test validation code that runs on each patch
16:23:17 sahid well for vhostuser the virtio driver needs to be recent enough and for vhost the kernel recent enough
16:23:18 superdan sahid: can/do we validate some of these types in upstream CI?
16:23:19 mriedem since people frequently make that mistake and then we have to fix it up later
16:24:01 jgwentworth mriedem: you mean in the tox run of nova-specs
16:24:06 mriedem yeah
16:24:08 sahid superdan: yes probably i was thinking about to refactor that method first...
16:24:08 superdan sahid: yeah, I'm just trying to make sure we're not ever selecting something different after this patch, or at least, selecting something different than what would work today
16:24:20 sahid oh i see
16:24:22 jgwentworth makes sense
16:24:26 sahid normally no :)
16:24:51 superdan sahid: if you did that first, then if we have a problem with this later, it would point to the refactor patch instead of your queues patch which would be better I think
16:25:13 superdan sahid: "if we had a problem" meaning.. we selected vhostuser when we should/would have selected vhost or something
16:26:19 jgwentworth mriedem: I think the only wrinkle there could be the LP timeouts that I've run into a fair bit blocking people's specs from passing the tox check
16:26:47 sahid superdan: the thing is the refactor could be big i wanted to limit the scope
16:27:23 sahid i will have to touch lot of thing like the multiqueue support
16:27:41 superdan sahid: so we call all of this from a block that says "if os_vif, do _get_config_os_vif(), otherwise call these per-type methods"
16:28:00 superdan sahid: do we call the os_vif path in this case or not?
16:28:29 sahid superdan: yes for bridge
16:29:01 sahid and actually for vhostuser
16:29:59 superdan okay, so, vif_type could be bridge, where we end up with driver as qemu or vhost, and then vif_type could be vhostuser where we always have driver as vhostuser, is that right?
16:30:13 sahid yes that is the point
16:30:27 superdan any other types that would have driver=vhost
16:30:27 superdan /
16:30:28 superdan ?
16:30:47 sahid yes except if the virt_type is qemu
16:31:27 mriedem jgwentworth: yeah good point
16:31:38 superdan sahid: I'm not sure I know what you mean.. bridge can be vhost or qemu, are there things other than bridge that can use vhost as the driver?
16:32:45 superdan oh, virt-type you meant parallels being the alternative I guess
16:32:52 sahid i mean we always want driver=vhost except when virt-type!=kvm
16:33:55 sahid i need to check my thinking is other driver may use vhost
16:34:02 sahid other vif
16:34:28 openstackgerrit ayoung proposed openstack/nova master: Set scope_types for admin password policies https://review.openstack.org/553613
16:34:55 superdan sahid: did you say that vhostuser would go through os_vif now too?
16:36:15 sahid i'm not sure i understand what you mean by go throught os_vif
16:37:05 sahid all ovs and linux bridge vifs pass go through os_vif
16:37:14 sahid here we are configuring guest xml
16:37:45 sahid os vif is configuring the network on host
16:38:02 superdan sahid: this: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L554-L559
16:38:29 superdan because if we take that path, we never call get_config_vhostuser()
16:39:00 sahid get_base_config is called anytime, and yes we have a specific class in os-vif for vhostuser
16:39:08 sahid everytime
16:39:30 superdan sahid: right, so will this ever get run? https://github.com/openstack/nova/blob/master/nova/virt/libvirt/vif.py#L420
16:39:44 sahid but no real relation is just about to pass some metadata which will be used by ovs
16:40:32 sahid hum that is why i said i need to double check i think vrouter is using that method
16:40:59 sahid but perhaps they now have a plugin in os_vif
16:41:04 superdan okay I'm not sure how that would get called
16:42:47 sahid superdan: i just check, you think we sould remove it?
16:43:28 superdan sahid: well, your patch modifies it, and it's one of the things I was trying to verify, so if it's dead code then we should remove it so I don't have to ask questions about whether or not your changes are correct :)
16:45:13 sahid i'm just asking myself why we don't have removed it when we have introduced it in os_vif
16:45:31 sahid i will try to find the git commit log
16:46:27 superdan it looks like we do have a vhostuser conversion in that utils moduke,
16:46:39 superdan but it can raise NotImplemented, so maybe there are cases where it isn't covered?
16:49:00 sahid i can't see when it can raise a NotImplemented, can yu point me the line?
16:49:29 superdan yep, sec
16:49:58 superdan https://github.com/openstack/nova/blob/master/nova/network/os_vif_util.py#L408-L408
16:50:29 superdan other methods in there don't raise it in a final else
16:50:40 superdan but maybe you can tell that that should never happen?
16:50:55 sahid superdan: i can't :/
16:51:10 sahid let see what it blame is saying
16:54:55 superdan the set_config_VIFVHostUser method is only used from the os_vif path
16:55:08 superdan so that covers us for the os_vif vif_types I guess
16:59:30 sahid superdan: so what is your thinking?
16:59:48 superdan sahid: I just added some more comments to the review,
17:00:04 superdan but you're only setting the queue sizes if we do _not_ convert the vif in os_vif_util,
17:00:14 superdan meaning only if it raises NotImplementedError,
17:00:28 superdan which would mean we're not setting queue sizes for the more common types of VIFs right?
17:00:54 superdan i.e. you're only setting the sizes if we _fail_ the os_vif test and fall back to this method that we think might actually be dead code
17:04:56 sahid superdan: for you first question, get_base_config is called even when we build os_vif object
17:04:57 superdan sahid: ah wait, nevermind that last point
17:08:20 superdan sahid: okay, I think I've got this more in my head now,
17:08:25 superdan so this is what I think I want to see:
17:08:53 superdan 1. I want to know if we're ever really calling the non-os_vif path (i.e. get_config_vhostuser). If we're not, we should just nuke that in a patch before this and then remove from this
17:09:19 superdan 2. I'd really like it if we could avoid setting driver in get_base_config and then re-setting it for the vhostuser cases
17:09:41 superdan for #2, we need an answer to the question I had before of whether or not things other than bridge use driver=vhost
17:10:28 superdan if we don't then I think we could set driver=vhost in set_config_VIFBridge (right?) and then set driver=vhostuser for _set_config_VIFHostUser() (right?)
17:10:48 superdan for #1, who can we ask that would know the answer to how we could get to the NotImplementedError in os_vif_util
17:10:50 superdan ?
17:11:02 sahid for #1 I think sean-k-mooney can help

Earlier   Later