[ovs-dev,v2] ofctrl: Send all flow modifications in a bundle.

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 <dceara@redhat.com>
Reported-at: https://bugzilla.redhat.com/1947398
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@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(-)

On Tue, Apr 13, 2021 at 1:23 AM Ilya Maximets <i.maximets@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 <dceara@redhat.com>
> Reported-at: https://bugzilla.redhat.com/1947398
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Ilya Maximets <i.maximets@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?

Acked-by: Han Zhou <hzhou@ovn.org>

On 4/15/21 7:15 PM, Han Zhou wrote:
> 
> 
> On Tue, Apr 13, 2021 at 1:23 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@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 <dceara@redhat.com <mailto:dceara@redhat.com>>
>> Reported-at: https://bugzilla.redhat.com/1947398 <https://bugzilla.redhat.com/1947398>
>> Acked-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@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 <hzhou@ovn.org <mailto:hzhou@ovn.org>>

On Thu, Apr 15, 2021 at 1:26 PM Ilya Maximets <i.maximets@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.maximets@ovn.org <mailto:i.maximets@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 <dceara@redhat.com <mailto:dceara@redhat.com>>
> >> Reported-at: https://bugzilla.redhat.com/1947398 <https://bugzilla.redhat.com/1947398>
> >> Acked-by: Dumitru Ceara <dceara@redhat.com <mailto:dceara@redhat.com>>
> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@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 <hzhou@ovn.org <mailto:hzhou@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev