Ben,
Thanks for the thorough review, I will update the code and resubmit the patch.
Any thoughts on the general approach to service chaining etc?
Regards
John
On 2/3/17, 2:10 PM, "Ben Pfaff" <b...@ovn.org> wrote:
On Thu, Feb 02, 2017 at 03:22:56PM -0800, jmcdow...@paloaltonetworks.com
wrote:
> From: John McDowall <jmcdow...@paloaltonetworks.com>
>
> This patchset is the first round at having Service Function Chaining
> functionality through OVN. The implementation is done entirely
> on the northbound side of OVN. It is a bump on the wire implementation,
> so no attempt is being made in keeping state while packets visit each
> hop of the chain. ACLs are used as the classifiers, with the augmentation
> of action SFC, as well as option column.
Thanks for working on this! Sorry it's taken so long to review.
cmp_port_pair_groups() treats two pair groups as equal if either one of
them has no keys, but this violates transitivity (see
https://urldefense.proofpoint.com/v2/url?u=https-3A__en.wikipedia.org_wiki_Comparison-5Fsort&d=DwIBAg&c=V9IgWpI5PvzTw83UyHGVSoW3Uc1MFWe5J8PTfkrzVSo&r=vZ6VUDaavDpfOdPQrz1ED54jEjvAE36A8TVJroVlrOQ&m=D8WTzjM4eBLAIUSaMpsuNQPXOo94n7zg2ZjpkjdkW64&s=BhGNi3da1UI7sRkk29-zyejanKHyZPF_ac-fm2Ctnis&e=
). Example: if 0 is a pair
group with no keys and x and y are arbitrary pair groups, then this
function will conclude that x <= 0 and 0 <= y. By transitivity, we
would also have x <= y, but that's a contradiction because x and y are
arbitrary.
I suggest that empty pair groups should sort as less than nonempty ones,
e.g.:
@@ -2772,8 +2772,10 @@ cmp_port_pair_groups(const void *ppg1_, const void
*ppg2_)
const struct nbrec_logical_port_pair_group *ppg1 = *ppg1p;
const struct nbrec_logical_port_pair_group *ppg2 = *ppg2p;
- if (ppg1->n_sortkey == 0 || ppg2->n_sortkey == 0) {
- return 0;
+ if (ppg1->n_sortkey == 0) {
+ return ppg2->n_sortkey == 0 ? -1 : 0;
+ } else if (ppg2->n_sortkey == 0) {
+ return 1;
}
const int64_t key1 = ppg1->sortkey[0];
ovn-northd.c has some new uses of VLOG without rate-limiters. These
should probably be rate-limited, like most VLOG calls in ovn-northd.
VLOG messages shouldn't include a new-line since VLOG takes care of that
itself.
The code has lots of lines longer than the coding style suggested limit
of 79 columns.
Comments should generally begin with a capital letter and end with a
period.
In build_chain_classifier_entry(), qsort() is a remarkably expensive way
to find the smallest element in an array.
In build_chain_classifier_entry(), it's strange to have "" in the middle
of the string here:
ds_put_format(ds_action, "outport = %s;"" output;",
first_ovn_port->json_key);
In build_chain(), please don't comment out code:
//const uint16_t egress_inner_priority = 150;
In build_chain(), a lot of debug logging has escaped as VLOG_INFO calls.
In build_chain(), you can use xmemdup() instead of xmalloc() followed by
memcpy().
In build_chain(), I don't know why there's a line-splicing \ here:
VLOG_INFO("Warning: Currently lacking support for more
than one port-pair %"PRIuSIZE"\n", \
lppg->n_port_pairs);
Also, we'd usually just use VLOG_WARN instead of writing "Warning:".
In build_chain(), please always put {} around a conditional statement,
e.g. here:
if (!input_port_array[j] || !output_port_array[j])
obtainedAllNeededInfo = false;
Please wrap at 79 columns in ovn-architecture.7.xml as well.
I spent some time with the documentation. I worked on getting rid of
passive voice because it's often unclear, e.g. "something happens"
doesn't tell the reader who does it, but "The CMS does something" does.
I dropped a lot of the items from the life cycle that didn't seem
really relevant to VNFs (it seemed like they were mostly cut-and-paste).
Some of the introductory documentation confuses me though. The
following paragraph mentions about the OVN northbound database but it
seems to actually be about the southbound database. I don't think this
is the right place to explain the logical flows that SFC puts into the
southbound database; that should be in ovn-northd.8.xml instead. I
deleted the following paragraph but presumably it should be adapted for
ovn-nrothd.8.xml:
<p>
Service insertion is implemented by adding 4 new flow rules into the OVN
northbound database for
each VNF inserted. The rules are added into the last table in the
ingress path (L2_LKUP).
The service insertion rules have a higher priority than the standard
forwarding rules.
This means that they override the existing forwarding rules. There are
four
new rules added for each insertion. Two ingress and two egress, The
first ingress
rule sends all traffic destined for the application into the VNF ingress
port and the
second rule takes all traffic destined to the application from the VNF
egress port
to the application, the priorities are such that the second rule is
always checked
first. In the egress direction the rules are similar if the traffic is
from the
application it is sent to the VNF egress port and if if is from the
application
and is from the VNF ingress port it is delivered to the destination.
<!-- Should this be a new table or is it a naturally part of the L2
lookup table ? -->
</p>
Also this paragraph is confusing because the patch doesn't add any new
table named Service or Logical_Service or anything like that:
<p>
The steps in this example refer to the details of the OVN Northbound
database schema.
There is a new table in the OVN Northbound database to support service
insertion
called <code>Services</code>, which contains the required information
for each new
service inserted. The same service can be used for multiple
applications, as
there is typically an N:1 relationship between applications and VNFs. A
single VNF may be part of several service insertions, but each one is
logically
separate.
</p>
The ovn-nb documentation for port_chains says, "It is an error for
multiple logical switches to include the same logical port." Should
this say "same port_chain" instead?
The ovn-nb documentation for Logical_Switch does not explain port chains
or port pairs. I think that it is necessary to give an overview of what
they mean and how they should be defined; as is, I do not understand
them. It might be reasonable to add a "Service Function Chaining"
<group> with some introductory material and then document these two
columns in there.
The new ovn-nbctl commands need documentation.
ovn-nbctl.c has a weird "#ifdef JED" block.
I'm appending my suggested documentation updates. You can also find
these on the "sfc" branch at
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_blp_ovs-2Dreviews_tree_sfc&d=DwIBAg&c=V9IgWpI5PvzTw83UyHGVSoW3Uc1MFWe5J8PTfkrzVSo&r=vZ6VUDaavDpfOdPQrz1ED54jEjvAE36A8TVJroVlrOQ&m=D8WTzjM4eBLAIUSaMpsuNQPXOo94n7zg2ZjpkjdkW64&s=v8Cu7uB_gPp7cLmM3W2xZ06uDb868ipoYf4qo3n0fhw&e=
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index ee69fed..4aa52f3 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -386,7 +386,7 @@
<dfn>Logical services</dfn> are logical references to virtual
network functions
(VNF). Adding a logical service requires adding steering rules to
the OVN Northbound
database. These are the only rules necessary to implement traffic
steering for VNFs.
- The section below "<code>Life Cycle of an inserted VNF</code>"
provides more details.
+ See <code>Life Cycle of an inserted VNF</code>, below, for details.
</li>
<li>
<p>
@@ -572,181 +572,79 @@
</li>
</ol>
- <h2>Life Cycle of an inserted Virtual Network Function (VNF)</h2>
+ <h2>Life Cycle of an Inserted Virtual Network Function (VNF)</h2>
<p>
OVN provides an abstraction to enable the insertion of an arbitrary
virtual network
function (VNF) into the path of traffic to and from an application. A
VNF is different
- from an application VM in that it acts on traffic between applications,
and im most
- cases does not terminiate a flow. Proxy functions are an exception as
they terminate the
- flow from the src and create a new flow to the dst. Examples of VNF's
are security functions,
- load balancing, and traffic enhancement services, this is not a
complete list.
+ from an application VM in that it acts on traffic between applications,
and in most
+ cases does not terminate a flow. Proxy functions are an exception as
they terminate the
+ flow from the source and create a new flow to the destination. Examples
of VNFs are security functions
+ (e.g. intrusion detection systems),
+ load balancers, and traffic enhancement services.
</p>
<p>
- The requirements on the VNF to be inserted are minimal, it must
+ The requirements on the VNF to be inserted are minimal: it must
act as a <code>bump in the wire (BITW)</code> and can have one or two
virtual network
- ports for traffic. If it has two network ports traffic is directed into
one and out the other,
- if it has only one port, then traffic is bi-directional. The
requirement for the VNF to act as
- a BITW removes the need for the VNF to participate in L3/L2 networking
which provides
+ ports for traffic. If it has two network ports, it accepts traffic on
one port and
+ transmits it out the other;
+ if it has only one port, that port serves both purposes. The
requirement for the VNF to act as
+ a BITW removes the need for the VNF to participate in L2/L3 networking,
which provides
improved agility and reduces the coupling between OVN and the VNF.
</p>
<p>
- The service insertion is implemented by adding 4 new flow rules into
the ovn-nb database for
- each VNF inserted. The rules are added into the last table in the
ingress path (L2_LKUP).
- The service insertion rules have a higher priority than the standard
forwarding rules.
- This means that they override the existing forwarding rules. There are
four
- new rules added for each insertion. Two ingress and two egress, The
first ingress
- rule sends all traffic destined for the application into the VNF
ingress port and the
- second rule takes all traffic destined to the application from the VNF
egress port
- to the application, the priorities are such that the second rule is
always checked
- first. In the egress direction the rules are similar if the traffic is
from the
- application it is sent to the VNF egress port and if if is from the
application
- and is from the VNF ingress port it is delivered to the destination.
- <!-- Should this be a new table or is it a naturally part of the L2
lookup table ? -->
- </p>
- <p>
The steps in this example refer to the details of the OVN Northbound
database schema.
There is a new table in the OVN Northbound database to support service
insertion
- called <code>Services</code> this contains the required information for
each new
+ called <code>Services</code>, which contains the required information
for each new
service inserted. The same service can be used for multiple
applications, as
- there is typically a n:1 relationship between applications and VNFs. A
+ there is typically an N:1 relationship between applications and VNFs. A
single VNF may be part of several service insertions, but each one is
logically
separate.
</p>
<p>
- The steps in this example refer often to details of the OVN and OVN
- Northbound database schemas. Please see <code>ovn-sb</code>(5) and
- <code>ovn-nb</code>(5), respectively, for the full story on these
- databases. The ovn-nb database has specific schema enhancements for the
service
- insertion function. The ovn-sb database has no schema changes for the
- service insertion function.
- </p>
- <p>
The following steps are an overview to inserting a new VNF into the
traffic path.
The sections below go into each step in more detail.
</p>
<ol>
<li>
- The service insertion lifecycle begins when a CMS administrator
creates a new
- virtual network function <code>(VNF)</code> using the CMS user
+ The CMS administrator creates a new
+ virtual network function <code>(VNF)</code>, using the CMS user
interface or API. The CMS administrator creates the logical ports
(ingress and egress)
- for the VNF. If the CMS is Openstack this will create a reusable
port-pair defining the
- interface to the VNF. There is also typically a seperate management
port for the VNF,
+ for the VNF. If the CMS is OpenStack, this creates a reusable
port-pair defining the
+ interface to the VNF. The administrator also typically creates a
separate management port for the VNF,
but that is not relevant to the service insertion workflow. A single
VNF can
participate with several applications, either as a security VM,
protecting multiple
- applications or as a load balancer VM, distributing load across
multiple applications.
+ applications, or as a load balancer VM, distributing load across
multiple applications.
</li>
<li>
- The next step in the life cycle occurs when a CMS administrator
creates a new application
- with a VIF using the CMS user interface or API and adds it to a
switch (one
- implemented by OVN as a logical switch). Alternatively an already
running application could
- be used.
-
- The CMS can now attach the port pair to the VIF by defining the
logical port in the
- service function classifier. This will direct traffic to the VIF to
go through
- the VNF, applying the rules discussed earlier.
+ The CMS administrator creates a new application with a VIF using the
CMS
+ user interface or API (or chooses a running application) and adds it
to an
+ OVN logical switch.
</li>
<li>
- While within CMS the service insertion may be broken down into
multiple reusable steps
- (as is the case with Openstack). Within OVN the creating of a new
service insertion
- is treated as an atomic operation. This enables the easy atomic
insertion and deletion of
- service insertions. This is important as it is envisioned that the
number of serivce
- insertions can easily number in the hundreds, all with separate
lifecycles. For each new
- service insertion operation a new row is added to the OVN Northbound
Database. The new row is
- only added to the ovn-nb when the VNF, application and network are
enabled by the CMS.
-
- Once the serivce insertion is applied to the ovn-nb database, the
ovn-nb controller will be
- notified of a change and the rules will be pushed down to the
specific OVS instances, using
- the existing OVN mechanisms. It is important to note here that the
logical abstraction of
- OVN enables service insertion with minimal changes.
+ The CMS administrator attaches the VNF port pair to the VIF by
defining
+ the logical port in the service function classifier. To do so, the
CMS
+ inserts a row into the <code>ACL</code> table in the OVN northbound
+ database. The ACL's <code>type</code> is <code>sfc</code> and its
+ <code>options:sfc-port-chain</code> designates the VNF. This directs
+ traffic to the VIF to go through the VNF, applying the rules discussed
+ earlier.
</li>
<li>
- When the application VM shuts down the classification rule should be
removed, however as
- no traffic will be destinated to the application there would be no
issues. If the VM is
- being moved and the new application VM port is used to update the
service then the change
- would be detected and the rules pushed down.
- </li>
- <li>
- A VNF can be removed at any time and traffic flows will revert to the
pre-VNF traffic
- paths if it is removed from the ovn-nb database. OVN must detect that
the VNF has been
- shut-off as it must remove all the rules that are attached to that
VNF. Otherwise
- traffic loss will occur, if a failure in a VNF occurs that is not
detected.
- </li>
-
- <li>
- On every hypervisor, <code>ovn-controller</code> receives the
- <code>Logical_Service</code> table updates that
<code>ovn-northd</code> made
- in the previous step. As long as the VM that owns the VIF is powered
- off, <code>ovn-controller</code> cannot do much; it cannot, for
example,
- arrange to send packets to or receive packets from the VIF, because the
- VIF does not actually exist anywhere. In addition the VNF cannot be
inserted
- into the traffic path as it will have no source to forward traffic too.
- <!-- If there is no VM then traffic cannot be sent to it therefore
having the
- rules inserted is probably not an issue? -->
- </li>
-
- <li>
- Some CMS systems, including OpenStack, fully start a VM only when its
- networking is ready. To support this, <code>ovn-northd</code> notices
- the <code>chassis</code> column updated for the row in
- <code>Binding</code> table and pushes this upward by updating the
- <ref column="up" table="Logical_Port" db="OVN_NB"/> column in the OVN
- Northbound database's <ref table="Logical_Port" db="OVN_NB"/> table to
- indicate that the VIF is now up. The CMS, if it uses this feature, can
- then react by allowing the VM's execution to proceed.
-
- Traffic now flows into and out of the VM that has a VNF inserted in its
- datapath. The rules are applied to direct traffic to the VNF on the way
- into the VM and on the way out of the VM.
- </li>
-
- <!-- Need a section on having multiple VM's using the same VNF
- ( physcially or logically ). Different rule sets -->
- <!-- Need a section on distributed cases - one section for each -->
- <li>
- On every hypervisor but the one where the VIF resides,
- <code>ovn-controller</code> notices the completely populated row in the
- <code>Binding</code> table. This provides <code>ovn-controller</code>
- the physical location of the logical port, so each instance updates the
- OpenFlow tables of its switch (based on logical datapath flows in the
OVN
- DB <code>Logical_Flow</code> table) so that packets to and from the VIF
- can be properly handled via tunnels.
- </li>
- <!-- Current implementation is open on delete, i.e. when the VNF is
removed
- the datapath behaviour reverts to the pre-existing paths. Does this
make sense?
- - could argue that close on delete should be an option ? -->
-
- <li>
- Eventually, a user removes the inserted service function from the ovn
nb
- database. The rules are then updated in the southbound database and
pushed
- down to the relevant ovs. No other SIF is impacted as the row in the
ovn nb
- is independant of all the other SIF.
- <!-- This is really important in the case where many SIF are being
added
- and removed. Without both the independance of the enteries confusion
would
- exist. Also for debugging - possible to remove/add individual VNF's
to
- determine potentail problems. -->
- </li>
-
- <li>
- The CMS plugin removes the SIF from the OVN Northbound database,
- by deleting its row in the <code>Logical_Service</code> table.
- </li>
-
- <li>
- <code>ovn-northd</code> receives the OVN Northbound update and in turn
- updates the OVN Southbound database accordingly, by removing or
updating
- the rows from the OVN Southbound database <code>Logical_Service</code>
table.
- </li>
+ <p>
+ Eventually, the application VM shuts down and the CMS removes the
+ <code>sfc</code> ACL. (However, it is harmless if it remains,
since no
+ traffic will be sent to the application.)
+ </p>
- <li>
- On every hypervisor, <code>ovn-controller</code> receives the
- <code>Logical_Service</code> table updates that
<code>ovn-northd</code> made
- in the previous step. <code>ovn-controller</code> updates OpenFlow
- tables to reflect the update. The datapath for the VM will now revert
to
- paths that existed before the VNF was inserted into the data path.
+ <p>
+ Alternatively, eventually the CMS administrator detaches the VIF
from
+ the VNF. The CMS deletes the <code>sfc</code> ACL row, and traffic
+ reverts to the pre-VNF traffic paths.
+ </p>
</li>
</ol>
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index e45c670..b952a46 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -994,9 +994,7 @@
<li>
<code>sfc</code>: Forward the packet into a logical port chain.
- The chain to be used -- as well as any other attributes that
determine
- the behavior of the packet while in the chain -- are provided
- via <ref column="options"/>.
+ The <ref column="options"/> column provides details.
</li>
</ul>
</column>
@@ -1014,7 +1012,7 @@
</column>
<group title="Options">
- <column name = "options">
+ <column name="options">
This column provides key/value settings specific to the ACL
<ref column="action"/>. The type-specific options are described
individually below.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
