diff mbox series

[ovs-dev] northd: fix IPv6-PD with northd IP rework

Message ID cec6f836f4d9f43d1afa7f15cfe8ccc18240b420.1641928714.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series [ovs-dev] northd: fix IPv6-PD with northd IP rework | expand

Checks

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

Commit Message

Lorenzo Bianconi Jan. 11, 2022, 7:19 p.m. UTC
Since commit 4597317f1 ("Introduce incremental processing for northd"),
we rely on (partial) IP for ovn-northd. We need to track SB port_binding
option column in order to notify the CMS whenever the controller
receives the IPv6 prefix from the server.

Fixes: 4597317f1 ("Introduce incremental processing for northd")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/ovn-northd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Numan Siddique Jan. 11, 2022, 10:52 p.m. UTC | #1
On Tue, Jan 11, 2022 at 2:19 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> Since commit 4597317f1 ("Introduce incremental processing for northd"),
> we rely on (partial) IP for ovn-northd. We need to track SB port_binding
> option column in order to notify the CMS whenever the controller
> receives the IPv6 prefix from the server.
>
> Fixes: 4597317f1 ("Introduce incremental processing for northd")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

The fix makes sense to me.  Do you think it's straightforward to add a
test case for this ?
If so, I'd suggest adding one.

Thanks
Numan

> ---
>  northd/ovn-northd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 2b58bfcec..793135ede 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -786,7 +786,8 @@ main(int argc, char *argv[])
>                         &sbrec_port_binding_col_parent_port);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_tag);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_type);
> -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_options);
> +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> +                               &sbrec_port_binding_col_options);
>      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
>      add_column_noalert(ovnsb_idl_loop.idl,
>                         &sbrec_port_binding_col_nat_addresses);
> --
> 2.34.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Lorenzo Bianconi Jan. 12, 2022, 1:43 p.m. UTC | #2
> On Tue, Jan 11, 2022 at 2:19 PM Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:
> >
> > Since commit 4597317f1 ("Introduce incremental processing for northd"),
> > we rely on (partial) IP for ovn-northd. We need to track SB port_binding
> > option column in order to notify the CMS whenever the controller
> > receives the IPv6 prefix from the server.
> >
> > Fixes: 4597317f1 ("Introduce incremental processing for northd")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Hi Lorenzo,
> 
> The fix makes sense to me.  Do you think it's straightforward to add a
> test case for this ?
> If so, I'd suggest adding one.

Hi Numan,

we already have one full-test in system-ovn.at. I posted a patch to replace
dibbler (no longer maintained) with dhcpd so we can enable it by default in
github repo.

Regards,
Lorenzo

> 
> Thanks
> Numan
> 
> > ---
> >  northd/ovn-northd.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 2b58bfcec..793135ede 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -786,7 +786,8 @@ main(int argc, char *argv[])
> >                         &sbrec_port_binding_col_parent_port);
> >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_tag);
> >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_type);
> > -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_options);
> > +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > +                               &sbrec_port_binding_col_options);
> >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
> >      add_column_noalert(ovnsb_idl_loop.idl,
> >                         &sbrec_port_binding_col_nat_addresses);
> > --
> > 2.34.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
Numan Siddique Jan. 12, 2022, 3 p.m. UTC | #3
On Wed, Jan 12, 2022 at 8:44 AM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> > On Tue, Jan 11, 2022 at 2:19 PM Lorenzo Bianconi
> > <lorenzo.bianconi@redhat.com> wrote:
> > >
> > > Since commit 4597317f1 ("Introduce incremental processing for northd"),
> > > we rely on (partial) IP for ovn-northd. We need to track SB port_binding
> > > option column in order to notify the CMS whenever the controller
> > > receives the IPv6 prefix from the server.
> > >
> > > Fixes: 4597317f1 ("Introduce incremental processing for northd")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> > Hi Lorenzo,
> >
> > The fix makes sense to me.  Do you think it's straightforward to add a
> > test case for this ?
> > If so, I'd suggest adding one.
>
> Hi Numan,
>
> we already have one full-test in system-ovn.at. I posted a patch to replace
> dibbler (no longer maintained) with dhcpd so we can enable it by default in
> github repo.
>

I forgot that Ipv6 PD can't be tested with unit tests.

Thanks
Numan

> Regards,
> Lorenzo
>
> >
> > Thanks
> > Numan
> >
> > > ---
> > >  northd/ovn-northd.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 2b58bfcec..793135ede 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -786,7 +786,8 @@ main(int argc, char *argv[])
> > >                         &sbrec_port_binding_col_parent_port);
> > >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_tag);
> > >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_type);
> > > -    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_options);
> > > +    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > > +                               &sbrec_port_binding_col_options);
> > >      add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
> > >      add_column_noalert(ovnsb_idl_loop.idl,
> > >                         &sbrec_port_binding_col_nat_addresses);
> > > --
> > > 2.34.1
> > >
> > > _______________________________________________
> > > 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/ovn-northd.c b/northd/ovn-northd.c
index 2b58bfcec..793135ede 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -786,7 +786,8 @@  main(int argc, char *argv[])
                        &sbrec_port_binding_col_parent_port);
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_tag);
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_type);
-    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_options);
+    ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
+                               &sbrec_port_binding_col_options);
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
     add_column_noalert(ovnsb_idl_loop.idl,
                        &sbrec_port_binding_col_nat_addresses);