Re: [ovs-dev] [PATCH ovn] ofctrl: Do not link a desired flow twice.
On 2/17/21 5:24 PM, Mark Gray wrote:
On 16/02/2021 15:53, Dumitru Ceara wrote:
Depending on the logical flow matches, multiple SB flows can point to
the same desired flow. If it happens that the desired flow conflicts
with a more restrictive (already installed) flow, then we shouldn't try
to add the desired flow multiple times to the list maintained for the
installed flow.
Fixes: 6f0b1e02d9ab ("ofctrl: Incremental processing for flow installation by
tracking.")
Signed-off-by: Dumitru Ceara
---
Note: I'm quite sure about the "Fixes" tag above, however, I couldn't
reproduce the issue without the following commit:
dadae4f800cc ("ofctrl.c: Only merge actions for conjunctive flows.")
---
controller/ofctrl.c | 3 +--
tests/ovn.at| 19 +++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 9d62e12..f2bb93a 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1975,8 +1975,7 @@ update_installed_flows_by_track(struct
ovn_desired_flow_table *flow_table,
/* The installed flow is installed for f, but f has change
* tracked, so it must have been modified. */
installed_flow_mod(&i->flow, &f->flow, msgs);
-ovn_flow_log(&i->flow, "updating installed (tracked)");
Why did you delete this log message?
Oops, completely by accident.
Thanks for spotting this!
-} else {> +} else if (!f->installed_flow) {
/* Adding a new flow that conflicts with an existing installed
* flow, so add it to the link. If this flow becomes active,
* e.g., it is less restrictive than the previous active flow
diff --git a/tests/ovn.at b/tests/ovn.at
index 55ea6d1..933958e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13955,6 +13955,25 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45
| ofctl_strip_all | \
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
actions=conjunction()
])
+# Add another ACL that overlaps with the existing less restrictive ones.
+check ovn-nbctl acl-add ls1 to-lport 3 'udp || ((ip4.src==10.0.0.1 ||
ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4))' allow
+check ovn-nbctl --wait=hv sync
+
Please add a comment here it will help explain what is expected and
follows the pattern of other tests.
Sure, will do in v2.
Thanks for the review!
Regards,
Dumitru
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
+ grep "priority=1003" | \
+ sed 's/conjunction([[^)]]*)/conjunction()/g' | sort], [0], [dnl
+ table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,conj_id=4,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(),conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(),conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction()
+ table=45, priority=1003,udp,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,udp6,metadata=0x1 actions=resubmit(,46)
+])
+
OVN_CLEANUP([hv1])
AT_CLEANUP
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ofctrl: Do not link a desired flow twice.
On 16/02/2021 15:53, Dumitru Ceara wrote:
> Depending on the logical flow matches, multiple SB flows can point to
> the same desired flow. If it happens that the desired flow conflicts
> with a more restrictive (already installed) flow, then we shouldn't try
> to add the desired flow multiple times to the list maintained for the
> installed flow.
>
> Fixes: 6f0b1e02d9ab ("ofctrl: Incremental processing for flow installation by
> tracking.")
> Signed-off-by: Dumitru Ceara
> ---
> Note: I'm quite sure about the "Fixes" tag above, however, I couldn't
> reproduce the issue without the following commit:
> dadae4f800cc ("ofctrl.c: Only merge actions for conjunctive flows.")
> ---
> controller/ofctrl.c | 3 +--
> tests/ovn.at| 19 +++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 9d62e12..f2bb93a 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1975,8 +1975,7 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
> /* The installed flow is installed for f, but f has change
> * tracked, so it must have been modified. */
> installed_flow_mod(&i->flow, &f->flow, msgs);
> -ovn_flow_log(&i->flow, "updating installed (tracked)");
Why did you delete this log message?
> -} else {> +} else if (!f->installed_flow) {
> /* Adding a new flow that conflicts with an existing
> installed
> * flow, so add it to the link. If this flow becomes active,
> * e.g., it is less restrictive than the previous active flow
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 55ea6d1..933958e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13955,6 +13955,25 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
> table=45 | ofctl_strip_all | \
> table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
> actions=conjunction()
> ])
>
> +# Add another ACL that overlaps with the existing less restrictive ones.
> +check ovn-nbctl acl-add ls1 to-lport 3 'udp || ((ip4.src==10.0.0.1 ||
> ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4))' allow
> +check ovn-nbctl --wait=hv sync
> +
Please add a comment here it will help explain what is expected and
follows the pattern of other tests.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
> + grep "priority=1003" | \
> + sed 's/conjunction([[^)]]*)/conjunction()/g' | sort], [0], [dnl
> + table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> + table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> + table=45, priority=1003,conj_id=4,ip,metadata=0x1 actions=resubmit(,46)
> + table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
> actions=conjunction(),conjunction(),conjunction()
> + table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(),conjunction(),conjunction()
> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1
> actions=resubmit(,46)
> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
> actions=conjunction(),conjunction()
> + table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42
> actions=conjunction()
> + table=45, priority=1003,udp,metadata=0x1 actions=resubmit(,46)
> + table=45, priority=1003,udp6,metadata=0x1 actions=resubmit(,46)
> +])
> +
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ofctrl: Do not link a desired flow twice.
Depending on the logical flow matches, multiple SB flows can point to
the same desired flow. If it happens that the desired flow conflicts
with a more restrictive (already installed) flow, then we shouldn't try
to add the desired flow multiple times to the list maintained for the
installed flow.
Fixes: 6f0b1e02d9ab ("ofctrl: Incremental processing for flow installation by
tracking.")
Signed-off-by: Dumitru Ceara
---
Note: I'm quite sure about the "Fixes" tag above, however, I couldn't
reproduce the issue without the following commit:
dadae4f800cc ("ofctrl.c: Only merge actions for conjunctive flows.")
---
controller/ofctrl.c | 3 +--
tests/ovn.at| 19 +++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 9d62e12..f2bb93a 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1975,8 +1975,7 @@ update_installed_flows_by_track(struct
ovn_desired_flow_table *flow_table,
/* The installed flow is installed for f, but f has change
* tracked, so it must have been modified. */
installed_flow_mod(&i->flow, &f->flow, msgs);
-ovn_flow_log(&i->flow, "updating installed (tracked)");
-} else {
+} else if (!f->installed_flow) {
/* Adding a new flow that conflicts with an existing installed
* flow, so add it to the link. If this flow becomes active,
* e.g., it is less restrictive than the previous active flow
diff --git a/tests/ovn.at b/tests/ovn.at
index 55ea6d1..933958e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -13955,6 +13955,25 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45
| ofctl_strip_all | \
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction()
])
+# Add another ACL that overlaps with the existing less restrictive ones.
+check ovn-nbctl acl-add ls1 to-lport 3 'udp || ((ip4.src==10.0.0.1 ||
ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4))' allow
+check ovn-nbctl --wait=hv sync
+
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
+ grep "priority=1003" | \
+ sed 's/conjunction([[^)]]*)/conjunction()/g' | sort], [0], [dnl
+ table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,conj_id=4,ip,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3
actions=conjunction(),conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4
actions=conjunction(),conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46)
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2
actions=conjunction(),conjunction()
+ table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction()
+ table=45, priority=1003,udp,metadata=0x1 actions=resubmit(,46)
+ table=45, priority=1003,udp6,metadata=0x1 actions=resubmit(,46)
+])
+
OVN_CLEANUP([hv1])
AT_CLEANUP
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
