On Wed, Aug 31, 2016 at 12:11 AM, <bscha...@redhat.com
<mailto:bscha...@redhat.com>> wrote:
ovn-northd sets 'ip.dscp' to the DSCP value
If we were to go with DSCP based on port as the initial functionality,
your changes look good. A couple of nits below, and the first patch
(which I have not looked at) needs a rebase after the removal of
incremental processing.
However, I think we should be more ambitious and support arbitrary
match criteria. I am always worried about the migration impact when
moving from one way of specifying a feature in NB schema (in this
case, port options "qos_dscp") to something completely different (see
below) later on.
During the OVN meeting this morning, there was a preference for
creating separate tables for each feature such as ACLs, QoS marking,
SFC insertion, rather than overloading ACLs. It was noted that tables
are fairly cheap, and each one can be customized to the purpose.
So I am proposing my earlier suggestion for ovn/ovn-nb.ovsschema once
again:
@@ -26,6 +26,11 @@
"refType": "strong"},
"min": 0,
"max": "unlimited"}},
+ "qos_rules": {"type": {"key": {"type": "uuid",
+ "refTable": "QoS",
+ "refType": "strong"},
+ "min": 0,
+ "max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",
"refTable": "Load_Balancer",
"refType": "strong"},
@@ -118,6 +123,23 @@
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
"isRoot": false},
+ "QoS": {
+ "columns": {
+ "priority": {"type": {"key": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 32767}}},
+ "direction": {"type": {"key": {"type": "string",
+ "enum": ["set", ["from-lport", "to-lport"]]}}},
+ "match": {"type": "string"},
+ "action": {"type": {"key": {"type": "string",
+ "enum": ["set", ["dscp"]]},
+ "value": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 63}}},
+ "external_ids": {
+ "type": {"key": "string", "value": "string",
+ "min": 0, "max": "unlimited"}}},
+ "isRoot": false},
"Logical_Router": {
"columns": {
"name": {"type": "string"},
Any opinions from others whether we should stick with the patch as is
or implement arbitrary match criteria?
I don't think arbitrary match criteria requires a lot of code, though
it would benefit from new nbctl commands, based on the existing acl
commands. I am willing to help out if you wish. Note that the
ovn/ovn-nb.ovsschema proposal above is dependent on an IDL fix to
overcome a build error. I have not submitted that fix yet but I will
raise it very soon.
Signed-off-by: Babu Shanmugam <bscha...@redhat.com
<mailto:bscha...@redhat.com>>
---
ovn/lib/logical-fields.c | 2 +-
ovn/northd/ovn-northd.8.xml | 30 +++++++++++++++----
ovn/northd/ovn-northd.c | 69
++++++++++++++++++++++++++----------------
ovn/ovn-nb.xml | 6 ++++
ovn/ovn-sb.xml | 5 ++++
tests/ovn.at <http://ovn.at> | 73
+++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 152 insertions(+), 33 deletions(-)
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 3448370..f142871 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -362,7 +362,25 @@
</li>
</ul>
- <h3>Ingress Table 7: LB</h3>
+ <h3>Ingress Table 7: Ingress Port DSCP</h3>
+
+ <p>
+ Ingress table 7 contains these logical flows:
+ </p>
+
+ <ul>
+ <li>
+ For every port with a DSCP setting, one priority-100 flow
that matches
+ the <code>inport</code> on the corresponding switch and
sets DSCP.
+ </li>
+
+ <li>
+ One priority-0 fallback flow that matches all packets and
advances to
+ the next table.
+ </li>
+ </ul>
+
+ <h3>Ingress Table 8: LB</h3>
<p>
It contains a priority-0 flow that simply moves traffic to
the next
@@ -375,7 +393,7 @@
connection.)
</p>
- <h3>Ingress Table 8: Stateful</h3>
+ <h3>Ingress Table 9: Stateful</h3>
<ul>
<li>
@@ -412,7 +430,7 @@
</li>
</ul>
- <h3>Ingress Table 9: ARP/ND responder</h3>
+ <h3>Ingress Table 10: ARP/ND responder</h3>
<p>
This table implements ARP/ND responder for known IPs. It
contains these
@@ -484,7 +502,7 @@ nd_na {
</li>
</ul>
- <h3>Ingress Table 10: DHCP option processing</h3>
+ <h3>Ingress Table 11: DHCP option processing</h3>
<p>
This table adds the DHCPv4 options to a DHCPv4 packet from the
@@ -544,7 +562,7 @@ next;
</li>
</ul>
- <h3>Ingress Table 11: DHCP responses</h3>
+ <h3>Ingress Table 12: DHCP responses</h3>
<p>
This table implements DHCP responder for the DHCP replies
generated by
@@ -626,7 +644,7 @@ output;
</li>
</ul>
- <h3>Ingress Table 12: Destination Lookup</h3>
+ <h3>Ingress Table 13 Destination Lookup</h3>
^^^:^^^
<p>
This table implements switching behavior. It contains
these logical
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index d7d61bf..f0b1bb7 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -93,21 +93,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") \
- PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2,
"ls_in_port_sec_nd") \
- PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3,
"ls_in_pre_acl") \
- PIPELINE_STAGE(SWITCH, IN, PRE_LB, 4,
"ls_in_pre_lb") \
- PIPELINE_STAGE(SWITCH, IN, PRE_STATEFUL, 5,
"ls_in_pre_stateful") \
- PIPELINE_STAGE(SWITCH, IN, ACL, 6, "ls_in_acl")
\
- PIPELINE_STAGE(SWITCH, IN, LB, 7, "ls_in_lb")
\
- PIPELINE_STAGE(SWITCH, IN, STATEFUL, 8,
"ls_in_stateful") \
- PIPELINE_STAGE(SWITCH, IN, ARP_ND_RSP, 9,
"ls_in_arp_rsp") \
- PIPELINE_STAGE(SWITCH, IN, DHCP_OPTIONS, 10,
"ls_in_dhcp_options") \
- PIPELINE_STAGE(SWITCH, IN, DHCP_RESPONSE, 11,
"ls_in_dhcp_response") \
- PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 12,
"ls_in_l2_lkup") \
+#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") \
+ PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2,
"ls_in_port_sec_nd") \
+ PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3,
"ls_in_pre_acl") \
+ PIPELINE_STAGE(SWITCH, IN, PRE_LB, 4,
"ls_in_pre_lb") \
+ PIPELINE_STAGE(SWITCH, IN, PRE_STATEFUL, 5,
"ls_in_pre_stateful") \
+ PIPELINE_STAGE(SWITCH, IN, ACL, 6, "ls_in_acl")
\
+ PIPELINE_STAGE(SWITCH, IN, PORT_DSCP, 7,
"ls_in_port_dscp") \
I believe we should eventually do more than just port-based DSCP.
In addition, if we were to ever implement 802.1p CoS marking, wouldn't
we want to do that in the same pipeline stage?
I suggest the pipeline stage name be either "QOS" or "QOS_MARKING".
+ PIPELINE_STAGE(SWITCH, IN, LB, 8, "ls_in_lb")
\
+ PIPELINE_STAGE(SWITCH, IN, STATEFUL, 9,
"ls_in_stateful") \
+ PIPELINE_STAGE(SWITCH, IN, ARP_ND_RSP, 10,
"ls_in_arp_rsp") \
+ PIPELINE_STAGE(SWITCH, IN, DHCP_OPTIONS, 11,
"ls_in_dhcp_options") \
+ PIPELINE_STAGE(SWITCH, IN, DHCP_RESPONSE, 12,
"ls_in_dhcp_response") \
+ PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 13,
"ls_in_l2_lkup") \
Mickey