diff mbox series

[ovs-dev,v3,15/16] northd: Add a noop handler for northd SB mac binding.

Message ID 20231128023803.570013-1-numans@ovn.org
State Changes Requested
Headers show
Series northd lflow incremental processing | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Numan Siddique Nov. 28, 2023, 2:38 a.m. UTC 
From: Numan Siddique <numans@ovn.org>

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/inc-proc-northd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Dumitru Ceara Dec. 13, 2023, 2:57 p.m. UTC | #1 
On 11/28/23 03:38, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  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(&en_northd, &en_sb_mirror, NULL);
>      engine_add_input(&en_northd, &en_sb_meter, NULL);
>      engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> -    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
>      engine_add_input(&en_northd, &en_sb_dns, NULL);
>      engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
>      engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>      engine_add_input(&en_northd, &en_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(&en_northd, &en_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(&en_northd, &en_sb_port_binding,
>                       northd_sb_port_binding_handler);
>      engine_add_input(&en_northd, &en_nb_logical_switch,

Regards,
Dumitru
Numan Siddique Dec. 18, 2023, 8:22 p.m. UTC | #2 
On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 11/28/23 03:38, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  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(&en_northd, &en_sb_mirror, NULL);
> >      engine_add_input(&en_northd, &en_sb_meter, NULL);
> >      engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> > -    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
> >      engine_add_input(&en_northd, &en_sb_dns, NULL);
> >      engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
> >      engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >      engine_add_input(&en_northd, &en_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(&en_northd, &en_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(&en_northd, &en_sb_port_binding,
> >                       northd_sb_port_binding_handler);
> >      engine_add_input(&en_northd, &en_nb_logical_switch,
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Dec. 19, 2023, 1:55 a.m. UTC | #3 
On Mon, Dec 18, 2023 at 12:22 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Wed, Dec 13, 2023 at 9:57 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > On 11/28/23 03:38, numans@ovn.org wrote:
> > > From: Numan Siddique <numans@ovn.org>
> > >
> > > Signed-off-by: Numan Siddique <numans@ovn.org>
> > > ---
> > >  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(&en_northd, &en_sb_mirror, NULL);
> > >      engine_add_input(&en_northd, &en_sb_meter, NULL);
> > >      engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> > > -    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
> > >      engine_add_input(&en_northd, &en_sb_dns, NULL);
> > >      engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
> > >      engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> > > @@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
> > >      engine_add_input(&en_northd, &en_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(&en_northd, &en_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 <amusil@redhat.com> may help confirm.

Thanks,
Han

> Thanks
> Numan
>
> >
> > >      engine_add_input(&en_northd, &en_sb_port_binding,
> > >                       northd_sb_port_binding_handler);
> > >      engine_add_input(&en_northd, &en_nb_logical_switch,
> >
> > Regards,
> > Dumitru
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

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(&en_northd, &en_sb_mirror, NULL);
     engine_add_input(&en_northd, &en_sb_meter, NULL);
     engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
-    engine_add_input(&en_northd, &en_sb_mac_binding, NULL);
     engine_add_input(&en_northd, &en_sb_dns, NULL);
     engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
     engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
@@ -196,6 +195,14 @@  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
     engine_add_input(&en_northd, &en_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(&en_northd, &en_sb_mac_binding,
+                     engine_noop_handler);
+
     engine_add_input(&en_northd, &en_sb_port_binding,
                      northd_sb_port_binding_handler);
     engine_add_input(&en_northd, &en_nb_logical_switch,