Message ID | 20231128023803.570013-1-numans@ovn.org |
---|---|
State | Changes Requested |
Headers | show |
Series | northd lflow incremental processing | expand |
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 |
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
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 >
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 --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,