[ovs-dev] [PATCH v5 2/3] dpctl, ovs-ofctl: Unify parsing of ct-flush arguments.

[Ales Musil]

In order to make the command extensible unify the arguments
parsing into single function. This will be later on used
for the mark and labels arguments.

Signed-off-by: Ales Musil 
---
v5: Rebase on top of current master.
v4: Rebase on top of current master.
v3: Rebase on top of current master.
Fix the system-tests failure.
---
 include/openvswitch/ofp-ct.h |  5 ++--
 lib/dpctl.c  | 41 ---
 lib/ofp-ct.c | 47 +++-
 tests/system-traffic.at  |  2 +-
 utilities/ovs-ofctl.c| 37 ++--
 5 files changed, 62 insertions(+), 70 deletions(-)

diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
index c8023c309..cd6192e6f 100644
--- a/include/openvswitch/ofp-ct.h
+++ b/include/openvswitch/ofp-ct.h
@@ -58,8 +58,9 @@ bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, 
uint8_t ip_proto);
 bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t ip_proto);
 
 void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
-bool ofp_ct_tuple_parse(struct ofp_ct_tuple *, const char *,
-struct ds *, uint8_t *ip_proto, uint16_t *l3_type);
+bool ofp_ct_match_parse(const char **, int argc, struct ds *,
+struct ofp_ct_match *, bool *with_zone,
+uint16_t *zone_id);
 
 enum ofperr ofp_ct_match_decode(struct ofp_ct_match *, bool *with_zone,
 uint16_t *zone_id, const struct ofp_header *);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 2a1aac5e5..7cc9d2805 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1773,48 +1773,17 @@ dpctl_flush_conntrack(int argc, const char *argv[],
 struct dpif *dpif = NULL;
 struct ofp_ct_match match = {0};
 struct ds ds = DS_EMPTY_INITIALIZER;
-uint16_t zone, *pzone = NULL;
+uint16_t zone;
 int error;
 int args = argc - 1;
-int zone_pos = 1;
+bool with_zone = false;
 
 if (dp_arg_exists(argc, argv)) {
 args--;
-zone_pos = 2;
-}
-
-/* Parse zone. */
-if (args && !strncmp(argv[zone_pos], "zone=", 5)) {
-if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, )) {
-ds_put_cstr(, "failed to parse zone");
-error = EINVAL;
-goto error;
-}
-pzone = 
-args--;
-}
-
-/* Parse ct tuples. */
-for (int i = 0; i < 2; i++) {
-if (!args) {
-break;
-}
-
-struct ofp_ct_tuple *tuple =
-i ? _reply : _orig;
-const char *arg = argv[argc - args];
-
-if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, , _proto,
-  _type)) {
-error = EINVAL;
-goto error;
-}
-args--;
 }
 
-/* Report error if there is more than one unparsed argument. */
-if (args > 0) {
-ds_put_cstr(, "invalid arguments");
+if (args && !ofp_ct_match_parse([argc - args], args, , ,
+_zone, )) {
 error = EINVAL;
 goto error;
 }
@@ -1825,7 +1794,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
 return error;
 }
 
