On Fri, Aug 11, 2023 at 2:25 PM Dumitru Ceara <dce...@redhat.com> wrote:

> It's safe to assume that tables that existed in the previous LTS branch
> first release (currently 22.03.0) can be monitored directly.  Do so and
> only "optionally" monitor the ones that have been added since.
>
> This way we avoid the need for the IDL to expose an API to change the
> default condition for monitored tables.  It also avoids complex code in
> ovn-controller because we'd otherwise have to explicitly re-initialize
> conditions to a non-default (false) value after every SB reconnect.
>
> NOTE: In order to make sure that pre-existing L3 and L2 gateways are not
> initially considered "non-local" we explicitly request for all port
> bindings of this type to be monitored in the startup stage (before we
> got the initial contents of the database and our chassis record).
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406201.html
> Reported-by: Vladislav Odintsov <odiv...@gmail.com>
> CC: Ales Musil <amu...@redhat.com>
> CC: Han Zhou <hz...@ovn.org>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
>  controller/ovn-controller.c |   40
> ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 1bba7715d7..0c975dc5ec 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -190,11 +190,15 @@ static char *get_file_system_id(void)
>  /* Only set monitor conditions on tables that are available in the
>   * server schema.
>   */
> -#define sb_table_set_monitor_condition(idl, table, cond)   \
> -    (sbrec_server_has_##table##_table(idl)              \
> -     ? sbrec_##table##_set_condition(idl, cond) \
> +#define sb_table_set_opt_mon_condition(idl, table, cond) \
> +    (sbrec_server_has_##table##_table(idl)               \
> +     ? sbrec_##table##_set_condition(idl, cond)          \
>       : 0)
>
> +/* Assume the table exists in the server schema and set its condition. */
> +#define sb_table_set_req_mon_condition(idl, table, cond) \
> +    sbrec_##table##_set_condition(idl, cond)
> +
>  static unsigned int
>  update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>                     const struct sbrec_chassis *chassis,
> @@ -294,6 +298,14 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>          /* During initialization, we monitor all records in
> Chassis_Private so
>           * that we don't try to recreate existing ones. */
>          ovsdb_idl_condition_add_clause_true(&chprv);
> +        /* Also, to avoid traffic disruption (e.g., conntrack flushing for
> +         * zones that are used by OVN but not yet known due to the SB
> initial
> +         * contents not being available), monitor all port bindings
> +         * connected to gateways; they might be claimed as soon as the
> +         * chassis is available.
> +         */
> +        sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway");
> +        sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway");
>      }
>
>      if (local_ifaces) {
> @@ -342,17 +354,17 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>
>  out:;
>      unsigned int cond_seqnos[] = {
> -        sb_table_set_monitor_condition(ovnsb_idl, port_binding, &pb),
> -        sb_table_set_monitor_condition(ovnsb_idl, logical_flow, &lf),
> -        sb_table_set_monitor_condition(ovnsb_idl, logical_dp_group,
> &ldpg),
> -        sb_table_set_monitor_condition(ovnsb_idl, mac_binding, &mb),
> -        sb_table_set_monitor_condition(ovnsb_idl, multicast_group, &mg),
> -        sb_table_set_monitor_condition(ovnsb_idl, dns, &dns),
> -        sb_table_set_monitor_condition(ovnsb_idl, controller_event, &ce),
> -        sb_table_set_monitor_condition(ovnsb_idl, ip_multicast,
> &ip_mcast),
> -        sb_table_set_monitor_condition(ovnsb_idl, igmp_group, &igmp),
> -        sb_table_set_monitor_condition(ovnsb_idl, chassis_private,
> &chprv),
> -        sb_table_set_monitor_condition(ovnsb_idl, chassis_template_var,
> &tv),
> +        sb_table_set_req_mon_condition(ovnsb_idl, port_binding, &pb),
> +        sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, &lf),
> +        sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group,
> &ldpg),
> +        sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, &mb),
> +        sb_table_set_req_mon_condition(ovnsb_idl, multicast_group, &mg),
> +        sb_table_set_req_mon_condition(ovnsb_idl, dns, &dns),
> +        sb_table_set_req_mon_condition(ovnsb_idl, controller_event, &ce),
> +        sb_table_set_req_mon_condition(ovnsb_idl, ip_multicast,
> &ip_mcast),
> +        sb_table_set_req_mon_condition(ovnsb_idl, igmp_group, &igmp),
> +        sb_table_set_req_mon_condition(ovnsb_idl, chassis_private,
> &chprv),
> +        sb_table_set_opt_mon_condition(ovnsb_idl, chassis_template_var,
> &tv),
>      };
>
>      unsigned int expected_cond_seqno = 0;
>
>
Looks good to me, thanks.

Acked-by: Ales Musil <amu...@redhat.com>
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

amu...@redhat.com    IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to