[ovs-dev] OVN: add the possibility to specify tunnel dst port

Introduce dst_port in options column of Encap table in order to add the
capability to configure destination port used for tunnel encapsulation

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 ovn/controller/encaps.c | 4 ++++
 ovn/ovn-sb.xml          | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

On Tue, Jun 25, 2019 at 4:06 PM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> wrote:

> Introduce dst_port in options column of Encap table in order to add the
> capability to configure destination port used for tunnel encapsulation
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>

Acked-by: Numan Siddique <nusiddiq@redhat.com>



> ---
>  ovn/controller/encaps.c | 4 ++++
>  ovn/ovn-sb.xml          | 8 +++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index 3b3921a73..d4a436df3 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -156,6 +156,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
>      struct smap options = SMAP_INITIALIZER(&options);
>      smap_add(&options, "remote_ip", encap->ip);
>      smap_add(&options, "key", "flow");
> +    const char *dst_port = smap_get(&encap->options, "dst_port");
>      const char *csum = smap_get(&encap->options, "csum");
>      char *tunnel_entry_id = NULL;
>
> @@ -169,6 +170,9 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
>      if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
>          smap_add(&options, "csum", csum);
>      }
> +    if (dst_port) {
> +        smap_add(&options, "dst_port", dst_port);
> +    }
>
>      /* Add auth info if ipsec is enabled. */
>      if (sbg->ipsec) {
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 1a2bc1da9..2dae2768f 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -364,7 +364,8 @@
>      <column name="options">
>        <p>
>          Options for configuring the encapsulation. Currently, the only
> -        option that has been defined is <code>csum</code>.
> +        options that has been defined are <code>csum</code> and
> +        <code>dst_port</code>.
>        </p>
>
>        <p>
> @@ -408,6 +409,11 @@
>          <code>csum</code> defaults to <code>false</code> for hardware
> VTEPs and
>          <code>true</code> for all other cases.
>        </p>
> +
> +      <p>
> +        <code>dst_port</code> is used to define the destination port used
> +        in tunnel encapsulation
> +      </p>
>      </column>
>
>      <column name="ip">
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Tue, Jun 25, 2019 at 12:35:26PM +0200, Lorenzo Bianconi wrote:
> Introduce dst_port in options column of Encap table in order to add the
> capability to configure destination port used for tunnel encapsulation
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Thank you.  I think that the documentation can be improved by using the
documentation system's ability to document individual keys.  Here is my
suggestion; I only changed the documentation.

--8<--------------------------cut here-------------------------->8--

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Tue, 25 Jun 2019 12:35:26 +0200
Subject: [PATCH] OVN: add the possibility to specify tunnel dst port

Introduce dst_port in options column of Encap table in order to add the
capability to configure destination port used for tunnel encapsulation

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovn/controller/encaps.c |  4 ++++
 ovn/ovn-sb.xml          | 28 ++++++++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 3b3921a736e2..d4a436df318c 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -156,6 +156,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
     struct smap options = SMAP_INITIALIZER(&options);
     smap_add(&options, "remote_ip", encap->ip);
     smap_add(&options, "key", "flow");
+    const char *dst_port = smap_get(&encap->options, "dst_port");
     const char *csum = smap_get(&encap->options, "csum");
     char *tunnel_entry_id = NULL;
 
@@ -169,6 +170,9 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
     if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
         smap_add(&options, "csum", csum);
     }