-error = ct_dpif_flush(dpif, pzone, );
+error = ct_dpif_flush(dpif, with_zone ?  : NULL, );
 if (!error) {
 dpif_close(dpif);
 return 0;
diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c
index a140fba47..b6500f905 100644
--- a/lib/ofp-ct.c
+++ b/lib/ofp-ct.c
@@ -101,7 +101,7 @@ ofp_ct_match_format(struct ds *ds, const struct 
ofp_ct_match *match)
 /* Parses a specification of a conntrack 5-tuple from 's' into 'tuple'.
  * Returns true on success.  Otherwise, returns false and puts the error
  * message in 'ds'. */
-bool
+static bool
 ofp_ct_tuple_parse(struct ofp_ct_tuple *tuple, const char *s,
struct ds *ds, uint8_t *ip_proto, uint16_t *l3_type)
 {
@@ -219,6 +219,51 @@ error:
 return false;
 }
 
+/* Parses a specification of a conntrack match from 'argv' into 'match'.
+ * Returns true on success. Otherwise, returns false and puts the error
+ * message in 'ds'. */
+bool
+ofp_ct_match_parse(const char **argv, int argc, struct ds *ds,
+   struct ofp_ct_match *match, bool *with_zone,
+   uint16_t *zone_id)
+{
+int args = argc;
+
+/* Parse zone. */
+if (args && !strncmp(argv[argc - args], "zone=", 5)) {
+if (!ovs_scan(argv[argc - args], "zone=%"SCNu16, zone_id)) {
+ds_put_cstr(ds, "failed to parse zone");
+return false;
+}
+*with_zone = true;
+args--;
+}
+
+/* Parse ct tuples. */
+for (int i = 0; i < 2; i++) {
+if (!args) {
+break;
+}
+
+struct ofp_ct_tuple *tuple =
+i ? >tuple_reply : >tuple_orig;
+const char *arg = argv[argc - args];
+
+if (arg[0] && !ofp_ct_tuple_parse(tuple, arg, ds, >ip_proto,
+ 

[ovs-dev] [PATCH v5 3/3] openflow: Allow CT flush to match on mark and labels.

[Ales Musil]

Extend the current NX_CT_FLUSH with four additional fields,
that allow to match on CT entry "mark" or "labels". This
is encoded as separate TLV values which is backward compatible.
Versions that do not support them will simply ignore it.

Extend also the ovs-dpctl and ovs-ofctl command line tools with
option to specify those two matching parameters for the "ct-flush"
command.

Reported-at: https://issues.redhat.com/browse/FDP-55
Signed-off-by: Ales Musil 
---
v5: Rebase on top of current master.
Remove the rebase artifacts from NEWS.
Remove the unrelated whitespace changes.
v4: Rebase on top of current master.
Address comments from Ilya:
- Add NEWs entry.
- Adjust the flags to use unsigned int.
- Make the encode function more user-friendly.
- Adjust the tests.
v3: Rebase on top of current master.
v2: Make sure that the mask decoding matches the dpctl/ovs-ofctl interface.
---
 NEWS  |   3 +
 include/openflow/nicira-ext.h |   4 +
 include/openvswitch/ofp-ct.h  |   9 ++-
 lib/ct-dpif.c |  12 ++-
 lib/dpctl.c   |   5 +-
 lib/ofp-ct.c  | 135 +-
 tests/ofp-print.at|  84 +
 tests/ovs-ofctl.at|  36 +
 tests/system-traffic.at   | 112 ++--
 utilities/ovs-ofctl.8.in  |  13 ++--
 utilities/ovs-ofctl.c |  12 +--
 11 files changed, 367 insertions(+), 58 deletions(-)

diff --git a/NEWS b/NEWS
index 63f2842ae..c26a68553 100644
--- a/NEWS
+++ b/NEWS
@@ -26,6 +26,9 @@ Post-v3.2.0
  * Added support for Generic Segmentation Offloading for the cases where
TSO is enabled but not supported by an egress interface (except for
tunnel interfaces).
+   - OpenFlow:
+ * Extended the NXT_CT_FLUSH to support matching on CT label and mark
+   fields.
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 768775898..959845ce6 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1075,6 +1075,10 @@ enum nx_ct_flush_tlv_type {
 * by 'enum nx_ct_flush_tuple_tlv_type'*/
 /* Primitive types. */
 NXT_CT_ZONE_ID = 2,/* be16 zone id. */
+NXT_CT_MARK = 3,   /* be32 mark. */
+NXT_CT_MARK_MASK = 4,  /* be32 mark mask. */
+NXT_CT_LABELS = 5, /* be128 labels. */
+NXT_CT_LABELS_MASK = 6,/* be128 labels mask. */
 };
 
 /* CT flush nested TLVs. */
diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
index cd6192e6f..d57b62678 100644
--- a/include/openvswitch/ofp-ct.h
+++ b/include/openvswitch/ofp-ct.h
@@ -51,11 +51,16 @@ struct ofp_ct_match {
 
 struct ofp_ct_tuple tuple_orig;
 struct ofp_ct_tuple tuple_reply;
+
+uint32_t mark;
+uint32_t mark_mask;
+
+ovs_u128 labels;
+ovs_u128 labels_mask;
 };
 
 bool ofp_ct_match_is_zero(const struct ofp_ct_match *);
-bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, uint8_t ip_proto);
-bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t ip_proto);
+bool ofp_ct_match_is_five_tuple(const struct ofp_ct_match *);
 
 void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
 bool ofp_ct_match_parse(const char **, int argc, struct ds *,
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 5115c886b..5a836b668 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -274,6 +274,15 @@ ct_dpif_entry_cmp(const struct ct_dpif_entry *entry,
 return false;
 }
 
+if ((match->mark & match->mark_mask) != (entry->mark & match->mark_mask)) {
+return false;
+}
+
+if (!ovs_u128_equals(ovs_u128_and(match->labels, match->labels_mask),
+ ovs_u128_and(entry->labels, match->labels_mask))) {
+return false;
+}
+
 return true;
 }
 
@@ -300,8 +309,7 @@ ct_dpif_flush_tuple(struct dpif *dpif, const uint16_t *zone,
 
 /* If we have full five tuple in original and empty reply tuple just
  * do the flush over original tuple directly. */
-if (ofp_ct_tuple_is_five_tuple(>tuple_orig, match->ip_proto) &&
-ofp_ct_tuple_is_zero(>tuple_reply, match->ip_proto)) {
+if (ofp_ct_match_is_five_tuple(match)) {
 struct ct_dpif_tuple tuple;
 
 ct_dpif_tuple_from_ofp_ct_tuple(>tuple_orig, ,
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 7cc9d2805..34ee7d0e2 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -3005,8 +3005,9 @@ static const struct dpctl_command all_commands[] = {
   0, 4, dpctl_dump_conntrack, DP_RO },
 { "dump-conntrack-exp", "[dp] [zone=N]",
   0, 2, dpctl_dump_conntrack_exp, DP_RO },
-{ "flush-conntrack", "[dp] [zone=N] [ct-orig-tuple] [ct-reply-tuple]",
-  0, 4, dpctl_flush_conntrack, DP_RW },
+{ "flush-conntrack", "[dp] [zone=N] [mark=X[/M]] [labels=Y[/N]] "
+ "[ct-orig-tuple [ct-reply-tuple]]",
+  0, 6, 

[ovs-dev] [PATCH v5 0/3] Extend the CT flush with mark and labels fields

[Ales Musil]

The CT flush is not capable of partial flush based
on the touples and zone combinations. Extend it
so it also accepts mark and labels. As part of this
series unify parsing of arguments in dpctl and ovs-ofctl
for the ct flush command.

Ales Musil (3):
  ofp-prop: Add helper for parsing and storing of ovs_u128.
  dpctl, ovs-ofctl: Unify parsing of ct-flush arguments.
  openflow: Allow CT flush to match on mark and labels.

 NEWS   |   3 +
 include/openflow/nicira-ext.h  |   4 +
 include/openvswitch/ofp-ct.h   |  14 ++-
 include/openvswitch/ofp-prop.h |   5 +
 lib/ct-dpif.c  |  12 ++-
 lib/dpctl.c|  46 ++---
 lib/ofp-ct.c   | 182 -
 lib/ofp-prop.c |  42 
 tests/ofp-print.at |  84 +++
 tests/ovs-ofctl.at |  36 +++
 tests/system-traffic.at| 112 +---
 utilities/ovs-ofctl.8.in   |  13 ++-
 utilities/ovs-ofctl.c  |  49 +++--
 13 files changed, 475 insertions(+), 127 deletions(-)

-- 
2.43.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v5 1/3] ofp-prop: Add helper for parsing and storing of ovs_u128.

[Ales Musil]

Add helper methods that allow us to store and parse the
ovs_u128 type.

Signed-off-by: Ales Musil 
---
v5: Rebase on top of current master.
v4: Rebase on top of current master.
Use ofprop_put instead of manual ofpbuf_start/put/end.
v3: Rebase on top of current master.
v2: Add missing ofpprop_parse_be128() function.
---
 include/openvswitch/ofp-prop.h |  5 
 lib/ofp-prop.c | 42 ++
 2 files changed, 47 insertions(+)

diff --git a/include/openvswitch/ofp-prop.h b/include/openvswitch/ofp-prop.h
index e676f8dc0..451f5dae2 100644
--- a/include/openvswitch/ofp-prop.h
+++ b/include/openvswitch/ofp-prop.h
@@ -84,10 +84,13 @@ enum ofperr ofpprop_pull(struct ofpbuf *msg, struct ofpbuf 
*property,
 enum ofperr ofpprop_parse_be16(const struct ofpbuf *, ovs_be16 *value);
 enum ofperr ofpprop_parse_be32(const struct ofpbuf *, ovs_be32 *value);
 enum ofperr ofpprop_parse_be64(const struct ofpbuf *, ovs_be64 *value);
+enum ofperr ofpprop_parse_be128(const struct ofpbuf *property,
+ovs_be128 *value);
 enum ofperr ofpprop_parse_u8(const struct ofpbuf *, uint8_t *value);
 enum ofperr ofpprop_parse_u16(const struct ofpbuf *, uint16_t *value);
 enum ofperr ofpprop_parse_u32(const struct ofpbuf *, uint32_t *value);
 enum ofperr ofpprop_parse_u64(const struct ofpbuf *, uint64_t *value);
+enum ofperr ofpprop_parse_u128(const struct ofpbuf *, ovs_u128 *value);
 enum ofperr ofpprop_parse_uuid(const struct ofpbuf *, struct uuid *);
 enum ofperr ofpprop_parse_nested(const struct ofpbuf *, struct ofpbuf *);
 
@@ -98,10 +101,12 @@ void *ofpprop_put_zeros(struct ofpbuf *, uint64_t type, 
size_t len);
 void ofpprop_put_be16(struct ofpbuf *, uint64_t type, ovs_be16 value);
 void ofpprop_put_be32(struct ofpbuf *, uint64_t type, ovs_be32 value);
 void ofpprop_put_be64(struct ofpbuf *, uint64_t type, ovs_be64 value);
+void ofpprop_put_be128(struct ofpbuf *, uint64_t type, ovs_be128 value);
 void ofpprop_put_u8(struct ofpbuf *, uint64_t type, uint8_t value);
 void ofpprop_put_u16(struct ofpbuf *, uint64_t type, uint16_t value);
 void ofpprop_put_u32(struct ofpbuf *, uint64_t type, uint32_t value);
 void ofpprop_put_u64(struct ofpbuf *, uint64_t type, uint64_t value);
+void ofpprop_put_u128(struct ofpbuf *, uint64_t type, ovs_u128 value);
 void ofpprop_put_bitmap(struct ofpbuf *, uint64_t type, uint64_t bitmap);
 void ofpprop_put_flag(struct ofpbuf *, uint64_t type);
 void ofpprop_put_uuid(struct ofpbuf *, uint64_t type, const struct uuid *);
diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
index 8b2d8a85a..2e323a08e 100644
--- a/lib/ofp-prop.c
+++ b/lib/ofp-prop.c
@@ -184,6 +184,20 @@ ofpprop_parse_be64(const struct ofpbuf *property, ovs_be64 
*value)
 return 0;
 }
 
+/* Attempts to parse 'property' as a property containing a 128-bit value.  If
+ * successful, stores the value into '*value' and returns 0; otherwise returns
+ * an OpenFlow error. */
+enum ofperr
+ofpprop_parse_be128(const struct ofpbuf *property, ovs_be128 *value)
+{
+ovs_be128 *p = property->msg;
+if (ofpbuf_msgsize(property) != sizeof *p) {
+return OFPERR_OFPBPC_BAD_LEN;
+}
+*value = *p;
+return 0;
+}
+
 /* Attempts to parse 'property' as a property containing a 8-bit value.  If
  * successful, stores the value into '*value' and returns 0; otherwise returns
  * an OpenFlow error. */
@@ -250,6 +264,20 @@ ofpprop_parse_u64(const struct ofpbuf *property, uint64_t 
*value)
 return 0;
 }
 
+/* Attempts to parse 'property' as a property containing a 128-bit value.  If
+ * successful, stores the value into '*value' and returns 0; otherwise returns
+ * an OpenFlow error. */
+enum ofperr
+ofpprop_parse_u128(const struct ofpbuf *property, ovs_u128 *value)
+{
+ovs_be128 *p = property->msg;
+if (ofpbuf_msgsize(property) != sizeof *p) {
+return OFPERR_OFPBPC_BAD_LEN;
+}
+*value = ntoh128(*p);
+return 0;
+}
+
 /* Attempts to parse 'property' as a property containing a UUID.  If
  * successful, stores the value into '*uuid' and returns 0; otherwise returns
  * an OpenFlow error. */
@@ -351,6 +379,13 @@ ofpprop_put_be64(struct ofpbuf *msg, uint64_t type, 
ovs_be64 value)
 ofpprop_end(msg, start);
 }
 
+/* Adds a property with the given 'type' and 128-bit 'value' to 'msg'. */
+void
+ofpprop_put_be128(struct ofpbuf *msg, uint64_t type, ovs_be128 value)
+{
+ofpprop_put(msg, type, , sizeof value);
+}
+
 /* Adds a property with the given 'type' and 8-bit 'value' to 'msg'. */
 void
 ofpprop_put_u8(struct ofpbuf *msg, uint64_t type, uint8_t value)
@@ -381,6 +416,13 @@ ofpprop_put_u64(struct ofpbuf *msg, uint64_t type, 
uint64_t value)
 ofpprop_put_be64(msg, type, htonll(value));
 }
 
+/* Adds a property with the given 'type' and 64-bit 'value' to 'msg'. */
+void
+ofpprop_put_u128(struct ofpbuf *msg, uint64_t type, ovs_u128 value)
+{
+ofpprop_put_be128(msg, type, hton128(value));
+}
+
 /* Appends a property to 'msg' whose type is 'type' 

Re: [ovs-dev] [PATCH ovn v2 09/18] northd: Add a new node ls_lbacls.

[Numan Siddique]

On Fri, Nov 24, 2023 at 5:54 AM Dumitru Ceara  wrote:
>
> On 10/26/23 20:15, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > This new engine now maintains the load balancer and ACL data of a
> > logical switch which was earlier part of northd engine node data.
> > The main inputs to this engine are:
> > - northd node
> > - NB logical switch node
> > - Port group node
> >
> > A record for each logical switch is maintained in the 'ls_lbacls'
> > hmap table and this record stores the below data which was earlier
> > part of 'struct ovn_datapath'.
> >
> > - bool has_stateful_acl;
> > - bool has_lb_vip;
> > - bool has_acls;
> > - uint64_t max_acl_tier;
> >
> > This engine node becomes an input to 'lflow' node.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  lib/stopwatch-names.h  |   1 +
> >  northd/automake.mk |   2 +
> >  northd/en-lflow.c  |   4 +
> >  northd/en-lr-lb-nat-data.c |   8 +-
> >  northd/en-lr-lb-nat-data.h |   2 +
> >  northd/en-ls-lb-acls.c | 527 +
> >  northd/en-ls-lb-acls.h |  88 +++
> >  northd/en-port-group.h |   3 +
> >  northd/inc-proc-northd.c   |   9 +
> >  northd/northd.c| 271 +--
> >  northd/northd.h|   7 +-
> >  11 files changed, 776 insertions(+), 146 deletions(-)
> >  create mode 100644 northd/en-ls-lb-acls.c
> >  create mode 100644 northd/en-ls-lb-acls.h
> >
> > diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> > index 7d85acdaea..8b0018a593 100644
> > --- a/lib/stopwatch-names.h
> > +++ b/lib/stopwatch-names.h
> > @@ -34,5 +34,6 @@
> >  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> >  #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
> >  #define LR_LB_NAT_DATA_RUN_STOPWATCH_NAME "lr_lb_nat_data"
> > +#define LS_LBACLS_RUN_STOPWATCH_NAME "lr_lb_acls"
> >
> >  #endif
> > diff --git a/northd/automake.mk b/northd/automake.mk
> > index 4116c487df..4593654726 100644
> > --- a/northd/automake.mk
> > +++ b/northd/automake.mk
> > @@ -28,6 +28,8 @@ northd_ovn_northd_SOURCES = \
> >   northd/en-lr-nat.h \
> >   northd/en-lr-lb-nat-data.c \
> >   northd/en-lr-lb-nat-data.h \
> > + northd/en-ls-lb-acls.c \
> > + northd/en-ls-lb-acls.h \
> >   northd/inc-proc-northd.c \
> >   northd/inc-proc-northd.h \
> >   northd/ipam.c \
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index 229f4be1d0..648a477916 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -21,6 +21,7 @@
> >  #include "en-lflow.h"
> >  #include "en-lr-nat.h"
> >  #include "en-lr-lb-nat-data.h"
> > +#include "en-ls-lb-acls.h"
> >  #include "en-northd.h"
> >  #include "en-meters.h"
> >
> > @@ -44,6 +45,8 @@ lflow_get_input_data(struct engine_node *node,
> >  engine_get_input_data("sync_meters", node);
> >  struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
> >  engine_get_input_data("lr_lb_nat_data", node);
> > +struct ed_type_ls_lbacls *ls_lbacls_data =
> > +engine_get_input_data("ls_lbacls", node);
> >
> >  lflow_input->nbrec_bfd_table =
> >  EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> > @@ -67,6 +70,7 @@ lflow_get_input_data(struct engine_node *node,
> >  lflow_input->lr_ports = _data->lr_ports;
> >  lflow_input->ls_port_groups = _data->ls_port_groups;
> >  lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
> > +lflow_input->ls_lbacls = _lbacls_data->ls_lbacls;
> >  lflow_input->meter_groups = _meters_data->meter_groups;
> >  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
> >  lflow_input->svc_monitor_map = _data->svc_monitor_map;
> > diff --git a/northd/en-lr-lb-nat-data.c b/northd/en-lr-lb-nat-data.c
> > index 19b638ce0b..d816d2321d 100644
> > --- a/northd/en-lr-lb-nat-data.c
> > +++ b/northd/en-lr-lb-nat-data.c
> > @@ -299,9 +299,11 @@ lr_lb_nat_data_lb_data_handler(struct engine_node 
> > *node, void *data_)
> >  if (!hmapx_is_empty(>tracked_data.crupdated)) {
> >  struct hmapx_node *hmapx_node;
> >  /* For all the modified lr_lb_nat_data records (re)build the
> > - * vip nats. */
> > + * vip nats and re-evaluate 'has_lb_vip'. */
> >  HMAPX_FOR_EACH (hmapx_node, >tracked_data.crupdated) {
> > -lr_lb_nat_data_build_vip_nats(hmapx_node->data);
> > +lr_lbnat_rec = hmapx_node->data;
> > +lr_lb_nat_data_build_vip_nats(lr_lbnat_rec);
> > +lr_lbnat_rec->has_lb_vip = od_has_lb_vip(lr_lbnat_rec->od);
> >  }
> >
> >  data->tracked = true;
> > @@ -523,6 +525,8 @@ lr_lb_nat_data_record_init(struct lr_lb_nat_data_record 
> > *lr_lbnat_rec,
> >  if (!nbr->n_nat) {
> >  lr_lb_nat_data_build_vip_nats(lr_lbnat_rec);
> >  }
> > +
> > +lr_lbnat_rec->has_lb_vip = od_has_lb_vip(lr_lbnat_rec->od);
> >  }
> >
> >  static struct lr_lb_nat_data_input
> > diff --git a/northd/en-lr-lb-nat-data.h 

Re: [ovs-dev] [PATCH ovn v2 08/18] northd: Don't commit dhcp response flows in the conntrack.

[Numan Siddique]

On Thu, Nov 23, 2023 at 4:26 PM Dumitru Ceara  wrote:
>
> On 11/15/23 07:45, Han Zhou wrote:
> > On Thu, Oct 26, 2023 at 11:16 AM  wrote:
> >>
> >> From: Numan Siddique 
> >>
> >> This is not required.
> >>
> > Thanks Numan for the fix. Could you provide a little more detail why this
> > is not required:
> > - Is it a bug fix? What's the impact?
> > - Shall we update the ovn-northd documentation for the related lflow
> > changes?
>
> I agree with Han, the commit log could be improved.  I think the
> ovn-northd documentation was already behind wrt this flow.  There was no
> explicit mention of it anywhere.
>
> I'm pretty sure it's safe to not commit dhcp responses in conntrack, we
> bypass conntrack for the requests anyway:
>
> https://github.com/ovn-org/ovn/blob/5ef2a08ade01d698f84e197987ea679d8978d2b9/northd/northd.c#L7215-L7224
>
> > - Is there a reason this is in the I-P patch series?
>
> Guessing again, I'm still reviewing the rest of the series, but probably
> to avoid an unnecessary dependency on logical switch data (if switches
> have stateful ACLs applied).

Correct.  I've squashed this commit with the next one in v3. so that
there is no confusion.  And updated the commit message
accordingly.

Numan

>
> Regards,
> Dumitru
>
> >
> > Thanks,
> > Han
> >
> >> Signed-off-by: Numan Siddique 
> >> ---
> >>  northd/northd.c | 8 ++--
> >>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index 1877cbc7df..c8a224d3cd 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -9223,9 +9223,7 @@ build_dhcpv4_options_flows(struct ovn_port *op,
> >>  >nbsp->dhcpv4_options->options, "lease_time");
> >>  ovs_assert(server_id && server_mac && lease_time);
> >>  const char *dhcp_actions =
> >> -(op->od->has_stateful_acl || op->od->has_lb_vip)
> >> - ? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit; next;"
> >> - : REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
> >> +REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
> >>  ds_clear();
> >>  ds_put_format(, "outport == %s && eth.src == %s "
> >>"&& ip4.src == %s && udp && udp.src == 67 "
> >> @@ -9308,9 +9306,7 @@ build_dhcpv6_options_flows(struct ovn_port *op,
> >>  ipv6_string_mapped(server_ip, );
> >>
> >>  const char *dhcp6_actions =
> >> -(op->od->has_stateful_acl || op->od->has_lb_vip)
> >> -? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit;
> > next;"
> >> -: REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
> >> +REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
> >>  ds_clear();
> >>  ds_put_format(, "outport == %s && eth.src == %s "
> >>"&& ip6.src == %s && udp && udp.src == 547
> > "
> >> --
> >> 2.41.0
> >>
> >> ___
> >> dev mailing list
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 07/18] northd: Generate logical router's LB and NAT flows using lr_lbnat_data.

[Numan Siddique]

On Thu, Nov 23, 2023 at 4:15 PM Dumitru Ceara  wrote:
>
> On 10/26/23 20:15, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > Previous commits added new engine nodes to store logical router's lb
> > and NAT data.  Make use of the data stored by these engine nodes
> > to generate logical flows related to router's LBs and NATs.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  northd/en-lflow.c  |   3 -
> >  northd/en-lr-lb-nat-data.h |   4 +
> >  northd/inc-proc-northd.c   |   1 -
> >  northd/northd.c| 752 -
> >  northd/northd.h|   1 -
> >  5 files changed, 496 insertions(+), 265 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index 9cb0ead3f0..229f4be1d0 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -42,8 +42,6 @@ lflow_get_input_data(struct engine_node *node,
> >  engine_get_input_data("port_group", node);
> >  struct sync_meters_data *sync_meters_data =
> >  engine_get_input_data("sync_meters", node);
> > -struct ed_type_lr_nat_data *lr_nat_data =
> > -engine_get_input_data("lr_nat", node);
> >  struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
> >  engine_get_input_data("lr_lb_nat_data", node);
> >
> > @@ -68,7 +66,6 @@ lflow_get_input_data(struct engine_node *node,
> >  lflow_input->ls_ports = _data->ls_ports;
> >  lflow_input->lr_ports = _data->lr_ports;
> >  lflow_input->ls_port_groups = _data->ls_port_groups;
> > -lflow_input->lr_nats = _nat_data->lr_nats;
> >  lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
> >  lflow_input->meter_groups = _meters_data->meter_groups;
> >  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
> > diff --git a/northd/en-lr-lb-nat-data.h b/northd/en-lr-lb-nat-data.h
> > index 9029aee339..ffe41cad73 100644
> > --- a/northd/en-lr-lb-nat-data.h
> > +++ b/northd/en-lr-lb-nat-data.h
> > @@ -56,6 +56,10 @@ struct lr_lb_nat_data_table {
> >  #define LR_LB_NAT_DATA_TABLE_FOR_EACH(LR_LB_NAT_REC, TABLE) \
> >  HMAP_FOR_EACH (LR_LB_NAT_REC, key_node, &(TABLE)->entries)
> >
> > +#define LR_LB_NAT_DATA_TABLE_FOR_EACH_IN_P(LR_LB_NAT_REC, JOBID, TABLE) \
> > +HMAP_FOR_EACH_IN_PARALLEL (LR_LB_NAT_REC, key_node, JOBID, \
> > +   &(TABLE)->entries)
> > +
> >  struct lr_lb_nat_data_tracked_data {
> >  /* Created or updated logical router with LB data. */
> >  struct hmapx crupdated; /* Stores 'struct lr_lb_nat_data_record'. */
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 369a151fa3..84627070a8 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -228,7 +228,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >  engine_add_input(_lflow, _sb_igmp_group, NULL);
> >  engine_add_input(_lflow, _northd, lflow_northd_handler);
> >  engine_add_input(_lflow, _port_group, lflow_port_group_handler);
> > -engine_add_input(_lflow, _lr_nat, NULL);
>
> Was this supposed to go in the previous patch, the one that adds
> en_lr_lb_nat_data?

Yes.  It should have been.  Addressed in v3.

Thanks



>
> >  engine_add_input(_lflow, _lr_lb_nat_data, NULL);
> >
> >  engine_add_input(_sync_to_sb_addr_set, _nb_address_set,
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 24df14c0de..1877cbc7df 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8854,18 +8854,14 @@ build_lrouter_groups(struct hmap *lr_ports, struct 
> > ovs_list *lr_list)
> >   */
> >  static void
> >  build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op,
> > -   uint32_t priority,
> > -   struct ovn_datapath *od,
> > -   const struct lr_nat_table 
> > *lr_nats,
> > -   struct hmap *lflows)
> > +uint32_t priority,
> > +const struct ovn_datapath *od,
> > +const struct lr_nat_record 
> > *lrnat_rec,
> > +struct hmap *lflows)
>
>
> Nit: indentation.

I had to do this way to keep under 80.


>
> >  {
> >  struct ds eth_src = DS_EMPTY_INITIALIZER;
> >  struct ds match = DS_EMPTY_INITIALIZER;
> >
> > -const struct lr_nat_record *lrnat_rec = lr_nat_table_find_by_index(
> > -lr_nats, op->od->index);
> > -ovs_assert(lrnat_rec);
> > -
> >  /* Self originated ARP requests/RARP/ND need to be flooded to the L2 
> > domain
> >   * (except on router ports).  Determine that packets are self 
> > originated
> >   * by also matching on source MAC. Matching on ingress port is not
> > @@ -8952,7 +8948,8 @@ lrouter_port_ipv6_reachable(const struct ovn_port *op,
> >   */
> >  static void
> >  build_lswitch_rport_arp_req_flow(const char *ips,

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

[Numan Siddique]

On Thu, Nov 23, 2023 at 9:04 AM Dumitru Ceara  wrote:
>
> On 11/17/23 23:05, Numan Siddique wrote:
> > On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:
> >>
> >> On Thu, Oct 26, 2023 at 11:15 AM  wrote:
> >>>
> >>> From: Numan Siddique 
> >>>
> >>> This new engine now maintains the NAT related data for each
> >>> logical router which was earlier maintained by the northd
> >>> engine node in the 'struct ovn_datapath'.  Main inputs to
> >>> this engine node are:
> >>>- northd
> >>>- NB logical router
> >>>
> >>> A record for each logical router is maintained in the 'lr_nats'
> >>> hmap table and this record
> >>>   - stores the ovn_nat's
> >>
> >> It seems the sentence is incomplete.
> >>
> >>>
> >>> Handlers are also added to handle the changes to both these
> >>> inputs.  This engine node becomes an input to 'lflow' node.
> >>> This essentially decouples the lr NAT data from the northd
> >>> engine node.
> >>>
> >>> Signed-off-by: Numan Siddique 
> >>> ---
> >>>  lib/ovn-util.c   |   6 +-
> >>>  lib/ovn-util.h   |   2 +-
> >>>  lib/stopwatch-names.h|   1 +
> >>>  northd/automake.mk   |   2 +
> >>>  northd/en-lflow.c|   5 +
> >>>  northd/en-lr-nat.c   | 498 +
> >>>  northd/en-lr-nat.h   | 134 ++
> >>>  northd/en-sync-sb.c  |  11 +-
> >>>  northd/inc-proc-northd.c |   9 +
> >>>  northd/northd.c  | 514 ++-
> >>>  northd/northd.h  |  32 ++-
> >>>  tests/ovn-northd.at  |  18 ++
> >>>  12 files changed, 877 insertions(+), 355 deletions(-)
> >>>  create mode 100644 northd/en-lr-nat.c
> >>>  create mode 100644 northd/en-lr-nat.h
>
> I would call these en-nat.{hc}, I think.  There's no NAT for switches.


In v3 I haven't renamed it.  I can rename it in the next version once
most of the review comments
are addressed  (it's a bit of a pain to rename and fix all the compiler errors).

But in this case I still prefer lr-nat (and the structures -
lr_nat_record, ... ) because,
 -  This engine node maintains the data for the NATs associated
with a logical router.  A router
 can have 0 or more NATs
 - We have a NB NAT table which is for each NAT entry, whereas
this engine node is for NATs associated with a router
and it could confuse and not convey the relationship between
router and NATs.
 -  This patch creates a 'lr_nat_record' entry for every logical
router even if it doesn't have any NATs.


>
> >>>
> >>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> >>> index 33105202f2..05e635a6b4 100644
> >>> --- a/lib/ovn-util.c
> >>> +++ b/lib/ovn-util.c
> >>> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
> >> sbrec_port_binding *binding,
> >>>  }
> >>>
> >>>  bool
> >>> -lport_addresses_is_empty(struct lport_addresses *laddrs)
> >>> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
> >>>  {
> >>>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> >>>  }
> >>> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
> >> *laddrs)
> >>>  {
> >>>  free(laddrs->ipv4_addrs);
> >>>  free(laddrs->ipv6_addrs);
> >>> +laddrs->ipv4_addrs = NULL;
> >>> +laddrs->ipv6_addrs = NULL;
> >>> +laddrs->n_ipv4_addrs = 0;
> >>> +laddrs->n_ipv6_addrs = 0;
> >>>  }
> >>>
> >>>  /* Returns a string of the IP address of 'laddrs' that overlaps with
> >> 'ip_s'.
> >>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> >>> index bff50dbde9..5805415885 100644
> >>> --- a/lib/ovn-util.h
> >>> +++ b/lib/ovn-util.h
> >>> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
> >> sbrec_port_binding *binding,
> >>>  bool extract_lrp_networks__(char *mac, char **networks, size_t
> >> n_networks,
> >>>  struct lport_addresses *laddrs);
> >>>
> >>> -bool lport_addresses_is_empty(struct lport_addresses *);
> >>> +bool lport_addresses_is_empty(const struct lport_addresses *);
> >>>  void destroy_lport_addresses(struct lport_addresses *);
> >>>  const char *find_lport_address(const struct lport_addresses *laddrs,
> >>> const char *ip_s);
> >>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> >>> index 3452cc71cf..0a16da211e 100644
> >>> --- a/lib/stopwatch-names.h
> >>> +++ b/lib/stopwatch-names.h
> >>> @@ -32,5 +32,6 @@
> >>>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
> >>>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
> >>>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> >>> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
> >>>
> >>>  #endif
> >>> diff --git a/northd/automake.mk b/northd/automake.mk
> >>> index cf622fc3c9..ae367a2a8b 100644
> >>> --- a/northd/automake.mk
> >>> +++ b/northd/automake.mk
> >>> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
> >>> northd/en-sync-from-sb.h \
> >>> northd/en-lb-data.c \
> >>> northd/en-lb-data.h \
> >>> +   northd/en-lr-nat.c 

Re: [ovs-dev] [PATCH ovn v2 04/18] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

[Numan Siddique]

On Thu, Nov 23, 2023 at 8:24 AM Dumitru Ceara  wrote:

> On 10/26/23 20:14, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > It also moves the logical router port IPv6 prefix delegation
> > updates to "sync-from-sb" engine node.
> >
> > Signed-off-by: Numan Siddique 
> > ---
>
> I think I agree in general with the patch; I'd like to see the new
> revision however because the check that Han also commented on [0] in
> northd_handle_sb_port_binding_changes() makes me wonder too if we're not
> missing cases (or if we should just relax the LSP check as well).
>
> [0]
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409451.html
>
>


> Aside from that I left a few minor comments below.
>
> Thanks,
> Dumitru
>
> >  northd/en-northd.c  |   2 +-
> >  northd/en-sync-sb.c |   3 +-
> >  northd/northd.c | 283 ++--
> >  northd/northd.h |   6 +-
> >  tests/ovn-northd.at |  31 -
> >  5 files changed, 198 insertions(+), 127 deletions(-)
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 96c2ce9f69..13e731cad9 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node
> *node,
> >  northd_get_input_data(node, _data);
> >
> >  if (!northd_handle_sb_port_binding_changes(
> > -input_data.sbrec_port_binding_table, >ls_ports)) {
> > +input_data.sbrec_port_binding_table, >ls_ports,
> >lr_ports)) {
> >  return false;
> >  }
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 2540fcfb97..a14c609acd 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void
> *data OVS_UNUSED)
> >  const struct engine_context *eng_ctx = engine_get_context();
> >  struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> >
> > -sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports);
> > +sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports,
> > + _data->lr_ports);
> >  engine_set_node_state(node, EN_UPDATED);
> >  }
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 9ce1b2cb5a..c9c7045755 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >  {
> >  sbrec_port_binding_set_datapath(op->sb, op->od->sb);
> >  if (op->nbrp) {
> > +/* Note: SB port binding options for router ports are set in
> > + * sync_pbs(). */
> > +
> >  /* If the router is for l3 gateway, it resides on a chassis
> >   * and its port type is "l3gateway". */
> >  const char *chassis_name = smap_get(>od->nbr->options,
> "chassis");
> > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >  sbrec_port_binding_set_type(op->sb, "patch");
> >  }
> >
> > -struct smap new;
> > -smap_init();
> >  if (is_cr_port(op)) {
> >  ovs_assert(sbrec_chassis_by_name);
> >  ovs_assert(sbrec_chassis_by_hostname);
> >  ovs_assert(sbrec_ha_chassis_grp_by_name);
> >  ovs_assert(active_ha_chassis_grps);
> > -const char *redirect_type = smap_get(>nbrp->options,
> > - "redirect-type");
> >
> >  if (op->nbrp->ha_chassis_group) {
> >  if (op->nbrp->n_gateway_chassis) {
> > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> *ovnsb_txn,
> >  /* Delete the legacy gateway_chassis from the pb. */
> >  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
> >  }
> > -smap_add(, "distributed-port", op->nbrp->name);
> > -
> > -bool always_redirect =
> > -!op->od->has_distributed_nat &&
> > -!l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
> > -
> > -if (redirect_type) {
> > -smap_add(, "redirect-type", redirect_type);
> > -/* XXX Why can't we enable always-redirect when
> redirect-type
> > - * is bridged? */
> > -if (!strcmp(redirect_type, "bridged")) {
> > -always_redirect = false;
> > -}
> > -}
> > -
> > -if (always_redirect) {
> > -smap_add(, "always-redirect", "true");
> > -}
> > -} else {
> > -if (op->peer) {
> > -smap_add(, "peer", op->peer->key);
> > -if (op->nbrp->ha_chassis_group ||
> > -op->nbrp->n_gateway_chassis) {
> > -char *redirect_name =
> > -ovn_chassis_redirect_name(op->nbrp->name);
> > -smap_add(, "chassis-redirect-port",
> redirect_name);
> > -

Re: [ovs-dev] [PATCH ovn v2 01/18] northd: Refactor the northd change tracking.

[Numan Siddique]

On Thu, Nov 23, 2023 at 6:19 AM Dumitru Ceara  wrote:

> On 11/15/23 19:22, Numan Siddique wrote:
> > On Wed, Nov 15, 2023 at 12:12 AM Han Zhou  wrote:
> >>
> >> On Thu, Oct 26, 2023 at 11:14 AM  wrote:
> >>>
> >>> From: Numan Siddique 
> >>>
> >>> northd engine tracking data now has the following tracking data
> >>>   - changed ovn_ports (right now only changed logical switch ports are
> >>> tracked.)
> >>>   - changed load balancers.
> >>>
> >>> This separation becomes easier to add lflow handling for these
> >>> changes in lflow northd engine handler.  This patch doesn't
> >>> handle the load balancer changes in lflow handler.  It will
> >>> be handled in upcoming commits.
> >>>
> >>> Signed-off-by: Numan Siddique 
> >>> ---
>
> Hi Numan,
>
> I have a couple of comments too below, nothing fundamental but
> hopefully something that would simplify the code a bit.
>
> Regards,
> Dumitru
>
> >>>  northd/en-lflow.c   |  11 +-
> >>>  northd/en-northd.c  |  13 +-
> >>>  northd/en-sync-sb.c |  10 +-
> >>>  northd/northd.c | 446 
> >>>  northd/northd.h |  63 ---
> >>>  tests/ovn-northd.at |  10 +-
> >>>  6 files changed, 313 insertions(+), 240 deletions(-)
> >>>
> >>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> >>> index 2b84fef0ef..96d03b7ada 100644
> >>> --- a/northd/en-lflow.c
> >>> +++ b/northd/en-lflow.c
> >>> @@ -108,8 +108,8 @@ lflow_northd_handler(struct engine_node *node,
> >>>  return false;
> >>>  }
> >>>
> >>> -/* Fall back to recompute if lb related data has changed. */
> >>> -if (northd_data->lb_changed) {
> >>> +/* Fall back to recompute if load balancers have changed. */
> >>> +if
> >> (northd_has_lbs_in_tracked_data(_data->trk_northd_changes)) {
> >>>  return false;
> >>>  }
> >>>
> >>> @@ -119,13 +119,14 @@ lflow_northd_handler(struct engine_node *node,
> >>>  struct lflow_input lflow_input;
> >>>  lflow_get_input_data(node, _input);
> >>>
> >>> -if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn,
> >>> -
> _data->tracked_ls_changes,
> >>> -_input,
> >> _data->lflows)) {
> >>> +if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn,
> >>> +
> >>  _data->trk_northd_changes.trk_ovn_ports,
> >>> +_input, _data->lflows)) {
> >>>  return false;
> >>>  }
> >>>
> >>>  engine_set_node_state(node, EN_UPDATED);
> >>> +
>
> Nit: unrelated?
>
> >>>  return true;
> >>>  }
> >>>
> >>> diff --git a/northd/en-northd.c b/northd/en-northd.c
> >>> index aa0f20f0c2..96c2ce9f69 100644
> >>> --- a/northd/en-northd.c
> >>> +++ b/northd/en-northd.c
> >>> @@ -230,15 +230,16 @@ northd_lb_data_handler(struct engine_node *node,
> >> void *data)
> >>> >ls_datapaths,
> >>> >lr_datapaths,
> >>> >lb_datapaths_map,
> >>> -   >lb_group_datapaths_map)) {
> >>> +   >lb_group_datapaths_map,
> >>> +   >trk_northd_changes)) {
> >>>  return false;
> >>>  }
> >>>
> >>> -/* Indicate the depedendant engine nodes that load balancer/group
> >>> - * related data has changed (including association to logical
> >>> - * switch/router). */
> >>> -nd->lb_changed = true;
> >>> -engine_set_node_state(node, EN_UPDATED);
> >>> +if (northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
> >>> +nd->change_tracked = true;
> >>> +engine_set_node_state(node, EN_UPDATED);
> >>> +}
> >>> +
> >>>  return true;
> >>>  }
> >>>
> >>> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> >>> index 2ec3bf54f8..2540fcfb97 100644
> >>> --- a/northd/en-sync-sb.c
> >>> +++ b/northd/en-sync-sb.c
> >>> @@ -236,7 +236,8 @@ sync_to_sb_lb_northd_handler(struct engine_node
> >> *node, void *data OVS_UNUSED)
> >>>  {
> >>>  struct northd_data *nd = engine_get_input_data("northd", node);
> >>>
> >>> -if (!nd->change_tracked || nd->lb_changed) {
> >>> +if (!nd->change_tracked ||
> >>> +northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
> >>>  /* Return false if no tracking data or if lbs changed. */
> >>>  return false;
> >>>  }
> >>> @@ -306,11 +307,14 @@ sync_to_sb_pb_northd_handler(struct engine_node
> >> *node, void *data OVS_UNUSED)
> >>>  }
> >>>
> >>>  struct northd_data *nd = engine_get_input_data("northd", node);
> >>> -if (!nd->change_tracked) {
> >>> +if (!nd->change_tracked ||
> >>> +northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
> >>> +/* Return false if no tracking data or if lbs changed. */
> >>>  return false;
> >>>  }
> >>>
> >>> -if (!sync_pbs_for_northd_ls_changes(>tracked_ls_changes)) {
> >>> +if 

Re: [ovs-dev] [PATCH ovn v2 03/18] tests: Add a couple of tests in ovn-northd for I-P.

[Numan Siddique]

On Thu, Nov 23, 2023 at 7:50 AM Dumitru Ceara  wrote:

> On 10/26/23 20:14, num...@ovn.org wrote:
> > From: Numan Siddique 
> >
> > These tests cover scenarios for load balancers and NATs
> > and check for the 'northd' and 'lflow' engine node
> > recompute and compute stats.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  tests/ovn-northd.at | 274 
> >  1 file changed, 274 insertions(+)
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 28c293473c..699f6cfdce 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10893,3 +10893,277 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> >  AT_CLEANUP
> >  ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Load balancer incremental processing with stateless ACLs])
> > +ovn_start
> > +
> > +check_engine_stats() {
> > +  node=$1
> > +  recompute=$2
> > +  compute=$3
> > +
> > +  echo "__file__:__line__: Checking engine stats for node $node :
> recompute - \
> > +$recompute : compute - $compute"
> > +
> > +  node_stat=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
> $node)
> > +  # node_stat will be of this format :
> > +  # - Node: lflow - recompute: 3 - compute: 0 - abort: 0
> > +  node_recompute_ct=$(echo $node_stat | cut -d '-' -f2 | cut -d ':' -f2)
> > +  node_compute_ct=$(echo $node_stat | cut -d '-' -f3 | cut -d ':' -f2)
> > +
> > +  if [[ "$recompute" == "norecompute" ]]; then
> > +# node should not be recomputed
> > +echo "Expecting $node recompute count - $node_recompute_ct to be 0"
> > +check test "$node_recompute_ct" -eq "0"
> > +  else
> > +echo "Expecting $node recompute count - $node_recompute_ct not to
> be 0"
> > +check test "$node_recompute_ct" -ne "0"
> > +  fi
> > +
> > +  if [[ "$compute" == "nocompute" ]]; then
> > +# node should not be computed
> > +echo "Expecting $node compute count - $node_compute_ct to be 0"
> > +check test "$node_compute_ct" -eq "0"
> > +  else
> > +echo "Expecting $node compute count - $node_compute_ct not to be 0"
> > +check test "$node_compute_ct" -ne "0"
> > +  fi
> > +}
>
> I think there's no difference between the 3 definitions of
> check_engine_stats() (this patch adds 2 of them, one already existed).
>
> It's probably best to factor it out and have a single definition (maybe
> in ovn-macros.at?).
>

Thanks for the review.

Ack.  Done.  Addressed in v3.

Numan


> > +
> > +# Test I-P for load balancers.
> > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine
> node
> > +# only.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb lb-add lb1 10.0.0.10:80 10.0.0.3:80
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl acl-add sw0 from-lport 1 1 allow-stateless
> > +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1 1 allow-stateless
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Clear the VIPs of lb1
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb clear load_balancer . vips
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb lb-del lb1
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd recompute nocompute
> > +check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +AT_CLEANUP
> > +])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Logical router incremental processing for NAT])
> > +
> > +ovn_start
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.11
> > +
> > +check_engine_stats() {
> > +  node=$1
> > +  recompute=$2
> > +  compute=$3
> > +
> > +  echo "__file__:__line__: Checking engine stats for node $node :
> recompute - \
> > +$recompute : compute - $compute"
> > +
> > +  node_stat=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
> $node)
> > +  # node_stat will be of this format :
> > +  # - Node: lflow - recompute: 3 - compute: 0 - abort: 0
> > +  node_recompute_ct=$(echo $node_stat | cut -d '-' -f2 | cut -d ':' -f2)
> > +  node_compute_ct=$(echo $node_stat | cut -d '-' -f3 | cut -d ':' -f2)
> > +
> > +  if [[ "$recompute" == "norecompute" ]]; then
> > +# node should not be recomputed
> > +echo "Expecting $node recompute count - $node_recompute_ct to be 0"
> > +check test "$node_recompute_ct" -eq "0"
> > +  else
> > +echo "Expecting $node recompute count - 

Re: [ovs-dev] [PATCH ovn v2 02/18] northd: Track ovn_datapaths in northd engine track data.

[Numan Siddique]

On Thu, Nov 23, 2023 at 6:24 AM Dumitru Ceara  wrote:

> On 11/15/23 06:30, Han Zhou wrote:
> > On Thu, Oct 26, 2023 at 11:14 AM  wrote:
> >>
> >> From: Numan Siddique 
> >>
> >> northd engine tracked data now also stores the logical switches
> >> and logical routers that got updated due to the changed load balancers.
> >>
> >> Eg 1.  For this command 'ovn-nbctl ls-lb-add sw0 lb1 -- lr-lb-add lr0
> >> lb1', northd engine tracking data will store 'sw0' and 'lr0'.
> >>
> >> Eg 2.  If load balancer lb1 is already associated with 'sw0' and 'lr0'
> >> then for this command 'ovn-nbctl set load_balancer 
> >> vips:10.0.0.10=20.0.0.20', northd engine tracking data will store
> >> 'sw0' and 'lr0'.
> >>
> >> An upcoming commit will make use of this tracked data.
> >>
> >> Signed-off-by: Numan Siddique 
> >> ---
> >>  northd/northd.c | 34 +-
> >>  northd/northd.h | 12 
> >>  2 files changed, 45 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index df22a9c658..9ce1b2cb5a 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -5146,6 +5146,8 @@ destroy_northd_data_tracked_changes(struct
> > northd_data *nd)
> >>  struct northd_tracked_data *trk_changes = >trk_northd_changes;
> >>  destroy_tracked_ovn_ports(_changes->trk_ovn_ports);
> >>  destroy_tracked_lbs(_changes->trk_lbs);
> >> +hmapx_clear(_changes->ls_with_changed_lbs.crupdated);
> >> +hmapx_clear(_changes->lr_with_changed_lbs.crupdated);
> >>  nd->change_tracked = false;
> >>  }
> >>
> >> @@ -5158,6 +5160,8 @@ init_northd_tracked_data(struct northd_data *nd)
> >>  hmapx_init(_changes->trk_ovn_ports.deleted);
> >>  hmapx_init(_changes->trk_lbs.crupdated);
> >>  hmapx_init(_changes->trk_lbs.deleted);
> >> +hmapx_init(_changes->ls_with_changed_lbs.crupdated);
> >> +hmapx_init(_changes->lr_with_changed_lbs.crupdated);
> >>  }
> >>
> >>  static void
> >> @@ -5169,6 +5173,8 @@ destroy_northd_tracked_data(struct northd_data
> *nd)
> >>  hmapx_destroy(_changes->trk_ovn_ports.deleted);
> >>  hmapx_destroy(_changes->trk_lbs.crupdated);
> >>  hmapx_destroy(_changes->trk_lbs.deleted);
> >> +hmapx_destroy(_changes->ls_with_changed_lbs.crupdated);
> >> +hmapx_destroy(_changes->lr_with_changed_lbs.crupdated);
> >>  }
> >>
> >>
> >> @@ -5179,7 +5185,10 @@ northd_has_tracked_data(struct
> northd_tracked_data
> > *trk_nd_changes)
> >>  || !hmapx_is_empty(_nd_changes->trk_ovn_ports.updated)
> >>  || !hmapx_is_empty(_nd_changes->trk_ovn_ports.deleted)
> >>  || !hmapx_is_empty(_nd_changes->trk_lbs.crupdated)
> >> -|| !hmapx_is_empty(_nd_changes->trk_lbs.deleted));
> >> +|| !hmapx_is_empty(_nd_changes->trk_lbs.deleted)
> >> +||
> > !hmapx_is_empty(_nd_changes->ls_with_changed_lbs.crupdated)
> >> +||
> > !hmapx_is_empty(_nd_changes->lr_with_changed_lbs.crupdated)
> >> +);
> >>  }
> >>
> >>  bool
> >> @@ -5188,6 +5197,8 @@ northd_has_only_ports_in_tracked_data(
> >>  {
> >>  return (hmapx_is_empty(_nd_changes->trk_lbs.crupdated)
> >>  && hmapx_is_empty(_nd_changes->trk_lbs.deleted)
> >> +&&
> > hmapx_is_empty(_nd_changes->ls_with_changed_lbs.crupdated)
> >> +&&
> > hmapx_is_empty(_nd_changes->lr_with_changed_lbs.crupdated)
> >>  && (!hmapx_is_empty(_nd_changes->trk_ovn_ports.created)
> >>  || !hmapx_is_empty(_nd_changes->trk_ovn_ports.updated)
> >>  ||
> !hmapx_is_empty(_nd_changes->trk_ovn_ports.deleted)));
> >> @@ -5828,6 +5839,9 @@ northd_handle_lb_data_changes(struct
> > tracked_lb_data *trk_lb_data,
> >> lb_dps->nb_ls_map) {
> >>  od = ls_datapaths->array[index];
> >>  init_lb_for_datapath(od);
> >> +
> >> +/* Add the ls datapath to the northd tracked data. */
> >> +hmapx_add(_changes->ls_with_changed_lbs.crupdated, od);
> >>  }
> >>
> >>  hmap_remove(lb_datapaths_map, _dps->hmap_node);
> >> @@ -5909,6 +5923,9 @@ northd_handle_lb_data_changes(struct
> > tracked_lb_data *trk_lb_data,
> >>
> >>  /* Re-evaluate 'od->has_lb_vip' */
> >>  init_lb_for_datapath(od);
> >> +
> >> +/* Add the ls datapath to the northd tracked data. */
> >> +hmapx_add(_changes->ls_with_changed_lbs.crupdated, od);
> >>  }
> >>
> >>  LIST_FOR_EACH (codlb, list_node, _lb_data->crupdated_lr_lbs) {
> >> @@ -5954,6 +5971,9 @@ northd_handle_lb_data_changes(struct
> > tracked_lb_data *trk_lb_data,
> >>
> >>  /* Re-evaluate 'od->has_lb_vip' */
> >>  init_lb_for_datapath(od);
> >> +
> >> +/* Add the lr datapath to the northd tracked data. */
> >> +hmapx_add(_changes->lr_with_changed_lbs.crupdated, od);
> >>  }
> >>
> >>  HMAP_FOR_EACH (clb, hmap_node, _lb_data->crupdated_lbs) {
> >> @@ -5968,6 +5988,9 @@ 

Re: [ovs-dev] [PATCH ovn v3 00/16] northd lflow incremental processing

[Numan Siddique]

Please see below the updated cover letter which has more details.

(Apologies for sending this as an html instead of plain text).


This patch series adds incremental processing in the lflow engine
node to handle changes to northd and other engine nodes.
Changed related to load balancers and NAT are mainly handled in
this patch series.

This patch series can also be found here -
https://github.com/numansiddique/ovn/tree/northd_lflow_ip/v1

Prior to this patch series, most of the changes to northd engine
resulted in full recomputation of logical flows. This series
aims to improve the performance of ovn-northd by adding the I-P
support. In order to add this support, some of the northd engine
node data (from struct ovn_datapath) is split and moved over to
new engine nodes - mainly related to load balancers, NAT and ACLs.

Below are the scale testing results done with these patches applied
using ovn-heater. The test ran the scenario -
ocp-500-density-heavy.yml [1].

With all the lflow I-P patches applied, the resuts are:

---
Min (s) Median (s) 90%ile (s) 99%ile (s) Max (s) Mean (s) Total (s) Count
Failed
---
Iteration Total 0.136883 1.129016 1.192001 1.204167 1.212728 0.665017
83.127099 125 0
Namespace.add_ports 0.005216 0.005736 0.007034 0.015486 0.018978 0.006211
0.776373 125 0
WorkerNode.bind_port 0.035030 0.046082 0.052469 0.058293 0.060311 0.045973
11.493259 250 0
WorkerNode.ping_port 0.005057 0.006727 1.047692 1.069253 1.071336 0.266896
66.724094 250 0
---

The results with the present main are:

---
Min (s) Median (s) 90%ile (s) 99%ile (s) Max (s) Mean (s) Total (s) Count
Failed
---
Iteration Total 0.135491 2.223805 3.311270 3.339078 3.345346 1.729172
216.146495 125 0
Namespace.add_ports 0.005380 0.005744 0.006819 0.018773 0.020800 0.006292
0.786532 125 0
WorkerNode.bind_port 0.034179 0.046055 0.053488 0.058801 0.071043 0.046117
11.529311 250 0
WorkerNode.ping_port 0.004956 0.006952 3.086952 3.191743 3.192807 0.791544
197.886026 250 0
---

[1] -
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml


v2 -> v3
---
* Addressed some of the review comments from Han and Dumitru. There
are still a few pending review comments which needs to be addressed
or discussed.

* Renamed the engine node from "lr_lbnat_data" to "lr_stateful"
(v3 patch 5).
* Renamed the engine node from "ls_lbacls" to "ls_stateful" (v3 patch 8).

* Removed v2 patch 2 from the series (northd: Track ovn_datapaths in
northd engine track data."). This patch is now part of v3 patch 7
(northd: Add a new node 'ls_stateful').

* Squashed v2 patch 8 (northd: Don't commit dhcp response flows in
the conntrack.) into v3 patch 7 (northd: Add a new node
'ls_stateful'.)


v1 -> v2

* Now also maintaing array indexes for ls_lbacls, lr_nat and
lr_lb_nat_data tables (similar to ovn_datapaths->array) to
make the lookup effecient. The same ovn_datapath->index
is reused.

* Made some signficant changes to 'struct lflow_ref' in lflow-mgr.c.
In v2 we don't use objdep_mgr to maintain the resource to lflow
references. Instead we maintain the 'struct lflow' pointer.
With this we don't need to maintain additional hmap of lflows.


Below is the brief description of this patch series. (This could help the
reviewers).


This patch series adds 3 new engine nodes
1. lr_nat:
It maintains the NAT related data for each logical router which was earlier
maintained by northd engine node in the 'struct ovn_datapath'

It has only one input node - "northd"

engine_add_input(_lr_nat, _northd, lr_nat_northd_handler);

Below NAT related data is stored for each logical router

struct lr_nat_record {
...
const struct ovn_datapath *od;

struct ovn_nat *nat_entries;
size_t n_nat_entries;

bool has_distributed_nat;

/* Set of nat external ips on the router. */
struct sset external_ips;

/* Set of nat external macs on the router. */
struct sset external_macs;

/* SNAT IPs owned by the router (shash of 'struct ovn_snat_ip'). */
struct shash snat_ips;

struct lport_addresses dnat_force_snat_addrs;
struct lport_addresses lb_force_snat_addrs;
bool lb_force_snat_router_ip;

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

[Daniel Ding]

Hi Dumitru!

On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara  wrote:

> On 12/5/23 13:58, Daniel Ding wrote:
> >
> >
> > On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > > wrote:
> >
> > Hi Daniel,
> >
> > Thanks for this new revision but why is it v3?  I don't think I saw
> v2
> > posted anywhere but maybe I missed it.
> >
> >
> > Sorry, that is my mistake.
> >
>
> No problem.
>
> >
> > On 12/5/23 08:33, Daniel Ding wrote:
> > > If the router has a snat rule and it's external ip isn't lrp
> address,
> > > when the arp request from other router for this external ip, will
> > > be drop, because of this external ip use same mac address as lrp,
> so
> > > can not forward to MC_FLOOD.
> > >
> > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> > whenever possible.")
> > > Reported-at: https://github.com/ovn-org/ovn/issues/209
> > 
> > >
> > > Signed-off-by: Daniel Ding  > >
> > > Acked-by: Dumitru Ceara  dce...@redhat.com>>
> >
> > Please don't add an "Acked-by: ... " if the person never explicitly
> > replied with "Acked-by: ... " on the previous version of the patch or
> > if you didn't have explicit agreement from that person to do so.
> >
> > Quoting from my previous reply to your v1, I said:
> >
> > "I think it makes sense to do what you're suggesting."
> >
> >
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> <
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >
> >
> > That doesn't mean I acked the patch.
> >
> >
> > Got it. Thx!
> >
>
> No worries.
>
> >
> > > ---
> > >  northd/northd.c | 18 +-
> > >  tests/ovn-northd.at  | 12 
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index e9cb906e2..99fb30f46 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > >  }
> > >  }
> > >
> > > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > > +
> > >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> > >  const struct nbrec_nat *nat = nat_entry->nb;
> > > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > >  }
> > >
> > >  if (!strcmp(nat->type, "snat")) {
> > > -continue;
> > > +if (nat_entry_is_v6(nat_entry)) {
> > > +if (sset_contains(_ips_v6,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_ips_v6, nat->external_ip);
> > > +} else {
> > > +if (sset_contains(_ips_v4,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(_ips_v4, nat->external_ip);
> > > +}
> >
> > Essentially this just makes sure we don't skip NAT entries and that
> we
> > consider unique external_ips.  I'm fine with relaxing the second
> part of
> > the condition in which case, as mentioned on v1, I think we can just
> > remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> >
> > With the following incremental change applied (it removes the block)
> > your test still passes:
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index df7d2d60a5..20efd3b74c 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> >  }
> >  }
> >
> > -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > -
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >  const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> >  continue;
> >  }
> >
> > -if (!strcmp(nat->type, "snat")) {
> > -if (nat_entry_is_v6(nat_entry)) {
> > -if (sset_contains(_ips_v6, nat->external_ip)) {
> > -continue;
> > -}
> > -sset_add(_ips_v6, nat->external_ip);
> > -} else {
> > 

Re: [ovs-dev] [PATCH v4] reconnect: Set defaults from environment variables.

[Aaron Conole]

Felix Huettner via dev  writes:

> This exposes the old constants regarding min backoff, max backoff and
> probe interval using environment variables. In case previously users
> wanted to tune the probe interval for all connections this required
> setting this setting in multiple locations. E.g. to configure the probe
> interval from a relay to the ovsdb server you need to call an appctl
> command after the relay has started up.
> The new environment variables make it easy to set them for all new
> connections. The existing configuration options for these settings stay
> in place and take precedence over the environment variables.
> In case the environment variables are not specified/invalid formatted we
> default to the previous defaults.
>
> Signed-off-by: Felix Huettner 
> ---

As before - testing the recheck.

Recheck-request: github-robot

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 0/3] Extend the CT flush with mark and labels fields

[Ilya Maximets]

On 11/28/23 07:27, Ales Musil wrote:
> 
> 
> On Mon, Nov 27, 2023 at 7:36 PM Ilya Maximets  > wrote:
> 
> On 11/16/23 16:08, Ales Musil wrote:
> > The CT flush is not capable of partial flush based
> > on the touples and zone combinations. Extend it
> > so it also accepts mark and labels. As part of this
> > series unify parsing of arguments in dpctl and ovs-ofctl
> > for the ct flush command.
> 
> Hi, Ales.  Thanks for the new version!
> 
> Are you planning to send a fix for unknown properties?
> I think, we need to get it before extending the functionality
> as we'll also need to backport it.
> 
> 
> I was under the impression that we want to do it after, my bad. I'll send a 
> fix so that this extension can be rebased on top later on.

Thanks!  I applied that separate fix and also the other set for
CT limits.  Please, rebase this patch set on top of the current
master, so we can continue from that.

BTW, there is a rebase artifact in the NEWS file in the patch #3.
And the trailing whitespace unrelated changes returned there.

Best regards, Ilya Maximets.

>  
> 
> 
> Best regards, Ilya Maximets.
> 
> >
> > Ales Musil (3):
> >   ofp-prop: Add helper for parsing and storing of ovs_u128.
> >   dpctl, ovs-ofctl: Unify parsing of ct-flush arguments.
> >   openflow: Allow CT flush to match on mark and labels.
> >
> >  NEWS                           |   4 +
> >  include/openflow/nicira-ext.h  |   4 +
> >  include/openvswitch/ofp-ct.h   |  14 ++-
> >  include/openvswitch/ofp-prop.h |   5 +
> >  lib/ct-dpif.c                  |  12 ++-
> >  lib/dpctl.c                    |  46 ++---
> >  lib/ofp-ct.c                   | 182 -
> >  lib/ofp-prop.c                 |  42 
> >  tests/ofp-print.at              |  85 
> +++
> >  tests/ovs-ofctl.at              |  42 +++-
> >  tests/system-traffic.at         | 112 
> +---
> >  utilities/ovs-ofctl.8.in        |  31 +++---
> >  utilities/ovs-ofctl.c          |  49 +++--
> >  13 files changed, 489 insertions(+), 139 deletions(-)
> >
> 
> 
> Thanks,
> Ales
> 
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA 
> 
> amu...@redhat.com  
> 
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v8 0/6] Expose CT limit via DB

[Ilya Maximets]

On 12/4/23 06:49, Ales Musil wrote:
> The series exposes CT limit via DB, adding
> user friendly ovs-vsctl interface. The DB value
> has priority before the dpctl interface, this is
> achieved by storing which datapath is protected.
> The dpctl will return an error if the limit is
> already set in DB for that datapath.
> 
> Ales Musil (6):
>   ct-dpif: Handle default zone limit the same way as other limits.
>   dpctl: Allow the default CT zone limit to de deleted.
>   ovs-vsctl: Add limit to CT zone.
>   vswitchd, ofproto-dpif: Propagate the CT limit from database.
>   ct-dpif: Enforce CT zone limit protection.
>   tests: Do not use zone 0 for CT limit system test.
> 
>  NEWS   |   7 ++
>  lib/conntrack.c|  14 ++--
>  lib/conntrack.h|   7 +-
>  lib/ct-dpif.c  |  53 ++---
>  lib/ct-dpif.h  |  16 ++--
>  lib/dpctl.c|  48 ---
>  lib/dpif-netdev.c  |  21 ++---
>  lib/dpif-netlink.c |  29 ++-
>  lib/dpif-provider.h|  24 +++---
>  ofproto/ofproto-dpif.c |  52 
>  ofproto/ofproto-dpif.h |   5 ++
>  ofproto/ofproto-provider.h |  13 +++
>  ofproto/ofproto.c  |  23 ++
>  ofproto/ofproto.h  |   4 +
>  tests/dpctl.at |   8 +-
>  tests/ovs-vsctl.at |  88 +++-
>  tests/system-traffic.at| 159 +
>  utilities/ovs-vsctl.8.in   |  31 ++--
>  utilities/ovs-vsctl.c  | 141 ++--
>  vswitchd/bridge.c  |  82 ++-
>  vswitchd/vswitch.ovsschema |  14 +++-
>  vswitchd/vswitch.xml   |  14 
>  22 files changed, 709 insertions(+), 144 deletions(-)
> 

Thanks, Ales!

I fixed a few typos and a misplaced NEWS entry and applied the set.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 ovn] ovn: add geneve PMTUD support

[Numan Siddique]

On Thu, Nov 30, 2023 at 10:07 AM Lorenzo Bianconi
 wrote:
>
> Introduce specif flows for E/W ICMPv{4,6} packets if tunnelled packets
> do not fit path MTU. This patch enable PMTUD for East/West Geneve traffic.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2241711
> Tested-by: Jaime Ruiz 
> Signed-off-by: Lorenzo Bianconi 

Hi Lorenzo,

Thanks for the fix.

I triggered fake multinode system test run on my private github repo
with this patch and the test fails for branch-22.03 [1]


[1] - 
https://github.com/numansiddique/ovn/actions/runs/7054318788/job/19203180738

So I fixed that and applied the patch to the main with the below
changes.  I'm in the process of backporting till branch-22.03.


diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
b/.github/workflows/ovn-fake-multinode-tests.yml
index 25610df534..b3ba82a30b 100644
--- a/.github/workflows/ovn-fake-multinode-tests.yml
+++ b/.github/workflows/ovn-fake-multinode-tests.yml
@@ -76,8 +76,8 @@ jobs:
   fail-fast: false
   matrix:
 cfg:
-- { branch: "main" }
-- { branch: "branch-22.03" }
+- { branch: "main", testsuiteflags: ""}
+- { branch: "branch-22.03", testsuiteflags: "-k 'basic test'" }
 name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
 env:
   RUNC_CMD: podman
@@ -176,7 +176,7 @@ jobs:

 - name: Run fake-multinode system tests
   run: |
-if ! sudo make check-multinode; then
+if ! sudo make check-multinode TESTSUITEFLAGS="${{
matrix.cfg.testsuiteflags }}"; then
   sudo podman exec -it ovn-central ovn-nbctl show || :
   sudo podman exec -it ovn-central ovn-sbctl show || :
   sudo podman exec -it ovn-chassis-1 ovs-vsctl show || :
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 9728546844..a77bd719e8 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2468,7 +2468,7 @@ output;
   a priority-120 flow that matches inport == cr-GW
!is_chassis_resident(cr-GW) where
   cr-GW is the chassis resident port of GW,
-  stores GW as inport.
+  stores GW as inport and advances to the next table.
 

 
--


Thanks
Numan






> ---
> Changes since v1:
> - add fix for vxlan and stt tunnels
> ---
>  NEWS|  1 +
>  controller/physical.c   | 45 +++
>  northd/northd.c | 24 ++
>  northd/ovn-northd.8.xml |  8 +
>  tests/multinode.at  | 69 +
>  tests/ovn-northd.at |  6 
>  6 files changed, 153 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index e10fb79dd..acb3b854f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,7 @@ Post v23.09.0
>  connection method and doesn't require additional probing.
>  external_ids:ovn-openflow-probe-interval configuration option for
>  ovn-controller no longer matters and is ignored.
> +  - Enable PMTU discovery on geneve tunnels for E/W traffic.
>
>  OVN v23.09.0 - 15 Sep 2023
>  --
> diff --git a/controller/physical.c b/controller/physical.c
> index ba88e1d8b..ce3b88d16 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -2446,6 +2446,51 @@ physical_run(struct physical_ctx *p_ctx,
>  , hc_uuid);
>  }
>
> +/* Add specif flows for E/W ICMPv{4,6} packets if tunnelled packets do 
> not
> + * fit path MTU.
> + */
> +HMAP_FOR_EACH (tun, hmap_node, p_ctx->chassis_tunnels) {
> +ofpbuf_clear();
> +if (tun->type == GENEVE) {
> +put_move(MFF_TUN_ID, 0, MFF_LOG_DATAPATH, 0, 24, );
> +put_move(p_ctx->mff_ovn_geneve, 16, MFF_LOG_OUTPORT, 0, 16,
> + );
> +put_move(p_ctx->mff_ovn_geneve, 0, MFF_LOG_INPORT, 0, 15,
> + );
> +} else if (tun->type == STT) {
> +put_move(MFF_TUN_ID, 0, MFF_LOG_DATAPATH, 0, 24, );
> +put_move(MFF_TUN_ID, 40, MFF_LOG_OUTPORT,  0, 16, );
> +put_move(MFF_TUN_ID, 24, MFF_LOG_INPORT, 0, 15, );
> +} else if (tun->type == VXLAN) {
> +put_move(MFF_TUN_ID, 0, MFF_LOG_DATAPATH, 0, 12, );
> +put_move(MFF_TUN_ID, 12, MFF_LOG_INPORT, 0, 12, );
> +} else {
> +OVS_NOT_REACHED();
> +}
> +put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, );
> +
> +/* IPv4 */
> +struct match match = MATCH_CATCHALL_INITIALIZER;
> +match_set_in_port(, tun->ofport);
> +match_set_dl_type(, htons(ETH_TYPE_IP));
> +match_set_nw_proto(, IPPROTO_ICMP);
> +match_set_icmp_type(, 3);
> +match_set_icmp_code(, 4);
> +
> +ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, 0, ,
> +, hc_uuid);
> +/* IPv6 */
> +match_init_catchall();
> +match_set_in_port(, tun->ofport);
> +match_set_dl_type(, htons(ETH_TYPE_IPV6));
> + 

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

