On 3/8/23 10:47, Ales Musil wrote:
> On Wed, Mar 8, 2023 at 10:20 AM 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]>
>>
>
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
