On Thu, Apr 15, 2021 at 1:26 PM Ilya Maximets <i.maxim...@ovn.org> wrote: > > On 4/15/21 7:15 PM, Han Zhou wrote: > > > > > > On Tue, Apr 13, 2021 at 1:23 AM Ilya Maximets <i.maxim...@ovn.org > > <mailto:i.maxim...@ovn.org>> 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 <dce...@redhat.com <mailto:dce...@redhat.com>> > >> Reported-at: https://bugzilla.redhat.com/1947398 > >> <https://bugzilla.redhat.com/1947398> > >> Acked-by: Dumitru Ceara <dce...@redhat.com <mailto:dce...@redhat.com>> > >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org > >> <mailto:i.maxim...@ovn.org>> > >> --- > >> > >> 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? > > OVS seems to be just collects them in a linked list, so, yes, > memory is the limit. > > > > > Acked-by: Han Zhou <hz...@ovn.org <mailto:hz...@ovn.org>>
Thanks Ilya for the patch and Dumitru and Han for the reviews. I applied this patch to the main branch. Numan > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
