| Message ID | 169175669719.2569966.3097273058540951682.stgit@dceara.remote.csb |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix Southbound conditional monitoring for known tables. | expand |
| 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 | fail | github build: failed |
On Fri, Aug 11, 2023 at 5:25 AM Dumitru Ceara <dceara@redhat.com> wrote: > > It's safe to assume that tables that existed in the previous LTS branch > first release (currently 22.03.0) can be monitored directly. Do so and > only "optionally" monitor the ones that have been added since. > > This way we avoid the need for the IDL to expose an API to change the > default condition for monitored tables. It also avoids complex code in > ovn-controller because we'd otherwise have to explicitly re-initialize > conditions to a non-default (false) value after every SB reconnect. > > NOTE: In order to make sure that pre-existing L3 and L2 gateways are not > initially considered "non-local" we explicitly request for all port > bindings of this type to be monitored in the startup stage (before we > got the initial contents of the database and our chassis record). > The commit subject and message had a good introduction about the solution but it would be better to at least add a brief description of the problem (about the memory spike) before the first paragraph. It would also be good to add a Fixes tag for commit 1b0dbde. Otherwise it looks good to me! Acked-by: Han Zhou <hzhou@ovn.org> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406201.html > Reported-by: Vladislav Odintsov <odivlad@gmail.com> > CC: Ales Musil <amusil@redhat.com> > CC: Han Zhou <hzhou@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ovn-controller.c | 40 ++++++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 1bba7715d7..0c975dc5ec 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -190,11 +190,15 @@ static char *get_file_system_id(void) > /* Only set monitor conditions on tables that are available in the > * server schema. > */ > -#define sb_table_set_monitor_condition(idl, table, cond) \ > - (sbrec_server_has_##table##_table(idl) \ > - ? sbrec_##table##_set_condition(idl, cond) \ > +#define sb_table_set_opt_mon_condition(idl, table, cond) \ > + (sbrec_server_has_##table##_table(idl) \ > + ? sbrec_##table##_set_condition(idl, cond) \ > : 0) > > +/* Assume the table exists in the server schema and set its condition. */ > +#define sb_table_set_req_mon_condition(idl, table, cond) \ > + sbrec_##table##_set_condition(idl, cond) > + > static unsigned int > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > const struct sbrec_chassis *chassis, > @@ -294,6 +298,14 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > /* During initialization, we monitor all records in Chassis_Private so > * that we don't try to recreate existing ones. */ > ovsdb_idl_condition_add_clause_true(&chprv); > + /* Also, to avoid traffic disruption (e.g., conntrack flushing for > + * zones that are used by OVN but not yet known due to the SB initial > + * contents not being available), monitor all port bindings > + * connected to gateways; they might be claimed as soon as the > + * chassis is available. > + */ > + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway"); > + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway"); > } > > if (local_ifaces) { > @@ -342,17 +354,17 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > out:; > unsigned int cond_seqnos[] = { > - sb_table_set_monitor_condition(ovnsb_idl, port_binding, &pb), > - sb_table_set_monitor_condition(ovnsb_idl, logical_flow, &lf), > - sb_table_set_monitor_condition(ovnsb_idl, logical_dp_group, &ldpg), > - sb_table_set_monitor_condition(ovnsb_idl, mac_binding, &mb), > - sb_table_set_monitor_condition(ovnsb_idl, multicast_group, &mg), > - sb_table_set_monitor_condition(ovnsb_idl, dns, &dns), > - sb_table_set_monitor_condition(ovnsb_idl, controller_event, &ce), > - sb_table_set_monitor_condition(ovnsb_idl, ip_multicast, &ip_mcast), > - sb_table_set_monitor_condition(ovnsb_idl, igmp_group, &igmp), > - sb_table_set_monitor_condition(ovnsb_idl, chassis_private, &chprv), > - sb_table_set_monitor_condition(ovnsb_idl, chassis_template_var, &tv), > + sb_table_set_req_mon_condition(ovnsb_idl, port_binding, &pb), > + sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, &lf), > + sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group, &ldpg), > + sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, &mb), > + sb_table_set_req_mon_condition(ovnsb_idl, multicast_group, &mg), > + sb_table_set_req_mon_condition(ovnsb_idl, dns, &dns), > + sb_table_set_req_mon_condition(ovnsb_idl, controller_event, &ce), > + sb_table_set_req_mon_condition(ovnsb_idl, ip_multicast, &ip_mcast), > + sb_table_set_req_mon_condition(ovnsb_idl, igmp_group, &igmp), > + sb_table_set_req_mon_condition(ovnsb_idl, chassis_private, &chprv), > + sb_table_set_opt_mon_condition(ovnsb_idl, chassis_template_var, &tv), > }; > > unsigned int expected_cond_seqno = 0; >
On Fri, Aug 11, 2023 at 2:25 PM Dumitru Ceara <dceara@redhat.com> wrote: > It's safe to assume that tables that existed in the previous LTS branch > first release (currently 22.03.0) can be monitored directly. Do so and > only "optionally" monitor the ones that have been added since. > > This way we avoid the need for the IDL to expose an API to change the > default condition for monitored tables. It also avoids complex code in > ovn-controller because we'd otherwise have to explicitly re-initialize > conditions to a non-default (false) value after every SB reconnect. > > NOTE: In order to make sure that pre-existing L3 and L2 gateways are not > initially considered "non-local" we explicitly request for all port > bindings of this type to be monitored in the startup stage (before we > got the initial contents of the database and our chassis record). > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406201.html > Reported-by: Vladislav Odintsov <odivlad@gmail.com> > CC: Ales Musil <amusil@redhat.com> > CC: Han Zhou <hzhou@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ovn-controller.c | 40 > ++++++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 1bba7715d7..0c975dc5ec 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -190,11 +190,15 @@ static char *get_file_system_id(void) > /* Only set monitor conditions on tables that are available in the > * server schema. > */ > -#define sb_table_set_monitor_condition(idl, table, cond) \ > - (sbrec_server_has_##table##_table(idl) \ > - ? sbrec_##table##_set_condition(idl, cond) \ > +#define sb_table_set_opt_mon_condition(idl, table, cond) \ > + (sbrec_server_has_##table##_table(idl) \ > + ? sbrec_##table##_set_condition(idl, cond) \ > : 0) > > +/* Assume the table exists in the server schema and set its condition. */ > +#define sb_table_set_req_mon_condition(idl, table, cond) \ > + sbrec_##table##_set_condition(idl, cond) > + > static unsigned int > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > const struct sbrec_chassis *chassis, > @@ -294,6 +298,14 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > /* During initialization, we monitor all records in > Chassis_Private so > * that we don't try to recreate existing ones. */ > ovsdb_idl_condition_add_clause_true(&chprv); > + /* Also, to avoid traffic disruption (e.g., conntrack flushing for > + * zones that are used by OVN but not yet known due to the SB > initial > + * contents not being available), monitor all port bindings > + * connected to gateways; they might be claimed as soon as the > + * chassis is available. > + */ > + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway"); > + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway"); > } > > if (local_ifaces) { > @@ -342,17 +354,17 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > out:; > unsigned int cond_seqnos[] = { > - sb_table_set_monitor_condition(ovnsb_idl, port_binding, &pb), > - sb_table_set_monitor_condition(ovnsb_idl, logical_flow, &lf), > - sb_table_set_monitor_condition(ovnsb_idl, logical_dp_group, > &ldpg), > - sb_table_set_monitor_condition(ovnsb_idl, mac_binding, &mb), > - sb_table_set_monitor_condition(ovnsb_idl, multicast_group, &mg), > - sb_table_set_monitor_condition(ovnsb_idl, dns, &dns), > - sb_table_set_monitor_condition(ovnsb_idl, controller_event, &ce), > - sb_table_set_monitor_condition(ovnsb_idl, ip_multicast, > &ip_mcast), > - sb_table_set_monitor_condition(ovnsb_idl, igmp_group, &igmp), > - sb_table_set_monitor_condition(ovnsb_idl, chassis_private, > &chprv), > - sb_table_set_monitor_condition(ovnsb_idl, chassis_template_var, > &tv), > + sb_table_set_req_mon_condition(ovnsb_idl, port_binding, &pb), > + sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, &lf), > + sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group, > &ldpg), > + sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, &mb), > + sb_table_set_req_mon_condition(ovnsb_idl, multicast_group, &mg), > + sb_table_set_req_mon_condition(ovnsb_idl, dns, &dns), > + sb_table_set_req_mon_condition(ovnsb_idl, controller_event, &ce), > + sb_table_set_req_mon_condition(ovnsb_idl, ip_multicast, > &ip_mcast), > + sb_table_set_req_mon_condition(ovnsb_idl, igmp_group, &igmp), > + sb_table_set_req_mon_condition(ovnsb_idl, chassis_private, > &chprv), > + sb_table_set_opt_mon_condition(ovnsb_idl, chassis_template_var, > &tv), > }; > > unsigned int expected_cond_seqno = 0; > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 8/14/23 00:24, Han Zhou wrote: > On Fri, Aug 11, 2023 at 5:25 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> It's safe to assume that tables that existed in the previous LTS branch >> first release (currently 22.03.0) can be monitored directly. Do so and >> only "optionally" monitor the ones that have been added since. >> >> This way we avoid the need for the IDL to expose an API to change the >> default condition for monitored tables. It also avoids complex code in >> ovn-controller because we'd otherwise have to explicitly re-initialize >> conditions to a non-default (false) value after every SB reconnect. >> >> NOTE: In order to make sure that pre-existing L3 and L2 gateways are not >> initially considered "non-local" we explicitly request for all port >> bindings of this type to be monitored in the startup stage (before we >> got the initial contents of the database and our chassis record). >> > > The commit subject and message had a good introduction about the solution > but it would be better to at least add a brief description of the problem > (about the memory spike) before the first paragraph. > It would also be good to add a Fixes tag for commit 1b0dbde. Ack, I did that now. > Otherwise it looks good to me! > > Acked-by: Han Zhou <hzhou@ovn.org> > Thanks, applied to main and backported to all branches down to 22.03. Regards, Dumitru
Thanks Dumitru for the efforts! > On 15 Aug 2023, at 13:39, Dumitru Ceara <dceara@redhat.com> wrote: > > On 8/14/23 00:24, Han Zhou wrote: >> On Fri, Aug 11, 2023 at 5:25 AM Dumitru Ceara <dceara@redhat.com> wrote: >>> >>> It's safe to assume that tables that existed in the previous LTS branch >>> first release (currently 22.03.0) can be monitored directly. Do so and >>> only "optionally" monitor the ones that have been added since. >>> >>> This way we avoid the need for the IDL to expose an API to change the >>> default condition for monitored tables. It also avoids complex code in >>> ovn-controller because we'd otherwise have to explicitly re-initialize >>> conditions to a non-default (false) value after every SB reconnect. >>> >>> NOTE: In order to make sure that pre-existing L3 and L2 gateways are not >>> initially considered "non-local" we explicitly request for all port >>> bindings of this type to be monitored in the startup stage (before we >>> got the initial contents of the database and our chassis record). >>> >> >> The commit subject and message had a good introduction about the solution >> but it would be better to at least add a brief description of the problem >> (about the memory spike) before the first paragraph. >> It would also be good to add a Fixes tag for commit 1b0dbde. > > Ack, I did that now. > >> Otherwise it looks good to me! >> >> Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> >> > > Thanks, applied to main and backported to all branches down to 22.03. > > Regards, > Dumitru Regards, Vladislav Odintsov
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 1bba7715d7..0c975dc5ec 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -190,11 +190,15 @@ static char *get_file_system_id(void) /* Only set monitor conditions on tables that are available in the * server schema. */ -#define sb_table_set_monitor_condition(idl, table, cond) \ - (sbrec_server_has_##table##_table(idl) \ - ? sbrec_##table##_set_condition(idl, cond) \ +#define sb_table_set_opt_mon_condition(idl, table, cond) \ + (sbrec_server_has_##table##_table(idl) \ + ? sbrec_##table##_set_condition(idl, cond) \ : 0) +/* Assume the table exists in the server schema and set its condition. */ +#define sb_table_set_req_mon_condition(idl, table, cond) \ + sbrec_##table##_set_condition(idl, cond) + static unsigned int update_sb_monitors(struct ovsdb_idl *ovnsb_idl, const struct sbrec_chassis *chassis, @@ -294,6 +298,14 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, /* During initialization, we monitor all records in Chassis_Private so * that we don't try to recreate existing ones. */ ovsdb_idl_condition_add_clause_true(&chprv); + /* Also, to avoid traffic disruption (e.g., conntrack flushing for + * zones that are used by OVN but not yet known due to the SB initial + * contents not being available), monitor all port bindings + * connected to gateways; they might be claimed as soon as the + * chassis is available. + */ + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway"); + sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway"); } if (local_ifaces) { @@ -342,17 +354,17 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, out:; unsigned int cond_seqnos[] = { - sb_table_set_monitor_condition(ovnsb_idl, port_binding, &pb), - sb_table_set_monitor_condition(ovnsb_idl, logical_flow, &lf), - sb_table_set_monitor_condition(ovnsb_idl, logical_dp_group, &ldpg), - sb_table_set_monitor_condition(ovnsb_idl, mac_binding, &mb), - sb_table_set_monitor_condition(ovnsb_idl, multicast_group, &mg), - sb_table_set_monitor_condition(ovnsb_idl, dns, &dns), - sb_table_set_monitor_condition(ovnsb_idl, controller_event, &ce), - sb_table_set_monitor_condition(ovnsb_idl, ip_multicast, &ip_mcast), - sb_table_set_monitor_condition(ovnsb_idl, igmp_group, &igmp), - sb_table_set_monitor_condition(ovnsb_idl, chassis_private, &chprv), - sb_table_set_monitor_condition(ovnsb_idl, chassis_template_var, &tv), + sb_table_set_req_mon_condition(ovnsb_idl, port_binding, &pb), + sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, &lf), + sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group, &ldpg), + sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, &mb), + sb_table_set_req_mon_condition(ovnsb_idl, multicast_group, &mg), + sb_table_set_req_mon_condition(ovnsb_idl, dns, &dns), + sb_table_set_req_mon_condition(ovnsb_idl, controller_event, &ce), + sb_table_set_req_mon_condition(ovnsb_idl, ip_multicast, &ip_mcast), + sb_table_set_req_mon_condition(ovnsb_idl, igmp_group, &igmp), + sb_table_set_req_mon_condition(ovnsb_idl, chassis_private, &chprv), + sb_table_set_opt_mon_condition(ovnsb_idl, chassis_template_var, &tv), }; unsigned int expected_cond_seqno = 0;
It's safe to assume that tables that existed in the previous LTS branch first release (currently 22.03.0) can be monitored directly. Do so and only "optionally" monitor the ones that have been added since. This way we avoid the need for the IDL to expose an API to change the default condition for monitored tables. It also avoids complex code in ovn-controller because we'd otherwise have to explicitly re-initialize conditions to a non-default (false) value after every SB reconnect. NOTE: In order to make sure that pre-existing L3 and L2 gateways are not initially considered "non-local" we explicitly request for all port bindings of this type to be monitored in the startup stage (before we got the initial contents of the database and our chassis record). Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406201.html Reported-by: Vladislav Odintsov <odivlad@gmail.com> CC: Ales Musil <amusil@redhat.com> CC: Han Zhou <hzhou@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ovn-controller.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)