[ovs-dev] [PATCH] dpdk: announce deprecation of vhost-user server ports

[Aaron Conole]

Hi Darrell,

Thanks so much for the review!  Comments below.

Darrell Ball <dball at vmware.com> writes:

> On 6/7/17, 3:46 PM, "Aaron Conole" <aconole at redhat.com> wrote:
>
>     Since vhost-user server mode ports are the preferred mechanism for
>     interconnecting Open vSwitch with VMs when using DPDK, and since there
>     are currently no known use cases for vhost-user server mode ports apart
>     from version incompatibilities with QEMU, announce that server mode ports
>     are considered deprecated and will be removed in a future release.
>     
>     Cc: Ciara Loftus <ciara.loftus at intel.com>
>     Cc: Kevin Traynor <ktraynor at redhat.com>
>     Suggested-by: Darrell Ball <dball at vmware.com>
>     Signed-off-by: Aaron Conole <aconole at redhat.com>
>     ---
>      Documentation/topics/dpdk/vhost-user.rst | 24 ++++++++++++++++--------
>      NEWS                                     |  2 ++
>      lib/netdev-dpdk.c                        |  2 ++
>      3 files changed, 20 insertions(+), 8 deletions(-)
>     
>     diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst
>     index a1c19fd..9d36cf2 100644
>     --- a/Documentation/topics/dpdk/vhost-user.rst
>     +++ b/Documentation/topics/dpdk/vhost-user.rst
>     @@ -32,13 +32,19 @@ documentation`_ on same.
>      Quick Example
>      -------------
>      
>     -This example demonstrates how to add two ``dpdkvhostuser`` ports to an existing
>     -bridge called ``br0``::
>     +This example demonstrates how to add two ``dpdkvhostuserclient`` ports to an
>     +existing bridge called ``br0``::
>      
>     -    $ ovs-vsctl add-port br0 dpdkvhostuser0 \
>     -        -- set Interface dpdkvhostuser0 type=dpdkvhostuser
>     -    $ ovs-vsctl add-port br0 dpdkvhostuser1 \
>     -        -- set Interface dpdkvhostuser1 type=dpdkvhostuser
>     +    $ ovs-vsctl add-port br0 dpdkvhostclient0 \
>     +        -- set Interface dpdkvhostclient0 type=dpdkvhostuserclient \
>     +           options:vhost-server-path=/tmp/dpdkvhostclient0
>     +    $ ovs-vsctl add-port br0 dpdkvhostclient1 \
>     +        -- set Interface dpdkvhostclient1 type=dpdkvhostuserclient \
>     +           options:vhost-server-path=/tmp/dpdkvhostclient1
>     +
>     +For the above examples to work, an appropriate server socket must be created
>     +at the paths specified (``/tmp/dpdkvhostclient0`` and
>     +``/tmp/dpdkvhostclient0``).
>      
>      vhost-user vs. vhost-user-client
>      --------------------------------
>     @@ -59,7 +65,8 @@ means if OVS dies, all VMs **must** be restarted. On the other hand, for
>      vhost-user-client ports, OVS acts as the client and QEMU the server. This means
>      OVS can die and be restarted without issue, and it is also possible to restart
>      an instance itself. For this reason, vhost-user-client ports are the preferred
>     -type for most use cases.
>     +type for most use cases.  Ports of type vhost-user are currently deprecated and
>     +will be removed in a future release.
>
> type for all known use cases; the only limitation is that vhost-user client mode ports
> require QEMU version 2.7.  Ports of type vhost-user are currently deprecated and
> will be removed in a future release.

Will update with this verbiage.  Thanks.

>      .. _dpdk-vhost-user:
>      
>     @@ -68,7 +75,8 @@ vhost-user
>      
>      .. important::
>      
>     -   Use of vhost-user ports requires QEMU >= 2.2
>     +   Use of vhost-user ports requires QEMU >= 2.2;  vhost-user ports are
>     +   *deprecated*.
>      
>      To use vhost-user ports, you must first add said ports to the switch. DPDK
>      vhost-user ports can have arbitrary names with the exception of forward and
>     diff --git a/NEWS b/NEWS
>     index 82004c8..b81d033 100644
>     --- a/NEWS
>     +++ b/NEWS
>     @@ -16,6 +16,8 @@ Post-v2.7.0
>             Log level can be changed in a usual OVS way using
>             'ovs-appctl vlog' commands for 'dpdk' module. Lower bound
>             still can be configured via extra arguments for DPDK EAL.
>     +     * dpdkvhostuser ports are marked as deprecated.  They will be removed
>     +       in an upcoming release.
>         - IPFIX now provides additional counters:
>           * Total counters since metering process startup.
>           * Per-flow TCP flag counters.
>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     index b770b70..9ab4aeb 100644
>     --- a/lib/netdev-dpdk.c
>     +++ b/lib/netdev-dpdk.c
>     @@ -966,6 +966,8 @@ netdev_dpdk_vhost_construct(struct netdev *netdev)
>          err = vhost_common_construct(netdev);
>      
>          ovs_mutex_unlock(&dpdk_mutex);
>     +    VLOG_WARN_ONCE("dpdkvhostuser ports are considered deprecated;  "
>     +                   "please migrate to dpdkvhostuserclient ports.");
>
> I think we can:
> 1) Print the socket name and port name
> 2) I am not sure ‘_ONCE’ is required; do you really think the log will have that many instances.

My idea to not print the socket / port name is because I figure there
would be cases that users have many VMs, do an upgrade, and see the log
spewed over and over.  Maybe that's a good thing though - not sure.

If you consider a deployment with 100 VMs, that means this will pop up
100 times.  Even more, in deployments where they are using orchestration
software, or a cluster management solution, I figure those systems may
still be using the old style dpdkvhostuser ports, and again didn't want
to print it for every start of a VM - especially when there isn't
anything the user could do about it.

If you think there's a strong value in warning on every start, and
including the details of the port, I'll do that.  I'm not married to
this particular code ;)

-Aaron