Re: [ovs-dev] [PACTCH v14] DSCP marking on packets

2016-10-05 Thread Ben Pfaff 
On Wed, Oct 05, 2016 at 01:57:47PM +0530, bscha...@redhat.com wrote:
> From: Babu Shanmugam 
> 
> This patch adds support for marking qos on IP packets based on arbitrary
> match criteria for a logical switch.
> 
> Signed-off-by: Babu Shanmugam 
> Suggested-by: Mickey Spiegel 
> Acked-by: Mickey Spiegel 

I figured out the problem I had with the test.  It was a race due to
lack of --wait on the ovn-nbctl calls.  I added --wait in the right
places and that fixed the problem.

While I was at it, I decided to improve the test by making it also check
with ovn-trace, since it's usually easier to understand failures from
ovn-trace than from physical packet traces.  I folded in the following
and applied this to master.

Thank you!

--8<--cut here-->8--

diff --git a/NEWS b/NEWS
index ee873d6..95cb2b2 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,7 @@ Post-v2.6.0
 -
- OVN:
  * QoS is now implemented via egress shaping rather than ingress policing.
+ * DSCP marking is now supported, via the new northbound QoS table.
- Fixed regression in table stats maintenance introduced in OVS
  2.3.0, wherein the number of OpenFlow table hits and misses was
  not accurate.
diff --git a/tests/ovn.at b/tests/ovn.at
index d9f9683..948716b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5367,9 +5367,7 @@ AT_SETUP([ovn -- DSCP marking check])
 AT_KEYWORDS([ovn])
 ovn_start
 
-# Configure the Northbound database
 ovn-nbctl ls-add lsw0
-
 ovn-nbctl --wait=sb lsp-add lsw0 lp1
 ovn-nbctl --wait=sb lsp-add lsw0 lp2
 ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:01
@@ -5385,44 +5383,66 @@ ovn_attach n1 br-phys 192.168.0.1
 ovs-vsctl add-port br-int vif1 -- set Interface vif1 external-ids:iface-id=lp1 
options:tx_pcap=vif1-tx.pcap options:rxq_pcap=vif1-rx.pcap ofport-request=1
 ovs-vsctl add-port br-int vif2 -- set Interface vif2 external-ids:iface-id=lp2 
options:tx_pcap=vif2-tx.pcap options:rxq_pcap=vif2-rx.pcap ofport-request=2
 
-# Extracts the final flow from ofproto/trace output,
-# removing the irrelevant MFF_LOG_CT_ZONE (reg13) value.
-get_final_flow() {
-sed -n "/Final flow:/s/reg13=[[^,]]*,//p" stdout
+AT_CAPTURE_FILE([trace])
+ovn_trace () {
+ovn-trace --all "$@" | tee trace | sed '1,/Minimal trace/d'
+}
+
+# Extracts nw_tos from the final flow from ofproto/trace output and prints
+# it on stdout.  Prints "none" if no nw_tos was included.
+get_final_nw_tos() {
+if flow=$(grep '^Final flow:' stdout); then :; else
+   # The output didn't have a final flow.
+   return 99
+fi
+
+tos=$(echo "$flow" | sed -n 's/.*nw_tos=\([[0-9]]\{1,\}\).*/\1/p')
+case $tos in
+'') echo none ;;
+   *) echo $tos ;;
+esac
+}
+
+# check_tos TOS
+#
+# Checks that a packet from 1.1.1.1 to 1.1.1.2 gets its DSCP set to TOS.
+check_tos() {
+# First check with ovn-trace for logical flows.
+echo "checking for tos $1"
+(if test $1 != 0; then echo "ip.dscp = $1;"; fi;
+ echo 'output("lp2");') > expout
+AT_CHECK_UNQUOTED([ovn_trace lsw0 'inport == "lp1" && eth.src == 
f0:00:00:00:00:01 && eth.dst == f0:00:00:00:00:02 && ip4.src == 1.1.1.1 && 
ip4.dst == 1.1.1.2'], [0], [expout])
+
+# Then re-check with ofproto/trace for a physical packet.
+AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x800,nw_src=1.1.1.1,nw_dst=1.1.1.2'],
 [0], [stdout-nolog])
+AT_CHECK_UNQUOTED([get_final_nw_tos], [0], [`expr $1 \* 4`
+])
 }
 
 # check at L2
-AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02' -generate], [0], 
[stdout])
-AT_CHECK([get_final_flow], [0],[Final flow: 
reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x
+AT_CHECK([ovn_trace lsw0 'inport == "lp1" && eth.src == f0:00:00:00:00:01 && 
eth.dst == f0:00:00:00:00:02'], [0], [output("lp2");
+])
+AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02'], [0], 
[stdout-nolog])
+AT_CHECK([get_final_nw_tos], [0], [none
 ])
 
 # check at L3 without dscp marking
-AT_CHECK([ovs-appctl ofproto/trace br-int 
'in_port=1,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x800,nw_src=1.1.1.1,nw_dst=1.1.1.2'
 -generate], [0], [stdout])
-AT_CHECK([get_final_flow], [0],[Final flow: 
ip,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,nw_src=1.1.1.1,nw_dst=1.1.1.2,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
-])
+check_tos 0
 
 # Mark DSCP with a valid value
-qos_id=$(ovn-nbctl -- --id=@lp1-qos create QoS priority=100 action=dscp=48 
match="inport\=\=\"lp1\"" direction="from-lport" -- set Logical_Switch lsw0 
qos_rules=@lp1-qos)
-AT_CHECK([ovs-appctl ofproto/trace br-int

[ovs-dev] [PACTCH v14] DSCP marking on packets

2016-10-05 Thread bschanmu 
From: Babu Shanmugam 

This patch adds support for marking qos on IP packets based on arbitrary
match criteria for a logical switch.

Signed-off-by: Babu Shanmugam 
Suggested-by: Mickey Spiegel 
Acked-by: Mickey Spiegel 
---
 ovn/lib/logical-fields.c|  2 +-
 ovn/northd/ovn-northd.8.xml | 47 --
 ovn/northd/ovn-northd.c | 80 ++---
 ovn/ovn-nb.ovsschema| 26 +--
 ovn/ovn-nb.xml  | 57 
 ovn/utilities/ovn-nbctl.c   |  5 +++
 tests/ovn.at| 66 -
 7 files changed, 242 insertions(+), 41 deletions(-)

diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
index 5229be3..2d3e217 100644
--- a/ovn/lib/logical-fields.c
+++ b/ovn/lib/logical-fields.c
@@ -137,7 +137,7 @@ ovn_init_symtab(struct shash *symtab)
 expr_symtab_add_predicate(symtab, "ip6", "eth.type == 0x86dd");
 expr_symtab_add_predicate(symtab, "ip", "ip4 || ip6");
 expr_symtab_add_field(symtab, "ip.proto", MFF_IP_PROTO, "ip", true);
-expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP, "ip", false);
+expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP_SHIFTED, "ip", false);
 expr_symtab_add_field(symtab, "ip.ecn", MFF_IP_ECN, "ip", false);
 expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
 
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 77eb3d1..df53d4c 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -362,7 +362,27 @@
   
 
 
-Ingress Table 7: LB
+Ingress Table 7: from-lport QoS marking
+
+
+  Logical flows in this table closely reproduce those in the
+  QoS table in the OVN_Northbound database
+  for the from-lport direction.
+
+
+
+  
+For every qos_rules for every logical switch a flow will be added at
+priorities mentioned in the QoS table.
+  
+
+  
+One priority-0 fallback flow that matches all packets and advances to
+the next table.
+  
+
+
+Ingress Table 8: LB
 
 
   It contains a priority-0 flow that simply moves traffic to the next
@@ -375,7 +395,7 @@
   connection.)
 
 
