| Message ID | 20230308092030.1035247-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: Ignore remote chassis when computing the supported feature set. | 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 |
On Wed, Mar 8, 2023 at 10:20 AM Dumitru Ceara <dceara@redhat.com> wrote: > Chassis in remote AZs are not programmed by the local ovn-northd. So we > don't need to take into account their supported feature set when > building logical flows. > > This patch also introduces a new northd unix command, > "debug/chassis-features-list". This is used by the newly added self > test but might also be useful when debugging live issues. > > Suggested-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > Hi Dumitru, just one small nit below. > --- > northd/inc-proc-northd.c | 26 ++++++++++++++++++++++++++ > northd/northd.c | 7 +++++++ > tests/ovn-northd.at | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+) > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index d23993a551..fd025c92b8 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -34,10 +34,13 @@ > #include "en-lflow.h" > #include "en-northd-output.h" > #include "en-sync-sb.h" > +#include "unixctl.h" > #include "util.h" > > VLOG_DEFINE_THIS_MODULE(inc_proc_northd); > > +static unixctl_cb_func chassis_features_list; > + > #define NB_NODES \ > NB_NODE(nb_global, "nb_global") \ > NB_NODE(copp, "copp") \ > @@ -306,6 +309,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_ovsdb_node_add_index(&en_sb_address_set, > "sbrec_address_set_by_name", > sbrec_address_set_by_name); > + > + struct northd_data *northd_data = > + engine_get_internal_data(&en_northd); > + unixctl_command_register("debug/chassis-features-list", "", 0, 0, > + chassis_features_list, > + &northd_data->features); > } > > /* Returns true if the incremental processing ended up updating nodes. */ > @@ -356,3 +365,20 @@ void inc_proc_northd_cleanup(void) > engine_cleanup(); > engine_set_context(NULL); > } > + > +static void > +chassis_features_list(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *features_) > +{ > + struct chassis_features *features = features_; > + struct ds ds = DS_EMPTY_INITIALIZER; > + > + ds_put_format(&ds, "ct_no_masked_label: %s\n", > + features->ct_no_masked_label ? "true" : "false"); > + ds_put_format(&ds, "ct_lb_related: %s\n", > + features->ct_lb_related ? "true" : "false"); > + ds_put_format(&ds, "mac_binding_timestamp: %s\n", > + features->mac_binding_timestamp ? "true" : "false"); > + unixctl_command_reply(conn, ds_cstr(&ds)); > + ds_destroy(&ds); > +} > diff --git a/northd/northd.c b/northd/northd.c > index 33025bb5c0..aed4194e7e 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -432,6 +432,13 @@ build_chassis_features(const struct northd_input > *input_data, > const struct sbrec_chassis *chassis; > > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { > + /* Only consider local AZ chassis. Remote ones don't install > + * flows generated by the local northd. > + */ > + if (smap_get_bool(&chassis->other_config, "is-remote", false)) { > + continue; > + } > + > bool ct_no_masked_label = > smap_get_bool(&chassis->other_config, > OVN_FEATURE_CT_NO_MASKED_LABEL, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 3fa02d2b3c..d5fc9e13f7 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -8649,3 +8649,36 @@ AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" > lflows2 | grep "priority=65532"], > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Chassis-feature compatibitility - remote chassis]) > +ovn_start > + > +AS_BOX([Local chassis]) > +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 \ > + -- set chassis hv1 other_config:ct-no-masked-label=true \ > + -- set chassis hv1 other_config:ovn-ct-lb-related=true \ > + -- set chassis hv1 other_config:mac-binding-timestamp=true > + > +check ovn-nbctl --wait=sb sync > + > +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE > debug/chassis-features-list], [0], [dnl > +ct_no_masked_label: true > +ct_lb_related: true > +mac_binding_timestamp: true > +]) > + > +AS_BOX([Remote chassis]) > +check ovn-sbctl chassis-add hv2 geneve 127.0.0.2 \ > + -- set chassis hv2 other_config:is-remote=true > IMO it would be better to explicitly set all features to false here, so it is clear right away that remote chassis shouldn't affect that. > + > +check ovn-nbctl --wait=sb sync > + > +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE > debug/chassis-features-list], [0], [dnl > +ct_no_masked_label: true > +ct_lb_related: true > +mac_binding_timestamp: true > +]) > + > +AT_CLEANUP > +]) > -- > 2.31.1 > > Thanks, Ales
On 3/8/23 10:47, Ales Musil wrote: > On Wed, Mar 8, 2023 at 10:20 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> Chassis in remote AZs are not programmed by the local ovn-northd. So we >> don't need to take into account their supported feature set when >> building logical flows. >> >> This patch also introduces a new northd unix command, >> "debug/chassis-features-list". This is used by the newly added self >> test but might also be useful when debugging live issues. >> >> Suggested-by: Numan Siddique <numans@ovn.org> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> > > Hi Dumitru, > just one small nit below. > Thanks for the review! > >> --- >> northd/inc-proc-northd.c | 26 ++++++++++++++++++++++++++ >> northd/northd.c | 7 +++++++ >> tests/ovn-northd.at | 33 +++++++++++++++++++++++++++++++++ >> 3 files changed, 66 insertions(+) >> >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index d23993a551..fd025c92b8 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -34,10 +34,13 @@ >> #include "en-lflow.h" >> #include "en-northd-output.h" >> #include "en-sync-sb.h" >> +#include "unixctl.h" >> #include "util.h" >> >> VLOG_DEFINE_THIS_MODULE(inc_proc_northd); >> >> +static unixctl_cb_func chassis_features_list; >> + >> #define NB_NODES \ >> NB_NODE(nb_global, "nb_global") \ >> NB_NODE(copp, "copp") \ >> @@ -306,6 +309,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_ovsdb_node_add_index(&en_sb_address_set, >> "sbrec_address_set_by_name", >> sbrec_address_set_by_name); >> + >> + struct northd_data *northd_data = >> + engine_get_internal_data(&en_northd); >> + unixctl_command_register("debug/chassis-features-list", "", 0, 0, >> + chassis_features_list, >> + &northd_data->features); >> } >> >> /* Returns true if the incremental processing ended up updating nodes. */ >> @@ -356,3 +365,20 @@ void inc_proc_northd_cleanup(void) >> engine_cleanup(); >> engine_set_context(NULL); >> } >> + >> +static void >> +chassis_features_list(struct unixctl_conn *conn, int argc OVS_UNUSED, >> + const char *argv[] OVS_UNUSED, void *features_) >> +{ >> + struct chassis_features *features = features_; >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + >> + ds_put_format(&ds, "ct_no_masked_label: %s\n", >> + features->ct_no_masked_label ? "true" : "false"); >> + ds_put_format(&ds, "ct_lb_related: %s\n", >> + features->ct_lb_related ? "true" : "false"); >> + ds_put_format(&ds, "mac_binding_timestamp: %s\n", >> + features->mac_binding_timestamp ? "true" : "false"); >> + unixctl_command_reply(conn, ds_cstr(&ds)); >> + ds_destroy(&ds); >> +} >> diff --git a/northd/northd.c b/northd/northd.c >> index 33025bb5c0..aed4194e7e 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -432,6 +432,13 @@ build_chassis_features(const struct northd_input >> *input_data, >> const struct sbrec_chassis *chassis; >> >> SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { >> + /* Only consider local AZ chassis. Remote ones don't install >> + * flows generated by the local northd. >> + */ >> + if (smap_get_bool(&chassis->other_config, "is-remote", false)) { >> + continue; >> + } >> + >> bool ct_no_masked_label = >> smap_get_bool(&chassis->other_config, >> OVN_FEATURE_CT_NO_MASKED_LABEL, >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 3fa02d2b3c..d5fc9e13f7 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -8649,3 +8649,36 @@ AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" >> lflows2 | grep "priority=65532"], >> >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([Chassis-feature compatibitility - remote chassis]) >> +ovn_start >> + >> +AS_BOX([Local chassis]) >> +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 \ >> + -- set chassis hv1 other_config:ct-no-masked-label=true \ >> + -- set chassis hv1 other_config:ovn-ct-lb-related=true \ >> + -- set chassis hv1 other_config:mac-binding-timestamp=true >> + >> +check ovn-nbctl --wait=sb sync >> + >> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE >> debug/chassis-features-list], [0], [dnl >> +ct_no_masked_label: true >> +ct_lb_related: true >> +mac_binding_timestamp: true >> +]) >> + >> +AS_BOX([Remote chassis]) >> +check ovn-sbctl chassis-add hv2 geneve 127.0.0.2 \ >> + -- set chassis hv2 other_config:is-remote=true >> > > IMO it would be better to explicitly set all features to false here, > so it is clear right away that remote chassis shouldn't affect that. > Makes sense, it's better. I posted v2. Thanks, Dumitru
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index d23993a551..fd025c92b8 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -34,10 +34,13 @@ #include "en-lflow.h" #include "en-northd-output.h" #include "en-sync-sb.h" +#include "unixctl.h" #include "util.h" VLOG_DEFINE_THIS_MODULE(inc_proc_northd); +static unixctl_cb_func chassis_features_list; + #define NB_NODES \ NB_NODE(nb_global, "nb_global") \ NB_NODE(copp, "copp") \ @@ -306,6 +309,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_ovsdb_node_add_index(&en_sb_address_set, "sbrec_address_set_by_name", sbrec_address_set_by_name); + + struct northd_data *northd_data = + engine_get_internal_data(&en_northd); + unixctl_command_register("debug/chassis-features-list", "", 0, 0, + chassis_features_list, + &northd_data->features); } /* Returns true if the incremental processing ended up updating nodes. */ @@ -356,3 +365,20 @@ void inc_proc_northd_cleanup(void) engine_cleanup(); engine_set_context(NULL); } + +static void +chassis_features_list(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *features_) +{ + struct chassis_features *features = features_; + struct ds ds = DS_EMPTY_INITIALIZER; + + ds_put_format(&ds, "ct_no_masked_label: %s\n", + features->ct_no_masked_label ? "true" : "false"); + ds_put_format(&ds, "ct_lb_related: %s\n", + features->ct_lb_related ? "true" : "false"); + ds_put_format(&ds, "mac_binding_timestamp: %s\n", + features->mac_binding_timestamp ? "true" : "false"); + unixctl_command_reply(conn, ds_cstr(&ds)); + ds_destroy(&ds); +} diff --git a/northd/northd.c b/northd/northd.c index 33025bb5c0..aed4194e7e 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -432,6 +432,13 @@ build_chassis_features(const struct northd_input *input_data, const struct sbrec_chassis *chassis; SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { + /* Only consider local AZ chassis. Remote ones don't install + * flows generated by the local northd. + */ + if (smap_get_bool(&chassis->other_config, "is-remote", false)) { + continue; + } + bool ct_no_masked_label = smap_get_bool(&chassis->other_config, OVN_FEATURE_CT_NO_MASKED_LABEL, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3fa02d2b3c..d5fc9e13f7 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8649,3 +8649,36 @@ AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows2 | grep "priority=65532"], AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Chassis-feature compatibitility - remote chassis]) +ovn_start + +AS_BOX([Local chassis]) +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 \ + -- set chassis hv1 other_config:ct-no-masked-label=true \ + -- set chassis hv1 other_config:ovn-ct-lb-related=true \ + -- set chassis hv1 other_config:mac-binding-timestamp=true + +check ovn-nbctl --wait=sb sync + +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE debug/chassis-features-list], [0], [dnl +ct_no_masked_label: true +ct_lb_related: true +mac_binding_timestamp: true +]) + +AS_BOX([Remote chassis]) +check ovn-sbctl chassis-add hv2 geneve 127.0.0.2 \ + -- set chassis hv2 other_config:is-remote=true + +check ovn-nbctl --wait=sb sync + +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE debug/chassis-features-list], [0], [dnl +ct_no_masked_label: true +ct_lb_related: true +mac_binding_timestamp: true +]) + +AT_CLEANUP +])
Chassis in remote AZs are not programmed by the local ovn-northd. So we don't need to take into account their supported feature set when building logical flows. This patch also introduces a new northd unix command, "debug/chassis-features-list". This is used by the newly added self test but might also be useful when debugging live issues. Suggested-by: Numan Siddique <numans@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/inc-proc-northd.c | 26 ++++++++++++++++++++++++++ northd/northd.c | 7 +++++++ tests/ovn-northd.at | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+)