[Dumitru Ceara]

On 12/5/23 13:58, Daniel Ding wrote:
> 
> 
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  > wrote:
> 
> Hi Daniel,
> 
> Thanks for this new revision but why is it v3?  I don't think I saw v2
> posted anywhere but maybe I missed it.
> 
>  
> Sorry, that is my mistake.
>  

No problem.

> 
> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> whenever possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> 
> >
> > Signed-off-by: Daniel Ding  >
> > Acked-by: Dumitru Ceara mailto:dce...@redhat.com>>
> 
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch or
> if you didn't have explicit agreement from that person to do so.
> 
> Quoting from my previous reply to your v1, I said:
> 
> "I think it makes sense to do what you're suggesting."
> 
> 
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>  
> 
> 
> That doesn't mean I acked the patch.
> 
> 
> Got it. Thx!
>  

No worries.

> 
> > ---
> >  northd/northd.c     | 18 +-
> >  tests/ovn-northd.at  | 12 
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >          }
> >      }
> > 
> > +    struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > +    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >          struct ovn_nat *nat_entry = >od->nat_entries[i];
> >          const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
> >          }
> > 
> >          if (!strcmp(nat->type, "snat")) {
> > -            continue;
> > +            if (nat_entry_is_v6(nat_entry)) {
> > +                if (sset_contains(_ips_v6, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_ips_v6, nat->external_ip);
> > +            } else {
> > +                if (sset_contains(_ips_v4, nat->external_ip)) {
> > +                    continue;
> > +                }
> > +                sset_add(_ips_v4, nat->external_ip);
> > +            }
> 
> Essentially this just makes sure we don't skip NAT entries and that we
> consider unique external_ips.  I'm fine with relaxing the second part of
> the condition in which case, as mentioned on v1, I think we can just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> 
> With the following incremental change applied (it removes the block)
> your test still passes:
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
>          }
>      }
> 
> -    struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> -    struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>      for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>          struct ovn_nat *nat_entry = >od->nat_entries[i];
>          const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct
> ovn_port *op,
>              continue;
>          }
> 
> -        if (!strcmp(nat->type, "snat")) {
> -            if (nat_entry_is_v6(nat_entry)) {
> -                if (sset_contains(_ips_v6, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v6, nat->external_ip);
> -            } else {
> -                if (sset_contains(_ips_v4, nat->external_ip)) {
> -                    continue;
> -                }
> -                sset_add(_ips_v4, nat->external_ip);
> -            }
> -        }
> -
>          /* Check if the ovn port has a network 

[ovs-dev] [PATCH v3 08/11] ci: Add make check-system-tso to GitHub actions ci.

[Eelco Chaudron]

This patch adds 'make check-system-tso' to the GitHub actions ci.

Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml |4 
 1 file changed, 4 insertions(+)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index db0a1ac39..d74668f61 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -187,6 +187,10 @@ jobs:
 dpdk: dpdk
 testsuite:check-system-userspace
 
+  - compiler: gcc
+dpdk: dpdk
+testsuite:check-system-tso
+
 steps:
 - name: checkout
   uses: actions/checkout@v3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 9/9] ci: Exclude tests that show random failures through GitHub actions.

[Eelco Chaudron]

On 5 Dec 2023, at 15:56, Ilya Maximets wrote:

> On 12/5/23 15:38, Eelco Chaudron wrote:
>>
>>
>> On 28 Nov 2023, at 8:52, Eelco Chaudron wrote:
>>
>>> On 27 Nov 2023, at 19:16, Ilya Maximets wrote:
>>>
 On 11/27/23 13:41, Eelco Chaudron wrote:
> I ran 80 series of full tests, and the following tests showed failures:

 Hi, Eelco.

 It looks like this patch should go before enabling the actual testing.
 Some of the previous patches in the sat failed CI because of these issues.
>>>
>>> ACK I noticed too, will move this be one of the first patches.
>
>  802.1ad - vlan_limit
>+2023-11-20T10:32:11.245Z|1|dpif_netdev(revalidator5)|ERR|internal
>  error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),
>  in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
>  
> packet_type(ns=0,id=0),eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),
>  eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x8100),
>  vlan(vid=100,pcp=0),encap(eth_type(0x86dd),ipv6(
>  src=::,dst=ff02::1:ff46:681b,label=0,proto=58,tclass=0,hlimit=255,
>  frag=no),icmpv6(type=135,code=0),nd(target=fe80::407e:4bff:fe46:681b,
>  sll=00:00:00:00:00:00,tll=00:00:00:00:00:00)))
>
> +2023-11-20T10:32:11.245Z|2|dpif(revalidator5)|WARN|netdev@ovs-netdev:
>  failed to put[modify] (Invalid argument)
>  ufid:ef1ca90c-dbd0-4ca7-9869-411bdffd1ece recirc_id(0),dp_hash(0/0),
>  
> skb_priority(0/0),in_port(2),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),
>  ct_mark(0/0),ct_label(0/0),packet_type(ns=0,id=0),
>  eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),eth_type(0x88a8),
>  vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>  vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x86dd),
>  ipv6(src=::/::,dst=ff02::1:ff46:681b/::,label=0/0,proto=58/0,
>  tclass=0/0,hlimit=255/0,frag=no),icmpv6(type=135/0,code=0/0),
>  nd(target=fe80::407e:4bff:fe46:681b/::,
>  sll=00:00:00:00:00:00/00:00:00:00:00:00,
>  tll=00:00:00:00:00:00/00:00:00:00:00:00))), actions:drop

 BTW, we should be able to fix this test now, since David fixed the
 revalidator pause/resume logic.
>>>
>>> Let me take a look at the test. I am not sure how to verify as I can not 
>>> make it fail easily.
>>>
>>
>> Looked at this, but it’s not revalidator related, it’s formatting done by 
>> the userspace part.
>
> Well, yes, but formatting is done by the revalidator and this
> formatting/parsing depends on the vlan_limit configuration.
> We just need to avoid the race between revalidation and the
> flow removal from the datapath.

That was my initial thought also so I made some changes to the netdev-specific 
function dpif_netdev_mask_from_nlattrs/dpif_netdev_flow_from_nlattrs (in line 
with what was done for tc). But it did not seem to solve the problem.

I’ll work on this outside of this patchset, and exclude it for now.

>>
>> Tried to replicate this but was not successful outside of GitHub actions. 
>> Will add this to my TODO, and revisit it later. For now will keep it 
>> excluded in this patch set.
>>
>>
>> //Eelco
>>
>> 
>>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 10/11] ci: Allow make check-dpdk to run the MFEX tests.

[Eelco Chaudron]

The testcase change will make sure the 'MFEX Configuration' test
will run without the need for scapy and it's auto generated tests.
In addition we exclude the traffic related MFEX tests from running
on GitHub actions due to limited resources.

Signed-off-by: Eelco Chaudron 
---
 .ci/dpdk-build.sh|2 +-
 .github/workflows/build-and-test.yml |2 +-
 python/test_requirements.txt |1 +
 tests/system-dpdk.at |6 +++---
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
index aa83e4464..d4c178ee0 100755
--- a/.ci/dpdk-build.sh
+++ b/.ci/dpdk-build.sh
@@ -38,7 +38,7 @@ function build_dpdk()
 # any DPDK driver.
 # check-dpdk unit tests requires testpmd and some net/ driver.
 DPDK_OPTS="$DPDK_OPTS -Denable_apps=test-pmd"
-enable_drivers="net/null,net/af_xdp,net/tap,net/virtio"
+enable_drivers="net/null,net/af_xdp,net/tap,net/virtio,net/pcap"
 DPDK_OPTS="$DPDK_OPTS -Denable_drivers=$enable_drivers"
 
 # Install DPDK using prefix.
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index e9a2714fb..1e92a0e2b 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -5,7 +5,7 @@ on: [push, pull_request]
 jobs:
   build-dpdk:
 env:
-  dependencies: gcc libbpf-dev libnuma-dev ninja-build pkgconf
+  dependencies: gcc libbpf-dev libnuma-dev libpcap-dev ninja-build pkgconf
   CC: gcc
   DPDK_GIT: https://dpdk.org/git/dpdk-stable
   DPDK_VER: 22.11.1
diff --git a/python/test_requirements.txt b/python/test_requirements.txt
index c85ce41ad..5043c71e2 100644
--- a/python/test_requirements.txt
+++ b/python/test_requirements.txt
@@ -2,4 +2,5 @@ netaddr
 pyftpdlib
 pyparsing
 pytest
+scapy
 tftpy
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 17742d20a..ffa31408d 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -837,6 +837,7 @@ dnl 
--
 dnl MFEX Autovalidator
 AT_SETUP([OVS-DPDK - MFEX Autovalidator])
 AT_KEYWORDS([dpdk])
+OVS_CHECK_GITHUB_ACTION()
 OVS_DPDK_PRE_CHECK()
 OVS_DPDK_START([--no-pci])
 AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev])
@@ -870,6 +871,7 @@ dnl 
--
 dnl MFEX Autovalidator Fuzzy
 AT_SETUP([OVS-DPDK - MFEX Autovalidator Fuzzy])
 AT_KEYWORDS([dpdk])
+OVS_CHECK_GITHUB_ACTION()
 OVS_DPDK_PRE_CHECK()
 OVS_DPDK_START([--no-pci])
 AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev])
@@ -904,13 +906,11 @@ dnl 
--
 AT_SETUP([OVS-DPDK - MFEX Configuration])
 AT_KEYWORDS([dpdk])
 OVS_DPDK_PRE_CHECK()
-AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
-AT_CHECK([$PYTHON3 $srcdir/mfex_fuzzy.py test_traffic.pcap 1], [], [stdout])
 OVS_DPDK_START([--no-pci])
 AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . 
other_config:pmd-cpu-mask=0x1])
 dnl Add userspace bridge and attach it to OVS
 AT_CHECK([ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev])
-AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dpdk 
options:dpdk-devargs=net_pcap1,rx_pcap=test_traffic.pcap,infinite_rx=1], [], 
[stdout], [stderr])
+AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dpdk 
options:dpdk-devargs=net_null0,no-rx=1], [], [stdout], [stderr])
 AT_CHECK([ovs-vsctl show], [], [stdout])
 
 AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set scalar 1], [2],

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 02/11] ci: Add make check-ovsdb-cluster tests to GitHub action ci.

[Eelco Chaudron]

This patch adds 'make check-ovsdb-cluster' tests to GitHub action ci.
In addition, this patch also makes sure this test and 'make check' do
not run as root.

Signed-off-by: Eelco Chaudron 
---
 .ci/linux-build.sh   |5 -
 .github/workflows/build-and-test.yml |3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 67c01a644..bb540703e 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -129,11 +129,14 @@ else
 build_ovs
 for testsuite in $TESTSUITE; do
 run_as_root=
+if [ "$testsuite" != "check" ] && \
+   [ "$testsuite" != "check-ovsdb-cluster" ] ; then
+run_as_root="sudo -E PATH=$PATH"
+fi
 if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
 sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
 [ "$(cat /proc/sys/vm/nr_hugepages)" = '1024' ]
 export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
-run_as_root="sudo -E PATH=$PATH"
 fi
 $run_as_root make $testsuite TESTSUITEFLAGS=${JOBS} RECHECK=yes
 done
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 09654205e..5d441157c 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -164,6 +164,9 @@ jobs:
 m32:  m32
 opts: --disable-ssl
 
+  - compiler: gcc
+testsuite:check-ovsdb-cluster
+
 steps:
 - name: checkout
   uses: actions/checkout@v3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 09/11] ci: Fix dpdk build cache key generation.

[Eelco Chaudron]

When new drivers are introduced, the cache key is not accurately computed.

Before the commit 1a1b3106d90e ("ci: Separate DPDK from OVS build."), the
DPDK build process was integrated in .ci/linux-{setup,build}.sh scripts,
where specific lines were employed to generate the key. Since it is now
separated in .ci/dpdk-{setup,build}.sh, this patch computes the key based
on the content of those dedicated scripts.

Fixes: 4e90baca89f0 ("system-dpdk: Run traffic tests.")
Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index d74668f61..e9a2714fb 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -30,7 +30,7 @@ jobs:
   # This also allows us to use cache from any branch as long as version
   # and a way we're building DPDK stays the same.
   run:  |
-grep -irE 'RTE_|DPDK|meson|ninja' .ci/dpdk-* > dpdk-ci-signature
+cat .ci/dpdk-* > dpdk-ci-signature
 grep -rwE 'DPDK_GIT|DPDK_VER' .github/ >> dpdk-ci-signature
 if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
 git ls-remote --heads $DPDK_GIT $DPDK_VER >> dpdk-ci-signature

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 07/11] ci: Add make check-system-userspace to GitHub actions ci.

[Eelco Chaudron]

This patch adds 'make check-system-userspace' to the GitHub actions ci.
The tests are not split into two seperate test runs as they complete in
around 10 minutes.

Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml |4 
 1 file changed, 4 insertions(+)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 586b0cdd9..db0a1ac39 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,6 +183,10 @@ jobs:
 testsuite:check-offloads
 test_range:   "100-"
 
+  - compiler: gcc
+dpdk: dpdk
+testsuite:check-system-userspace
+
 steps:
 - name: checkout
   uses: actions/checkout@v3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 06/11] ci: Add make check-offloads to GitHub actions ci.

[Eelco Chaudron]

This patch also adds the 'CHECK_GITHUB_ACTION' macro to skip
tests that won't execute successfully through GitHub actions.
We could not use the -k !keyword option, as it can not be
combined with a range of tests.

Signed-off-by: Eelco Chaudron 
---
 .ci/linux-build.sh   |2 +-
 .github/workflows/build-and-test.yml |7 +++
 tests/system-offloads-traffic.at |2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 05b944ead..90581c10b 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -131,7 +131,7 @@ else
 run_as_root=
 if [ "$testsuite" != "check" ] && \
[ "$testsuite" != "check-ovsdb-cluster" ] ; then
-run_as_root="sudo -E PATH=$PATH"
+run_as_root="sudo -E PATH=$PATH GITHUB_ACTIONS=$GITHUB_ACTIONS"
 fi
 if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
 sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 0b881ca91..586b0cdd9 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -176,6 +176,13 @@ jobs:
 testsuite:check-kernel
 test_range:   "100-"
 
+  - compiler: gcc
+testsuite:check-offloads
+test_range:   "-100"
+  - compiler: gcc
+testsuite:check-offloads
+test_range:   "100-"
+
 steps:
 - name: checkout
   uses: actions/checkout@v3
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 0bedee753..6bd49a3ee 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -192,6 +192,7 @@ AT_CLEANUP
 AT_SETUP([offloads - check interface meter offloading -  offloads disabled])
 AT_KEYWORDS([dp-meter])
 AT_SKIP_IF([test $HAVE_NC = "no"])
+OVS_CHECK_GITHUB_ACTION()
 OVS_TRAFFIC_VSWITCHD_START()
 
 AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps bands=type=drop 
rate=1'])
@@ -240,6 +241,7 @@ AT_CLEANUP
 
 AT_SETUP([offloads - check interface meter offloading -  offloads enabled])
 AT_KEYWORDS([offload-meter])
+OVS_CHECK_GITHUB_ACTION()
 CHECK_TC_INGRESS_PPS()
 AT_SKIP_IF([test $HAVE_NC = "no"])
 OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
other_config:hw-offload=true])

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 11/11] ci: Add make check-afxdp to GitHub actions ci.

[Eelco Chaudron]

This patch adds 'make check-afxdp' to the GitHub actions ci.
The tests are not split into two seperate test runs as they
complete in around 10 minutes.

Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml |4 
 1 file changed, 4 insertions(+)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 1e92a0e2b..710757693 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -191,6 +191,10 @@ jobs:
 dpdk: dpdk
 testsuite:check-system-tso
 
+  - compiler: gcc
+dpdk: dpdk
+testsuite:check-afxdp
+
 steps:
 - name: checkout
   uses: actions/checkout@v3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 05/11] ci: Add make check-kernel to GitHub actions ci.

[Eelco Chaudron]

This patch adds 'make check-kernel' to the GitHub actions ci.
However, to do this, some additional changes were needed. First,
some of the missing test and package dependencies had to be added.
Finally, we added an option to the GitHub run matrix that allows
the tests to be split up, to avoid lengthy single test runs.

Signed-off-by: Eelco Chaudron 
---
 .ci/linux-build.sh   |3 ++-
 .github/workflows/build-and-test.yml |   11 ++-
 python/test_requirements.txt |4 +++-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index bb540703e..05b944ead 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -138,7 +138,8 @@ else
 [ "$(cat /proc/sys/vm/nr_hugepages)" = '1024' ]
 export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
 fi
-$run_as_root make $testsuite TESTSUITEFLAGS=${JOBS} RECHECK=yes
+$run_as_root make $testsuite TESTSUITEFLAGS="${JOBS} ${TEST_RANGE}" \
+ RECHECK=yes
 done
 fi
 
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index acb57ac46..0b881ca91 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -76,7 +76,8 @@ jobs:
 env:
   dependencies: |
 automake libtool gcc bc libjemalloc2 libjemalloc-dev libssl-dev \
-llvm-dev libnuma-dev libpcap-dev selinux-policy-dev libbpf-dev
+llvm-dev libnuma-dev libpcap-dev selinux-policy-dev libbpf-dev \
+lftp libreswan
   ASAN:${{ matrix.asan }}
   UBSAN:   ${{ matrix.ubsan }}
   CC:  ${{ matrix.compiler }}
@@ -87,6 +88,7 @@ jobs:
   OPTS:${{ matrix.opts }}
   STD: ${{ matrix.std }}
   TESTSUITE:   ${{ matrix.testsuite }}
+  TEST_RANGE:  ${{ matrix.test_range }}
 
 name: linux ${{ join(matrix.*, ' ') }}
 runs-on: ubuntu-22.04
@@ -167,6 +169,13 @@ jobs:
   - compiler: gcc
 testsuite:check-ovsdb-cluster
 
+  - compiler: gcc
+testsuite:check-kernel
+test_range:   "-100"
+  - compiler: gcc
+testsuite:check-kernel
+test_range:   "100-"
+
 steps:
 - name: checkout
   uses: actions/checkout@v3
diff --git a/python/test_requirements.txt b/python/test_requirements.txt
index 6aaee13e3..c85ce41ad 100644
--- a/python/test_requirements.txt
+++ b/python/test_requirements.txt
@@ -1,3 +1,5 @@
-pytest
 netaddr
+pyftpdlib
 pyparsing
+pytest
+tftpy

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 04/11] ci: Exclude tests that show random failures through GitHub actions.

[Eelco Chaudron]

I ran 80 series of full tests, and the following tests showed failures:

 802.1ad - vlan_limit
   +2023-11-20T10:32:11.245Z|1|dpif_netdev(revalidator5)|ERR|internal
 error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),
 in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
 packet_type(ns=0,id=0),eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),
 eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x8100),
 vlan(vid=100,pcp=0),encap(eth_type(0x86dd),ipv6(
 src=::,dst=ff02::1:ff46:681b,label=0,proto=58,tclass=0,hlimit=255,
 frag=no),icmpv6(type=135,code=0),nd(target=fe80::407e:4bff:fe46:681b,
 sll=00:00:00:00:00:00,tll=00:00:00:00:00:00)))
   +2023-11-20T10:32:11.245Z|2|dpif(revalidator5)|WARN|netdev@ovs-netdev:
 failed to put[modify] (Invalid argument)
 ufid:ef1ca90c-dbd0-4ca7-9869-411bdffd1ece recirc_id(0),dp_hash(0/0),
 skb_priority(0/0),in_port(2),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),
 ct_mark(0/0),ct_label(0/0),packet_type(ns=0,id=0),
 eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),eth_type(0x88a8),
 vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
 vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x86dd),
 ipv6(src=::/::,dst=ff02::1:ff46:681b/::,label=0/0,proto=58/0,
 tclass=0/0,hlimit=255/0,frag=no),icmpv6(type=135/0,code=0/0),
 nd(target=fe80::407e:4bff:fe46:681b/::,
 sll=00:00:00:00:00:00/00:00:00:00:00:00,
 tll=00:00:00:00:00:00/00:00:00:00:00:00))), actions:drop

  conntrack - zones from other field, more tests
+2023-11-20T10:45:43.015Z|1|dpif(handler5)|WARN|system@ovs-system:
  execute ct(commit),3 failed (Invalid argument) on packet tcp,
  vlan_tci=0x,dl_src=42:7e:4b:46:68:1b,dl_dst=ba:72:4c:a5:31:6b,
  nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_tos=0,nw_ecn=0,nw_ttl=64,
  nw_frag=no,tp_src=53738,tp_dst=80,tcp_flags=psh|ack tcp_csum:e4a

  conntrack - limit by zone
./system-traffic.at:5154: ovs-appctl dpctl/ct-get-limits zone=0,1,2,3,4,5
--- -   2023-11-20 10:51:09.965375141 +
+++ /home/runner/work/ovs/ovs/tests/system-kmod-testsuite.dir/at-groups/
  114/stdout2023-11-20 10:51:09.956723756 +
@@ -1,5 +1,5 @@
 default limit=10
-zone=0,limit=5,count=5
+zone=0,limit=5,count=6

  conntrack - Multiple ICMP traverse
./system-traffic.at:7571: ovs-appctl dpctl/dump-conntrack | ...
  -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
--- -   2023-11-20 15:36:02.591051192 +
+++ /home/runner/work/ovs/ovs/tests/system-kmod-testsuite.dir/...
@@ -1,2 +1,9 @@
+tcp,orig=(src=10.1.1.7,dst=13.107.43.16,sport=,
  dport=),reply=(src=13.107.43.16,dst=10.1.1.7,sport=,
  dport=),protoinfo=(state=)
+tcp,orig=(src=10.1.1.7,dst=168.63.129.16,sport=,
  dport=),reply=(src=168.63.129.16,dst=10.1.1.7,sport=,
  dport=),protoinfo=(state=)
...
+tcp,orig=(src=20.22.98.201,dst=10.1.1.7,sport=,dport=),
  reply=(src=10.1.1.7,dst=20.22.98.201,sport=,dport=),
  protoinfo=(state=)

  conntrack - ct flush
+++ /home/runner/work/ovs/ovs/tests/system-kmod-testsuite.dir/...
@@ -1,3 +1,5 @@
+tcp,orig=(src=10.1.1.154,dst=13.107.42.16,sport=45300,dport=443),
  reply=(src=13.107.42.16,dst=10.1.1.154,sport=443,dport=45300),
  protoinfo=(state=ESTABLISHED)
+tcp,orig=(src=10.1.1.154,dst=20.72.125.48,sport=45572,dport=443),
  reply=(src=20.72.125.48,dst=10.1.1.154,sport=443,dport=45572),
  protoinfo=(state=ESTABLISHED)

As I do not see those failures when running these stand alone on the
same Ubuntu distribution, I've disabled first three tests for now.

For the other tests we narrowed the result to not include any of the local
IP addresses in the test results.

This patch also adds the 'CHECK_GITHUB_ACTION' macro to skip
tests that won't execute successfully through GitHub actions.
We could not use the -k !keyword option, as it can not be
combined with a range of tests.

Signed-off-by: Eelco Chaudron 
---
 tests/system-common-macros.at |4 
 tests/system-traffic.at   |   45 ++---
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 0113aae8b..0620be0c7 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -365,3 +365,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP],
 # OVS_CHECK_CT_CLEAR()
 m4_define([OVS_CHECK_CT_CLEAR],
 [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" 
ovs-vswitchd.log])])
+
+# OVS_CHECK_GITHUB_ACTION
+m4_define([OVS_CHECK_GITHUB_ACTION],
+[AT_SKIP_IF([test "$GITHUB_ACTIONS" = "true"])])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index a7d4ed83b..459e6e07b 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2584,133 +2584,133 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 
"in_port=1 packet=5054000a5
 AT_CHECK([ovs-ofctl -O OpenFlow13 

[ovs-dev] [PATCH v3 03/11] ci: Update the GitHub Ubuntu runner image to Ubuntu 22.04.

[Eelco Chaudron]

Updating this image is a requirement for the kernel system-traffic
tests to pass on Ubuntu. In addition, 20.04 might be replaced,
as soon as 24.04 comes out. Or we need to do this when it becomes
EOL in April 2025.

Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 5d441157c..acb57ac46 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -12,7 +12,7 @@ jobs:
 name: dpdk gcc
 outputs:
   dpdk_key: ${{ steps.gen_dpdk_key.outputs.key }}
-runs-on: ubuntu-20.04
+runs-on: ubuntu-22.04
 timeout-minutes: 30
 
 steps:
@@ -89,7 +89,7 @@ jobs:
   TESTSUITE:   ${{ matrix.testsuite }}
 
 name: linux ${{ join(matrix.*, ' ') }}
-runs-on: ubuntu-20.04
+runs-on: ubuntu-22.04
 timeout-minutes: 30
 
 strategy:

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 01/11] ci: Add JOBS variable to replace all the '-j4' instances.

[Eelco Chaudron]

Add a JOBS variable, which defaults to '-j4' but can be overwritten
with the same environment variable. This can be useful if you use
this linux-build.sh script outside of GitHub actions on a machine
with many cores.

Signed-off-by: Eelco Chaudron 
---
 .ci/linux-build.sh |9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index aa2ecc505..67c01a644 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -6,6 +6,7 @@ set -x
 CFLAGS_FOR_OVS="-g -O2"
 SPARSE_FLAGS=""
 EXTRA_OPTS="--enable-Werror"
+JOBS=${JOBS:-"-j4"}
 
 function install_dpdk()
 {
@@ -46,7 +47,7 @@ function build_ovs()
 configure_ovs $OPTS
 make selinux-policy
 
-make -j4
+make ${JOBS}
 }
 
 if [ "$DEB_PACKAGE" ]; then
@@ -122,8 +123,8 @@ if [ "$TESTSUITE" = 'test' ]; then
 configure_ovs
 
 export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
-make distcheck -j4 CFLAGS="${CFLAGS_FOR_OVS}" \
-TESTSUITEFLAGS=-j4 RECHECK=yes
+make distcheck ${JOBS} CFLAGS="${CFLAGS_FOR_OVS}" \
+TESTSUITEFLAGS=${JOBS} RECHECK=yes
 else
 build_ovs
 for testsuite in $TESTSUITE; do
@@ -134,7 +135,7 @@ else
 export DPDK_EAL_OPTIONS="--lcores 0@1,1@1,2@1"
 run_as_root="sudo -E PATH=$PATH"
 fi
-$run_as_root make $testsuite TESTSUITEFLAGS=-j4 RECHECK=yes
+$run_as_root make $testsuite TESTSUITEFLAGS=${JOBS} RECHECK=yes
 done
 fi
 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 00/11] ci: Add remaining check tests to GitHub actions.

[Eelco Chaudron]

This set of patches incorporates the remaining make check tests into
the GitHub action run. Notably, this inclusion does not appear to
extend the overall run time, as the newly added tests complete prior
to the existing ones.

V2:
  - Added patch to fix DPDK cache key generation.
v3:
  - Moved the disable test patch before the actual test inclusion to avoid
potential robot failures.
  - Split some patches as requested by Ilya
  - Update commit messages
  - Exclude MFEX trafic tests on GitHub, update MFEX configuration test to run
with a dummy port.

Eelco Chaudron (11):
  ci: Add JOBS variable to replace all the '-j4' instances.
  ci: Add make check-ovsdb-cluster tests to GitHub action ci.
  ci: Update the GitHub Ubuntu runner image to Ubuntu 22.04.
  ci: Exclude tests that show random failures through GitHub actions.
  ci: Add make check-kernel to GitHub actions ci.
  ci: Add make check-offloads to GitHub actions ci.
  ci: Add make check-system-userspace to GitHub actions ci.
  ci: Add make check-system-tso to GitHub actions ci.
  ci: Fix dpdk build cache key generation.
  ci: Allow make check-dpdk to run the MFEX tests.
  ci: Add make check-afxdp to GitHub actions ci.


 .ci/dpdk-build.sh|  2 +-
 .ci/linux-build.sh   | 15 ++
 .github/workflows/build-and-test.yml | 41 +
 python/test_requirements.txt |  5 +++-
 tests/system-common-macros.at|  4 +++
 tests/system-dpdk.at |  6 ++--
 tests/system-offloads-traffic.at |  2 ++
 tests/system-traffic.at  | 45 +++-
 8 files changed, 84 insertions(+), 36 deletions(-)


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 9/9] ci: Exclude tests that show random failures through GitHub actions.

[Ilya Maximets]

On 12/5/23 15:38, Eelco Chaudron wrote:
> 
> 
> On 28 Nov 2023, at 8:52, Eelco Chaudron wrote:
> 
>> On 27 Nov 2023, at 19:16, Ilya Maximets wrote:
>>
>>> On 11/27/23 13:41, Eelco Chaudron wrote:
 I ran 80 series of full tests, and the following tests showed failures:
>>>
>>> Hi, Eelco.
>>>
>>> It looks like this patch should go before enabling the actual testing.
>>> Some of the previous patches in the sat failed CI because of these issues.
>>
>> ACK I noticed too, will move this be one of the first patches.

  802.1ad - vlan_limit
+2023-11-20T10:32:11.245Z|1|dpif_netdev(revalidator5)|ERR|internal
  error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),
  in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
  
 packet_type(ns=0,id=0),eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),
  eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x8100),
  vlan(vid=100,pcp=0),encap(eth_type(0x86dd),ipv6(
  src=::,dst=ff02::1:ff46:681b,label=0,proto=58,tclass=0,hlimit=255,
  frag=no),icmpv6(type=135,code=0),nd(target=fe80::407e:4bff:fe46:681b,
  sll=00:00:00:00:00:00,tll=00:00:00:00:00:00)))

 +2023-11-20T10:32:11.245Z|2|dpif(revalidator5)|WARN|netdev@ovs-netdev:
  failed to put[modify] (Invalid argument)
  ufid:ef1ca90c-dbd0-4ca7-9869-411bdffd1ece recirc_id(0),dp_hash(0/0),
  skb_priority(0/0),in_port(2),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),
  ct_mark(0/0),ct_label(0/0),packet_type(ns=0,id=0),
  eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),eth_type(0x88a8),
  vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
  vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x86dd),
  ipv6(src=::/::,dst=ff02::1:ff46:681b/::,label=0/0,proto=58/0,
  tclass=0/0,hlimit=255/0,frag=no),icmpv6(type=135/0,code=0/0),
  nd(target=fe80::407e:4bff:fe46:681b/::,
  sll=00:00:00:00:00:00/00:00:00:00:00:00,
  tll=00:00:00:00:00:00/00:00:00:00:00:00))), actions:drop
>>>
>>> BTW, we should be able to fix this test now, since David fixed the
>>> revalidator pause/resume logic.
>>
>> Let me take a look at the test. I am not sure how to verify as I can not 
>> make it fail easily.
>>
> 
> Looked at this, but it’s not revalidator related, it’s formatting done by the 
> userspace part.

Well, yes, but formatting is done by the revalidator and this
formatting/parsing depends on the vlan_limit configuration.
We just need to avoid the race between revalidation and the
flow removal from the datapath.

> 
> Tried to replicate this but was not successful outside of GitHub actions. 
> Will add this to my TODO, and revisit it later. For now will keep it excluded 
> in this patch set.
> 
> 
> //Eelco
> 
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ofp-ct: Return error for unknown property in CT flush.

[Ilya Maximets]

On 12/4/23 19:22, Aaron Conole wrote:
> Ales Musil  writes:
> 
>> CT flush extension would silently ignore unknown properties,
>> which could lead to potential surprise by deleting more than
>> it was requested to. Return error on unknown property instead
>> to avoid this problem and at the same time inform the user
>> that the specified property is not supported.
>>
>> Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic match.")
>> Signed-off-by: Ales Musil 
>> ---
> 
> This is a test - please don't change the state in patchwork yet.
> 
> Recheck-request: github
> 

I hope it was enough time for testing. :)
I applied the change now.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 9/9] ci: Exclude tests that show random failures through GitHub actions.

[Eelco Chaudron]

On 28 Nov 2023, at 8:52, Eelco Chaudron wrote:

> On 27 Nov 2023, at 19:16, Ilya Maximets wrote:
>
>> On 11/27/23 13:41, Eelco Chaudron wrote:
>>> I ran 80 series of full tests, and the following tests showed failures:
>>
>> Hi, Eelco.
>>
>> It looks like this patch should go before enabling the actual testing.
>> Some of the previous patches in the sat failed CI because of these issues.
>
> ACK I noticed too, will move this be one of the first patches.
>>>
>>>  802.1ad - vlan_limit
>>>+2023-11-20T10:32:11.245Z|1|dpif_netdev(revalidator5)|ERR|internal
>>>  error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),
>>>  in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
>>>  
>>> packet_type(ns=0,id=0),eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),
>>>  eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x8100),
>>>  vlan(vid=100,pcp=0),encap(eth_type(0x86dd),ipv6(
>>>  src=::,dst=ff02::1:ff46:681b,label=0,proto=58,tclass=0,hlimit=255,
>>>  frag=no),icmpv6(type=135,code=0),nd(target=fe80::407e:4bff:fe46:681b,
>>>  sll=00:00:00:00:00:00,tll=00:00:00:00:00:00)))
>>>
>>> +2023-11-20T10:32:11.245Z|2|dpif(revalidator5)|WARN|netdev@ovs-netdev:
>>>  failed to put[modify] (Invalid argument)
>>>  ufid:ef1ca90c-dbd0-4ca7-9869-411bdffd1ece recirc_id(0),dp_hash(0/0),
>>>  skb_priority(0/0),in_port(2),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),
>>>  ct_mark(0/0),ct_label(0/0),packet_type(ns=0,id=0),
>>>  eth(src=42:7e:4b:46:68:1b,dst=33:33:ff:46:68:1b),eth_type(0x88a8),
>>>  vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>>>  vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x86dd),
>>>  ipv6(src=::/::,dst=ff02::1:ff46:681b/::,label=0/0,proto=58/0,
>>>  tclass=0/0,hlimit=255/0,frag=no),icmpv6(type=135/0,code=0/0),
>>>  nd(target=fe80::407e:4bff:fe46:681b/::,
>>>  sll=00:00:00:00:00:00/00:00:00:00:00:00,
>>>  tll=00:00:00:00:00:00/00:00:00:00:00:00))), actions:drop
>>
>> BTW, we should be able to fix this test now, since David fixed the
>> revalidator pause/resume logic.
>
> Let me take a look at the test. I am not sure how to verify as I can not make 
> it fail easily.
>

Looked at this, but it’s not revalidator related, it’s formatting done by the 
userspace part.

Tried to replicate this but was not successful outside of GitHub actions. Will 
add this to my TODO, and revisit it later. For now will keep it excluded in 
this patch set.


//Eelco



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ofp-ct: Return error for unknown property in CT flush.

[Ilya Maximets]

On 12/4/23 06:11, Ales Musil wrote:
> 
> 
> On Sat, Dec 2, 2023 at 12:21 AM Ilya Maximets  > wrote:
> 
> On 11/30/23 08:31, Ales Musil wrote:
> > CT flush extension would silently ignore unknown properties,
> > which could lead to potential surprise by deleting more than
> > it was requested to. Return error on unknown property instead
> > to avoid this problem and at the same time inform the user
> > that the specified property is not supported.
> >
> > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic 
> match.")
> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> > ---
> > v2: Rebase on top of current master.
> >     Address comments from Ilya:
> >     - Add the check also into ofp_ct_tuple_decode_nested.
> > ---
> >  lib/ofp-ct.c       | 11 +++
> >  tests/ofp-print.at  | 18 ++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c
> > index 85a9d8bec..53964cf09 100644
> > --- a/lib/ofp-ct.c
> > +++ b/lib/ofp-ct.c
> > @@ -31,6 +31,9 @@
> >  #include "openvswitch/ofp-prop.h"
> >  #include "openvswitch/ofp-util.h"
> >  #include "openvswitch/packets.h"
> > +#include "openvswitch/vlog.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(ofp_ct);
> > 
> >  static void
> >  ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple,
> > @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf 
> *property, struct ofp_ct_tuple *tuple,
> >          case NXT_CT_TUPLE_ICMP_CODE:
> >              error = ofpprop_parse_u8(, >icmp_code);
> >              break;
> > +
> > +        default:
> > +            error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type);
> > +            break;
> >          }
> > 
> >          if (error) {
> > @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, 
> bool *with_zone,
> >              }
> >              error = ofpprop_parse_u16(, zone_id);
> >              break;
> > +
> > +        default:
> > +            error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type);
> > +            break;
> >          }
> > 
> >          if (error) {
> > diff --git a/tests/ofp-print.at  
> b/tests/ofp-print.at 
> > index 14aa55416..b4b4c6685 100644
> > --- a/tests/ofp-print.at 
> > +++ b/tests/ofp-print.at 
> > @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\
> >  00 01 00 20 00 00 00 00 \
> >  00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 
> 00 \
> >  " | grep -q OFPBPC_BAD_VALUE], [0])
> > +
> > +AT_CHECK([ovs-ofctl ofp-print "\
> > +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \
> > +06 \
> > +00 00 00 00 00 00 00 \
> > +00 80 00 08 00 00 00 00 \
> > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr])
> > +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], 
> [0])
> 
> Hmm, the 'ofp_ct_match' is not really something user-visible.
> I think, it's better to point to the actual OpenFlow entity
> here, i.e. 'NXT_CT_FLUSH'.
> 
> > +
> > +AT_CHECK([ovs-ofctl ofp-print "\
> > +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \
> > +06 \
> > +00 00 00 00 00 00 00 \
> > +00 00 00 10 00 00 00 00 \
> > +00 80 00 08 00 50 00 00 \
> > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr])
> > +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], 
> [0])
> 
> Same here.  We do not have a direct replacement, but something like
> 'NXT_CT_TUPLE' might make sense as it is a prefix of the actual
> properties here.
> 
> If you agree, I can replace these while applying the patch.
> 
> WDYT?
> 
> 
> That sounds reasonable.

Thanks!  I made the change.
Applied and backported down to 3.1.

Best regards, Ilya Maximets.

>  
> 
> 
> Best regards, Ilya Maximets.
> 
> Thanks,
> Ales
> 
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA 
> 
> amu...@redhat.com  
> 
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tunnel: Do not carry source port from a previous tunnel.

[Ilya Maximets]

On 12/4/23 16:24, Eelco Chaudron wrote:
> 
> 
> On 1 Dec 2023, at 22:04, Ilya Maximets wrote:
> 
>> If a packet is received from a UDP tunnel, it has a source port
>> populated in the tunnel metadata.  This field cannot be read or
>> changed with OpenFlow or the tunnel configuration.  However, while
>> sending this packet to a different tunnel, the value remains in
>> the metadata and is being sent to the datapath to use as a source
>> port for this new tunnel.  Tunnel implementations largely ignore
>> this value, and it is a random value from a different tunnel
>> anyway.
>>
>> Clear it while sending to a different tunnel, so the unnecessary
>> information is not being passed to the datapath.  This additionally
>> allows traffic from one tunnel to anther to be offloaded to TC,
>> as TC doesn't allow setting the source port at all.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> I tested this patch on multiple datapath configurations, and it all seems to 
> pass.
> 
> Thanks for looking into this, and adding the test case.
> 
> //Eelco
> 
> Acked-by: Eelco Chaudron 
> 


Thanks, Eelco and Valdislav!

Applied.  This one is on the edge between a bug fix and enhancement,
but it seems to be low risk, so I backported it down to 2.17 as well.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

[Ilya Maximets]

On 12/4/23 14:18, Marcelo Leitner wrote:
> On Mon, Dec 04, 2023 at 01:57:50PM +0100, Ilya Maximets wrote:
>> On 12/4/23 13:40, Marcelo Leitner wrote:
>>> On Sat, Dec 02, 2023 at 12:06:59AM +0100, Ilya Maximets wrote:
>>> ...
 Adding a test case that demonstrates a scenario where the issue
 occurs - bridging of two tunnels.
>>>
>>> I get the fix and it LGTM. What I don't get is the test case.
>>> Considering that the attr was getting simply ignored, wouldn't the
>>> test case always succeed? That is, I don't see how ignoring tp_src
>>> would cause the test to fail. Can you please elaborate?
>>
>> Ignoring the tp_src is causing tc code to warn about kernel
>> acknowledgement mismatch.  Testsuite doesn't tolerate warnings,
>> so the test fails on checking the logs:
> 
> Nice.
> 
> ...
>> We do not check if the offloading is actually happening, but we
>> check if there is any miscommunication between ovs-vswitchd and
>> the kernel (TC).
>>
>> Does that make sense?
> 
> Yes, thanks.
> 
> Reviewed-by: Marcelo Ricardo Leitner 
> 


Thanks, everyone!
Applied and backported down to 2.17.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] system-dpdk: Wait for MTU changes to be applied.

[Kevin Traynor]

On 01/12/2023 14:29, David Marchand wrote:

Because a DPDK backed netdev configuration is done in an asynchronous way,
and a MTU change requires a reconfiguration, directly checking
ovs-vswitchd logs or querying ovsdb for the interface current MTU value
is racy.

Add synchronisation points on the interface MTU value in ovsdb as it
ensures that a netdev (re)configuration did happen.
With those synchronisation points in place, error messages may be checked
in logs afterward.

Fixes: bf47829116a8 ("tests: Add OVS-DPDK MTU unit tests.")
Signed-off-by: David Marchand
---
Changes since v1:
- dropped test output,


---
  tests/system-dpdk.at | 42 --
  1 file changed, 12 insertions(+), 30 deletions(-)



Thanks David, this removes the race.

Acked-by: Kevin Traynor 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [dpdk-latest] system-dpdk: Ignore net/ice error log about QinQ offloading.

[Kevin Traynor]

On 05/12/2023 08:36, David Marchand wrote:

The net/ice DPDK driver complains with an ERROR level log message if the
hw firmware only supports SVM (Single Vlan Mode).
DVM (Dual Vlan mode) seems required when using QinQ offloading.
OVS does not care about this offloading feature and configures nothing
on that topic.

While seeing this error log, some manual tests show that
untagged/tagged/"double" tagged packets (with 0x8100 ethertype)
are still received/transmitted fine.

Ignore this log waiting for a fix on the DPDK side.

Link: https://bugs.dpdk.org/show_bug.cgi?id=1331
Signed-off-by: David Marchand 
---
  tests/system-dpdk-macros.at | 1 +
  1 file changed, 1 insertion(+)

diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index dcdfa55741..c011487541 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -86,6 +86,7 @@ $1";/does not exist. The Open vSwitch kernel module is 
probably not loaded./d
  /does not support MTU configuration,/d
  /EAL: No \(available\|free\) .*hugepages reported/d
  /Failed to enable flow control/d
+/ice_vsi_config_outer_vlan_stripping(): Single VLAN mode (SVM) does not 
support qinq/d
  /Rx checksum offload is not supported on/d
  /TELEMETRY: No legacy callbacks, legacy socket not created/d"])
  ])


Thanks David

Acked-by: Kevin Traynor 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-dev, v3, 2/3] conntrack: Use helpers from committed connections.

[Aaron Conole]

Viacheslav Galaktionov  writes:

> On 12/4/23 11:23, Viacheslav Galaktionov wrote:
>>
>>
>> On 12/1/23 15:32, Aaron Conole wrote:
>>> Viacheslav Galaktionov  writes:
>>>
 On 11/27/23 19:33, Aaron Conole wrote:
> Viacheslav Galaktionov  writes:
>
>> On 11/22/23 16:14, Aaron Conole wrote:
 Viacheslav Galaktionov writes:

 When a packet hits a flow rule without an explicitly specified
 helper,
 OvS has to rely on automatic application layer gateway detection to
 find related connections. This works as long as services are
 running on
 their standard ports, e.g. when FTP servers use TCP port 21.

 However, sometimes it's necessary to run services on
 non-standard ports.
 In that case, there is no way for OvS to guess which protocol
 is used
 within a given flow. Of course, this means that no related
 connections
 can be recognized.

 When a connection is committed with a particular helper, it's
 reasonable
 to assume this helper will be used in subsequent CT actions,
 as long as
 they don't override it. Achieve this behaviour by using the
 committed
 connection's helper when a flow rule does not specify one.

 Signed-off-by: Viacheslav Galaktionov
 
 Acked-by: Ivan Malov 
 ---
     lib/conntrack.c | 9 +
     1 file changed, 9 insertions(+)
>>> Hi Viacheslav,
>>>
>>> Do you plan to send a v4 which has news and faq updated?
>> Hi Aaron!
>>
>> I was actually waiting for your comments, so I could address them and
>> update the NEWS and FAQ at the same time. I can send a v4 in the next
>> couple of days if you want.
> Sorry - that's my mistake.  Please send the v4.
>
 Hi Aaron,

 Sorry, I've only just now taken a proper look at the FAQ, and I don't
 see what
 I should change there. There doesn't seem to be any mention of
 this subject
 in the file and my patches actually make netdev datapaths behave
 in the same
 manner as system datapaths when it comes to conntrack helpers, so
 there's
 probably no point describing a difference that's no longer there.
>>> I was thinking that it would make sense to update the following from
>>> Documentation/faq/releases.rst:
>>>
   Q: Are all features available with all datapaths?

   A: Open vSwitch supports different datapaths on different
 platforms.  Each
   datapath has a different feature set: the following tables
 try to summarize
   the status.
>>> There is a table there where we try to list any compatibility or
>>> differences.  Helper behavior here is a difference in older versions so
>>> we could put something like:
>>>
>>>  Conntrack Helper Assign.    YES    YES 3.2
>>>
>>> Maybe it would introduce more confusion that it removes?  But since it
>>> is a behavior change which aligns the datapaths, it would be good to
>>> document it and that question does deal with some of it.
>>>
>> Ok, I guess that makes sense. I've looked at the Hyper-V datapath,
>> seems like
>> it only uses the helper specified in a given rule, so I'm going to
>> put a NO in
>> the table. I don't really have any means to test changes to Hyper-V,
>> so I can't
>> implement it myself. I hope that's okay.
> Although now it looks like Hyper-V can't use helpers at all. Should we
> change
> "Assign." to "Persist." to indicate that helpers persist across
> different flow
> rules? I'm not absolutely sure about this phrasing too, I think it may
> require
> some additional explanation somewhere?

Maybe Alin has some suggestion.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

[Daniel Ding]

On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara  wrote:

> Hi Daniel,
>
> Thanks for this new revision but why is it v3?  I don't think I saw v2
> posted anywhere but maybe I missed it.
>
>
Sorry, that is my mistake.


> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >
> > Signed-off-by: Daniel Ding 
> > Acked-by: Dumitru Ceara 
>
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch or
> if you didn't have explicit agreement from that person to do so.
>
> Quoting from my previous reply to your v1, I said:
>
> "I think it makes sense to do what you're suggesting."
>
>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>
> That doesn't mean I acked the patch.
>
>
Got it. Thx!


> > ---
> >  northd/northd.c | 18 +-
> >  tests/ovn-northd.at | 12 
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> >  }
> >  }
> >
> > +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> > +
> >  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >  struct ovn_nat *nat_entry = >od->nat_entries[i];
> >  const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> >  }
> >
> >  if (!strcmp(nat->type, "snat")) {
> > -continue;
> > +if (nat_entry_is_v6(nat_entry)) {
> > +if (sset_contains(_ips_v6, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(_ips_v4, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(_ips_v4, nat->external_ip);
> > +}
>
> Essentially this just makes sure we don't skip NAT entries and that we
> consider unique external_ips.  I'm fine with relaxing the second part of
> the condition in which case, as mentioned on v1, I think we can just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>
> With the following incremental change applied (it removes the block)
> your test still passes:
>
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  }
>  }
>
> -struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> -
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  continue;
>  }
>
> -if (!strcmp(nat->type, "snat")) {
> -if (nat_entry_is_v6(nat_entry)) {
> -if (sset_contains(_ips_v6, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_ips_v6, nat->external_ip);
> -} else {
> -if (sset_contains(_ips_v4, nat->external_ip)) {
> -continue;
> -}
> -sset_add(_ips_v4, nat->external_ip);
> -}
> -}
> -
>  /* Check if the ovn port has a network configured on which we
> could
>   * expect ARP requests/NS for the DNAT external_ip.
>   */
> @@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
>  if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
>  build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
>  }
> -
> -sset_destroy(_ips_v4);
> -sset_destroy(_ips_v6);
>  }
>
>
If the nat_entries has multiple snats with the same external_ip, I think it
should check exernal_ip whether in a string hash. In addition, the
snat_and_dnat entry is working normally, so exclude it.

 static void
> ---
>
> Best regards,
> Dumitru
>
> >  }
> >
> >  /* Check if the ovn port has a network configured on which we
> could
> > @@ -9025,6 +9038,9 @@ 

Re: [ovs-dev] [PATCH ovn] ofctrl: Track the OvS flow update time

[Ales Musil]

On Tue, Dec 5, 2023 at 11:32 AM Dumitru Ceara  wrote:

> On 12/5/23 10:10, Ales Musil wrote:
> > Add unixctl command called "ofctrl/flow-install-time"
> > that returns the last time it took OvS to process
> > and install all flows. The initial time is taken right
> > before controller queues the updates to rconn.
> > The end is marked when we receive barrier reply with
> > corresponding xid.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-134
> > Signed-off-by: Ales Musil 
> > ---
> >  controller/ofctrl.c | 31 +--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
>
> Hi Ales,
>
> Thanks for the patch!
>

Hi Dumitru,

thank you for the review.


>
> This is supposed to be a rather "stable" command as the CMS wants to
> rely on this to gather relevant load statistics.  We should make sure we
> document its behavior in ovn-controller.8.xml and mention it in the NEWS
> file.
>

You are right, it should definitely be documented.


>
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 7aac0128b..4f8b80012 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -51,6 +51,7 @@
> >  #include "openvswitch/rconn.h"
> >  #include "socket-util.h"
> >  #include "timeval.h"
> > +#include "unixctl.h"
> >  #include "util.h"
> >  #include "vswitch-idl.h"
> >
> > @@ -323,6 +324,7 @@ struct ofctrl_flow_update {
> >  struct ovs_list list_node;  /* In 'flow_updates'. */
> >  ovs_be32 xid;   /* OpenFlow transaction ID for barrier.
> */
> >  uint64_t req_cfg;   /* Requested sequence number. */
> > +long long start_msec;   /* Timestamp when the update started. */
> >  };
> >
> >  static struct ofctrl_flow_update *
> > @@ -402,6 +404,10 @@ static enum mf_field_id mff_ovn_geneve;
> >   * (e.g. after OVS restart). */
> >  static bool ofctrl_initial_clear;
> >
> > +/* The time in ms it took for the last flow installation to be processed
> > + * by OvS. */
> > +static long long last_flow_install_time_ms = 0;
> > +
> >  static ovs_be32 queue_msg(struct ofpbuf *);
> >
> >  static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
> > @@ -413,6 +419,8 @@ static struct ofpbuf *encode_meter_mod(const struct
> ofputil_meter_mod *);
> >  static void ovn_installed_flow_table_clear(void);
> >  static void ovn_installed_flow_table_destroy(void);
> >
> > +static void flow_install_time_report(struct unixctl_conn *conn, int
> argc,
> > + const char *argv[], void *param);
> >
> >  static void ofctrl_recv(const struct ofp_header *, enum ofptype);
> >
> > @@ -429,6 +437,8 @@ ofctrl_init(struct ovn_extend_table *group_table,
> >  groups = group_table;
> >  meters = meter_table;
> >  shash_init(_bands);
> > +unixctl_command_register("ofctrl/flow-install-time", "", 0, 0,
> > + flow_install_time_report, NULL);
> >  }
> >
> >  /* S_NEW, for a new connection.
> > @@ -732,6 +742,9 @@ recv_S_UPDATE_FLOWS(const struct ofp_header *oh,
> enum ofptype type,
> >  if (fup->req_cfg >= cur_cfg) {
> >  cur_cfg = fup->req_cfg;
> >  }
> > +
> > +last_flow_install_time_ms = time_msec() - fup->start_msec;
>
> Nit: very unlikely but this can underflow.
>

As discussed offline this very unlikely is closer to almost impossible.


>
> Nit2: too many empty lines IMO.  I'd just move this line immediately
> before the "if (fup->req_cfg >= cur_cfg) {".
>

Done in v2.


>
> > +
> >  mem_stats.oflow_update_usage -=
> ofctrl_flow_update_size(fup);
> >  ovs_list_remove(>list_node);
> >  free(fup);
> > @@ -2900,6 +2913,7 @@ ofctrl_put(struct ovn_desired_flow_table
> *lflow_table,
> >  const struct ofp_header *oh = barrier->data;
> >  ovs_be32 xid_ = oh->xid;
> >  ovs_list_push_back(, >list_node);
> > +long long flow_update_start = time_msec();
> >
> >  /* Queue the messages. */
> >  struct ofpbuf *msg;
> > @@ -2945,9 +2959,13 @@ ofctrl_put(struct ovn_desired_flow_table
> *lflow_table,
> >
> >  /* Add a flow update. */
> >  fup = xmalloc(sizeof *fup);
> > +*fup = (struct ofctrl_flow_update) {
> > +.xid = xid_,
> > +.req_cfg = req_cfg,
> > +.start_msec = flow_update_start,
> > +};
> > +
> >  ovs_list_push_back(_updates, >list_node);
> > -fup->xid = xid_;
> > -fup->req_cfg = req_cfg;
> >  mem_stats.oflow_update_usage += ofctrl_flow_update_size(fup);
> >  done:;
> >  } else if (!ovs_list_is_empty(_updates)) {
> > @@ -3090,3 +3108,12 @@ ofctrl_get_memory_usage(struct simap *usage)
> > ROUND_UP(rconn_packet_counter_n_bytes(tx_counter),
> 1024)
> > / 1024);
> >  }
> > +
> > +static void
> > +flow_install_time_report(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > + const char 

[ovs-dev] [PATCH ovn v2] ofctrl: Track the OvS flow update time

[Ales Musil]

Add unixctl command called "ofctrl/flow-install-time"
that returns the last time it took OvS to process
and install all flows. The initial time is taken right
before controller queues the updates to rconn.
The end is marked when we receive barrier reply with
corresponding xid.

Reported-at: https://issues.redhat.com/browse/FDP-134
Signed-off-by: Ales Musil 
---
v2: Address comments from Dumitru:
- Add NEWS and ovn-controller.8.xml entry.
---
 NEWS|  2 ++
 controller/ofctrl.c | 31 +--
 controller/ovn-controller.8.xml |  6 ++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index acb3b854f..b2ece860f 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,8 @@ Post v23.09.0
 external_ids:ovn-openflow-probe-interval configuration option for
 ovn-controller no longer matters and is ignored.
   - Enable PMTU discovery on geneve tunnels for E/W traffic.
+  - Add "ovn-appctl ofctrl/flow-install-time" that returns how long the last
+flow processing and installation took in OvS.
 
 OVN v23.09.0 - 15 Sep 2023
 --
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 7aac0128b..ef5afe662 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -51,6 +51,7 @@
 #include "openvswitch/rconn.h"
 #include "socket-util.h"
 #include "timeval.h"
+#include "unixctl.h"
 #include "util.h"
 #include "vswitch-idl.h"
 
@@ -323,6 +324,7 @@ struct ofctrl_flow_update {
 struct ovs_list list_node;  /* In 'flow_updates'. */
 ovs_be32 xid;   /* OpenFlow transaction ID for barrier. */
 uint64_t req_cfg;   /* Requested sequence number. */
+long long start_msec;   /* Timestamp when the update started. */
 };
 
 static struct ofctrl_flow_update *
@@ -402,6 +404,10 @@ static enum mf_field_id mff_ovn_geneve;
  * (e.g. after OVS restart). */
 static bool ofctrl_initial_clear;
 
+/* The time in ms it took for the last flow installation to be processed
+ * by OvS. */
+static long long last_flow_install_time_ms = 0;
+
 static ovs_be32 queue_msg(struct ofpbuf *);
 
 static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
@@ -413,6 +419,8 @@ static struct ofpbuf *encode_meter_mod(const struct 
ofputil_meter_mod *);
 static void ovn_installed_flow_table_clear(void);
 static void ovn_installed_flow_table_destroy(void);
 
+static void flow_install_time_report(struct unixctl_conn *conn, int argc,
+ const char *argv[], void *param);
 
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
 
@@ -429,6 +437,8 @@ ofctrl_init(struct ovn_extend_table *group_table,
 groups = group_table;
 meters = meter_table;
 shash_init(_bands);
+unixctl_command_register("ofctrl/flow-install-time", "", 0, 0,
+ flow_install_time_report, NULL);
 }
 
 /* S_NEW, for a new connection.
@@ -729,9 +739,12 @@ recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum 
ofptype type,
 struct ofctrl_flow_update *fup = ofctrl_flow_update_from_list_node(
 ovs_list_front(_updates));
 if (fup->xid == oh->xid) {
+last_flow_install_time_ms = time_msec() - fup->start_msec;
+
 if (fup->req_cfg >= cur_cfg) {
 cur_cfg = fup->req_cfg;
 }
+
 mem_stats.oflow_update_usage -= ofctrl_flow_update_size(fup);
 ovs_list_remove(>list_node);
 free(fup);
@@ -2900,6 +2913,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 const struct ofp_header *oh = barrier->data;
 ovs_be32 xid_ = oh->xid;
 ovs_list_push_back(, >list_node);
+long long flow_update_start = time_msec();
 
 /* Queue the messages. */
 struct ofpbuf *msg;
@@ -2945,9 +2959,13 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 
 /* Add a flow update. */
 fup = xmalloc(sizeof *fup);
+*fup = (struct ofctrl_flow_update) {
+.xid = xid_,
+.req_cfg = req_cfg,
+.start_msec = flow_update_start,
+};
+
 ovs_list_push_back(_updates, >list_node);
-fup->xid = xid_;
-fup->req_cfg = req_cfg;
 mem_stats.oflow_update_usage += ofctrl_flow_update_size(fup);
 done:;
 } else if (!ovs_list_is_empty(_updates)) {
@@ -3090,3 +3108,12 @@ ofctrl_get_memory_usage(struct simap *usage)
ROUND_UP(rconn_packet_counter_n_bytes(tx_counter), 1024)
/ 1024);
 }
