[ovs-dev] ovs-save: Don't always include the default flow during restore

Message ID 5f28990407f2684eebed421c4feea1f1d12ef127.1536495602.git.tredaelli@redhat.com
State New
Delegated to: Guru Shetty
Headers show
Series
  • [ovs-dev] ovs-save: Don't always include the default flow during restore
Related show

Commit Message

Timothy Redaelli Sept. 9, 2018, 12:20 p.m.
Currently the default flow (actions=NORMAL) is present in the flow table after
the flow table is restored also when the default flow is removed.

This commit changes the behaviour of the "ovs-save save-flows" command to use
"replace-flows" instead of "add-flows" to restore the flows. This is needed in
order to always have the new flow table as it was before restoring it.

Reported-by: Flavio Leitner <fbl@sysclose.org>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
---
 utilities/ovs-save | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Flavio Leitner Sept. 11, 2018, 4:06 p.m. | #1
On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> Currently the default flow (actions=NORMAL) is present in the flow table after
> the flow table is restored also when the default flow is removed.
> 
> This commit changes the behaviour of the "ovs-save save-flows" command to use
> "replace-flows" instead of "add-flows" to restore the flows. This is needed in
> order to always have the new flow table as it was before restoring it.
> 
> Reported-by: Flavio Leitner <fbl@sysclose.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> ---

From the performance point of view, I thought we could do a
del-flows first followed by add-flows, but you mentioned off
list that the performance with replace-flows is still good.

Patch looks good and works for me, thanks Timothy!

Acked-by: Flavio Leitner <fbl@sysclose.org>
Flavio Leitner Sept. 11, 2018, 5:01 p.m. | #2
On Tue, Sep 11, 2018 at 01:06:48PM -0300, Flavio Leitner wrote:
> On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> > Currently the default flow (actions=NORMAL) is present in the flow table after
> > the flow table is restored also when the default flow is removed.
> > 
> > This commit changes the behaviour of the "ovs-save save-flows" command to use
> > "replace-flows" instead of "add-flows" to restore the flows. This is needed in
> > order to always have the new flow table as it was before restoring it.
> > 
> > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > ---
> 
> From the performance point of view, I thought we could do a
> del-flows first followed by add-flows, but you mentioned off
> list that the performance with replace-flows is still good.
> 
> Patch looks good and works for me, thanks Timothy!
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>

BTW, we need this on master, 2.10 and 2.9.
Guru Shetty Sept. 12, 2018, 5:36 p.m. | #3
On Tue, 11 Sep 2018 at 09:07, Flavio Leitner <fbl@sysclose.org> wrote:

> On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> > Currently the default flow (actions=NORMAL) is present in the flow table
> after
> > the flow table is restored also when the default flow is removed.
> >
> > This commit changes the behaviour of the "ovs-save save-flows" command
> to use
> > "replace-flows" instead of "add-flows" to restore the flows. This is
> needed in
> > order to always have the new flow table as it was before restoring it.
> >
> > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > ---
>
> From the performance point of view, I thought we could do a
> del-flows first followed by add-flows, but you mentioned off
> list that the performance with replace-flows is still good.
>

Can you please provide some data. For e.g., with 200,000 flows.


>
> Patch looks good and works for me, thanks Timothy!
>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Guru Shetty Sept. 12, 2018, 5:54 p.m. | #4
On Sun, 9 Sep 2018 at 05:20, Timothy Redaelli <tredaelli@redhat.com> wrote:

> Currently the default flow (actions=NORMAL) is present in the flow table
> after
> the flow table is restored also when the default flow is removed.
>
> This commit changes the behaviour of the "ovs-save save-flows" command to
> use
> "replace-flows" instead of "add-flows" to restore the flows. This is
> needed in
> order to always have the new flow table as it was before restoring it.
>

Also, have you considered setting the bridge as "secure" to avoid adding
NORMAL flow?
Or is there is a use case to have it not be "secure"?


