Re: [ovs-dev] [PATCH 1/3] Revert conjunctive match removal patches.
On Sat, Oct 26, 2019, 2:38 AM Mark Michelson wrote:
> This partially reverts commits 6f914327a55aaab11507db0911b08d695e17ce2a
> and 298701dbc99645700be41680a43d049cb061847a. This restores the behavior
> of making conjunctive matches, and it restores the tests to their state.
>
> The one thing it does not revert is the addition of the
> force_cross_product variable in expr.c. This variable will be used in a
> later commit in this series. Instead of removing it, we just set it
> "false" to restore previous behavior.
>
> With this commit, the conjunctive match misbehavior described in commit
> 298701dbc99645700be41680a43d049cb061847a is restored. However, this will
> be fixed in an upcoming commit in this series.
>
> Signed-off-by: Mark Michelson
>
Acked-by: Numan Siddique
---
> TODO.rst | 10 -
> lib/expr.c |9 +-
> tests/ovn.at | 1250
> +++---
> 3 files changed, 61 insertions(+), 1208 deletions(-)
>
> diff --git a/TODO.rst b/TODO.rst
> index ed55ea236..943d9bf81 100644
> --- a/TODO.rst
> +++ b/TODO.rst
> @@ -145,13 +145,3 @@ OVN To-do List
>* Support FTP ALGs.
>
>* Support reject action.
> -
> -* Conjunction: Conjunction is disabled in OVN. This needs to be revisisted
> - to enable conjunction again after addressing the issues related to it.
> - Like, if there are multiple ACLs with overlapping Conjunction matches,
> - conjunction flows are not added properly.
> - Eg. match(ip4.src == {IP1, IP2, IP3} && ip4.dst == {IP4, IP5, IP6} &&
> - tcp.dst >= 800 && tcp.dst <= 900) actions=drop
> -
> - match(ip4.src == {IP1, IP2, IP3} && ip4.dst == {IP4, IP5, IP6} &&
> - tcp.dst >= 1000 && tcp.dst <= 2000) actions=allow
> diff --git a/lib/expr.c b/lib/expr.c
> index 9b9b6bcca..71de61543 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -42,13 +42,8 @@ VLOG_DEFINE_THIS_MODULE(expr);
> *
> * match 2 - ip4.src == {IP1, IP2} && tcp.dst >=700 && tcp.src <=800
> * action - allow.
> - *
> - * To handle this issue temporarily force crossproduct so that conjunction
> - * flows are not generated.
> - *
> - * Remove this once fixed.
> - * */
> -static bool force_crossproduct = true;
> + */
> +static bool force_crossproduct = false;
>
> static struct expr *parse_and_annotate(const char *s,
> const struct shash *symtab,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 22b272a60..641a646fc 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -589,22 +589,6 @@ ip,reg14=0x6
> ipv6,reg14=0x5
> ipv6,reg14=0x6
> ])
> -AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp
> && tcp.dst == {500, 501}'], [0], [dnl
> -tcp,reg14=0x5,tp_dst=500
> -tcp,reg14=0x5,tp_dst=501
> -tcp,reg14=0x6,tp_dst=500
> -tcp,reg14=0x6,tp_dst=501
> -])
> -AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp
> && tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
> -tcp,reg15=0x5,tp_src=400,tp_dst=500
> -tcp,reg15=0x5,tp_src=400,tp_dst=501
> -tcp,reg15=0x5,tp_src=401,tp_dst=500
> -tcp,reg15=0x5,tp_src=401,tp_dst=501
> -tcp,reg15=0x6,tp_src=400,tp_dst=500
> -tcp,reg15=0x6,tp_src=400,tp_dst=501
> -tcp,reg15=0x6,tp_src=401,tp_dst=500
> -tcp,reg15=0x6,tp_src=401,tp_dst=501
> -])
> AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
> (no flows)
> ])
> @@ -709,14 +693,6 @@ reg15=0x11
> reg15=0x12
> reg15=0x13
> ])
> -AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4,
> 10.0.0.5}'], [0], [dnl
> -ip,reg15=0x11,nw_src=10.0.0.4
> -ip,reg15=0x11,nw_src=10.0.0.5
> -ip,reg15=0x12,nw_src=10.0.0.4
> -ip,reg15=0x12,nw_src=10.0.0.5
> -ip,reg15=0x13,nw_src=10.0.0.4
> -ip,reg15=0x13,nw_src=10.0.0.5
> -])
> AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
> (no flows)
> ])
> @@ -725,27 +701,22 @@ reg15=0x11
> ])
> AT_CLEANUP
>
> -AT_SETUP([ovn -- converting expressions to flows -- no conjunction])
> -AT_KEYWORDS([no conjunction])
> +AT_SETUP([ovn -- converting expressions to flows -- conjunction])
> +AT_KEYWORDS([conjunction])
> expr_to_flow () {
> echo "$1" | ovstest test-ovn expr-to-flows | sort
> }
>
> -# conjunction is disabled in OVN until some of the issues
> -# related to conjunction flows are fixed.
> -# expr-to-flows should not generate any conjunction flows.
> lflow="ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3} && \
> ip4.dst == {20.0.0.1, 20.0.0.2, 20.0.0.3}"
> AT_CHECK([expr_to_flow "$lflow"], [0], [dnl
> -ip,nw_src=10.0.0.1,nw_dst=20.0.0.1
> -ip,nw_src=10.0.0.1,nw_dst=20.0.0.2
> -ip,nw_src=10.0.0.1,nw_dst=20.0.0.3
> -ip,nw_src=10.0.0.2,nw_dst=20.0.0.1
> -ip,nw_src=10.0.0.2,nw_dst=20.0.0.2
> -ip,nw_src=10.0.0.2,nw_dst=20.0.0.3
> -ip,nw_src=10.0.0.3,nw_dst=20.0.0.1
> -ip,nw_src=10.0.0.3,nw_dst=20.0.0.2
> -ip,nw_src=10.0.0.3,nw_dst=20.0.0.3
> +conj_id=1,ip
> +ip,nw_dst=20.0.0.1: conjunction(1, 0/2)
> +ip,nw_dst=20.0.0.2: conjunction(1, 0/2)
> +ip,nw_dst=20.0.0.3: conjunction(1, 0/2)
> +ip,nw_src=10.0.0.1: conjunc
[ovs-dev] [PATCH 1/3] Revert conjunctive match removal patches.
This partially reverts commits 6f914327a55aaab11507db0911b08d695e17ce2a
and 298701dbc99645700be41680a43d049cb061847a. This restores the behavior
of making conjunctive matches, and it restores the tests to their state.
The one thing it does not revert is the addition of the
force_cross_product variable in expr.c. This variable will be used in a
later commit in this series. Instead of removing it, we just set it
"false" to restore previous behavior.
With this commit, the conjunctive match misbehavior described in commit
298701dbc99645700be41680a43d049cb061847a is restored. However, this will
be fixed in an upcoming commit in this series.
Signed-off-by: Mark Michelson
---
TODO.rst | 10 -
lib/expr.c |9 +-
tests/ovn.at | 1250 +++---
3 files changed, 61 insertions(+), 1208 deletions(-)
diff --git a/TODO.rst b/TODO.rst
index ed55ea236..943d9bf81 100644
--- a/TODO.rst
+++ b/TODO.rst
@@ -145,13 +145,3 @@ OVN To-do List
* Support FTP ALGs.
* Support reject action.
-
-* Conjunction: Conjunction is disabled in OVN. This needs to be revisisted
- to enable conjunction again after addressing the issues related to it.
- Like, if there are multiple ACLs with overlapping Conjunction matches,
- conjunction flows are not added properly.
- Eg. match(ip4.src == {IP1, IP2, IP3} && ip4.dst == {IP4, IP5, IP6} &&
- tcp.dst >= 800 && tcp.dst <= 900) actions=drop
-
- match(ip4.src == {IP1, IP2, IP3} && ip4.dst == {IP4, IP5, IP6} &&
- tcp.dst >= 1000 && tcp.dst <= 2000) actions=allow
diff --git a/lib/expr.c b/lib/expr.c
index 9b9b6bcca..71de61543 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -42,13 +42,8 @@ VLOG_DEFINE_THIS_MODULE(expr);
*
* match 2 - ip4.src == {IP1, IP2} && tcp.dst >=700 && tcp.src <=800
* action - allow.
- *
- * To handle this issue temporarily force crossproduct so that conjunction
- * flows are not generated.
- *
- * Remove this once fixed.
- * */
-static bool force_crossproduct = true;
+ */
+static bool force_crossproduct = false;
static struct expr *parse_and_annotate(const char *s,
const struct shash *symtab,
diff --git a/tests/ovn.at b/tests/ovn.at
index 22b272a60..641a646fc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -589,22 +589,6 @@ ip,reg14=0x6
ipv6,reg14=0x5
ipv6,reg14=0x6
])
-AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp &&
tcp.dst == {500, 501}'], [0], [dnl
-tcp,reg14=0x5,tp_dst=500
-tcp,reg14=0x5,tp_dst=501
-tcp,reg14=0x6,tp_dst=500
-tcp,reg14=0x6,tp_dst=501
-])
-AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp &&
tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
-tcp,reg15=0x5,tp_src=400,tp_dst=500
-tcp,reg15=0x5,tp_src=400,tp_dst=501
-tcp,reg15=0x5,tp_src=401,tp_dst=500
-tcp,reg15=0x5,tp_src=401,tp_dst=501
-tcp,reg15=0x6,tp_src=400,tp_dst=500
-tcp,reg15=0x6,tp_src=400,tp_dst=501
-tcp,reg15=0x6,tp_src=401,tp_dst=500
-tcp,reg15=0x6,tp_src=401,tp_dst=501
-])
AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
(no flows)
])
@@ -709,14 +693,6 @@ reg15=0x11
reg15=0x12
reg15=0x13
])
-AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'],
[0], [dnl
-ip,reg15=0x11,nw_src=10.0.0.4
-ip,reg15=0x11,nw_src=10.0.0.5
-ip,reg15=0x12,nw_src=10.0.0.4
-ip,reg15=0x12,nw_src=10.0.0.5
-ip,reg15=0x13,nw_src=10.0.0.4
-ip,reg15=0x13,nw_src=10.0.0.5
-])
AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
(no flows)
])
@@ -725,27 +701,22 @@ reg15=0x11
])
AT_CLEANUP
-AT_SETUP([ovn -- converting expressions to flows -- no conjunction])
-AT_KEYWORDS([no conjunction])
+AT_SETUP([ovn -- converting expressions to flows -- conjunction])
+AT_KEYWORDS([conjunction])
expr_to_flow () {
echo "$1" | ovstest test-ovn expr-to-flows | sort
}
-# conjunction is disabled in OVN until some of the issues
-# related to conjunction flows are fixed.
-# expr-to-flows should not generate any conjunction flows.
lflow="ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3} && \
ip4.dst == {20.0.0.1, 20.0.0.2, 20.0.0.3}"
AT_CHECK([expr_to_flow "$lflow"], [0], [dnl
-ip,nw_src=10.0.0.1,nw_dst=20.0.0.1
-ip,nw_src=10.0.0.1,nw_dst=20.0.0.2
-ip,nw_src=10.0.0.1,nw_dst=20.0.0.3
-ip,nw_src=10.0.0.2,nw_dst=20.0.0.1
-ip,nw_src=10.0.0.2,nw_dst=20.0.0.2
-ip,nw_src=10.0.0.2,nw_dst=20.0.0.3
-ip,nw_src=10.0.0.3,nw_dst=20.0.0.1
-ip,nw_src=10.0.0.3,nw_dst=20.0.0.2
-ip,nw_src=10.0.0.3,nw_dst=20.0.0.3
+conj_id=1,ip
+ip,nw_dst=20.0.0.1: conjunction(1, 0/2)
+ip,nw_dst=20.0.0.2: conjunction(1, 0/2)
+ip,nw_dst=20.0.0.3: conjunction(1, 0/2)
+ip,nw_src=10.0.0.1: conjunction(1, 1/2)
+ip,nw_src=10.0.0.2: conjunction(1, 1/2)
+ip,nw_src=10.0.0.3: conjunction(1, 1/2)
])
lflow="ip && (!ct.est || (ct.est && ct_label.blocked == 1))"
@@ -759,12 +730,12 @@ ct_state=-est+trk,ipv6
lflow="ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3} && \
ip4.dst == {20.0.0.1, 20.0.0.2}"
AT_CHECK([expr_to_flow "$lflow"], [0], [d
Re: [ovs-dev] [PATCH 1/3] Revert conjunctive match removal patches.
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (TODO.rst). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Revert conjunctive match removal patches. The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