+
+static void
+flow_install_time_report(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *param OVS_UNUSED)
+{
+char *msg = xasprintf("%lld ms", last_flow_install_time_ms);
+unixctl_command_reply(conn, msg);
+free(msg);
+}
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 735bc1221..a5ed8fcd5 100644
--- 

Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.

[Dumitru Ceara]

Hi Daniel,

Thanks for this new revision but why is it v3?  I don't think I saw v2
posted anywhere but maybe I missed it.

On 12/5/23 08:33, Daniel Ding wrote:
> If the router has a snat rule and it's external ip isn't lrp address,
> when the arp request from other router for this external ip, will
> be drop, because of this external ip use same mac address as lrp, so
> can not forward to MC_FLOOD.
> 
> Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever 
> possible.")
> Reported-at: https://github.com/ovn-org/ovn/issues/209
> 
> Signed-off-by: Daniel Ding 
> Acked-by: Dumitru Ceara 

Please don't add an "Acked-by: ... " if the person never explicitly
replied with "Acked-by: ... " on the previous version of the patch or
if you didn't have explicit agreement from that person to do so.

Quoting from my previous reply to your v1, I said:

"I think it makes sense to do what you're suggesting."

https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934

That doesn't mean I acked the patch.

> ---
>  northd/northd.c | 18 +-
>  tests/ovn-northd.at | 12 
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index e9cb906e2..99fb30f46 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  }
>  }
>  
> +struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
> +struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
> +
>  for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>  struct ovn_nat *nat_entry = >od->nat_entries[i];
>  const struct nbrec_nat *nat = nat_entry->nb;
> @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  }
>  
>  if (!strcmp(nat->type, "snat")) {
> -continue;
> +if (nat_entry_is_v6(nat_entry)) {
> +if (sset_contains(_ips_v6, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_ips_v6, nat->external_ip);
> +} else {
> +if (sset_contains(_ips_v4, nat->external_ip)) {
> +continue;
> +}
> +sset_add(_ips_v4, nat->external_ip);
> +}

Essentially this just makes sure we don't skip NAT entries and that we
consider unique external_ips.  I'm fine with relaxing the second part of
the condition in which case, as mentioned on v1, I think we can just
remove the whole "if (!strcmp(nat->type, "snat")) {" block.

With the following incremental change applied (it removes the block)
your test still passes:

diff --git a/northd/northd.c b/northd/northd.c
index df7d2d60a5..20efd3b74c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 }
 }
 
