Re: [ovs-dev] [PATCH v2 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

[Justin Pettit]

> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei  wrote:
> 
> This patch also adds missing symbols in the windows datapath so
> that the build on windows can pass.

Alin, can you do a quick sanity-check on the Windows part?  Thanks.

Yi-Hung, it looks good to me.  Just mostly some minor stuff below.

> diff --git a/datapath-windows/ovsext/Netlink/NetlinkProto.h 
> b/datapath-windows/ovsext/Netlink/NetlinkProto.h
> index 59b56565c1dc..db1fa2bacae8 100644
> --- a/datapath-windows/ovsext/Netlink/NetlinkProto.h
> +++ b/datapath-windows/ovsext/Netlink/NetlinkProto.h
> @@ -51,6 +51,7 @@
> #define NLM_F_ECHO  0x008
> 
> #define NLM_F_ROOT  0x100
> +#define NLM_F_REPLACE   0x100
> #define NLM_F_MATCH 0x200
> #define NLM_F_EXCL  0x200
> #define NLM_F_ATOMIC0x400

Below, I mention reorganizing the similar Linux header file based on request 
type.  I'd recommend that here, too.

> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 2f4906817946..8dacb1c7c253 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> ...
> +struct ct_dpif_timeout_policy {
> +uint32_tid; /* An unique identifier of a timeout policy in
> + * the datapath. */
> +uint32_tpresent;/* If a timeout attribute is present set the
> + * corresponding bit. */

This is a minor nit, but I think this would be clearer if it were less 
abstract.  For example:

uint32_tid; /* Unique identifier for the timeout policy in
 * the datapath. */
uint32_tpresent;/* If a timeout attribute is present set the
 * corresponding CT_DPIF_TP_ATTR_* mapping bit. */

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0a1c58adace..2079e368fb52 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7529,6 +7529,12 @@ const struct dpif_class dpif_netdev_class = {
> NULL,   /* ct_set_limits */
> NULL,   /* ct_get_limits */
> NULL,   /* ct_del_limits */
> +NULL,   /* ct_set_timeout_policy */
> +NULL,   /* ct_get_timeout_policy */
> +NULL,   /* ct_del_timeout_policy */
> +NULL,   /* ct_timeout_policy_dump_start */
> +NULL,   /* ct_timeout_policy_dump_next */
> +NULL,   /* ct_timeout_policy_dump_done */

Is there a plan to add timeout policies to the userspace datapath?  It's 
probably worth adding a line to "Documentation/faq/releases.rst".

> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 7bc71d6d19d7..b859508f718a 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> 
> +enum OVS_PACKED_ENUM dpif_netlink_support_timeout_policy_protocol {
> +DPIF_NL_TP_AF_INET_TCP,
> +DPIF_NL_TP_AF_INET_UDP,
> +DPIF_NL_TP_AF_INET_ICMP,
> +DPIF_NL_TP_AF_INET6_TCP,
> +DPIF_NL_TP_AF_INET6_UDP,
> +DPIF_NL_TP_AF_INET6_ICMPV6,
> +DPIF_NL_TP_MAX
> +};
> +
> +#define DPIF_NL_ALL_TP 0x3F

Not that it will be changing all that much, but I think it's always nice to 
auto-generate these sorts of values:

#define DPIF_NL_ALL_TP ((1UL << DPIF_NL_TP_MAX) - 1)

> +static int
> +dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
> +   const struct ct_dpif_timeout_policy *tp)
> +{
> ...
> +out:
> +ds_destroy(_tp_name);
> +return  err;

There's an extra space before 'err'.

> +struct dpif_netlink_tp_dump_node {
> +struct  hmap_node hmap_node;  /* node in tp_dump_map. */
> +struct  ct_dpif_timeout_policy *tp;
> +uint32_tpresent;

I think having this called 'present' is a little confusing since 'tp' has a 
different member called 'present'.  How about something like "l3_l4_present"?

> +static int
> +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
> + void *state,
> + struct ct_dpif_timeout_policy *tp)
> +{
> ...
> +struct nl_ct_timeout_policy nl_tp;
> +uint32_t tp_id;

These two variables could be declared in a tighter scope.

> +static int
> +dpif_netlink_ct_timeout_policy_dump_done(struct dpif *dpif OVS_UNUSED,
> + void *state)
> +{
> +struct dpif_netlink_ct_timeout_policy_dump_state *dump_state = state;
> +int err;
> +
> +err = nl_ct_timeout_policy_dump_done(dump_state->nl_dump_state);
> +hmap_destroy(_state->tp_dump_map);
> +free(dump_state);
> +return err;
> +}

If someone calls "_dump_done()" before they've called "_dump_next()" until it 
returned EOF, it seems like we could leak nodes in the dump state.  Should 
those be cleaned up?  If not, it should probably at least be documented in this 
function and the prototype description.  (I didn't 

Re: [ovs-dev] [PATCH 0/3] be able to tune revalidator timing

[Roi Dayan]

On 2019-07-22 8:10 PM, Ben Pfaff wrote:
> On Sun, Jul 21, 2019 at 11:34:20AM +0300, Roi Dayan wrote:
>> The following series expose configuration options to
>> be able to tune revalidator timing to different setups
>> at runtime instead of using hard coded values.
> 
> These seem fine.
> 
> I'd like to wait until we've branched for 2.12 before applying them.
> 

Hi Ben,

I see there is now a branch for 2.12.
can you take these commits now?

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

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

[Darrell Ball]

Thanks for the patch

I see the test is much improved now from V1 and passes - thanks

Ideally, tests should be associated with some code for context
It could be folded into patch 8


On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei  wrote:

> This patch adds a system traffic test to verify the zone-based conntrack
> timeout feature.  The test uses ovs-vsctl commands to configure
> the customized ICMP and UDP timeout on zone 5 to a shorter period.
> It then injects ICMP and UDP traffic to conntrack, and checks if the
> corresponding conntrack entry expires after the predefined timeout.
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  tests/system-kmod-macros.at  | 25 +++
>  tests/system-traffic.at  | 66
> 
>  tests/system-userspace-macros.at | 26 
>  3 files changed, 117 insertions(+)
>
> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
> index 554a61e9bd95..1bc6f246f426 100644
> --- a/tests/system-kmod-macros.at
> +++ b/tests/system-kmod-macros.at
> @@ -100,6 +100,15 @@ m4_define([CHECK_CONNTRACK_FRAG_OVERLAP],
>  #
>  m4_define([CHECK_CONNTRACK_NAT])
>
> +# CHECK_CONNTRACK_TIMEOUT()
> +#
> +# Perform requirements checks for running conntrack customized timeout
> tests.
> +#
> +m4_define([CHECK_CONNTRACK_TIMEOUT],
> +[
> +AT_SKIP_IF([! cat /boot/config-$(uname -r) | grep
> NF_CONNTRACK_TIMEOUT | grep '=y' > /dev/null])
> +])
> +
>  # CHECK_CT_DPIF_PER_ZONE_LIMIT()
>  #
>  # Perform requirements checks for running ovs-dpctl
> ct-[set|get|del]-limits per
> @@ -185,3 +194,19 @@ m4_define([OVS_CHECK_KERNEL_EXCL],
>  sublevel=$(uname -r | sed -e 's/\./ /g' | awk '{print $ 2}')
>  AT_SKIP_IF([ ! ( test $version -lt $1 || ( test $version -eq $1 &&
> test $sublevel -lt $2 ) || test $version -gt $3 || ( test $version -eq $3
> && test $sublevel -gt $4 ) ) ])
>  ])
> +
> +# VSCTL_ADD_DATAPATH_TABLE()
> +#
> +# Create system datapath table "system" for kernel tests in ovsdb
> +m4_define([VSCTL_ADD_DATAPATH_TABLE],
> +[
> +AT_CHECK([ovs-vsctl -- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"system"=@m], [0], [stdout])
> +])
> +
> +# VSCTL_ADD_ZONE_TIMEOUT_POLICY([parameters])
> +#
> +# Add zone based timeout policy to kernel datapath
> +m4_define([VSCTL_ADD_ZONE_TIMEOUT_POLICY],
> +[
> +AT_CHECK([ovs-vsctl add-zone-tp system $1], [0], [stdout])
> +])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 1a04199dcfe9..f4ac8a8f2c06 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -3179,6 +3179,72 @@ NXST_FLOW reply:
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([conntrack - zone-based timeout policy])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_TIMEOUT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_DATA([flows.txt], [dnl
> +priority=1,action=drop
> +priority=10,arp,action=normal
> +priority=100,in_port=1,ip,action=ct(zone=5, table=1)
> +priority=100,in_port=2,ip,action=ct(zone=5, table=1)
> +table=1,in_port=2,ip,ct_state=+trk+est,action=1
> +table=1,in_port=1,ip,ct_state=+trk+new,action=ct(commit,zone=5),2
> +table=1,in_port=1,ip,ct_state=+trk+est,action=2
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +dnl Test with default timeout
> +dnl The default udp_single and icmp_first timeouts are 30 seconds in
> +dnl kernel DP, and 60 seconds in userspace DP.
> +
> +dnl Send ICMP and UDP traffic
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 |
> FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
> actions=resubmit(,0)"])
> +
> +sleep 4
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sort],
> [0], [dnl
>
> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0),zone=5
>
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=5
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
> +dnl Shorten the udp_single and icmp_first timeout in zone 5
> +VSCTL_ADD_DATAPATH_TABLE()
> +VSCTL_ADD_ZONE_TIMEOUT_POLICY([zone=5 udp_single=3 icmp_first=3])
> +
> +dnl Send ICMP and UDP traffic
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 |
> FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
> actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sort],
> [0], [dnl
>
> 

Re: [ovs-dev] [PATCH v2 8/9] ofproto-dpif-xlate: Translate timeout policy in ct action

[Darrell Ball]

Thanks for the patch

The main comment I had from the V1 patch was adding the check

+if (ofc->flags & NX_CT_F_COMMIT) {

in compose_conntrack_action()

I see that was done.

After a quick scan, I had one minor comment inline.

On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei  wrote:

> This patch derives the timeout policy based on ct zone from the
> internal data structure that reads the configuration from ovsdb.
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  lib/ct-dpif.c| 10 ++
>  lib/ct-dpif.h|  3 +++
>  lib/dpif-netdev.c|  1 +
>  lib/dpif-netlink.c   | 10 ++
>  lib/dpif-provider.h  |  5 +
>  ofproto/ofproto-dpif-xlate.c | 29 +
>  ofproto/ofproto-dpif.c   | 24 
>  ofproto/ofproto-provider.h   |  5 +
>  ofproto/ofproto.c| 11 +++
>  ofproto/ofproto.h|  2 ++
>  10 files changed, 100 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 7f9ce0a561f7..5d2acfd7810b 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -864,3 +864,13 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif,
> void *state)
>  ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
>  : EOPNOTSUPP);
>  }
> +
> +int
> +ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
> +   uint16_t dl_type, uint8_t nw_proto,
> +   struct ds *ds)
> +{
> +return (dpif->dpif_class->ct_format_timeout_policy_name
> +? dpif->dpif_class->ct_format_timeout_policy_name(
> +dpif, tp_id, dl_type, nw_proto, ds) : EOPNOTSUPP);
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 8dacb1c7c253..0a27568880c0 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -318,5 +318,8 @@ int ct_dpif_timeout_policy_dump_start(struct dpif
> *dpif, void **statep);
>  int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
>   struct ct_dpif_timeout_policy *tp);
>  int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state);
> +int ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
> +   uint16_t dl_type, uint8_t nw_proto,
> +   struct ds *ds);
>
>  #endif /* CT_DPIF_H */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 7240a3e6f3c8..19cf9f21ec85 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7539,6 +7539,7 @@ const struct dpif_class dpif_netdev_class = {
>  NULL,   /* ct_timeout_policy_dump_start */
>  NULL,   /* ct_timeout_policy_dump_next */
>  NULL,   /* ct_timeout_policy_dump_done */
> +NULL,   /* ct_format_timeout_policy_name */
>  dpif_netdev_ipf_set_enabled,
>  dpif_netdev_ipf_set_min_frag,
>  dpif_netdev_ipf_set_max_nfrags,
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index b859508f718a..92da87027c58 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3071,6 +3071,15 @@ dpif_netlink_format_tp_name(uint32_t id, uint16_t
> l3num, uint8_t l4num,
>  ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX);
>  }
>
> +static int
> +dpif_netlink_ct_format_timeout_policy_name(struct dpif *dpif OVS_UNUSED,
> +uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *ds)
> +{
> +dpif_netlink_format_tp_name(tp_id,
> +dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, ds);
> +return 0;
> +}
> +
>  #define CT_DPIF_NL_TP_TCP_MAPPINGS  \
>  CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \
>  CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \
> @@ -3891,6 +3900,7 @@ const struct dpif_class dpif_netlink_class = {
>  dpif_netlink_ct_timeout_policy_dump_start,
>  dpif_netlink_ct_timeout_policy_dump_next,
>  dpif_netlink_ct_timeout_policy_dump_done,
> +dpif_netlink_ct_format_timeout_policy_name,
>  NULL,   /* ipf_set_enabled */
>  NULL,   /* ipf_set_min_frag */
>  NULL,   /* ipf_set_max_nfrags */
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 79a2314500cf..57b32ccb610f 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -536,6 +536,11 @@ struct dpif_class {
> struct ct_dpif_timeout_policy *tp);
>  int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
>
> +/* Get timeout policy name (OVS_CT_ATTR_TIMEOUT) from datapath. */
> +int (*ct_format_timeout_policy_name)(struct dpif *, uint32_t tp_id,
> + uint16_t dl_type, uint8_t
> nw_proto,
> + struct ds *ds);
> +
>  /* IP Fragmentation. */
>
>  /* Disables 

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

[Darrell Ball]

Thanks for the patch

comments inline

On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei  wrote:

> This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
> the zone-based timeout policy in the vswitchd. Whenever there is a
> database change, vswitchd will read the datapath, CT_Zone, and
> CT_Timeout_Policy tables from ovsdb to detect if the is any timeout
> policy changes.
>
> If a new timeout policy is added, it stores the information in
> per datapath type internal datapath structure in struct dpif-backer,
> and pushes down the conntrack timeout policy into the datapath via dpif
> interface.
>
> If a timeout policy is no longer used, vswitchd may not be able to
> remove it from datapath immediately since the datapath flow may still
> reference that. Instead, we keep an timeout policy kill list, that
> vswitchd will goes back to the list regularly and try to kill the
> unused timeout policies.
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  ofproto/ofproto-dpif.c | 266
> +
>  ofproto/ofproto-dpif.h |  10 ++
>  ofproto/ofproto-provider.h |   8 ++
>  ofproto/ofproto.c  |  20 
>  ofproto/ofproto.h  |   4 +
>  vswitchd/bridge.c  |  41 +++
>  6 files changed, 349 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..6336494e0bc8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -156,6 +156,25 @@ struct ofport_dpif {
>  size_t n_qdscp;
>  };
>
> +struct ct_timeout_policy {
> +struct uuid uuid;
> +unsigned int last_used_seqno;
> +struct ct_dpif_timeout_policy cdtp;
> +struct cmap_node node;  /* Element in struct dpif_backer's
> + * "ct_tps" cmap. */
>


This looks like a layering violation
should be in dpif-netlink or netlink-conntrack for kernel side


> +struct ovs_list list_node;  /* Element in struct dpif_backer's
> + * "ct_tp_kill_list" list. */





> +};
> +
> +struct ct_zone {
> +uint16_t id;
> +unsigned int last_used_seqno;
> +struct uuid tp_uuid;/* uuid that identifies a timeout
> policy in
> + * struct dpif_backer's "ct_tps"
> cmap. */
> +struct cmap_node node;  /* Element in struct dpif_backer's
> + * "ct_zones" cmap. */
> +};
> +
>  static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
> ofp_port_t);
>
> @@ -196,6 +215,8 @@ static struct hmap all_ofproto_dpifs_by_uuid =
>
>  static bool ofproto_use_tnl_push_pop = true;
>  static void ofproto_unixctl_init(void);
> +static void destroy_ct_zone_timeout_policy(struct dpif_backer *backer);
> +static void init_ct_zone_timeout_policy(struct dpif_backer *backer);
>
>  static inline struct ofproto_dpif *
>  ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -683,6 +704,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
>  }
>  dpif_close(backer->dpif);
>  id_pool_destroy(backer->meter_ids);
> +destroy_ct_zone_timeout_policy(backer);
>  free(backer);
>  }
>
> @@ -694,6 +716,8 @@ struct odp_garbage {
>
>  static void check_support(struct dpif_backer *backer);
>
> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
> +
>  static int
>  open_dpif_backer(const char *type, struct dpif_backer **backerp)
>  {
> @@ -811,6 +835,8 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>  backer->meter_ids = NULL;
>  }
>
> +init_ct_zone_timeout_policy(backer);
> +
>  /* Make a pristine snapshot of 'support' into 'boottime_support'.
>   * 'boottime_support' can be checked to prevent 'support' to be
> changed
>   * beyond the datapath capabilities. In case 'support' is changed by
> @@ -5086,6 +5112,244 @@ ct_flush(const struct ofproto *ofproto_, const
> uint16_t *zone)
>  ct_dpif_flush(ofproto->backer->dpif, zone, NULL);
>  }
>
> +static struct ct_zone *
> +ct_zone_lookup(struct cmap *ct_zones, uint16_t zone_id)
> +{
> +struct ct_zone *zone;
> +
> +CMAP_FOR_EACH_WITH_HASH (zone, node, hash_int(zone_id, 0), ct_zones) {
> +if (zone->id == zone_id) {
> +return zone;
> +}
> +}
> +return NULL;
> +}
> +
> +static struct ct_zone *
> +ct_zone_alloc(uint16_t zone_id)
> +{
> +struct ct_zone *zone;
> +
> +zone = xzalloc(sizeof *zone);
> +zone->id = zone_id;
> +
> +return zone;
> +}
> +
> +static void
> +ct_zone_remove_and_destroy(struct dpif_backer *backer, struct ct_zone
> *zone)
> +{
> +cmap_remove(>ct_zones, >node, hash_int(zone->id, 0));
> +ovsrcu_postpone(free, zone);
> +}
> +
> +static struct ct_timeout_policy *
> +ct_timeout_policy_lookup(struct cmap *ct_tps, struct uuid *uuid)
> +{
> +struct ct_timeout_policy *tp;
> +
> +CMAP_FOR_EACH_WITH_HASH (tp, node, uuid_hash(uuid), ct_tps) {
> +if 

Re: [ovs-dev] [PATCH v2 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

[Darrell Ball]

Thanks for the patch

I am going to avoid commenting on style or code conciseness in the
interests of time

On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei  wrote:

> This patch first defines the dpif interface for a datapath to support
> adding, deleting, getting and dumping conntrack timeout policy.
> The timeout policy is identified by a 4 bytes unsigned integer in
> datapath, and it currently support timeout for TCP, UDP, and ICMP
> protocols.
>

Using a 4 integer to identify a timeout policy is needed to address a Linux
restriction.
However, in general, policies can be identified by generic string or UUIDs.
We add that flexibility later, however.


>
> Moreover, this patch provides the implemetation for Linux kernel
> datapath in dpif-netlink.
>
> In Linux kernel, the timeout policy is maintained per L3/L4 protocol,
> and it is identified by 32 bytes null terminated string.  On the other
> hand, in vswitchd, the timeout policy is a generic one that consists of
> all the supported L4 protocols.  Therefore, one of the main task in
> dpif-netlink is to break down the generic timeout policy into 6
> sub policies (ipv4 tcp, udp, icmp, and ipv6 tcp, udp, icmp),
> and push down the configuration using the netlink API in
> netlink-conntrack.c.
>
> This patch also adds missing symbols in the windows datapath so
> that the build on windows can pass.
>
> Appveyor CI:
> * https://ci.appveyor.com/project/YiHungWei/ovs/builds/26387754
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath-windows/include/OvsDpInterfaceCtExt.h | 114 +
>  datapath-windows/ovsext/Netlink/NetlinkProto.h |   1 +
>  include/windows/automake.mk|   1 +
>  .../windows/linux/netfilter/nfnetlink_cttimeout.h  |   0
>  lib/ct-dpif.c  | 104 +
>  lib/ct-dpif.h  |  56 +++
>  lib/dpif-netdev.c  |   6 +
>  lib/dpif-netlink.c | 462
> +
>  lib/dpif-netlink.h |   1 +
>  lib/dpif-provider.h|  38 ++
>  lib/netlink-conntrack.c| 363 
>  lib/netlink-conntrack.h|  29 ++
>  lib/netlink-protocol.h |   1 +
>  13 files changed, 1176 insertions(+)
>  create mode 100644 include/windows/linux/netfilter/nfnetlink_cttimeout.h
>
> diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h
> b/datapath-windows/include/OvsDpInterfaceCtExt.h
> index 3b947782e90c..4379855bb8dd 100644
> --- a/datapath-windows/include/OvsDpInterfaceCtExt.h
> +++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
> @@ -421,4 +421,118 @@ struct nf_ct_tcp_flags {
>  UINT8 mask;
>  };
>
> +/* File: nfnetlink_cttimeout.h */
> +enum ctnl_timeout_msg_types {
> +IPCTNL_MSG_TIMEOUT_NEW,
> +IPCTNL_MSG_TIMEOUT_GET,
> +IPCTNL_MSG_TIMEOUT_DELETE,
> +IPCTNL_MSG_TIMEOUT_DEFAULT_SET,
> +IPCTNL_MSG_TIMEOUT_DEFAULT_GET,
> +
> +IPCTNL_MSG_TIMEOUT_MAX
> +};
> +
> +enum ctattr_timeout {
> +CTA_TIMEOUT_UNSPEC,
> +CTA_TIMEOUT_NAME,
> +CTA_TIMEOUT_L3PROTO,
> +CTA_TIMEOUT_L4PROTO,
> +CTA_TIMEOUT_DATA,
> +CTA_TIMEOUT_USE,
> +__CTA_TIMEOUT_MAX
> +};
> +#define CTA_TIMEOUT_MAX (__CTA_TIMEOUT_MAX - 1)
> +
> +enum ctattr_timeout_generic {
> +CTA_TIMEOUT_GENERIC_UNSPEC,
> +CTA_TIMEOUT_GENERIC_TIMEOUT,
> +__CTA_TIMEOUT_GENERIC_MAX
> +};
> +#define CTA_TIMEOUT_GENERIC_MAX (__CTA_TIMEOUT_GENERIC_MAX - 1)
> +
> +enum ctattr_timeout_tcp {
> +CTA_TIMEOUT_TCP_UNSPEC,
> +CTA_TIMEOUT_TCP_SYN_SENT,
> +CTA_TIMEOUT_TCP_SYN_RECV,
> +CTA_TIMEOUT_TCP_ESTABLISHED,
> +CTA_TIMEOUT_TCP_FIN_WAIT,
> +CTA_TIMEOUT_TCP_CLOSE_WAIT,
> +CTA_TIMEOUT_TCP_LAST_ACK,
> +CTA_TIMEOUT_TCP_TIME_WAIT,
> +CTA_TIMEOUT_TCP_CLOSE,
> +CTA_TIMEOUT_TCP_SYN_SENT2,
> +CTA_TIMEOUT_TCP_RETRANS,
> +CTA_TIMEOUT_TCP_UNACK,
> +__CTA_TIMEOUT_TCP_MAX
> +};
> +#define CTA_TIMEOUT_TCP_MAX (__CTA_TIMEOUT_TCP_MAX - 1)
> +
> +enum ctattr_timeout_udp {
> +CTA_TIMEOUT_UDP_UNSPEC,
> +CTA_TIMEOUT_UDP_UNREPLIED,
> +CTA_TIMEOUT_UDP_REPLIED,
> +__CTA_TIMEOUT_UDP_MAX
> +};
> +#define CTA_TIMEOUT_UDP_MAX (__CTA_TIMEOUT_UDP_MAX - 1)
> +
> +enum ctattr_timeout_udplite {
> +CTA_TIMEOUT_UDPLITE_UNSPEC,
> +CTA_TIMEOUT_UDPLITE_UNREPLIED,
> +CTA_TIMEOUT_UDPLITE_REPLIED,
> +__CTA_TIMEOUT_UDPLITE_MAX
> +};
> +#define CTA_TIMEOUT_UDPLITE_MAX (__CTA_TIMEOUT_UDPLITE_MAX - 1)
> +
> +enum ctattr_timeout_icmp {
> +CTA_TIMEOUT_ICMP_UNSPEC,
> +CTA_TIMEOUT_ICMP_TIMEOUT,
> +__CTA_TIMEOUT_ICMP_MAX
> +};
> +#define CTA_TIMEOUT_ICMP_MAX (__CTA_TIMEOUT_ICMP_MAX - 1)
> +
> +enum ctattr_timeout_dccp {
> +CTA_TIMEOUT_DCCP_UNSPEC,
> +CTA_TIMEOUT_DCCP_REQUEST,
> +CTA_TIMEOUT_DCCP_RESPOND,
> +CTA_TIMEOUT_DCCP_PARTOPEN,
> +CTA_TIMEOUT_DCCP_OPEN,
> +CTA_TIMEOUT_DCCP_CLOSEREQ,
> +

Re: [ovs-dev] [PATCH v3 2/4 ovn] OVN: Vlan backed DVR N-S, redirect-type option

[Ankur Sharma]

Hi Numan,

Thanks a lot for the feedback.

I was considering ovn-nbctl as a tool for playing with the feature and CMS 
should not be relying on it.
However, it is better not to override rest of the options. I will address this 
in V4.

I will wait for your comments on other patches in the series, will submit v4 
with addressed comments on all of them.

Thanks

Regards,
Ankur

From: Numan Siddique 
Sent: Monday, August 5, 2019 9:24 AM
To: Ankur Sharma 
Cc: ovs-dev@openvswitch.org 
Subject: Re: [ovs-dev] [PATCH v3 2/4 ovn] OVN: Vlan backed DVR N-S, 
redirect-type option




On Fri, Aug 2, 2019 at 5:24 AM Ankur Sharma 
mailto:ankur.sha...@nutanix.com>> wrote:
Background:
With c0974331b7a19a87ab8f1f2cec8fbe366af92fa2, we have added
support for E-W workflow for vlan backed DVRs.

This series enables N-S workflow for vlan backed DVRs.

Key difference between E-W and N-S traffic flow is that
N-S flow requires a gateway chassis. A gateway chassis
will be respondible for following:
a. Doing Network Address Translation (NAT).
b. Becoming entry and exit point for North->South
   and South->North traffic respectively.

OVN by default always uses overlay encapsulation to redirect
the packet to gateway chassis. This series will enable
the redirection to gateway chassis in the absence of encapsulation.

This patch:
a. Add a new key-value in options of a router port.
b. This new config key will be used by ovn-controller
   to determine if a redirected packet will go out of
   tunnel port or localnet port.
c. key is "redirect-type" and it takes "overlay" and
   "vlan" as values.
d. Added ovn-nbctl command to set and get redirect-type
   option on a router port.
e. This new configuration is added because vlan or overlay
   based forwarding is considered to be a logical switch property,
   hence for a router configuration has to be done at the router port
   level.

Signed-off-by: Ankur Sharma 
mailto:ankur.sha...@nutanix.com>>
---
 northd/ovn-northd.c   |  6 ++
 ovn-nb.xml| 43 ++
 tests/ovn-nbctl.at 
[ovn-nbctl.at]
| 25 ++
 tests/ovn-northd.at 
[ovn-northd.at]
   | 31 +++
 utilities/ovn-nbctl.c | 58 +++
 5 files changed, 163 insertions(+)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index cd776fa..7c0fd6c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -2445,6 +2445,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 if (op->derived) {
 const char *redirect_chassis = smap_get(>nbrp->options,
 "redirect-chassis");
+const char *redirect_type = smap_get(>nbrp->options,
+ "redirect-type");
+
 int n_gw_options_set = 0;
 if (op->nbrp->ha_chassis_group) {
 n_gw_options_set++;
@@ -2537,6 +2540,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
 sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
 }
 smap_add(, "distributed-port", op->nbrp->name);
+if (redirect_type) {
+smap_add(, "redirect-type", redirect_type);
+}
 } else {
 if (op->peer) {
 smap_add(, "peer", op->peer->key);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index f5f10a5..971017b 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1947,6 +1947,49 @@
   issues.
 
   
+
+  
+
+  This options dictates if a packet redirected to
+  gateway chassis will be overlay encapsulated
+  or go as a regular vlan packet.
+
+
+
+  Option takes following values
+
+
+
+  
+OVERLAY
+  
+
+  
+VLAN
+  
+
+
+
+  OVERLAY option will ensure that redirected packet goes out as
+  encapsulation via the tunnel port.
+
+
+
+  VLAN option will ensure that redirected packet goes out as vlan
+  tagged via the localnet port.
+
+
+
+  OVERLAY is the default redirection type.
+
+
+
+  Option is applicable only to gateway chassis attached logical
+  router ports.
+
+
+  
+
 

 
diff --git a/tests/ovn-nbctl.at 

Re: [ovs-dev] [PATCH v3 1/4 ovn] OVN: Do not replace router port mac on gateway chassis.

[Ankur Sharma]

Hi Numan,

Sure, i will address the comments in V4.

Regards,
Ankur

From: Numan Siddique 
Sent: Monday, August 5, 2019 9:03 AM
To: Ankur Sharma 
Cc: ovs-dev@openvswitch.org 
Subject: Re: [ovs-dev] [PATCH v3 1/4 ovn] OVN: Do not replace router port mac 
on gateway chassis.


The patch LGTM. Few comments inline.

Please see below

Thanks
Numan

On Fri, Aug 2, 2019 at 5:23 AM Ankur Sharma 
mailto:ankur.sha...@nutanix.com>> wrote:
With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
support for E-W routing on vlan backed networks by replacing
router port macs with chassis macs.

This replacement of router port mac need NOT be done on
gateway chassis for following reasons:

a. For N-S traffic, gateway chassis will respond to ARP
for the router port (to which it is attached) and
traffic will be using router port mac as destination mac.

b. Chassis redirect port is a centralized version of distributed
router port, hence we need not replace its mac with chassis mac
on the resident chassis.

This patch addresses the same.

Signed-off-by: Ankur Sharma 
mailto:ankur.sha...@nutanix.com>>
---
 controller/physical.c |  19 ++-
 controller/pinctrl.c  |  37 +++---
 controller/pinctrl.h  |   5 +
 tests/ovn.at 
[ovn.at]
  | 313 ++
 4 files changed, 354 insertions(+), 20 deletions(-)

diff --git a/controller/physical.c b/controller/physical.c
index 5c2f74e..aa06b3f 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -38,6 +38,7 @@
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
 #include "physical.h"
+#include "pinctrl.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
 #include "smap.h"
@@ -228,9 +229,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
 }

 static void
-put_replace_router_port_mac_flows(const struct
+put_replace_router_port_mac_flows(struct ovsdb_idl_index
+  *sbrec_port_binding_by_name,
+  const struct
   sbrec_port_binding *localnet_port,
   const struct sbrec_chassis *chassis,
+  const struct sset *active_tunnels,
   const struct hmap *local_datapaths,
   struct ofpbuf *ofpacts_p,
   ofp_port_t ofport,
@@ -270,6 +274,16 @@ put_replace_router_port_mac_flows(const struct
 struct eth_addr router_port_mac;
 struct match match;
 struct ofpact_mac *replace_mac;
+char *cr_peer_name = xasprintf("cr-%s", rport_binding->logical_port);
+if (pinctrl_is_chassis_resident(sbrec_port_binding_by_name,
+chassis, active_tunnels,
+cr_peer_name)) {
+/* If a router port's chassisredirect port is
+ * resident on this chassis, then we need not do mac replace. */
+free(cr_peer_name);
+continue;
+}
+free(cr_peer_name);

 /* Table 65, priority 150.
  * ===
@@ -787,7 +801,8 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 , ofpacts_p, >header_.uuid);

 if (!strcmp(binding->type, "localnet")) {
-put_replace_router_port_mac_flows(binding, chassis,
+put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
+  binding, chassis, active_tunnels,
   local_datapaths, ofpacts_p,
   ofport, flow_table);
 }
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579f..d0b2e27 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -455,6 +455,25 @@ pinctrl_init(void)
 );
 }

+bool
+pinctrl_is_chassis_resident(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+const struct sbrec_chassis *chassis,
+const struct sset *active_tunnels,
+const char *port_name)
+{
+const struct sbrec_port_binding *pb
+= lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
+if (!pb || !pb->chassis) {
+return false;
+}
+if (strcmp(pb->type, "chassisredirect")) {
+return pb->chassis == chassis;
+} else {
+return ha_chassis_group_is_active(pb->ha_chassis_group,
+  active_tunnels, chassis);
+}
+}
+
 static ovs_be32
 queue_msg(struct rconn *swconn, struct ofpbuf *msg)
 {
@@ 

Re: [ovs-dev] [PATCH v2 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

[Darrell Ball]

One comment fix:

s/ "min": 0, "max": "65535"}},/ "min": 0, "max": "65536"}},/

On Mon, Aug 5, 2019 at 4:09 PM Darrell Ball  wrote:

> Thanks for the patch
>
> I avoided duplicate comments from what Justin suggested
>
> comments inline
>
> On Thu, Aug 1, 2019 at 3:08 PM Yi-Hung Wei  wrote:
>
>> From: Justin Pettit 
>>
>> From: Justin Pettit 
>>
>> Signed-off-by: Justin Pettit 
>> ---
>>  vswitchd/vswitch.ovsschema |  43 +++-
>>  vswitchd/vswitch.xml   | 252
>> -
>>  2 files changed, 246 insertions(+), 49 deletions(-)
>>
>> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
>> index f7c6eb8983cd..d215f4edfefa 100644
>> --- a/vswitchd/vswitch.ovsschema
>> +++ b/vswitchd/vswitch.ovsschema
>> @@ -1,9 +1,14 @@
>>  {"name": "Open_vSwitch",
>> - "version": "8.0.0",
>> - "cksum": "3962141869 23978",
>> + "version": "8.1.0",
>> + "cksum": "1566974404 25483",
>>   "tables": {
>> "Open_vSwitch": {
>>   "columns": {
>> +   "datapaths": {
>> + "type": {"key": {"type": "string"},
>>
>
> Should 'type' be an enum
> something like:
>
>  "type": {"key": {"type": "string",
>   "enum": ["set", ["system", "netdev"]]}},
>
> The schema can still be upgraded by adding new datapath types should more
> ever arise.
>
>
>
>> +  "value": {"type": "uuid",
>> +"refTable": "Datapath"},
>> +  "min": 0, "max": "unlimited"}},
>>
>
> accordingly:
>
> "min": 0, "max": "2"}},
>
>
>
>> "bridges": {
>>   "type": {"key": {"type": "uuid",
>>"refTable": "Bridge"},
>> @@ -629,6 +634,40 @@
>>"min": 0, "max": "unlimited"},
>>   "ephemeral": true}},
>>   "indexes": [["target"]]},
>> +   "Datapath": {
>> + "columns": {
>> +   "datapath_version": {
>> + "type": "string"},
>> +   "ct_zones": {
>> + "type": {"key": {"type": "integer",
>> +  "minInteger": 0,
>> +  "maxInteger": 65535},
>> +  "value": {"type": "uuid",
>> +"refTable": "CT_Zone"},
>> +  "min": 0, "max": "unlimited"}},
>>
>
>
> How about ?
>
>  "min": 0, "max": "65535"}},
>

s/ "min": 0, "max": "65535"}},/ "min": 0, "max": "65536"}},/


>
> I don't think we can have multiple entries for the same zone and if we
> did, we don't
> handle it.
>
>
>
>> +   "external_ids": {
>> + "type": {"key": "string", "value": "string",
>> +  "min": 0, "max": "unlimited",
>> +   "CT_Zone": {
>> + "columns": {
>> +   "timeout_policy": {
>> + "type": {"key": {"type": "uuid",
>> +  "refTable": "CT_Timeout_Policy"},
>> +  "min": 0, "max": 1}},
>> +   "external_ids": {
>> + "type": {"key": "string", "value": "string",
>> +  "min": 0, "max": "unlimited",
>> +   "CT_Timeout_Policy": {
>> + "columns": {
>> +   "timeouts": {
>> + "type": {"key": "string",
>> +  "value": {"type" : "integer",
>> +"minInteger" : 0,
>> +"maxInteger" : 4294967295},
>> +  "min": 0, "max": "unlimited"}},
>> +   "external_ids": {
>> + "type": {"key": "string", "value": "string",
>> +  "min": 0, "max": "unlimited",
>> "SSL": {
>>   "columns": {
>> "private_key": {
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 027aee2f523b..a0706c9c0fc1 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -52,6 +52,13 @@
>>  one record in the  table.
>>
>>  
>> +  
>> +Map of datapath types to datapaths.  The
>> + column of the 
>> +table is used as a key for this map.  The value points to a row
>> in
>> +the  table.
>> +  
>> +
>>
>>  Set of bridges managed by the daemon.
>>
>> @@ -1192,53 +1199,11 @@
>>
>>
>>
>> -
>> -  Reports the version number of the Open vSwitch datapath in use.
>> -  This allows management software to detect and report
>> discrepancies
>> -  between Open vSwitch userspace and datapath versions.  (The
>> > -  column="ovs_version" table="Open_vSwitch"/> column in the > -  table="Open_vSwitch"/> reports the Open vSwitch userspace
>> version.)
>> -  The version reported depends on the datapath in use:
>> -
>> -
>> -
>> -  
>> -When the kernel module included in the Open vSwitch source
>> tree is
>> -used, this column reports the Open vSwitch version from
>> which the
>> -module was taken.
>> -  
>> -
>> -  
>> -When the kernel module that is part of the upstream Linux
>> kernel is
>> -used, this column reports 

Re: [ovs-dev] [PATCH v2 2/9] ovs-vsctl: Add conntrack zone commands.

[Darrell Ball]

Thanks for the patch

I noticed '--may-exist' and '--if-exists' are supported now for
add--zone-tp/del-zone-tp - thanks
The check for duplicate timeout policies now correctly checks all key and
values - thanks

Some more comments inline
I am trying to avoid duplicate comment from Justin, so I just won't comment
on some parts in this version
to avoid confusion.


On Thu, Aug 1, 2019 at 3:09 PM Yi-Hung Wei  wrote:

> From: William Tu 
>
> The patch adds commands creating/deleting/listing conntrack zone
> timeout policies:
>   $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ...
>
> Signed-off-by: William Tu 
> ---
>  tests/ovs-vsctl.at   |  34 +++-
>  utilities/ovs-vsctl.8.in |  25 ++
>  utilities/ovs-vsctl.c| 204
> +++
>  3 files changed, 261 insertions(+), 2 deletions(-)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 46fa3c5b1a33..f0c5975edd0e 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -805,6 +805,20 @@ AT_CHECK(
>[RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
> "'])])
>  AT_CHECK(
>[RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
>

What happens if we add a datapath type here and there are no bridges of
that type; meaning the datapath of that type does not even exist.
Seems like a contradiction.
Maybe we should check for that at least and raise an error.
Ideally, it is better if these 'datapaths' are auto-managed by bridge
creation/deletion with given datapath types,
but we can certainly defer that.


> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> icmp_reply=2])])
>

I mentioned this in V1:
There is no filtering of bad timeout key; for example

AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
foo_bar=2])])

is accepted as valid

Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'.

AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
icmp_repl=2])])


> +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1
> icmp_first=1 icmp_reply=2])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> Policies: icmp_first=1 icmp_reply=2
>

I mentioned in V1
We should check all possible timeout keys to make sure they work.


> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> Policies: icmp_first=1 icmp_reply=2
> +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
> +])
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
>
> @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
> flood_vlans=-1])],
>  AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
>[1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
> range 0 to 4095 (inclusive)
>  ])
> -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
>

I mentioned in V1.
I don't think we should make unrelated changes in a feature patch
especially since it seems the author wanted to convey
short form syntax is valid


>[1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
> allowed values ([in-band, out-of-band])
>  ]])
> -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
> +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
>[1], [], [ovs-vsctl: cannot specify key to set for non-map column
> connection_mode
>  ])
>  AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
> @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1
> flood-vlans true])],
>  AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
>[1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge
>  ])
> +
> +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> icmp_reply=2])],
> +  [1], [], [ovs-vsctl: datapath: netdevxx record not found
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply=3])],
> +  [1], [], [ovs-vsctl: zone id 2 alread exists
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> Policies: icmp_first=2 icmp_reply=3
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
> +  [1], [], [ovs-vsctl: zone id 11 not exists.
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], 

Re: [ovs-dev] [PATCH v2 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

[Darrell Ball]

Thanks for the patch

I avoided duplicate comments from what Justin suggested

comments inline

On Thu, Aug 1, 2019 at 3:08 PM Yi-Hung Wei  wrote:

> From: Justin Pettit 
>
> From: Justin Pettit 
>
> Signed-off-by: Justin Pettit 
> ---
>  vswitchd/vswitch.ovsschema |  43 +++-
>  vswitchd/vswitch.xml   | 252
> -
>  2 files changed, 246 insertions(+), 49 deletions(-)
>
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index f7c6eb8983cd..d215f4edfefa 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,9 +1,14 @@
>  {"name": "Open_vSwitch",
> - "version": "8.0.0",
> - "cksum": "3962141869 23978",
> + "version": "8.1.0",
> + "cksum": "1566974404 25483",
>   "tables": {
> "Open_vSwitch": {
>   "columns": {
> +   "datapaths": {
> + "type": {"key": {"type": "string"},
>

Should 'type' be an enum
something like:

 "type": {"key": {"type": "string",
  "enum": ["set", ["system", "netdev"]]}},

The schema can still be upgraded by adding new datapath types should more
ever arise.



> +  "value": {"type": "uuid",
> +"refTable": "Datapath"},
> +  "min": 0, "max": "unlimited"}},
>

accordingly:

"min": 0, "max": "2"}},



> "bridges": {
>   "type": {"key": {"type": "uuid",
>"refTable": "Bridge"},
> @@ -629,6 +634,40 @@
>"min": 0, "max": "unlimited"},
>   "ephemeral": true}},
>   "indexes": [["target"]]},
> +   "Datapath": {
> + "columns": {
> +   "datapath_version": {
> + "type": "string"},
> +   "ct_zones": {
> + "type": {"key": {"type": "integer",
> +  "minInteger": 0,
> +  "maxInteger": 65535},
> +  "value": {"type": "uuid",
> +"refTable": "CT_Zone"},
> +  "min": 0, "max": "unlimited"}},
>


How about ?

 "min": 0, "max": "65535"}},

I don't think we can have multiple entries for the same zone and if we did,
we don't
handle it.



> +   "external_ids": {
> + "type": {"key": "string", "value": "string",
> +  "min": 0, "max": "unlimited",
> +   "CT_Zone": {
> + "columns": {
> +   "timeout_policy": {
> + "type": {"key": {"type": "uuid",
> +  "refTable": "CT_Timeout_Policy"},
> +  "min": 0, "max": 1}},
> +   "external_ids": {
> + "type": {"key": "string", "value": "string",
> +  "min": 0, "max": "unlimited",
> +   "CT_Timeout_Policy": {
> + "columns": {
> +   "timeouts": {
> + "type": {"key": "string",
> +  "value": {"type" : "integer",
> +"minInteger" : 0,
> +"maxInteger" : 4294967295},
> +  "min": 0, "max": "unlimited"}},
> +   "external_ids": {
> + "type": {"key": "string", "value": "string",
> +  "min": 0, "max": "unlimited",
> "SSL": {
>   "columns": {
> "private_key": {
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 027aee2f523b..a0706c9c0fc1 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -52,6 +52,13 @@
>  one record in the  table.
>
>  
> +  
> +Map of datapath types to datapaths.  The
> + column of the 
> +table is used as a key for this map.  The value points to a row in
> +the  table.
> +  
> +
>
>  Set of bridges managed by the daemon.
>
> @@ -1192,53 +1199,11 @@
>
>
>
> -
> -  Reports the version number of the Open vSwitch datapath in use.
> -  This allows management software to detect and report
> discrepancies
> -  between Open vSwitch userspace and datapath versions.  (The  -  column="ovs_version" table="Open_vSwitch"/> column in the  -  table="Open_vSwitch"/> reports the Open vSwitch userspace
> version.)
> -  The version reported depends on the datapath in use:
> -
> -
> -
> -  
> -When the kernel module included in the Open vSwitch source
> tree is
> -used, this column reports the Open vSwitch version from which
> the
> -module was taken.
> -  
> -
> -  
> -When the kernel module that is part of the upstream Linux
> kernel is
> -used, this column reports unknown.
> -  
> -
> -  
> -When the datapath is built into the ovs-vswitchd
> -binary, this column reports built-in.  A
> -built-in datapath is by definition the same version as the
> rest of
> -the Open VSwitch userspace.
> -  
> -
> -  
> -Other datapaths (such as the Hyper-V kernel datapath)
> 

[ovs-dev] Старинные Русские Романсы. Арии из опер и оперетт. Лирические, неаполитанские, народные, деревенские, городские - песни и баллады. 13_05_2019 02_03 198707

[Роман Колунов via dev]

ВЕЛИКИЕ РОССИЙСКИЕ ИСПОЛНИТЕЛИ 20 ВЕКА
Старинные Русские и Цыганские Романсы. Арии из опер и оперетт. Лирические песни.
Неаполитанские песни. Народные, деревенские, городские - песни и баллады. Песни 
военных лет.

Предлагаем Вашему вниманию, пожалуй, самую большую и уникальную коллекцию: 
старинных русских и цыганских романсов, арий из опер и оперетт, лирических и 
неаполитанских песен, народных, деревенских, городских песен и баллад, песен 
военных лет, а также песен русских и советских композиторов. Была проделана 
огромная работа, в результате которой удалось нам удалось собрать практически 
всех исполнителей. Больших трудов стоило собрать коллекцию, поскольку 
большинство произведений давно не переиздавались. Русский романс – это песни, 
которые затрагивают что-то в нашей душе, это чувства, которые положены на 
музыку, это поэзия, которая заставляет плакать и смеяться. Русские романсы 
трудно с чем-либо сравнивать, они стоят обособлено, среди прочих песен и 
музыки. 

Очень жаль, что русские романсы в современное время потеряли свое место в 
сердцах людей, которые перестали задумываться о таких вечных ценностях, как 
любовь, вера, надежда. Вечная спешка современного мира не может вместить в себя 
спокойную нежность и страстность романса. Мы предлагаем Вам окунутся в это 
уникальное явление в русской музыке, которое объединяет людей, наполняет их умы 
и души добротой, красотой и любовью. Сложно найти более красивые, душевные и 
трогательные песни, которые бы находили отклик в сердцах самых разных людей. 
Наша коллекция удовлетворяет вкус самых разных слушателей, романсы подходят для 
романтического вечера, праздничного обеда, прослушивания в машине, для 
уединенных раздумий, а также послужит хорошим подарком близким и дорогим людям.

В нашу коллекцию вошли следующие исполнители: Агния Лазовская\Аида 
Ведищева\Александр Ведерников\Александр Вертинский\Александр Огнивцев\Александр 
Пирогов\Александр Розум\Алексей Большаков\Алексей Мартынов\Алексей 
Покровский\Алла Баянова\Алла Соленкова\Анастасия Вяльцева\Анатолий 
Орфёнов\Анатолий Соловьяненко\Андрей Славный\Анегина Ильина\Анна 
Литвиненко\Анна Нетребко\Антонина Нежданова\Артур Эйзен\Борис Дейнека\Борис 
Рубашкин\Борис Христов\Борис Штоколов\Бронислава Златогорова\Бэла Руденко\Вадим 
Козин\Вадим Мулерман\Валентина Ивантеева\Валентина Левко\Валентина 
Пономарёва\Валентина Толкунова\Валентин Баглаенко\Валерий Агафонов\Валерий 
Лебедь\Валерий Ободзинский\Валерия Барсова\Вера Баева\Вера Давыдова\Вера 
Кудрявцева\Вероника Борисенко\Вивея Громова\Виктория Иванова\Владимир 
Атлантов\Владимир Бунчиков\Владимир Девятов\Владимир Касторский\Владимир 
Нечаев\Владимир Панкратов\Владимир Политковский\Владимир Розинг\Владимир 
Трошин\Галина Баранова\Галина Вишневская\Галина Ковалёва\Галина 
Писаренко\Галина Сахарова\Галина Туфтина\Галина Улётова\Гелена 
Великанова\Геннадий Каменный\Георгий Абрамов\Георгий Виноградов\Георгий 
Нэлепп\Георг Отс\Глафира Королёва\Глеб Романов\Денис Королёв\Дмитрий 
Хворостовский\Евгений Белов\Евгений Беляев\Евгений Мартынов\Евгений 
Нестеренко\Евгений Поликанин\Евгений Черняк\Евгения Гороховская\Евгения 
Разина\Евгения Целовальник\Екатерина Громова\Екатерина Юровская\Елена 
Даль\Елена Образцова\Елизавета Антонова\Елизавета Долина\Елизавета 
Шумская\Ефрем Флакс\Зара Долуханова\Заур Тутов\Зураб Соткилава\Иван 
Алексеев\Иван Жадан\Иван Козловский\Иван Котляр\Иван Петров\Иван Ребров\Иван 
Скобцов\Изабелла Юрьева\Инесса Просаловская\Иосиф Кобзон\Ирина Архипова\Ирина 
Богачёва\Ирина Масленникова\Ирина Чмыхова\Ирма Голодяевская\Капиталина 
Лазаренко\Карина и Рузанна Лисициан\Кира Изотова\Клавдия Шульженко\Клара 
Кадинская\Константин Лисовский\Константин Огневой\Константин Плужников\Ксения 
Держинская\Кэто Джапаридзе\Ламара Чкония\Лариса Голубкина\Лев 
Сибиряков\Леокадия Масленникова\Леонид Кострица\Леонид Серебренников\Леонид 
Сметанников\Леонид Собинов\Леонид Утёсов\Леонид Харитонов\Лидия Мясникова\Лидия 
Русланова\Любовь Исаева\Людмила Зыкина\Людмила Легостаева\Людмила Филатова\Майя 
Кристалинская\Максим Михайлов\Мария Биешу\Мария Каринская\Мария Максакова\Мария 
Наровская\Марк Бернес\Марк Рейзен\Михаил Александрович\Михаил Вавич\Михаил 
Рыба\Мовсар Минцаев\Муслим Магомаев\Надежда Казанцева\Надежда Обухова\Надежда 
Плевицкая\Нани Брегвадзе\Наталья Герасимова\Наталья Шпиллер\Николай 
Гедда\Николай Гуторович\Николай Гяуров\Николай Никитский\Николай 
Охотников\Николай Тимченко\Николай Шевелёв\Николай Эрденко\Нина Дорлиак\Нина 
Исакова\Нина Поставничева\Нина Фомина\Нинель Ткаченко\Ольга Воронец\Ольга 
Мшанская\Павел Лисициан\Пантелеймон Норцов\Пётр Киричек\Пётр Словцов\Римантас 
Сипарис\Ружена Сикора\Сергей Захаров\Сергей Лейферкус\Сергей Лемешев\Сергей 
Мигай\Сергей Шапошников\Сергей Юдин\Соломон Хромченко\Софья 
Преображенская\Стронгилла Иртлач\Тамара Кравцова\Тамара Милашкина\Тамара 
Синявская\Тамара Церетели\Татьяна Лаврова\Татьяна Тугаринова\Тийт Куузик\Фидан 
Касимова\Фёдор Шаляпин\Эдита Пьеха\Эдит Утёсова\Эдуард Хиль\Эльмира 
Жерздева\Эмиль 

Re: [ovs-dev] [PATCH ovn] ovn-northd: Use HMAP_FOR_EACH when adding multicast lflows

[Mark Michelson]

I applied the change to master.

On 8/5/19 9:08 AM, Numan Siddique wrote:

On Thu, Aug 1, 2019 at 5:09 PM Dumitru Ceara  wrote:


There's no need to use HMAP_FOR_EACH_SAFE when we add the logical flows
that correspond to IGMP groups as we don't modify the hashmap in the
loop.

Also, we should reinitialize match and actions only if the flow will be
added.

Fixes: ddc64665b678 ("OVN: Add ovn-northd IGMP support")
Signed-off-by: Dumitru Ceara 



Acked-by: Numan Siddique 




---
  northd/ovn-northd.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index dabd422..880773b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5258,12 +5258,9 @@ build_lswitch_flows(struct hmap *datapaths, struct
hmap *ports,

  /* Ingress table 17: Add IP multicast flows learnt from IGMP
   * (priority 90). */
-struct ovn_igmp_group *igmp_group, *next_igmp_group;
-
-HMAP_FOR_EACH_SAFE (igmp_group, next_igmp_group, hmap_node,
igmp_groups) {
-ds_clear();
-ds_clear();
+struct ovn_igmp_group *igmp_group;

+HMAP_FOR_EACH (igmp_group, hmap_node, igmp_groups) {
  if (!igmp_group->datapath) {
  continue;
  }
@@ -5275,6 +5272,9 @@ build_lswitch_flows(struct hmap *datapaths, struct
hmap *ports,
  }
  mcast_info->active_flows++;

+ds_clear();
+ds_clear();
+
  ds_put_format(, "eth.mcast && ip4 && ip4.dst == %s ",
igmp_group->mcgroup.name);
  ds_put_format(, "outport = \"%s\"; output; ",
--
1.8.3.1

___
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 v2 ovn] Encode the virtual port key in vport_bind action in network byte order

[Mark Michelson]

I applied the change to master.

On 8/5/19 10:49 AM, Dumitru Ceara wrote:

On Mon, Aug 5, 2019 at 3:50 PM  wrote:


From: Numan Siddique 

The commit [1] encoded the vport key using uint32_t and the test case
"action parsing" is failing for s380 arch.

This patch fixes this issue by encoding the vport key in the network byte
order.

[1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")

Signed-off-by: Numan Siddique 


Looks good to me. Thanks!

Acked-by: Dumitru Ceara 


---
v1 -> v2
===
   * There was a sparse compilation error when I missed checking when
 submitting v1.  Corrected it.


  controller/pinctrl.c | 11 ++-
  lib/actions.c|  3 ++-
  tests/ovn.at |  2 +-
  3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579fcc..f27718f55 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
  uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];

  /* Get the virtual port key from the userdata buffer. */
-uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
+ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);

-if (!vport_key) {
+if (!vp_key) {
  return;
  }

-uint32_t hash = hash_2words(dp_key, *vport_key);
+uint32_t vport_key = ntohl(*vp_key);
+uint32_t hash = hash_2words(dp_key, vport_key);

  struct put_vport_binding *vpb
-= pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
+= pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
  if (!vpb) {
  if (hmap_count(_vport_bindings) >= 1000) {
  COVERAGE_INC(pinctrl_drop_put_vport_binding);
@@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
  }

  vpb->dp_key = dp_key;
-vpb->vport_key = *vport_key;
+vpb->vport_key = vport_key;
  vpb->vport_parent_key = vport_parent_key;

  notify_pinctrl_main();
diff --git a/lib/actions.c b/lib/actions.c
index 66916a837..b0cb3490b 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
  size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
false, NX_CTLR_NO_METER,
ofpacts);
-ofpbuf_put(ofpacts, _key, sizeof(uint32_t));
+ovs_be32 vp_key = htonl(vport_key);
+ofpbuf_put(ofpacts, _key, sizeof(ovs_be32));
  encode_finish_controller_op(oc_offset, ofpacts);
  encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
  }
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa20..344efad26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
  # bind_vport
  # lsp1's port key is 0x11.
  bind_vport("lsp1", inport);
-encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
+encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
  # lsp2 doesn't exist. So it should be encoded as drop.
  bind_vport("lsp2", inport);
  encodes as drop
--
2.21.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

Re: [ovs-dev] [PATCH ovn] RHEL: Suppress systemd status message during pre-installation.

[Numan Siddique]

On Mon, Aug 5, 2019 at 11:45 PM Mark Michelson  wrote:

> When running the pre-installation hook, the spec file attempts to
> determine if the old openvswitch-provided OVN services are running. If
> this is a fresh installation, then there never was an
> openvswitch-provided service installed on the machine. Therefore,
> systemd will emit a warning saying the service can't be found. There is
> nothing that indicates that this message is coming pre-installation, and
> so it appears as though something has gone wrong while trying to install
> the package.
>
> This commit suppresses stderr when checking the status of the ovn
> services pre-installation. This way, if a problem is encountered, it
> does not appear that there was an error during installation.
>
> Signed-off-by: Mark Michelson 
>

Acked-by: Numan Siddique 



> ---
>  rhel/ovn-fedora.spec.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
> index 2ecc629f2..2234e949f 100644
> --- a/rhel/ovn-fedora.spec.in
> +++ b/rhel/ovn-fedora.spec.in
> @@ -219,7 +219,7 @@ rm -rf $RPM_BUILD_ROOT
>  %pre central
>  if [ $1 -eq 1 ] ; then
>  # Package install.
> -/bin/systemctl status ovn-northd.service >/dev/null
> +/bin/systemctl status ovn-northd.service &>/dev/null
>  ovn_status=$?
>  rpm -ql openvswitch-ovn-central > /dev/null
>  if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
> @@ -233,7 +233,7 @@ fi
>  %pre host
>  if [ $1 -eq 1 ] ; then
>  # Package install.
> -/bin/systemctl status ovn-controller.service >/dev/null
> +/bin/systemctl status ovn-controller.service &>/dev/null
>  ovn_status=$?
>  rpm -ql openvswitch-ovn-host > /dev/null
>  if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
> --
> 2.14.5
>
> ___
> 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

[ovs-dev] [PATCH ovn] RHEL: Suppress systemd status message during pre-installation.

[Mark Michelson]

When running the pre-installation hook, the spec file attempts to
determine if the old openvswitch-provided OVN services are running. If
this is a fresh installation, then there never was an
openvswitch-provided service installed on the machine. Therefore,
systemd will emit a warning saying the service can't be found. There is
nothing that indicates that this message is coming pre-installation, and
so it appears as though something has gone wrong while trying to install
the package.

This commit suppresses stderr when checking the status of the ovn
services pre-installation. This way, if a problem is encountered, it
does not appear that there was an error during installation.

Signed-off-by: Mark Michelson 
---
 rhel/ovn-fedora.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 2ecc629f2..2234e949f 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -219,7 +219,7 @@ rm -rf $RPM_BUILD_ROOT
 %pre central
 if [ $1 -eq 1 ] ; then
 # Package install.
-/bin/systemctl status ovn-northd.service >/dev/null
+/bin/systemctl status ovn-northd.service &>/dev/null
 ovn_status=$?
 rpm -ql openvswitch-ovn-central > /dev/null
 if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
@@ -233,7 +233,7 @@ fi
 %pre host
 if [ $1 -eq 1 ] ; then
 # Package install.
-/bin/systemctl status ovn-controller.service >/dev/null
+/bin/systemctl status ovn-controller.service &>/dev/null
 ovn_status=$?
 rpm -ql openvswitch-ovn-host > /dev/null
 if [[ "$?" = "0" && "$ovn_status" = "0" ]]; then
-- 
2.14.5

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

Re: [ovs-dev] [PATCH v2 ovn] Build OVN using external OVS directory

[Numan Siddique]

On Mon, Aug 5, 2019 at 11:20 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> > From: Numan Siddique 
> >
> > With this patch we have to configure OVN to refer to external OVS
> source/build
> > directory instead of the ovs subtree.
> >
> > The new configuration options added are:
> >  * --with-ovs-source=/path/to/ovs/source/dir
> >  * --with-ovs-build=/path/to/ovs/build/dir
> >
> > Before configuring OVN, user should configure and compile OVS. If the
> user has
> > configured OVS on a different directory than the source dir, then
> 'with-ovs-build'
> > should be specified.
> >
> > If ovs-build dir is not defined, then ovs-source is used.
> >
> > An upcoming patch will delete the ovs subtree.
> >
> > Example usage:
> >   $ # Clone OVS repo
> >   $cd /home/foo/ovs
> >   $./boot.sh
> >   $mkdir _gcc
> >   $cd _gcc && ../configure && cd ..
> >   $make -C _gcc
> >
> >   $ # Clone OVN repo
> >   $cd /home/foo/ovn
> >   $./boot.sh
> >   $./configure --with-ovs-source=/home/foo/ovs/
> --with-ovs-build=/home/foo/ovs/_gcc
> >   $make
> >
> > Acked-by: Lucas Alvares Gomes 
> > Signed-off-by: Numan Siddique 
>
> Tested-by: Lorenzo Bianconi 
>

Thanks Lorenzo for testing it out.

I still the failiures in travis CI -
https://travis-ci.com/numansiddique/ovn/jobs/222512273

I will debug further and will submit v3 only after the CI passes next time.

Thanks
Numan



>
> > ---
> > v1 -> v2
> > ===
> >   * Travis CI builds were failing as "make distcheck" was not working as
> > expected. Fixed it in v2.
> >
> >
> >  .travis/linux-build.sh  |  17 ++-
> >  Documentation/intro/install/general.rst |  33 -
> >  Makefile.am |  24 ++--
> >  acinclude.m4|  35 ++
> >  configure.ac|  29 ++---
> >  controller-vtep/automake.mk |   2 +-
> >  include/ovn/version.h.in|  28 +
> >  lib/ovsdb_automake.mk   |   7 +-
> >  tests/automake.mk   |   6 +-
> >  tests/ofproto-macros.at |   4 +-
> >  tests/ovn-controller-vtep.at|   4 +-
> >  tests/ovn.at| 158 
> >  tests/ovsdb-macros.at   |   2 +-
> >  13 files changed, 218 insertions(+), 131 deletions(-)
> >  create mode 100644 include/ovn/version.h.in
> >
> > diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> > index a20474345..6f8d77ff4 100755
> > --- a/.travis/linux-build.sh
> > +++ b/.travis/linux-build.sh
> > @@ -10,7 +10,18 @@ TARGET="x86_64-native-linuxapp-gcc"
> >
> >  function configure_ovs()
> >  {
> > +git clone https://github.com/openvswitch/ovs.git ovs_src
> > +pushd ovs_src
> >  ./boot.sh && ./configure $* || { cat config.log; exit 1; }
> > +make -j4
> > +popd
> > +}
> > +
> > +function configure_ovn()
> > +{
> > +configure_ovs
> > +./boot.sh && ./configure --with-ovs-source=$PWD/ovs_src $* || \
> > +{ cat config.log; exit 1; }
> >  }
> >
> >  OPTS="$EXTRA_OPTS $*"
> > @@ -28,16 +39,16 @@ fi
> >  if [ "$TESTSUITE" ]; then
> >  # 'distcheck' will reconfigure with required options.
> >  # Now we only need to prepare the Makefile without sparse-wrapped
> CC.
> > -configure_ovs
> > +configure_ovn
> >
> > -export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
> > +export DISTCHECK_CONFIGURE_FLAGS="$OPTS
> --with-ovs-source=$PWD/ovs_src"
> >  if ! make distcheck -j4 TESTSUITEFLAGS="-j4 -k ovn" RECHECK=yes;
> then
> >  # testsuite.log is necessary for debugging.
> >  cat */_build/tests/testsuite.log
> >  exit 1
> >  fi
> >  else
> > -configure_ovs $OPTS
> > +configure_ovn $OPTS
> >  make selinux-policy
> >
> >  make -j4
> > diff --git a/Documentation/intro/install/general.rst
> b/Documentation/intro/install/general.rst
> > index 99d8fec04..ab1cf57ed 100644
> > --- a/Documentation/intro/install/general.rst
> > +++ b/Documentation/intro/install/general.rst
> > @@ -42,9 +42,8 @@ out.  This is the right branch for general development.
> >
> >  As of now there are no official OVN releases.
> >
> > -Although building OVN, also builds OVS, it is recommended to clone
> > -and build OVS from its own repo. Please see the Open vSwitch
> > -documentation to build and install OVS.
> > +Before building OVN you should configure and build OVS.
> > +Please see the Open vSwitch documentation to build and install OVS.
> >
> >  .. _general-build-reqs:
> >
> > @@ -143,16 +142,24 @@ the "configure" script::
> >
> >  $ ./boot.sh
> >
> > +Before configuring OVN, clone, configure and build Open vSwitch.
> > +
> >  .. _general-configuring:
> >
> >  Configuring
> >  ---
> >
> > -Configure the package by running the configure script. You can usually
> > -invoke configure without any arguments. For example::
> > +Configure the package by running the configure script. You need to
> > +invoke configure with atleast the 

Re: [ovs-dev] [PATCH v2 ovn] Build OVN using external OVS directory

[Lorenzo Bianconi]

> From: Numan Siddique 
> 
> With this patch we have to configure OVN to refer to external OVS source/build
> directory instead of the ovs subtree.
> 
> The new configuration options added are:
>  * --with-ovs-source=/path/to/ovs/source/dir
>  * --with-ovs-build=/path/to/ovs/build/dir
> 
> Before configuring OVN, user should configure and compile OVS. If the user has
> configured OVS on a different directory than the source dir, then 
> 'with-ovs-build'
> should be specified.
> 
> If ovs-build dir is not defined, then ovs-source is used.
> 
> An upcoming patch will delete the ovs subtree.
> 
> Example usage:
>   $ # Clone OVS repo
>   $cd /home/foo/ovs
>   $./boot.sh
>   $mkdir _gcc
>   $cd _gcc && ../configure && cd ..
>   $make -C _gcc
> 
>   $ # Clone OVN repo
>   $cd /home/foo/ovn
>   $./boot.sh
>   $./configure --with-ovs-source=/home/foo/ovs/ 
> --with-ovs-build=/home/foo/ovs/_gcc
>   $make
> 
> Acked-by: Lucas Alvares Gomes 
> Signed-off-by: Numan Siddique 

Tested-by: Lorenzo Bianconi 

> ---
> v1 -> v2
> ===
>   * Travis CI builds were failing as "make distcheck" was not working as
> expected. Fixed it in v2.
> 
> 
>  .travis/linux-build.sh  |  17 ++-
>  Documentation/intro/install/general.rst |  33 -
>  Makefile.am |  24 ++--
>  acinclude.m4|  35 ++
>  configure.ac|  29 ++---
>  controller-vtep/automake.mk |   2 +-
>  include/ovn/version.h.in|  28 +
>  lib/ovsdb_automake.mk   |   7 +-
>  tests/automake.mk   |   6 +-
>  tests/ofproto-macros.at |   4 +-
>  tests/ovn-controller-vtep.at|   4 +-
>  tests/ovn.at| 158 
>  tests/ovsdb-macros.at   |   2 +-
>  13 files changed, 218 insertions(+), 131 deletions(-)
>  create mode 100644 include/ovn/version.h.in
> 
> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> index a20474345..6f8d77ff4 100755
> --- a/.travis/linux-build.sh
> +++ b/.travis/linux-build.sh
> @@ -10,7 +10,18 @@ TARGET="x86_64-native-linuxapp-gcc"
>  
>  function configure_ovs()
>  {
> +git clone https://github.com/openvswitch/ovs.git ovs_src
> +pushd ovs_src
>  ./boot.sh && ./configure $* || { cat config.log; exit 1; }
> +make -j4
> +popd
> +}
> +
> +function configure_ovn()
> +{
> +configure_ovs
> +./boot.sh && ./configure --with-ovs-source=$PWD/ovs_src $* || \
> +{ cat config.log; exit 1; }
>  }
>  
>  OPTS="$EXTRA_OPTS $*"
> @@ -28,16 +39,16 @@ fi
>  if [ "$TESTSUITE" ]; then
>  # 'distcheck' will reconfigure with required options.
>  # Now we only need to prepare the Makefile without sparse-wrapped CC.
> -configure_ovs
> +configure_ovn
>  
> -export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
> +export DISTCHECK_CONFIGURE_FLAGS="$OPTS --with-ovs-source=$PWD/ovs_src"
>  if ! make distcheck -j4 TESTSUITEFLAGS="-j4 -k ovn" RECHECK=yes; then
>  # testsuite.log is necessary for debugging.
>  cat */_build/tests/testsuite.log
>  exit 1
>  fi
>  else
> -configure_ovs $OPTS
> +configure_ovn $OPTS
>  make selinux-policy
>  
>  make -j4
> diff --git a/Documentation/intro/install/general.rst 
> b/Documentation/intro/install/general.rst
> index 99d8fec04..ab1cf57ed 100644
> --- a/Documentation/intro/install/general.rst
> +++ b/Documentation/intro/install/general.rst
> @@ -42,9 +42,8 @@ out.  This is the right branch for general development.
>  
>  As of now there are no official OVN releases.
>  
> -Although building OVN, also builds OVS, it is recommended to clone
> -and build OVS from its own repo. Please see the Open vSwitch
> -documentation to build and install OVS.
> +Before building OVN you should configure and build OVS.
> +Please see the Open vSwitch documentation to build and install OVS.
>  
>  .. _general-build-reqs:
>  
> @@ -143,16 +142,24 @@ the "configure" script::
>  
>  $ ./boot.sh
>  
> +Before configuring OVN, clone, configure and build Open vSwitch.
> +
>  .. _general-configuring:
>  
>  Configuring
>  ---
>  
> -Configure the package by running the configure script. You can usually
> -invoke configure without any arguments. For example::
> +Configure the package by running the configure script. You need to
> +invoke configure with atleast the argument --with-ovs-source.
> +For example::
> +
> +$ ./configure --with-ovs-source=/path/to/ovs/source
>  
> -$ ./configure
> +If you have built Open vSwitch in a separate directory, then you
> +need to provide that path in the option - --with-ovs-build.
>  
> +As of now, OVN uses all the run time directory of Open vSwitch. This
> +will be changed to ``ovn`` specific directories.
>  By default all files are installed under ``/usr/local``. OVN and Open vSwitch
>  also expects to find its database in ``/usr/local/etc/openvswitch`` by 
> 

[ovs-dev] [PATCH ovn] Fix DNAT/SNAT system-ovn unit tests

[Lorenzo Bianconi]

Fix conntrack checks in the following tests in tests/system-ovn.at:
- ovn -- DNAT and SNAT on distributed router - N/S
- ovn -- DNAT and SNAT on distributed router - E/W

Fixes: a6ee09882283 ("OVN: run local logical flows first in S_ROUTER_OUT_SNAT 
table")
Signed-off-by: Lorenzo Bianconi 
---
 tests/system-ovn.at | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 10fbd2649..f88ad31e4 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1334,11 +1334,13 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 
172.16.1.2 | FORMAT_PING], \
 ])
 
 # We verify that SNAT indeed happened via 'dump-conntrack' command.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
 sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=,type=0,code=0),zone=
+icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=,type=0,code=0),zone=
 ])
 
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
 # South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
 # from 172.16.1.1
 NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
@@ -1507,12 +1509,14 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 
172.16.1.4 | FORMAT_PING], \
 
 # Check conntrack entries.  First SNAT of 'foo1' address happens.
 # Then DNAT of 'bar1' address happens (listed first below).
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.3) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
 sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
-icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=,type=0,code=0),zone=
-icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=,type=0,code=0),zone=
+icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=,type=0,code=0),zone=
+icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=,type=0,code=0),zone=
 ])
 
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
 # East-West NAT: 'foo2' pings 'bar1' using 172.16.1.4.
 NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
 [0], [dnl
-- 
2.21.0

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

Re: [ovs-dev] [PATCH branch 2.12] ovn-controller: Fix inject-pkt command error response.

[0-day Robot]

Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Numan Siddique 
Lines checked: 60, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH branch 2.12 2/2] ovn-controller: Fix IP engine run with in-flight messages

[0-day Robot]

Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Numan Siddique 
Lines checked: 74, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH branch 2.12 1/2] ovn-controller: Fix flow installation latency caused by recompute.

[0-day Robot]

Bleep bloop.  Greetings Han Zhou, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Numan Siddique 
Lines checked: 244, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 2/4 ovn] OVN: Vlan backed DVR N-S, redirect-type option

[Numan Siddique]

On Fri, Aug 2, 2019 at 5:24 AM Ankur Sharma 
wrote:

> Background:
> With c0974331b7a19a87ab8f1f2cec8fbe366af92fa2, we have added
> support for E-W workflow for vlan backed DVRs.
>
> This series enables N-S workflow for vlan backed DVRs.
>
> Key difference between E-W and N-S traffic flow is that
> N-S flow requires a gateway chassis. A gateway chassis
> will be respondible for following:
> a. Doing Network Address Translation (NAT).
> b. Becoming entry and exit point for North->South
>and South->North traffic respectively.
>
> OVN by default always uses overlay encapsulation to redirect
> the packet to gateway chassis. This series will enable
> the redirection to gateway chassis in the absence of encapsulation.
>
> This patch:
> a. Add a new key-value in options of a router port.
> b. This new config key will be used by ovn-controller
>to determine if a redirected packet will go out of
>tunnel port or localnet port.
> c. key is "redirect-type" and it takes "overlay" and
>"vlan" as values.
> d. Added ovn-nbctl command to set and get redirect-type
>option on a router port.
> e. This new configuration is added because vlan or overlay
>based forwarding is considered to be a logical switch property,
>hence for a router configuration has to be done at the router port
>level.
>
> Signed-off-by: Ankur Sharma 
> ---
>  northd/ovn-northd.c   |  6 ++
>  ovn-nb.xml| 43 ++
>  tests/ovn-nbctl.at| 25 ++
>  tests/ovn-northd.at   | 31 +++
>  utilities/ovn-nbctl.c | 58
> +++
>  5 files changed, 163 insertions(+)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index cd776fa..7c0fd6c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -2445,6 +2445,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
>  if (op->derived) {
>  const char *redirect_chassis = smap_get(>nbrp->options,
>  "redirect-chassis");
> +const char *redirect_type = smap_get(>nbrp->options,
> + "redirect-type");
> +
>  int n_gw_options_set = 0;
>  if (op->nbrp->ha_chassis_group) {
>  n_gw_options_set++;
> @@ -2537,6 +2540,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
>  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
>  }
>  smap_add(, "distributed-port", op->nbrp->name);
> +if (redirect_type) {
> +smap_add(, "redirect-type", redirect_type);
> +}
>  } else {
>  if (op->peer) {
>  smap_add(, "peer", op->peer->key);
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index f5f10a5..971017b 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1947,6 +1947,49 @@
>issues.
>  
>
> +
> +  
> +
> +  This options dictates if a packet redirected to
> +  gateway chassis will be overlay encapsulated
> +  or go as a regular vlan packet.
> +
> +
> +
> +  Option takes following values
> +
> +
> +
> +  
> +OVERLAY
> +  
> +
> +  
> +VLAN
> +  
> +
> +
> +
> +  OVERLAY option will ensure that redirected packet goes out as
> +  encapsulation via the tunnel port.
> +
> +
> +
> +  VLAN option will ensure that redirected packet goes out as vlan
> +  tagged via the localnet port.
> +
> +
> +
> +  OVERLAY is the default redirection type.
> +
> +
> +
> +  Option is applicable only to gateway chassis attached logical
> +  router ports.
> +
> +
> +  
> +
>  
>
>  
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index a19e33f..11a3273 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -1220,6 +1220,31 @@ lrp0-chassis1 1
>
>  dnl -
>
> +OVN_NBCTL_TEST([ovn_nbctl_redirect_type], [logical router port redirect
> type], [
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
> +AT_CHECK([ovn-nbctl lrp-get-redirect-type lrp0], [0], [dnl
> +overlay
> +])
> +AT_CHECK([ovn-nbctl lrp-set-redirect-type lp0 vlan], [1], [],
> +[ovn-nbctl: lp0: port name not found
> +])
> +AT_CHECK([ovn-nbctl lrp-set-redirect-type lrp0 vlan], [0], [])
> +AT_CHECK([ovn-nbctl lrp-get-redirect-type lrp0], [0], [dnl
> +vlan
> +])
> +AT_CHECK([ovn-nbctl lrp-set-redirect-type lrp0 overlay], [0], [])
> +AT_CHECK([ovn-nbctl lrp-get-redirect-type lrp0], [0], [dnl
> +overlay
> +])
> +AT_CHECK([ovn-nbctl lrp-set-redirect-type lrp0 abcd], [1], [],
> +[ovn-nbctl: Invalid redirect type: 

[ovs-dev] [PATCH branch 2.12] ovn-controller: Fix inject-pkt command error response.

[Han Zhou]

From: Han Zhou 

From: Han Zhou 

When using unixctl command inject-packet, it always respond with
failure "server not ready", although the command was actually executed
successfully.

Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - quiet 
mode.")
Signed-off-by: Han Zhou 
Acked-by: Dumitru Ceara 
Signed-off-by: Numan Siddique 
---
 ovn/controller/ovn-controller.c | 12 +++-
 tests/ovn.at|  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index cf6c8ae..3f03a18 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -2055,12 +2055,14 @@ main(int argc, char *argv[])
 unixctl_command_reply_error(pending_pkt.conn, error);
 free(error);
 } else {
-VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
- "%p not ready. run-id: %"PRIu64, br_int,
- chassis, engine_run_id);
-unixctl_command_reply_error(pending_pkt.conn,
-"ovn-controller not ready.");
+unixctl_command_reply(pending_pkt.conn, NULL);
 }
+} else {
+VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
+ "%p not ready. run-id: %"PRIu64, br_int,
+ chassis, engine_run_id);
+unixctl_command_reply_error(pending_pkt.conn,
+"ovn-controller not ready.");
 }
 pending_pkt.conn = NULL;
 free(pending_pkt.flow_s);
diff --git a/tests/ovn.at b/tests/ovn.at
index 5d6c90c..372ba01 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3723,7 +3723,7 @@ sleep 1
 packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac &&
 ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
 udp && udp.src==53 && udp.dst==4369"
-as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
 
 
 echo "-NB dump-"
-- 
2.1.0

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

Re: [ovs-dev] [PATCH v2 3/9] ct-dpif: Export ct_dpif_format_ipproto()

[Yi-Hung Wei]

On Fri, Aug 2, 2019 at 5:51 PM Justin Pettit  wrote:
>
>
> > On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei  wrote:
> >
> > This function will be useful for following patchs.
>
> s/patchs/patches/
>
> Acked-by: Justin Pettit 
>
> --Justin

Thanks,  will fix the typo in v3.

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

Re: [ovs-dev] [PATCH v2 branch-2.11] ovsdb-server: drop all connections on read/write status change

[0-day Robot]

Bleep bloop.  Greetings Daniel Alvarez, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Ben Pfaff 
Lines checked: 69, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@bytheb.org

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

[Yi-Hung Wei]

On Fri, Aug 2, 2019 at 11:15 AM Justin Pettit  wrote:
>
>
> > On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei  wrote:
> >
> > From: Justin Pettit 
> >
> > From: Justin Pettit 
>
> Can you drop one of these "From:" statements?  Otherwise it appears in the 
> commit message.
>
> As we discussed off-line, can you apply the following diff, which we worked 
> on together along with your co-authored-by tag?
>

Thanks for review.  I will add the diff into v3.

Thanks,

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

Re: [ovs-dev] [PATCH v3 1/4 ovn] OVN: Do not replace router port mac on gateway chassis.

[Numan Siddique]

The patch LGTM. Few comments inline.

Please see below

Thanks
Numan

On Fri, Aug 2, 2019 at 5:23 AM Ankur Sharma 
wrote:

> With 795d7f24ce0e2ed5454e193a059451d237289542 we have added
> support for E-W routing on vlan backed networks by replacing
> router port macs with chassis macs.
>
> This replacement of router port mac need NOT be done on
> gateway chassis for following reasons:
>
> a. For N-S traffic, gateway chassis will respond to ARP
> for the router port (to which it is attached) and
> traffic will be using router port mac as destination mac.
>
> b. Chassis redirect port is a centralized version of distributed
> router port, hence we need not replace its mac with chassis mac
> on the resident chassis.
>
> This patch addresses the same.
>
> Signed-off-by: Ankur Sharma 
> ---
>  controller/physical.c |  19 ++-
>  controller/pinctrl.c  |  37 +++---
>  controller/pinctrl.h  |   5 +
>  tests/ovn.at  | 313
> ++
>  4 files changed, 354 insertions(+), 20 deletions(-)
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 5c2f74e..aa06b3f 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -38,6 +38,7 @@
>  #include "lib/ovn-sb-idl.h"
>  #include "lib/ovn-util.h"
>  #include "physical.h"
> +#include "pinctrl.h"
>  #include "openvswitch/shash.h"
>  #include "simap.h"
>  #include "smap.h"
> @@ -228,9 +229,12 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>  }
>
>  static void
> -put_replace_router_port_mac_flows(const struct
> +put_replace_router_port_mac_flows(struct ovsdb_idl_index
> +  *sbrec_port_binding_by_name,
> +  const struct
>sbrec_port_binding *localnet_port,
>const struct sbrec_chassis *chassis,
> +  const struct sset *active_tunnels,
>const struct hmap *local_datapaths,
>struct ofpbuf *ofpacts_p,
>ofp_port_t ofport,
> @@ -270,6 +274,16 @@ put_replace_router_port_mac_flows(const struct
>  struct eth_addr router_port_mac;
>  struct match match;
>  struct ofpact_mac *replace_mac;
> +char *cr_peer_name = xasprintf("cr-%s",
> rport_binding->logical_port);
> +if (pinctrl_is_chassis_resident(sbrec_port_binding_by_name,
> +chassis, active_tunnels,
> +cr_peer_name)) {
> +/* If a router port's chassisredirect port is
> + * resident on this chassis, then we need not do mac replace.
> */
> +free(cr_peer_name);
> +continue;
> +}
> +free(cr_peer_name);
>
>  /* Table 65, priority 150.
>   * ===
> @@ -787,7 +801,8 @@ consider_port_binding(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
>  , ofpacts_p, >header_.uuid);
>
>  if (!strcmp(binding->type, "localnet")) {
> -put_replace_router_port_mac_flows(binding, chassis,
> +put_replace_router_port_mac_flows(sbrec_port_binding_by_name,
> +  binding, chassis,
> active_tunnels,
>local_datapaths, ofpacts_p,
>ofport, flow_table);
>  }
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f05579f..d0b2e27 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -455,6 +455,25 @@ pinctrl_init(void)
>  );
>  }
>
> +bool
> +pinctrl_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> +const struct sbrec_chassis *chassis,
> +const struct sset *active_tunnels,
> +const char *port_name)
> +{
> +const struct sbrec_port_binding *pb
> += lport_lookup_by_name(sbrec_port_binding_by_name, port_name);
> +if (!pb || !pb->chassis) {
> +return false;
> +}
> +if (strcmp(pb->type, "chassisredirect")) {
> +return pb->chassis == chassis;
> +} else {
> +return ha_chassis_group_is_active(pb->ha_chassis_group,
> +  active_tunnels, chassis);
> +}
> +}
> +
>  static ovs_be32
>  queue_msg(struct rconn *swconn, struct ofpbuf *msg)
>  {
> @@ -3755,24 +3774,6 @@ get_localnet_vifs_l3gwports(
>  sbrec_port_binding_index_destroy_row(target);
>  }
>
> -static bool
> -pinctrl_is_chassis_resident(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> -const struct sbrec_chassis *chassis,
> -const struct sset *active_tunnels,
> -const char *port_name)

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix inject-pkt command error response.

[Dumitru Ceara]

On Mon, Aug 5, 2019 at 5:25 PM Han Zhou  wrote:
>
>
>
> On Mon, Aug 5, 2019 at 6:53 AM Numan Siddique  wrote:
> >
> >
> >
> > On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara  wrote:
> >>
> >> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou  wrote:
> >> >
> >> > From: Han Zhou 
> >> >
> >> > When using unixctl command inject-packet, it always respond with
> >> > failure "server not ready", although the command was actually executed
> >> > successfully.
> >> >
> >> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine 
> >> > - quiet mode.")
> >> > Signed-off-by: Han Zhou 
> >>
> >> Hi Han,
> >>
> >> I had already sent the exact same patch here :)
> >> https://patchwork.ozlabs.org/patch/1140480/
> >>
> >> Nevertheless:
> >> Acked-by: Dumitru Ceara 
>
> Dumitru, I am so sorry that I missed that email even it had me in cc. I just 
> updated my filter rules to make sure I won't miss such email again. Thank you 
> for fixing so many issues recently!

No problem. Your patch was better anyway as you also added a test for the issue.

>
> >
> >
> > Thanks Han and Dumitru (for the review). I applied this patch to master as 
> > this one had a test.
> >
> > Thanks
> > Numan
> >
> >>
> >> Thanks,
> >> Dumitru
> >>
> >> > ---
> >> >  controller/ovn-controller.c | 12 +++-
> >> >  tests/ovn.at|  2 +-
> >> >  2 files changed, 8 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> > index ad33d17..9df598f 100644
> >> > --- a/controller/ovn-controller.c
> >> > +++ b/controller/ovn-controller.c
> >> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
> >> >  unixctl_command_reply_error(pending_pkt.conn, 
> >> > error);
> >> >  free(error);
> >> >  } else {
> >> > -VLOG_DBG("Pending_pkt conn but br_int %p or 
> >> > chassis "
> >> > - "%p not ready. run-id: %"PRIu64, 
> >> > br_int,
> >> > - chassis, engine_run_id);
> >> > -unixctl_command_reply_error(pending_pkt.conn,
> >> > -"ovn-controller not ready.");
> >> > +unixctl_command_reply(pending_pkt.conn, NULL);
> >> >  }
> >> > +} else {
> >> > +VLOG_DBG("Pending_pkt conn but br_int %p or chassis 
> >> > "
> >> > + "%p not ready. run-id: %"PRIu64, br_int,
> >> > + chassis, engine_run_id);
> >> > +unixctl_command_reply_error(pending_pkt.conn,
> >> > +"ovn-controller not ready.");
> >> >  }
> >> >  pending_pkt.conn = NULL;
> >> >  free(pending_pkt.flow_s);
> >> > diff --git a/tests/ovn.at b/tests/ovn.at
> >> > index e88cffa..b85e549 100644
> >> > --- a/tests/ovn.at
> >> > +++ b/tests/ovn.at
> >> > @@ -3723,7 +3723,7 @@ sleep 1
> >> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && 
> >> > eth.dst==$rp_ls1_mac &&
> >> >  ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && 
> >> > ip4.dst==$ls2_lp1_ip &&
> >> >  udp && udp.src==53 && udp.dst==4369"
> >> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> >> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
> >> >
> >> >
> >> >  echo "-NB dump-"
> >> > --
> >> > 2.1.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

Re: [ovs-dev] [PATCH] ovn-controller: Fix flow installation latency caused by recompute.

[Han Zhou]

I resent it for backporting to 2.12 as a series followed by a fix from
Dumitru, which has been merged in OVN repo, too.
https://patchwork.ozlabs.org/project/openvswitch/list/?series=123382

Thanks Dumitru again for the fix.

On Tue, Jul 30, 2019 at 12:16 AM Han Zhou  wrote:

> Thanks Mark and Numan for the review.
> Ben, now this is merged to OVN repo. Could you help backporting it to 2.12?
>
> Thanks,
> Han
>
>
> On Thu, Jul 25, 2019 at 1:23 PM Mark Michelson 
> wrote:
> >
> > Forgot the e-mail address after my name :)
> >
> > Acked-by: Mark Michelson 
> >
> > On 7/25/19 4:16 PM, Mark Michelson wrote:
> > > Acked-by: Mark Michelson
> > >
> > > Thanks for the informative commit message!
> > >
> > > On 7/23/19 10:52 PM, Han Zhou wrote:
> > >> From: Han Zhou 
> > >>
> > >> When there are in-flight flow-installations pending to ovs-vswitchd,
> > >> current incremental processing logic prioritizes new change handling.
> > >> However, in scenarios where frequent recomputes are triggered, the
> > >> later recompute would block the flow-installation for previously
> > >> computed flows because recompute usually takes long time, especially
> > >> when there are large number of flows. This results in worse latency
> > >> than the version without incremental processing in specific scale
> > >> test scenarios.
> > >>
> > >> While we can simply fix the problem by prioritizing flow installation
> > >> rather than new change handling, it can cause the incremental
> > >> processing to degrade to always recompute in certain scenarios when
> > >> there are some changes triggering recomputes, followed by a lot of
> > >> continously coming changes that can be handled incrementally. Because
> > >> OVSDB change tracker cannot preserve changes across iterations, once
> > >> the recompute is triggered and resulted in a lot of pending messages
> > >> to ovs-vswitchd, and if we choose to skip the engine_run()
> > >> in the next iteration when a incrementally processible change comes,
> > >> we miss the opportunity to handle that tracked change and will have
> > >> to trigger recompute again in the next next iteration, and so on, if
> > >> such changes come continously.
> > >>
> > >> This patch solves the problem by introducing
> > >> engine_set_abort_recompute(),
> > >> so that we can prioritize new change handling if the change can be
> > >> incrementally processed, but if the change triggers recompute, we
> > >> abort there without spending CPU on the recompute to avoid blocking
> > >> the previous computed flow installation.
> > >>
> > >> Reported-by: Daniel Alvarez Sanchez 
> > >> Reported-by: Numan Siddique 
> > >> Reported-at:
> > >>
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
> > >> Tested-by: Numan Siddique 
> > >> Signed-off-by: Han Zhou 
> > >> ---
> > >>   ovn/controller/ofctrl.c |  2 +-
> > >>   ovn/controller/ofctrl.h |  1 +
> > >>   ovn/controller/ovn-controller.c | 30 +++---
> > >>   ovn/lib/inc-proc-eng.c  | 26 ++
> > >>   ovn/lib/inc-proc-eng.h  |  9 +++--
> > >>   5 files changed, 58 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > >> index 043abd6..0fcaa72 100644
> > >> --- a/ovn/controller/ofctrl.c
> > >> +++ b/ovn/controller/ofctrl.c
> > >> @@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
> > >>* in the correct state and not backlogged with existing flow_mods.
> > >> (Our
> > >>* criteria for being backlogged appear very conservative, but the
> > >> socket
> > >>* between ovn-controller and OVS provides some buffering.) */
> > >> -static bool
> > >> +bool
> > >>   ofctrl_can_put(void)
> > >>   {
> > >>   if (state != S_UPDATE_FLOWS
> > >> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> > >> index ed8918a..2b21c11 100644
> > >> --- a/ovn/controller/ofctrl.h
> > >> +++ b/ovn/controller/ofctrl.h
> > >> @@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
> > >>   const struct sbrec_meter_table *,
> > >>   int64_t nb_cfg,
> > >>   bool flow_changed);
> > >> +bool ofctrl_can_put(void);
> > >>   void ofctrl_wait(void);
> > >>   void ofctrl_destroy(void);
> > >>   int64_t ofctrl_get_cur_cfg(void);
> > >> diff --git a/ovn/controller/ovn-controller.c
> > >> b/ovn/controller/ovn-controller.c
> > >> index c4883aa..866cc1c 100644
> > >> --- a/ovn/controller/ovn-controller.c
> > >> +++ b/ovn/controller/ovn-controller.c
> > >> @@ -1869,6 +1869,7 @@ main(int argc, char *argv[])
> > >>   uint64_t engine_run_id = 0;
> > >>   uint64_t old_engine_run_id = 0;
> > >> +bool engine_aborted = false;
> > >>   unsigned int ovs_cond_seqno = UINT_MAX;
> > >>   unsigned int ovnsb_cond_seqno = UINT_MAX;
> > >> @@ -1955,7 +1956,30 @@ main(int argc, char *argv[])
> > >>   

[ovs-dev] [PATCH branch-2.12] [ovn-northd] Don't emit ICMP need to frag on LRP with no IPv4 address

[Daniel Alvarez]

Prior to this patch, when a LRP has only IPv6 addresses, ovn-northd
will crash (SIGSEV) because the current code injects a flow to
emit the ICMP need-to-frag from its IPv4 address which does not
exist.

This patch is adding a check to skip the flow installation in case
the port does not have any IPv4 address.

Signed-off-by: Daniel Alvarez 
---
 ovn/northd/ovn-northd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ae09cf338..21e546d38 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -7855,7 +7855,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
 for (size_t i = 0; i < od->nbr->n_ports; i++) {
 struct ovn_port *rp = ovn_port_find(ports,
 od->nbr->ports[i]->name);
-if (!rp || rp == od->l3dgw_port) {
+if (!rp || rp == od->l3dgw_port ||
+!rp->lrp_networks.ipv4_addrs) {
 continue;
 }
 ds_clear();
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH branch 2.12 1/2] ovn-controller: Fix flow installation latency caused by recompute.

[Han Zhou]

From: Han Zhou 

When there are in-flight flow-installations pending to ovs-vswitchd,
current incremental processing logic prioritizes new change handling.
However, in scenarios where frequent recomputes are triggered, the
later recompute would block the flow-installation for previously
computed flows because recompute usually takes long time, especially
when there are large number of flows. This results in worse latency
than the version without incremental processing in specific scale
test scenarios.

While we can simply fix the problem by prioritizing flow installation
rather than new change handling, it can cause the incremental
processing to degrade to always recompute in certain scenarios when
there are some changes triggering recomputes, followed by a lot of
continously coming changes that can be handled incrementally. Because
OVSDB change tracker cannot preserve changes across iterations, once
the recompute is triggered and resulted in a lot of pending messages
to ovs-vswitchd, and if we choose to skip the engine_run()
in the next iteration when a incrementally processible change comes,
we miss the opportunity to handle that tracked change and will have
to trigger recompute again in the next next iteration, and so on, if
such changes come continously.

This patch solves the problem by introducing engine_set_abort_recompute(),
so that we can prioritize new change handling if the change can be
incrementally processed, but if the change triggers recompute, we
abort there without spending CPU on the recompute to avoid blocking
the previous computed flow installation.

Reported-by: Daniel Alvarez Sanchez 
Reported-by: Numan Siddique 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048822.html
Tested-by: Numan Siddique 
Acked-by: Numan Siddique 
Acked-by: Mark Michelson 
Signed-off-by: Han Zhou 
Signed-off-by: Numan Siddique 
---
 ovn/controller/ofctrl.c |  2 +-
 ovn/controller/ofctrl.h |  1 +
 ovn/controller/ovn-controller.c | 30 +++---
 ovn/lib/inc-proc-eng.c  | 26 ++
 ovn/lib/inc-proc-eng.h  |  9 +++--
 5 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 043abd6..0fcaa72 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -985,7 +985,7 @@ add_meter(struct ovn_extend_table_info *m_desired,
  * in the correct state and not backlogged with existing flow_mods.  (Our
  * criteria for being backlogged appear very conservative, but the socket
  * between ovn-controller and OVS provides some buffering.) */
-static bool
+bool
 ofctrl_can_put(void)
 {
 if (state != S_UPDATE_FLOWS
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index ed8918a..2b21c11 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -51,6 +51,7 @@ void ofctrl_put(struct ovn_desired_flow_table *,
 const struct sbrec_meter_table *,
 int64_t nb_cfg,
 bool flow_changed);
+bool ofctrl_can_put(void);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 int64_t ofctrl_get_cur_cfg(void);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index cf6c8ae..54b1aa9 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -1896,6 +1896,7 @@ main(int argc, char *argv[])
 
 uint64_t engine_run_id = 0;
 uint64_t old_engine_run_id = 0;
+bool engine_aborted = false;
 
 unsigned int ovs_cond_seqno = UINT_MAX;
 unsigned int ovnsb_cond_seqno = UINT_MAX;
@@ -1982,7 +1983,30 @@ main(int argc, char *argv[])
 stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
 time_msec());
 if (ovnsb_idl_txn) {
-engine_run(_flow_output, ++engine_run_id);
+if (!ofctrl_can_put()) {
+/* When there are in-flight messages pending to
+ * ovs-vswitchd, we should hold on recomputing so
+ * that the previous flow installations won't be
+ * delayed.  However, we still want to try if
+ * recompute is not needed and we can quickly
+ * incrementally process the new changes, to avoid
+ * unnecessarily forced recomputes later on.  This
+ * is because the OVSDB change tracker cannot
+ * preserve tracked changes across iterations.  If
+ * change tracking is improved, we can simply skip
+ * this round of engine_run and continue processing
+ * acculated changes incrementally later when
+ * ofctrl_can_put() returns true. */
+if (!engine_aborted) {

[ovs-dev] [PATCH branch 2.12 2/2] ovn-controller: Fix IP engine run with in-flight messages

[Han Zhou]

From: Dumitru Ceara 

When trying to incrementally process changes even if there are in-flight
messages we were incorrectly setting the engine_aborted variable to the
value returned by engine_run. However, engine_run returns true if the
run was completed.

This was causing discrepancies between logical flows and openflow flows
due to the fact that the rerun wasn't happening after an aborted run.

In order to avoid confusion the engine_aborted variable is now renamed to
engine_run_done thus avoiding the negated logic.

CC: Han Zhou 
Fixes: a6b7d9f4f047 ("ovn-controller: Fix flow installation latency caused by
recompute.")
Acked-by: Han Zhou 
Acked-by: Mark Michelson 
Tested-by: Numan Siddique 
Signed-off-by: Dumitru Ceara 
Signed-off-by: Numan Siddique 
---
 ovn/controller/ovn-controller.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 54b1aa9..b4a027c 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -1896,7 +1896,7 @@ main(int argc, char *argv[])
 
 uint64_t engine_run_id = 0;
 uint64_t old_engine_run_id = 0;
-bool engine_aborted = false;
+bool engine_run_done = true;
 
 unsigned int ovs_cond_seqno = UINT_MAX;
 unsigned int ovnsb_cond_seqno = UINT_MAX;
@@ -1997,14 +1997,14 @@ main(int argc, char *argv[])
  * this round of engine_run and continue processing
  * acculated changes incrementally later when
  * ofctrl_can_put() returns true. */
-if (!engine_aborted) {
+if (engine_run_done) {
 engine_set_abort_recompute(true);
-engine_aborted = engine_run(_flow_output,
-++engine_run_id);
+engine_run_done = engine_run(_flow_output,
+ ++engine_run_id);
 }
 } else {
 engine_set_abort_recompute(false);
-engine_aborted = false;
+engine_run_done = true;
 engine_run(_flow_output, ++engine_run_id);
 }
 }
@@ -2048,8 +2048,8 @@ main(int argc, char *argv[])
 }
 
 }
-if (old_engine_run_id == engine_run_id || engine_aborted) {
-if (engine_aborted || engine_need_run(_flow_output)) {
+if (old_engine_run_id == engine_run_id || !engine_run_done) {
+if (!engine_run_done || engine_need_run(_flow_output)) {
 VLOG_DBG("engine did not run, force recompute next time: "
  "br_int %p, chassis %p", br_int, chassis);
 engine_set_force_recompute(true);
-- 
2.1.0

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

[ovs-dev] [PATCH v2 branch-2.11] ovsdb-server: drop all connections on read/write status change

[Daniel Alvarez]

Prior to this patch, only db change aware connections were dropped
on a read/write status change. However, current schema in OVN does
not allow clients to monitor whether a particular DB changes this
status. In order to accomplish this, we'd need to change the schema
and adapting ovsdb-server and existing clients.

Before tackling that, this patch is changing ovsdb-server to drop
*all* the existing connections upon a read/write status change. This
will force clients to reconnect and honor the change.

Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2019-July/048981.html
Signed-off-by: Daniel Alvarez 
Signed-off-by: Ben Pfaff 
---
 ovsdb/jsonrpc-server.c |  2 +-
 tests/ovn.at   | 21 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 7c7a277f0..739e0e72e 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -365,7 +365,7 @@ ovsdb_jsonrpc_server_set_read_only(struct 
ovsdb_jsonrpc_server *svr,
 {
 if (svr->read_only != read_only) {
 svr->read_only = read_only;
-ovsdb_jsonrpc_server_reconnect(svr, false,
+ovsdb_jsonrpc_server_reconnect(svr, true,
xstrdup(read_only
? "making server read-only"
: "making server read/write"));
diff --git a/tests/ovn.at b/tests/ovn.at
index 4b14720f3..291330b3b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12090,3 +12090,24 @@ ovn-nbctl list logical_switch_port
 ovn-nbctl list logical_router_port

 AT_CLEANUP
+
+# Run ovn-nbctl in daemon mode, change to a backup database and verify that
+# an insert operation is not allowed.
+AT_SETUP([ovn -- can't write to a backup database server instance])
+ovn_start
+on_exit 'kill $(cat ovn-nbctl.pid)'
+export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach)
+
+AT_CHECK([ovn-nbctl ls-add sw0])
+as ovn-nb
+AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/sync-status | grep active | 
wc -l], [0], [1
+])
+ovs-appctl -t ovsdb-server ovsdb-server/set-active-ovsdb-server 
tcp:192.0.2.2:6641
+ovs-appctl -t ovsdb-server ovsdb-server/connect-active-ovsdb-server
+AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/sync-status | grep -c 
backup], [0], [1
+])
+AT_CHECK([ovn-nbctl ls-add sw1], [1], [ignore],
+[ovn-nbctl: transaction error: {"details":"insert operation not allowed when 
database server is in read only mode","error":"not allowed"}
+])
+
+AT_CLEANUP
--
2.21.0 (Apple Git-120)

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

Re: [ovs-dev] [PATCH ovn v2] OVN: Fix learning of neighbors from ARP/ND packets.

[Han Zhou]

On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara  wrote:
>
> On Mon, Aug 5, 2019 at 1:41 AM Han Zhou  wrote:
> >
> >
> >
> > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara  wrote:
> > >
> > > Add a restriction on the target protocol addresses to match the
> > > configured subnets. All other ARP/ND packets are invalid in this
> > > context.
> > >
> > > One exception is for ARP replies that are received for route next-hops
> > > that are only reachable via a port but can't be directly resolved
> > > through route lookups. Such support was introduced by commit:
> > >
> > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.")
> > >
> > > Reported-at: https://bugzilla.redhat.com/1729846
> > > Reported-by: Haidong Li 
> > > CC: Han Zhou 
> > > CC: Guru Shetty 
> > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP
request.")
> > > Signed-off-by: Dumitru Ceara 
> >
> > Hi Dumitru,
>
> Hi Han,
>
> Thanks for reviewing this.
>
> >
> > Sorry for my slow response, and thanks a lot for revising the patch for
a bigger scope of validations. However, the exception of /32 address makes
me thinking more about this patch. If ARP replies is allowed from any
nexthop for a LR port with /32, at least ARP request for GARP purpose
should also be allowed. But before asking for a v3, I'd hold on to rethink
the purpose of this patch.
>
> Right, we should allow GARP requests too. If we decide to go ahead
> with this patch I'll add a function in v3 to handle all types of ARPs
> and call it both for unreachable static route next-hops and attached
> subnets.
>
> >
> > The nexthop specific flows are now from static routes. What if OVN
support dynamical routing protocols in the future? Of course we can then
take those dynamic nexthops into allowed peers. But then I am thinking what
is the real benefit of all these restrictions? Why can't we just have
simpler logic to handle all these situations without validation? I think
the major benefit of the validation is to avoid handling the noise ARP/NDs
when multiple subnets shares same L2, but most cases it is really not a big
deal, right? For the CPU problem caused by ARP flooding as mentioned by the
bug report, it is a real problem, but this patch seems not really helpful
for that, because the attacker can just trigger the same CPU problem with
*valid* packets. So I am not sure if the benefit of the change is worth the
complexity it introduced. Please share your thought and correct me if I
missed something.
> >
> > Thanks,
> > Han
>
> I assume the simpler logic to handle all these situations without
> validation is to add rate limiting for ARP packets that get punted to
> the controller. I agree that this should be implemented too.
>
> But I think rate limiting all ARP packets regardless of IP addresses
> is not enough. In the following scenario and if we would have a way to
> rate limit ARP packets:
> - Subnet 42.42.42.0/24 configured on the OVN
> - "Invalid" ARP packets are injected at high rate in the network for
41.41.41.41
> - Host 42.42.42.42 sends GARP.
> - Rate limiting of ARP packets towards controller at 100pps
>
> With the current code, ARP packets for 41.41.41.41 will be punted to
> controller at a rate of at most 100 per second. But the chances that
> the valid 42.42.42.42 GARP is dropped is really high.
>
> If we instead use the following approach:
> a. Punt only useful ARPs to controller.
> b. Rate limit ARPs that are sent to controller.
>
> Then ARP packets outside 42.42.42./24 are never punted to the
> controller and don't consume any rate limiting tokens. For the second
> case, when an attacker would flood with valid ARP packets we would
> have the rate limit in place to protect the controller CPU.
>
> My commit addresses point "a" above as I think point "b" should be
> done in a generic way for all protocol packets that need to reach the
> controller.
>
> For dynamic routing protocols on the other hand I think we should not
> install routes with next-hops that are unreachable through other
> direct or indirect routes.
>
> Thanks,
> Dumitru

I agree that blindly ARP rate limit is not helpful, but a) is not really
helpful in this case either. In your example, the attacker can just use any
valid IP in 42.42.42.0/24 to send GARP flooding, which would result in
exactly same result that a useful GARP from 42.42.42.42 is dropped because
of blindly rate limiting all ARPs. To solve the problem properly, the ARP
rate limiting must be done per IP.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix inject-pkt command error response.

[Han Zhou]

On Mon, Aug 5, 2019 at 6:53 AM Numan Siddique  wrote:
>
>
>
> On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara  wrote:
>>
>> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou  wrote:
>> >
>> > From: Han Zhou 
>> >
>> > When using unixctl command inject-packet, it always respond with
>> > failure "server not ready", although the command was actually executed
>> > successfully.
>> >
>> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental
engine - quiet mode.")
>> > Signed-off-by: Han Zhou 
>>
>> Hi Han,
>>
>> I had already sent the exact same patch here :)
>> https://patchwork.ozlabs.org/patch/1140480/
>>
>> Nevertheless:
>> Acked-by: Dumitru Ceara 

Dumitru, I am so sorry that I missed that email even it had me in cc. I
just updated my filter rules to make sure I won't miss such email again.
Thank you for fixing so many issues recently!

>
>
> Thanks Han and Dumitru (for the review). I applied this patch to master
as this one had a test.
>
> Thanks
> Numan
>
>>
>> Thanks,
>> Dumitru
>>
>> > ---
>> >  controller/ovn-controller.c | 12 +++-
>> >  tests/ovn.at|  2 +-
>> >  2 files changed, 8 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> > index ad33d17..9df598f 100644
>> > --- a/controller/ovn-controller.c
>> > +++ b/controller/ovn-controller.c
>> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
>> >  unixctl_command_reply_error(pending_pkt.conn,
error);
>> >  free(error);
>> >  } else {
>> > -VLOG_DBG("Pending_pkt conn but br_int %p or
chassis "
>> > - "%p not ready. run-id: %"PRIu64,
br_int,
>> > - chassis, engine_run_id);
>> > -unixctl_command_reply_error(pending_pkt.conn,
>> > -"ovn-controller not ready.");
>> > +unixctl_command_reply(pending_pkt.conn, NULL);
>> >  }
>> > +} else {
>> > +VLOG_DBG("Pending_pkt conn but br_int %p or
chassis "
>> > + "%p not ready. run-id: %"PRIu64, br_int,
>> > + chassis, engine_run_id);
>> > +unixctl_command_reply_error(pending_pkt.conn,
>> > +"ovn-controller not ready.");
>> >  }
>> >  pending_pkt.conn = NULL;
>> >  free(pending_pkt.flow_s);
>> > diff --git a/tests/ovn.at b/tests/ovn.at
>> > index e88cffa..b85e549 100644
>> > --- a/tests/ovn.at
>> > +++ b/tests/ovn.at
>> > @@ -3723,7 +3723,7 @@ sleep 1
>> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac &&
eth.dst==$rp_ls1_mac &&
>> >  ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip &&
ip4.dst==$ls2_lp1_ip &&
>> >  udp && udp.src==53 && udp.dst==4369"
>> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
>> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
>> >
>> >
>> >  echo "-NB dump-"
>> > --
>> > 2.1.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

[ovs-dev] [PATCH v2 ovn] Build OVN using external OVS directory

[nusiddiq]

From: Numan Siddique 

With this patch we have to configure OVN to refer to external OVS source/build
directory instead of the ovs subtree.

The new configuration options added are:
 * --with-ovs-source=/path/to/ovs/source/dir
 * --with-ovs-build=/path/to/ovs/build/dir

Before configuring OVN, user should configure and compile OVS. If the user has
configured OVS on a different directory than the source dir, then 
'with-ovs-build'
should be specified.

If ovs-build dir is not defined, then ovs-source is used.

An upcoming patch will delete the ovs subtree.

Example usage:
  $ # Clone OVS repo
  $cd /home/foo/ovs
  $./boot.sh
  $mkdir _gcc
  $cd _gcc && ../configure && cd ..
  $make -C _gcc

  $ # Clone OVN repo
  $cd /home/foo/ovn
  $./boot.sh
  $./configure --with-ovs-source=/home/foo/ovs/ 
--with-ovs-build=/home/foo/ovs/_gcc
  $make

Acked-by: Lucas Alvares Gomes 
Signed-off-by: Numan Siddique 
---
v1 -> v2
===
  * Travis CI builds were failing as "make distcheck" was not working as
expected. Fixed it in v2.


 .travis/linux-build.sh  |  17 ++-
 Documentation/intro/install/general.rst |  33 -
 Makefile.am |  24 ++--
 acinclude.m4|  35 ++
 configure.ac|  29 ++---
 controller-vtep/automake.mk |   2 +-
 include/ovn/version.h.in|  28 +
 lib/ovsdb_automake.mk   |   7 +-
 tests/automake.mk   |   6 +-
 tests/ofproto-macros.at |   4 +-
 tests/ovn-controller-vtep.at|   4 +-
 tests/ovn.at| 158 
 tests/ovsdb-macros.at   |   2 +-
 13 files changed, 218 insertions(+), 131 deletions(-)
 create mode 100644 include/ovn/version.h.in

diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index a20474345..6f8d77ff4 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -10,7 +10,18 @@ TARGET="x86_64-native-linuxapp-gcc"
 
 function configure_ovs()
 {
+git clone https://github.com/openvswitch/ovs.git ovs_src
+pushd ovs_src
 ./boot.sh && ./configure $* || { cat config.log; exit 1; }
+make -j4
+popd
+}
+
+function configure_ovn()
+{
+configure_ovs
+./boot.sh && ./configure --with-ovs-source=$PWD/ovs_src $* || \
+{ cat config.log; exit 1; }
 }
 
 OPTS="$EXTRA_OPTS $*"
@@ -28,16 +39,16 @@ fi
 if [ "$TESTSUITE" ]; then
 # 'distcheck' will reconfigure with required options.
 # Now we only need to prepare the Makefile without sparse-wrapped CC.
-configure_ovs
+configure_ovn
 
-export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
+export DISTCHECK_CONFIGURE_FLAGS="$OPTS --with-ovs-source=$PWD/ovs_src"
 if ! make distcheck -j4 TESTSUITEFLAGS="-j4 -k ovn" RECHECK=yes; then
 # testsuite.log is necessary for debugging.
 cat */_build/tests/testsuite.log
 exit 1
 fi
 else
-configure_ovs $OPTS
+configure_ovn $OPTS
 make selinux-policy
 
 make -j4
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 99d8fec04..ab1cf57ed 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -42,9 +42,8 @@ out.  This is the right branch for general development.
 
 As of now there are no official OVN releases.
 
-Although building OVN, also builds OVS, it is recommended to clone
-and build OVS from its own repo. Please see the Open vSwitch
-documentation to build and install OVS.
+Before building OVN you should configure and build OVS.
+Please see the Open vSwitch documentation to build and install OVS.
 
 .. _general-build-reqs:
 
@@ -143,16 +142,24 @@ the "configure" script::
 
 $ ./boot.sh
 
+Before configuring OVN, clone, configure and build Open vSwitch.
+
 .. _general-configuring:
 
 Configuring
 ---
 
-Configure the package by running the configure script. You can usually
-invoke configure without any arguments. For example::
+Configure the package by running the configure script. You need to
+invoke configure with atleast the argument --with-ovs-source.
+For example::
+
+$ ./configure --with-ovs-source=/path/to/ovs/source
 
-$ ./configure
+If you have built Open vSwitch in a separate directory, then you
+need to provide that path in the option - --with-ovs-build.
 
+As of now, OVN uses all the run time directory of Open vSwitch. This
+will be changed to ``ovn`` specific directories.
 By default all files are installed under ``/usr/local``. OVN and Open vSwitch
 also expects to find its database in ``/usr/local/etc/openvswitch`` by default.
 If you want to install all files into, e.g., ``/usr`` and ``/var`` instead of
@@ -272,6 +279,20 @@ you wish to link with jemalloc add it to LIBS::
 
 $ ./configure LIBS=-ljemalloc
 
+Example usage::
+$ # Clone OVS repo
+$cd /home/foo/ovs
+$./boot.sh
+$mkdir _gcc
+$cd _gcc && ../configure && 

Re: [ovs-dev] [PATCH v2 ovn] fix default L4 default proto reported by ovn-nbctl

[Numan Siddique]

On Mon, Aug 5, 2019 at 3:55 PM Lorenzo Bianconi 
wrote:

> If no protcol is specified defining a load balancing rule TCP is
> selected as defualt but ovn-nbctl lb-list reports 'tcp/udp' in this
> case. Fix it reporting tcp in this case
>
> Fixes: e2bfcad6cbb0 ("ovn-nbctl: Add LB commands")
> Acked-by: Dumitru Ceara 
> Signed-off-by: Lorenzo Bianconi 
>

Thanks Lorenzo (and Dumitru). I applied this to master fixing a typo in the
commit message - s/defualt/default

Numan

---
> Changes since v1:
> - fix LB ovn-nbctl unit-tests
> ---
>  tests/ovn-nbctl.at| 24 
>  utilities/ovn-nbctl.c |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index a19e33f5e..cf06966ed 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -657,8 +657,8 @@ AT_CHECK([ovn-nbctl lb-list | uuidfilt], [0], [dnl
>  UUIDLB  PROTO
> VIP IPs
>  <0>lb0 tcp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
>  <1>lb1 udp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:8080
> -<2>lb2 tcp/udp30.0.0.30   192.168.10.10
> -<3>lb3 tcp/udp30.0.0.30   192.168.10.10
> +<2>lb2 tcp30.0.0.30   192.168.10.10
> +<3>lb3 tcp30.0.0.30   192.168.10.10
>  ])
>  AT_CHECK([ovn-nbctl lb-del lb2 30.0.0.30])
>  AT_CHECK([ovn-nbctl lb-del lb3 30.0.0.30])
> @@ -710,14 +710,14 @@ AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0],
> [dnl
>  UUIDLB  PROTO
> VIP IPs
>  <0>lb0 tcp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
>  <1>lb1 udp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
> -<2>lb3 tcp/udp30.0.0.10
>  192.168.10.10,192.168.10.20
> +<2>lb3 tcp30.0.0.10
>  192.168.10.10,192.168.10.20
>  ])
>
>  AT_CHECK([ovn-nbctl ls-lb-del ls0 lb0])
>  AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
>  UUIDLB  PROTO
> VIP IPs
>  <0>lb1 udp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
> -<1>lb3 tcp/udp30.0.0.10
>  192.168.10.10,192.168.10.20
> +<1>lb3 tcp30.0.0.10
>  192.168.10.10,192.168.10.20
>  ])
>
>  AT_CHECK([ovn-nbctl ls-lb-del ls0 lb1])
> @@ -746,14 +746,14 @@ AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0],
> [dnl
>  UUIDLB  PROTO
> VIP IPs
>  <0>lb0 tcp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
>  <1>lb1 udp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
> -<2>lb3 tcp/udp30.0.0.10
>  192.168.10.10,192.168.10.20
> +<2>lb3 tcp30.0.0.10
>  192.168.10.10,192.168.10.20
>  ])
>
>  AT_CHECK([ovn-nbctl lr-lb-del lr0 lb0])
>  AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [dnl
>  UUIDLB  PROTO
> VIP IPs
>  <0>lb1 udp30.0.0.10:80192.168.10.10:80,
> 192.168.10.20:80
> -<1>lb3 tcp/udp30.0.0.10
>  192.168.10.10,192.168.10.20
> +<1>lb3 tcp30.0.0.10
>  192.168.10.10,192.168.10.20
>  ])
>
>  AT_CHECK([ovn-nbctl lr-lb-del lr0 lb1])
> @@ -924,8 +924,8 @@ AT_CHECK([ovn-nbctl lb-list | uuidfilt], [0], [dnl
>  UUIDLB  PROTO
> VIP  IPs
>  <0>lb0 tcp[[ae0f::10]]:80
> [[fd0f::10]]:80,[[fd0f::20]]:80
>  <1>lb1 udp[[ae0f::10]]:80
> [[fd0f::10]]:80,[[fd0f::20]]:8080
> -<2>lb2 tcp/udpae0f::30 fd0f::10
> -<3>lb3 tcp/udpae0f::30 fd0f::10
> +<2>lb2 tcpae0f::30 fd0f::10
> +<3>lb3 tcpae0f::30 fd0f::10
>  ])
>  AT_CHECK([ovn-nbctl lb-del lb2 ae0f::30])
>  AT_CHECK([ovn-nbctl lb-del lb3 ae0f::30])
> @@ -977,14 +977,14 @@ AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0],
> [dnl
>  UUIDLB  PROTO
> VIP  IPs
>  <0>lb0 tcp[[ae0f::10]]:80
> [[fd0f::10]]:80,[[fd0f::20]]:80
>  <1>lb1 udp[[ae0f::10]]:80
> [[fd0f::10]]:80,[[fd0f::20]]:80
> -<2>lb3 tcp/udpae0f::10 fd0f::10,fd0f::20
> +<2>lb3 tcpae0f::10 fd0f::10,fd0f::20
>  ])
>
>  AT_CHECK([ovn-nbctl ls-lb-del ls0 lb0])
>  AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
>  UUID 

Re: [ovs-dev] [PATCH v2 ovn] Encode the virtual port key in vport_bind action in network byte order

[Dumitru Ceara]

On Mon, Aug 5, 2019 at 3:50 PM  wrote:
>
> From: Numan Siddique 
>
> The commit [1] encoded the vport key using uint32_t and the test case
> "action parsing" is failing for s380 arch.
>
> This patch fixes this issue by encoding the vport key in the network byte
> order.
>
> [1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
> Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")
>
> Signed-off-by: Numan Siddique 

Looks good to me. Thanks!

Acked-by: Dumitru Ceara 

> ---
> v1 -> v2
> ===
>   * There was a sparse compilation error when I missed checking when
> submitting v1.  Corrected it.
>
>
>  controller/pinctrl.c | 11 ++-
>  lib/actions.c|  3 ++-
>  tests/ovn.at |  2 +-
>  3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f05579fcc..f27718f55 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
>  uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
>
>  /* Get the virtual port key from the userdata buffer. */
> -uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
> +ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
>
> -if (!vport_key) {
> +if (!vp_key) {
>  return;
>  }
>
> -uint32_t hash = hash_2words(dp_key, *vport_key);
> +uint32_t vport_key = ntohl(*vp_key);
> +uint32_t hash = hash_2words(dp_key, vport_key);
>
>  struct put_vport_binding *vpb
> -= pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
> += pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
>  if (!vpb) {
>  if (hmap_count(_vport_bindings) >= 1000) {
>  COVERAGE_INC(pinctrl_drop_put_vport_binding);
> @@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
>  }
>
>  vpb->dp_key = dp_key;
> -vpb->vport_key = *vport_key;
> +vpb->vport_key = vport_key;
>  vpb->vport_parent_key = vport_parent_key;
>
>  notify_pinctrl_main();
> diff --git a/lib/actions.c b/lib/actions.c
> index 66916a837..b0cb3490b 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
>  size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
>false, NX_CTLR_NO_METER,
>ofpacts);
> -ofpbuf_put(ofpacts, _key, sizeof(uint32_t));
> +ovs_be32 vp_key = htonl(vport_key);
> +ofpbuf_put(ofpacts, _key, sizeof(ovs_be32));
>  encode_finish_controller_op(oc_offset, ofpacts);
>  encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
>  }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e88cffa20..344efad26 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
>  # bind_vport
>  # lsp1's port key is 0x11.
>  bind_vport("lsp1", inport);
> -encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
> +encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
>  # lsp2 doesn't exist. So it should be encoded as drop.
>  bind_vport("lsp2", inport);
>  encodes as drop
> --
> 2.21.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

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix inject-pkt command error response.

[Numan Siddique]

On Mon, Aug 5, 2019 at 1:43 PM Dumitru Ceara  wrote:

> On Mon, Aug 5, 2019 at 9:29 AM Han Zhou  wrote:
> >
> > From: Han Zhou 
> >
> > When using unixctl command inject-packet, it always respond with
> > failure "server not ready", although the command was actually executed
> > successfully.
> >
> > Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine
> - quiet mode.")
> > Signed-off-by: Han Zhou 
>
> Hi Han,
>
> I had already sent the exact same patch here :)
> https://patchwork.ozlabs.org/patch/1140480/
>
> Nevertheless:
> Acked-by: Dumitru Ceara 
>

Thanks Han and Dumitru (for the review). I applied this patch to master as
this one had a test.

Thanks
Numan


> Thanks,
> Dumitru
>
> > ---
> >  controller/ovn-controller.c | 12 +++-
> >  tests/ovn.at|  2 +-
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index ad33d17..9df598f 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
> >  unixctl_command_reply_error(pending_pkt.conn,
> error);
> >  free(error);
> >  } else {
> > -VLOG_DBG("Pending_pkt conn but br_int %p or
> chassis "
> > - "%p not ready. run-id: %"PRIu64,
> br_int,
> > - chassis, engine_run_id);
> > -unixctl_command_reply_error(pending_pkt.conn,
> > -"ovn-controller not ready.");
> > +unixctl_command_reply(pending_pkt.conn, NULL);
> >  }
> > +} else {
> > +VLOG_DBG("Pending_pkt conn but br_int %p or chassis
> "
> > + "%p not ready. run-id: %"PRIu64, br_int,
> > + chassis, engine_run_id);
> > +unixctl_command_reply_error(pending_pkt.conn,
> > +"ovn-controller not ready.");
> >  }
> >  pending_pkt.conn = NULL;
> >  free(pending_pkt.flow_s);
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index e88cffa..b85e549 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -3723,7 +3723,7 @@ sleep 1
> >  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac &&
> eth.dst==$rp_ls1_mac &&
> >  ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip &&
> ip4.dst==$ls2_lp1_ip &&
> >  udp && udp.src==53 && udp.dst==4369"
> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> > +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
> >
> >
> >  echo "-NB dump-"
> > --
> > 2.1.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

