On 10/1/23 21:26, Han Zhou wrote:
> Use conditional monitor for the FDB for local datapaths.
>
> Signed-off-by: Han Zhou <[email protected]>
> ---
Hi Han,
This change makes complete sense and I think it should be backported to
all supported stable branches:
Fixes: 6ec3b12590f9 ("MAC learning: Add a new FDB table in southbound db.")
There's however one minor issue (also reported by ovsrobot CI at
https://github.com/ovsrobot/ovn/actions/runs/6373049552).
> v2: remove the accidentally modified OVS commit id of v1.
>
> controller/ovn-controller.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 859d9cab917b..2ff78619a3a7 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -211,8 +211,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> {
> /* Monitor Port_Bindings rows for local interfaces and local datapaths.
> *
> - * Monitor Logical_Flow, MAC_Binding, Multicast_Group, and DNS tables for
> - * local datapaths.
> + * Monitor Logical_Flow, MAC_Binding, FDB, Multicast_Group, and DNS
> tables
> + * for local datapaths.
> *
> * Monitor Controller_Event rows for local chassis.
> *
> @@ -230,6 +230,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> struct ovsdb_idl_condition lf = OVSDB_IDL_CONDITION_INIT(&lf);
> struct ovsdb_idl_condition ldpg = OVSDB_IDL_CONDITION_INIT(&ldpg);
> struct ovsdb_idl_condition mb = OVSDB_IDL_CONDITION_INIT(&mb);
> + struct ovsdb_idl_condition fdb = OVSDB_IDL_CONDITION_INIT(&fdb);
> struct ovsdb_idl_condition mg = OVSDB_IDL_CONDITION_INIT(&mg);
> struct ovsdb_idl_condition dns = OVSDB_IDL_CONDITION_INIT(&dns);
> struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT(&ce);
> @@ -248,6 +249,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> ovsdb_idl_condition_add_clause_true(&pb);
> ovsdb_idl_condition_add_clause_true(&lf);
> ovsdb_idl_condition_add_clause_true(&mb);
> + ovsdb_idl_condition_add_clause_true(&fdb);
> ovsdb_idl_condition_add_clause_true(&mg);
> ovsdb_idl_condition_add_clause_true(&dns);
> ovsdb_idl_condition_add_clause_true(&ce);
> @@ -337,6 +339,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> sbrec_logical_flow_add_clause_logical_datapath(&lf, OVSDB_F_EQ,
> uuid);
> sbrec_mac_binding_add_clause_datapath(&mb, OVSDB_F_EQ, uuid);
> + sbrec_fdb_add_clause_dp_key(&fdb, OVSDB_F_EQ,
> + ld->datapath->tunnel_key);
> sbrec_multicast_group_add_clause_datapath(&mg, OVSDB_F_EQ, uuid);
> sbrec_dns_add_clause_datapaths(&dns, OVSDB_F_INCLUDES, &uuid, 1);
> sbrec_ip_multicast_add_clause_datapath(&ip_mcast, OVSDB_F_EQ,
> @@ -360,6 +364,7 @@ out:;
> 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, fdb, &fdb),
> 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),
We're leaking the condition at the end of the function. We're
missing:
ovsdb_idl_condition_destroy(&fdb);
With that addressed and assuming all the OVN tests pass with
this applied:
Acked-by: Dumitru Ceara <[email protected]>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
