Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-13
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 # argument for vhostuser "
17:16:57 superdan "since QEMU ignores the driver
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
17:45:03 kashyap superdan: Ah, noted.
17:45:09 superdan kashyap: I spent an epic hour or so with sahid getting it all figured out myself
17:45:45 kashyap lyarwood: or any "Stable People": Want to put this out of its misery: https://review.openstack.org/#/c/561168/
17:47:35 kashyap superdan: FWIW, so many TODOs in already one patch: https://review.openstack.org/#/c/484997/16/nova/virt/libvirt/vif.py?
17:47:45 kashyap A bit moderation would be nice
17:48:16 superdan kashyap: yeah, he's going to move some of the stuff out to another patch, which will pull some TODOs with it
17:48:27 superdan kashyap: they're not TODOs about the patch itself, but some stupid stuff that was in the file before this
17:48:42 superdan like "I noticed this was already wrong when I was in here"
17:48:44 kashyap Ah, I see.
17:48:49 superdan which should be in another patch,
17:48:57 superdan especially because it took me days to suss that out :)
17:48:58 kashyap Right. "One logical change per patch"
17:49:40 jgwentworth jackie-truong: is this the tempest test where we can see the cert validation working end-to-end? https://review.openstack.org/#/c/526485
17:50:04 superdan jgwentworth: it's linked in the bottom patch
17:50:08 superdan I don't think that's it
17:50:26 superdan jgwentworth: https://review.openstack.org/#/c/515210/
17:50:31 jgwentworth linked in the comments? cause I don't see anything in the commit message
17:50:36 superdan yeah comments
17:50:47 jgwentworth k, thanks
17:50:57 kashyap superdan: Also, it's never a "mess" (as you put it there) to do connsciousness stream write-ups on reviews are perfectly "legal", as you know :P
17:51:16 superdan I just rechecked it a bit ago after pushing up the split, but it wasn't a change, so the last run of that should be fine for validation
17:51:19 jgwentworth oh, k. I did see that patch but missed that it's tempest via pluginness
17:51:23 superdan kashyap: it's a total mess
17:51:53 kashyap If you say so...
17:52:50 cfriesen do we do anything to prevent a port deletion of a port that is connected to an instance? or should I be asking the neutron folks?
17:53:26 kashyap Unrelated: Are Gerrit review comments look-up-able? As in: I know that dansmith said $unique-phrase on a change, putting that phrase in double quotes in Google will yield the right URL?
17:53:54 superdan what did I say?

Earlier   Later