[ovs-dev] [PATCH v2 ovn] Encode the virtual port key in vport_bind action in network byte order

[nusiddiq]

From: Numan Siddique 

The commit [1] encoded the vport key using uint32_t and the test case
"action parsing" is failing for s380 arch.

This patch fixes this issue by encoding the vport key in the network byte
order.

[1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")

Signed-off-by: Numan Siddique 
---
v1 -> v2
===
  * There was a sparse compilation error when I missed checking when
submitting v1.  Corrected it.


 controller/pinctrl.c | 11 ++-
 lib/actions.c|  3 ++-
 tests/ovn.at |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579fcc..f27718f55 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
 uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
 
 /* Get the virtual port key from the userdata buffer. */
-uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
+ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
 
-if (!vport_key) {
+if (!vp_key) {
 return;
 }
 
-uint32_t hash = hash_2words(dp_key, *vport_key);
+uint32_t vport_key = ntohl(*vp_key);
+uint32_t hash = hash_2words(dp_key, vport_key);
 
 struct put_vport_binding *vpb
-= pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
+= pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
 if (!vpb) {
 if (hmap_count(_vport_bindings) >= 1000) {
 COVERAGE_INC(pinctrl_drop_put_vport_binding);
@@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
 }
 
 vpb->dp_key = dp_key;
-vpb->vport_key = *vport_key;
+vpb->vport_key = vport_key;
 vpb->vport_parent_key = vport_parent_key;
 
 notify_pinctrl_main();
diff --git a/lib/actions.c b/lib/actions.c
index 66916a837..b0cb3490b 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
 size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
   false, NX_CTLR_NO_METER,
   ofpacts);
-ofpbuf_put(ofpacts, _key, sizeof(uint32_t));
+ovs_be32 vp_key = htonl(vport_key);
+ofpbuf_put(ofpacts, _key, sizeof(ovs_be32));
 encode_finish_controller_op(oc_offset, ofpacts);
 encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa20..344efad26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
 # bind_vport
 # lsp1's port key is 0x11.
 bind_vport("lsp1", inport);
-encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
+encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
 # lsp2 doesn't exist. So it should be encoded as drop.
 bind_vport("lsp2", inport);
 encodes as drop
-- 
2.21.0

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

Re: [ovs-dev] [PATCH ovn] ovn-northd: Use HMAP_FOR_EACH when adding multicast lflows

[Numan Siddique]

On Thu, Aug 1, 2019 at 5:09 PM Dumitru Ceara  wrote:

> There's no need to use HMAP_FOR_EACH_SAFE when we add the logical flows
> that correspond to IGMP groups as we don't modify the hashmap in the
> loop.
>
> Also, we should reinitialize match and actions only if the flow will be
> added.
>
> Fixes: ddc64665b678 ("OVN: Add ovn-northd IGMP support")
> Signed-off-by: Dumitru Ceara 
>

Acked-by: Numan Siddique 



> ---
>  northd/ovn-northd.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index dabd422..880773b 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5258,12 +5258,9 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
>
>  /* Ingress table 17: Add IP multicast flows learnt from IGMP
>   * (priority 90). */
> -struct ovn_igmp_group *igmp_group, *next_igmp_group;
> -
> -HMAP_FOR_EACH_SAFE (igmp_group, next_igmp_group, hmap_node,
> igmp_groups) {
> -ds_clear();
> -ds_clear();
> +struct ovn_igmp_group *igmp_group;
>
> +HMAP_FOR_EACH (igmp_group, hmap_node, igmp_groups) {
>  if (!igmp_group->datapath) {
>  continue;
>  }
> @@ -5275,6 +5272,9 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
>  }
>  mcast_info->active_flows++;
>
> +ds_clear();
> +ds_clear();
> +
>  ds_put_format(, "eth.mcast && ip4 && ip4.dst == %s ",
>igmp_group->mcgroup.name);
>  ds_put_format(, "outport = \"%s\"; output; ",
> --
> 1.8.3.1
>
> ___
> 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

[ovs-dev] [PATCH ovn] Encode the virtual port key in vport_bind action in network byte order

[nusiddiq]

From: Numan Siddique 

The commit [1] encoded the vport key using uint32_t and the test case
"action parsing" is failing for s380 arch.

This patch fixes this issue by encoding the vport key in the network byte
order.

[1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")

Signed-off-by: Numan Siddique 
---
 controller/pinctrl.c | 11 ++-
 lib/actions.c|  3 ++-
 tests/ovn.at |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579fcc..f27718f55 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
 uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
 
 /* Get the virtual port key from the userdata buffer. */
-uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
+ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
 
-if (!vport_key) {
+if (!vp_key) {
 return;
 }
 
-uint32_t hash = hash_2words(dp_key, *vport_key);
+uint32_t vport_key = ntohl(*vp_key);
+uint32_t hash = hash_2words(dp_key, vport_key);
 
 struct put_vport_binding *vpb
-= pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
+= pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
 if (!vpb) {
 if (hmap_count(_vport_bindings) >= 1000) {
 COVERAGE_INC(pinctrl_drop_put_vport_binding);
@@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
 }
 
 vpb->dp_key = dp_key;
-vpb->vport_key = *vport_key;
+vpb->vport_key = vport_key;
 vpb->vport_parent_key = vport_parent_key;
 
 notify_pinctrl_main();
diff --git a/lib/actions.c b/lib/actions.c
index 66916a837..0b90da772 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
 size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
   false, NX_CTLR_NO_METER,
   ofpacts);
-ofpbuf_put(ofpacts, _key, sizeof(uint32_t));
+ovs_be32 vp_key = htonl(vp_key);
+ofpbuf_put(ofpacts, _key, sizeof(ovs_be32));
 encode_finish_controller_op(oc_offset, ofpacts);
 encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa20..344efad26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
 # bind_vport
 # lsp1's port key is 0x11.
 bind_vport("lsp1", inport);
-encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
+encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
 # lsp2 doesn't exist. So it should be encoded as drop.
 bind_vport("lsp2", inport);
 encodes as drop
-- 
2.21.0

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

[ovs-dev] [PATCH] Encode the virtual port key in vport_bind action in network byte order

[nusiddiq]

From: Numan Siddique 

The commit [1] encoded the vport key using uint32_t and the test case
"action parsing" is failing for s380 arch.

This patch fixes this issue by encoding the vport key in the network byte
order.

[1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")

Signed-off-by: Numan Siddique 
---
 controller/pinctrl.c | 11 ++-
 lib/actions.c|  3 ++-
 tests/ovn.at |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579fcc..f27718f55 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
 uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
 
 /* Get the virtual port key from the userdata buffer. */
-uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
+ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
 
-if (!vport_key) {
+if (!vp_key) {
 return;
 }
 
-uint32_t hash = hash_2words(dp_key, *vport_key);
+uint32_t vport_key = ntohl(*vp_key);
+uint32_t hash = hash_2words(dp_key, vport_key);
 
 struct put_vport_binding *vpb
-= pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
+= pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
 if (!vpb) {
 if (hmap_count(_vport_bindings) >= 1000) {
 COVERAGE_INC(pinctrl_drop_put_vport_binding);
@@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
 }
 
 vpb->dp_key = dp_key;
-vpb->vport_key = *vport_key;
+vpb->vport_key = vport_key;
 vpb->vport_parent_key = vport_parent_key;
 
 notify_pinctrl_main();
diff --git a/lib/actions.c b/lib/actions.c
index 66916a837..0b90da772 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
 size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
   false, NX_CTLR_NO_METER,
   ofpacts);
-ofpbuf_put(ofpacts, _key, sizeof(uint32_t));
+ovs_be32 vp_key = htonl(vp_key);
+ofpbuf_put(ofpacts, _key, sizeof(ovs_be32));
 encode_finish_controller_op(oc_offset, ofpacts);
 encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa20..344efad26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
 # bind_vport
 # lsp1's port key is 0x11.
 bind_vport("lsp1", inport);
-encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
+encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
 # lsp2 doesn't exist. So it should be encoded as drop.
 bind_vport("lsp2", inport);
 encodes as drop
-- 
2.21.0

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

[ovs-dev] [PATCH v2 ovn] fix default L4 default proto reported by ovn-nbctl

[Lorenzo Bianconi]

If no protcol is specified defining a load balancing rule TCP is
selected as defualt but ovn-nbctl lb-list reports 'tcp/udp' in this
case. Fix it reporting tcp in this case

Fixes: e2bfcad6cbb0 ("ovn-nbctl: Add LB commands")
Acked-by: Dumitru Ceara 
Signed-off-by: Lorenzo Bianconi 
---
Changes since v1:
- fix LB ovn-nbctl unit-tests
---
 tests/ovn-nbctl.at| 24 
 utilities/ovn-nbctl.c |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index a19e33f5e..cf06966ed 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -657,8 +657,8 @@ AT_CHECK([ovn-nbctl lb-list | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
IPs
 <0>lb0 tcp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
 <1>lb1 udp30.0.0.10:80
192.168.10.10:80,192.168.10.20:8080
-<2>lb2 tcp/udp30.0.0.30   192.168.10.10
-<3>lb3 tcp/udp30.0.0.30   192.168.10.10
+<2>lb2 tcp30.0.0.30   192.168.10.10
+<3>lb3 tcp30.0.0.30   192.168.10.10
 ])
 AT_CHECK([ovn-nbctl lb-del lb2 30.0.0.30])
 AT_CHECK([ovn-nbctl lb-del lb3 30.0.0.30])
@@ -710,14 +710,14 @@ AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
IPs
 <0>lb0 tcp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
 <1>lb1 udp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
-<2>lb3 tcp/udp30.0.0.10   
192.168.10.10,192.168.10.20
+<2>lb3 tcp30.0.0.10   
192.168.10.10,192.168.10.20
 ])
 
 AT_CHECK([ovn-nbctl ls-lb-del ls0 lb0])
 AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
IPs
 <0>lb1 udp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
-<1>lb3 tcp/udp30.0.0.10   
192.168.10.10,192.168.10.20
+<1>lb3 tcp30.0.0.10   
192.168.10.10,192.168.10.20
 ])
 
 AT_CHECK([ovn-nbctl ls-lb-del ls0 lb1])
@@ -746,14 +746,14 @@ AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
IPs
 <0>lb0 tcp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
 <1>lb1 udp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
-<2>lb3 tcp/udp30.0.0.10   
192.168.10.10,192.168.10.20
+<2>lb3 tcp30.0.0.10   
192.168.10.10,192.168.10.20
 ])
 
 AT_CHECK([ovn-nbctl lr-lb-del lr0 lb0])
 AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
IPs
 <0>lb1 udp30.0.0.10:80
192.168.10.10:80,192.168.10.20:80
-<1>lb3 tcp/udp30.0.0.10   
192.168.10.10,192.168.10.20
+<1>lb3 tcp30.0.0.10   
192.168.10.10,192.168.10.20
 ])
 
 AT_CHECK([ovn-nbctl lr-lb-del lr0 lb1])
