On 3/14/23 17:53, Numan Siddique wrote:
> On Thu, Mar 9, 2023 at 9:21 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>
>> On 3/8/23 15:48, Ales Musil wrote:
>>> On Wed, Mar 8, 2023 at 3:44 PM Dumitru Ceara <dce...@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 <num...@ovn.org>
>>>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>>>> ---
>>>> 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 <amu...@redhat.com>
>>>
>>
>> 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 ?
>
Sounds good. I pushed this to branch-23.03 and branch-22.12 too. It
applies cleanly there. We could do custom backports for older branches
too but I'll wait with that until someone explicitly asks for it.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
