Thank Mickey for your review. My comments are inlined.
On Tuesday 16 August 2016 09:59 PM, Mickey Spiegel wrote:
On Tue, Aug 16, 2016 at 3:55 AM, <bscha...@redhat.com
<mailto:bscha...@redhat.com>> wrote:
From: Babu Shanmugam <bscha...@redhat.com
<mailto:bscha...@redhat.com>>
ovn-northd sets 'ip.dscp' to the DSCP value
Signed-off-by: Babu Shanmugam <bscha...@redhat.com
<mailto:bscha...@redhat.com>>
---
ovn/lib/logical-fields.c | 2 +-
ovn/northd/ovn-northd.8.xml | 5 ++++
ovn/northd/ovn-northd.c | 13 ++++++++
ovn/ovn-nb.xml | 6 ++++
ovn/ovn-sb.xml | 5 ++++
tests/ovn.at <http://ovn.at> | 73
+++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 103 insertions(+), 1 deletion(-)
diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
index 6dbb4ae..068c000 100644
--- a/ovn/lib/logical-fields.c
+++ b/ovn/lib/logical-fields.c
@@ -134,7 +134,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 1cf6b6e..8f203b3 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -155,6 +155,11 @@
<ul>
<li>
+ Priority 100 flow to mark DSCP, only when the port
has a valid DSCP
+ setting
+ </li>
+
+ <li>
By adding a priority 100 flow that matches only on inport, you just
clobbered
the main port security functionality at priority 90 which restricts
traffic to certain
combinations of inport, MAC, and IP addresses. This really should be in a
separate pipeline stage where it does not affect other features.
DSCP marking is done at a different table. Priority 90 flows will be in
the next table. I agree my documentation is not proper. I will correct it.
In the long run, this should be more like ACL, allowing matches on
arbitrary
fields (not just inport), but with the action being set DSCP rather
than allow or drop.
Priority 90 flow to allow IPv4 traffic if it has IPv4 addresses
which match the <code>inport</code>, valid
<code>eth.src</code>
and valid <code>ip4.src</code> address(es).
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 91e1687..f6f9afe 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1739,6 +1739,7 @@ build_port_security_nd(struct ovn_port *op,
struct hmap *lflows)
*
* - If the port security has IPv4 addresses or IPv6 addresses
or both
* - Priority 80 flow to drop all IPv4 and IPv6 traffic
+ * - Priority 100 flow to set dscp, if DSCP setting is in
port options
*/
static void
build_port_security_ip(enum ovn_pipeline pipeline, struct
ovn_port *op,
@@ -1748,6 +1749,18 @@ build_port_security_ip(enum ovn_pipeline
pipeline, struct ovn_port *op,
enum ovn_stage stage;
if (pipeline == P_IN) {
port_direction = "inport";
+ const char *dscp = smap_get(&op->sb->options, "qos_dscp");
+ if (dscp) {
+ struct ds dscp_actions = DS_EMPTY_INITIALIZER;
+ struct ds dscp_match = DS_EMPTY_INITIALIZER;
+
+ ds_put_format(&dscp_match, "inport == %s", op->json_key);
+ ds_put_format(&dscp_actions, "ip.dscp = %s; next;",
dscp);
+ ovn_lflow_add(lflows, op->od,
S_SWITCH_IN_PORT_SEC_IP, 100,
+ ds_cstr(&dscp_match),
ds_cstr(&dscp_actions));
+ ds_destroy(&dscp_match);
+ ds_destroy(&dscp_actions);
+ }
stage = S_SWITCH_IN_PORT_SEC_IP;
} else {
port_direction = "outport";
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 76309f8..b56b304 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -291,6 +291,12 @@
If set, indicates the maximum burst size for data sent
from this
interface, in bits.
</column>
+
+ <column name="options" key="qos_dscp">
+ If set, indicates the DSCP code to be marked on the
packets ingressing
+ the switch from the VIF interface. Value should be in
the range of
+ 0 to 63 (inclusive).
+ </column>
</group>
</group>
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 0d5ddb4..36fce59 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1879,6 +1879,11 @@ tcp.flags = RST;
interface, in bits.
</column>
+ <column name="options" key="qos_dscp">
+ If set, indicates the DSCP code to be marked on the
packets egressing
+ the VIF interface. Value should be in the range of 0 to
63 (inclusive).
+ </column>
+
I don't see any code that does anything with this value.
Is your intent still to mark DSCP on egress, or only on ingress?
Is this just here because options get copied from NB to SB for nbsp of
type
other than router?
You are right. DSCP marking is done through the logical flow ('ip.dscp'
set action). qos_dscp option is just copied from NB to SB like all the
other options for a VIF port.
Mickey
<column name="options" key="qdisc_queue_id"
type='{"type": "integer", "minInteger": 1,
"maxInteger": 61440}'>
Indicates the queue number on the physical device. This
is same as the
diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
<http://ovn.at>
index a3aebd9..0e935ef 100644
--- a/tests/ovn.at <http://ovn.at>
+++ b/tests/ovn.at <http://ovn.at>
@@ -4813,3 +4813,76 @@ OVS_WAIT_UNTIL([
OVN_CLEANUP([hv1])
AT_CLEANUP
+
+AT_SETUP([ovn -- DSCP marking - check NB to SB DB transfer])
+AT_KEYWORDS([ovn])
+ovn_start
+
+# Configure the Northbound database
+ovn-nbctl ls-add lsw0
+
+ovn-nbctl lsp-add lsw0 lp1
+ovn-nbctl lsp-set-addresses lp1 "f0:00:00:00:00:01 1.1.1.1"
+
+ovn-nbctl set Logical_Switch_Port lp1 options:mark_dscp=34
+AT_CHECK([ovn-sbctl get Port_Binding lp1 options:mark_dscp], [0],
["34"
+])
+AT_CLEANUP
+
+AT_SETUP([ovn -- DSCP marking check])
+AT_KEYWORDS([ovn])
+ovn_start
+
+# Configure the Northbound database
+ovn-nbctl ls-add lsw0
+
+ovn-nbctl lsp-add lsw0 lp1
+ovn-nbctl lsp-add lsw0 lp2
+ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:01
+ovn-nbctl lsp-set-addresses lp2 f0:00:00:00:00:02
+ovn-nbctl lsp-set-port-security lp1 f0:00:00:00:00:01
+ovn-nbctl lsp-set-port-security lp2 f0:00:00:00:00:02
+net_add n1
+sim_add hv
+as hv
+ovs-vsctl add-br br-phys
+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
+
+# 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([grep "Final flow:" stdout], [0],[Final flow:
reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,dl_src=f0:00:00:00:00:01,dl_dst=f0:00:00:00:00:02,dl_type=0x0000
+])
+
+# 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([grep "Final flow:" stdout], [0],[Final flow:
ip,reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,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
+])
+
+# Mark DSCP with a valid value
+ovn-nbctl set Logical_Switch_Port lp1 options:qos_dscp=48
+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([grep "Final flow:" stdout], [0],[Final flow:
ip,reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,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=192,nw_ecn=0,nw_ttl=0
+])
+
+# Update the DSCP marking
+ovn-nbctl set Logical_Switch_Port lp1 options:qos_dscp=63
+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([grep "Final flow:" stdout], [0],[Final flow:
ip,reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,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=252,nw_ecn=0,nw_ttl=0
+])
+
+# Mark DSCP with invalid value
+ovn-nbctl set Logical_Switch_Port lp1 options:qos_dscp=64
+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([grep "Final flow:" stdout], [0],[Final flow:
ip,reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,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
+])
+
+# Disable DSCP marking
+ovn-nbctl clear Logical_Switch_Port lp1 options
+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([grep "Final flow:" stdout], [0],[Final flow:
ip,reg13=0x2,reg14=0x1,reg15=0x2,metadata=0x1,in_port=1,vlan_tci=0x0000,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
+])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
--
2.5.5
_______________________________________________
dev mailing list
dev@openvswitch.org <mailto:dev@openvswitch.org>
http://openvswitch.org/mailman/listinfo/dev
<http://openvswitch.org/mailman/listinfo/dev>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