@@ -924,8 +924,8 @@ AT_CHECK([ovn-nbctl lb-list | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
 IPs
 <0>lb0 tcp[[ae0f::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
 <1>lb1 udp[[ae0f::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:8080
-<2>lb2 tcp/udpae0f::30 fd0f::10
-<3>lb3 tcp/udpae0f::30 fd0f::10
+<2>lb2 tcpae0f::30 fd0f::10
+<3>lb3 tcpae0f::30 fd0f::10
 ])
 AT_CHECK([ovn-nbctl lb-del lb2 ae0f::30])
 AT_CHECK([ovn-nbctl lb-del lb3 ae0f::30])
@@ -977,14 +977,14 @@ AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
 IPs
 <0>lb0 tcp[[ae0f::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
 <1>lb1 udp[[ae0f::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
-<2>lb3 tcp/udpae0f::10 fd0f::10,fd0f::20
+<2>lb3 tcpae0f::10 fd0f::10,fd0f::20
 ])
 
 AT_CHECK([ovn-nbctl ls-lb-del ls0 lb0])
 AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [dnl
 UUIDLB  PROTO  VIP 
 IPs
 <0>lb1 udp[[ae0f::10]]:80
[[fd0f::10]]:80,[[fd0f::20]]:80
-<1>lb3 tcp/udpae0f::10 fd0f::10,fd0f::20
+<1>lb3 tcpae0f::10   

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix inject-pkt command error response.

[Dumitru Ceara]

On Mon, Aug 5, 2019 at 9:29 AM Han Zhou  wrote:
>
> From: Han Zhou 
>
> When using unixctl command inject-packet, it always respond with
> failure "server not ready", although the command was actually executed
> successfully.
>
> Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - 
> quiet mode.")
> Signed-off-by: Han Zhou 

Hi Han,

I had already sent the exact same patch here :)
https://patchwork.ozlabs.org/patch/1140480/

Nevertheless:
Acked-by: Dumitru Ceara 

Thanks,
Dumitru

> ---
>  controller/ovn-controller.c | 12 +++-
>  tests/ovn.at|  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index ad33d17..9df598f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
>  unixctl_command_reply_error(pending_pkt.conn, error);
>  free(error);
>  } else {
> -VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> - "%p not ready. run-id: %"PRIu64, br_int,
> - chassis, engine_run_id);
> -unixctl_command_reply_error(pending_pkt.conn,
> -"ovn-controller not ready.");
> +unixctl_command_reply(pending_pkt.conn, NULL);
>  }
> +} else {
> +VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
> + "%p not ready. run-id: %"PRIu64, br_int,
> + chassis, engine_run_id);
> +unixctl_command_reply_error(pending_pkt.conn,
> +"ovn-controller not ready.");
>  }
>  pending_pkt.conn = NULL;
>  free(pending_pkt.flow_s);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e88cffa..b85e549 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -3723,7 +3723,7 @@ sleep 1
>  packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac 
> &&
>  ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
>  udp && udp.src==53 && udp.dst==4369"
> -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> +AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
>
>
>  echo "-NB dump-"
> --
> 2.1.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

Re: [ovs-dev] [PATCH ovn v2] OVN: Fix learning of neighbors from ARP/ND packets.

[Dumitru Ceara]

On Mon, Aug 5, 2019 at 1:41 AM Han Zhou  wrote:
>
>
>
> On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara  wrote:
> >
> > Add a restriction on the target protocol addresses to match the
> > configured subnets. All other ARP/ND packets are invalid in this
> > context.
> >
> > One exception is for ARP replies that are received for route next-hops
> > that are only reachable via a port but can't be directly resolved
> > through route lookups. Such support was introduced by commit:
> >
> > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.")
> >
> > Reported-at: https://bugzilla.redhat.com/1729846
> > Reported-by: Haidong Li 
> > CC: Han Zhou 
> > CC: Guru Shetty 
> > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP 
> > request.")
> > Signed-off-by: Dumitru Ceara 
>
> Hi Dumitru,

Hi Han,

Thanks for reviewing this.

>
> Sorry for my slow response, and thanks a lot for revising the patch for a 
> bigger scope of validations. However, the exception of /32 address makes me 
> thinking more about this patch. If ARP replies is allowed from any nexthop 
> for a LR port with /32, at least ARP request for GARP purpose should also be 
> allowed. But before asking for a v3, I'd hold on to rethink the purpose of 
> this patch.

Right, we should allow GARP requests too. If we decide to go ahead
with this patch I'll add a function in v3 to handle all types of ARPs
and call it both for unreachable static route next-hops and attached
subnets.

>
> The nexthop specific flows are now from static routes. What if OVN support 
> dynamical routing protocols in the future? Of course we can then take those 
> dynamic nexthops into allowed peers. But then I am thinking what is the real 
> benefit of all these restrictions? Why can't we just have simpler logic to 
> handle all these situations without validation? I think the major benefit of 
> the validation is to avoid handling the noise ARP/NDs when multiple subnets 
> shares same L2, but most cases it is really not a big deal, right? For the 
> CPU problem caused by ARP flooding as mentioned by the bug report, it is a 
> real problem, but this patch seems not really helpful for that, because the 
> attacker can just trigger the same CPU problem with *valid* packets. So I am 
> not sure if the benefit of the change is worth the complexity it introduced. 
> Please share your thought and correct me if I missed something.
>
> Thanks,
> Han

I assume the simpler logic to handle all these situations without
validation is to add rate limiting for ARP packets that get punted to
the controller. I agree that this should be implemented too.

But I think rate limiting all ARP packets regardless of IP addresses
is not enough. In the following scenario and if we would have a way to
rate limit ARP packets:
- Subnet 42.42.42.0/24 configured on the OVN
- "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41
- Host 42.42.42.42 sends GARP.
- Rate limiting of ARP packets towards controller at 100pps

With the current code, ARP packets for 41.41.41.41 will be punted to
controller at a rate of at most 100 per second. But the chances that
the valid 42.42.42.42 GARP is dropped is really high.

If we instead use the following approach:
a. Punt only useful ARPs to controller.
b. Rate limit ARPs that are sent to controller.

Then ARP packets outside 42.42.42./24 are never punted to the
controller and don't consume any rate limiting tokens. For the second
case, when an attacker would flood with valid ARP packets we would
have the rate limit in place to protect the controller CPU.

My commit addresses point "a" above as I think point "b" should be
done in a generic way for all protocol packets that need to reach the
controller.

For dynamic routing protocols on the other hand I think we should not
install routes with next-hops that are unreachable through other
direct or indirect routes.

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

[ovs-dev] why action "meter" only can be specified once?

[杨�D]

Hi, all

 

I was told meter only can be specified once, but actually there is such case
existing, i.e. multiple flows share a total bandwidth, but every flow also
has its own bandwidth limit, by two meters, we can not only get every flow
stats but also get total stats, I think this is very reasonable user
scenario.

 

ovs-ofctl: instruction meter may be specified only once

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

[ovs-dev] [PATCH ovn] ovn-controller: Fix inject-pkt command error response.

[Han Zhou]

From: Han Zhou 

When using unixctl command inject-packet, it always respond with
failure "server not ready", although the command was actually executed
successfully.

Fixes: 0bd4d85c36ef ("ovn-controller: Initial use of incremental engine - quiet 
mode.")
Signed-off-by: Han Zhou 
---
 controller/ovn-controller.c | 12 +++-
 tests/ovn.at|  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index ad33d17..9df598f 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2079,12 +2079,14 @@ main(int argc, char *argv[])
 unixctl_command_reply_error(pending_pkt.conn, error);
 free(error);
 } else {
-VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
- "%p not ready. run-id: %"PRIu64, br_int,
- chassis, engine_run_id);
-unixctl_command_reply_error(pending_pkt.conn,
-"ovn-controller not ready.");
+unixctl_command_reply(pending_pkt.conn, NULL);
 }
+} else {
+VLOG_DBG("Pending_pkt conn but br_int %p or chassis "
+ "%p not ready. run-id: %"PRIu64, br_int,
+ chassis, engine_run_id);
+unixctl_command_reply_error(pending_pkt.conn,
+"ovn-controller not ready.");
 }
 pending_pkt.conn = NULL;
 free(pending_pkt.flow_s);
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa..b85e549 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3723,7 +3723,7 @@ sleep 1
 packet="inport==\"ls1-lp1\" && eth.src==$ls1_lp1_mac && eth.dst==$rp_ls1_mac &&
 ip4 && ip.ttl==64 && ip4.src==$ls1_lp1_ip && ip4.dst==$ls2_lp1_ip &&
 udp && udp.src==53 && udp.dst==4369"
