[ovs-dev] ovs-save: Use --bundle to restore flows (on OpenFlow 1.4+)

Message ID c8f0ef83bde3e421c21618c2ae794a0e41e85ba4.1504867984.git.tredaelli@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] ovs-save: Use --bundle to restore flows (on OpenFlow 1.4+)
Related show

Commit Message

Timothy Redaelli Sept. 8, 2017, 11:03 a.m.
If possible, use OpenFlow 1.4 atomic bundle transaction to restore flows.
The patch uses also the highest enabled OpenFlow version to do the queries.

With the actual implementation if you have the default openflow version
disabled ovs-save fails, this patch also fixes that.

Replaced "echo -n" with "printf" since "echo -n" is not POSIX and may
not work with some shells.

Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
---
This is a pre-requisite for the new patchset of patch with patchwok id 799223
---
 utilities/ovs-save | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

Comments

Flavio Leitner Sept. 22, 2017, 11:09 p.m. | #1
On Fri,  8 Sep 2017 13:03:48 +0200
Timothy Redaelli <tredaelli@redhat.com> wrote:

> If possible, use OpenFlow 1.4 atomic bundle transaction to restore flows.
> The patch uses also the enabled OpenFlow version to do the queries.
> 
> With the actual implementation if you have the default openflow version
> disabled ovs-save fails, this patch also fixes that.
> 
> Replaced "echo -n" with "printf" since "echo -n" is not POSIX and may
> not work with some shells.
> 
> Signed-off-by: Timothy Redaelli <tredaelli@redhat.com>
> ---
> This is a pre-requisite for the new patchset of patch with patchwok id 799223
> ---
>  utilities/ovs-save | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/utilities/ovs-save b/utilities/ovs-save
> index 8b8dbf421..3cfa4c870 100755
> --- a/utilities/ovs-save
> +++ b/utilities/ovs-save
> @@ -99,6 +99,11 @@ save_interfaces () {
>      fi
>  }
>  
> +get_highest_ofp_version() {
> +    ovs-vsctl get bridge "$1" protocols | \
> +        awk -F '"' '{ print (NF>1)? $(NF-1) : "OpenFlow14" }'
> +}
> +
>  save_flows () {
>      if (ovs-ofctl --version) > /dev/null 2>&1; then :; else
>          echo "$0: ovs-ofctl not found in $PATH" >&2
> @@ -106,16 +111,26 @@ save_flows () {
>      fi
>  
>      for bridge in "$@"; do
> -        echo -n "ovs-ofctl add-tlv-map ${bridge} '"
> -        ovs-ofctl dump-tlv-map ${bridge} | \
> +        # Get the highest enabled OpenFlow version
> +        ofp_version=$(get_highest_ofp_version "$bridge")
> +
> +        printf "ovs-ofctl -O $ofp_version add-tlv-map %s '" "$bridge"
> +        ovs-ofctl -O $ofp_version dump-tlv-map $bridge | \
>          awk '/^ 0x/ {if (cnt != 0) printf ","; \
>               cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
>          echo "'"
>  
> -        echo "ovs-ofctl add-flows ${bridge} - << EOF"
> -        ovs-ofctl dump-flows "${bridge}" | sed -e '/NXST_FLOW/d' \
> -            -e 's/\(idle\|hard\)_age=[^,]*,//g'
> -        echo "EOF"
> +        printf "%s" "ovs-ofctl -O $ofp_version add-flows $bridge \
> +            \"$workdir/$bridge.flows.dump\""


Where is $workdir set?  Looks like it's empty and will use root fs.

Although this is a small patch, it actually does two things. One is
fixing to use highest enabled OF and another is replacing echo with
printf.

I suggest to split them apart so it's easier to review.

Thanks,
fbl

> +
> +        # If possible, use OpenFlow 1.4 atomic bundle transaction to add flows
> +        [ ${ofp_version#OpenFlow} -ge 14 ] && echo " --bundle"
> +
> +        ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \
> +            sed -e '/NXST_FLOW/d' \
> +                -e '/OFPST_FLOW/d' \
> +                -e 's/\(idle\|hard\)_age=[^,]*,//g' \
> +                > "$workdir/$bridge.flows.dump"
>      done
>  }
>
Timothy Redaelli Sept. 25, 2017, 3:21 p.m. | #2
On 09/23/2017 01:09 AM, Flavio Leitner wrote:
[...]
> 
> Where is $workdir set?  Looks like it's empty and will use root fs.
> 
> Although this is a small patch, it actually does two things. One is
> fixing to use highest enabled OF and another is replacing echo with
> printf.
> 
> I suggest to split them apart so it's easier to review.

All done, thank you.

https://patchwork.ozlabs.org/project/openvswitch/list/?series=4973

Patch

diff --git a/utilities/ovs-save b/utilities/ovs-save
index 8b8dbf421..3cfa4c870 100755
--- a/utilities/ovs-save
+++ b/utilities/ovs-save
@@ -99,6 +99,11 @@  save_interfaces () {
     fi
 }
 
+get_highest_ofp_version() {
+    ovs-vsctl get bridge "$1" protocols | \
+        awk -F '"' '{ print (NF>1)? $(NF-1) : "OpenFlow14" }'
+}
+
 save_flows () {
     if (ovs-ofctl --version) > /dev/null 2>&1; then :; else
         echo "$0: ovs-ofctl not found in $PATH" >&2
@@ -106,16 +111,26 @@  save_flows () {
     fi
 
     for bridge in "$@"; do
-        echo -n "ovs-ofctl add-tlv-map ${bridge} '"
-        ovs-ofctl dump-tlv-map ${bridge} | \
+        # Get the highest enabled OpenFlow version
+        ofp_version=$(get_highest_ofp_version "$bridge")
+
+        printf "ovs-ofctl -O $ofp_version add-tlv-map %s '" "$bridge"
+        ovs-ofctl -O $ofp_version dump-tlv-map $bridge | \
         awk '/^ 0x/ {if (cnt != 0) printf ","; \
              cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
         echo "'"
 
-        echo "ovs-ofctl add-flows ${bridge} - << EOF"
-        ovs-ofctl dump-flows "${bridge}" | sed -e '/NXST_FLOW/d' \
-            -e 's/\(idle\|hard\)_age=[^,]*,//g'
-        echo "EOF"
+        printf "%s" "ovs-ofctl -O $ofp_version add-flows $bridge \
+            \"$workdir/$bridge.flows.dump\""
+
+        # If possible, use OpenFlow 1.4 atomic bundle transaction to add flows
+        [ ${ofp_version#OpenFlow} -ge 14 ] && echo " --bundle"
+
+        ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \
+            sed -e '/NXST_FLOW/d' \
+                -e '/OFPST_FLOW/d' \
+                -e 's/\(idle\|hard\)_age=[^,]*,//g' \
+                > "$workdir/$bridge.flows.dump"
     done
 }