+    if (dst_port) {
+        smap_add(&options, "dst_port", dst_port);
+    }
 
     /* Add auth info if ipsec is enabled. */
     if (sbg->ipsec) {
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 1a2bc1da9ccc..5f7783e0de8e 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -362,14 +362,14 @@
     </column>
 
     <column name="options">
-      <p>
-        Options for configuring the encapsulation. Currently, the only
-        option that has been defined is <code>csum</code>.
-      </p>
+      Options for configuring the encapsulation, which may be <ref column="type"/> specific.
+    </column>
 
+    <column name="options" key="csum" type='{"type": "boolean"}'>
       <p>
-        <code>csum</code> indicates that encapsulation checksums can be
-        transmitted and received with reasonable performance. It is a hint
+        <code>csum</code> indicates whether this chassis can transmit and
+        receive packets that include checksums with reasonable performance.  It
+        hints
         to senders transmitting data to this chassis that they should use
         checksums to protect OVN metadata. <code>ovn-controller</code>
         populates this key with the value defined in
@@ -380,7 +380,7 @@
       </p>
 
       <p>
-        In terms of performance, this actually significantly increases
+        In terms of performance, checksumming actually significantly increases
         throughput in most common cases when running on Linux based hosts
         without NICs supporting encapsulation hardware offload (around 60% for
         bulk traffic). The reason is that generally all NICs are capable of
@@ -399,7 +399,7 @@
         efficiently compute or validate full packet checksums. In addition
         certain versions of the Linux kernel are not able to fully take
         advantage of encapsulation NIC offloads in the presence of checksums.
-        (This is actually a pretty narrow corner case though - earlier
+        (This is actually a pretty narrow corner case though: earlier
         versions of Linux don't support encapsulation offloads at all and
         later versions support both offloads and checksums well.)
       </p>
@@ -408,6 +408,18 @@
         <code>csum</code> defaults to <code>false</code> for hardware VTEPs and
         <code>true</code> for all other cases.
       </p>
+
+      <p>
+        This option applies to <code>geneve</code> and <code>vxlan</code>
+        encapsulations.
+      </p>
+    </column>
+
+    <column name="options" key="dst_port" type='{"type": "integer"}'>
+      <p>
+        If set, overrides the UDP (for <code>geneve</code> and
+        <code>vxlan</code>) or TCP (for <code>stt</code>) destination port.
+      </p>
     </column>
 
     <column name="ip">

>
> On Tue, Jun 25, 2019 at 12:35:26PM +0200, Lorenzo Bianconi wrote:
> > Introduce dst_port in options column of Encap table in order to add the
> > capability to configure destination port used for tunnel encapsulation
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>
> Thank you.  I think that the documentation can be improved by using the
> documentation system's ability to document individual keys.  Here is my
> suggestion; I only changed the documentation.
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Date: Tue, 25 Jun 2019 12:35:26 +0200
> Subject: [PATCH] OVN: add the possibility to specify tunnel dst port
>
> Introduce dst_port in options column of Encap table in order to add the
> capability to configure destination port used for tunnel encapsulation
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  ovn/controller/encaps.c |  4 ++++
>  ovn/ovn-sb.xml          | 28 ++++++++++++++++++++--------
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index 3b3921a736e2..d4a436df318c 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -156,6 +156,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
>      struct smap options = SMAP_INITIALIZER(&options);
>      smap_add(&options, "remote_ip", encap->ip);
>      smap_add(&options, "key", "flow");
> +    const char *dst_port = smap_get(&encap->options, "dst_port");
>      const char *csum = smap_get(&encap->options, "csum");
>      char *tunnel_entry_id = NULL;
>
> @@ -169,6 +170,9 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
>      if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
>          smap_add(&options, "csum", csum);
>      }
> +    if (dst_port) {
> +        smap_add(&options, "dst_port", dst_port);
> +    }
>
>      /* Add auth info if ipsec is enabled. */
>      if (sbg->ipsec) {
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 1a2bc1da9ccc..5f7783e0de8e 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -362,14 +362,14 @@
>      </column>
>
>      <column name="options">
> -      <p>
> -        Options for configuring the encapsulation. Currently, the only
> -        option that has been defined is <code>csum</code>.
> -      </p>
> +      Options for configuring the encapsulation, which may be <ref column="type"/> specific.
> +    </column>
>
> +    <column name="options" key="csum" type='{"type": "boolean"}'>
>        <p>
> -        <code>csum</code> indicates that encapsulation checksums can be
> -        transmitted and received with reasonable performance. It is a hint
> +        <code>csum</code> indicates whether this chassis can transmit and
> +        receive packets that include checksums with reasonable performance.  It
> +        hints
>          to senders transmitting data to this chassis that they should use
>          checksums to protect OVN metadata. <code>ovn-controller</code>
>          populates this key with the value defined in
> @@ -380,7 +380,7 @@
>        </p>
>
>        <p>
> -        In terms of performance, this actually significantly increases
> +        In terms of performance, checksumming actually significantly increases
>          throughput in most common cases when running on Linux based hosts
>          without NICs supporting encapsulation hardware offload (around 60% for
>          bulk traffic). The reason is that generally all NICs are capable of
> @@ -399,7 +399,7 @@
>          efficiently compute or validate full packet checksums. In addition
>          certain versions of the Linux kernel are not able to fully take
>          advantage of encapsulation NIC offloads in the presence of checksums.
> -        (This is actually a pretty narrow corner case though - earlier
> +        (This is actually a pretty narrow corner case though: earlier
>          versions of Linux don't support encapsulation offloads at all and
>          later versions support both offloads and checksums well.)
>        </p>
> @@ -408,6 +408,18 @@
>          <code>csum</code> defaults to <code>false</code> for hardware VTEPs and
>          <code>true</code> for all other cases.
>        </p>
> +
> +      <p>
> +        This option applies to <code>geneve</code> and <code>vxlan</code>
> +        encapsulations.
> +      </p>
> +    </column>
> +
> +    <column name="options" key="dst_port" type='{"type": "integer"}'>
> +      <p>
> +        If set, overrides the UDP (for <code>geneve</code> and
> +        <code>vxlan</code>) or TCP (for <code>stt</code>) destination port.
> +      </p>
>      </column>
>
>      <column name="ip">
> --
> 2.20.1
>

Hi Ben,

I agree it is definitely better :) Do I need to resend?

Regards,
Lorenzo

On Fri, Jun 28, 2019 at 03:08:31PM +0200, Lorenzo Bianconi wrote:
> >
> > On Tue, Jun 25, 2019 at 12:35:26PM +0200, Lorenzo Bianconi wrote:
> > > Introduce dst_port in options column of Encap table in order to add the
> > > capability to configure destination port used for tunnel encapsulation
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> > Thank you.  I think that the documentation can be improved by using the
> > documentation system's ability to document individual keys.  Here is my
> > suggestion; I only changed the documentation.
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > Date: Tue, 25 Jun 2019 12:35:26 +0200
> > Subject: [PATCH] OVN: add the possibility to specify tunnel dst port
> >
> > Introduce dst_port in options column of Encap table in order to add the
> > capability to configure destination port used for tunnel encapsulation
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  ovn/controller/encaps.c |  4 ++++
> >  ovn/ovn-sb.xml          | 28 ++++++++++++++++++++--------
> >  2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > index 3b3921a736e2..d4a436df318c 100644
> > --- a/ovn/controller/encaps.c
> > +++ b/ovn/controller/encaps.c
> > @@ -156,6 +156,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> >      struct smap options = SMAP_INITIALIZER(&options);
> >      smap_add(&options, "remote_ip", encap->ip);
> >      smap_add(&options, "key", "flow");
> > +    const char *dst_port = smap_get(&encap->options, "dst_port");
> >      const char *csum = smap_get(&encap->options, "csum");
> >      char *tunnel_entry_id = NULL;
> >
> > @@ -169,6 +170,9 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg,
> >      if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) {
> >          smap_add(&options, "csum", csum);
> >      }
> > +    if (dst_port) {
> > +        smap_add(&options, "dst_port", dst_port);
> > +    }
> >
> >      /* Add auth info if ipsec is enabled. */
> >      if (sbg->ipsec) {
> > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> > index 1a2bc1da9ccc..5f7783e0de8e 100644
> > --- a/ovn/ovn-sb.xml
> > +++ b/ovn/ovn-sb.xml
> > @@ -362,14 +362,14 @@
> >      </column>
> >
> >      <column name="options">
> > -      <p>
> > -        Options for configuring the encapsulation. Currently, the only
> > -        option that has been defined is <code>csum</code>.
> > -      </p>
> > +      Options for configuring the encapsulation, which may be <ref column="type"/> specific.
> > +    </column>
> >
> > +    <column name="options" key="csum" type='{"type": "boolean"}'>
> >        <p>
> > -        <code>csum</code> indicates that encapsulation checksums can be
> > -        transmitted and received with reasonable performance. It is a hint
> > +        <code>csum</code> indicates whether this chassis can transmit and
> > +        receive packets that include checksums with reasonable performance.  It
> > +        hints
> >          to senders transmitting data to this chassis that they should use
> >          checksums to protect OVN metadata. <code>ovn-controller</code>
> >          populates this key with the value defined in
> > @@ -380,7 +380,7 @@
> >        </p>
> >
> >        <p>
> > -        In terms of performance, this actually significantly increases
> > +        In terms of performance, checksumming actually significantly increases
> >          throughput in most common cases when running on Linux based hosts
> >          without NICs supporting encapsulation hardware offload (around 60% for
> >          bulk traffic). The reason is that generally all NICs are capable of
> > @@ -399,7 +399,7 @@
> >          efficiently compute or validate full packet checksums. In addition
> >          certain versions of the Linux kernel are not able to fully take
> >          advantage of encapsulation NIC offloads in the presence of checksums.
> > -        (This is actually a pretty narrow corner case though - earlier
> > +        (This is actually a pretty narrow corner case though: earlier
> >          versions of Linux don't support encapsulation offloads at all and
> >          later versions support both offloads and checksums well.)
> >        </p>
> > @@ -408,6 +408,18 @@
> >          <code>csum</code> defaults to <code>false</code> for hardware VTEPs and
> >          <code>true</code> for all other cases.
> >        </p>
> > +
> > +      <p>
> > +        This option applies to <code>geneve</code> and <code>vxlan</code>
> > +        encapsulations.
> > +      </p>
> > +    </column>
> > +
> > +    <column name="options" key="dst_port" type='{"type": "integer"}'>
> > +      <p>
> > +        If set, overrides the UDP (for <code>geneve</code> and
> > +        <code>vxlan</code>) or TCP (for <code>stt</code>) destination port.
> > +      </p>
> >      </column>
> >
> >      <column name="ip">
> > --
> > 2.20.1
> >
> 
> Hi Ben,
> 
> I agree it is definitely better :) Do I need to resend?

No.  Since you approve, I added Numan's ack and applied this to master.
Thanks again!