-struct sset snat_ips_v4 = SSET_INITIALIZER(_ips_v4);
-struct sset snat_ips_v6 = SSET_INITIALIZER(_ips_v6);
-
 for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
 struct ovn_nat *nat_entry = >od->nat_entries[i];
 const struct nbrec_nat *nat = nat_entry->nb;
@@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 continue;
 }
 
-if (!strcmp(nat->type, "snat")) {
-if (nat_entry_is_v6(nat_entry)) {
-if (sset_contains(_ips_v6, nat->external_ip)) {
-continue;
-}
-sset_add(_ips_v6, nat->external_ip);
-} else {
-if (sset_contains(_ips_v4, nat->external_ip)) {
-continue;
-}
-sset_add(_ips_v4, nat->external_ip);
-}
-}
-
 /* Check if the ovn port has a network configured on which we could
  * expect ARP requests/NS for the DNAT external_ip.
  */
@@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
 if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
 build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
 }
-
-sset_destroy(_ips_v4);
-sset_destroy(_ips_v6);
 }
 
 static void
---

Best regards,
Dumitru

>  }
>  
>  /* Check if the ovn port has a network configured on which we could
> @@ -9025,6 +9038,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
>  if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
>  build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
>  }
> +
> +sset_destroy(_ips_v4);
> +sset_destroy(_ips_v6);
>  }
>  
>  static void
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5c9da811f..953e0d829 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed 
> 's/table=../table=??/' | sort],
>

Re: [ovs-dev] [PATCH ovn] ofctrl: Track the OvS flow update time

[Dumitru Ceara]

On 12/5/23 10:10, Ales Musil wrote:
> Add unixctl command called "ofctrl/flow-install-time"
> that returns the last time it took OvS to process
> and install all flows. The initial time is taken right
> before controller queues the updates to rconn.
> The end is marked when we receive barrier reply with
> corresponding xid.
> 
> Reported-at: https://issues.redhat.com/browse/FDP-134
> Signed-off-by: Ales Musil 
> ---
>  controller/ofctrl.c | 31 +--
>  1 file changed, 29 insertions(+), 2 deletions(-)

Hi Ales,

Thanks for the patch!

This is supposed to be a rather "stable" command as the CMS wants to
rely on this to gather relevant load statistics.  We should make sure we
document its behavior in ovn-controller.8.xml and mention it in the NEWS
file.

> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 7aac0128b..4f8b80012 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -51,6 +51,7 @@
>  #include "openvswitch/rconn.h"
>  #include "socket-util.h"
>  #include "timeval.h"
> +#include "unixctl.h"
>  #include "util.h"
>  #include "vswitch-idl.h"
>  
> @@ -323,6 +324,7 @@ struct ofctrl_flow_update {
>  struct ovs_list list_node;  /* In 'flow_updates'. */
>  ovs_be32 xid;   /* OpenFlow transaction ID for barrier. */
>  uint64_t req_cfg;   /* Requested sequence number. */
> +long long start_msec;   /* Timestamp when the update started. */
>  };
>  
>  static struct ofctrl_flow_update *
> @@ -402,6 +404,10 @@ static enum mf_field_id mff_ovn_geneve;
>   * (e.g. after OVS restart). */
>  static bool ofctrl_initial_clear;
>  
> +/* The time in ms it took for the last flow installation to be processed
> + * by OvS. */
> +static long long last_flow_install_time_ms = 0;
> +
>  static ovs_be32 queue_msg(struct ofpbuf *);
>  
>  static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
> @@ -413,6 +419,8 @@ static struct ofpbuf *encode_meter_mod(const struct 
> ofputil_meter_mod *);
>  static void ovn_installed_flow_table_clear(void);
>  static void ovn_installed_flow_table_destroy(void);
>  
> +static void flow_install_time_report(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *param);
>  
>  static void ofctrl_recv(const struct ofp_header *, enum ofptype);
>  
> @@ -429,6 +437,8 @@ ofctrl_init(struct ovn_extend_table *group_table,
>  groups = group_table;
>  meters = meter_table;
>  shash_init(_bands);
> +unixctl_command_register("ofctrl/flow-install-time", "", 0, 0,
> + flow_install_time_report, NULL);
>  }
>  
>  /* S_NEW, for a new connection.
> @@ -732,6 +742,9 @@ recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum 
> ofptype type,
>  if (fup->req_cfg >= cur_cfg) {
>  cur_cfg = fup->req_cfg;
>  }
> +
> +last_flow_install_time_ms = time_msec() - fup->start_msec;

Nit: very unlikely but this can underflow.

Nit2: too many empty lines IMO.  I'd just move this line immediately
before the "if (fup->req_cfg >= cur_cfg) {".

> +
>  mem_stats.oflow_update_usage -= ofctrl_flow_update_size(fup);
>  ovs_list_remove(>list_node);
>  free(fup);
> @@ -2900,6 +2913,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
>  const struct ofp_header *oh = barrier->data;
>  ovs_be32 xid_ = oh->xid;
>  ovs_list_push_back(, >list_node);
> +long long flow_update_start = time_msec();
>  
>  /* Queue the messages. */
>  struct ofpbuf *msg;
> @@ -2945,9 +2959,13 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
>  
>  /* Add a flow update. */
>  fup = xmalloc(sizeof *fup);
> +*fup = (struct ofctrl_flow_update) {
> +.xid = xid_,
> +.req_cfg = req_cfg,
> +.start_msec = flow_update_start,
> +};
> +
>  ovs_list_push_back(_updates, >list_node);
> -fup->xid = xid_;
> -fup->req_cfg = req_cfg;
>  mem_stats.oflow_update_usage += ofctrl_flow_update_size(fup);
>  done:;
>  } else if (!ovs_list_is_empty(_updates)) {
> @@ -3090,3 +3108,12 @@ ofctrl_get_memory_usage(struct simap *usage)
> ROUND_UP(rconn_packet_counter_n_bytes(tx_counter), 1024)
> / 1024);
>  }
> +
> +static void
> +flow_install_time_report(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *param 
> OVS_UNUSED)
> +{
> +char *msg = xasprintf("%lld ms", last_flow_install_time_ms);
> +unixctl_command_reply(conn, msg);
> +free(msg);
> +}

