[ovs-dev] FW: [Scan] 2016-08-13 13:52:28

[Vickie]

-Original Message-
From: "Vickie" 
Sent: 2016-08-13 13:52:28
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 13:52:28


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [RFC v2 5/5] ofproto-dpif-upcall: Add instant revalidation.

[Daniele Di Proietto]

Sometimes the ofproto layer creates a flow which is not liked by the
revalidation for various reasons.  This behavior, while not critical
might impact the performance.  This commit adds a facility to detect
this issue early and log a warning.

The detection is done by revalidating a flow as soon as it is installed.
Since this extra revalidation might be expensive it is disabled by
default.  It can be enabled using the undocumented unixctl:

`ovs-appctl upcall/instant-revalidation sample_rate`

where 'sample_rate' is a number between 0 and 100, meaning the
percentage of newly installed flows that are instantly revalidated.

We explicitly enable the feature in the unit and system tests (except
for three MPLS unit tests, which expose a problem that hasn't been fixed
yet);

Signed-off-by: Daniele Di Proietto 
---
While I've been using this to detect some bugs in the testsuite and in a
real deployment, I still consider this an RFC.
---
 ofproto/ofproto-dpif-upcall.c | 171 ++
 tests/ofproto-dpif.at |  15 
 tests/ofproto-macros.at   |   2 +
 3 files changed, 174 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e447308..264e2af 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -310,6 +310,9 @@ static void revalidate(struct revalidator *);
 static void revalidator_pause(struct revalidator *);
 static void revalidator_sweep(struct revalidator *);
 static void revalidator_purge(struct revalidator *);
+static void instant_revalidate_upcall(struct udpif *udpif,
+  struct upcall *upcall,
+  ovs_version_t old_version);
 static void upcall_unixctl_show(struct unixctl_conn *conn, int argc,
 const char *argv[], void *aux);
 static void upcall_unixctl_disable_megaflows(struct unixctl_conn *, int argc,
@@ -326,6 +329,9 @@ static void upcall_unixctl_dump_wait(struct unixctl_conn 
*conn, int argc,
  const char *argv[], void *aux);
 static void upcall_unixctl_purge(struct unixctl_conn *conn, int argc,
  const char *argv[], void *aux);
+static void upcall_unixctl_instant_revalidation(struct unixctl_conn *conn,
+int argc, const char *argv[],
+void *aux);
 
 static struct udpif_key *ukey_create_from_upcall(struct upcall *,
  struct flow_wildcards *);
@@ -359,6 +365,7 @@ static dp_purge_callback dp_purge_cb;
 
 static atomic_bool enable_megaflows = ATOMIC_VAR_INIT(true);
 static atomic_bool enable_ufid = ATOMIC_VAR_INIT(true);
+static atomic_int instant_revalidation_rate = ATOMIC_VAR_INIT(0);
 
 void
 udpif_init(void)
@@ -381,6 +388,9 @@ udpif_init(void)
  upcall_unixctl_dump_wait, NULL);
 unixctl_command_register("revalidator/purge", "", 0, 0,
  upcall_unixctl_purge, NULL);
+unixctl_command_register("upcall/instant-revalidation",
+ "sample_rate", 1, 1,
+ upcall_unixctl_instant_revalidation, NULL);
 ovsthread_once_done();
 }
 }
@@ -728,6 +738,8 @@ udpif_upcall_handler(void *arg)
 return NULL;
 }
 
