Hi Dumitru,
Thanks for review.
On 2021/2/8, 6:19 PM, "Dumitru Ceara" <dce...@redhat.com> wrote:
On 2/7/21 1:25 PM, gmingchen(陈供明) wrote:
> From: Gongming Chen <gmingc...@tencent.com>
Hi Gongming,
First of all, thanks for the contribution!
This is not a full review, just some comments for now.
It seems that there's a memory leak with your patch applied.
AddressSanitizer reports:
Direct leak of 12 byte(s) in 1 object(s) allocated from:
#0 0x49644d in malloc
(/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/tests/ovstest+0x49644d)
#1 0x538604 in xmalloc
/home/runner/work/ovn/ovn/ovs_src/lib/util.c:138:15
#2 0x62636f in expr_const_sets_add
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../lib/expr.c:1237:18
#3 0x4cf8f6 in create_addr_sets
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:230:5
#4 0x4cf8f6 in test_parse_expr__
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:296:5
#5 0x4dfd04 in ovs_cmdl_run_command__
/home/runner/work/ovn/ovn/ovs_src/lib/command-line.c:247:17
#6 0x4c6810 in test_ovn_main
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:1635:5
#7 0x4c6810 in ovstest_wrapper_test_ovn_main__
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/test-ovn.c:1638:1
#8 0x4dfd04 in ovs_cmdl_run_command__
/home/runner/work/ovn/ovn/ovs_src/lib/command-line.c:247:17
#9 0x4c5fe3 in main
/home/runner/work/ovn/ovn/ovn-20.12.90/_build/sub/../../tests/ovstest.c:150:9
#10 0x7f71aa6ddbf6 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
Full reports are available in the ovsrobot OVN github CI run artifacts:
https://github.com/ovsrobot/ovn/actions/runs/545416492
Just a note, if you push the branch to your own fork it will trigger the
github action to run CI and the "linux clang test asan" job will also
enable AddressSanitizer.
Thanks, there are really missing the free ip_data.ip, resulting in a memory
leak.
There are also a few style related issues (e.g., sizeof args), please
see the guidelines here:
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst
Sorry, does this mean that
ip_r_data->ip = xmalloc(4 * sizeof(uint32_t))
should be replaced with
ip_r_data->ip = xmalloc(4 * sizeof ip_r_data->ip)?
>
> 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.
I think it would be great to describe the algorithm here, in the commit
log, but also in the comments in the code.
You are right, I will resubmit and add the description of the algorithm to the
commit log and the code comments.
>
> 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
Would it make sense and potentially make the life of the users easier by
also adding a utility to automatically do the IP/MASK match?
I'm thinking of something like:
$ ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6
[..]
I totally agree with you, adding a utility similar to ovn-match-ip will be more
optimized and easier to troubleshoot.
I will resubmit and add an ovn-match-ip utility.
I didn't look at the implementation in detail but I'm surprised by the
following:
- change test-ovn.c to define $set1 = {1.1.1.2, .. 1.1.1.254}, basically
the example you gave above.
- then run:
$ echo 'ip4.src == $set1' | ./tests/ovstest test-ovn expr-to-flows
ip,nw_src=1.1.1.2/31
ip,nw_src=1.1.1.4
$
Shouldn't this output actually match the 13 entries in your sample output?
I guess you forgot to modify the 3 in test-ovn.c
expr_const_sets_add(addr_sets, "set1", addrs1, 3, true) to 253.
I also tested it, and the output is the same as the 13 entries in the
above example:
$ echo'ip4.src == $set1' | ./tests/ovstest test-ovn expr-to-flows
ip,nw_src=1.1.1.64/26
ip,nw_src=1.1.1.128/26
ip,nw_src=1.1.1.252/31
ip,nw_src=1.1.1.248/30
ip,nw_src=1.1.1.2/31
ip,nw_src=1.1.1.32/27
ip,nw_src=1.1.1.240/29
ip,nw_src=1.1.1.254
ip,nw_src=1.1.1.8/29
ip,nw_src=1.1.1.224/28
ip,nw_src=1.1.1.4/30
ip,nw_src=1.1.1.16/28
ip,nw_src=1.1.1.192/27
$
>
> 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
Is there a specific reason to not have this enabled by default? It
would be great if we could avoid HAVE_COMBINE_IPV4 ifdefs in the code
and tests.
In fact, it is enabled by default.
Previously, because there is no utility like ovn-match-ip, disable the
feature is more helpful for troubleshooting, when the scale is small.
After adding the ovn-match-ip utility, I think it does not matter whether
there is an option to disable this feature.
Then I think HAVE_COMBINE_IPV4 ifdefs can be removed.
>
> 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;
> +};
> +
> +/* Macro to check ip return data and xrealloc. */
In my opinion it would be clearer if this would be a function.
I agree with you, I will replace this macro with function.
> +#define CHECK_REALLOC_IP_R_DATA(IP_R_DATA) \
> + if (IP_R_DATA->used >= IP_R_DATA->size) { \
Probably best to wrap IP_R_DATA as "(IP_R_DATA)"
Yes, that would be better.
> + 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; \
> + }
> +
> +static void
> +combine_ipv4_in_mask(struct ip_v *ip_data, struct ip_r *ip_r_data){
Some comments describing the algorithm would help.
I'll wait for your feedback on all the above before continuing the review.
Thanks,
Dumitru
In addition to the commit log, I will also add a brief description of
the algorithm here.
Thanks,
Gongming
> + int i = 0, recorded =0;
> + 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) {
> + 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
