| Message ID | 5f28990407f2684eebed421c4feea1f1d12ef127.1536495602.git.tredaelli@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Guru Shetty |
| Headers | show |
| Series | [ovs-dev] ovs-save: Don't always include the default flow during restore | expand |
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>
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.
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 >
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 >
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.)
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 > >
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?
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
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.
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 >
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
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(-)