On Tue, Apr 13, 2021 at 1:23 AM Ilya Maximets <[email protected]> wrote: > > If some OF rules needs to be replaced due to logical flow changes, > ovn-controller deletes old rules first and adds new ones later. > In a complex scenario with big number of flows a lot of time > can pass between these events leading to the dataplane downtime > and packet loss. Also, while these changes are in progress, > OVS will use incomplete pipelines that will also lead to packet > drops. > > To avoid this, all flow modifications should be done atomically, > so there will be always some version of OF rules installed that > can handle dataplane traffic and it will be complete in terms of > reflecting some consistent set of logical flows. Wrapping all > flow modifications into atomic ordered bundle to achieve that. > > Reported-by: Dumitru Ceara <[email protected]> > Reported-at: https://bugzilla.redhat.com/1947398 > Acked-by: Dumitru Ceara <[email protected]> > Signed-off-by: Ilya Maximets <[email protected]> > --- > > Version 2: > * Updated TODO.rst. > * Removed extra spaces. > * Added Ack from Dumitru. > > TODO.rst | 2 -- > controller/ofctrl.c | 80 +++++++++++++++++++++++++++++++++++---------- > 2 files changed, 62 insertions(+), 20 deletions(-) > > diff --git a/TODO.rst b/TODO.rst > index ecfe62870..c89fe203e 100644 > --- a/TODO.rst > +++ b/TODO.rst > @@ -53,8 +53,6 @@ OVN To-do List > > * Hitless upgrade, especially for data plane. > > -* Use OpenFlow "bundles" for transactional data plane updates. > - > * Dynamic IP to MAC binding enhancements. > > OVN has basic support for establishing IP to MAC bindings dynamically, using > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 415d9b7e1..c29c3d180 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -29,6 +29,7 @@ > #include "openvswitch/list.h" > #include "openvswitch/match.h" > #include "openvswitch/ofp-actions.h" > +#include "openvswitch/ofp-bundle.h" > #include "openvswitch/ofp-flow.h" > #include "openvswitch/ofp-group.h" > #include "openvswitch/ofp-match.h" > @@ -1567,10 +1568,22 @@ encode_flow_mod(struct ofputil_flow_mod *fm) > } > > static void > -add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs) > +add_flow_mod(struct ofputil_flow_mod *fm, > + struct ofputil_bundle_ctrl_msg *bc, > + struct ovs_list *msgs) > { > struct ofpbuf *msg = encode_flow_mod(fm); > - ovs_list_push_back(msgs, &msg->list_node); > + struct ofputil_bundle_add_msg bam = { > + .bundle_id = bc->bundle_id, > + .flags = bc->flags, > + .msg = msg->data, > + }; > + struct ofpbuf *bundle_msg; > + > + bundle_msg = ofputil_encode_bundle_add(OFP15_VERSION, &bam); > + > + ofpbuf_delete(msg); > + ovs_list_push_back(msgs, &bundle_msg->list_node); > } > > /* group_table. */ > @@ -1752,7 +1765,9 @@ add_meter(struct ovn_extend_table_info *m_desired, > } > > static void > -installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs) > +installed_flow_add(struct ovn_flow *d, > + struct ofputil_bundle_ctrl_msg *bc, > + struct ovs_list *msgs) > { > /* Send flow_mod to add flow. */ > struct ofputil_flow_mod fm = { > @@ -1764,11 +1779,12 @@ installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs) > .new_cookie = htonll(d->cookie), > .command = OFPFC_ADD, > }; > - add_flow_mod(&fm, msgs); > + add_flow_mod(&fm, bc, msgs); > } > > static void > installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, > + struct ofputil_bundle_ctrl_msg *bc, > struct ovs_list *msgs) > { > /* Update actions in installed flow. */ > @@ -1787,7 +1803,7 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, > /* Use OFPFC_ADD so that cookie can be updated. */ > fm.command = OFPFC_ADD; > } > - add_flow_mod(&fm, msgs); > + add_flow_mod(&fm, bc, msgs); > > /* Replace 'i''s actions and cookie by 'd''s. */ > free(i->ofpacts); > @@ -1797,7 +1813,9 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, > } > > static void > -installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) > +installed_flow_del(struct ovn_flow *i, > + struct ofputil_bundle_ctrl_msg *bc, > + struct ovs_list *msgs) > { > struct ofputil_flow_mod fm = { > .match = i->match, > @@ -1805,11 +1823,12 @@ installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) > .table_id = i->table_id, > .command = OFPFC_DELETE_STRICT, > }; > - add_flow_mod(&fm, msgs); > + add_flow_mod(&fm, bc, msgs); > } > > static void > update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, > + struct ofputil_bundle_ctrl_msg *bc, > struct ovs_list *msgs) > { > ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); > @@ -1823,7 +1842,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, > if (!d) { > /* Installed flow is no longer desirable. Delete it from the > * switch and from installed_flows. */ > - installed_flow_del(&i->flow, msgs); > + installed_flow_del(&i->flow, bc, msgs); > ovn_flow_log(&i->flow, "removing installed"); > > hmap_remove(&installed_flows, &i->match_hmap_node); > @@ -1832,7 +1851,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, > if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, > d->flow.ofpacts, d->flow.ofpacts_len) || > i->flow.cookie != d->flow.cookie) { > - installed_flow_mod(&i->flow, &d->flow, msgs); > + installed_flow_mod(&i->flow, &d->flow, bc, msgs); > ovn_flow_log(&i->flow, "updating installed"); > } > link_installed_to_desired(i, d); > @@ -1847,7 +1866,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, > i = installed_flow_lookup(&d->flow); > if (!i) { > ovn_flow_log(&d->flow, "adding installed"); > - installed_flow_add(&d->flow, msgs); > + installed_flow_add(&d->flow, bc, msgs); > > /* Copy 'd' from 'flow_table' to installed_flows. */ > i = installed_flow_dup(d); > @@ -1860,7 +1879,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, > * flow then modify the installed flow. > */ > if (link_installed_to_desired(i, d)) { > - installed_flow_mod(&i->flow, &d->flow, msgs); > + installed_flow_mod(&i->flow, &d->flow, bc, msgs); > ovn_flow_log(&i->flow, "updating installed (conflict)"); > } > } > @@ -1941,6 +1960,7 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > static void > update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > + struct ofputil_bundle_ctrl_msg *bc, > struct ovs_list *msgs) > { > merge_tracked_flows(flow_table); > @@ -1956,7 +1976,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > struct desired_flow *d = installed_flow_get_active(i); > > if (!d) { > - installed_flow_del(&i->flow, msgs); > + installed_flow_del(&i->flow, bc, msgs); > ovn_flow_log(&i->flow, "removing installed (tracked)"); > > hmap_remove(&installed_flows, &i->match_hmap_node); > @@ -1966,7 +1986,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > * installed flow, so update the OVS flow for the new > * active flow (at least the cookie will be different, > * even if the actions are the same). */ > - installed_flow_mod(&i->flow, &d->flow, msgs); > + installed_flow_mod(&i->flow, &d->flow, bc, msgs); > ovn_flow_log(&i->flow, "updating installed (tracked)"); > } > } > @@ -1976,7 +1996,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > struct installed_flow *i = installed_flow_lookup(&f->flow); > if (!i) { > /* Adding a new flow. */ > - installed_flow_add(&f->flow, msgs); > + installed_flow_add(&f->flow, bc, msgs); > ovn_flow_log(&f->flow, "adding installed (tracked)"); > > /* Copy 'f' from 'flow_table' to installed_flows. */ > @@ -1987,7 +2007,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > } else if (installed_flow_get_active(i) == f) { > /* The installed flow is installed for f, but f has change > * tracked, so it must have been modified. */ > - installed_flow_mod(&i->flow, &f->flow, msgs); > + installed_flow_mod(&i->flow, &f->flow, bc, msgs); > ovn_flow_log(&i->flow, "updating installed (tracked)"); > } else if (!f->installed_flow) { > /* Adding a new flow that conflicts with an existing installed > @@ -1996,7 +2016,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, > * then modify the installed flow. > */ > if (link_installed_to_desired(i, f)) { > - installed_flow_mod(&i->flow, &f->flow, msgs); > + installed_flow_mod(&i->flow, &f->flow, bc, msgs); > ovn_flow_log(&i->flow, > "updating installed (tracked conflict)"); > } > @@ -2126,10 +2146,34 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, > } > } > > + /* Add all flow updates into a bundle. */ > + static int bundle_id = 0; > + struct ofputil_bundle_ctrl_msg bc = { > + .bundle_id = bundle_id++, > + .flags = OFPBF_ORDERED | OFPBF_ATOMIC, > + }; > + struct ofpbuf *bundle_open, *bundle_commit; > + > + /* Open a new bundle. */ > + bc.type = OFPBCT_OPEN_REQUEST; > + bundle_open = ofputil_encode_bundle_ctrl_request(OFP15_VERSION, &bc); > + ovs_list_push_back(&msgs, &bundle_open->list_node); > + > if (flow_table->change_tracked) { > - update_installed_flows_by_track(flow_table, &msgs); > + update_installed_flows_by_track(flow_table, &bc, &msgs); > + } else { > + update_installed_flows_by_compare(flow_table, &bc, &msgs); > + } > + > + if (ovs_list_back(&msgs) == &bundle_open->list_node) { > + /* No flow updates. Removing the bundle open request. */ > + ovs_list_pop_back(&msgs); > + ofpbuf_delete(bundle_open); > } else { > - update_installed_flows_by_compare(flow_table, &msgs); > + /* Committing the bundle. */ > + bc.type = OFPBCT_COMMIT_REQUEST; > + bundle_commit = ofputil_encode_bundle_ctrl_request(OFP15_VERSION, &bc); > + ovs_list_push_back(&msgs, &bundle_commit->list_node); > } > > /* Iterate through the installed groups from previous runs. If they > -- > 2.26.2 >
The logic looks good to me. I am not sure if there is any size limit of a bundle supported by OVS? Or is it just limited by memory? Acked-by: Han Zhou <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
