On 4/8/21 2:31 PM, Ilya Maximets 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
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
Hi Ilya,
Thanks for working on this, I think it's very useful.
With this patch we should probably also remove the item from TODO.rst:
"* Use OpenFlow "bundles" for transactional data plane updates."
> controller/ofctrl.c | 80 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 62 insertions(+), 18 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 415d9b7e1..0b24d98b3 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;
Nit: please remove one of the spaces before '='.
Shall we move the 'type' initializing in the struct initializer above?
I'm not sure whether it improves readability or not, I'll leave it up to
you.
> + 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;
Nit: please remove one of the spaces before '='.
> + 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
>
With the small nits above addressed:
Acked-by: Dumitru Ceara <[email protected]>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
