Hi Mark, Sorry for getting back to you late. Thanks for your review. I totally agree with your very detailed suggestion. I think it is very useful, especially x2nrealloc. I will resubmit later.
Thanks, Gongming On 2021/2/11, 9:26 AM, "Mark Michelson" <mmich...@redhat.com> wrote: Hi Gongming. I saw Dumitru's review, so I won't repeat anything he said. I have a few comments of my own below. On 2/7/21 7:25 AM, gmingchen(陈供明) wrote: > From: Gongming Chen <gmingc...@tencent.com> > > In the ovn security group, each host ip corresponds to at least 4 flow > tables (different connection states). As the scale of hosts using the > security group increases, the ovs security group flow table will > increase sharply, especially when it is applied the remote group > feature in OpenStack. > > This patch merges ipv4 addresses with wildcard masks, and replaces this > ipv4 addresses with the merged ip/mask. This will greatly reduce the > entries in the ovs security group flow table, especially when the host > size is large. After being used in a production environment, the number > of ovs flow tables will be reduced by at least 50% in most scenarios, > when the remote group in OpenStack is applied. > > Analysis in the simplest scenario, a network 1.1.1.0/24 network, enable > the OpenStack security group remote group feature, create 253 virtual > machine ports(1.1.1.2-1.1.1.254). > > Only focus on the number of ip addresses, in the table=44 table: > ./configure --disable-combine-ipv4: > 1.1.1.2-1.1.1.254(253 flow meters) * 4(connection status) * > 1(local net of localport) = 1012 > > ./configure --enable-combine-ipv4(default): > 1.1.1.2/31 > 1.1.1.4/30 > 1.1.1.8/29 > 1.1.1.16/28 > 1.1.1.32/27 > 1.1.1.64/26 > 1.1.1.128/26 > 1.1.1.192/27 > 1.1.1.224/28 > 1.1.1.240/29 > 1.1.1.248/30 > 1.1.1.252/31 > 1.1.1.254 > 13 flow tables * 4(connection status) * 1(local net of localport) = 52 > > Reduced from 1012 flow meters to 52, a 19.4 times reduction. > > Some scenes are similar to the following: > 1.1.1.2, 1.1.1.6 > After the combine: > 1.1.1.2/255.255.255.251 > This will slightly increase the difficulty of finding the flow table > corresponding to a single address. > such as: > ovs-ofctl dump-flows br-int | grep 1.1.1.6 > The result is empty. > 1.1.1.6 will match 1.1.1.2/255.255.255.251 > > Signed-off-by: Gongming Chen <gmingc...@tencent.com> > --- > configure.ac | 1 + > lib/expr.c | 217 +++++++++++++++++++++++++++++++++++ > m4/ovn.m4 | 21 ++++ > tests/atlocal.in | 1 + > tests/ovn.at | 286 +++++++++++++++++++++++++++++++++-------------- > 5 files changed, 443 insertions(+), 83 deletions(-) > > diff --git a/configure.ac b/configure.ac > index b2d084318..6dc51a5cc 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -131,6 +131,7 @@ OVS_LIBTOOL_VERSIONS > OVS_CHECK_CXX > AX_FUNC_POSIX_MEMALIGN > OVN_CHECK_UNBOUND > +OVN_CHECK_COMBINE_IPV4 > > OVS_CHECK_INCLUDE_NEXT([stdio.h string.h]) > AC_CONFIG_FILES([lib/libovn.sym]) > diff --git a/lib/expr.c b/lib/expr.c > index 796e88ac7..0b6dee3b3 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -1030,6 +1030,194 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > } > } > > +struct ip_v > +{ > + uint32_t *ip; > + uint32_t size; > +}; > + > +struct ip_r > +{ > + uint32_t *ip; > + uint32_t *mask; > + uint32_t used; > + uint32_t size; > + bool *masked; > +}; Since the ip, mask, and masked arrays all have the same size, it would make memory management easier if they were combined into a single struct. For example: struct ip_r_entry { uint32_t ip; uint32_t mask; bool masked; }; struct ip_r { struct ip_r_entry *entries; uint32_t used; uint32_t size; }; This reduces the number of malloced pointers from 3 to 1, making it easier to reallocate and free data when necessary. Also what do the names ip_v and ip_r mean? I'm guessing that ip_v might mean "IP vector"? But I have no idea what ip_r means. I think these structs could use better names. > + > +/* Macro to check ip return data and xrealloc. */ > +#define CHECK_REALLOC_IP_R_DATA(IP_R_DATA) \ > + if (IP_R_DATA->used >= IP_R_DATA->size) { \ > + IP_R_DATA->ip = xrealloc(IP_R_DATA->ip, \ > + 2 * IP_R_DATA->size * sizeof(uint32_t)); \ > + IP_R_DATA->mask = xrealloc(IP_R_DATA->mask, \ > + 2 * IP_R_DATA->size * sizeof(uint32_t)); \ > + IP_R_DATA->masked = xrealloc(IP_R_DATA->masked, \ > + 2 * IP_R_DATA->size * sizeof(bool)); \ > + IP_R_DATA->size = IP_R_DATA->size * 2; \ > + } OVS provides a function called x2nrealloc() that will double the size of an array and update the size. If you use my suggestion above, then this macro becomes: #define CHECK_REALLOC_IP_R_DATA(IP_R_DATA) \ if (IP_R_DATA->used >= IP_R_DATA->size) { \ IP_R_DATA->entries = x2nrealloc(IP_R_DATA->entries, &IP_R_DATA->size, sizeof *IP_R_DATA->entries); \ } > + > +static void > +combine_ipv4_in_mask(struct ip_v *ip_data, struct ip_r *ip_r_data){ I have to admit, I'm having trouble understanding this algorithm. As a result, I've only made structural suggestions below. > + int i = 0, recorded =0; I noticed that "recorded" is always set to either 0 or 1, and it's never set based on some mathematical operation. So it probably would make more sense to change it to a bool. > + uint32_t diff, connect, start, end, continuous_size, mask_base; > + > + start = 0; > + continuous_size = 0; > + mask_base = 0; > + end = 0; > + > + memset(ip_r_data, 0, sizeof(struct ip_r)); > + ip_r_data->ip = xmalloc(4 * sizeof(uint32_t)); > + ip_r_data->mask = xmalloc(4 * sizeof(uint32_t)); > + ip_r_data->masked = xmalloc(4 * sizeof(bool)); > + ip_r_data->size = 4; > + > + while (i < ip_data->size) { Suggestion: I think this while loop could be replaced with a for loop. The reason is that every continue statement in the loop has "i++" before it. So I think the while could be replaced with: for (i = 0; i < ip_data->size; i++) Then you can remove all the "i++" calls before the continue statements. > + if (i + 1 >= ip_data->size) { > + goto end_segment; > + } > + > + diff = ip_data->ip[i + 1] - ip_data->ip[i]; > + /* Ignore equal node. */ > + if (0 == diff) { > + i++; > + continue; > + } > + /* Continuous in the segment. */ > + if ((diff & (diff - 1)) == 0) { > + /* New segment. */ > + if (0 == recorded) { > + connect = ip_data->ip[i + 1] ^ ip_data->ip[i]; > + /* Only one bit different. */ > + if (0 == (connect & (connect - 1))) { > + recorded = 1; > + start = ip_data->ip[i]; > + continuous_size = 2; > + mask_base = connect; > + > + int j = 0; > + end = start; > + /* The first non-zero place in the high direction is > + * the end of the segment. */ > + while (j < 32 && (0 == (start & (mask_base << j)))) { > + end |= (mask_base << j); > + j++; > + } > + > + i++; > + continue; > + /* Different segments and different bit, dnot merge. */ > + }else { > + CHECK_REALLOC_IP_R_DATA(ip_r_data) > + > + ip_r_data->ip[ip_r_data->used] = ip_data->ip[i]; > + ip_r_data->masked[ip_r_data->used] = false; > + ip_r_data->used++; > + > + i++; > + continue; > + } > + > + /* Recording in the current segment. */ > + }else{ > + /* Stop and merge mask. */ > + if (ip_data->ip[i + 1] > end) { > + recorded = 0; > + while (continuous_size) { > + CHECK_REALLOC_IP_R_DATA(ip_r_data) > + > + int segment_power, pow_base; > + if (0 == continuous_size) { > + segment_power = 0; > + } else { > + segment_power = 31 - clz32(continuous_size); > + } > + > + if (0 == mask_base) { > + pow_base = 0; > + } else { > + pow_base = 31 - clz32(mask_base); > + } > + > + ip_r_data->mask[ip_r_data->used] = > + ~(((1 << segment_power) - 1) << pow_base); > + ip_r_data->ip[ip_r_data->used] = > + ip_r_data->mask[ip_r_data->used] & start; > + ip_r_data->masked[ip_r_data->used] = true; > + ip_r_data->used++; > + > + continuous_size &= (~(1 << segment_power)); > + start = ip_r_data->ip[ip_r_data->used - 1] > + + (1 << (segment_power + pow_base)); > + } > + > + i++; > + continue; > + } > + > + continuous_size++; > + i++; > + continue; > + } > + /* Not continuous in segment or is the end of the ip data. */ > + } else { > +end_segment: > + if (recorded) { > + recorded = 0; > + while (continuous_size) { > + CHECK_REALLOC_IP_R_DATA(ip_r_data) > + > + int segment_power, pow_base; > + if (0 == continuous_size) { > + segment_power = 0; > + } else { > + segment_power = 31 - clz32(continuous_size); > + } > + > + if (0 == mask_base) { > + pow_base = 0; > + } else { > + pow_base = 31 - clz32(mask_base); > + } > + > + ip_r_data->mask[ip_r_data->used] = > + ~(((1 << segment_power) - 1) << pow_base); > + ip_r_data->ip[ip_r_data->used] = > + ip_r_data->mask[ip_r_data->used] & start; > + ip_r_data->masked[ip_r_data->used] = true; > + ip_r_data->used++; > + > + continuous_size &= (~(1 << segment_power)); > + start = ip_r_data->ip[ip_r_data->used - 1] > + + (1 << (segment_power + pow_base)); > + } > + > + i++; > + continue; > + } else { > + CHECK_REALLOC_IP_R_DATA(ip_r_data) > + > + ip_r_data->ip[ip_r_data->used] = ip_data->ip[i]; > + ip_r_data->masked[ip_r_data->used] = false; > + ip_r_data->used++; > + > + i++; > + continue; > + } > + } > + } > +} > + > +static int > +compare_mask_ip(const void *a, const void *b) > +{ > + uint32_t a_ = *(uint32_t *)a; > + uint32_t b_ = *(uint32_t *)b; > + > + return a_ < b_ ? -1 : a_ > b_; > +} > + > /* Adds an constant set named 'name' to 'const_sets', replacing any existing > * constant set entry with the given name. */ > void > @@ -1044,6 +1232,11 @@ expr_const_sets_add(struct shash *const_sets, const char *name, > cs->in_curlies = true; > cs->n_values = 0; > cs->values = xmalloc(n_values * sizeof *cs->values); > + struct ip_r ip_r_data; > + struct ip_v ip_data; > + ip_data.ip = xmalloc(n_values * sizeof(uint32_t)); > + ip_data.size = 0; > + > if (convert_to_integer) { > cs->type = EXPR_C_INTEGER; > for (size_t i = 0; i < n_values; i++) { > @@ -1056,6 +1249,11 @@ expr_const_sets_add(struct shash *const_sets, const char *name, > && lex.token.type != LEX_T_MASKED_INTEGER) { > VLOG_WARN("Invalid constant set entry: '%s', token type: %d", > values[i], lex.token.type); > +#ifdef HAVE_COMBINE_IPV4 > + } else if (lex.token.type == LEX_T_INTEGER > + && lex.token.format == LEX_F_IPV4) { > + ip_data.ip[ip_data.size++] = ntohl(lex.token.value.ipv4); > +#endif > } else { > union expr_constant *c = &cs->values[cs->n_values++]; > c->value = lex.token.value; > @@ -1075,6 +1273,25 @@ expr_const_sets_add(struct shash *const_sets, const char *name, > } > } > > + if (ip_data.size > 0) { > + qsort(ip_data.ip, ip_data.size, sizeof(uint32_t), compare_mask_ip); > + combine_ipv4_in_mask(&ip_data, &ip_r_data); > + for (int i = 0; i < ip_r_data.used; ++i) { > + union expr_constant *c = &cs->values[cs->n_values++]; > + memset(&c->value, 0, sizeof c->value); > + memset(&c->mask, 0, sizeof c->mask); > + c->value.ipv4 = htonl(ip_r_data.ip[i]); > + c->format = LEX_F_IPV4; > + c->masked = ip_r_data.masked[i]; > + if (c->masked) { > + c->mask.ipv4 = htonl(ip_r_data.mask[i]); > + } > + } > + free(ip_r_data.ip); > + free(ip_r_data.mask); > + free(ip_r_data.masked); > + } > + > shash_add(const_sets, name, cs); > } > > diff --git a/m4/ovn.m4 b/m4/ovn.m4 > index dacfabb2a..49fcbef9b 100644 > --- a/m4/ovn.m4 > +++ b/m4/ovn.m4 > @@ -576,3 +576,24 @@ AC_DEFUN([OVN_CHECK_UNBOUND], > fi > AM_CONDITIONAL([HAVE_UNBOUND], [test "$HAVE_UNBOUND" = yes]) > AC_SUBST([HAVE_UNBOUND])]) > + > +dnl Checks for combine multiple ipv4 with wildcard mask. > +AC_DEFUN([OVN_CHECK_COMBINE_IPV4], > + [AC_ARG_ENABLE( > + [combine-ipv4], > + [AC_HELP_STRING([--disable-combine-ipv4], [Disable combine multiple ipv4 feature])], > + [case "${enableval}" in > + (yes) combine_ipv4=true ;; > + (no) combine_ipv4=false ;; > + (*) AC_MSG_ERROR([bad value ${enableval} for --disable-combine-ipv4]) ;; > + esac], > + [combine_ipv4=true]) > + > + #AM_CONDITIONAL([HAVE_COMBINE_IPV4], [test "$combine_ipv4" = true]) > + #AC_DEFINE([HAVE_COMBINE_IPV4], [1], [Define to 1 if combine-ipv4 is enable.]) > + if test "$combine_ipv4" = true; then > + AC_DEFINE([HAVE_COMBINE_IPV4], [1], [Define to 1 if combine-ipv4 is enable.]) > + HAVE_COMBINE_IPV4=yes > + fi > + AC_SUBST([HAVE_COMBINE_IPV4]) > + AM_CONDITIONAL([HAVE_COMBINE_IPV4], [test "$combine_ipv4" = true])]) > diff --git a/tests/atlocal.in b/tests/atlocal.in > index 5ebc8e117..3024a2d2a 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -3,6 +3,7 @@ HAVE_OPENSSL='@HAVE_OPENSSL@' > OPENSSL_SUPPORTS_SNI='@OPENSSL_SUPPORTS_SNI@' > HAVE_UNBOUND='@HAVE_UNBOUND@' > EGREP='@EGREP@' > +HAVE_COMBINE_IPV4='@HAVE_COMBINE_IPV4@' > > if test x"$PYTHON3" = x; then > PYTHON3='@PYTHON3@' > diff --git a/tests/ovn.at b/tests/ovn.at > index 80c9fe138..094a3f1e8 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -616,6 +616,24 @@ ip,nw_src=10.0.0.1 > ip,nw_src=10.0.0.2 > ip,nw_src=10.0.0.3 > ]) > + > +if test "$HAVE_COMBINE_IPV4" = yes; then > +AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2/31 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl > +ip,nw_src=1.2.3.4 > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2/31 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'], [0], [dnl > +ip,nw_src=1.2.0.0/20 > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2/31 > +ip,nw_src=5.5.5.0/24 > +]) > +else > AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl > ip,nw_src=10.0.0.1 > ip,nw_src=10.0.0.2 > @@ -634,6 +652,8 @@ ip,nw_src=10.0.0.2 > ip,nw_src=10.0.0.3 > ip,nw_src=5.5.5.0/24 > ]) > +fi > + > AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl > ipv6,ipv6_src=::1 > ipv6,ipv6_src=::2 > @@ -13580,26 +13600,46 @@ cat 2.expected > expout > $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets > AT_CHECK([cat 2.packets], [0], [expout]) > > -# There should be total of 9 flows present with conjunction action and 2 flows > -# with conj match. Eg. > -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2) > - > -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conj_id | wc -l`]) > +if test "$HAVE_COMBINE_IPV4" = yes; then > + # There should be total of 6 flows present with conjunction action and 2 flows > + # with conj match. Eg. > + #table=45, n_packets=1, n_bytes=42, idle_age=211, priority=2001,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) > + #table=45, n_packets=0, n_bytes=0, idle_age=211, priority=2001,conj_id=3,ip,metadata=0x1 actions=drop > + #priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,2/2),conjunction(3,2/2) > + #priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(2,2/2),conjunction(3,2/2) > + #priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(2,1/2) > + #priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,1/2) > + #priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2) > + #priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2) > + > + OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conj_id | wc -l`]) > +else > + # There should be total of 9 flows present with conjunction action and 2 flows > + # with conj match. Eg. > + # table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) > + # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(3,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(3,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(2,1/2),conjunction(3,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(2,1/2),conjunction(3,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(2,1/2),conjunction(3,1/2) > + > + OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conj_id | wc -l`]) > +fi > > as hv1 ovs-ofctl dump-flows br-int > > @@ -13621,25 +13661,44 @@ AT_CHECK([cat 2.packets], [0], []) > > # Remove the first ACL, and verify that the conjunction flows are updated > # properly. > -# There should be total of 6 flows present with conjunction action and 1 flow > -# with conj match. Eg. > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2) > > ovn-nbctl acl-del ls1 to-lport 1001 \ > 'ip4 && ip4.src == $set1 && ip4.dst == $set1' > > -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conj_id | wc -l`]) > + > +if test "$HAVE_COMBINE_IPV4" = yes; then > + # There should be total of 6 flows present with conjunction action and 1 flow > + # with conj match. Eg. > + # table=45, n_packets=1, n_bytes=42, idle_age=34, priority=2001,conj_id=3,ip,metadata=0x1 actions=drop > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(3,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2) > + > + OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conj_id | wc -l`]) > +else > + # There should be total of 6 flows present with conjunction action and 1 flow > + # with conj match. Eg. > + # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,1/2) > + > + OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 1 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conj_id | wc -l`]) > +fi > > # Add the ACL back > ovn-nbctl acl-add ls1 to-lport 1001 \ > @@ -13648,44 +13707,80 @@ ovn-nbctl acl-add ls1 to-lport 1001 \ > ovn-nbctl acl-add ls1 to-lport 1001 \ > 'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9, 10.0.0.10}' drop > > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > - > -OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction.*conjunction | wc -l`]) > +if test "$HAVE_COMBINE_IPV4" = yes; then > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(3,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31 actions=conjunction(3,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2) > + > + OVS_WAIT_UNTIL([test 8 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +else > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,1/2),conjunction(6,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(6,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2) > + > + OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +fi > > # Remove 10.0.0.7 from address set2. All flows should be updated properly. > ovn-nbctl set Address_Set set2 \ > addresses=\"10.0.0.8\",\"10.0.0.9\" > > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > - > -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction.*conjunction | wc -l`]) > +if test "$HAVE_COMBINE_IPV4" = yes; then > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31 actions=drop > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(4,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(5,2/2),conjunction(4,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(5,2/2),conjunction(4,2/2) > + > + OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +else > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(9,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(7,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(8,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(9,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(7,1/2),conjunction(8,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(9,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2) > + > + OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +fi > > # Remove an ACL again > ovn-nbctl acl-del ls1 to-lport 1001 \ > @@ -13693,19 +13788,36 @@ ovn-nbctl acl-del ls1 to-lport 1001 \ > > wait_for_ports_up > ovn-nbctl --wait=hv sync > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2) > -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2) > -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2) > - > -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction | wc -l`]) > -OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > -grep conjunction.*conjunction.*conjunction | wc -l`]) > + > +if test "$HAVE_COMBINE_IPV4" = yes; then > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31 actions=drop > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(5,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31 actions=conjunction(5,2/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(5,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(5,1/2) > + > + OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +else > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10 actions=conjunction(10,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(11,1/2) > + # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(10,1/2),conjunction(11,1/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.5 actions=conjunction(10,2/2),conjunction(11,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.6 actions=conjunction(10,2/2),conjunction(11,2/2) > + # priority=2001,ip,metadata=0x1,nw_src=10.0.0.4 actions=conjunction(10,2/2),conjunction(11,2/2) > + > + OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction | wc -l`]) > + OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows br-int | \ > + grep conjunction.*conjunction.*conjunction | wc -l`]) > +fi > > OVN_CLEANUP([hv1]) > AT_CLEANUP > @@ -15350,11 +15462,19 @@ for i in 1 2 3; do > > # Update address set as1 > ovn-nbctl --wait=hv set addr as1 addresses="10.1.2.10 10.1.2.11" > - AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.11"], [0], [ignore]) > + if test "$HAVE_COMBINE_IPV4" = yes; then > + AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.10/31"], [0], [ignore]) > + else > + AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.11"], [0], [ignore]) > + fi > > # Update address set as2 > ovn-nbctl --wait=hv set addr as2 addresses="10.1.2.12 10.1.2.13" > - AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [0], [ignore]) > + if test "$HAVE_COMBINE_IPV4" = yes; then > + AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12/31"], [0], [ignore]) > + else > + AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [0], [ignore]) > + fi > > # Add another ACL referencing as1 > n_flows_before=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l` > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev