On 4/15/21 7:15 PM, Han Zhou wrote: > > > On Tue, Apr 13, 2021 at 1:23 AM Ilya Maximets <[email protected] > <mailto:[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] <mailto:[email protected]>> >> Reported-at: https://bugzilla.redhat.com/1947398 >> <https://bugzilla.redhat.com/1947398> >> Acked-by: Dumitru Ceara <[email protected] <mailto:[email protected]>> >> Signed-off-by: Ilya Maximets <[email protected] <mailto:[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?
OVS seems to be just collects them in a linked list, so, yes, memory is the limit. > > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
