| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-03-13 | |||
| 12:39:29 | justas_napa | Hi. To answer the question regarding ovs support - it really depends on the end user requirements | |
| 12:39:38 | sean-k-mooney | in nova check for both and in a new if in os_vif_util implement the logic you require | |
| 12:40:13 | justas_napa | we maintain our own OvS to support features like QinQ and some special QoS schemes | |
| 12:40:18 | sean-k-mooney | justas_napa: the in tree ml2/ovs and ml2/ovn dirver are only ment to supprot ovs form openvsiwtch.org | |
| 12:41:04 | sean-k-mooney | so if the basic virtio-forward fucntionality is not upstream to vanilla ovs it should be in a seperatem ml2 driver and os_vif driver | |
| 12:41:12 | justas_napa | I think we are OK to limit ourselves to vanilla ovs | |
| 12:41:53 | justas_napa | unless adding support for our own OVS is trivial | |
| 12:42:52 | sean-k-mooney | well vaniall ovs has two ways to enabel vhost-user but the proposal is not using either of them | |
| 12:42:59 | sean-k-mooney | https://docs.openvswitch.org/en/latest/topics/dpdk/vhost-user/ | |
| 12:43:17 | sean-k-mooney | instead you are created dpdk vdevs https://docs.openvswitch.org/en/latest/topics/dpdk/vdev/ | |
| 12:44:58 | sean-k-mooney | there is supprot for dpdk representor prots | |
| 12:45:27 | justas_napa | so then it's custom ml2 and os_vif driver | |
| 12:45:57 | sean-k-mooney | https://docs.openvswitch.org/en/latest/topics/dpdk/phy/#representors | |
| 12:46:02 | sean-k-mooney | am i think so. | |
| 12:46:26 | justas_napa | yes, we are heavy users of representors | |
| 12:46:26 | sean-k-mooney | if you go with the out of tree ml2/os-vif drivers then all that is requried in nova is a minor change to call them | |
| 12:46:46 | sean-k-mooney | so ok maybe we need to level set | |
| 12:47:07 | sean-k-mooney | justas_napa: the dpdk type port that you are creating for use with virio-forward | |
| 12:47:14 | sean-k-mooney | is that a vf representor | |
| 12:47:21 | sean-k-mooney | as descirbed here https://docs.openvswitch.org/en/latest/topics/dpdk/phy/#representors | |
| 12:47:32 | justas_napa | yes | |
| 12:47:50 | justas_napa | becuase all dataplane is offloaded to FPGA | |
| 12:48:01 | sean-k-mooney | ok so if that is what we are enableing and the supprot in upstream ovs also works with your nics we can enabel that in the intree support | |
| 12:48:54 | sean-k-mooney | so next question you require the vhost-user socket path to have a specific format to corralate teh vhost-user path to the dpdk representor correct | |
| 12:49:42 | justas_napa | I don't think we require specific path, just the socket name | |
| 12:49:53 | sean-k-mooney | the orginal requriements when vhsot-user was first added to ovs was the socket path's final segment and port name must be the same | |
| 12:50:20 | sean-k-mooney | right so for normal vhost-user when it was first added the socket-name and port name had to be the same | |
| 12:50:47 | sean-k-mooney | later we added a socket path to ovs to remove that limiation as part of supproting vhost-user-client mode where qemu is the server | |
| 12:51:22 | justas_napa | -chardev socket,id=char0,path=/usr/local/var/run/stdvio5,server | |
| 12:51:50 | justas_napa | so we are flexible on thje path, but the last part is stdvio<#VF> | |
| 12:52:00 | sean-k-mooney | ya so your dirver breaks the convention that the final segment of the name matches the name of the ovs port which is fine | |
| 12:52:07 | sean-k-mooney | ack | |
| 12:52:32 | sean-k-mooney | https://review.opendev.org/c/openstack/neutron/+/869510/2/neutron/plugins/ml2/drivers/openvswitch/mech_driver/mech_openvswitch.py#217 | |
| 12:52:37 | sean-k-mooney | so that is why you need this code | |
| 12:53:06 | justas_napa | yep | |
| 12:53:28 | sean-k-mooney | so the problem i see is the pci_slot is always going to be set for vnic_type virtio-forwarder | |
| 12:53:54 | sean-k-mooney | but we only want to take the else branch if its napatech | |
| 12:54:14 | sean-k-mooney | so ninstead of the current if we shoudl add a config value to the agent and check if that is set | |
| 12:54:25 | sean-k-mooney | so just like | |
| 12:54:27 | sean-k-mooney | sockdir = agent['configurations'].get('vhostuser_socket_dir', | |
| 12:54:29 | sean-k-mooney | ovs_const.VHOST_USER_SOCKET_DIR) | |
| 12:55:20 | sean-k-mooney | you shoudl add something like sockdir = agent['configurations'].get('vhostuser_socket_name_scheme','port_name') | |
| 12:55:50 | sean-k-mooney | and for napatech you woudl set vhostuser_socket_name_scheme to VF_index | |
| 12:55:54 | sean-k-mooney | or something liek that | |
| 12:55:56 | justas_napa | OK | |
| 12:56:22 | justas_napa | dvo-plv - does this work for us? | |
| 12:57:23 | sean-k-mooney | i need to think about the nova patch a bit but i think if you do that the nova patch is fine as is | |
| 12:57:54 | justas_napa | ack | |
| 12:57:55 | sean-k-mooney | well it needs test but the core functionality is proably correct | |
| 12:58:32 | dvo-plv | in our solution, socket name value is depends on the pci slot | |
| 12:58:55 | dvo-plv | This value vhostuser_socket_name_scheme will be located at the neutron conf file | |
| 12:59:11 | sean-k-mooney | yes used by the neutron_l2_agent | |
| 12:59:24 | sean-k-mooney | it will need to get added to the agent report | |
| 12:59:36 | sean-k-mooney | which will make it avaiable to the ml2 driver during binding | |
| 13:00:23 | sean-k-mooney | like this https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py#L379-L380 | |
| 13:00:45 | sean-k-mooney | its how we pass the per host vhostuser_socket_dir and datapath_type info today | |
| 13:01:59 | sean-k-mooney | dvo-plv: looking at teh os-vif code i think that is also ok just needs tests | |
| 13:02:28 | sean-k-mooney | justas_napa: dvo-plv my concern basically is if we enable this and we use vaniall ovs-dpdk will it still work | |
| 13:03:06 | dvo-plv | So, instead of extending ovs with virtio-forwarder, you suggest to add new parameter ( socket scheme ) and we will create port with vhost user vif type, and based on a new parameter, it will affect socket path | |
| 13:03:08 | justas_napa | as long as user does not try to setup qinq and advance qos, it will work | |
| 13:03:36 | sean-k-mooney | dvo-plv: not quite | |
| 13:03:52 | sean-k-mooney | dvo-plv: keep your ml2/ovs driver patch as it is using virtio-forwarder | |
| 13:03:59 | sean-k-mooney | i think that is the correct vnic_type | |
| 13:04:24 | sean-k-mooney | im suggesting not basing the format on if the pci_slot it set | |
| 13:04:44 | sean-k-mooney | we shoudl either have a config option for it or base it on vnic_type virtio-forwarder | |
| 13:04:49 | sean-k-mooney | im just checkign something | |
| 13:05:01 | sean-k-mooney | we might not need to modify your patches at all | |
| 13:06:45 | dvo-plv | I see, make this function agent_vhu_sockpath able to change the structure of socket's name if it is specified in the config file | |
| 13:06:59 | sean-k-mooney | yep | |
| 13:07:19 | sean-k-mooney | so im trying to make sure we dont break agilio_ovs in the process of enabling napatech | |
| 13:07:33 | sean-k-mooney | im just checkign how we determin its agilio_ovs today in nova | |
| 13:08:20 | sean-k-mooney | ok your current patch shoudl be ok as is | |
| 13:08:28 | sean-k-mooney | they use a seperate vif_type | |
| 13:09:07 | sean-k-mooney | justas_napa: dvo-plv: so based on the refactoring that you have already done following my intial feedback | |
| 13:09:19 | sean-k-mooney | i think the current solution shoudl be generic enough to work as is | |
| 13:09:40 | sean-k-mooney | we dont have any exsiting use of vif_type=ovs and virtio-forwarder | |
| 13:10:06 | sean-k-mooney | we can add a comment that if we need a diffent nameing scheme in the futrue then a config option can be added at that point | |
| 13:11:24 | justas_napa | I'm not sure I follow | |
| 13:11:37 | justas_napa | do you think we need further updates | |
| 13:11:53 | justas_napa | or are we going as-is? | |
| 13:11:54 | sean-k-mooney | justas_napa: let me rephase. your code is fine as is it just need tests and docs | |
| 13:11:58 | justas_napa | OK | |
| 13:12:50 | sean-k-mooney | ill try and review the spec this week. if i have not done so by thrusday ping me | |
| 13:13:43 | dvo-plv | We need to update spec file accodring this concept, I will ping you when it will be ready | |
| 13:13:53 | sean-k-mooney | my general feedback is we need test to go along with the functional changes you have made and based on a quick review of the surrounding code and the questions you answered here i think the current approch is correct | |
| 13:15:10 | sean-k-mooney | so the spec is pretty light on detail in general | |
| 13:15:25 | sean-k-mooney | it would be good to update it with links to https://docs.openvswitch.org/en/latest/topics/dpdk/phy/#representors | |
| 13:15:34 | sean-k-mooney | since that is really what you are tryign to enable | |
| 13:16:07 | justas_napa | sure. will do | |
| 13:16:12 | sean-k-mooney | i was hevially invovled in enabling dpdk supprot in openstack in general so most of the other core reviews dont have the same levle of context that i have | |
| 13:21:46 | plibeau | bauzas: thx for the review, I have reply on your comment. https://review.opendev.org/c/openstack/nova/+/861172 | |
| 13:22:07 | sean-k-mooney | justas_napa: dvo-plv: one general comment on the spec (this is hard to get right by the way) the spec is intended to capture all the info require so that if you could not complete the feature someone else with a familararity with nova could compelte it. given the code is already written and looks mostly correct what you shoudl really focus on is providing enouch context for | |
| 13:22:09 | sean-k-mooney | nova reviews who dont really have famiariaty with ovs/dpdk/vf representors. basically add som short context paragraphs explaing what the technology is and how you are reusing/extendign the existing functionaly in nova/os-vif so that its simpler for other cores to review. | |
| 13:24:47 | dvo-plv | yes, sure, we will process this comment | |
| 13:24:49 | dvo-plv | I have some concenrs about this patch https://review.opendev.org/c/openstack/os-vif/+/859574/4/vif_plug_ovs/ovs.py How properly we should pass scheme to the os-vif, it hould be part of the vif in _plug_vf method? | |
| 13:29:03 | jrosser | should HW_ARCH_* trait be automatically set on compute hosts? | |
| 13:31:42 | sean-k-mooney | dvo-plv: we should be able to just pass the vhost-user socket path | |
| 13:32:28 | sean-k-mooney | jrosser: ideally yes but i don tknow if the libvirt driver does that today | |
| 13:33:11 | jrosser | my test suggests that for Z they're not there | |
| 13:34:50 | sean-k-mooney | then that was likely not implemtned | |