Thanks,
Dumitru


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [dpdk-latest] system-dpdk: Ignore net/ice error log about QinQ offloading.

[Eelco Chaudron]

On 5 Dec 2023, at 9:36, David Marchand wrote:

> The net/ice DPDK driver complains with an ERROR level log message if the
> hw firmware only supports SVM (Single Vlan Mode).
> DVM (Dual Vlan mode) seems required when using QinQ offloading.
> OVS does not care about this offloading feature and configures nothing
> on that topic.
>
> While seeing this error log, some manual tests show that
> untagged/tagged/"double" tagged packets (with 0x8100 ethertype)
> are still received/transmitted fine.
>
> Ignore this log waiting for a fix on the DPDK side.
>
> Link: https://bugs.dpdk.org/show_bug.cgi?id=1331
> Signed-off-by: David Marchand 

Thanks for getting to the bottom of this!

Acked-by: Eelco Chaudron 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] ofctrl: Track the OvS flow update time

[Ales Musil]

Add unixctl command called "ofctrl/flow-install-time"
that returns the last time it took OvS to process
and install all flows. The initial time is taken right
before controller queues the updates to rconn.
The end is marked when we receive barrier reply with
corresponding xid.

Reported-at: https://issues.redhat.com/browse/FDP-134
Signed-off-by: Ales Musil 
---
 controller/ofctrl.c | 31 +--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 7aac0128b..4f8b80012 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -51,6 +51,7 @@
 #include "openvswitch/rconn.h"
 #include "socket-util.h"
 #include "timeval.h"