>
> Reported-by: Flavio Leitner <fbl@sysclose.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> ---
>  utilities/ovs-save | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utilities/ovs-save b/utilities/ovs-save
> index ea8fb6a45..2294583d6 100755
> --- a/utilities/ovs-save
> +++ b/utilities/ovs-save
> @@ -121,7 +121,7 @@ save_flows () {
>               cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
>          echo "'"
>
> -        printf "%s" "ovs-ofctl -O $ofp_version add-flows ${bridge} " \
> +        printf "%s" "ovs-ofctl -O $ofp_version replace-flows ${bridge} " \
>              "\"$workdir/$bridge.flows.dump\""
>
>          # If possible, use OpenFlow 1.4 atomic bundle transaction to add
> flows
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Sept. 12, 2018, 9:35 p.m. | #5
On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> Currently the default flow (actions=NORMAL) is present in the flow table after
> the flow table is restored also when the default flow is removed.
> 
> This commit changes the behaviour of the "ovs-save save-flows" command to use
> "replace-flows" instead of "add-flows" to restore the flows. This is needed in
> order to always have the new flow table as it was before restoring it.
> 
> Reported-by: Flavio Leitner <fbl@sysclose.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>

Seems reasonable to me.  (I'll leave this one to Guru for shepherding.)
Flavio Leitner Sept. 12, 2018, 10:36 p.m. | #6
On Wed, Sep 12, 2018 at 10:54:36AM -0700, Guru Shetty wrote:
> On Sun, 9 Sep 2018 at 05:20, Timothy Redaelli <tredaelli@redhat.com> wrote:
> 
> > Currently the default flow (actions=NORMAL) is present in the flow table
> > after
> > the flow table is restored also when the default flow is removed.
> >
> > This commit changes the behaviour of the "ovs-save save-flows" command to
> > use
> > "replace-flows" instead of "add-flows" to restore the flows. This is
> > needed in
> > order to always have the new flow table as it was before restoring it.
> >
> 
> Also, have you considered setting the bridge as "secure" to avoid adding
> NORMAL flow?
> Or is there is a use case to have it not be "secure"?

The issue is that there is no NORMAL flow when the flow table
is dumped. But then when ovs starts again, it is there by default
even after restoring the original flow table.

In the end you have the previous flow table plus the normal flow
which is unexpected.

The patch fix to restore the flow table as it was dumped.
HTH,
fbl


> 
> 
> >
> > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > ---
> >  utilities/ovs-save | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/utilities/ovs-save b/utilities/ovs-save
> > index ea8fb6a45..2294583d6 100755
> > --- a/utilities/ovs-save
> > +++ b/utilities/ovs-save
> > @@ -121,7 +121,7 @@ save_flows () {
> >               cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
> >          echo "'"
> >
> > -        printf "%s" "ovs-ofctl -O $ofp_version add-flows ${bridge} " \
> > +        printf "%s" "ovs-ofctl -O $ofp_version replace-flows ${bridge} " \
> >              "\"$workdir/$bridge.flows.dump\""
> >
> >          # If possible, use OpenFlow 1.4 atomic bundle transaction to add
> > flows
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
Flavio Leitner Sept. 12, 2018, 10:37 p.m. | #7
On Wed, Sep 12, 2018 at 10:36:22AM -0700, Guru Shetty wrote:
> On Tue, 11 Sep 2018 at 09:07, Flavio Leitner <fbl@sysclose.org> wrote:
> 
> > On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> > > Currently the default flow (actions=NORMAL) is present in the flow table
> > after
> > > the flow table is restored also when the default flow is removed.
> > >
> > > This commit changes the behaviour of the "ovs-save save-flows" command
> > to use
> > > "replace-flows" instead of "add-flows" to restore the flows. This is
> > needed in
> > > order to always have the new flow table as it was before restoring it.
> > >
> > > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > > ---
> >
> > From the performance point of view, I thought we could do a
> > del-flows first followed by add-flows, but you mentioned off
> > list that the performance with replace-flows is still good.
> >
> 
> Can you please provide some data. For e.g., with 200,000 flows.

Timothy, could you please share?
Timothy Redaelli Sept. 13, 2018, 5:38 p.m. | #8
On Wed, 12 Sep 2018 10:36:22 -0700
Guru Shetty <guru@ovn.org> wrote:

> On Tue, 11 Sep 2018 at 09:07, Flavio Leitner <fbl@sysclose.org> wrote:
> 
> > On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:  
> > > Currently the default flow (actions=NORMAL) is present in the
> > > flow table  
> > after  
> > > the flow table is restored also when the default flow is removed.
> > >
> > > This commit changes the behaviour of the "ovs-save save-flows"
> > > command  
> > to use  
> > > "replace-flows" instead of "add-flows" to restore the flows. This
> > > is  
> > needed in  
> > > order to always have the new flow table as it was before
> > > restoring it.
> > >
> > > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > > ---  
> >
> > From the performance point of view, I thought we could do a
> > del-flows first followed by add-flows, but you mentioned off
> > list that the performance with replace-flows is still good.
> >  
> 
> Can you please provide some data. For e.g., with 200,000 flows.

Hi,
my usual setup to test this scenario is:

VM1 (tap0) <-> OVS (ovsbr0) <-> VM2 (tap1).

In this case I'm testing it by using kernel datapath.

I add 1 milion of rules like
"table=0, dl_src=$MAC, actions=drop" with different mac addresses,
using a script and the 2 rules in order to make the VM pings:
table=0,in_port=1,action=output:2
table=0,in_port=2,action=output:1

I launch a ping (1 second for each ping) from VM1 to VM2 IP, then I
reload openvswitch on the host and I have the following downtime (lost
ICMP packets):

with add-flows (aka without the patch):  11 seconds
with replace-flows (aka with the patch): 11 seconds too
Guru Shetty Sept. 13, 2018, 5:40 p.m. | #9
On Thu, 13 Sep 2018 at 10:38, Timothy Redaelli <tredaelli@redhat.com> wrote:

> On Wed, 12 Sep 2018 10:36:22 -0700
> Guru Shetty <guru@ovn.org> wrote:
>
> > On Tue, 11 Sep 2018 at 09:07, Flavio Leitner <fbl@sysclose.org> wrote:
> >
> > > On Sun, Sep 09, 2018 at 02:20:02PM +0200, Timothy Redaelli wrote:
> > > > Currently the default flow (actions=NORMAL) is present in the
> > > > flow table
> > > after
> > > > the flow table is restored also when the default flow is removed.
> > > >
> > > > This commit changes the behaviour of the "ovs-save save-flows"
> > > > command
> > > to use
> > > > "replace-flows" instead of "add-flows" to restore the flows. This
> > > > is
> > > needed in
> > > > order to always have the new flow table as it was before
> > > > restoring it.
> > > >
> > > > Reported-by: Flavio Leitner <fbl@sysclose.org>
> > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> > > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> > > > ---
> > >
> > > From the performance point of view, I thought we could do a
> > > del-flows first followed by add-flows, but you mentioned off
> > > list that the performance with replace-flows is still good.
> > >
> >
> > Can you please provide some data. For e.g., with 200,000 flows.
>
> Hi,
> my usual setup to test this scenario is:
>
> VM1 (tap0) <-> OVS (ovsbr0) <-> VM2 (tap1).
>
> In this case I'm testing it by using kernel datapath.
>
> I add 1 milion of rules like
> "table=0, dl_src=$MAC, actions=drop" with different mac addresses,
> using a script and the 2 rules in order to make the VM pings:
> table=0,in_port=1,action=output:2
> table=0,in_port=2,action=output:1
>
> I launch a ping (1 second for each ping) from VM1 to VM2 IP, then I
> reload openvswitch on the host and I have the following downtime (lost
> ICMP packets):
>
> with add-flows (aka without the patch):  11 seconds
> with replace-flows (aka with the patch): 11 seconds too
>
Thats good enough for me. I wanted to make sure that there is no penalty
here for large flows.
Guru Shetty Sept. 13, 2018, 11:18 p.m. | #10
On Sun, 9 Sep 2018 at 05:20, Timothy Redaelli <tredaelli@redhat.com> wrote:

> Currently the default flow (actions=NORMAL) is present in the flow table
> after
> the flow table is restored also when the default flow is removed.
>
> This commit changes the behaviour of the "ovs-save save-flows" command to
> use
> "replace-flows" instead of "add-flows" to restore the flows. This is
> needed in
> order to always have the new flow table as it was before restoring it.
>
> Reported-by: Flavio Leitner <fbl@sysclose.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1626096
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
>

I applied this to master, 2.10 and 2.9


> ---
>  utilities/ovs-save | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utilities/ovs-save b/utilities/ovs-save
> index ea8fb6a45..2294583d6 100755
> --- a/utilities/ovs-save
> +++ b/utilities/ovs-save
> @@ -121,7 +121,7 @@ save_flows () {
>               cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
>          echo "'"
>
> -        printf "%s" "ovs-ofctl -O $ofp_version add-flows ${bridge} " \
> +        printf "%s" "ovs-ofctl -O $ofp_version replace-flows ${bridge} " \
>              "\"$workdir/$bridge.flows.dump\""
>
>          # If possible, use OpenFlow 1.4 atomic bundle transaction to add
> flows
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch

diff --git a/utilities/ovs-save b/utilities/ovs-save
index ea8fb6a45..2294583d6 100755
--- a/utilities/ovs-save
+++ b/utilities/ovs-save
@@ -121,7 +121,7 @@  save_flows () {
              cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
         echo "'"
 
-        printf "%s" "ovs-ofctl -O $ofp_version add-flows ${bridge} " \
+        printf "%s" "ovs-ofctl -O $ofp_version replace-flows ${bridge} " \
             "\"$workdir/$bridge.flows.dump\""
 
         # If possible, use OpenFlow 1.4 atomic bundle transaction to add flows