On Thu, Mar 9, 2023 at 9:21 AM Dumitru Ceara <[email protected]> wrote: > > On 3/8/23 15:48, Ales Musil wrote: > > On Wed, Mar 8, 2023 at 3:44 PM Dumitru Ceara <[email protected]> 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 <[email protected]> > >> Signed-off-by: Dumitru Ceara <[email protected]> > >> --- > >> V2: > >> - Addressed Ales' comment: > >> - explicitly set remote chassis features as unsupported in the test. > >> --- > >> northd/inc-proc-northd.c | 26 ++++++++++++++++++++++++++ > >> northd/northd.c | 7 +++++++ > >> tests/ovn-northd.at | 36 ++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 69 insertions(+) > >> > >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > >> index 1c8550df5d..b28f551895 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") \ > >> @@ -305,6 +308,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. */ > >> @@ -355,3 +364,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 ab10fea6ab..661f91ff7e 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 d0f6893e94..6c901a202b 100644 > >> --- a/tests/ovn-northd.at > >> +++ b/tests/ovn-northd.at > >> @@ -8650,3 +8650,39 @@ 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 \ > >> + -- set chassis hv2 other_config:ct-no-masked-label=false \ > >> + -- set chassis hv2 other_config:ovn-ct-lb-related=false \ > >> + -- set chassis hv2 other_config:mac-binding-timestamp=false > >> + > >> +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 > >> > >> > > Looks good to me, thanks. > > > > Acked-by: Ales Musil <[email protected]> > > > > Thanks for the review! I pushed this to the main branch.
Thanks Dumitru for the patch/fix. I'd suggest considering this commit as a fix and backporting this to OVN 23.03 (at least(. What do you think ? Numan > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
