Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-13
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,
17:19:59 superdan I want to know that answer to #1
17:20:38 sahid yea, i think that will be diffuclt to response for example i'm sure before of the update for vrouter they were using that
17:20:52 sahid so i imagine it might have other kind of switch which are using it
17:20:58 sahid but let see with sean
17:21:45 superdan okay, one other thing:
17:21:52 superdan the L171 override,
17:22:28 superdan er, nevermind
17:22:45 superdan I forgot about setting the rx_queue_size for the frontend
17:23:08 sahid ok cool
17:23:22 sahid thanks for you time superdan. time for me to take a break
17:23:37 superdan sahid: yeah, thanks for your patience, I will summarize in the review (again :P)
17:24:04 sahid :)
17:38:41 superdan jgwentworth: you wanna take a trip through this? https://review.openstack.org/#/c/479949/57
17:39:05 superdan now that it's split into a smaller piece I think it's close to reasonable but I'm getting a little blind to it at this point
17:40:46 jgwentworth superdan: yes, will do that today
17:41:00 superdan thanks
17:41:08 superdan I bet jackie-truong thanks you too
17:41:19 jgwentworth :)
17:42:31 jackie-truong Yes, please :-) Thanks, jgwentworth and superdan
17:42:44 superdan jackie-truong: I was looking for you on here earlier today to ask forgiveness for asking to split the patch so late
17:42:45 superdan you not being here led to guilt and me splitting it for you
17:42:46 superdan so, good job.
17:43:06 kashyap superdan: Was AFK; just re-launched IRC client, /me scrolls back
17:43:48 jackie-truong superdan: Sorry I wasn't around. Friday meetings suck. Thanks for splitting the patch - I think it makes a lot more sense that way
17:44:54 superdan kashyap: not a thing anymore

Earlier   Later