-Ingress Table 8: Stateful
+Ingress Table 9: Stateful
 
 
   
@@ -412,7 +432,7 @@
   
 
 
-Ingress Table 9: ARP/ND responder
+Ingress Table 10: ARP/ND responder
 
 
   This table implements ARP/ND responder for known IPs.  It contains these
@@ -507,7 +527,7 @@ nd_na {
   
 
 
-Ingress Table 10: DHCP option processing
+Ingress Table 11: DHCP option processing
 
 
   This table adds the DHCPv4 options to a DHCPv4 packet from the
@@ -567,7 +587,7 @@ next;
   
 
 
-Ingress Table 11: DHCP responses
+Ingress Table 12: DHCP responses
 
 
   This table implements DHCP responder for the DHCP replies generated by
@@ -649,7 +669,7 @@ output;
   
 
 
-Ingress Table 12: Destination Lookup
+Ingress Table 13 Destination Lookup
 
 
   This table implements switching behavior.  It contains these logical
@@ -716,7 +736,14 @@ output;
   to-lport ACLs.
 
 
-Egress Table 5: Stateful
+Egress Table 5: to-lport QoS marking
+
+
+  This is similar to ingress table QoS marking except for
+  to-lport qos rules.
+
+
+Egress Table 6: Stateful
 
 
   This is similar to ingress table Stateful except that
@@ -727,10 +754,10 @@ output;
   Also a priority 34000 logical flow is added for each logical port which
   has DHCPv4 options defined to allow the DHCPv4 reply packet and which has
   DHCPv6 options defined to allow the DHCPv6 reply packet from the
-  Ingress Table 11: DHCP responses.
+  Ingress Table 12: DHCP responses.
 
 
-Egress Table 6: Egress Port Security - IP
+Egress Table 7: Egress Port Security - IP
 
 
   This is similar to the port security logic in table
@@ -740,7 +767,7 @@ output;
   ip4.src and ip6.src
 
 
-Egress Table 7: Egress Port Security - L2
+Egress Table 8: Egress Port Security - L2
 
 
   This is similar to the ingress port security logic in ingress table
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 0150a8c..ad4e38d 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -96,21 +96,22 @@ enum ovn_datapath_type {
  * form the stage's full name, e.g. S_SWITCH_IN_PORT_SEC_L2,
  * S_ROUTER_OUT_DELIVERY. */
 enum ovn_stage {
-#define PIPELINE_STAGES   \
-/* Logical switch ingress stages. */  \
-PIPELINE_STAGE(SWITCH, IN,  PORT_SEC_L2,0, "ls_in_port_sec_l2") \
-PIPELINE_STAGE(SWITCH, IN,  PORT_SEC_IP,1, "ls_in_port_sec_ip") \
-