Re: [ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
On Mon, Dec 18, 2023 at 12:22 PM Numan Siddique wrote: > > On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara wrote: > > > > On 11/28/23 03:38, num...@ovn.org wrote: > > > From: Numan Siddique > > > > > > Signed-off-by: Numan Siddique > > > --- > > > northd/inc-proc-northd.c | 9 - > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > > index 7b1c6597e2..28f397ff39 100644 > > > --- a/northd/inc-proc-northd.c > > > +++ b/northd/inc-proc-northd.c > > > @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > engine_add_input(_northd, _sb_mirror, NULL); > > > engine_add_input(_northd, _sb_meter, NULL); > > > engine_add_input(_northd, _sb_datapath_binding, NULL); > > > -engine_add_input(_northd, _sb_mac_binding, NULL); > > > engine_add_input(_northd, _sb_dns, NULL); > > > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > > > engine_add_input(_northd, _sb_ip_multicast, NULL); > > > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > engine_add_input(_northd, _global_config, > > > northd_global_config_handler); > > > > > > +/* northd engine node uses the sb mac binding table to > > > + * cleanup mac binding entries for deleted logical ports > > > + * and datapaths. Any update to to SB mac binding doesn't > > > + * change the northd engine node state or data. Hence > > > + * it is ok to add a noop_handler here. */ > > > +engine_add_input(_northd, _sb_mac_binding, > > > + engine_noop_handler); > > > + > > > > Isn't this just a case of "ovn-northd" is not really interested in > > change tracking for SB.MAC_Binding? Can't we instead just disable > > alerting, ovsdb_idl_omit_alert(..), for all SBREC_MAC_BINDING columns > > like we do for other SB tables (lflow, multicast_group, meter, > > portt_group, logical_dp_group)? > > I thought about that. But mac_binding_ageing engine node also depends > on SB mac_binding and it results in full recompute (no handler for it). > This is a reasonable consideration. In this case I would put such consideration in the comment that explains why using the noop handler. > If @Ales Musil can confirm that the mac_binding_ageing node doesn't need > to handle SB mac_binding changes, then I'm fine with your suggestion. > I tend to believe that mac_binding_aging node doesn't really need to handle mac_binding changes. The aging node processing should be triggered solely by the aging timer. @Ales Musil may help confirm. Thanks, Han > Thanks > Numan > > > > > > engine_add_input(_northd, _sb_port_binding, > > > northd_sb_port_binding_handler); > > > engine_add_input(_northd, _nb_logical_switch, > > > > Regards, > > Dumitru > > > > ___ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara wrote: > > On 11/28/23 03:38, num...@ovn.org wrote: > > From: Numan Siddique > > > > Signed-off-by: Numan Siddique > > --- > > northd/inc-proc-northd.c | 9 - > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 7b1c6597e2..28f397ff39 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(_northd, _sb_mirror, NULL); > > engine_add_input(_northd, _sb_meter, NULL); > > engine_add_input(_northd, _sb_datapath_binding, NULL); > > -engine_add_input(_northd, _sb_mac_binding, NULL); > > engine_add_input(_northd, _sb_dns, NULL); > > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > > engine_add_input(_northd, _sb_ip_multicast, NULL); > > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(_northd, _global_config, > > northd_global_config_handler); > > > > +/* northd engine node uses the sb mac binding table to > > + * cleanup mac binding entries for deleted logical ports > > + * and datapaths. Any update to to SB mac binding doesn't > > + * change the northd engine node state or data. Hence > > + * it is ok to add a noop_handler here. */ > > +engine_add_input(_northd, _sb_mac_binding, > > + engine_noop_handler); > > + > > Isn't this just a case of "ovn-northd" is not really interested in > change tracking for SB.MAC_Binding? Can't we instead just disable > alerting, ovsdb_idl_omit_alert(..), for all SBREC_MAC_BINDING columns > like we do for other SB tables (lflow, multicast_group, meter, > portt_group, logical_dp_group)? I thought about that. But mac_binding_ageing engine node also depends on SB mac_binding and it results in full recompute (no handler for it). If @Ales Musil can confirm that the mac_binding_ageing node doesn't need to handle SB mac_binding changes, then I'm fine with your suggestion. Thanks Numan > > > engine_add_input(_northd, _sb_port_binding, > > northd_sb_port_binding_handler); > > engine_add_input(_northd, _nb_logical_switch, > > Regards, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
On 11/28/23 03:38, num...@ovn.org wrote: > From: Numan Siddique > > Signed-off-by: Numan Siddique > --- > northd/inc-proc-northd.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 7b1c6597e2..28f397ff39 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(_northd, _sb_mirror, NULL); > engine_add_input(_northd, _sb_meter, NULL); > engine_add_input(_northd, _sb_datapath_binding, NULL); > -engine_add_input(_northd, _sb_mac_binding, NULL); > engine_add_input(_northd, _sb_dns, NULL); > engine_add_input(_northd, _sb_ha_chassis_group, NULL); > engine_add_input(_northd, _sb_ip_multicast, NULL); > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(_northd, _global_config, > northd_global_config_handler); > > +/* northd engine node uses the sb mac binding table to > + * cleanup mac binding entries for deleted logical ports > + * and datapaths. Any update to to SB mac binding doesn't > + * change the northd engine node state or data. Hence > + * it is ok to add a noop_handler here. */ > +engine_add_input(_northd, _sb_mac_binding, > + engine_noop_handler); > + Isn't this just a case of "ovn-northd" is not really interested in change tracking for SB.MAC_Binding? Can't we instead just disable alerting, ovsdb_idl_omit_alert(..), for all SBREC_MAC_BINDING columns like we do for other SB tables (lflow, multicast_group, meter, portt_group, logical_dp_group)? > engine_add_input(_northd, _sb_port_binding, > northd_sb_port_binding_handler); > engine_add_input(_northd, _nb_logical_switch, Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 15/16] northd: Add a noop handler for northd SB mac binding.
From: Numan Siddique Signed-off-by: Numan Siddique --- northd/inc-proc-northd.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 7b1c6597e2..28f397ff39 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(_northd, _sb_mirror, NULL); engine_add_input(_northd, _sb_meter, NULL); engine_add_input(_northd, _sb_datapath_binding, NULL); -engine_add_input(_northd, _sb_mac_binding, NULL); engine_add_input(_northd, _sb_dns, NULL); engine_add_input(_northd, _sb_ha_chassis_group, NULL); engine_add_input(_northd, _sb_ip_multicast, NULL); @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(_northd, _global_config, northd_global_config_handler); +/* northd engine node uses the sb mac binding table to + * cleanup mac binding entries for deleted logical ports + * and datapaths. Any update to to SB mac binding doesn't + * change the northd engine node state or data. Hence + * it is ok to add a noop_handler here. */ +engine_add_input(_northd, _sb_mac_binding, + engine_noop_handler); + engine_add_input(_northd, _sb_port_binding, northd_sb_port_binding_handler); engine_add_input(_northd, _nb_logical_switch, -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
