The approach of using OVN logical ports to build the chain seems clean, but I wonder whether it's general-purpose enough to do what users want. If not, that is, if some users want to use physical appliances rather than VMs or containers that are integrated into OVN, the question arises of how to do that. It might be that such appliances can somehow be integrated with OVN by introducing new types of OVN logical ports, or some other alternative might need to be invented.
I don't know what operators want. I believe that Guru (CCed) has some of the same concerns. On Fri, Feb 03, 2017 at 10:17:05PM +0000, John McDowall wrote: > 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