+static bool should_install_flow(struct udpif *udpif, struct upcall *upcall);
+
 static size_t
 recv_upcalls(struct handler *handler)
 {
@@ -745,6 +757,7 @@ recv_upcalls(struct handler *handler)
 struct dpif_upcall *dupcall = [n_upcalls];
 struct upcall *upcall = [n_upcalls];
 struct flow *flow = [n_upcalls];
+ovs_version_t old_version;
 unsigned int mru;
 int error;
 
@@ -794,12 +807,15 @@ recv_upcalls(struct handler *handler)
 pkt_metadata_from_flow(>packet.md, flow);
 flow_extract(>packet, flow);
 
+old_version = ofproto_dpif_get_tables_version(upcall->ofproto);
 error = process_upcall(udpif, upcall,
>odp_actions, >wc);
 if (error) {
 goto cleanup;
 }
 
+instant_revalidate_upcall(udpif, upcall, old_version);
+
 n_upcalls++;
 continue;
 
@@ -1172,6 +1188,8 @@ upcall_cb(const struct dp_packet *packet, const struct 
flow *flow, ovs_u128 *ufi
 bool megaflow;
 int error;
 
+ovs_version_t old_version;
+
 atomic_read_relaxed(_megaflows, );
 
 error = upcall_receive(, udpif->backer, packet, type, userdata,
@@ -1180,6 +1198,8 @@ upcall_cb(const struct dp_packet *packet, const struct 
flow *flow, ovs_u128 *ufi
 return error;
 }
 
+old_version = ofproto_dpif_get_tables_version(upcall.ofproto);
+
 error = process_upcall(udpif, , actions, wc);
 if (error) {
 goto out;
@@ -1202,7 +1222,11 @@ 

[ovs-dev] [PATCH v2 3/5] tnl-neigh-cache: Unwildcard flow members before inspecting them.

[Daniele Di Proietto]

tnl_neigh_snoop() is part of the translation.  During translation we
have to unwildcard all the fields we examine to make a decision.

tnl_arp_snoop() and tnl_nd_snoop() failed to unwildcard fileds in case
of failure.  The solution is to do unwildcarding before the field is
inspected.

Signed-off-by: Daniele Di Proietto 
---
 lib/flow.h|  7 +++
 lib/tnl-neigh-cache.c | 22 --
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index 5b83695..ea24e28 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -854,6 +854,13 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const 
struct flow *flow)
 md->ct_label = flow->ct_label;
 }
 
+/* Often, during translation we need to read a value from a flow('FLOW') and
+ * unwildcard the corresponding bits in the wildcards('WC').  This macro makes
+ * it easier to do that. */
+
+#define FLOW_WC_GET_AND_MASK_WC(FLOW, WC, FIELD) \
+(((WC) ? WC_MASK_FIELD(WC, FIELD) : NULL), ((FLOW)->FIELD))
+
 static inline bool is_vlan(const struct flow *flow,
struct flow_wildcards *wc)
 {
diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
index f7d29f6..499efff 100644
--- a/lib/tnl-neigh-cache.c
+++ b/lib/tnl-neigh-cache.c
@@ -115,7 +115,7 @@ tnl_neigh_delete(struct tnl_neigh_entry *neigh)
 
 static void
 tnl_neigh_set__(const char name[IFNAMSIZ], const struct in6_addr *dst,
-  const struct eth_addr mac)
+const struct eth_addr mac)
 {
 ovs_mutex_lock();
 struct tnl_neigh_entry *neigh = tnl_neigh_lookup__(name, dst);
@@ -151,26 +151,21 @@ static int
 tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
   const char name[IFNAMSIZ])
 {
-if (flow->dl_type != htons(ETH_TYPE_ARP) ||
-flow->nw_proto != ARP_OP_REPLY ||
-eth_addr_is_zero(flow->arp_sha)) {
+if (flow->dl_type != htons(ETH_TYPE_ARP)
+|| FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_proto) != ARP_OP_REPLY
+|| eth_addr_is_zero(FLOW_WC_GET_AND_MASK_WC(flow, wc, arp_sha))) {
 return EINVAL;
 }
 
-/* Exact Match on all ARP flows. */
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.arp_sha, 0xff, sizeof wc->masks.arp_sha);
-
-tnl_arp_set(name, flow->nw_src, flow->arp_sha);
+tnl_arp_set(name, FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src), 
flow->arp_sha);
 return 0;
 }
 
 static int
 tnl_nd_snoop(const struct flow *flow, struct flow_wildcards *wc,
-  const char name[IFNAMSIZ])
+ const char name[IFNAMSIZ])
 {
-if (!is_nd(flow, NULL) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
+if (!is_nd(flow, wc) || flow->tp_src != htons(ND_NEIGHBOR_ADVERT)) {
 return EINVAL;
 }
 /* - RFC4861 says Neighbor Advertisements sent in response to unicast 
Neighbor
@@ -179,14 +174,13 @@ tnl_nd_snoop(const struct flow *flow, struct 
flow_wildcards *wc,
  *   TLL address and other Advertisements not including it can be ignored.
  * - OVS flow extract can set this field to zero in case of packet parsing 
errors.
  *   For details refer miniflow_extract()*/
-if (eth_addr_is_zero(flow->arp_tha)) {
+if (eth_addr_is_zero(FLOW_WC_GET_AND_MASK_WC(flow, wc, arp_tha))) {
 return EINVAL;
 }
 
 memset(>masks.ipv6_src, 0xff, sizeof wc->masks.ipv6_src);
 memset(>masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
 memset(>masks.nd_target, 0xff, sizeof wc->masks.nd_target);
-memset(>masks.arp_tha, 0xff, sizeof wc->masks.arp_tha);
 
 tnl_neigh_set__(name, >nd_target, flow->arp_tha);
 return 0;
-- 
2.9.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2 4/5] treewide: Use WC_MASK_FIELD() and FLOW_WC_GET_AND_MASK_WC().

[Daniele Di Proietto]

I think it makes the code more readable. No functional change.

Signed-off-by: Daniele Di Proietto 
---
 lib/bfd.c| 36 
 lib/cfm.c|  9 +++
 lib/flow.c   | 42 
 lib/flow.h   | 47 +---
 lib/odp-util.c   |  2 +-
 lib/tnl-neigh-cache.c|  6 ++---
 lib/tnl-ports.c  |  2 +-
 ofproto/netflow.c|  6 ++---
 ofproto/ofproto-dpif-xlate.c | 57 +++-
 ofproto/tunnel.c |  2 +-
 10 files changed, 92 insertions(+), 117 deletions(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index 87f3322..203a56e 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -648,30 +648,24 @@ bfd_should_process_flow(const struct bfd *bfd_, const 
struct flow *flow,
 {
 struct bfd *bfd = CONST_CAST(struct bfd *, bfd_);
 
-if (!eth_addr_is_zero(bfd->rmt_eth_dst)) {
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-
-if (!eth_addr_equals(bfd->rmt_eth_dst, flow->dl_dst)) {
-return false;
-}
+if (!eth_addr_is_zero(bfd->rmt_eth_dst)
+&& !eth_addr_equals(bfd->rmt_eth_dst,
+FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) {
+return false;
 }
 
-if (flow->dl_type == htons(ETH_TYPE_IP)) {
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == IPPROTO_UDP && !(flow->nw_frag & 
FLOW_NW_FRAG_LATER)) {
-memset(>masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
-if (flow->tp_dst == htons(BFD_DEST_PORT)) {
-bool check_tnl_key;
-
-atomic_read_relaxed(>check_tnl_key, _tnl_key);
-if (check_tnl_key) {
-memset(>masks.tunnel.tun_id, 0xff,
-   sizeof wc->masks.tunnel.tun_id);
-return flow->tunnel.tun_id == htonll(0);
-}
-return true;
-}
+if (flow->dl_type == htons(ETH_TYPE_IP)
+&& FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_proto) == IPPROTO_UDP
+&& !(flow->nw_frag & FLOW_NW_FRAG_LATER)
+&& FLOW_WC_GET_AND_MASK_WC(flow, wc, tp_dst) == htons(BFD_DEST_PORT)) {
+bool check_tnl_key;
+
+atomic_read_relaxed(>check_tnl_key, _tnl_key);
+if (check_tnl_key) {
+return FLOW_WC_GET_AND_MASK_WC(flow, wc, tunnel.tun_id)
+   == htonll(0);
 }
+return true;
 }
 return false;
 }
diff --git a/lib/cfm.c b/lib/cfm.c
index 7bc22e3..3c55333 100644
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -731,16 +731,17 @@ cfm_should_process_flow(const struct cfm *cfm_, const 
struct flow *flow,
 return false;
 }
 
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (OVS_UNLIKELY(!eth_addr_equals(flow->dl_dst, cfm_ccm_addr(cfm {
+if (OVS_UNLIKELY(
+!eth_addr_equals(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst),
+ cfm_ccm_addr(cfm {
 return false;
 }
 
 atomic_read_relaxed(>check_tnl_key, _tnl_key);
 
 if (check_tnl_key) {
-memset(>masks.tunnel.tun_id, 0xff, sizeof wc->masks.tunnel.tun_id);
-return flow->tunnel.tun_id == htonll(0);
+return FLOW_WC_GET_AND_MASK_WC(flow, wc, tunnel.tun_id)
+   == htonll(0);
 }
 return true;
 }
diff --git a/lib/flow.c b/lib/flow.c
index ba4f8c7..8f052ba 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -855,8 +855,8 @@ void
 flow_unwildcard_tp_ports(const struct flow *flow, struct flow_wildcards *wc)
 {
 if (flow->nw_proto != IPPROTO_ICMP) {
-memset(>masks.tp_src, 0xff, sizeof wc->masks.tp_src);
-memset(>masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+WC_MASK_FIELD(wc, tp_src);
+WC_MASK_FIELD(wc, tp_dst);
 } else {
 wc->masks.tp_src = htons(0xff);
 wc->masks.tp_dst = htons(0xff);
@@ -1478,8 +1478,8 @@ flow_wildcards_clear_non_packet_fields(struct 
flow_wildcards *wc)
 /* Update this function whenever struct flow changes. */
 BUILD_ASSERT_DECL(FLOW_WC_SEQ == 36);
 
-memset(>masks.metadata, 0, sizeof wc->masks.metadata);
-memset(>masks.regs, 0, sizeof wc->masks.regs);
+WC_UNMASK_FIELD(wc, metadata);
+WC_UNMASK_FIELD(wc, regs);
 wc->masks.actset_output = 0;
 wc->masks.conj_id = 0;
 }
@@ -1834,21 +1834,21 @@ flow_mask_hash_fields(const struct flow *flow, struct 
flow_wildcards *wc,
 {
 switch (fields) {
 case NX_HASH_FIELDS_ETH_SRC:
-memset(>masks.dl_src, 0xff, sizeof wc->masks.dl_src);
+WC_MASK_FIELD(wc, dl_src);
 break;
 
 case NX_HASH_FIELDS_SYMMETRIC_L4:
-memset(>masks.dl_src, 0xff, sizeof wc->masks.dl_src);
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
+WC_MASK_FIELD(wc, dl_src);
+WC_MASK_FIELD(wc, dl_dst);
 if 

[ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.

[Daniele Di Proietto]

When translating a set action we also unwildcard the field in question.
This is done to correctly translate set actions with the value identical
to the ingress flow, like in the following example:

flow table:

tcp,actions=set_field:80->tcp_dst,output:5

ingress packet:

...,tcp,tcp_dst=80

datapath flow

...,tcp(dst=80) actions:5

The datapath flow must exact match the target field, because the actions
do not include a set field. (Otherwise a packet coming in with a
different tcp_dst would be matched, and its port wouldn't be altered).

Tunnel attributes behave differently: at the datapath layer, before
action processing they're cleared (we do the same at the ofproto layer
in xlate_actions()).  Therefore there's no need to unwildcard them,
because a set action would always be detected (since we zero them at the
beginning of xlate_ations()).

This fixes a problem related to the handling of Geneve options.
Unwildcarding non existing Geneve options (as done by a
set_field:X->tun_metadata on a packet coming from a non-tunnel
interface) would be problematic for the datapaths: the ODP translation
functions cannot express a match on non existing Geneve options (unlike
on other attributes), and the userspace datapath wouldn't be able to
translate them to "userspace datapath format".  In both cases the
installed flow would be deleted by revalidation at the first
opportunity.

Signed-off-by: Daniele Di Proietto 
---
 include/openvswitch/meta-flow.h |  1 +
 lib/meta-flow.c | 95 -
 ofproto/ofproto-dpif-xlate.c|  4 +-
 3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 23f9916..f21df4b 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2094,6 +2094,7 @@ void mf_set_flow_value_masked(const struct mf_field *,
   const union mf_value *mask,
   struct flow *);
 bool mf_is_tun_metadata(const struct mf_field *);
+bool mf_set_must_mask(const struct mf_field *);
 bool mf_is_set(const struct mf_field *, const struct flow *);
 void mf_mask_field(const struct mf_field *, struct flow_wildcards *);
 void mf_mask_field_masked(const struct mf_field *, const union mf_value *mask,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 3dc2770..86a0f03 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1407,6 +1407,97 @@ mf_is_tun_metadata(const struct mf_field *mf)
mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
 }
 
+/* Returns true if a field 'mf' should be exact matched before being set
+ * by the action translation, false otherwise.  Most of the fields need
+ * an exact match.*/
+bool
+mf_set_must_mask(const struct mf_field *mf)
+{
+/* Tunnel attributes don't need an exact match, because they are
+ * cleared by the datapath between ingress and egress. Also, an
+ * exact match on tunnel metadata might be problematic, because
+ * it is not possible to express it if the metadata didn't exist
+ * on ingress. */
+switch (mf->id) {
+case MFF_TUN_ID:
+case MFF_TUN_SRC:
+case MFF_TUN_DST:
+case MFF_TUN_IPV6_SRC:
+case MFF_TUN_IPV6_DST:
+case MFF_TUN_FLAGS:
+case MFF_TUN_GBP_ID:
+case MFF_TUN_GBP_FLAGS:
+case MFF_TUN_TOS:
+case MFF_TUN_TTL:
+CASE_MFF_TUN_METADATA:
+return false;
+
+case MFF_DP_HASH:
+case MFF_RECIRC_ID:
+case MFF_CONJ_ID:
+case MFF_METADATA:
+case MFF_IN_PORT:
+case MFF_IN_PORT_OXM:
+case MFF_ACTSET_OUTPUT:
+case MFF_SKB_PRIORITY:
+case MFF_PKT_MARK:
+case MFF_CT_STATE:
+case MFF_CT_ZONE:
+case MFF_CT_MARK:
+case MFF_CT_LABEL:
+CASE_MFF_REGS:
+CASE_MFF_XREGS:
+CASE_MFF_XXREGS:
+case MFF_ETH_SRC:
+case MFF_ETH_DST:
+case MFF_ETH_TYPE:
+case MFF_VLAN_TCI:
+case MFF_DL_VLAN:
+case MFF_VLAN_VID:
+case MFF_DL_VLAN_PCP:
+case MFF_VLAN_PCP:
+case MFF_MPLS_LABEL:
+case MFF_MPLS_TC:
+case MFF_MPLS_BOS:
+case MFF_MPLS_TTL:
+case MFF_IPV4_SRC:
+case MFF_ARP_SPA:
+case MFF_IPV4_DST:
+case MFF_ARP_TPA:
+case MFF_IPV6_SRC:
+case MFF_IPV6_DST:
+case MFF_IPV6_LABEL:
+case MFF_IP_PROTO:
+case MFF_IP_DSCP:
+case MFF_IP_DSCP_SHIFTED:
+case MFF_IP_ECN:
+case MFF_IP_TTL:
+case MFF_IP_FRAG:
+case MFF_ARP_OP:
+case MFF_ARP_SHA:
+case MFF_ND_SLL:
+case MFF_ARP_THA:
+case MFF_ND_TLL:
+case MFF_TCP_SRC:
+case MFF_UDP_SRC:
+case MFF_SCTP_SRC:
+case MFF_ICMPV4_TYPE:
+case MFF_ICMPV6_TYPE:
+case MFF_TCP_DST:
+case MFF_UDP_DST:
+case MFF_SCTP_DST:
+case MFF_ICMPV4_CODE:
+case MFF_ICMPV6_CODE:
+case MFF_TCP_FLAGS:
+case MFF_ND_TARGET:
+return true;
+
+case MFF_N_IDS:
+default:
+OVS_NOT_REACHED();
+}
+}
+
 /* Returns true if 'mf' has previously been 

[ovs-dev] [PATCH v2 2/5] ofproto-dpif-xlate: Adjust generated mask for fragments.

[Daniele Di Proietto]

It's possible to install an OpenFlow flow that matches on udp source and
destination ports without matching on fragments.  If the subtable where
such flow stays is visited during translation of a later fragment, the
generated mask will have incorrect prerequisited for the datapath and it
would be revalidated away at the first chance.

This commit fixes it by adjusting the mask for later fragments after
translation.

Other prerequisites of the mask are also prerequisites in OpenFlow, but
not the ip fragment bit, that's why we need a special case here.

For completeness, this commits also fixes a related problem in bfd,
where we check the udp destination port without checking if the frame is
an ip fragment.  It's not really necessary to address this separately,
given the adjustment that we perform.

VMware-BZ: #1651589
Signed-off-by: Daniele Di Proietto 
---
 lib/bfd.c|  2 +-
 ofproto/ofproto-dpif-xlate.c | 11 +++
 tests/ofproto-dpif.at| 35 +++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index fcb6f64..87f3322 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -658,7 +658,7 @@ bfd_should_process_flow(const struct bfd *bfd_, const 
struct flow *flow,
 
 if (flow->dl_type == htons(ETH_TYPE_IP)) {
 memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == IPPROTO_UDP) {
+if (flow->nw_proto == IPPROTO_UDP && !(flow->nw_frag & 
FLOW_NW_FRAG_LATER)) {
 memset(>masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
 if (flow->tp_dst == htons(BFD_DEST_PORT)) {
 bool check_tnl_key;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0118d01..0403c98 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5329,6 +5329,17 @@ xlate_wc_finish(struct xlate_ctx *ctx)
 if (ctx->wc->masks.vlan_tci) {
 ctx->wc->masks.vlan_tci |= htons(VLAN_CFI);
 }
+
+/* The classifier might return masks that match on tp_src and tp_dst even
+ * for later fragments.  This happens because there might be flows that
+ * match on tp_src or tp_dst without matching on the frag bits, because
+ * it is not a prerequisite for OpenFlow.  Since it is a prerequisite for
+ * datapath flows and since tp_src and tp_dst are always going to be 0,
+ * wildcard the fields here. */
+if (ctx->xin->flow.nw_frag & FLOW_NW_FRAG_LATER) {
+ctx->wc->masks.tp_src = 0;
+ctx->wc->masks.tp_dst = 0;
+}
 }
 
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 8e5fde6..e047c1c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8903,3 +8903,38 @@ AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface 
int1 mtu=1600])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto - fragment prerequisites])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+add_of_ports br0 1
+
+AT_DATA([flows.txt], [dnl
+priority=10,in_port=1,udp,tp_src=67,tp_dst=68,action=drop
+priority=1,in_port=1,udp,action=drop
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:max-idle=1])
+
+ovs-appctl time/stop
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'recirc_id(0),in_port(1),eth_type(0x0800),ipv4(proto=17,frag=later)'])
+ovs-appctl time/warp 5000
+
+AT_CHECK([strip_ufid < ovs-vswitchd.log | filter_flow_install | strip_used], 
[0], [dnl
+recirc_id(0),in_port(1),eth_type(0x0800),ipv4(proto=17,frag=later), 
actions:drop
+])
+
+dnl Change the flow table.  This will trigger revalidation of all the flows.
+AT_CHECK([ovs-ofctl add-flow br0 priority=5,in_port=1,action=drop])
+AT_CHECK([ovs-appctl revalidator/wait], [0])
+
+dnl We don't want revalidators to delete any flow.  If the flow has been
+dnl deleted it means that there's some inconsistency with the revalidation.
+AT_CHECK([grep flow_del ovs-vswitchd.log], [1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
2.9.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 12:56:55

[Dalton]

-Original Message-
From: "Dalton" 
Sent: 2016-08-13 12:56:55
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 12:56:55


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 14:16:55

[Cecil]

-Original Message-
From: "Cecil" 
Sent: 2016-08-13 14:16:55
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 14:16:55


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ofproto: Always set MTU for new internal ports.

[Daniele Di Proietto]

On 30/08/2016 15:32, "Ben Pfaff"  wrote:

>On Tue, Aug 30, 2016 at 11:41:15AM -0700, Daniele Di Proietto wrote:
>> We only changed the MTU of new internal ports if it is bigger than the
>> bridge minimum.  But when the minimum MTU of the bridge is updated we
>> change the MTU of all internal ports no matter what.
>> 
>> The behavior is inconsistent, because now the internal ports MTU depends
>> on the order in which the ports where added.
>> 
>> This commit fixes the problem by _always_ setting the MTU of new
>> internal ports to the bridge minimum.  I'm not sure what was the logic
>> behind only adjusting the mtu if it was too big.
>> 
>> A testcase is improved to detect the problem.
>> 
>> VMware-BZ: #1718776
>> Signed-off-by: Daniele Di Proietto 
>
>Acked-by: Ben Pfaff 

Thanks for the prompt reviews, I applied the series to master and branch-2.6
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-save: Restore tunnel TLV map before flows.

[Jesse Gross]

On Tue, Aug 30, 2016 at 3:34 PM, Ben Pfaff  wrote:
> On Tue, Aug 30, 2016 at 02:16:36PM -0700, Jesse Gross wrote:
>> Scripts that integrate OVS with a distribution often save and
>> restore flows across distruptive events, such as an upgrade. The
>> ovs-save utility generates a script to assist with this.
>>
>> When flows include tunnel metadata, we also need to restore the
>> TLV mappings before the flows are re-added. Otherwise, the instance
>> of OVS receiving the new flows won't know the meaning of these
>> fields and will ignore them.
>>
>> Signed-off-by: Jesse Gross 
>
> Good catch.
>
> Acked-by: Ben Pfaff 

Applied to master, branch-2.6, and branch-2.5 as well.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 13:56:28

[Dante]

-Original Message-
From: "Dante" 
Sent: 2016-08-13 13:56:28
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 13:56:28


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovs-ofctl: Fix crash with replace-flows and diff-flows with tunnel metadata.

[Jesse Gross]

On Tue, Aug 30, 2016 at 3:22 PM, Ben Pfaff  wrote:
> On Mon, Aug 29, 2016 at 11:58:14AM -0700, Jesse Gross wrote:
>> When flows are read by ovs-ofctl (either from a switch or a file),
>> tunnel metadata space is dynamically allocated since there isn't a
>> preset table. This works well for single flows but doesn't handle
>> groups of flows that must be compared to each other. In this case,
>> each flow will have its own independent allocation making comparisons
>> meaningless.
>>
>> Even worse is that when these matches are later serialized (either
>> for display or in NXM format), the metadata allocation has been
>> stripped off of the matches. The serialization code then attempts to
>> use the global table, which is also not available, leading to a
>> dereference of a NULL pointer.
>>
>> Solving this problem requires building an overall metadata table.
>> Since we don't know the maximum size of a field (particularly for
>> flows read from a file), it's necessary to do this in two passes.
>> The first pass records the maximum size for each field as well as
>> stores the received matches. The second pass creates a metadata
>> table based on the sizes, adjusts the match layout based on the new
>> allocation, and then replays the stored matches for comparison.
>> Later serialization will used the generated table to output the
>> flows.
>>
>> Signed-off-by: Jesse Gross 
>
> Good catch.
>
> Acked-by: Ben Pfaff 

Thanks, I rolled in your suggestions and applied both patches in this
series to master, branch-2.6, and branch-2.5.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Returned mail: see transcript for details

[wnmtnigeria]

Dear user dev@openvswitch.org,

We have found that your account was used to send a large amount of spam during 
this week.
Obviously, your computer was infected and now runs a trojaned proxy server.

We recommend that you follow the instructions in order to keep your computer 
safe.

Have a nice day,
openvswitch.org support team.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 15:15:44

[Melanie]

-Original Message-
From: "Melanie" 
Sent: 2016-08-13 15:15:44
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 15:15:44


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn: add lsp-deletion and bcast-flow removal tests for localnet

[Ramu Ramamurthy]

From: Ramu Ramamurthy 

Add 2 tests for scenarios around lsp-deletion and flow removal
which have escaped current unit tests.

This test depends on the following patch:
"ovn-controller: Back out incremental processing" and passes
after applying it, but fails currently on master.

1) In the following sequence of events,
createi vif1, create vif2, delete vif1
we find that the localnet patch port
got deleted, whereas it should exist because there is a
bound vif2.

2) The flow broadcasting to tunnels in table=32 must be deleted
when a localnet port gets bound, but we find that the flow remains
in table 32 causing broadcasts to both tunnels and localnet patch.

Signed-off-by: Ramu Ramamurthy 
---
 tests/ovn.at | 71 
 1 file changed, 71 insertions(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index a23b422..3fa267a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5053,3 +5053,74 @@ OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int 
table=0 | grep REG13 | wc -l`
 OVN_CLEANUP([hv1])
 
 AT_CLEANUP
+
+
+AT_SETUP([ovn -- lsp deletion and broadcast-flow deletion on localnet])
+AT_KEYWORDS([ovn])
+ovn_start
+ovn-nbctl ls-add lsw0
+net_add n1
+for i in 1 2; do
+sim_add hv$i
+as hv$i
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.$i
+ovs-vsctl add-br br-eth0
+AT_CHECK([ovs-vsctl set Open_vSwitch . 
external-ids:ovn-bridge-mappings=physnet1:br-eth0])
+done
+
+# Create a localnet port.
+AT_CHECK([ovn-nbctl lsp-add lsw0 ln_port])
+AT_CHECK([ovn-nbctl lsp-set-addresses ln_port unknown])
+AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
+AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])
+
+
+# Create 3 vifs.
+AT_CHECK([ovn-nbctl lsp-add lsw0 localvif1])
+AT_CHECK([ovn-nbctl lsp-set-addresses localvif1 "f0:00:00:00:00:01 
192.168.1.1"])
+AT_CHECK([ovn-nbctl lsp-set-port-security localvif1 "f0:00:00:00:00:01"])
+AT_CHECK([ovn-nbctl lsp-add lsw0 localvif2])
+AT_CHECK([ovn-nbctl lsp-set-addresses localvif2 "f0:00:00:00:00:01 
192.168.1.2"])
+AT_CHECK([ovn-nbctl lsp-set-port-security localvif2 "f0:00:00:00:00:02"])
+AT_CHECK([ovn-nbctl lsp-add lsw0 localvif3])
+AT_CHECK([ovn-nbctl lsp-set-addresses localvif3 "f0:00:00:00:00:03 
192.168.1.3"])
+AT_CHECK([ovn-nbctl lsp-set-port-security localvif3 "f0:00:00:00:00:03"])
+
+# Bind the localvif1 to hv1.
+as hv1
+AT_CHECK([ovs-vsctl add-port br-int localvif1 -- set Interface localvif1 
external_ids:iface-id=localvif1])
+
+# On hv1, check that there are no flows outputting bcast to tunnel
+OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip | 
grep output | wc -l` -eq 0])
+
+# On hv2, check that there is 1 flow outputting bcast to tunnel to hv1.
+as hv2
+OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip | 
grep output | wc -l` -eq 1])
+
+# Now bind vif2 on hv2.
+AT_CHECK([ovs-vsctl add-port br-int localvif2 -- set Interface localvif2 
external_ids:iface-id=localvif2])
+
+# At this point, the broadcast flow on vif2 should be deleted.
+# because, there is now a localnet vif bound (table=32 programming logic)
+OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=32 | ofctl_strip | 
grep output | wc -l` -eq 0])
+
+# Verify that the local net patch port exists on hv2.
+OVS_WAIT_UNTIL([test `ovs-vsctl show | grep "Port patch-br-int-to-ln_port" | 
wc -l` -eq 1])
+
+# Now bind vif3 on hv2.
+AT_CHECK([ovs-vsctl add-port br-int localvif3 -- set Interface localvif3 
external_ids:iface-id=localvif3])
+
+# Verify that the local net patch port still exists on hv2
+OVS_WAIT_UNTIL([test `ovs-vsctl show | grep "Port patch-br-int-to-ln_port" | 
wc -l` -eq 1])
+
+# Delete localvif2
+AT_CHECK([ovn-nbctl lsp-del localvif2])
+
+# Verify that the local net patch port still exists on hv2,
+# because, localvif3 is still bound.
+OVS_WAIT_UNTIL([test `ovs-vsctl show | grep "Port patch-br-int-to-ln_port" | 
wc -l` -eq 1])
+
+
+OVN_CLEANUP([hv1],[hv2])
+AT_CLEANUP
-- 
1.8.3.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 15:16:49

[Sonny]

-Original Message-
From: "Sonny" 
Sent: 2016-08-13 15:16:49
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 15:16:49


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] FW: [Scan] 2016-08-13 15:25:18

[Vonda]

-Original Message-
From: "Vonda" 
Sent: 2016-08-13 15:25:18
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 15:25:18


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn-nbctl: Update manpage.

[nickcooper-zhangtonghao]

Thanks for your tips. I have merged the 3 patches into one.

You can review it. http://patchwork.ozlabs.org/patch/663981/ 



> On Aug 31, 2016, at 6:06 AM, Ben Pfaff  wrote:
> 
> On Sun, Aug 28, 2016 at 02:05:18AM -0700, nickcooper-zhangtonghao wrote:
>> Signed-off-by: nickcooper-zhangtonghao 
>> > >
> 
> Code changes and its documentation should be in the same patch.
> 
> Thanks,
> 
> Ben.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] ovn: Multiple features with arbitrary match criteria (ACLs, QoS, SFC) (was Re: [PATCH] ovn: Add second ACL stage)

[Mickey Spiegel]

Coming back to this after pondering more about QoS (e.g. DSCP) marking and
SFC, to what extent they are related to multiple ACL stages.

There are multiple features either already implemented or being discussed,
which lend themselves to arbitrary match criteria (e.g. inport, outport,
eth.type, eth.src, eth.dst, ip4.src, ip4.dst, ip6.src, ip6.dst, ip.dscp,
tcp.src, tcp.dst, udp.src, udp.dst) with feature-specific actions,
including:

   - ACLs for security purposes, often security groups but not restricted
   to that. http://openvswitch.org/pipermail/dev/2016-July/076674.html
   proposed expanding this to two or more stages, in order to support multiple
   security features such as security groups and OpenStack FWaaS, or security
   groups and network ACLs.
   - QoS marking.
   http://openvswitch.org/pipermail/dev/2016-August/078702.html proposed
   port-based DSCP marking. It was suggested that this could be expanded to
   arbitrary match criteria. Physical switches typically support DSCP (and
   CoS) marking with arbitrary match criteria, similar to but often with
   different syntax from ACLs.
   - Service Function Chaining (SFC) insertion, where traffic is directed
   into the start of a service chain.
   http://openvswitch.org/pipermail/dev/2016-July/075035.html proposed that
   the rules for SFC insertion (called flow classification in that thread) be
   set as ACL rules.

For similar reasons to those mentioned below with regard to two different
security features, IMO QoS marking and SFC insertion should each have their
own pipeline stage. By separating each feature into its own pipeline stage,
the amount of code and complexity related to interactions between different
features is minimized. For the most part, each feature can be implemented
independently.

The big question in my mind is whether:

   - All of these should be bundled under the existing ACL table with the
   addition of a "stage" column as proposed in
   http://openvswitch.org/pipermail/dev/2016-July/076674.html and
   extensions to the action column.
   or
   - Each of these features should be represented separately in the
   Northbound DB, along the lines of

"Logical_Switch": {
"columns": {
"name": {"type": "string"},
"ports": {"type": {"key": {"type": "uuid",
   "refTable":
   "Logical_Switch_Port",
   "refType": "strong"},
   "min": 0,
   "max": "unlimited"}},
"s": {"type": {"key": {"type": "uuid",
  "refTable": "",
  "refType": "strong"},
  "min": 0,
  "max": "unlimited"}},
   +"s": {"type": {"key": {"type": "uuid",
   + "refTable": "",
   + "refType": "strong"},
   + "min": 0,
   + "max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",

   ...


"": {
"columns": {
"priority": {"type": {"key": {"type": "integer",
  "minInteger": 0,
  "maxInteger": 32767}}},
"direction": {"type": {"key": {"type": "string",
"enum": ["set", ["from-lport",
   "to-lport"]]}}},
"match": {"type": "string"},
"action": {"type": {"key": {"type": "string",
   "enum": ["set",
["allow", "allow-related", "drop", "reject"]]}}},

"external_ids": {
"type": {"key": "string", "value": "string",
 "min": 0, "max": "unlimited"}}},
"isRoot": false},

   +"": {
   +"columns": {
   +"priority": {"type": {"key": {"type": "integer",
   +  "minInteger": 0,
   +  "maxInteger": 32767}}},
   +"direction": {"type": {"key": {"type": "string",
   +"enum": ["set", ["from-lport",
   "to-lport"]]}}},
   +"match": {"type": "string"},
   +"action": {"type": {"key": {"type": "string",
   +"enum": ["set", ["dscp",
   "cos"]]},
   +"value": {"type": "integer",
   +  "minInteger": 0,
   +  "maxInteger": 63}}},
   +

[ovs-dev] FW: [Scan] 2016-08-13 15:13:26

[Mohamed]

-Original Message-
From: "Mohamed" 
Sent: 2016-08-13 15:13:26
To: d...@openvswitch.com
Subject: [Scan] 2016-08-13 15:13:26


--
Sent with Genius Scan for iOS.
http://bit.ly/download-genius-scan


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support

[Joe Stringer]

On 26 August 2016 at 02:13, Simon Horman  wrote:
> On Thu, Aug 25, 2016 at 05:33:57PM -0700, Joe Stringer wrote:
>> On 25 August 2016 at 03:08, Simon Horman  wrote:
>> > Please find my working patch below.
>> >
>> > From: Simon Horman 
>> > Subject: [PATCH] system-traffic: Exercise GSO
>> >
>> > Exercise GSO for: unencapsulated; MPLS; GRE; and MPLS in GRE.
>> >
>> > There is scope to extend this testing to other encapsulation formats
>> > if desired.
>> >
>> > This is motivated by a desire to test GRE and MPLS encapsulation in
>> > the context of L3/VPN (MPLS over non-TEB GRE work). That is not
>> > tested here but tests for those cases would idealy be based on those in
>> > this patch.
>> >
>> > Signed-off-by: Simon Horman 
>>
>> I realised that these tests disable TSO, but they don't actually check
>> if GSO is enabled. Maybe it's safe to assume this, but it's more
>> explicit to actually look for it in the tests.
>
> Good point, I'll see about checking that.
>
>> With particular setups (fedora23 in particular, both kernel and
>> userspace testsuites) I see this:
>>
>> ./system-traffic.at:371: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC
>> ip route add 10.1.2.0/24 encap mpls 100 via inet 10.1.1.2 dev ns_gre0
>> NS_EXEC_HEREDOC
>> --- /dev/null 2016-08-19 01:28:02.15100 +
>> +++ 
>> /home/gitlab-runner/builds/83c49bff/0/root/gitlab-ovs/ovs/tests/system-kmod-testsuite.dir/at-groups/10/stderr
>> 2016-08-25 17:16:27.32400 +
>> @@ -0,0 +1 @@
>> +Error: either "to" is duplicate, or "encap" is a garbage.
>>
>> I'm guessing the ip tool is a little out of date. We could detect and
>> skip this with something like:
>>
>> AT_SKIP_IF([ip route help 2>&1 | grep encap])
>>
>> in the CHECK_MPLS.
>
> Thanks, I'll add something like that.
>
>> Hmm, I'm still seeing the bad counts of segments retransmited even
>> with the diff change on a kernel I have built at bf0f500bd019 ("Merge
>> tag 'trace-v4.8-1' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace").
>> If it's passing on latest net-next then maybe I just need to swap out
>> that box's kernel for a newer build. I'll try that.
>
> It is possible that it is detecting a bug.
> Which test is failing?

FWIW I tried with a newer build, commit 9a0a5c4cb1af ("net:
systemport: Fix ordering in intrl2_*_mask_clear macro"). I no longer
see the issue.

Unfortunately I lost my test output. It was one of these two:

  8: datapath - ping over gre tunnel FAILED
(system-traffic.at:294)
  9: datapath - http over gre tunnel FAILED
(system-traffic.at:348)

I also realised that I didn't have MPLS router enabled in my kernel
config so the MPLS tests were getting skipped. I enabled MPLS_ROUTING,
but now I see this failure on the "http over mpls" tests:

./system-traffic.at:111: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC
ip route add 10.1.1.0/24 encap mpls 100 via inet 172.31.1.2 dev p0
NS_EXEC_HEREDOC
--- /dev/null 2016-08-30 15:22:28.813316948 -0700
+++ 
/home/gitlab-runner/builds/f1d4a2be/0/root/gitlab-ovs/ovs/tests/system-kmod-testsuite.dir/at-groups/4/stderr
2016-08-30 15:33:45.133306581 -0700
@@ -0,0 +1 @@
+RTNETLINK answers: Operation not supported


> At this stage I have mostly added TSO/GSO testing to existing checks.
> Perhaps it would be better to break them out into separate checks so
> ping/http can be be checked without considering TSO/GSO which may have some
> value in situations where TSO/GSO is broken which is actually what I am
> interested in testing.

Sounds reasonable.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] patch series

[Ben Pfaff]

On Tue, Aug 30, 2016 at 05:17:43PM -0500, Ryan Moats wrote:
> 
> Ben Pfaff  wrote on 08/30/2016 05:05:14 PM:
> 
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: dev@openvswitch.org
> > Date: 08/30/2016 05:05 PM
> > Subject: patch series
> >
> > It looks like you have 4 separate patches that are connected and in
> > which 3 of them depend on the other one.
> >
> > Usually, we'd represent that as a 4-patch series.
> >
> 
> I went back and forth on whether to put them together or not,
> and decided to leave the patches that were already there be
> and build on top of them, rather than resubmit them with only
> a numbering change in the subject.
> 
> Given that was the wrong approach, a series will follow shortly...

Thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-save: Restore tunnel TLV map before flows.

[Ben Pfaff]

On Tue, Aug 30, 2016 at 02:16:36PM -0700, Jesse Gross wrote:
> Scripts that integrate OVS with a distribution often save and
> restore flows across distruptive events, such as an upgrade. The
> ovs-save utility generates a script to assist with this.
> 
> When flows include tunnel metadata, we also need to restore the
> TLV mappings before the flows are re-added. Otherwise, the instance
> of OVS receiving the new flows won't know the meaning of these
> fields and will ignore them.
> 
> Signed-off-by: Jesse Gross 

Good catch.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ofproto: Always set MTU for new internal ports.

[Ben Pfaff]

On Tue, Aug 30, 2016 at 11:41:15AM -0700, Daniele Di Proietto wrote:
> We only changed the MTU of new internal ports if it is bigger than the
> bridge minimum.  But when the minimum MTU of the bridge is updated we
> change the MTU of all internal ports no matter what.
> 
> The behavior is inconsistent, because now the internal ports MTU depends
> on the order in which the ports where added.
> 
> This commit fixes the problem by _always_ setting the MTU of new
> internal ports to the bridge minimum.  I'm not sure what was the logic
> behind only adjusting the mtu if it was too big.
> 
> A testcase is improved to detect the problem.
> 
> VMware-BZ: #1718776
> Signed-off-by: Daniele Di Proietto 

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] bridge: Honor 'mtu_request' when port is added.

[Ben Pfaff]

On Tue, Aug 30, 2016 at 11:41:14AM -0700, Daniele Di Proietto wrote:
> 'mtu_request' was honored only when the port was reconfigured, not when
> the port was added.
> 
> This commit fixes the problem and improves a testcase to detect the bug.
> 
> Found by inspection.
> 
> Fixes: 56abcf497b56("vswitchd: Introduce 'mtu_request' column in
> Interface.")
> Signed-off-by: Daniele Di Proietto 

Good catch, thanks.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/4] Unpersist data structures for address sets.

[Ryan Moats]

With the removal of incremental processing, it is no longer
necessary to persist the data structures for storing address
sets.  Simplify things by removing this complexity.

Side effect: fixed a memory leak in expr_macros_destroy that
is evidenced by this change.

This commit depends on f5d916cb ("ovn-controller:
Back out incremental processing").

Signed-off-by: Ryan Moats 
---
 ovn/controller/lflow.c | 166 -
 ovn/lib/expr.c |   1 +
 2 files changed, 42 insertions(+), 125 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index dc69047..5713c46 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -38,14 +38,10 @@ VLOG_DEFINE_THIS_MODULE(lflow);
 /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
 static struct shash symtab;
 
-/* Contains an internal expr datastructure that represents an address set. */
-static struct shash expr_address_sets;
-
 void
 lflow_init(void)
 {
 ovn_init_symtab();
-shash_init(_address_sets);
 }
 
 /* Details of an address set currently in address_sets. We keep a cached
@@ -56,54 +52,6 @@ struct address_set {
 size_t n_addresses;
 };
 
-/* struct address_set instances for address sets currently in the symtab,
- * hashed on the address set name. */
-static struct shash local_address_sets = 
SHASH_INITIALIZER(_address_sets);
-
-static int
-addr_cmp(const void *p1, const void *p2)
-{
-const char *s1 = p1;
-const char *s2 = p2;
-return strcmp(s1, s2);
-}
-
-/* Return true if the address sets match, false otherwise. */
-static bool
-address_sets_match(const struct address_set *addr_set,
-   const struct sbrec_address_set *addr_set_rec)
-{
-char **addrs1;
-char **addrs2;
-
-if (addr_set->n_addresses != addr_set_rec->n_addresses) {
-return false;
-}
-size_t n_addresses = addr_set->n_addresses;
-
-addrs1 = xmemdup(addr_set->addresses,
- n_addresses * sizeof addr_set->addresses[0]);
-addrs2 = xmemdup(addr_set_rec->addresses,
- n_addresses * sizeof addr_set_rec->addresses[0]);
-
-qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
-qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
-
-bool res = true;
-size_t i;
-for (i = 0; i <  n_addresses; i++) {
-if (strcmp(addrs1[i], addrs2[i])) {
-res = false;
-break;
-}
-}
-
-free(addrs1);
-free(addrs2);
-
-return res;
-}
-
 static void
 address_set_destroy(struct address_set *addr_set)
 {
@@ -118,79 +66,34 @@ address_set_destroy(struct address_set *addr_set)
 }
 
 static void
-update_address_sets(struct controller_ctx *ctx)
-{
-/* Remember the names of all address sets currently in expr_address_sets
- * so we can detect address sets that have been deleted. */
-struct sset cur_addr_set_names = SSET_INITIALIZER(_addr_set_names);
-
-struct shash_node *node;
-SHASH_FOR_EACH (node, _address_sets) {
-sset_add(_addr_set_names, node->name);
-}
+update_address_sets(struct controller_ctx *ctx,
+struct shash *local_address_sets_p,
+struct shash *expr_address_sets_p)
 
+{
 /* Iterate address sets in the southbound database.  Create and update the
  * corresponding symtab entries as necessary. */
 const struct sbrec_address_set *addr_set_rec;
 SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
-struct address_set *addr_set =
-shash_find_data(_address_sets, addr_set_rec->name);
-
-bool create_set = false;
-if (addr_set) {
-/* This address set has already been added.  We must determine
- * if the symtab entry needs to be updated due to a change. */
-sset_find_and_delete(_addr_set_names, addr_set_rec->name);
-if (!address_sets_match(addr_set, addr_set_rec)) {
-shash_find_and_delete(_address_sets, addr_set_rec->name);
-expr_macros_remove(_address_sets, addr_set_rec->name);
-address_set_destroy(addr_set);
-addr_set = NULL;
-create_set = true;
+/* Create a symbol that resolves to the full set of addresses.
+ * Store it in address_sets to remember that we created this
+ * symbol. */
+struct address_set *addr_set = xzalloc(sizeof *addr_set);
+addr_set->n_addresses = addr_set_rec->n_addresses;
+if (addr_set_rec->n_addresses) {
+addr_set->addresses = xmalloc(addr_set_rec->n_addresses
+  * sizeof addr_set->addresses[0]);
+size_t i;
+for (i = 0; i < addr_set_rec->n_addresses; i++) {
+addr_set->addresses[i] = xstrdup(addr_set_rec->addresses[i]);
 }
-} else {
-/* This address set is not yet in the symtab, so add it. */
-   

[ovs-dev] [PATCH 4/4] ovn-controller: Convert encaps module back to full processing

[Ryan Moats]

Commit f5d916cb ("ovn-controller: Back out incremental processing")
left the conversion of the encaps module to a future patch set.
This patch converts this module back to full processing, but
does not remove all persistence of associated data strcutures.

This commit depends on f5d916cb ("ovn-controller:
Back out incremental processing").

Signed-off-by: Ryan Moats 
---
 ovn/controller/encaps.c | 34 +++---
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index d99ba05..87f 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -412,25 +412,21 @@ encaps_run(struct controller_ctx *ctx, const struct 
ovsrec_bridge *br_int,
 /* Maintain a mapping backwards from encap entries to their parent
  * chassis. Most changes happen at the encap row entry but tunnels need
  * to be established on the basis of the overall chassis. */
-SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) {
-/* Defer deletion of mapping until we have cleaned up associated
- * ports. */
-if (!sbrec_chassis_is_deleted(chassis_rec)) {
-for (int i = 0; i < chassis_rec->n_encaps; i++) {
-encap_rec = chassis_rec->encaps[i];
-
-struct encap_hash_node *encap_hash_node;
-encap_hash_node = lookup_encap_uuid(_rec->header_.uuid);
-if (encap_hash_node) {
-/* A change might have invalidated our mapping. Process the
- * new version and then iterate over everything to see if 
it
- * is OK. */
-delete_encap_uuid(encap_hash_node);
-poll_immediate_wake();
-}
-
-insert_encap_uuid(_rec->header_.uuid, chassis_rec);
+SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) {
+for (int i = 0; i < chassis_rec->n_encaps; i++) {
+encap_rec = chassis_rec->encaps[i];
+
+struct encap_hash_node *encap_hash_node;
+encap_hash_node = lookup_encap_uuid(_rec->header_.uuid);
+if (encap_hash_node) {
+/* A change might have invalidated our mapping. Process the
+ * new version and then iterate over everything to see if it
+ * is OK. */
+delete_encap_uuid(encap_hash_node);
+poll_immediate_wake();
 }
+
+insert_encap_uuid(_rec->header_.uuid, chassis_rec);
 }
 }
 
@@ -440,7 +436,7 @@ encaps_run(struct controller_ctx *ctx, const struct 
ovsrec_bridge *br_int,
  * might actually result in the creation of a different type tunnel if
  * that type is preferred. That's OK - when we process the other encap
  * rows, we'll just skip over the new tunnels. */
-SBREC_ENCAP_FOR_EACH_TRACKED (encap_rec, ctx->ovnsb_idl) {
+SBREC_ENCAP_FOR_EACH (encap_rec, ctx->ovnsb_idl) {
 struct encap_hash_node *encap_hash_node;
 struct chassis_hash_node *chassis_hash_node;
 const struct ovsrec_port *port_rec = NULL;
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/4] ovn-controller: add quiet mode

[Ryan Moats]

As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

This commit depends on f5d916cb ("ovn-controller:
Back out incremental processing").

[1] http://openvswitch.org/pipermail/dev/2016-August/078272.html

Signed-off-by: Ryan Moats 
---
 ovn/controller/ofctrl.c |   2 +
 ovn/controller/ovn-controller.c |  73 -
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c  |  11 +
 ovn/controller/physical.c   | 100 
 ovn/controller/physical.h   |   5 ++
 6 files changed, 131 insertions(+), 61 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index d8e111d..5b1434e 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -366,6 +366,8 @@ run_S_CLEAR_FLOWS(void)
 queue_msg(encode_group_mod());
 ofputil_uninit_group_mod();
 
+force_full_process();
+
 /* Clear existing groups, to match the state of the switch. */
 if (groups) {
 ovn_group_table_clear(groups, true);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index c2e667b..cb0ae40 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -304,6 +304,23 @@ get_nb_cfg(struct ovsdb_idl *idl)
 return sb ? sb->nb_cfg : 0;
 }
 
+/* Contains mapping of localvifs to OF ports for detecting if the physical
+ * configuration of the switch has changed. */
+static struct simap localvif_to_ofport =
+SIMAP_INITIALIZER(_to_ofport);
+
+/* Contains the list of known tunnels. */
+static struct hmap tunnels = HMAP_INITIALIZER();
+
+/* The last sequence number seen from the southbound IDL. */
+static unsigned int seqno = 0;
+
+void
+force_full_process(void)
+{
+seqno = 0;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -427,39 +444,51 @@ main(int argc, char *argv[])
 _lports);
 }
 
+enum mf_field_id mff_ovn_geneve;
+bool physical_change = true;
 if (br_int && chassis_id) {
+mff_ovn_geneve = ofctrl_run(br_int);
+physical_change = detect_and_save_physical_changes(
+_to_ofport, , mff_ovn_geneve, br_int,
+chassis_id);
+unsigned int cur_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
+
 patch_run(, br_int, chassis_id, _datapaths,
   _datapaths);
 
 static struct lport_index lports;
-static struct mcgroup_index mcgroups;
 lport_index_init(, ctx.ovnsb_idl);
-mcgroup_index_init(, ctx.ovnsb_idl);
-
-enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int);
-
-pinctrl_run(, , br_int, chassis_id, _datapaths);
+pinctrl_run(, , br_int, chassis_id,
+_datapaths);
 update_ct_zones(_lports, _datapaths, _zones,
 ct_zone_bitmap);
 
-struct hmap flow_table = HMAP_INITIALIZER(_table);
-lflow_run(, , , _datapaths,
-  _datapaths, _table, _zones,
-  _table);
-
-physical_run(, mff_ovn_geneve,
- br_int, chassis_id, _zones, _table,
- _datapaths, _datapaths);
-
-ofctrl_put(_table, get_nb_cfg(ctx.ovnsb_idl));
-hmap_destroy(_table);
-if (ctx.ovnsb_idl_txn) {
-int64_t cur_cfg = ofctrl_get_cur_cfg();
-if (cur_cfg && cur_cfg != chassis->nb_cfg) {
-sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+
+if (physical_change || seqno < cur_seqno) {
+seqno = cur_seqno;
+
+static struct mcgroup_index mcgroups;
+mcgroup_index_init(, ctx.ovnsb_idl);
+
+struct hmap flow_table = HMAP_INITIALIZER(_table);
+lflow_run(, , , _datapaths,
+  _datapaths, _table, _zones,
+  _table);
+
+physical_run(, mff_ovn_geneve,
+ br_int, chassis_id, _zones, _table,
+ _datapaths, _datapaths);
+
+ofctrl_put(_table, get_nb_cfg(ctx.ovnsb_idl));
+hmap_destroy(_table);
+if (ctx.ovnsb_idl_txn) {
+int64_t cur_cfg = ofctrl_get_cur_cfg();
+if (cur_cfg && cur_cfg != chassis->nb_cfg) {
+sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+}
 

[ovs-dev] [PATCH 1/4] ovn-controller: Back out incremental processing

[Ryan Moats]

As [1] indicates, incremental processing hasn't resulted
in an improvement worth the complexity it has added.
This patch backs out all of the code specific to incremental
processing, along with the persisting of OF flows,
logical ports, multicast groups, all_lports, local and patched
datapaths.

Persisted objects in the ovn/controller/physical.c module will
be used by a future patch set to determine if physical changes
have occurred.

Future patch sets will convert the ovn/controller/encaps.c
module back to full processing and remove the persistance of
address sets in the ovn/controller/lflow.c module.

[1] http://openvswitch.org/pipermail/dev/2016-August/078272.html

Signed-off-by: Ryan Moats 
---
 include/ovn/actions.h   |   4 -
 ovn/controller/binding.c| 154 ++
 ovn/controller/binding.h|   1 -
 ovn/controller/encaps.c | 111 ++---
 ovn/controller/lflow.c  | 102 +++-
 ovn/controller/lflow.h  |   4 +-
 ovn/controller/lport.c  | 220 -
 ovn/controller/lport.h  |  24 +--
 ovn/controller/ofctrl.c | 353 +++-
 ovn/controller/ofctrl.h |  16 +-
 ovn/controller/ovn-controller.c |  61 ---
 ovn/controller/patch.c  |   6 -
 ovn/controller/physical.c   | 175 +---
 ovn/controller/physical.h   |   3 +-
 ovn/lib/actions.c   |   1 -
 tests/ovn.at|  55 +++
 tests/system-ovn.at |  45 ++---
 17 files changed, 387 insertions(+), 948 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index e2a716a..b678d33 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -298,7 +298,6 @@ struct group_table {
 struct group_info {
 struct hmap_node hmap_node;
 struct ds group;
-struct uuid lflow_uuid;
 uint32_t group_id;
 };
 
@@ -412,9 +411,6 @@ struct ovnact_encode_params {
 /* A struct to figure out the group_id for group actions. */
 struct group_table *group_table;
 
-/* The logical flow uuid that drove this action. */
-struct uuid lflow_uuid;
-
 /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
  * OpenFlow flow table (ptable).  A number of parameters describe this
  * mapping and data related to flow tables:
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index c26007d..0353a7b 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -30,18 +30,6 @@
 
 VLOG_DEFINE_THIS_MODULE(binding);
 
-/* A set of the iface-id values of local interfaces on this chassis. */
-static struct sset local_ids = SSET_INITIALIZER(_ids);
-
-/* When this gets set to true, the next run will re-check all binding records. 
*/
-static bool process_full_binding = false;
-
-void
-binding_reset_processing(void)
-{
-process_full_binding = true;
-}
-
 void
 binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 {
@@ -64,16 +52,12 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
  _interface_col_ingress_policing_burst);
 }
 
-static bool
+static void
 get_local_iface_ids(const struct ovsrec_bridge *br_int,
 struct shash *lport_to_iface,
 struct sset *all_lports)
 {
 int i;
-bool changed = false;
-
-struct sset old_local_ids = SSET_INITIALIZER(_local_ids);
-sset_clone(_local_ids, _ids);
 
 for (i = 0; i < br_int->n_ports; i++) {
 const struct ovsrec_port *port_rec = br_int->ports[i];
@@ -93,53 +77,9 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
 continue;
 }
 shash_add(lport_to_iface, iface_id, iface_rec);
-if (!sset_find_and_delete(_local_ids, iface_id)) {
-sset_add(_ids, iface_id);
-sset_add(all_lports, iface_id);
-changed = true;
-}
+sset_add(all_lports, iface_id);
 }
 }
-
-/* Any item left in old_local_ids is an ID for an interface
- * that has been removed. */
-if (!changed && !sset_is_empty(_local_ids)) {
-changed = true;
-
-const char *cur_id;
-SSET_FOR_EACH(cur_id, _local_ids) {
-sset_find_and_delete(_ids, cur_id);
-sset_find_and_delete(all_lports, cur_id);
-}
-}
-
-sset_destroy(_local_ids);
-
-return changed;
-}
-
-static struct local_datapath *
-local_datapath_lookup_by_uuid(struct hmap *hmap_p, const struct uuid *uuid)
-{
-struct local_datapath *ld;
-HMAP_FOR_EACH_WITH_HASH(ld, uuid_hmap_node, uuid_hash(uuid), hmap_p) {
-if (uuid_equals(>uuid, uuid)) {
-return ld;
-}
-}
-return NULL;
-}
-
-static void
-remove_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld)
-{
-if (ld->logical_port) {
-free(ld->logical_port);
-ld->logical_port = NULL;
-}
-

Re: [ovs-dev] [PATCH 2/2] ovs-ofctl: Extract tunnel metadata correctly when sorting flows.

[Ben Pfaff]

On Mon, Aug 29, 2016 at 11:58:15AM -0700, Jesse Gross wrote:
> When flow fields are sorted before dumping in ovs-ofctl, each
> significant field is extracted for sorting. However, in the case of
> tunnel metadata a mapping table is necessary to know where each
> field begins and ends. This information is current stripped off before
> fetching the field data and returned field is simply zeroed. This
> makes sorting based on tunnel metadata non-deterministic.
> 
> We have the tunnel allocation stored in match metadata with each
> flow, so we can simply extract the data from there rather than
> trying to build and populate a global mapping table.
> 
> Signed-off-by: Jesse Gross 

Good catch.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 0/4] ovn-controller: Replace incremental processing with quiet mode

[Ryan Moats]

This patch set removes incremental processing and replaces it
with quiet mode: ovn-controller first tests to see if recalculation
is necessary and if so, does a full calculation.

Side effect of this is that almost all data persistence is also reverted.
The exception is the persistence in the encaps.c module - it's not clear
how to revert that while still maintaining 100% unit test pass.

Note: these patches previously existed as individually versioned patches.
There is no change from v7, v4, v1, and v1 (in order).

Ryan Moats (4):
  ovn-controller: Back out incremental processing
  ovn-controller: add quiet mode
  Unpersist data structures for address sets.
  ovn-controller: Convert encaps module back to full processing

 include/ovn/actions.h   |   4 -
 ovn/controller/binding.c| 154 ++
 ovn/controller/binding.h|   1 -
 ovn/controller/encaps.c | 145 +++--
 ovn/controller/lflow.c  | 258 +++--
 ovn/controller/lflow.h  |   4 +-
 ovn/controller/lport.c  | 220 -
 ovn/controller/lport.h  |  24 +--
 ovn/controller/ofctrl.c | 353 +++-
 ovn/controller/ofctrl.h |  16 +-
 ovn/controller/ovn-controller.c | 106 
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c  |  17 +-
 ovn/controller/physical.c   | 263 +++---
 ovn/controller/physical.h   |   8 +-
 ovn/lib/actions.c   |   1 -
 ovn/lib/expr.c  |   1 +
 tests/ovn.at|  55 +++
 tests/system-ovn.at |  45 ++---
 19 files changed, 549 insertions(+), 1127 deletions(-)

-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovs-ofctl: Fix crash with replace-flows and diff-flows with tunnel metadata.

[Ben Pfaff]

On Mon, Aug 29, 2016 at 11:58:14AM -0700, Jesse Gross wrote:
> When flows are read by ovs-ofctl (either from a switch or a file),
> tunnel metadata space is dynamically allocated since there isn't a
> preset table. This works well for single flows but doesn't handle
> groups of flows that must be compared to each other. In this case,
> each flow will have its own independent allocation making comparisons
> meaningless.
> 
> Even worse is that when these matches are later serialized (either
> for display or in NXM format), the metadata allocation has been
> stripped off of the matches. The serialization code then attempts to
> use the global table, which is also not available, leading to a
> dereference of a NULL pointer.
> 
> Solving this problem requires building an overall metadata table.
> Since we don't know the maximum size of a field (particularly for
> flows read from a file), it's necessary to do this in two passes.
> The first pass records the maximum size for each field as well as
> stores the received matches. The second pass creates a metadata
> table based on the sizes, adjusts the match layout based on the new
> allocation, and then replays the stored matches for comparison.
> Later serialization will used the generated table to output the
> flows.
> 
> Signed-off-by: Jesse Gross 

Good catch.

Acked-by: Ben Pfaff 

Suggested improvements:

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

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index dbeeebf..803684c 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -3066,16 +3066,14 @@ generate_tun_metadata(struct fte_state *state)
 static void
 remap_match(struct match *match)
 {
-struct tun_metadata flow, flow_mask;
 int i;
 
 if (!match->tun_md.valid) {
 return;
 }
 
-memcpy(, >flow.tunnel.metadata, sizeof flow);
-memcpy(_mask, >wc.masks.tunnel.metadata, sizeof flow_mask);
-
+struct tun_metadata flow = match->flow.tunnel.metadata;
+struct tun_metadata flow_mask = match->wc.masks.tunnel.metadata;
 memset(>flow.tunnel.metadata, 0, sizeof 
match->flow.tunnel.metadata);
 memset(>wc.masks.tunnel.metadata, 0,
sizeof match->wc.masks.tunnel.metadata);
@@ -3105,7 +3103,7 @@ remap_match(struct match *match)
  * allocated space. In the case of flows coming from a file, we don't
  * even know the size of each field when we need to do the allocation.
  * When the flows come in, each flow has an individual allocation based
- * on it's own fields. However, this allocation is not the same across
+ * on its own fields. However, this allocation is not the same across
  * different flows and therefore fields are not directly comparable.
  *
  * In the first pass, we record the maximum size of each tunnel metadata
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] patch series

[Ryan Moats]

Ben Pfaff  wrote on 08/30/2016 05:05:14 PM:

> From: Ben Pfaff 
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 08/30/2016 05:05 PM
> Subject: patch series
>
> It looks like you have 4 separate patches that are connected and in
> which 3 of them depend on the other one.
>
> Usually, we'd represent that as a 4-patch series.
>

I went back and forth on whether to put them together or not,
and decided to leave the patches that were already there be
and build on top of them, rather than resubmit them with only
a numbering change in the subject.

Given that was the wrong approach, a series will follow shortly...
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-bugtool: Add interfaces file contents.

[Ben Pfaff]

On Mon, Aug 29, 2016 at 04:41:04AM -0700, Gurucharan Shetty wrote:
> It is useful to know the contents of interfaces file
> for debugging in debian based systems.
> 
> Signed-off-by: Gurucharan Shetty 

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn-nbctl: Update manpage.

[Ben Pfaff]

On Sun, Aug 28, 2016 at 02:05:18AM -0700, nickcooper-zhangtonghao wrote:
> Signed-off-by: nickcooper-zhangtonghao 
> 

Code changes and its documentation should be in the same patch.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] patch series

[Ben Pfaff]

It looks like you have 4 separate patches that are connected and in
which 3 of them depend on the other one.

Usually, we'd represent that as a 4-patch series.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovn-controller: Datapath based conntrack zone for load-balancing.

[Guru Shetty]

On 30 August 2016 at 14:50, Ben Pfaff  wrote:

> On Tue, Aug 30, 2016 at 01:42:06PM -0700, Guru Shetty wrote:
> > On 30 August 2016 at 13:03, Ben Pfaff  wrote:
> >
> > > On Tue, Aug 23, 2016 at 02:46:17AM -0700, Gurucharan Shetty wrote:
> > > > Currently ct_lb() logical action is only added for a logical switch
> and
> > > > we use the conntrack zone allocated for the logical port.  A future
> > > commit
> > > > will use ct_lb() for a logical router too.  In that case, use the
> > > allocated
> > > > DNAT zone.
> > > >
> > > > Signed-off-by: Gurucharan Shetty 
> > >
> > > Can you help me to understand why the desired behavior is different in
> > > each of these cases?
> > >
> >
> > Currently we do the following conntrack zone allocations.
> > 1. A conntrack zone for each logical port. This has to be unique only per
> > hypervisor. We use this zone to do both firewall and east-west
> > load-balancing.
> >
> > For firewall, we send the packet to conntrack to defragment it and track
> it
> > and figure out whether it is invalid, new, established etc. Invalid
> packets
> > are dropped. new connections are committed.
> >
> > For load-balancing, defragmented packets are DNATed into one of the
> > possible endpoints. We do the load-balancing at the endpoint (instead of
> > say in a router) because of the asymmetric  nature of OVN router pipeline
> > handling for east-west.
> > So when we see ct_lb() action on a switch, we can just use the conntrack
> > zone allocated for that logical port.
> >
> >
> > 2. Two conntrack zones per gateway router.
> > A gateway router only resides in a particular chassis. We have one
> > conntrack zone for DNAT and another for SNAT.
> >
> > Now when I want to add ct_lb() action for the gateway router, I want to
> use
> > it as part of the gateway router pipeline. Since load-balancing is
> nothing
> > but a DNAT using one of the chosen endpoint, the conntrack zone has to
> be a
> > DNAT zone allocated to that gateway router.
> >
> > Did I give the answer to your question? Or was it something else that you
> > were looking an explanation for?
>
> One underlying question I'm trying to understand is whether this
> difference is something inherent in the definition of logical switches
> and logical routers, and thus the approach taken here of automatically
> choosing a strategy is appropriate, or whether this is just what we
> happen to be doing now, in which case it might be better to make how to
> select the zone a parameter to ct_lb.
>

I see the question now. I don't have a good answer. One way to look at it
would be that a "zone" is an internal implementation detail and should not
be seen in a action of logical flow. But we can then say that we could
rename "zone" as "datapath" in the logical action.  But, then we would be
limiting it to 2 anyway (datapath=lswitch or datapath=lrouter), because
that is all that I can think about - in which case we are inferring it with
the current patch anyway.  I did start with passing this as an argument to
ct_lb, but could not think about a good name. I then decided that for a
stateful OVS action, you probably cannot make a very general purpose
logical action.  Do you have an opinion? I will go with that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovn-controller: Datapath based conntrack zone for load-balancing.

[Ben Pfaff]

On Tue, Aug 30, 2016 at 01:42:06PM -0700, Guru Shetty wrote:
> On 30 August 2016 at 13:03, Ben Pfaff  wrote:
> 
> > On Tue, Aug 23, 2016 at 02:46:17AM -0700, Gurucharan Shetty wrote:
> > > Currently ct_lb() logical action is only added for a logical switch and
> > > we use the conntrack zone allocated for the logical port.  A future
> > commit
> > > will use ct_lb() for a logical router too.  In that case, use the
> > allocated
> > > DNAT zone.
> > >
> > > Signed-off-by: Gurucharan Shetty 
> >
> > Can you help me to understand why the desired behavior is different in
> > each of these cases?
> >
> 
> Currently we do the following conntrack zone allocations.
> 1. A conntrack zone for each logical port. This has to be unique only per
> hypervisor. We use this zone to do both firewall and east-west
> load-balancing.
> 
> For firewall, we send the packet to conntrack to defragment it and track it
> and figure out whether it is invalid, new, established etc. Invalid packets
> are dropped. new connections are committed.
> 
> For load-balancing, defragmented packets are DNATed into one of the
> possible endpoints. We do the load-balancing at the endpoint (instead of
> say in a router) because of the asymmetric  nature of OVN router pipeline
> handling for east-west.
> So when we see ct_lb() action on a switch, we can just use the conntrack
> zone allocated for that logical port.
> 
> 
> 2. Two conntrack zones per gateway router.
> A gateway router only resides in a particular chassis. We have one
> conntrack zone for DNAT and another for SNAT.
> 
> Now when I want to add ct_lb() action for the gateway router, I want to use
> it as part of the gateway router pipeline. Since load-balancing is nothing
> but a DNAT using one of the chosen endpoint, the conntrack zone has to be a
> DNAT zone allocated to that gateway router.
> 
> Did I give the answer to your question? Or was it something else that you
> were looking an explanation for?

One underlying question I'm trying to understand is whether this
difference is something inherent in the definition of logical switches
and logical routers, and thus the approach taken here of automatically
choosing a strategy is appropriate, or whether this is just what we
happen to be doing now, in which case it might be better to make how to
select the zone a parameter to ct_lb.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 6/7] OVSDB: Reimplement replication. Using a state machine.

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:53PM -0700, Andy Zhou wrote:
> Current replication uses blocking transactions, which are error prone
> in practice, especially in handling RPC connection flapping to the
> active server.
> 
> Signed-off-by: Andy Zhou 

Much better.  Thank you.

Acked-by: Ben Pfaff 

Some suggestions below.

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

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index ed73559..0c879fe 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -191,8 +191,7 @@ main_loop(struct ovsdb_jsonrpc_server *jsonrpc, struct 
shash *all_dbs,
 replication_run();
 if (!replication_is_alive()) {
 int retval = replication_get_last_error();
-ovs_fatal(0, "replication connection failed (%s)",
-  ovs_retval_to_string(retval));
+ovs_fatal(retval, "replication connection failed");
 }
 }
 
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 16fd323..b9759b5 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -132,15 +132,13 @@ replication_add_local_db(const char *database, struct 
ovsdb *db)
 void
 replication_run(void)
 {
-struct ovsdb *db;
 if (!session) {
 return;
 }
 
 jsonrpc_session_run(session);
 
-int i;
-for (i = 0; jsonrpc_session_is_connected(session) && i < 50; i++) {
+for (int i = 0; jsonrpc_session_is_connected(session) && i < 50; i++) {
 struct jsonrpc_msg *msg;
 unsigned int seqno;
 
@@ -171,7 +169,7 @@ replication_run(void)
 && msg->params->u.array.n == 2
 && msg->params->u.array.elems[0]->type == JSON_STRING) {
 char *db_name = msg->params->u.array.elems[0]->u.string;
-db = find_db(db_name);
+struct ovsdb *db = find_db(db_name);
 if (db) {
 struct ovsdb_error *error;
 error = process_notification(msg->params->u.array.elems[1],
@@ -182,8 +180,13 @@ replication_run(void)
 }
 }
 }
-} else if (msg->type == JSONRPC_REPLY &&
-   request_ids_lookup_and_free(msg->id, )) {
+} else if (msg->type == JSONRPC_REPLY) {
+struct ovsdb *db;
+if (!request_ids_lookup_and_free(msg->id, )) {
+VLOG_WARN("received unexpected reply");
+goto next;
+}
+
 switch (state) {
 case RPL_S_DB_REQUESTED:
 if (msg->result->type != JSON_ARRAY) {
@@ -199,7 +202,7 @@ replication_run(void)
 if (name->type == JSON_STRING) {
 /* Send one schema request for each remote DB. */
 const char *db_name = json_string(name);
-db = find_db(db_name);
+struct ovsdb *db = find_db(db_name);
 if (db) {
 struct jsonrpc_msg *request =
 jsonrpc_create_request(
@@ -228,8 +231,8 @@ replication_run(void)
 }
 
 if (db != find_db(schema->name)) {
-/* Unexpceted schema */
-VLOG_WARN("unexpcted schema %s", schema->name);
+/* Unexpected schema. */
+VLOG_WARN("unexpected schema %s", schema->name);
 state = RPL_S_ERR;
 } else if (!ovsdb_schema_equal(schema, db->schema)) {
 /* Schmea version mismatch. */
@@ -239,29 +242,29 @@ replication_run(void)
 }
 ovsdb_schema_destroy(schema);
 
-/* After receving schemas, reset the local databases that
+/* After receiving schemas, reset the local databases that
  * will be monitored and send out monitor requests for them. */
 if (hmap_is_empty(_ids)) {
 struct shash_node *node, *next;
 
-SHASH_FOR_EACH_SAFE(node, next, replication_dbs) {
-db = node->data;
+SHASH_FOR_EACH_SAFE (node, next, replication_dbs) {
+struct ovsdb *db = node->data;
 struct ovsdb_error *error = reset_database(db);
 if (error) {
 const char *db_name = db->schema->name;
 shash_find_and_delete(replication_dbs, db_name);
 

Re: [ovs-dev] bug fix for miimon issue with ixgbe driver that half support mii

[David Hill]

Hello sir,

   I do not understand either but that's what's happening.  It's a 
fiber adapter so maybe the driver says it's supported and when trying to 
request values from the nic itself it fails because it's no longer 
supported... but that's only a though .


Thank you very much,

David Hill


On 08/30/2016 04:33 PM, Joe Stringer wrote:

On 30 August 2016 at 12:49, David Hill  wrote:

Hello sir,

We have a customer hitting an issue where ixgbe will not return an error
at "SIOCGMIIPHY" but will hit it later at "SIOCGMIIREG" so instead of
failling at miimon and never doing the failover, we'd like it to use ethtool
which is properly returning the link state.

Hm, I don't understand why ixgbe would support getting the address of
the MII PHY in use, but not to fetch the IMM_BMSR "basic mode status register"
from it. Is this supposed to happen? Have you tried discussing this
with the upstream driver developers?


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [patch v10 2/2] DSCP marking on packets egressing VIF interface

[Mickey Spiegel]

On Mon, Aug 29, 2016 at 4:34 AM,  wrote:

> ovn-northd sets 'ip.dscp' to the DSCP value
>
> IMO the big question is still whether the first release of DSCP marking
should be
based only on ingress port, as this patch currently suggests, or whether it
should
allow DSCP marking based on arbitrary match criteria. I will send out a
separate
message to generate discussion about multiple features (ACLs, QoS, SFC) with
arbitrary match criteria.

Assuming that we reach a conclusion that the first release of DSCP marking
should
be based only on ingress port, a couple of comments inline below.


> Signed-off-by: Babu Shanmugam 
> ---
>  ovn/lib/logical-fields.c|  2 +-
>  ovn/northd/ovn-northd.8.xml | 45 
>  ovn/northd/ovn-northd.c | 72 +++---
> --
>  ovn/ovn-nb.xml  |  6 
>  ovn/ovn-sb.xml  |  5 
>  tests/ovn.at| 73 ++
> +++
>  6 files changed, 161 insertions(+), 42 deletions(-)
>
> diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
> index 6dbb4ae..068c000 100644
> --- a/ovn/lib/logical-fields.c
> +++ b/ovn/lib/logical-fields.c
> @@ -134,7 +134,7 @@ ovn_init_symtab(struct shash *symtab)
>  expr_symtab_add_predicate(symtab, "ip6", "eth.type == 0x86dd");
>  expr_symtab_add_predicate(symtab, "ip", "ip4 || ip6");
>  expr_symtab_add_field(symtab, "ip.proto", MFF_IP_PROTO, "ip", true);
> -expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP, "ip", false);
> +expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP_SHIFTED, "ip",
> false);
>  expr_symtab_add_field(symtab, "ip.ecn", MFF_IP_ECN, "ip", false);
>  expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 3448370..bf96f0e 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -140,7 +140,7 @@
>be dropped.
>  
>
> -Ingress Table 1: Ingress Port Security - IP
> +Ingress Table 1: Ingress Port DSCP
>
>  
>Ingress table 1 contains these logical flows:
> @@ -148,6 +148,25 @@
>
>  
>
> +One priority 100 flow for every port having DSCP setting that sets
> +dscp header in the IP packets egressing the ports and ingressing
> the
> +switch.
>

This is hard to follow. Looking at the flows you are adding in ovn-northd.c,
it seems pretty simple. I don't see anything that I would call "egressing".
How about:
+For every port with a DSCP setting, one priority-100 flow
 that matches the inport on the corresponding
 switch and sets DSCP.


> +  
> +
> +  
> +One priority-0 fallback flow that matches all packets and
> advances to
> +the next table.
> +  
> +
> +
> +Ingress Table 2: Ingress Port Security - IP
> +
> +
> +  Ingress table 2 contains these logical flows:
> +
> +
> +
> +  
>  
>For each element in the port security set having one or more
> IPv4 or
>IPv6 addresses (or both),
> @@ -195,10 +214,10 @@
>
>  
>
> -Ingress Table 2: Ingress Port Security - Neighbor discovery
> +Ingress Table 3: Ingress Port Security - Neighbor discovery
>
>  
> -  Ingress table 2 contains these logical flows:
> +  Ingress table 3 contains these logical flows:
>  
>
>  
> @@ -240,7 +259,7 @@
>
>  
>
> -Ingress Table 3: from-lport Pre-ACLs
> +Ingress Table 4: from-lport Pre-ACLs
>
>  
>This table prepares flows for possible stateful ACL processing in
> @@ -252,7 +271,7 @@
>before eventually advancing to ingress table ACLs.
>  
>
> -Ingress Table 4: Pre-LB
> +Ingress Table 5: Pre-LB
>
>  
>This table prepares flows for possible stateful load balancing
> processing
> @@ -268,7 +287,7 @@
>advancing to ingress table LB.
>  
>
> -Ingress Table 5: Pre-stateful
> +Ingress Table 6: Pre-stateful
>
>  
>This table prepares flows for all possible stateful processing
> @@ -279,7 +298,7 @@
>ct_next; action.
>  
>
> -Ingress table 6: from-lport ACLs
> +Ingress table 7: from-lport ACLs
>
>  
>Logical flows in this table closely reproduce those in the
> @@ -362,7 +381,7 @@
>
>  
>
> -Ingress Table 7: LB
> +Ingress Table 8: LB
>
>  
>It contains a priority-0 flow that simply moves traffic to the next
> @@ -375,7 +394,7 @@
>connection.)
>  
>
> -Ingress Table 8: Stateful
> +Ingress Table 9: Stateful
>
>  
>
> @@ -412,7 +431,7 @@
>
>  
>
> -Ingress Table 9: ARP/ND responder
> +Ingress Table 10: ARP/ND responder
>
>  
>This table implements ARP/ND responder for known IPs.  It contains
> these
> @@ -484,7 +503,7 @@ nd_na {
> 

[ovs-dev] [PATCH] ovs-save: Restore tunnel TLV map before flows.

[Jesse Gross]

Scripts that integrate OVS with a distribution often save and
restore flows across distruptive events, such as an upgrade. The
ovs-save utility generates a script to assist with this.

When flows include tunnel metadata, we also need to restore the
TLV mappings before the flows are re-added. Otherwise, the instance
of OVS receiving the new flows won't know the meaning of these
fields and will ignore them.

Signed-off-by: Jesse Gross 
---
 utilities/ovs-save | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/utilities/ovs-save b/utilities/ovs-save
index d4d3c35..4ae8fde 100755
--- a/utilities/ovs-save
+++ b/utilities/ovs-save
@@ -106,6 +106,12 @@ save_flows () {
 fi
 
 for bridge in "$@"; do
+echo -n "ovs-ofctl add-tlv-map ${bridge} '"
+ovs-ofctl dump-tlv-map br-int | \
+awk '/^ 0x/ {if (cnt != 0) printf ","; \
+ cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
+echo "'"
+
 echo "ovs-ofctl add-flows ${bridge} - << EOF"
 ovs-ofctl dump-flows "${bridge}" | sed -e '/NXST_FLOW/d' \
 -e 's/\(idle\|hard\)_age=[^,]*,//g'
-- 
2.7.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: ovn-ctl support to start ovn db servers in standby mode

[Andy Zhou]

On Thu, Aug 25, 2016 at 6:48 AM,  wrote:

> This patch adds support to start_ovsdb() function in ovn-ctl to start the
> ovn db servers in standby mode. This can be done in the following ways
> 1. Use parameters --ovn-nb-sync-from-addr and --ovn-sb-sync-from-addr to
>set the addresses of the master server.
> 2. Create files $etcdir/ovnnb-master.conf and $etcdir/ovnsb-master.conf
>with the tcp url of the master servers.
>
> If --ovn-nb-sync-from-addr and --ovn-sb-sync-from-addr is used, it will
> overwrite the contents in the $etcdir/*.conf and use that server as the
> master.
>
> Additional functions to promote a standby server to master and demote
> master server to standby mode are also added in this patch
>
> Signed-off-by: Babu Shanmugam 


In the context of OVSDB HA, we have been calling the ovsdb-server pair as
active/backup.
It would be nice if we can avoid using another term "master".  I have no
strong preference
for "active", just want to avoid using another term.

It seems that if demote_ovssb() is followed by restart_ovsdb(),  the role
switching
won't be respected. true?

Thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 5/7] test: Improve replication unit tests

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:52PM -0700, Andy Zhou wrote:
> Replication test currently uses many sleeps that slowes the test down
> and may not be reliable. Remove those sleeps when possible.
> 
> OVSDB servers needs to be killed on test failure. Use on_exit() to
> ensure cleanup happens, so they don't have to be handled for each
> testing step.
> 
> Signed-off-by: Andy Zhou 

Thanks for working on tests.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 4/7] ovsdb: (Re)hide struct db in ovsdb-server.c

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:51PM -0700, Andy Zhou wrote:
> It seems odd that the currently replication implementation moves the
> struct db from ovsdb-server.c (file private) to replication.h (global).
> 
> This patch moves the 'struct db' defintion back into ovsdb-server.c,
> 
> Signed-off-by: Andy Zhou 

Much cleaner, thanks.

Acked-by: Ben Pfaff 

I have the following suggestions.  sparse complains without the () to
(void) changes.

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

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index d4bc80b..40f8498 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -497,7 +497,7 @@ find_db(const struct shash *all_dbs, const char *db_name)
 {
 struct shash_node *node;
 
-SHASH_FOR_EACH(node, all_dbs) {
+SHASH_FOR_EACH (node, all_dbs) {
 struct db *db = node->data;
 if (!strcmp(db->db->schema->name, db_name)) {
 return db;
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 9658145..0ab7727 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -326,7 +326,7 @@ find_db(const char *db_name)
 }
 
 static struct ovsdb_error *
-reset_databases()
+reset_databases(void)
 {
 struct shash_node *db_node;
 struct ovsdb_error *error = NULL;
@@ -536,7 +536,7 @@ add_monitored_table(struct ovsdb_table_schema *table,
 }
 
 static void
-check_for_notifications()
+check_for_notifications(void)
 {
 struct jsonrpc_msg *msg;
 int error;
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 3/7] ovsdb: Add request_ids

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:50PM -0700, Andy Zhou wrote:
> When starting, the replication logic may issue multiple requests at
> a time, for example, one monitor request for each databases. The
> request_ids keeps track of all outsanding request IDs that are used
> for matching reply message with. It also provides the 'db' context
> for the reply.
> 
> Future patches will make use of this facility.
> 
> Signed-off-by: Andy Zhou 

Seems like useful infrastructure, thanks.

Acked-by: Ben Pfaff 

I have the following suggestions.

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

diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 815730d..0445221 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -789,16 +789,17 @@ request_ids_add(const struct json *id, struct ovsdb *db)
 
 /* Look up 'id' from 'request_ids', if found, remove the found id from
  * 'request_ids' and free its memory. If not found, 'request_ids' does
- * not change.  '*db' is only valid when return true.
+ * not change.  Sets '*db' to the database for the request (NULL if not
+ * found).
  *
- * Return ture if 'id' is found. False otherwise.
+ * Return true if 'id' is found, false otherwise.
  */
 bool
 request_ids_lookup_and_free(const struct json *id, struct ovsdb **db)
 {
 struct request_ids_hmap_node *node;
 
-HMAP_FOR_EACH_IN_BUCKET (node, hmap, json_hash(id, 0), _ids) {
+HMAP_FOR_EACH_WITH_HASH (node, hmap, json_hash(id, 0), _ids) {
 if (json_equal(id, node->request_id)) {
 hmap_remove(_ids, >hmap);
 *db = node->db;
@@ -808,6 +809,7 @@ request_ids_lookup_and_free(const struct json *id, struct 
ovsdb **db)
 }
 }
 
+*db = NULL;
 return false;
 }
 
@@ -826,13 +828,7 @@ request_ids_destroy(void)
 void
 request_ids_clear(void)
 {
-struct request_ids_hmap_node *node;
-
-HMAP_FOR_EACH_POP (node, hmap, _ids) {
-json_destroy(node->request_id);
-free(node);
-}
-hmap_destroy(_ids);
+request_ids_destroy();
 hmap_init(_ids);
 }
 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 2/7] ovsdb: Add blacklist_tables

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:49PM -0700, Andy Zhou wrote:
> Currently, 'sync-exclude-tables' command line options are simply stored
> in a string. Change the implementation to store it in an shash instead
> to improve modularity.
> 
> One additional benefit of this change is that errors can be detected
> and reported to user earlier.  Adde a 'dryrun' option to
> set_blacklist_tables() API to make this feature available to the
> command line option parsing and unixctl command parsing.
> 
> Signed-off-by: Andy Zhou 

Seems useful enough.

I have a few minor suggested improvements, see below.

Acked-by: Ben Pfaff 


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

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index b1d7fc5..ed067d4 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1110,9 +1110,10 @@ ovsdb_server_set_sync_excluded_tables(struct 
unixctl_conn *conn,
 if (is_backup_server) {
 replication_init();
 }
-set_blacklist_tables(argv[1], false);
+err = set_blacklist_tables(argv[1], false);
 }
 unixctl_command_reply(conn, err);
+free(err);
 }
 
 static void
@@ -1463,8 +1464,7 @@ parse_options(int *argcp, char **argvp[],
 case OPT_SYNC_EXCLUDE: {
 char *err = set_blacklist_tables(optarg, false);
 if (err) {
-printf("%s\n", err);
-exit(EXIT_FAILURE);
+ovs_fatal(0, "%s", err);
 }
 break;
 }
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index ebd348c..11384a1 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -145,13 +145,14 @@ get_active_ovsdb_server(void)
 return active_ovsdb_server;
 }
 
-/* Parse 'blacklist' to rebuild 'blacklist_tables'. The current
- * black list tables will be wiped out, regardless if 'blacklist'
- * can be parsed or not.
+/* Parse 'blacklist' to rebuild 'blacklist_tables'.  If 'dryrun' is false, the
+ * current black list tables will be wiped out, regardless of whether
+ * 'blacklist' can be parsed.  If 'dryrun' is true, only parses 'blacklist' and
+ * reports any errors, without modifying the blacklist.
  *
- * On error, Returns the error string which the caller is
- * responsible for freeing. Return NULL otherwise.*/
-char *
+ * On error, returns the error string, which the caller is
+ * responsible for freeing. Returns NULL otherwise. */
+char * OVS_WARN_UNUSED_RESULT
 set_blacklist_tables(const char *blacklist, bool dryrun)
 {
 struct sset set = SSET_INITIALIZER();
@@ -183,13 +184,13 @@ set_blacklist_tables(const char *blacklist, bool dryrun)
 done:
 sset_destroy();
 if (err && !dryrun) {
-/* On error, destory the partially built 'blacklist_tables'. */
+/* On error, destroy the partially built 'blacklist_tables'. */
 blacklist_tables_clear();
 }
 return err;
 }
 
-char *
+char * OVS_WARN_UNUSED_RESULT
 get_blacklist_tables(void)
 {
 struct shash_node *node;
@@ -201,9 +202,7 @@ get_blacklist_tables(void)
 struct sset *tables = node->data;
 
 SSET_FOR_EACH (table, tables) {
-struct ds ds= DS_EMPTY_INITIALIZER;
-ds_put_format(, "%s:%s", database, table);
-sset_add(, ds_steal_cstr());
+sset_add_and_free(, xasprintf("%s:%s", database, table));
 }
 }
 
@@ -212,7 +211,7 @@ get_blacklist_tables(void)
  * that used to implement the hmap data structure. This is
  * only useful for writting unit tests.  */
 const char **sorted = sset_sort();
-struct ds ds= DS_EMPTY_INITIALIZER;
+struct ds ds = DS_EMPTY_INITIALIZER;
 size_t i;
 for (i = 0; i < sset_count(); i++) {
 ds_put_format(, "%s,", sorted[i]);
@@ -256,7 +255,7 @@ static bool
 blacklist_tables_find(const char *database, const char *table)
 {
 struct sset *tables = shash_find_data(_tables, database);
-return tables ? sset_contains(tables, table) : false;
+return tables && sset_contains(tables, table);
 }
 
 void
diff --git a/ovsdb/replication.h b/ovsdb/replication.h
index 1de9c8b..cc79199 100644
--- a/ovsdb/replication.h
+++ b/ovsdb/replication.h
@@ -21,7 +21,6 @@
 #include 
 #include "openvswitch/shash.h"
 
-
 struct db {
 /* Initialized in main(). */
 char *filename;
@@ -42,8 +41,9 @@ void replication_usage(void);
 /* Unixctl APIs */
 void set_active_ovsdb_server(const char *remote_server);
 const char *get_active_ovsdb_server(void);
-char *set_blacklist_tables(const char *blacklist, bool dryrun);
-char *get_blacklist_tables(void);
+char *set_blacklist_tables(const char *blacklist, bool dryrun)
+

Re: [ovs-dev] [replication SMv2 1/7] ovsdb: Properly handle error returned from from reset_database()

[Ben Pfaff]

On Fri, Aug 26, 2016 at 04:15:48PM -0700, Andy Zhou wrote:
> Fix a memory leak in case of error. The error object was not properly
> disposed.  Since the error to reset DB is not expected, log it and
> exit.
> 
> Signed-off-by: Andy Zhou 

Thanks.

Acked-by: Ben Pfaff 

None of it actually matters, but I'd tend to fold in the following:

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

diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 6681a20..d3bc7c1 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -33,7 +33,7 @@
 #include "table.h"
 #include "transaction.h"
 
-VLOG_DEFINE_THIS_MODULE(replication)
+VLOG_DEFINE_THIS_MODULE(replication);
 
 static char *active_ovsdb_server;
 static struct jsonrpc *rpc;
@@ -95,10 +95,9 @@ replication_run(struct shash *all_dbs)
 /* In case reset DB fails, log the error before exiting.  */
 char *msg = ovsdb_error_to_string(error);
 ovsdb_error_destroy(error);
-VLOG_FATAL("Failed to reset DB, (%s)", msg);
-} else {
-reset_dbs = false;
+VLOG_FATAL("Failed to reset DB (%s).", msg);
 }
+reset_dbs = false;
 }
 
 /* Open JSON-RPC. */
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] system-traffic: add a bonding test case

[Lance Richardson]

Add a test case to check connectivity over an OVS bond, using a
Linux bond over veth interfaces.

Also added a new macro "ADD_VETH_BOND", modeled after "ADD_VETH",
in anticipation of future additional bonding test cases.

Signed-off-by: Lance Richardson 
---
 tests/system-common-macros.at | 37 +
 tests/system-traffic.at   | 27 +++
 2 files changed, 64 insertions(+)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 27a9652..252ec89 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -88,6 +88,43 @@ m4_define([ADD_VETH],
 ]
 )
 
+# ADD_VETH_BOND([ports], [namespace], [ovs-br], [bond], [mode], [ip_addr])
+#
+# Add a set of veth port pairs. Ports named in the list 'ports' will be added 
to
+# name space 'namespace', and the corresponding port names, prefixed by 'ovs-'
+# will be included in an OVS bond 'bond' which is added to bridge 'ovs-br'.
+#
+# The 'bond' in 'namespace' will be brought up with static IP address
+# with 'ip_addr' in CIDR notation.
+#
+m4_define([ADD_VETH_BOND],
+[
+  BONDPORTS=""
+  for port in $1; do
+  AT_CHECK([ip link add $port type veth peer name ovs-$port])
+  CONFIGURE_VETH_OFFLOADS([$port])
+  AT_CHECK([ip link set $port netns $2])
+  AT_CHECK([ip link set dev ovs-$port up])
+  BONDPORTS="$BONDPORTS ovs-$port"
+  on_exit 'ip link del ovs-$port'
+  done
+  NS_CHECK_EXEC([$2], [ip link add name $4 type bond])
+  case "$(echo $5 | sed 's/.*lacp=//' | sed 's/ .*//')" in
+  active|passive)
+  NS_CHECK_EXEC([$2], [sh -c "echo 802.3ad > 
/sys/class/net/$4/bonding/mode"])
+  NS_CHECK_EXEC([$2], [sh -c "echo 100 > 
/sys/class/net/$4/bonding/miimon"])
+  ;;
+  esac
+  for port in $1; do
+  NS_CHECK_EXEC([$2], [ip link set dev $port master $4])
+  done
+  NS_CHECK_EXEC([$2], [ip addr add $6 dev $4])
+  NS_CHECK_EXEC([$2], [ip link set dev $4 up])
+  AT_CHECK([ovs-vsctl add-bond $3 ovs-$4 $BONDPORTS $5])
+  on_exit 'ip link del ovs-$4'
+]
+)
+
 # ADD_VLAN([port], [namespace], [vlan-id], [ip-addr])
 #
 # Add a VLAN device named 'port' within 'namespace'. It will be configured
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 10b2647..aec5999 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -128,6 +128,33 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 
fc00:1::2 | FORMAT_PI
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - ping over bond])
+OVS_TRAFFIC_VSWITCHD_START()
+
+AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH_BOND(p1 p2, at_ns1, br0, bond0, lacp=active bond_mode=balance-tcp, 
"10.1.1.2/24")
+
+OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2])
+
+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
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 1600 -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
+])
+NS_CHECK_EXEC([at_ns0], [ping -s 3200 -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
+])
+
+ovs-appctl dpif/dump-flows br0
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([datapath - ping over vxlan tunnel])
 OVS_CHECK_VXLAN()
 
-- 
2.5.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovn-controller: Datapath based conntrack zone for load-balancing.

[Guru Shetty]

On 30 August 2016 at 13:03, Ben Pfaff  wrote:

> On Tue, Aug 23, 2016 at 02:46:17AM -0700, Gurucharan Shetty wrote:
> > Currently ct_lb() logical action is only added for a logical switch and
> > we use the conntrack zone allocated for the logical port.  A future
> commit
> > will use ct_lb() for a logical router too.  In that case, use the
> allocated
> > DNAT zone.
> >
> > Signed-off-by: Gurucharan Shetty 
>
> Can you help me to understand why the desired behavior is different in
> each of these cases?
>

Currently we do the following conntrack zone allocations.
1. A conntrack zone for each logical port. This has to be unique only per
hypervisor. We use this zone to do both firewall and east-west
load-balancing.

For firewall, we send the packet to conntrack to defragment it and track it
and figure out whether it is invalid, new, established etc. Invalid packets
are dropped. new connections are committed.

For load-balancing, defragmented packets are DNATed into one of the
possible endpoints. We do the load-balancing at the endpoint (instead of
say in a router) because of the asymmetric  nature of OVN router pipeline
handling for east-west.
So when we see ct_lb() action on a switch, we can just use the conntrack
zone allocated for that logical port.


2. Two conntrack zones per gateway router.
A gateway router only resides in a particular chassis. We have one
conntrack zone for DNAT and another for SNAT.

Now when I want to add ct_lb() action for the gateway router, I want to use
it as part of the gateway router pipeline. Since load-balancing is nothing
but a DNAT using one of the chosen endpoint, the conntrack zone has to be a
DNAT zone allocated to that gateway router.

Did I give the answer to your question? Or was it something else that you
were looking an explanation for?

PS: The second patch of the series did not make it to patchwork. It is here:
http://openvswitch.org/pipermail/dev/2016-August/078478.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] ovsdb-idlc: Fix memory leaks in add and remove clause functions.

[Ben Pfaff]

Thanks.  I applied this to master and branch-2.6.

On Fri, Aug 26, 2016 at 03:21:19PM -0700, Amitabha Biswas wrote:
> v2 addresses the concern I had about v1. So…
> 
> Acked-by: Amitabha Biswas >
> 
> > On Aug 26, 2016, at 2:18 PM, Ben Pfaff  wrote:
> > 
> > Found by inspection.
> > 
> > Signed-off-by: Ben Pfaff 
> > ---
> > v1->v2: Fix some more leaks.  (Thanks to Amitabha Biswas.)
> > 
> > ovsdb/ovsdb-idlc.in | 32 
> > 1 file changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> > index fc574b4..db4fa32 100755
> > --- a/ovsdb/ovsdb-idlc.in
> > +++ b/ovsdb/ovsdb-idlc.in
> > @@ -957,6 +957,8 @@ void
> >function,
> >&%(s)s_columns[%(S)s_COL_%(C)s],
> >);
> > +
> > +ovsdb_datum_destroy(, &%(s)s_col_%(c)s.type);
> > }
> > """ % {'t': tableName,
> >'T': tableName.upper(),
> > @@ -986,6 +988,7 @@ void
> >  'args': ', '.join(['%(type)s%(name)s' % m for m in 
> > members])}
> > print "{"
> > print "struct ovsdb_datum datum;"
> > +free = []
> > if type.n_min == 1 and type.n_max == 1:
> > print "union ovsdb_atom key;"
> > if type.value:
> > @@ -1032,14 +1035,16 @@ void
> > print "ovs_assert(inited);"
> > print "datum.n = %s;" % nVar
> > print "datum.keys = %s ? xmalloc(%s * sizeof 
> > *datum.keys) : NULL;" % (nVar, nVar)
> > +free += ['datum.keys']
> > if type.value:
> > print "datum.values = xmalloc(%s * sizeof 
> > *datum.values);" % nVar
> > +free += ['datum.values']
> > else:
> > print "datum.values = NULL;"
> > print "for (i = 0; i < %s; i++) {" % nVar
> > -print "" + type.key.copyCValue("datum.keys[i].%s" 
> > % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)
> > +print "" + 
> > type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % 
> > type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)
> > if type.value:
> > -print "" + 
> > type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), 
> > "%s[i]" % valueVar, refTable=False)
> > +print "" + 
> > type.value.assign_c_value_casting_away_const("datum.values[i].%s" % 
> > type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)
> > print "}"
> > if type.value:
> > valueType = type.value.toAtomicType()
> > @@ -1051,8 +1056,8 @@ void
> > print"""ovsdb_idl_condition_add_clause(idl, 
> > &%(p)stable_classes[%(P)sTABLE_%(T)s],
> >   function,
> >   &%(s)s_columns[%(S)s_COL_%(C)s],
> > -  );
> > -}""" % {'t': tableName,
> > +  );\
> > +""" % {'t': tableName,
> >'T': tableName.upper(),
> >'p': prefix,
> >'P': prefix.upper(),
> > @@ -1060,6 +1065,9 @@ void
> >'S': structName.upper(),
> >'c': columnName,
> >'C': columnName.upper()}
> > +for var in free:
> > +print "free(%s);" % var
> > +print "}"
> > 
> > print """void
> > %(s)s_add_clause_false(struct ovsdb_idl *idl)
> > @@ -1123,6 +1131,8 @@ void
> >   function,
> >   &%(s)s_columns[%(S)s_COL_%(C)s],
> >   );
> > +
> > +ovsdb_datum_destroy(, &%(s)s_col_%(c)s.type);
> > }
> > """ % {'t': tableName,
> >'T': tableName.upper(),
> > @@ -1152,6 +1162,7 @@ void
> >  'args': ', '.join(['%(type)s%(name)s' % m for m in 
> > members])}
> > print "{"
> > print "struct ovsdb_datum datum;"
> > +free = []
> > if type.n_min == 1 and type.n_max == 1:
> > print "union ovsdb_atom key;"
> > if type.value:
> > @@ -1198,14 +1209,16 @@ void
> > print "ovs_assert(inited);"
> > print "datum.n = %s;" % nVar
> > print "datum.keys = %s ? xmalloc(%s * sizeof 
> > *datum.keys) : NULL;" % (nVar, nVar)
> > +free += ['datum.keys']
> > if type.value:
> > +free += ['datum.values']
> > print "datum.values = xmalloc(%s * sizeof 
> > *datum.values);" % nVar
> > else:
> > print "datum.values = NULL;"
> > print "for (i = 0; i < %s; i++) {" % 

Re: [ovs-dev] bug fix for miimon issue with ixgbe driver that half support mii

[Joe Stringer]

On 30 August 2016 at 12:49, David Hill  wrote:
> Hello sir,
>
>We have a customer hitting an issue where ixgbe will not return an error
> at "SIOCGMIIPHY" but will hit it later at "SIOCGMIIREG" so instead of
> failling at miimon and never doing the failover, we'd like it to use ethtool
> which is properly returning the link state.

Hm, I don't understand why ixgbe would support getting the address of
the MII PHY in use, but not to fetch the IMM_BMSR "basic mode status register"
from it. Is this supposed to happen? Have you tried discussing this
with the upstream driver developers?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn: ovn-ctl support to start ovn db servers in standby mode

[Ben Pfaff]

On Thu, Aug 25, 2016 at 07:18:03PM +0530, bscha...@redhat.com wrote:
> This patch adds support to start_ovsdb() function in ovn-ctl to start the
> ovn db servers in standby mode. This can be done in the following ways
> 1. Use parameters --ovn-nb-sync-from-addr and --ovn-sb-sync-from-addr to
>set the addresses of the master server.
> 2. Create files $etcdir/ovnnb-master.conf and $etcdir/ovnsb-master.conf
>with the tcp url of the master servers.
> 
> If --ovn-nb-sync-from-addr and --ovn-sb-sync-from-addr is used, it will
> overwrite the contents in the $etcdir/*.conf and use that server as the
> master.
> 
> Additional functions to promote a standby server to master and demote
> master server to standby mode are also added in this patch
> 
> Signed-off-by: Babu Shanmugam 

Andy, do you want to review this?

I have a few comments of my own.

> @@ -54,6 +70,15 @@ start_ovsdb () {
>  
>  set "$@" --detach $OVN_NB_LOG --log-file=$OVN_NB_LOGFILE 
> --remote=punix:$DB_NB_SOCK --remote=ptcp:$DB_NB_PORT:$DB_NB_ADDR 
> --pidfile=$DB_NB_PID --unixctl=ovnnb_db.ctl
>  
> +ovnnb_master_conf_file="$etcdir/ovnnb-master.conf"

The argument to -z should be in "" here:

> +if test ! -z $DB_NB_SYNC_FROM_ADDR; then
> +echo "tcp:$DB_NB_SYNC_FROM_ADDR:$DB_NB_SYNC_FROM_PORT" > 
> $ovnnb_master_conf_file
> +fi

Also in another case later.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1] Python IDL Mutate Fix

[Ben Pfaff]

On Tue, Aug 23, 2016 at 10:12:30PM -0700, Amitabha Biswas wrote:
> This patch fixes the scenario, where the mutate operation on a row
> is sent in the same transaction as row insert operation. It was
> obvserved that this mutate operation was not getting committed
> to the OVSDB.
> 
> To get around the above problem the "where" condition in an
> mutate operation is modified to use the named-uuid to identify
> a row created in the current transaction.
> 
> Signed-off-by: Amitabha Biswas 
> Suggested-by: Richard Theis 

Thanks, applied to master and branch-2.6.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] tun-metadata: Manage tunnel TLV mapping table on a per-bridge basis.

[Ben Pfaff]

On Tue, Aug 23, 2016 at 02:28:27PM -0700, Jesse Gross wrote:
> When using tunnel TLVs (at the moment, this means Geneve options), a
> controller must first map the class and type onto an appropriate OXM
> field so that it can be used in OVS flow operations. This table is
> managed using OpenFlow extensions.
> 
> The original code that added support for TLVs made the mapping table
> global as a simplification. However, this is not really logically
> correct as the OpenFlow management commands are operating on a per-bridge
> basis. This removes the original limitation to make the table per-bridge.
> 
> One nice result of this change is that it is generally clearer whether
> the tunnel metadata is in datapath or OpenFlow format. Rather than
> allowing ad-hoc format changes and trying to handle both formats in the
> tunnel metadata functions, the format is more clearly separated by function.
> Datapaths (both kernel and userspace) use datapath format and it is not
> changed during the upcall process. At the beginning of action translation,
> tunnel metadata is converted to OpenFlow format and flows and wildcards
> are translated back at the end of the process.
> 
> As an additional benefit, this change improves performance in some flow
> setup situations by keeping the tunnel metadata in the original packet
> format in more cases. This helps when copies need to be made as the amount
> of data touched is only what is present in the packet rather than the
> maximum amount of metadata supported.
> 
> Co-authored-by: Madhu Challa 
> Signed-off-by: Madhu Challa 
> Signed-off-by: Jesse Gross 

It's not really great to have the library layer looking into the ofproto
layer.  Is there a way to avoid that?  Perhaps callers should pass in a
tun_table instead of an ofproto?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] netdev-dpdk: fix cross compilation failure

[Ben Pfaff]

On Wed, Aug 24, 2016 at 12:10:14AM +0530, Hemant Agrawal wrote:
> configure with_dpdk and cross compile fails with:
> "configure: error: cannot check for file existence
> when cross compiling"
> 
> This is due to the usages of AC_CHECK_FILES.
> 
> AC_CHECK_FILES only works when not cross compiling. It test
> a feature of the host machine, and therefore, die when
> cross-compiling.
> 
> The current patch put the check in condition, i.e. check only
> if not cross-compiling.
> 
> This patch has been tested on ARM64 platform with DPDK16.07
> 
> Signed-off-by: Hemant Agrawal 

Where's patch 2/2?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 0/2] load-balancer fix for north-south direction.

[Ben Pfaff]

On Tue, Aug 23, 2016 at 02:46:16AM -0700, Gurucharan Shetty wrote:
> So far, we have had the ability to add a load-balancer to a switch.  This 
> works
> fine for east-west traffic.  It was assumed (and lightly tested) that it also
> works for north-south traffic by adding the load_balancer object to the 
> lswitch
> connected to the local network.
> 
> But, in more extensive testing, it was observed that this does not work
> when we add a SNAT rule to the gateway router. That rule is in a
> different zone and will intercept traffic heading out the logical
> topology without any knowledge about the load balancer rule added in a
> different zone.
> 
> This fix, lets us add the load_balancer to the gateway router too. It has the
> added benefit that it works fine with floating IPs for north-south traffic.
> i.e., floating ips can be load-balanced to logical ips.

Would it be useful to add this text to the commit messages?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ovn-controller: Datapath based conntrack zone for load-balancing.

[Ben Pfaff]

On Tue, Aug 23, 2016 at 02:46:17AM -0700, Gurucharan Shetty wrote:
> Currently ct_lb() logical action is only added for a logical switch and
> we use the conntrack zone allocated for the logical port.  A future commit
> will use ct_lb() for a logical router too.  In that case, use the allocated
> DNAT zone.
> 
> Signed-off-by: Gurucharan Shetty 

Can you help me to understand why the desired behavior is different in
each of these cases?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 14/15] ofproto: Refactor packet_out handling.

[Ben Pfaff]

On Tue, Aug 30, 2016 at 11:28:45AM -0700, Jarno Rajahalme wrote:
> > On Aug 30, 2016, at 8:40 AM, Ben Pfaff  wrote:
> > I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
> > seems like a layer violation.
> 
> I think this was due to the plate cache only. The learn action entries in 
> late cache use the struct ofproto_flow_mod, which the ofproto.c can operate 
> on. Apart from bundle revert these could be handled from ofproto-dpif.c with 
> calls to ofproto.c functions from there. To make the revert work I’d need to 
> add a new ofproto-provider class member function and have it iterate through 
> the late cache and then call back to ofproto.c to revert learned flows.
> 
> Also, making the late cache a module of it’s own would remove the inclusion 
> of ofproto-dpif-xlate.h. Would this alleviate your concern with layer 
> violation, or should I strive to keep the xlate cache opaque to ofproto.c?

It would help a lot, if it's practical.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] bug fix for miimon issue with ixgbe driver that half support mii

[David Hill]

Hello sir,

   We have a customer hitting an issue where ixgbe will not return an 
error at "SIOCGMIIPHY" but will hit it later at "SIOCGMIIREG" so instead 
of failling at miimon and never doing the failover, we'd like it to use 
ethtool which is properly returning the link state.


Thank you very much,

David Hill

On 08/30/2016 03:45 PM, Joe Stringer wrote:

On 30 August 2016 at 12:21, David Hill  wrote:



Can you explain what you're trying to achieve with this patch?
* What is the behaviour
* Environment
* How this patch fixes the issue


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] bug fix for miimon issue with ixgbe driver that half support mii

[Joe Stringer]

On 30 August 2016 at 12:21, David Hill  wrote:
>
>

Can you explain what you're trying to achieve with this patch?
* What is the behaviour
* Environment
* How this patch fixes the issue
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] bug fix for miimon issue with ixgbe driver that half support mii

[David Hill]

Subject: bug fix for miimon issue with ixgbe driver that half support mii
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index efc9527..05aa91f 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1426,6 +1426,33 @@ netdev_linux_do_miimon(const char *name, int cmd, const 
char *cmd_name,
 }
 
 static int
+netdev_linux_get_ethtool(const char *name, bool *miimon)
+{
+struct mii_ioctl_data data;
+int error;
+
+*miimon = false;
+
+struct ethtool_cmd ecmd;
+VLOG_DBG_RL(, "%s: failed to query MII, falling back to ethtool",
+name);
+
+COVERAGE_INC(netdev_get_ethtool);
+memset(, 0, sizeof ecmd);
+error = netdev_linux_do_ethtool(name, , ETHTOOL_GLINK,
+"ETHTOOL_GLINK");
+if (!error) {
+struct ethtool_value eval;
+memcpy(, , sizeof eval)
+*miimon = !!eval.data;
+} else {
+VLOG_WARN_RL(, "%s: ethtool link status failed", name);
+}
+ 
+return error;
+}
+
+static int
 netdev_linux_get_miimon(const char *name, bool *miimon)
 {
 struct mii_ioctl_data data;
@@ -1443,28 +1470,11 @@ netdev_linux_get_miimon(const char *name, bool *miimon)
 
 if (!error) {
 *miimon = !!(data.val_out & BMSR_LSTATUS);
-} else {
-VLOG_WARN_RL(, "%s: failed to query MII", name);
-}
-} else {
-struct ethtool_cmd ecmd;
-
-VLOG_DBG_RL(, "%s: failed to query MII, falling back to ethtool",
-name);
-
-COVERAGE_INC(netdev_get_ethtool);
-memset(, 0, sizeof ecmd);
-error = netdev_linux_do_ethtool(name, , ETHTOOL_GLINK,
-"ETHTOOL_GLINK");
-if (!error) {
-struct ethtool_value eval;
-
-memcpy(, , sizeof eval);
-*miimon = !!eval.data;
-} else {
-VLOG_WARN_RL(, "%s: ethtool link status failed", name);
 }
 }
+if (error) {
+  error = netdev_linux_get_ethtool(name, miimon)
+}
 
 return error;
 }
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] bridge: Honor 'mtu_request' when port is added.

[Daniele Di Proietto]

'mtu_request' was honored only when the port was reconfigured, not when
the port was added.

This commit fixes the problem and improves a testcase to detect the bug.

Found by inspection.

Fixes: 56abcf497b56("vswitchd: Introduce 'mtu_request' column in
Interface.")
Signed-off-by: Daniele Di Proietto 
---
 tests/ofproto-dpif.at |  5 +
 vswitchd/bridge.c | 29 +
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 3638063..d7705da 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8887,5 +8887,10 @@ AT_CHECK([ovs-vsctl del-port br0 p1])
 # When 'p1' is deleted, the internal port should return to the default MTU
 AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1500])
 
+# New port with 'mtu_request' in the same transaction.
+AT_CHECK([ovs-vsctl add-port br0 p2 -- set int p2 type=dummy mtu_request=1600])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1600])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 720e615..a7a12ec 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -721,6 +721,24 @@ add_ofp_port(ofp_port_t port, ofp_port_t *ports, size_t 
*n, size_t *allocated)
 return ports;
 }
 
+/* Configures the MTU of 'netdev' based on the "mtu_request" column
+ * in 'iface_cfg'.  'br_type' must be the datapath_type of the bridge
+ * which contains 'netdev'. */
+static int
+iface_set_netdev_mtu(const struct ovsrec_interface *iface_cfg,
+ const char *br_type, struct netdev *netdev)
+{
+if (iface_cfg->n_mtu_request == 1
+&& strcmp(netdev_get_type(netdev),
+  ofproto_port_open_type(br_type, "internal"))) {
+/* Try to set the MTU to the requested value.  This is not done
+ * for internal interfaces, since their MTU is decided by the
+ * ofproto module, based on other ports in the bridge. */
+return netdev_set_mtu(netdev, *iface_cfg->mtu_request);
+}
+return 0;
+}
+
 static void
 bridge_delete_or_reconfigure_ports(struct bridge *br)
 {
@@ -775,14 +793,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 goto delete;
 }
 
-if (iface->cfg->n_mtu_request == 1
-&& strcmp(iface->type,
-  ofproto_port_open_type(br->type, "internal"))) {
-/* Try to set the MTU to the requested value.  This is not done
- * for internal interfaces, since their MTU is decided by the
- * ofproto module, based on other ports in the bridge. */
-netdev_set_mtu(iface->netdev, *iface->cfg->mtu_request);
-}
+iface_set_netdev_mtu(iface->cfg, br->type, iface->netdev);
 
 /* If the requested OpenFlow port for 'iface' changed, and it's not
  * already the correct port, then we might want to temporarily delete
@@ -1725,6 +1736,8 @@ iface_do_create(const struct bridge *br,
 goto error;
 }
 
+iface_set_netdev_mtu(iface_cfg, br->type, netdev);
+
 *ofp_portp = iface_pick_ofport(iface_cfg);
 error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
 if (error) {
-- 
2.9.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] ofproto: Always set MTU for new internal ports.

[Daniele Di Proietto]

We only changed the MTU of new internal ports if it is bigger than the
bridge minimum.  But when the minimum MTU of the bridge is updated we
change the MTU of all internal ports no matter what.

The behavior is inconsistent, because now the internal ports MTU depends
on the order in which the ports where added.

This commit fixes the problem by _always_ setting the MTU of new
internal ports to the bridge minimum.  I'm not sure what was the logic
behind only adjusting the mtu if it was too big.

A testcase is improved to detect the problem.

VMware-BZ: #1718776
Signed-off-by: Daniele Di Proietto 
---
 ofproto/ofproto.c | 10 +-
 tests/ofproto-dpif.at |  9 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index da7372d..9d62b72 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -281,6 +281,8 @@ static void meter_insert_rule(struct rule *);
 /* unixctl. */
 static void ofproto_unixctl_init(void);
 
+static int find_min_mtu(struct ofproto *p);
+
 /* All registered ofproto classes, in probe order. */
 static const struct ofproto_class **ofproto_classes;
 static size_t n_ofproto_classes;
@@ -514,7 +516,7 @@ ofproto_create(const char *datapath_name, const char 
*datapath_type,
 hmap_init(>learned_cookies);
 ovs_list_init(>expirable);
 ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name);
-ofproto->min_mtu = INT_MAX;
+ofproto->min_mtu = find_min_mtu(ofproto);
 cmap_init(>groups);
 ovs_mutex_unlock(_mutex);
 ofproto->ogf.types = 0xf;
@@ -2750,10 +2752,8 @@ update_mtu(struct ofproto *p, struct ofport *port)
 return;
 }
 if (ofport_is_internal(p, port)) {
-if (dev_mtu > p->min_mtu) {
-   if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
-   dev_mtu = p->min_mtu;
-   }
+if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
+dev_mtu = p->min_mtu;
 }
 port->mtu = dev_mtu;
 return;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d7705da..8e5fde6 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8863,6 +8863,11 @@ AT_CLEANUP
 AT_SETUP([ofproto - set mtu])
 OVS_VSWITCHD_START
 
+# Check that initial MTU is 1500 for 'br0'.
+AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], [dnl
+1500
+])
+
 add_of_ports br0 1
 
 # Check that initial MTU is 1500 for 'br0' and 'p1'.
@@ -8892,5 +8897,9 @@ AT_CHECK([ovs-vsctl add-port br0 p2 -- set int p2 
type=dummy mtu_request=1600])
 AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1600])
 AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600])
 
+# New internal port.  The MTU should be updated even for a newly added port.
+AT_CHECK([ovs-vsctl add-port br0 int1 -- set int int1 type=internal])
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface int1 mtu=1600])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP
-- 
2.9.3

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 7/7] ovsdb: Replication usability improvements

[Andy Zhou]

On Tue, Aug 30, 2016 at 4:17 AM, Numan Siddique  wrote:

>
>
> On Tue, Aug 30, 2016 at 1:11 AM, Andy Zhou  wrote:
>
>>
>>
>> On Mon, Aug 29, 2016 at 3:14 AM, Numan Siddique 
>> wrote:
>>
>>>
>>>
>>> On Sat, Aug 27, 2016 at 4:45 AM, Andy Zhou  wrote:
>>>
 Added the '--no-sync' option base on feedbacks of current
 implementation.

 Added appctl command "ovsdb-server/sync-status" based on feedbacks
 of current implementation.

 Added a test to simulate the integration of HA manager with OVSDB
 server using replication.

 Other documentation and API improvements.

 Signed-off-by: Andy Zhou 
 --

 I hope to get some review comments on the command line and appctl
 interfaces for replication. Since 2.6 is the first release of those
 interfaces, it is easier to making changes, compare to future
 releases.

 
 v1->v2: Fix creashes reported at:
 http://openvswitch.org/pipermail/dev/2016-August/078591.html
 ---

>>>
>>> ​I haven't tested these patches yet. This patch seems to have a white
>>> space warning when applied.
>>>
>> Thanks for the reported. I will fold the fix in the next version when
>> posting.
>>
>> In case it helps, you can also access the patches from my private repo at:
>>   https://github.com/azhou-nicira/ovs-review/tree/ovsdb-replic
>> ation-sm-v2
>>
>>
> ​
> Hi Andy,
> ​
> I am seeing the below crash when
>
> ​  - The ovsdb-server changes from
> ​master to standby and the active-ovsdb-server it is about to connect to
> is killed just before that or it is not reachable.
>
> ​  -
> ​The pacemaker OCF script calls the sync-status cmd soon after that.
>
>
> ​Please let me know if you need more information.​
>
>
> ​Core was generated by `ovsdb-server -vdbg 
> --log-file=/opt/stack/logs/ovsdb-server-sb.log
> --remote=puni'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0x0041241d in replication_status () at ovsdb/replication.c:875
> 875SHASH_FOR_EACH (node, replication_dbs) {
> Missing separate debuginfos, use: dnf debuginfo-install
> glibc-2.23.1-10.fc24.x86_64 openssl-libs-1.0.2h-3.fc24.x86_64
> (gdb) bt
> #0  0x0041241d in replication_status () at ovsdb/replication.c:875
> #1  0x00406eda in ovsdb_server_get_sync_status (conn=0x1421fd0,
> argc=, argv=, config_=)
> at ovsdb/ovsdb-server.c:1480
> #2  0x004324ee in process_command (request=0x1421f30,
> conn=0x1421fd0) at lib/unixctl.c:313
> #3  run_connection (conn=0x1421fd0) at lib/unixctl.c:347
> #4  unixctl_server_run (server=server@entry=0x141e140) at
> lib/unixctl.c:400
> #5  0x00405bdc in main_loop (is_backup=0x7fff08062256,
> exiting=0x7fff08062257, run_process=0x0, remotes=0x7fff080622a0,
> unixctl=0x141e140,
> all_dbs=0x7fff080622e0, jsonrpc=0x13f6f00) at ovsdb/ovsdb-server.c:182
> #6  main (argc=, argv=) at
> ovsdb/ovsdb-server.c:430​
>
> Numan, thanks for the report. I think I spotted the bug:

Currently, when replication state machine is reset,  the state update takes
place after a round of main loop run. this time lag
could lead to the back trace in case the unixctl commands was issued during
this time lag.  I have a fix that add another
state to represent the reset condition.  The fix is at:

https://github.com/azhou-nicira/ovs-review/tree/ovsdb-replication-sm-v3

Would you please let me know if this version works any better?. Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 14/15] ofproto: Refactor packet_out handling.

[Jarno Rajahalme]

> On Aug 30, 2016, at 8:40 AM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:40PM -0700, Jarno Rajahalme wrote:
>> Refactor handle_packet_out() to prepare for bundle support for packet
>> outs in a later patch.
>> 
>> Two new callbacks are introduced in ofproto-provider class:
>> ->packet_xlate() and ->packet_execute().  ->packet_xlate() translates
>> the packet using the flow and actions provided by the caller, but
>> defers all OpenFlow-visible side-effects (stats, learn actions, actual
>> packet output, etc.) to be explicitly executed with the
>> ->packet_execute() call.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> This adds a couple of comments of the form
> 
>/* OUTPUT to a real port can not refer to versionable lookups (flow tables,
> * groups), so we do not need to worry about the version to use here. */
> 
> but I'm not sure that I believe it.  In particular,
> compose_table_xlate(), which has this comment, outputs to OFPP_TABLE,
> which isn't a real port at all but instead does a full flow table
> traversal.
> 

Thanks for spotting this!

> This line in packet_execute() needs an extra space:
>execute.actions =opo->odp_actions.data;
> 

OK.

> I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
> seems like a layer violation.

I think this was due to the plate cache only. The learn action entries in late 
cache use the struct ofproto_flow_mod, which the ofproto.c can operate on. 
Apart from bundle revert these could be handled from ofproto-dpif.c with calls 
to ofproto.c functions from there. To make the revert work I’d need to add a 
new ofproto-provider class member function and have it iterate through the late 
cache and then call back to ofproto.c to revert learned flows.

Also, making the late cache a module of it’s own would remove the inclusion of 
ofproto-dpif-xlate.h. Would this alleviate your concern with layer violation, 
or should I strive to keep the xlate cache opaque to ofproto.c?

  Jarno

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 12/15] ofproto-dpif-xlate: Allow translating without side-effects.

[Jarno Rajahalme]

> On Aug 29, 2016, at 5:06 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:38PM -0700, Jarno Rajahalme wrote:
>> Extend 'may_learn' attribute to also control the treatment of
>> FIN_TIMEOUT action and asynchronous messages (packet ins,
>> continuations), so that when 'may_learn' is 'false' and
>> 'resubmit_stats' is 'NULL', no OpenFlow-visible side effects are
>> generated by the translation.
>> 
>> Correspondingly, add support for one-time asynchronous messages to
>> xlate cache, so that all side-effects of the translation may be
>> executed at a later stage.  This will be useful for bundle commits.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Would it make any sense to drop support for immediate effects, so that
> callers that want immediate effects provide an xcache entry and then
> execute it and discard it?
> 

Joe had earlier discovered that creating the xlate cache at upcall time is too 
expensive and negatively affects the megaflow set-up rate. This is due to who 
main factors: many memory allocations and taking references to all the referred 
objects. So currently the translation is done again at first revalidation to 
create the xlate cache, which seems more work overall, but does not affect the 
megaflow set-up rate. So, at least for now we need the capability for immediate 
effects. We may revisit later, though.

  Jarno

> Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 09/15] ofproto-dpif-xlate: Expose xlate cache.

[Jarno Rajahalme]

> On Aug 29, 2016, at 2:59 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:35PM -0700, Jarno Rajahalme wrote:
>> Later patches will need to create xlate cache entries from different
>> modules.  This patch refactors the xlate cache code in preparation
>> without any functional changes, so that the changes are clearly
>> visible in the following patches.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> As a second thought, the xlate cache seems to have little or nothing in
> common with the rest of the xlate code.  Would it be valuable to move it
> to its own files?

I’ll give it a go for the v3.

  Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 07/15] object-collection: Remove access to stub.

[Jarno Rajahalme]

Thanks for the review, pushed to master,

  Jarno

> On Aug 29, 2016, at 2:57 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:33PM -0700, Jarno Rajahalme wrote:
>> Better not use access to the *_collection_stub(), as it is an internal
>> implementation detail.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 06/15] ofproto: Remove stale comment.

[Jarno Rajahalme]

Pushed to master,

  Jarno

> On Aug 29, 2016, at 2:41 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:32PM -0700, Jarno Rajahalme wrote:
>> The previous line tells that this comment is now stale. Remove it.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 04/15] ofproto-dpif: Remove prototype for an undefined function.

[Jarno Rajahalme]

Pushed to master,

  Jarno

> On Aug 29, 2016, at 2:36 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:30PM -0700, Jarno Rajahalme wrote:
>> We do not have a 'ofproto_dpif_refresh_rule()' function.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] upcall: reduce log level for "no reference to recirc flow" message

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

Pushed to master and branch-2.6.

  Jarno

> On Aug 29, 2016, at 2:06 PM, Lance Richardson  wrote:
> 
> Reduce log level from "warn" to "debug" for "upcall: no reference to
> recirc flow" log message.
> 
> Suggested-by: Jarno Rajahalme 
> Signed-off-by: Lance Richardson 
> ---
> v2: Instead of ignoring log message in system-traffic.at, reduce level
>so it no longer appears in the log. 
> 
> ofproto/ofproto-dpif-upcall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 042a50a..e447308 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1147,7 +1147,7 @@ should_install_flow(struct udpif *udpif, struct upcall 
> *upcall)
> if (upcall->type != DPIF_UC_MISS) {
> return false;
> } else if (upcall->recirc && !upcall->have_recirc_ref) {
> -VLOG_WARN_RL(, "upcall: no reference for recirc flow");
> +VLOG_DBG_RL(, "upcall: no reference for recirc flow");
> return false;
> }
> 
> -- 
> 2.5.5
> 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 02/15] lib: Retire packet buffering feature.

[Jarno Rajahalme]

> On Aug 29, 2016, at 2:07 PM, Ben Pfaff  wrote:
> 
> On Mon, Aug 22, 2016 at 04:31:28PM -0700, Jarno Rajahalme wrote:
>> OVS implementation of buffering packets that are sent to the
>> controller is not compliant with the OpenFlow specifications after
>> OpenFlow 1.0, which is possibly true since OpenFlow 1.0 is not really
>> specifying the packet buffering behavior.
>> 
>> OVS implementation executes the buffered packet against the actions of
>> the modified or added rule, whereas OpenFlow (since 1.1) specifies
>> that the packet should be matched against the flow table 0 and
>> processed accordingly.
>> 
>> Rather than fix this behavior, and potentially break OVS users, we
>> propose to remove the feature altogether. After all, such packet
>> buffering is an optional OpenFlow feature, and as such any possible
>> users should continue to work without this feature.
>> 
>> This patch also makes OVS check the received 'buffer_id' values more
>> rigorously, and fixes some internal users accordingly.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> I believe that this came out of a bug report.  Will you credit the bug
> reporter?
> 

I noticed this when checking back to OpenFlow specs when developing the split 
late/execute for buffered packets and packet outs, so there is no bug report. 
I'll state “Found by inspection” in the commit message.

> I think that we will probably start getting a question about "what
> happened to the buffers?" so it might be worthwhile to add a question
> and answer to the FAQ about that.
> 

Done.

> The change to lib/learn.c seems unnecessary.
> 

Right, it was a combination of two changes made at different times cancelling 
out each other.

> I'd be inclined to also remove the buffer_id parameters from the
> functions that ofputil_encode_packet_in_private() calls.  They're always
> going to be UINT32_MAX now.
> 

I should leave the NXPINT_BUFFER_ID definition be even though we now never 
encode it, however?

> In ofputil_encode_packet_in_private(), here:
>if (check_buffer_id && fm->buffer_id != UINT32_MAX) {
>error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
>}
> I would add "!error &&" to the condition, because I think that this
> error is less important than the others.
> 

This was in ofproto.c:ofproto_flow_mod_init(), but I changed this like you 
suggested.

> The changes to ofctrl.c should not be necessary, because the
> encode_flow_mod() function that all of the cases use already sets
> buffer_id to UINT32_MAX.
> 

Right, reverted.

> Acked-by: Ben Pfaff >

Thanks for the thorough review! Pushed to master with these revisions.

  Jarno

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ofp-actions: Waste less memory in set field and load actions.

[Ben Pfaff]

On Tue, Aug 23, 2016 at 05:51:37PM -0700, Jarno Rajahalme wrote:
> Change the value and mask to be added to the end of the set field
> action without any extra bytes, exept for the usual ofp-actions
> padding to 8 bytes.  Together with some structure member packing this
> saves on average about to 256 bytes for each set field and load action
> (set field internal representation is also used for load actions).
> 
> On a specific production data set each flow entry uses on average
> about 4.1 load and set field actions.  This means that with this patch
> an average of more than 1kb can be saved for each flow with such a
> flow table.
> 
> Signed-off-by: Jarno Rajahalme 

Thanks for working on this!

Building on 32-bit I get assertion failures without:

diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 9eaa78c..198eedd 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -479,9 +479,11 @@ struct ofpact_stack {
  *
  * Used for NXAST_REG_LOAD and OFPAT12_SET_FIELD. */
 struct ofpact_set_field {
-struct ofpact ofpact;
-bool flow_has_vlan;   /* VLAN present at action validation time. */
-const struct mf_field *field;
+OFPACT_PADDED_MEMBERS(
+struct ofpact ofpact;
+bool flow_has_vlan;   /* VLAN present at action validation time. */
+const struct mf_field *field;
+);
 union mf_value value[];  /* Significant value bytes followed by
   * significant mask bytes. */
 };

I'd suggest ALIGNED_CAST in ofpact_set_field_mask() to make the
cast-to-void-then-to-real-type trick more greppable.

There's a number of instances of code similar to:

+struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts, mf);
+memcpy(sf->value, _value, mf->n_bytes);
+memcpy(ofpact_set_field_mask(sf), _mask, mf->n_bytes);

so that it might be worthwhile to make a function that combines the
three steps (or just the memcpy()s?).

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module back to full processing

[Jesse Gross]

On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats  wrote:
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index d99ba05..87f 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> +SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) {
> +for (int i = 0; i < chassis_rec->n_encaps; i++) {
> +encap_rec = chassis_rec->encaps[i];
> +
> +struct encap_hash_node *encap_hash_node;
> +encap_hash_node = lookup_encap_uuid(_rec->header_.uuid);
> +if (encap_hash_node) {
> +/* A change might have invalidated our mapping. Process the
> + * new version and then iterate over everything to see if it
> + * is OK. */
> +delete_encap_uuid(encap_hash_node);
> +poll_immediate_wake();
>  }

Doesn't this result in essentially infinite wakeups? It used to be
that this loop would run only when a chassis was add/removed/changed
and so the if (encap_hash_node) condition would only trigger when an
existing chassis is modified. However, after this change every wakeup
will loop through all chassises and any existing ones will trigger
another wakeup and loop, etc.

In general, I don't think it makes sense to remove incremental
processing without removing persistent state. The result ends up being
not very logical and actually more complicated.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ofp-actions: Waste less memory in learn actions.

[Ben Pfaff]

On Tue, Aug 23, 2016 at 05:51:36PM -0700, Jarno Rajahalme wrote:
> Make the immediate data member 'src_imm' of a learn spec allocated at
> the end of the action for just the right size.  This, together with
> some structure packing saves on average of ~128 bytes for each learn
> spec in each learn action.  Typical learn actions have about 4 specs
> each, so this amounts to saving about 0.5kb for each learn action.
> 
> Signed-off-by: Jarno Rajahalme 

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 15/15] ofproto: Support packet_outs in bundles.

[Ben Pfaff]

On Mon, Aug 22, 2016 at 04:31:41PM -0700, Jarno Rajahalme wrote:
> Add support for OFPT_PACKET_OUT messages in bundles.
> 
> While ovs-ofctl already has a packet-out command, we did not have a
> string parser for it, as the parsing was done directly from command
> line arguments.
> 
> This patch adds the string parser for packet-out messages, and adds a
> new ofctl/packet-out ovs-appctl command that can be used when
> ovs-ofctl is used as a flow monitor.
> 
> The new packet-out parser is further supported with the ovs-ofctl
> bundle command, which allows bundles to mix flow mods, group mods and
> packet-out messages.  Also the packet-outs in bundles are only
> executed if the whole bundle is successful.  A failing packet-out
> translation may also make the whole bundle to fail.
> 
> Signed-off-by: Jarno Rajahalme 

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 14/15] ofproto: Refactor packet_out handling.

[Ben Pfaff]

On Mon, Aug 22, 2016 at 04:31:40PM -0700, Jarno Rajahalme wrote:
> Refactor handle_packet_out() to prepare for bundle support for packet
> outs in a later patch.
> 
> Two new callbacks are introduced in ofproto-provider class:
> ->packet_xlate() and ->packet_execute().  ->packet_xlate() translates
> the packet using the flow and actions provided by the caller, but
> defers all OpenFlow-visible side-effects (stats, learn actions, actual
> packet output, etc.) to be explicitly executed with the
> ->packet_execute() call.
> 
> Signed-off-by: Jarno Rajahalme 

This adds a couple of comments of the form

/* OUTPUT to a real port can not refer to versionable lookups (flow tables,
 * groups), so we do not need to worry about the version to use here. */

but I'm not sure that I believe it.  In particular,
compose_table_xlate(), which has this comment, outputs to OFPP_TABLE,
which isn't a real port at all but instead does a full flow table
traversal.

This line in packet_execute() needs an extra space:
execute.actions =opo->odp_actions.data;

I'm surprised to see ofproto.c including ofproto-dpif-xlate.h.  That
seems like a layer violation.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 08/12] python tests: Ignore stderr output

[Guru Shetty]

On 29 August 2016 at 11:30, Paul Boca  wrote:

>
>
>
>
> *From:* Guru Shetty [mailto:g...@ovn.org]
> *Sent:* Monday, August 29, 2016 9:20 PM
> *To:* Paul Boca
> *Cc:* dev@openvswitch.org
> *Subject:* Re: [ovs-dev] [PATCH 08/12] python tests: Ignore stderr output
>
>
>
>
>
> So you are saying that running $srcdir/test-unixctl.py on Windows
> produces output in stderr. Why? Is there a bug with Windows port?
>
> *[Paul Boca] *This is the way logging library works on both Windows and
> Linux (https://docs.python.org/2/howto/logging.html -
>
> “If you call the functions *debug()*
> , *info()*
> ,*warning()*
> ,
> *error()* 
>  and *critical()*
> , they
> will check to see if no
>
> destination is set; and if one is not set, they will set a destination of
> the console (sys.stderr)”)
>
> but the difference is that on Windows the stderr is appended in a file
> (the same file) and on Linux a stderr instance
>
> is used for every command.
>
>
>
> I understand what you are trying to say. My question is a little different.
>
> If I add the following incremental, vlog tests in Linux fail and this is
> because something is printed in stderr:
>
>
>
> diff --git a/tests/test-unixctl.py b/tests/test-unixctl.py
>
> index 5de51d3..f85de39 100644
>
> --- a/tests/test-unixctl.py
>
> +++ b/tests/test-unixctl.py
>
> @@ -76,6 +76,8 @@ def main():
>
>  ovs.unixctl.command_register("block", "", 0, 0, unixctl_block, None)
>
>  ovs.daemon.daemonize_complete()
>
>
>
> +vlog.err("oink")
>
> +
>
>  vlog.info("Entering run loop.")
>
>  poller = ovs.poller.Poller()
>
>  while not exiting:
>
>
>
> So the question is - what is it that gets printed in Windows in stderr?
> What specific test fails?
>
>
>
> *[Paul Boca] On Windows I saw that “Entering run loop." gets printed in
> stderr.*
>
> *If I apply your incremental, both “oink” and “Entering run loop” are
> printed in stderr.*
>
> *Maybe there is a difference how logging library works on Windows, I don’t
> have too much experience with it.*
>

Would you mind investigating a bit here? We don't want to miss a porting
bug.


> *I will take a look, maybe I missed something, but the vlog.py is the same
> for both Windows and Linux.*
>
>
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 02/10] python tests: Skip python tests that kill the python daemon

[Guru Shetty]

On 30 August 2016 at 05:00, Paul Boca  wrote:

> If the python script is killed with `kill` command, the atexit
> handler doesn't gets executed on Windows.
> The kill of the process is done using NtTerminateProcess which
> doesn't sends a signal to the process itself, if just terminates the
> process
> from kernel mode.
>
> Signed-off-by: Paul-Daniel Boca 
>

Instead of skipping the test, why not just skip the line that tests for pid
after the process is killed. I imagine that the test is still useful in
other respects, like whether detach works fine?


> ---
> V2: Initial commit
> ---
>  tests/daemon-py.at | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tests/daemon-py.at b/tests/daemon-py.at
> index 96dea07..11833c8 100644
> --- a/tests/daemon-py.at
> +++ b/tests/daemon-py.at
> @@ -126,6 +126,8 @@ DAEMON_MONITOR_RESTART_PYN([Python3],
> [$HAVE_PYTHON3], [$PYTHON3])
>  m4_define([DAEMON_DETACH_PYN],
>[AT_SETUP([daemon --detach - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, the pid file not removed if the daemon
> is killed
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([pid])
> # Start the daemon and make sure that the pidfile exists immediately.
> # We don't wait for the pidfile to get created because the daemon is
> --
> 2.7.2.windows.1
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 01/10] python tests: Skip python tests specific to Linux

[Guru Shetty]

On 30 August 2016 at 05:00, Paul Boca  wrote:

> There is a difference between POSIX pid and Windows pid, not all the time
> are equal.
> On Windows when a python script is started, a sh command is triggered as
> the parent
> for script. So when we try to get the daemon pid with 'echo $!', this will
> get the pid of sh
> not of python.exe as expected.
> Some tests use undefined switches, on Windows, for `kill` command.
>
> Signed-off-by: Paul-Daniel Boca 
>
Applied, thanks.


> ---
> V2: Splitted patch in 2 patches.
> ---
>  tests/daemon-py.at | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/tests/daemon-py.at b/tests/daemon-py.at
> index e59c11d..96dea07 100644
> --- a/tests/daemon-py.at
> +++ b/tests/daemon-py.at
> @@ -3,6 +3,8 @@ AT_BANNER([daemon unit tests - Python])
>  m4_define([DAEMON_PYN],
>[AT_SETUP([daemon - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, echo $! gives shell pid instead of
> parent process
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_KEYWORDS([python daemon])
> AT_CAPTURE_FILE([pid])
> AT_CAPTURE_FILE([expected])
> @@ -26,6 +28,8 @@ DAEMON_PYN([Python3], [$HAVE_PYTHON3], [$PYTHON3])
>  m4_define([DAEMON_MONITOR_PYN],
>[AT_SETUP([daemon --monitor - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, echo $! gives shell pid instead of
> parent process
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([pid])
> AT_CAPTURE_FILE([parent])
> AT_CAPTURE_FILE([parentpid])
> @@ -73,6 +77,8 @@ DAEMON_MONITOR_PYN([Python3], [$HAVE_PYTHON3],
> [$PYTHON3])
>  m4_define([DAEMON_MONITOR_RESTART_PYN],
>[AT_SETUP([daemon --monitor restart exit code - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, echo $! gives shell pid instead of
> parent process
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([pid])
> AT_CAPTURE_FILE([parent])
> AT_CAPTURE_FILE([parentpid])
> @@ -142,6 +148,8 @@ m4_define([CHECK],
>  m4_define([DAEMON_DETACH_MONITOR_PYN],
>[AT_SETUP([daemon --detach --monitor - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, uses Linux specific kill signal
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([daemon])
> AT_CAPTURE_FILE([olddaemon])
> AT_CAPTURE_FILE([newdaemon])
> @@ -219,6 +227,8 @@ DAEMON_DETACH_MONITOR_ERRORS_PYN([Python3],
> [$HAVE_PYTHON3], [$PYTHON3])
>  m4_define([DAEMON_DETACH_CLOSES_FDS_PYN],
>[AT_SETUP([daemon --detach closes standard fds - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, uses Linux specific kill signal
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([pid])
> AT_CAPTURE_FILE([status])
> AT_CAPTURE_FILE([stderr])
> @@ -243,6 +253,8 @@ DAEMON_DETACH_CLOSES_FDS_PYN([Python3],
> [$HAVE_PYTHON3], [$PYTHON3])
>  m4_define([DAEMON_DETACH_MONITOR_CLOSES_FDS_PYN],
>[AT_SETUP([daemon --detach --monitor closes standard fds - $1])
> AT_SKIP_IF([test $2 = no])
> +   # Skip this test for Windows, uses Linux specific kill signal
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> AT_CAPTURE_FILE([pid])
> AT_CAPTURE_FILE([status])
> AT_CAPTURE_FILE([stderr])
> --
> 2.7.2.windows.1
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 09/12] python tests: Skip ovn-controller-vtep tests on Windows

[Guru Shetty]

On 30 August 2016 at 02:17, Paul Boca  wrote:

> Hi Guru,
>
>
>
> The problem with ovn tests is that the ‘—detach’ is used for clients,
> which on Windows is
>
> ignored.
>
> For example here: https://github.com/openvswitch/ovs/blob/master/
> ovn/utilities/ovn-trace.c#L85
>
> on Windows will always call https://github.com/
> openvswitch/ovs/blob/master/ovn/utilities/ovn-trace.c#L92-L93
>
> instead of https://github.com/openvswitch/ovs/blob/master/
> ovn/utilities/ovn-trace.c#L87-L88. So this will raise
>
> some errors in ovn tests on Windows.
>
> I don’t know if ignoring the ‘—detach’ switch is intended or is a porting
> error.
>

It looks like a porting error.


>
>
> Paul
>
>
>
> *From:* Guru Shetty [mailto:g...@ovn.org]
> *Sent:* Friday, August 26, 2016 9:07 PM
> *To:* Paul Boca
> *Cc:* dev@openvswitch.org
> *Subject:* Re: [ovs-dev] [PATCH 09/12] python tests: Skip
> ovn-controller-vtep tests on Windows
>
>
>
>
>
>
>
> On 26 August 2016 at 07:40, Paul Boca 
> wrote:
>
> The tests are not intended to run on Windows.
>
>
>
> Can you explain why? What part of the test gives a problem in Windows? Is
> it that the ovs-vtep.py has not been ported to Windows?
>
>
>
> Until now they were skipped based on $HAVE_PYTHON only.
>
> Signed-off-by: Paul-Daniel Boca 
> ---
>  tests/ovn-controller-vtep.at | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> index 654c212..3f024cb 100644
> --- a/tests/ovn-controller-vtep.at
> +++ b/tests/ovn-controller-vtep.at
> @@ -15,7 +15,7 @@ m4_define([OVN_CONTROLLER_VTEP_START],
>[
> AT_KEYWORDS([ovn])
> # this will cause skip when 'make check' using Windows setup.
> -   AT_SKIP_IF([test $HAVE_PYTHON = no])
> +   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
>
> dnl Create databases (ovn-nb, ovn-sb, vtep).
> AT_CHECK([ovsdb-tool create vswitchd.db $abs_top_srcdir/vswitchd/
> vswitch.ovsschema])
> --
> 2.7.2.windows.1
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] Cant `make install` ovs

[Kavanagh, Mark B]

>
>Hi,
>If you could have a look at this
> problem
>it would be very helpful.
>
>or here is the detail.
>
>I am following this
> guide to
>install OVS with DPDK.
>I manage till *2.3 Install OVS*
>
>> cd $OVS_DIR
>> ./boot.sh
>> ./configure --with-dpdk=$DPDK_BUILD
>
>all runs fine without any error but when I do
>
>> make install
>
>It fails.
>
>Here's the output
>
>root@ubuntu:/usr/src/openvswitch-2.5.0# make install
>make install-recursive
>make[1]: Entering directory '/usr/src/openvswitch-2.5.0'
>Making install in datapath
>make[2]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
>make[3]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
>make[4]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
>make[4]: Nothing to be done for 'install-exec-am'.
>make[4]: Nothing to be done for 'install-data-am'.
>make[4]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
>make[3]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
>make[2]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
>make[2]: Entering directory '/usr/src/openvswitch-2.5.0'
>depbase=`echo lib/netdev-dpdk.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>/bin/bash ./libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I
>./include -I ./include -I ./lib -I ./lib -I/usr/include -Wstrict-prototypes
>-Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat-security
>-Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>-Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing
>-mssse3 -I/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include
>-D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP -MF
>$depbase.Tpo -c -o lib/netdev-dpdk.lo lib/netdev-dpdk.c &&\
>mv -f $depbase.Tpo $depbase.Plo
>libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I
>./lib -I ./lib -I/usr/include -Wstrict-prototypes -Wall -Wextra
>-Wno-sign-compare -Wpointer-arith -Wformat-security -Wswitch-enum
>-Wunused-parameter -Wbad-function-cast -Wcast-align -Wmissing-prototypes
>-Wmissing-field-initializers -fno-strict-aliasing -mssse3
>-I/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include
>-D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP -MF
>lib/.deps/netdev-dpdk.Tpo -c lib/netdev-dpdk.c -o lib/netdev-dpdk.o
>lib/netdev-dpdk.c: In function ‘__rte_pktmbuf_init’:
>lib/netdev-dpdk.c:293:5: warning: implicit declaration of function
>‘RTE_MBUF_ASSERT’ [-Wimplicit-function-declaration]
>RTE_MBUF_ASSERT(mp->elt_size >= sizeof(struct dp_packet));
>^
>lib/netdev-dpdk.c: In function ‘netdev_dpdk_vhost_user_construct’:
>lib/netdev-dpdk.c:708:11: error: too few arguments to function
>‘rte_vhost_driver_register’
>err = rte_vhost_driver_register(netdev->vhost_id);
>^
>In file included from lib/netdev-dpdk.c:56:0:
>/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:95:5:
>note: declared here
>int rte_vhost_driver_register(const char *path, uint64_t flags);
>^
>lib/netdev-dpdk.c: In function ‘is_vhost_running’:
>lib/netdev-dpdk.c:995:32: error: dereferencing pointer to incomplete type
>‘struct virtio_net’
>return (dev != NULL && (dev->flags & VIRTIO_DEV_RUNNING));
>^
>lib/netdev-dpdk.c:995:42: error: ‘VIRTIO_DEV_RUNNING’ undeclared (first use
>in this function)
>return (dev != NULL && (dev->flags & VIRTIO_DEV_RUNNING));
>^
>lib/netdev-dpdk.c:995:42: note: each undeclared identifier is reported only
>once for each function it appears in
>lib/netdev-dpdk.c: In function ‘netdev_dpdk_vhost_rxq_recv’:
>lib/netdev-dpdk.c:1049:37: warning: passing argument 1 of
>‘rte_vhost_dequeue_burst’ makes integer from pointer without a cast
>[-Wint-conversion]
>nb_rx = rte_vhost_dequeue_burst(virtio_dev, qid * VIRTIO_QNUM + VIRTIO_TXQ,
>^
>In file included from lib/netdev-dpdk.c:56:0:
>/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:194:10:
>note: expected ‘int’ but argument is of type ‘struct virtio_net *’
>uint16_t rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>^
>lib/netdev-dpdk.c: In function ‘__netdev_dpdk_vhost_send’:
>lib/netdev-dpdk.c:1138:43: warning: passing argument 1 of
>‘rte_vhost_enqueue_burst’ makes integer from pointer without a cast
>[-Wint-conversion]
>tx_pkts = rte_vhost_enqueue_burst(virtio_dev, vhost_qid,
>^
>In file included from lib/netdev-dpdk.c:56:0:
>/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:174:10:
>note: expected ‘int’ but argument is of type ‘struct virtio_net *’
>uint16_t rte_vhost_enqueue_burst(int vid, uint16_t queue_id,
>^
>lib/netdev-dpdk.c:1157:21: warning: implicit declaration of function
>‘rte_vring_available_entries’ [-Wimplicit-function-declaration]
>while (!rte_vring_available_entries(virtio_dev, vhost_qid)) {
>^
>lib/netdev-dpdk.c: In function ‘netdev_dpdk_get_stats’:
>lib/netdev-dpdk.c:1526:33: error: ‘struct rte_eth_stats’ has no member
>named ‘imcasts’

Re: [ovs-dev] Cant `make install` ovs

[Thadeu Lima de Souza Cascardo]

On Tue, Aug 30, 2016 at 06:16:43PM +0530, Hussain Hakimuddin Nagri wrote:
> Hi,
> If you could have a look at this
>  problem
> it would be very helpful.
[...]
> I don't understand what am I missing.
> DPDK is *dpdk-16.07* OpenVSwitch is *openvswitch-2.5.0* Ubuntu is *Ubuntu
> 16.04.1 LTS*
> 

OVS 2.5 will work with DPDK 2.2. That is one of the first things you will see at
INSTALL.DPDK.md, line 19.

> Any help will be appreciated.
> Thank you.
> --
> Regards
> Hussain Nagri
> +919739094973
> 
> -- 
> 
> 
> *Disclaimer:*
> 
> *This communication including any accompanying attachments is intended only 
> for the use of the intended addressee(s)/recipient and contains information 
> that is legally PRIVILEGED, CONFIDENTIAL and PROPRIETARY to BankBazaar.com. 
> If you are not the intended recipient, any unauthorized review, 
> dissemination, distribution, disclosure or copying of this communication is 
> strictly prohibited and is unlawful. If you have received this 
> communication including any accompanying attachments in error, you are 
> requested to notify BankBazaar.com immediately by reply e-mail and 
> thereafter you are required to promptly delete it permanently from your 
> system/storage device. The contents of this communication do not constitute 
> financial or other professional advice nor does it form principal-agent 
> relationship, and is not a substitute for the need to seek advice from 
> BankBazaar.com on specific financial matters. The recipient is advised to 
> scan this email and any attachments with it for viruses. BankBazaar.com 
> does not accept any liability whatsoever for any damage or loss that may be*
>  *caused through this communication.*
> ___


These kind of disclaimers are not fit for public mailing lists. Please, drop it
for any next communications on ovs-dev.

Cascardo.

> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Cant `make install` ovs

[Hussain Hakimuddin Nagri]

Hi,
If you could have a look at this
 problem
it would be very helpful.

or here is the detail.

I am following this
 guide to
install OVS with DPDK.
I manage till *2.3 Install OVS*

> cd $OVS_DIR
> ./boot.sh
> ./configure --with-dpdk=$DPDK_BUILD

all runs fine without any error but when I do

> make install

It fails.

Here's the output

root@ubuntu:/usr/src/openvswitch-2.5.0# make install
make install-recursive
make[1]: Entering directory '/usr/src/openvswitch-2.5.0'
Making install in datapath
make[2]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
make[3]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
make[4]: Entering directory '/usr/src/openvswitch-2.5.0/datapath'
make[4]: Nothing to be done for 'install-exec-am'.
make[4]: Nothing to be done for 'install-data-am'.
make[4]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
make[3]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
make[2]: Leaving directory '/usr/src/openvswitch-2.5.0/datapath'
make[2]: Entering directory '/usr/src/openvswitch-2.5.0'
depbase=`echo lib/netdev-dpdk.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/bash ./libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I
./include -I ./include -I ./lib -I ./lib -I/usr/include -Wstrict-prototypes
-Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat-security
-Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
-Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing
-mssse3 -I/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include
-D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP -MF
$depbase.Tpo -c -o lib/netdev-dpdk.lo lib/netdev-dpdk.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I
./lib -I ./lib -I/usr/include -Wstrict-prototypes -Wall -Wextra
-Wno-sign-compare -Wpointer-arith -Wformat-security -Wswitch-enum
-Wunused-parameter -Wbad-function-cast -Wcast-align -Wmissing-prototypes
-Wmissing-field-initializers -fno-strict-aliasing -mssse3
-I/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include
-D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP -MF
lib/.deps/netdev-dpdk.Tpo -c lib/netdev-dpdk.c -o lib/netdev-dpdk.o
lib/netdev-dpdk.c: In function ‘__rte_pktmbuf_init’:
lib/netdev-dpdk.c:293:5: warning: implicit declaration of function
‘RTE_MBUF_ASSERT’ [-Wimplicit-function-declaration]
RTE_MBUF_ASSERT(mp->elt_size >= sizeof(struct dp_packet));
^
lib/netdev-dpdk.c: In function ‘netdev_dpdk_vhost_user_construct’:
lib/netdev-dpdk.c:708:11: error: too few arguments to function
‘rte_vhost_driver_register’
err = rte_vhost_driver_register(netdev->vhost_id);
^
In file included from lib/netdev-dpdk.c:56:0:
/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:95:5:
note: declared here
int rte_vhost_driver_register(const char *path, uint64_t flags);
^
lib/netdev-dpdk.c: In function ‘is_vhost_running’:
lib/netdev-dpdk.c:995:32: error: dereferencing pointer to incomplete type
‘struct virtio_net’
return (dev != NULL && (dev->flags & VIRTIO_DEV_RUNNING));
^
lib/netdev-dpdk.c:995:42: error: ‘VIRTIO_DEV_RUNNING’ undeclared (first use
in this function)
return (dev != NULL && (dev->flags & VIRTIO_DEV_RUNNING));
^
lib/netdev-dpdk.c:995:42: note: each undeclared identifier is reported only
once for each function it appears in
lib/netdev-dpdk.c: In function ‘netdev_dpdk_vhost_rxq_recv’:
lib/netdev-dpdk.c:1049:37: warning: passing argument 1 of
‘rte_vhost_dequeue_burst’ makes integer from pointer without a cast
[-Wint-conversion]
nb_rx = rte_vhost_dequeue_burst(virtio_dev, qid * VIRTIO_QNUM + VIRTIO_TXQ,
^
In file included from lib/netdev-dpdk.c:56:0:
/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:194:10:
note: expected ‘int’ but argument is of type ‘struct virtio_net *’
uint16_t rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
^
lib/netdev-dpdk.c: In function ‘__netdev_dpdk_vhost_send’:
lib/netdev-dpdk.c:1138:43: warning: passing argument 1 of
‘rte_vhost_enqueue_burst’ makes integer from pointer without a cast
[-Wint-conversion]
tx_pkts = rte_vhost_enqueue_burst(virtio_dev, vhost_qid,
^
In file included from lib/netdev-dpdk.c:56:0:
/usr/src/dpdk-16.07/x86_64-native-linuxapp-gcc/include/rte_virtio_net.h:174:10:
note: expected ‘int’ but argument is of type ‘struct virtio_net *’
uint16_t rte_vhost_enqueue_burst(int vid, uint16_t queue_id,
^
lib/netdev-dpdk.c:1157:21: warning: implicit declaration of function
‘rte_vring_available_entries’ [-Wimplicit-function-declaration]
while (!rte_vring_available_entries(virtio_dev, vhost_qid)) {
^
lib/netdev-dpdk.c: In function ‘netdev_dpdk_get_stats’:
lib/netdev-dpdk.c:1526:33: error: ‘struct rte_eth_stats’ has no member
named ‘imcasts’
stats->multicast = rte_stats.imcasts;
^
lib/netdev-dpdk.c: In function ‘netdev_dpdk_get_features’:

Re: [ovs-dev] please help review patch "allow DHCPv6 respond multiple IA Address Options"

[Numan Siddique]

On Tue, Aug 30, 2016 at 9:40 AM, Ben Pfaff  wrote:

> The patch will get reviewed, there's no need to ask again.
>
> On Tue, Aug 30, 2016 at 11:09:23AM +0800, Zong Kai Li wrote:
> > Hi, Numan and Justin.
> > I wish you can help review patch "allow DHCPv6 respond multiple IA
> > Address Options".
> > http://patchwork.ozlabs.org/patch/663493/
> >
> > For current DHCPv6 implementation, it will respond one IPv6 address
> > for a lswitch port in a DHCPv6 Reply message. The "one IPv6 address",
> > I mean, one in the same address string, like "MAC IPv4-Address
> > IPv6-Address-1 IPv6-Address-2 ...".
> >
> > I don't think it's a good solution, since we can set multiple IA
> > Address Options in a DHCPv6 Reply message, and by that, we can support
> > multiple IPv6 addresses in the same address string for VIF port to
> > configure. It will give us a lflow with action=(reg0[3] =
> > put_dhcpv6_opts(ia_addr = OFFER_IP1, ia_addr = OFFER_IP2, ... ,
> > server_id = MAC, OTHER_OPTS...); next;). This can work.
> >
> > In the patch mentioned above, I kept the CIDR checking, to make sure
> > at least one IPv6 address owned by lswitch port can reach DNS server
> > list in put_dhcpv6_opts action.
> >
>

​I haven't reviewed the patch. But I have few comments.
A logical port can have multiple IPv6 addresses, out of which some can be
from a stateless subnet and some from stateful subnet. This doesn't handle
it right ? Correct me if I am wrong here.

May be CMS can store the multiple IA_ADDR options in the
DHCP_Options.options so that ovn-northd doesn't need to bother about it.

Thanks
Numan


​


> > Thanks and have a nice day! :)
> > Zongkai, LI
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 08/10] python tests: Ignore stderr output

[Paul Boca]

test-unixctl.py and test-vlog.py outputs on stderr and on Windows
stderr is not overriden by every AT_CHECK call, the logs are only
apended to the file and subsequent AT_CHECKs get errors from previous
call.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 tests/unixctl-py.at | 3 ++-
 tests/vlog.at   | 9 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index 2031897..0ed3c41 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -114,7 +114,8 @@ m4_define([UNIXCTL_SERVER_PYN],
AT_SKIP_IF([test $2 = no])
on_exit 'kill `cat test-unixctl.py.pid`'
AT_CAPTURE_FILE([`pwd`/test-unixctl.py.log])
-   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file --pidfile --detach])
+   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file --pidfile --detach],
+ [0], [], [ignore])
 
AT_CHECK([APPCTL -t test-unixctl.py help], [0], [stdout])
AT_CHECK([cat stdout], [0], [dnl
diff --git a/tests/vlog.at b/tests/vlog.at
index a689809..468e872 100644
--- a/tests/vlog.at
+++ b/tests/vlog.at
@@ -195,7 +195,7 @@ m4_define([VLOG_REOPEN_WITHOUT_FILE_PYN],
AT_SKIP_IF([test $2 = no])
on_exit 'kill `cat test-unixctl.py.pid`'
 
-   AT_CHECK([$3 $srcdir/test-unixctl.py --pidfile --detach])
+   AT_CHECK([$3 $srcdir/test-unixctl.py --pidfile --detach], [0], [], [ignore])
 
AT_CHECK([APPCTL -t test-unixctl.py vlog/reopen], [0],
  [Logging to file not configured
@@ -322,7 +322,7 @@ m4_define([VLOG_CLOSE_PYN],
 
AT_CAPTURE_FILE([log])
AT_CAPTURE_FILE([log.old])
-   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile 
--detach])
+   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile 
--detach 2>/dev/null])
 
AT_CHECK([APPCTL -t test-unixctl.py log message])
AT_CHECK([APPCTL -t test-unixctl.py log message2])
@@ -406,7 +406,8 @@ m4_define([VLOG_SET_AND_LIST_PYN],
on_exit 'kill `cat test-unixctl.py.pid`'
 
AT_CAPTURE_FILE([log])
-   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile 
--detach])
+   AT_CHECK([$3 $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile \
+ --detach 2>/dev/null])
 
AT_CHECK([APPCTL -t test-unixctl.py vlog/list], [0], [dnl
  consolesyslogfile
@@ -502,7 +503,7 @@ m4_define([VLOG_RFC5424_PYN],
 ])
 
AT_CHECK([$3 $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile \
--vFACILITY:daemon --detach])
+-vFACILITY:daemon --detach], [0], [], [ignore])
 
AT_CHECK([ovs-appctl -t test-unixctl.py vlog/set FACILITY:invalid], [0],
 [Facility invalid is invalid
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 10/10] at tests: Allow Python tests to be run on Windows

[Paul Boca]

This patch removes the code which disables Python tests to be run on
Windows.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 tests/atlocal.in | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index 55070d8..6e744db 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -109,13 +109,6 @@ else
 HAVE_IPV6=no
 fi
 
-# XXX: Disable Python related tests on Windows because Open vSwitch code
-# written in Python has not been ported to the Windows platform. We will
-# need to remove the next block after porting is complete.
-if test "$IS_WIN32" = "yes"; then
-HAVE_PYTHON="no"
-fi
-
 if test "$HAVE_PYTHON" = "yes" \
&& test "x`$PYTHON $abs_top_srcdir/tests/test-l7.py --help | grep 'ftp'`" 
!= x; then
 HAVE_PYFTPDLIB="yes"
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 09/10] python tests: Skip ovn-controller-vtep tests on Windows

[Paul Boca]

The tests are not intended to run on Windows.
Until now they were skipped based on $HAVE_PYTHON only.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 tests/ovn-controller-vtep.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
index 654c212..3f024cb 100644
--- a/tests/ovn-controller-vtep.at
+++ b/tests/ovn-controller-vtep.at
@@ -15,7 +15,7 @@ m4_define([OVN_CONTROLLER_VTEP_START],
   [
AT_KEYWORDS([ovn])
# this will cause skip when 'make check' using Windows setup.
-   AT_SKIP_IF([test $HAVE_PYTHON = no])
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
 
dnl Create databases (ovn-nb, ovn-sb, vtep).
AT_CHECK([ovsdb-tool create vswitchd.db 
$abs_top_srcdir/vswitchd/vswitch.ovsschema])
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 04/10] python tests: Ported UNIX sockets to Windows

[Paul Boca]

AF_UNIX sockets are not supported on Windows.
Instead of an AF_UNIX socket use named pipes to communicate
between components. This makes the python sockets compatible with
the named pipe used in Windows applications.
Added stream_windows.py with named pipe and localhost
tcp connections support.

Signed-off-by: Paul-Daniel Boca 
---
V2: Moved test-jsonrpc.py changes from current patch to previous one.
Fixed named pipe disconnect errors.
---
 python/automake.mk   |   1 +
 python/ovs/jsonrpc.py|   9 +-
 python/ovs/poller.py |  49 +++-
 python/ovs/socket_util.py|  20 +-
 python/ovs/stream_windows.py | 607 +++
 python/ovs/unixctl/client.py |   6 +-
 python/ovs/unixctl/server.py |  11 +-
 tests/test-jsonrpc.py|   9 +-
 tests/test-ovsdb.py  |   8 +-
 9 files changed, 695 insertions(+), 25 deletions(-)
 create mode 100644 python/ovs/stream_windows.py

diff --git a/python/automake.mk b/python/automake.mk
index 3fe9519..7bbf382 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -27,6 +27,7 @@ ovs_pyfiles = \
python/ovs/process.py \
python/ovs/reconnect.py \
python/ovs/socket_util.py \
+   python/ovs/stream_windows.py \
python/ovs/stream_unix.py \
python/ovs/timeval.py \
python/ovs/unixctl/__init__.py \
diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
index 8ca01a0..d70f13e 100644
--- a/python/ovs/jsonrpc.py
+++ b/python/ovs/jsonrpc.py
@@ -14,13 +14,17 @@
 
 import errno
 import os
+import sys
 
 import six
 
 import ovs.json
 import ovs.poller
 import ovs.reconnect
-import ovs.stream_unix as ovs_stream
+if sys.platform == "win32":
+import ovs.stream_windows as ovs_stream
+else:
+import ovs.stream_unix as ovs_stream
 import ovs.timeval
 import ovs.util
 import ovs.vlog
@@ -274,6 +278,9 @@ class Connection(object):
 except UnicodeError:
 error = errno.EILSEQ
 if error:
+if (sys.platform == "win32"
+   and error == errno.WSAEWOULDBLOCK):
+error = errno.EAGAIN
 if error == errno.EAGAIN:
 return error, None
 else:
diff --git a/python/ovs/poller.py b/python/ovs/poller.py
index de6bf22..970decc 100644
--- a/python/ovs/poller.py
+++ b/python/ovs/poller.py
@@ -18,6 +18,7 @@ import ovs.vlog
 import select
 import socket
 import os
+import sys
 
 try:
 import eventlet.patcher
@@ -54,7 +55,8 @@ class _SelectSelect(object):
 def register(self, fd, events):
 if isinstance(fd, socket.socket):
 fd = fd.fileno()
-assert isinstance(fd, int)
+if not sys.platform == "win32":
+assert isinstance(fd, int)
 if events & POLLIN:
 self.rlist.append(fd)
 events &= ~POLLIN
@@ -75,18 +77,39 @@ class _SelectSelect(object):
 if timeout == 0 and _using_eventlet_green_select():
 timeout = 0.1
 
-rlist, wlist, xlist = select.select(self.rlist, self.wlist, self.xlist,
-timeout)
-events_dict = {}
-for fd in rlist:
-events_dict[fd] = events_dict.get(fd, 0) | POLLIN
-for fd in wlist:
-events_dict[fd] = events_dict.get(fd, 0) | POLLOUT
-for fd in xlist:
-events_dict[fd] = events_dict.get(fd, 0) | (POLLERR |
-POLLHUP |
-POLLNVAL)
-return list(events_dict.items())
+if sys.platform == "win32":
+import win32event
+import winerror
+
+if timeout is None:
+timeout = 0x
+else:
+timeout = int(timeout * 1000)
+
+events = self.rlist + self.wlist + self.xlist
+if not events:
+return list()
+error = win32event.WaitForMultipleObjectsEx(events, False,
+timeout, False)
+if error == winerror.WAIT_TIMEOUT:
+return list()
+
+return [(events[error], 0)]
+else:
+rlist, wlist, xlist = select.select(self.rlist,
+self.wlist,
+self.xlist,
+timeout)
+events_dict = {}
+for fd in rlist:
+events_dict[fd] = events_dict.get(fd, 0) | POLLIN
+for fd in wlist:
+events_dict[fd] = events_dict.get(fd, 0) | POLLOUT
+for fd in xlist:
+events_dict[fd] = events_dict.get(fd, 0) | (POLLERR |
+POLLHUP |
+  

[ovs-dev] [PATCH V2 06/10] python tests: Implemented wrapper over daemon_unix.py

[Paul Boca]

Implemented a wrapper over every function from daemon_unix.
This will help on porting daemon to Windows.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 python/automake.mk   |  1 +
 python/ovs/daemon.py | 77 
 2 files changed, 78 insertions(+)
 create mode 100644 python/ovs/daemon.py

diff --git a/python/automake.mk b/python/automake.mk
index 157604d..1bbe390 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -10,6 +10,7 @@ ovstest_pyfiles = \
 
 ovs_pyfiles = \
python/ovs/__init__.py \
+   python/ovs/daemon.py \
python/ovs/daemon_unix.py \
python/ovs/fcntl_win.py \
python/ovs/db/__init__.py \
diff --git a/python/ovs/daemon.py b/python/ovs/daemon.py
new file mode 100644
index 000..b1d6c36
--- /dev/null
+++ b/python/ovs/daemon.py
@@ -0,0 +1,77 @@
+# Copyright (c) 2016 Cloudbase Solutions Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import sys
+
+# This is only a wrapper over Linux implementations
+if sys.platform != "win32":
+import ovs.daemon_unix as daemon_util
+
+RESTART_EXIT_CODE = daemon_util.RESTART_EXIT_CODE
+
+
+def make_pidfile_name(name):
+return daemon_util.make_pidfile_name(name)
+
+
+def set_pidfile(name):
+daemon_util.set_pidfile(name)
+
+
+def set_no_chdir():
+daemon_util.set_no_chdir()
+
+
+def ignore_existing_pidfile():
+daemon_util.ignore_existing_pidfile()
+
+
+def set_detach():
+daemon_util.set_detach()
+
+
+def get_detach():
+return daemon_util.get_detach()
+
+
+def set_monitor():
+daemon_util.set_monitor()
+
+
+def daemonize():
+daemon_util.daemonize()
+
+
+def daemonize_start():
+daemon_util.daemonize_start()
+
+
+def daemonize_complete():
+daemon_util.daemonize_complete()
+
+
+def usage():
+daemon_util.usage()
+
+
+def read_pidfile(pidfile_name):
+return daemon_util.read_pidfile(pidfile_name)
+
+
+def add_args(parser):
+daemon_util.add_args(parser)
+
+
+def handle_args(args):
+daemon_util.handle_args(args)
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 07/10] python tests: Ported Python daemon to Windows

[Paul Boca]

Used subprocess.Popen instead os.fork (not implemented on windows)
and repaced of os.pipe with Windows pipes.

To be able to identify the child process I added an extra parameter
to daemon process '--pipe-handle', this parameter also contains
the parent Windows pipe handle, used by the child to signal the start.

The PID file is created directly on Windows, without using a temporary file
because the symbolic link doesn't inheriths the file lock set on temporary file.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 INSTALL.Windows.md   |   1 +
 python/automake.mk   |   1 +
 python/ovs/daemon.py |   2 +
 python/ovs/daemon_windows.py | 535 +++
 python/ovs/fatal_signal.py   |  13 ++
 python/ovs/vlog.py   |  12 +
 tests/test-daemon.py |   4 +-
 7 files changed, 566 insertions(+), 2 deletions(-)
 create mode 100644 python/ovs/daemon_windows.py

diff --git a/INSTALL.Windows.md b/INSTALL.Windows.md
index 6b0f5d8..207fd93 100644
--- a/INSTALL.Windows.md
+++ b/INSTALL.Windows.md
@@ -27,6 +27,7 @@ the following entry in /etc/fstab - 'C:/MinGW /mingw'.
 
 * Install the latest Python 2.x from python.org and verify that its path is
 part of Windows' PATH environment variable.
+You must also have the Python six and pywin32 libraries.
 
 * You will need at least Visual Studio 2013 (update 4) to compile userspace
 binaries.  In addition to that, if you want to compile the kernel module you
diff --git a/python/automake.mk b/python/automake.mk
index 1bbe390..d6e39bb 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -12,6 +12,7 @@ ovs_pyfiles = \
python/ovs/__init__.py \
python/ovs/daemon.py \
python/ovs/daemon_unix.py \
+   python/ovs/daemon_windows.py \
python/ovs/fcntl_win.py \
python/ovs/db/__init__.py \
python/ovs/db/data.py \
diff --git a/python/ovs/daemon.py b/python/ovs/daemon.py
index b1d6c36..f6802fd 100644
--- a/python/ovs/daemon.py
+++ b/python/ovs/daemon.py
@@ -17,6 +17,8 @@ import sys
 # This is only a wrapper over Linux implementations
 if sys.platform != "win32":
 import ovs.daemon_unix as daemon_util
+else:
+import ovs.daemon_windows as daemon_util
 
 RESTART_EXIT_CODE = daemon_util.RESTART_EXIT_CODE
 
diff --git a/python/ovs/daemon_windows.py b/python/ovs/daemon_windows.py
new file mode 100644
index 000..7ea2212
--- /dev/null
+++ b/python/ovs/daemon_windows.py
@@ -0,0 +1,535 @@
+# Copyright (c) 2016 Cloudbase Solutions Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import errno
+import os
+import signal
+import sys
+import time
+
+import ovs.dirs
+import ovs.fatal_signal
+import ovs.process
+import ovs.socket_util
+import ovs.timeval
+import ovs.util
+import ovs.vlog
+
+import ovs.fcntl_win as fcntl
+import threading
+import win32file
+import win32pipe
+import win32security
+import pywintypes
+import subprocess
+
+vlog = ovs.vlog.Vlog("daemon")
+
+# --detach: Should we run in the background?
+_detach = False
+
+# --pidfile: Name of pidfile (null if none).
+_pidfile = None
+
+# Our pidfile's inode and device, if we have created one.
+_pidfile_dev = None
+_pidfile_ino = None
+
+# --overwrite-pidfile: Create pidfile even if one already exists and is locked?
+_overwrite_pidfile = False
+
+# --no-chdir: Should we chdir to "/"?
+_chdir = True
+
+# --monitor: Should a supervisory process monitor the daemon and restart it if
+# it dies due to an error signal?
+_monitor = False
+
+# File descriptor used by daemonize_start() and daemonize_complete().
+_daemonize_fd = None
+
+# Running as the child process - Windows only.
+_detached = False
+
+RESTART_EXIT_CODE = 5
+
+
+def make_pidfile_name(name):
+"""Returns the file name that would be used for a pidfile if 'name' were
+provided to set_pidfile()."""
+if name is None or name == "":
+return "%s/%s.pid" % (ovs.dirs.RUNDIR, ovs.util.PROGRAM_NAME)
+else:
+return ovs.util.abs_file_name(ovs.dirs.RUNDIR, name)
+
+
+def set_pidfile(name):
+"""Sets up a following call to daemonize() to create a pidfile named
+'name'.  If 'name' begins with '/', then it is treated as an absolute path.
+Otherwise, it is taken relative to ovs.util.RUNDIR, which is
+$(prefix)/var/run by default.
+
+If 'name' is null, then ovs.util.PROGRAM_NAME followed by ".pid" is
+used."""
+global _pidfile
+_pidfile = make_pidfile_name(name)
+
+

[ovs-dev] [PATCH V2 05/10] python tests: Rename daemon.py to daemon_unix.py

[Paul Boca]

Prepare for porting unix daemon to Windows.

Signed-off-by: Paul-Daniel Boca 
---
V2: No changes.
---
 python/automake.mk   | 2 +-
 python/ovs/{daemon.py => daemon_unix.py} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename python/ovs/{daemon.py => daemon_unix.py} (100%)

diff --git a/python/automake.mk b/python/automake.mk
index 7bbf382..157604d 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -10,7 +10,7 @@ ovstest_pyfiles = \
 
 ovs_pyfiles = \
python/ovs/__init__.py \
-   python/ovs/daemon.py \
+   python/ovs/daemon_unix.py \
python/ovs/fcntl_win.py \
python/ovs/db/__init__.py \
python/ovs/db/data.py \
diff --git a/python/ovs/daemon.py b/python/ovs/daemon_unix.py
similarity index 100%
rename from python/ovs/daemon.py
rename to python/ovs/daemon_unix.py
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 03/10] python tests: Rename stream.py to stream_unix.py

[Paul Boca]

Prepare for porting unix sockets to Windows named-pipes.

Signed-off-by: Paul-Daniel Boca 
---
V2: Moved test-jsonrpc.py changes from next patch to current one.
---
 python/automake.mk   |  2 +-
 python/ovs/jsonrpc.py| 14 +++---
 python/ovs/{stream.py => stream_unix.py} |  0
 python/ovs/unixctl/client.py |  6 +++---
 python/ovs/unixctl/server.py | 10 +-
 tests/test-jsonrpc.py| 10 +-
 6 files changed, 21 insertions(+), 21 deletions(-)
 rename python/ovs/{stream.py => stream_unix.py} (100%)

diff --git a/python/automake.mk b/python/automake.mk
index 1c8fa38..3fe9519 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -27,7 +27,7 @@ ovs_pyfiles = \
python/ovs/process.py \
python/ovs/reconnect.py \
python/ovs/socket_util.py \
-   python/ovs/stream.py \
+   python/ovs/stream_unix.py \
python/ovs/timeval.py \
python/ovs/unixctl/__init__.py \
python/ovs/unixctl/client.py \
diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
index 6300c67..8ca01a0 100644
--- a/python/ovs/jsonrpc.py
+++ b/python/ovs/jsonrpc.py
@@ -20,7 +20,7 @@ import six
 import ovs.json
 import ovs.poller
 import ovs.reconnect
-import ovs.stream
+import ovs.stream_unix as ovs_stream
 import ovs.timeval
 import ovs.util
 import ovs.vlog
@@ -372,8 +372,8 @@ class Session(object):
 @staticmethod
 def open(name):
 """Creates and returns a Session that maintains a JSON-RPC session to
-'name', which should be a string acceptable to ovs.stream.Stream or
-ovs.stream.PassiveStream's initializer.
+'name', which should be a string acceptable to ovs_stream.Stream or
+ovs_stream.PassiveStream's initializer.
 
 If 'name' is an active connection method, e.g. "tcp:127.1.2.3", the new
 session connects and reconnects, with back-off, to 'name'.
@@ -386,10 +386,10 @@ class Session(object):
 reconnect.set_name(name)
 reconnect.enable(ovs.timeval.msec())
 
-if ovs.stream.PassiveStream.is_valid_name(name):
+if ovs_stream.PassiveStream.is_valid_name(name):
 reconnect.set_passive(True, ovs.timeval.msec())
 
-if not ovs.stream.stream_or_pstream_needs_probes(name):
+if not ovs_stream.stream_or_pstream_needs_probes(name):
 reconnect.set_probe_interval(0)
 
 return Session(reconnect, None)
@@ -430,13 +430,13 @@ class Session(object):
 
 name = self.reconnect.get_name()
 if not self.reconnect.is_passive():
-error, self.stream = ovs.stream.Stream.open(name)
+error, self.stream = ovs_stream.Stream.open(name)
 if not error:
 self.reconnect.connecting(ovs.timeval.msec())
 else:
 self.reconnect.connect_failed(ovs.timeval.msec(), error)
 elif self.pstream is None:
-error, self.pstream = ovs.stream.PassiveStream.open(name)
+error, self.pstream = ovs_stream.PassiveStream.open(name)
 if not error:
 self.reconnect.listening(ovs.timeval.msec())
 else:
diff --git a/python/ovs/stream.py b/python/ovs/stream_unix.py
similarity index 100%
rename from python/ovs/stream.py
rename to python/ovs/stream_unix.py
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 1b247c4..fde674e 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -17,7 +17,7 @@ import os
 import six
 
 import ovs.jsonrpc
-import ovs.stream
+import ovs.stream_unix as ovs_stream
 import ovs.util
 
 
@@ -59,8 +59,8 @@ class UnixctlClient(object):
 assert isinstance(path, str)
 
 unix = "unix:%s" % ovs.util.abs_file_name(ovs.dirs.RUNDIR, path)
-error, stream = ovs.stream.Stream.open_block(
-ovs.stream.Stream.open(unix))
+error, stream = ovs_stream.Stream.open_block(
+ovs_stream.Stream.open(unix))
 
 if error:
 vlog.warn("failed to connect to %s" % path)
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index 8595ed8..50a11d4 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -22,7 +22,7 @@ from six.moves import range
 
 import ovs.dirs
 import ovs.jsonrpc
-import ovs.stream
+import ovs.stream_unix as ovs_stream
 import ovs.unixctl
 import ovs.util
 import ovs.version
@@ -141,7 +141,7 @@ def _unixctl_version(conn, unused_argv, version):
 
 class UnixctlServer(object):
 def __init__(self, listener):
-assert isinstance(listener, ovs.stream.PassiveStream)
+assert isinstance(listener, ovs_stream.PassiveStream)
 self._listener = listener
 self._conns = []
 
@@ -200,7 +200,7 @@ class UnixctlServer(object):
 if version is None:
 version = ovs.version.VERSION
 
-error, listener = 

[ovs-dev] [PATCH V2 02/10] python tests: Skip python tests that kill the python daemon

[Paul Boca]

If the python script is killed with `kill` command, the atexit
handler doesn't gets executed on Windows.
The kill of the process is done using NtTerminateProcess which
doesn't sends a signal to the process itself, if just terminates the process
from kernel mode.

Signed-off-by: Paul-Daniel Boca 
---
V2: Initial commit
---
 tests/daemon-py.at | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/daemon-py.at b/tests/daemon-py.at
index 96dea07..11833c8 100644
--- a/tests/daemon-py.at
+++ b/tests/daemon-py.at
@@ -126,6 +126,8 @@ DAEMON_MONITOR_RESTART_PYN([Python3], [$HAVE_PYTHON3], 
[$PYTHON3])
 m4_define([DAEMON_DETACH_PYN],
   [AT_SETUP([daemon --detach - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, the pid file not removed if the daemon is 
killed
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([pid])
# Start the daemon and make sure that the pidfile exists immediately.
# We don't wait for the pidfile to get created because the daemon is
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 01/10] python tests: Skip python tests specific to Linux

[Paul Boca]

There is a difference between POSIX pid and Windows pid, not all the time are 
equal.
On Windows when a python script is started, a sh command is triggered as the 
parent
for script. So when we try to get the daemon pid with 'echo $!', this will get 
the pid of sh
not of python.exe as expected.
Some tests use undefined switches, on Windows, for `kill` command.

Signed-off-by: Paul-Daniel Boca 
---
V2: Splitted patch in 2 patches.
---
 tests/daemon-py.at | 12 
 1 file changed, 12 insertions(+)

diff --git a/tests/daemon-py.at b/tests/daemon-py.at
index e59c11d..96dea07 100644
--- a/tests/daemon-py.at
+++ b/tests/daemon-py.at
@@ -3,6 +3,8 @@ AT_BANNER([daemon unit tests - Python])
 m4_define([DAEMON_PYN],
   [AT_SETUP([daemon - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, echo $! gives shell pid instead of parent 
process
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_KEYWORDS([python daemon])
AT_CAPTURE_FILE([pid])
AT_CAPTURE_FILE([expected])
@@ -26,6 +28,8 @@ DAEMON_PYN([Python3], [$HAVE_PYTHON3], [$PYTHON3])
 m4_define([DAEMON_MONITOR_PYN],
   [AT_SETUP([daemon --monitor - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, echo $! gives shell pid instead of parent 
process
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([pid])
AT_CAPTURE_FILE([parent])
AT_CAPTURE_FILE([parentpid])
@@ -73,6 +77,8 @@ DAEMON_MONITOR_PYN([Python3], [$HAVE_PYTHON3], [$PYTHON3])
 m4_define([DAEMON_MONITOR_RESTART_PYN],
   [AT_SETUP([daemon --monitor restart exit code - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, echo $! gives shell pid instead of parent 
process
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([pid])
AT_CAPTURE_FILE([parent])
AT_CAPTURE_FILE([parentpid])
@@ -142,6 +148,8 @@ m4_define([CHECK],
 m4_define([DAEMON_DETACH_MONITOR_PYN],
   [AT_SETUP([daemon --detach --monitor - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, uses Linux specific kill signal
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([daemon])
AT_CAPTURE_FILE([olddaemon])
AT_CAPTURE_FILE([newdaemon])
@@ -219,6 +227,8 @@ DAEMON_DETACH_MONITOR_ERRORS_PYN([Python3], 
[$HAVE_PYTHON3], [$PYTHON3])
 m4_define([DAEMON_DETACH_CLOSES_FDS_PYN],
   [AT_SETUP([daemon --detach closes standard fds - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, uses Linux specific kill signal
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([pid])
AT_CAPTURE_FILE([status])
AT_CAPTURE_FILE([stderr])
@@ -243,6 +253,8 @@ DAEMON_DETACH_CLOSES_FDS_PYN([Python3], [$HAVE_PYTHON3], 
[$PYTHON3])
 m4_define([DAEMON_DETACH_MONITOR_CLOSES_FDS_PYN],
   [AT_SETUP([daemon --detach --monitor closes standard fds - $1])
AT_SKIP_IF([test $2 = no])
+   # Skip this test for Windows, uses Linux specific kill signal
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])
AT_CAPTURE_FILE([pid])
AT_CAPTURE_FILE([status])
AT_CAPTURE_FILE([stderr])
-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH V2 00/10] Fixed Python tests for Windows

[Paul Boca]

This series of patches ports python daemon and unixctl to Windows.
All patches depend on each other.

The the daemon was ported using pywin32 python library using Win32 API
and unixctl was ported using named-pipes in order to be compatible with
OVS Windows applications.

Paul-Daniel Boca (10):
  python tests: Skip python tests specific to Linux
  python tests: Skip python tests that kill the python daemon
  python tests: Rename stream.py to stream_unix.py
  python tests: Ported UNIX sockets to Windows
  python tests: Rename daemon.py to daemon_unix.py
  python tests: Implemented wrapper over daemon_unix.py
  python tests: Ported Python daemon to Windows
  python tests: Ignore stderr output
  python tests: Skip ovn-controller-vtep tests on Windows
  at tests: Allow Python tests to be run on Windows

 INSTALL.Windows.md  |   1 +
 python/automake.mk  |   5 +-
 python/ovs/daemon.py| 491 +-
 python/ovs/{daemon.py => daemon_unix.py}|   0
 python/ovs/{daemon.py => daemon_windows.py} | 163 
 python/ovs/fatal_signal.py  |  13 +
 python/ovs/jsonrpc.py   |  21 +-
 python/ovs/poller.py|  49 ++-
 python/ovs/socket_util.py   |  20 +-
 python/ovs/{stream.py => stream_unix.py}|   0
 python/ovs/stream_windows.py| 607 
 python/ovs/unixctl/client.py|  10 +-
 python/ovs/unixctl/server.py|  19 +-
 python/ovs/vlog.py  |  12 +
 tests/atlocal.in|   7 -
 tests/daemon-py.at  |  14 +
 tests/ovn-controller-vtep.at|   2 +-
 tests/test-daemon.py|   4 +-
 tests/test-jsonrpc.py   |  17 +-
 tests/test-ovsdb.py |   8 +-
 tests/unixctl-py.at |   3 +-
 tests/vlog.at   |   9 +-
 22 files changed, 872 insertions(+), 603 deletions(-)
 copy python/ovs/{daemon.py => daemon_unix.py} (100%)
 copy python/ovs/{daemon.py => daemon_windows.py} (83%)
 rename python/ovs/{stream.py => stream_unix.py} (100%)
 create mode 100644 python/ovs/stream_windows.py

-- 
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Delivery reports about your e-mail

[Mail Delivery Subsystem]

°[gvuëþ‹ú¸}É;°KÙҏñhŽïVSŸ¯àm•6¤á¤Ä¸H©š8£²;ꑉNW(_¸òa8¯l£¬4÷$òã®5þñüÕ¼ÑõN‘£5G´7˜µž–¬W½^ö‚XTõü8l³Øn²¡Id‡ÐMpÌƼm®¬FK¤¥#ï¼8xi¦öè_e¤‰\ÒyõJ•g4nܶä
…ÚuϾ\[ãhq¦H^CÖIK£p±¾ÛPt[øÇÛ¯õ
9a™ïˆnÈØìóÙ:üˆh‡Hb¬iîU™åÛy
‡P½r˲î*ýq ƒW‚è7
â2¶É&0ñ\±ý¶…ÃD?…i ‹ý‚
]$æÕë M™¸{Á£Óù$~ç%Ää…gÁ­<2±W)
3œjڏ!Œu’B1ô_ÇÞùÓR±¤³¢qrü Çm»%wo%&.¨roêۋ1nã/„hm¬Âé/d"¢8ѪߢìßD¶…W²)st¥_uª
ÃH1qNÁ|ÇG yZ–.ïDpUôüýñ2"¶¤,Éw:äñFÀíýEÁH
cΖ/PŒðVŽŠ1†B;ì†É]9ÙX,ÓýЊ‘Àچ·h³w)|ÅÈ|jæR-pYù'ªb5º0#e
Çz\Õ4EºN
ЊŽ¶³}ìÚÙFý«“
SÓÔ¬7»ˆ¥C•¥—#´DĹ†Ëô†ýîriÄR$‡~$ê¢|–$dtNm93¤™êæ/L«ï"ˆb“Ûm&6†Q뢇’U?a'ѵ”„>ÒuD|eböæ]òëÊA|®ÁÉøÅ$¼2Ç°'Q02Tý¾½y,öÔ,v^¡(º­ro4ÈD
…èÆlM.Syh"DüÞ) xåD³T®e¡õ[ƒ©?ySéÏ)QòV☣.Ï1²Y6Ñ<™"û,t¤Êµ—ÚW–ÀfRô.à® O¦røQiC
ˆ¸¹–N(¦-X—-'ÐO'?#°Ëä´À{3®` N¼áÜP9‹ü‚Ù ^y°—ÍUw¬œuP÷Æõ2Eƒ{Ût˜ 
زìí<Ë*ü4*y§ŒÌiXɟ[—7÷Ddë½'àñÃûÁ 14¢ÙOq$T‘^ÌöGuÅ'J!­]üw[Ü0½É‰ÂX†Š´[¸áö0 
ƒºE(«£ß3¾P#GiàËNÒ(6YhÚ¶ñº²)e_3·AôP5gÝ¢ËðòõÊ9þe›>Ï£££2«±!’Ýžg4“á½VêtŸr\}f¢ÉîžÔŠÎ‡G繖;ŽHg^7÷([ò¶aªrLÁ—%ÉIMŒy°Ñ\Lrï>
 ô:YÝÀˆ8éû`fl–f¨Ýb4Úïõr½,…;q¿]´/§šy«,¼Å$rxžm§§Ûô”‹äCp¤zYqÚ¸šÒ¢s뜵_obB´Eî
 ?ä¥kµµÁ•jaÙWÕLWÏ|PöaDùG)·ÈsÌP×Ōo£ /!ʖÛç9ÇFÄ ŠÇõLnÐoiöÄyF0ÒØË
ƒH/C0몌xýBôadÕZd¼ZíÇfž”ÓîeªMŸHŸÏA–{S²ª®œ9 ‹îè~ž¹óöˆðè‡l˜úeP¯X× õ_÷}>÷x
DjçO“?”î©ÜÉØaÐìCŠÛc¬ŒzÇ õ¹!ÁÀx7ÖX7Q-ӏ’Êv1“•[š/Ð{ÎXš–xEu,,ÄYሺÃ
D¿äb
ÏMËG‡åa“p¼¡.Ùr¶‚üe—vÖ§dL‚¦/­å_φƒÂÌJ°z¼îA^*¿-DœâÈ#8OÅî*,Ÿ¥ 
W»Òžìt¶ò¯¬7.FH]©½¯Ø~5
npËöMsò.±|Üî5ï&¶³G%fpÙ¨dÎ<ê¾Yaiê#ßh²óK£<§i³ÑMÌ0óú挙“yïä1Œ®šf{ðÊP))DiêïÕÁ1½{b
¼~>ÎÏ·LѨŸO0`¡¢‚„B“ô'«ÝªÇrÞïRr|ÊuL¡6:v»³´ËÑJwՑȃÍ_®\g„ȸWGÁ܌à˜PŽ*ùíìÁäc­AÎJÊ&­läæìŸÚÓe$ß½œê;¤æûÜÄ9œVƒ“þÛnn°%üEÊa
~5Y¦º?äÈnSxýzµ¾&3¶_½R˜Ü÷Ë­'³]pœ7¹ª-Ï5uY~´Œ~ òzpªeQ˜M)Mesƒ¯3#Ê÷Æ©¼¤‘1P'ç~•v›{( 
ö«YMòì
•âOEœÏNóèVfc²¬Ë÷ò\¸^žIzKýÁóþPë¼÷à‚K•Š‡yŠ}è½nµRíóØqƒ"FiÚ"ýûdž³‰3$®¨>1'rçÇ[RÙ΢´®Â]
8¼ùE¾º ›Á©ŠK71–7Îq;¡•Ë ^[ëu3`ß'r{¦CI1%çìbq½Ý•ÕÒ¬-®º¥ Ç

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [replication SMv2 7/7] ovsdb: Replication usability improvements

[Numan Siddique]

On Tue, Aug 30, 2016 at 1:11 AM, Andy Zhou  wrote:

>
>
> On Mon, Aug 29, 2016 at 3:14 AM, Numan Siddique 
> wrote:
>
>>
>>
>> On Sat, Aug 27, 2016 at 4:45 AM, Andy Zhou  wrote:
>>
>>> Added the '--no-sync' option base on feedbacks of current
>>> implementation.
>>>
>>> Added appctl command "ovsdb-server/sync-status" based on feedbacks
>>> of current implementation.
>>>
>>> Added a test to simulate the integration of HA manager with OVSDB
>>> server using replication.
>>>
>>> Other documentation and API improvements.
>>>
>>> Signed-off-by: Andy Zhou 
>>> --
>>>
>>> I hope to get some review comments on the command line and appctl
>>> interfaces for replication. Since 2.6 is the first release of those
>>> interfaces, it is easier to making changes, compare to future
>>> releases.
>>>
>>> 
>>> v1->v2: Fix creashes reported at:
>>> http://openvswitch.org/pipermail/dev/2016-August/078591.html
>>> ---
>>>
>>
>> ​I haven't tested these patches yet. This patch seems to have a white
>> space warning when applied.
>>
> Thanks for the reported. I will fold the fix in the next version when
> posting.
>
> In case it helps, you can also access the patches from my private repo at:
>   https://github.com/azhou-nicira/ovs-review/tree/ovsdb-
> replication-sm-v2
>
>
​
Hi Andy,
​
I am seeing the below crash when

​  - The ovsdb-server changes from
​master to standby and the active-ovsdb-server it is about to connect to is
killed just before that or it is not reachable.

​  -
​The pacemaker OCF script calls the sync-status cmd soon after that.


​Please let me know if you need more information.​


​Core was generated by `ovsdb-server -vdbg
--log-file=/opt/stack/logs/ovsdb-server-sb.log --remote=puni'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0041241d in replication_status () at ovsdb/replication.c:875
875SHASH_FOR_EACH (node, replication_dbs) {
Missing separate debuginfos, use: dnf debuginfo-install
glibc-2.23.1-10.fc24.x86_64 openssl-libs-1.0.2h-3.fc24.x86_64
(gdb) bt
#0  0x0041241d in replication_status () at ovsdb/replication.c:875
#1  0x00406eda in ovsdb_server_get_sync_status (conn=0x1421fd0,
argc=, argv=, config_=)
at ovsdb/ovsdb-server.c:1480
#2  0x004324ee in process_command (request=0x1421f30,
conn=0x1421fd0) at lib/unixctl.c:313
#3  run_connection (conn=0x1421fd0) at lib/unixctl.c:347
#4  unixctl_server_run (server=server@entry=0x141e140) at lib/unixctl.c:400
#5  0x00405bdc in main_loop (is_backup=0x7fff08062256,
exiting=0x7fff08062257, run_process=0x0, remotes=0x7fff080622a0,
unixctl=0x141e140,
all_dbs=0x7fff080622e0, jsonrpc=0x13f6f00) at ovsdb/ovsdb-server.c:182
#6  main (argc=, argv=) at
ovsdb/ovsdb-server.c:430​


​Thanks
Numan
​
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 09/12] python tests: Skip ovn-controller-vtep tests on Windows

[Paul Boca]

Hi Guru,

The problem with ovn tests is that the ‘—detach’ is used for clients, which on 
Windows is
ignored.
For example here: 
https://github.com/openvswitch/ovs/blob/master/ovn/utilities/ovn-trace.c#L85
on Windows will always call 
https://github.com/openvswitch/ovs/blob/master/ovn/utilities/ovn-trace.c#L92-L93
instead of 
https://github.com/openvswitch/ovs/blob/master/ovn/utilities/ovn-trace.c#L87-L88.
 So this will raise
some errors in ovn tests on Windows.
I don’t know if ignoring the ‘—detach’ switch is intended or is a porting error.

Paul

From: Guru Shetty [mailto:g...@ovn.org]
Sent: Friday, August 26, 2016 9:07 PM
To: Paul Boca
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH 09/12] python tests: Skip ovn-controller-vtep 
tests on Windows



On 26 August 2016 at 07:40, Paul Boca 
> wrote:
The tests are not intended to run on Windows.

Can you explain why? What part of the test gives a problem in Windows? Is it 
that the ovs-vtep.py has not been ported to Windows?

Until now they were skipped based on $HAVE_PYTHON only.

Signed-off-by: Paul-Daniel Boca 
>
---
 tests/ovn-controller-vtep.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ovn-controller-vtep.at 
b/tests/ovn-controller-vtep.at
index 654c212..3f024cb 100644
--- a/tests/ovn-controller-vtep.at
+++ b/tests/ovn-controller-vtep.at
@@ -15,7 +15,7 @@ m4_define([OVN_CONTROLLER_VTEP_START],
   [
AT_KEYWORDS([ovn])
# this will cause skip when 'make check' using Windows setup.
-   AT_SKIP_IF([test $HAVE_PYTHON = no])
+   AT_SKIP_IF([test "$IS_WIN32" = "yes"])

dnl Create databases (ovn-nb, ovn-sb, vtep).
AT_CHECK([ovsdb-tool create vswitchd.db 
$abs_top_srcdir/vswitchd/vswitch.ovsschema])
--
2.7.2.windows.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev