| Message ID | 20230609191256.3509034-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] ovn-northd.c: Omit unused columns in SB_Global. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Han, Acked-by: Mark Michelson <mmichels@redhat.com> I like the changes, but I don't like that patch 1 on its own results in breaking CI. To whoever merges this, I suggest reversing the order of the patches. This way CI is likely to succeed after patch 1 and will definitely succeed after patch 2. This is better than definitely failing after patch 1 and definitely succeeding after patch 2. On 6/9/23 15:12, Han Zhou wrote: > Connections and SSL columns are not used by ovn-northd. Omit them in > IDL. > > This is not a functional problem, but it may hide incremental processing > problems, because when status changes in Connection table, which is > referenced by SB_Global, ovn-northd will receive notifications for > SB_Global change, which triggers recompute. Some tests may pass when > this happens, while it should have failed if the recompute were not > triggered. An example is the test case "testing propagate > Port_Binding.up to NB and OVS", which has passed in most cases but fails > permanently afte this change (and will be fixed in a separate patch). > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > northd/ovn-northd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 0b8bbfb95cf7..647f60c8583f 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -818,6 +818,10 @@ main(int argc, char *argv[]) > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true); > > + /* Omit unused columns. */ > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_connections); > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ssl); > + > /* Disable alerting for pure write-only columns. */ > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg); > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_address_set_col_name);
On Mon, Jun 12, 2023 at 11:05 AM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Han, > > Acked-by: Mark Michelson <mmichels@redhat.com> > > I like the changes, but I don't like that patch 1 on its own results in > breaking CI. > > To whoever merges this, I suggest reversing the order of the patches. > This way CI is likely to succeed after patch 1 and will definitely > succeed after patch 2. This is better than definitely failing after > patch 1 and definitely succeeding after patch 2. > Thanks Mark. Is your Ack for both patches? If so, I can reorder and adjust the commit message for that when merging. Or I can do the same and send a v2 waiting for Ack for patch2. Thanks, Han > On 6/9/23 15:12, Han Zhou wrote: > > Connections and SSL columns are not used by ovn-northd. Omit them in > > IDL. > > > > This is not a functional problem, but it may hide incremental processing > > problems, because when status changes in Connection table, which is > > referenced by SB_Global, ovn-northd will receive notifications for > > SB_Global change, which triggers recompute. Some tests may pass when > > this happens, while it should have failed if the recompute were not > > triggered. An example is the test case "testing propagate > > Port_Binding.up to NB and OVS", which has passed in most cases but fails > > permanently afte this change (and will be fixed in a separate patch). > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > northd/ovn-northd.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 0b8bbfb95cf7..647f60c8583f 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -818,6 +818,10 @@ main(int argc, char *argv[]) > > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > > ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true); > > > > + /* Omit unused columns. */ > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_connections); > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ssl); > > + > > /* Disable alerting for pure write-only columns. */ > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg); > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_address_set_col_name); >
On 6/12/23 22:07, Han Zhou wrote: > > > On Mon, Jun 12, 2023 at 11:05 AM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > > > Hi Han, > > > > Acked-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > > > I like the changes, but I don't like that patch 1 on its own results in > > breaking CI. > > > > To whoever merges this, I suggest reversing the order of the patches. > > This way CI is likely to succeed after patch 1 and will definitely > > succeed after patch 2. This is better than definitely failing after > > patch 1 and definitely succeeding after patch 2. > > > Thanks Mark. Is your Ack for both patches? If so, I can reorder and > adjust the commit message for that when merging. > Or I can do the same and send a v2 waiting for Ack for patch2. > > Thanks, > Han Sorry for the confusion. My ack was for both patches. > > > On 6/9/23 15:12, Han Zhou wrote: > > > Connections and SSL columns are not used by ovn-northd. Omit them in > > > IDL. > > > > > > This is not a functional problem, but it may hide incremental > processing > > > problems, because when status changes in Connection table, which is > > > referenced by SB_Global, ovn-northd will receive notifications for > > > SB_Global change, which triggers recompute. Some tests may pass when > > > this happens, while it should have failed if the recompute were not > > > triggered. An example is the test case "testing propagate > > > Port_Binding.up to NB and OVS", which has passed in most cases but > fails > > > permanently afte this change (and will be fixed in a separate patch). > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > > > --- > > > northd/ovn-northd.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > index 0b8bbfb95cf7..647f60c8583f 100644 > > > --- a/northd/ovn-northd.c > > > +++ b/northd/ovn-northd.c > > > @@ -818,6 +818,10 @@ main(int argc, char *argv[]) > > > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > > > ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true); > > > > > > + /* Omit unused columns. */ > > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, > &sbrec_sb_global_col_connections); > > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ssl); > > > + > > > /* Disable alerting for pure write-only columns. */ > > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > &sbrec_sb_global_col_nb_cfg); > > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > &sbrec_address_set_col_name); > >
On Tue, Jun 13, 2023 at 6:44 AM Mark Michelson <mmichels@redhat.com> wrote: > > On 6/12/23 22:07, Han Zhou wrote: > > > > > > On Mon, Jun 12, 2023 at 11:05 AM Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> wrote: > > > > > > Hi Han, > > > > > > Acked-by: Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> > > > > > > I like the changes, but I don't like that patch 1 on its own results in > > > breaking CI. > > > > > > To whoever merges this, I suggest reversing the order of the patches. > > > This way CI is likely to succeed after patch 1 and will definitely > > > succeed after patch 2. This is better than definitely failing after > > > patch 1 and definitely succeeding after patch 2. > > > > > Thanks Mark. Is your Ack for both patches? If so, I can reorder and > > adjust the commit message for that when merging. > > Or I can do the same and send a v2 waiting for Ack for patch2. > > > > Thanks, > > Han > > Sorry for the confusion. My ack was for both patches. > Thanks Mark. I applied both patches to main with the order and commit message adjusted accordingly. Regards, Han > > > > > On 6/9/23 15:12, Han Zhou wrote: > > > > Connections and SSL columns are not used by ovn-northd. Omit them in > > > > IDL. > > > > > > > > This is not a functional problem, but it may hide incremental > > processing > > > > problems, because when status changes in Connection table, which is > > > > referenced by SB_Global, ovn-northd will receive notifications for > > > > SB_Global change, which triggers recompute. Some tests may pass when > > > > this happens, while it should have failed if the recompute were not > > > > triggered. An example is the test case "testing propagate > > > > Port_Binding.up to NB and OVS", which has passed in most cases but > > fails > > > > permanently afte this change (and will be fixed in a separate patch). > > > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > > > > --- > > > > northd/ovn-northd.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > > index 0b8bbfb95cf7..647f60c8583f 100644 > > > > --- a/northd/ovn-northd.c > > > > +++ b/northd/ovn-northd.c > > > > @@ -818,6 +818,10 @@ main(int argc, char *argv[]) > > > > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > > > > ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true); > > > > > > > > + /* Omit unused columns. */ > > > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, > > &sbrec_sb_global_col_connections); > > > > + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ssl); > > > > + > > > > /* Disable alerting for pure write-only columns. */ > > > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > > &sbrec_sb_global_col_nb_cfg); > > > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > > &sbrec_address_set_col_name); > > > >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 0b8bbfb95cf7..647f60c8583f 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -818,6 +818,10 @@ main(int argc, char *argv[]) ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true); + /* Omit unused columns. */ + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_connections); + ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_ssl); + /* Disable alerting for pure write-only columns. */ ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_sb_global_col_nb_cfg); ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_address_set_col_name);
Connections and SSL columns are not used by ovn-northd. Omit them in IDL. This is not a functional problem, but it may hide incremental processing problems, because when status changes in Connection table, which is referenced by SB_Global, ovn-northd will receive notifications for SB_Global change, which triggers recompute. Some tests may pass when this happens, while it should have failed if the recompute were not triggered. An example is the test case "testing propagate Port_Binding.up to NB and OVS", which has passed in most cases but fails permanently afte this change (and will be fixed in a separate patch). Signed-off-by: Han Zhou <hzhou@ovn.org> --- northd/ovn-northd.c | 4 ++++ 1 file changed, 4 insertions(+)