Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-13
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
17:11:27 superdan okay that would have been my first thought
17:11:36 sahid for #2 we are using vhost for more that bridge
17:11:48 sahid i was confusing
17:12:13 sahid but basicallt for every net device type virtio
17:12:23 sahid which is always the case
17:12:33 sahid we use vhost user if the host support it
17:12:59 sahid basically to plug a tap device in qemu we always use a device virtio-net
17:13:27 superdan okay and that gets set in get_base_config() today because of _get_virtio_mq_settings() ?
17:14:02 sahid yes, we specically set it because we want the driver node to pass the attirbute queues
17:14:32 superdan okay, so today before your patch, how do we get driver=vhostuser in the vhostuser case?
17:15:20 sahid that was the point of the TODOs i added
17:15:21 superdan meaning, why didn't we need L549 in your patch before?
17:16:00 sahid before of my patch we where setting vhost even for a vhostuser interface
17:16:29 superdan ah, okay and you were saying that was just getting ignored?
17:16:39 sahid exactly
17:16:57 superdan "since QEMU ignores the driver
17:16:57 superdan # argument for vhostuser "
17:16:58 superdan okay
17:17:07 superdan there are waaaay too many things in here with almost the same name :)
17:17:08 sahid yes that is what i meant
17:17:21 sahid it's really confusing yes
17:17:23 superdan okay, so,
17:17:43 superdan this seems silly, but can we remove L549 from this patch and put it either before or after?
17:17:53 superdan I think it's super confusing because I thought you were changing something,
17:18:03 superdan but really you're just correcting something that is ignored
17:18:09 sahid yes :)
17:18:12 sahid but...
17:19:07 sahid no, ok I can provide a patch to set driver_name = vhostuser for set_config_VIFHostUser
17:19:31 sahid and then provide the queue sizes change
17:19:48 sahid looks good for you?
17:19:49 superdan yeah, do that
17:19:52 sahid ok
17:19:54 superdan but also,

Earlier   Later