-as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+AT_CHECK([as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"])
 
 
 echo "-NB dump-"
-- 
2.1.0

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

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix IP engine run with in-flight messages

[Dumitru Ceara]

On Mon, Aug 5, 2019 at 8:52 AM Numan Siddique  wrote:
>
>
>
> On Mon, Aug 5, 2019 at 2:11 AM Han Zhou  wrote:
>>
>> On Fri, Aug 2, 2019 at 2:23 AM Dumitru Ceara  wrote:
>> >
>> > When trying to incrementally process changes even if there are in-flight
>> > messages we were incorrectly setting the engine_aborted variable to the
>> > value returned by engine_run. However, engine_run returns true if the
>> > run was completed.
>> >
>> > This was causing discrepancies between logical flows and openflow flows
>> > due to the fact that the rerun wasn't happening after an aborted run.
>> >
>> > In order to avoid confusion the engine_aborted variable is now renamed to
>> > engine_run_done thus avoiding the negated logic.
>> >
>> > CC: Han Zhou 
>> > Fixes: a6b7d9f4f047 ("ovn-controller: Fix flow installation latency
>> caused by recompute.")
>> > Signed-off-by: Dumitru Ceara 
>> > ---
>> >  controller/ovn-controller.c | 14 +++---
>> >  1 file changed, 7 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> > index ad33d17..15b9f4e 100644
>> > --- a/controller/ovn-controller.c
>> > +++ b/controller/ovn-controller.c
>> > @@ -1896,7 +1896,7 @@ main(int argc, char *argv[])
>> >
>> >  uint64_t engine_run_id = 0;
>> >  uint64_t old_engine_run_id = 0;
>> > -bool engine_aborted = false;
>> > +bool engine_run_done = true;
>> >
>> >  unsigned int ovs_cond_seqno = UINT_MAX;
>> >  unsigned int ovnsb_cond_seqno = UINT_MAX;
>> > @@ -1997,14 +1997,14 @@ main(int argc, char *argv[])
>> >   * this round of engine_run and continue
>> processing
>> >   * acculated changes incrementally later when
>> >   * ofctrl_can_put() returns true. */
>> > -if (!engine_aborted) {
>> > +if (engine_run_done) {
>> >  engine_set_abort_recompute(true);
>> > -engine_aborted =
>> engine_run(_flow_output,
>> > -
>>  ++engine_run_id);
>> > +engine_run_done =
>> engine_run(_flow_output,
>> > +
>> ++engine_run_id);
>> >  }
>> >  } else {
>> >  engine_set_abort_recompute(false);
>> > -engine_aborted = false;
>> > +engine_run_done = true;
>> >  engine_run(_flow_output, ++engine_run_id);
>> >  }
>> >  }
>> > @@ -2048,8 +2048,8 @@ main(int argc, char *argv[])
>> >  }
>> >
>> >  }
>> > -if (old_engine_run_id == engine_run_id || engine_aborted) {
>> > -if (engine_aborted || engine_need_run(_flow_output)) {
>> > +if (old_engine_run_id == engine_run_id || !engine_run_done) {
>> > +if (!engine_run_done ||
>> engine_need_run(_flow_output)) {
>> >  VLOG_DBG("engine did not run, force recompute next
>> time: "
>> >   "br_int %p, chassis %p", br_int, chassis);
>> >  engine_set_force_recompute(true);
>> > --
>> > 1.8.3.1
>> >
>> > ___
>> > dev mailing list
>> > d...@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>> Acked-by: Han Zhou 
>
>
>
> Thanks.
> I applied this patch to master.
>
> Numan
>
>>
>>
>> Thanks Dumitru for this critical fix! It was my silly mistake. I tried to
>> reproduce the test case failure you reported, but it still didn't happen on
>> my laptop, even with -j10. Later we may need to add a test case that
>> triggers a lot of flows being installed so that the scenario with in-flight
>> messages are easily covered.
>>
>> The original patch is still waiting to be backported to 2.12. I will send a
>> new version followed by your fix on 2.12.
>> ___


Thanks Han, Mark, Numan for reviewing and testing the patch!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ovn-controller: Fix IP engine run with in-flight messages

[Numan Siddique]

On Mon, Aug 5, 2019 at 2:11 AM Han Zhou  wrote:

> On Fri, Aug 2, 2019 at 2:23 AM Dumitru Ceara  wrote:
> >
> > When trying to incrementally process changes even if there are in-flight
> > messages we were incorrectly setting the engine_aborted variable to the
> > value returned by engine_run. However, engine_run returns true if the
> > run was completed.
> >
> > This was causing discrepancies between logical flows and openflow flows
> > due to the fact that the rerun wasn't happening after an aborted run.
> >
> > In order to avoid confusion the engine_aborted variable is now renamed to
> > engine_run_done thus avoiding the negated logic.
> >
> > CC: Han Zhou 
> > Fixes: a6b7d9f4f047 ("ovn-controller: Fix flow installation latency
> caused by recompute.")
> > Signed-off-by: Dumitru Ceara 
> > ---
> >  controller/ovn-controller.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index ad33d17..15b9f4e 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1896,7 +1896,7 @@ main(int argc, char *argv[])
> >
> >  uint64_t engine_run_id = 0;
> >  uint64_t old_engine_run_id = 0;
> > -bool engine_aborted = false;
> > +bool engine_run_done = true;
> >
> >  unsigned int ovs_cond_seqno = UINT_MAX;
> >  unsigned int ovnsb_cond_seqno = UINT_MAX;
> > @@ -1997,14 +1997,14 @@ main(int argc, char *argv[])
> >   * this round of engine_run and continue
> processing
> >   * acculated changes incrementally later
> when
> >   * ofctrl_can_put() returns true. */
> > -if (!engine_aborted) {
> > +if (engine_run_done) {
> >  engine_set_abort_recompute(true);
> > -engine_aborted =
> engine_run(_flow_output,
> > -
>  ++engine_run_id);
> > +engine_run_done =
> engine_run(_flow_output,
> > +
> ++engine_run_id);
> >  }
> >  } else {
> >  engine_set_abort_recompute(false);
> > -engine_aborted = false;
> > +engine_run_done = true;
> >  engine_run(_flow_output,
> ++engine_run_id);
> >  }
> >  }
> > @@ -2048,8 +2048,8 @@ main(int argc, char *argv[])
> >  }
> >
> >  }
> > -if (old_engine_run_id == engine_run_id || engine_aborted) {
> > -if (engine_aborted || engine_need_run(_flow_output))
> {
> > +if (old_engine_run_id == engine_run_id || !engine_run_done)
> {
> > +if (!engine_run_done ||
> engine_need_run(_flow_output)) {
> >  VLOG_DBG("engine did not run, force recompute next
> time: "
> >   "br_int %p, chassis %p", br_int, chassis);
> >  engine_set_force_recompute(true);
> > --
> > 1.8.3.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Acked-by: Han Zhou 
>


Thanks.
I applied this patch to master.

Numan


>
> Thanks Dumitru for this critical fix! It was my silly mistake. I tried to
> reproduce the test case failure you reported, but it still didn't happen on
> my laptop, even with -j10. Later we may need to add a test case that
> triggers a lot of flows being installed so that the scenario with in-flight
> messages are easily covered.
>
> The original patch is still waiting to be backported to 2.12. I will send a
> new version followed by your fix on 2.12.
> ___
> 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