+#include "unixctl.h"
 #include "util.h"
 #include "vswitch-idl.h"
 
@@ -323,6 +324,7 @@ struct ofctrl_flow_update {
 struct ovs_list list_node;  /* In 'flow_updates'. */
 ovs_be32 xid;   /* OpenFlow transaction ID for barrier. */
 uint64_t req_cfg;   /* Requested sequence number. */
+long long start_msec;   /* Timestamp when the update started. */
 };
 
 static struct ofctrl_flow_update *
@@ -402,6 +404,10 @@ static enum mf_field_id mff_ovn_geneve;
  * (e.g. after OVS restart). */
 static bool ofctrl_initial_clear;
 
+/* The time in ms it took for the last flow installation to be processed
+ * by OvS. */
+static long long last_flow_install_time_ms = 0;
+
 static ovs_be32 queue_msg(struct ofpbuf *);
 
 static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
@@ -413,6 +419,8 @@ static struct ofpbuf *encode_meter_mod(const struct 
ofputil_meter_mod *);
 static void ovn_installed_flow_table_clear(void);
 static void ovn_installed_flow_table_destroy(void);
 
+static void flow_install_time_report(struct unixctl_conn *conn, int argc,
+ const char *argv[], void *param);
 
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
 
@@ -429,6 +437,8 @@ ofctrl_init(struct ovn_extend_table *group_table,
 groups = group_table;
 meters = meter_table;
 shash_init(_bands);
+unixctl_command_register("ofctrl/flow-install-time", "", 0, 0,
+ flow_install_time_report, NULL);
 }
 
 /* S_NEW, for a new connection.
@@ -732,6 +742,9 @@ recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum 
ofptype type,
 if (fup->req_cfg >= cur_cfg) {
 cur_cfg = fup->req_cfg;
 }
+
+last_flow_install_time_ms = time_msec() - fup->start_msec;
+
 mem_stats.oflow_update_usage -= ofctrl_flow_update_size(fup);
 ovs_list_remove(>list_node);
 free(fup);
@@ -2900,6 +2913,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 const struct ofp_header *oh = barrier->data;
 ovs_be32 xid_ = oh->xid;
 ovs_list_push_back(, >list_node);
+long long flow_update_start = time_msec();
 
 /* Queue the messages. */
 struct ofpbuf *msg;
@@ -2945,9 +2959,13 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 
 /* Add a flow update. */
 fup = xmalloc(sizeof *fup);
+*fup = (struct ofctrl_flow_update) {
+.xid = xid_,
+.req_cfg = req_cfg,
+.start_msec = flow_update_start,
+};
+
 ovs_list_push_back(_updates, >list_node);
-fup->xid = xid_;
-fup->req_cfg = req_cfg;
 mem_stats.oflow_update_usage += ofctrl_flow_update_size(fup);
 done:;
 } else if (!ovs_list_is_empty(_updates)) {
@@ -3090,3 +3108,12 @@ ofctrl_get_memory_usage(struct simap *usage)
ROUND_UP(rconn_packet_counter_n_bytes(tx_counter), 1024)
/ 1024);
 }
+
+static void
+flow_install_time_report(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *param OVS_UNUSED)
+{
+char *msg = xasprintf("%lld ms", last_flow_install_time_ms);
+unixctl_command_reply(conn, msg);
+free(msg);
+}
-- 
2.43.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [dpdk-latest] system-dpdk: Ignore net/ice error log about QinQ offloading.

[David Marchand]

The net/ice DPDK driver complains with an ERROR level log message if the
hw firmware only supports SVM (Single Vlan Mode).
DVM (Dual Vlan mode) seems required when using QinQ offloading.
OVS does not care about this offloading feature and configures nothing
on that topic.

While seeing this error log, some manual tests show that
untagged/tagged/"double" tagged packets (with 0x8100 ethertype)
are still received/transmitted fine.

Ignore this log waiting for a fix on the DPDK side.

Link: https://bugs.dpdk.org/show_bug.cgi?id=1331
Signed-off-by: David Marchand 
---
 tests/system-dpdk-macros.at | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index dcdfa55741..c011487541 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -86,6 +86,7 @@ $1";/does not exist. The Open vSwitch kernel module is 
probably not loaded./d
 /does not support MTU configuration,/d
 /EAL: No \(available\|free\) .*hugepages reported/d
 /Failed to enable flow control/d
+/ice_vsi_config_outer_vlan_stripping(): Single VLAN mode (SVM) does not 
support qinq/d
 /Rx checksum offload is not supported on/d
 /TELEMETRY: No legacy callbacks, legacy socket not created/d"])
 ])
-- 
2.42.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev