On Fri, Jan 17, 2025 at 5:13 AM Mark Michelson <[email protected]> wrote:
> This capability is not in all OVS versions, so we need to check for it > in order to use it. > > A future commit will make use of this feature, so this lays the > groundwork for being able to confirm if chassis all support being able > to flush CT entries using label or mark bits. > > Signed-off-by: Mark Michelson <[email protected]> > --- > Hi Mark, this has a pretty trivial conflict in ovn-controller.at, I have just one small comment down below. v4 -> v5: > * Fixed failing tests > > v3 -> v4: > * Rebased, but no functional changes > > v2 -> v3: > * No changes > > v1 -> v2: > * Added this patch to the series > --- > controller/chassis.c | 15 +++++++++++ > include/ovn/features.h | 3 +++ > lib/features.c | 57 ++++++++++++++++++++++++++++++++++++++- > northd/en-global-config.c | 10 +++++++ > northd/en-global-config.h | 1 + > tests/ovn-controller.at | 8 +++--- > 6 files changed, 89 insertions(+), 5 deletions(-) > > diff --git a/controller/chassis.c b/controller/chassis.c > index 19a251f26..aa5980d92 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -71,6 +71,8 @@ struct ovs_chassis_cfg { > bool is_interconn; > /* Does OVS support sampling with ids taken from registers? */ > bool sample_with_regs; > + /* Does OVS support flushing CT zones using label/mark? */ > + bool ct_label_flush; > }; > > static void > @@ -358,6 +360,8 @@ chassis_parse_ovs_config(const struct > ovsrec_open_vswitch_table *ovs_table, > ovs_cfg->is_interconn = get_is_interconn(&cfg->external_ids, > chassis_id); > ovs_cfg->sample_with_regs = > ovs_feature_is_supported(OVS_SAMPLE_REG_SUPPORT); > + ovs_cfg->ct_label_flush = > + ovs_feature_is_supported(OVS_CT_LABEL_FLUSH_SUPPORT); > > return true; > } > @@ -395,6 +399,8 @@ chassis_build_other_config(const struct > ovs_chassis_cfg *ovs_cfg, > smap_replace(config, OVN_FEATURE_SAMPLE_WITH_REGISTERS, > ovs_cfg->sample_with_regs ? "true" : "false"); > smap_replace(config, OVN_FEATURE_CT_NEXT_ZONE, "true"); > + smap_replace(config, OVN_FEATURE_CT_LABEL_FLUSH, > + ovs_cfg->ct_label_flush ? "true" :"false"); > } > > /* > @@ -560,6 +566,14 @@ chassis_other_config_changed(const struct > ovs_chassis_cfg *ovs_cfg, > return true; > } > > + bool chassis_ct_label_flush = > + smap_get_bool(&chassis_rec->other_config, > + OVN_FEATURE_CT_LABEL_FLUSH, > + false); > + if (chassis_ct_label_flush != ovs_cfg->ct_label_flush) { > + return true; > + } > + > return false; > } > > @@ -718,6 +732,7 @@ update_supported_sset(struct sset *supported) > sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); > sset_add(supported, OVN_FEATURE_SAMPLE_WITH_REGISTERS); > sset_add(supported, OVN_FEATURE_CT_NEXT_ZONE); > + sset_add(supported, OVN_FEATURE_CT_LABEL_FLUSH); > } > > static void > diff --git a/include/ovn/features.h b/include/ovn/features.h > index 3566ab60f..94595ba77 100644 > --- a/include/ovn/features.h > +++ b/include/ovn/features.h > @@ -31,6 +31,7 @@ > #define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" > #define OVN_FEATURE_SAMPLE_WITH_REGISTERS "ovn-sample-with-registers" > #define OVN_FEATURE_CT_NEXT_ZONE "ct-next-zone" > +#define OVN_FEATURE_CT_LABEL_FLUSH "ct-label-flush" > > /* OVS datapath supported features. Based on availability OVN might > generate > * different types of openflows. > @@ -42,6 +43,7 @@ enum ovs_feature_support_bits { > OVS_DP_HASH_L4_SYM_BIT, > OVS_OF_GROUP_SUPPORT_BIT, > OVS_SAMPLE_REG_SUPPORT_BIT, > + OVS_CT_LABEL_FLUSH_BIT, > }; > > enum ovs_feature_value { > @@ -51,6 +53,7 @@ enum ovs_feature_value { > OVS_DP_HASH_L4_SYM_SUPPORT = (1 << OVS_DP_HASH_L4_SYM_BIT), > OVS_OF_GROUP_SUPPORT = (1 << OVS_OF_GROUP_SUPPORT_BIT), > OVS_SAMPLE_REG_SUPPORT = (1 << OVS_SAMPLE_REG_SUPPORT_BIT), > + OVS_CT_LABEL_FLUSH_SUPPORT = (1 << OVS_CT_LABEL_FLUSH_BIT), > }; > > void ovs_feature_support_destroy(void); > diff --git a/lib/features.c b/lib/features.c > index 98d56602c..11448bba8 100644 > --- a/lib/features.c > +++ b/lib/features.c > @@ -32,6 +32,7 @@ > #include "openvswitch/ofp-group.h" > #include "openvswitch/ofp-print.h" > #include "openvswitch/ofp-util.h" > +#include "openvswitch/ofp-ct.h" > #include "openvswitch/rconn.h" > #include "ovn/features.h" > #include "controller/ofctrl.h" > @@ -268,6 +269,52 @@ sample_with_reg_handle_barrier(struct > ovs_openflow_feature *feature OVS_UNUSED) > return true; > } > > +static void > +ct_label_flush_send_request(struct ovs_openflow_feature *feature) > +{ > + /* At the time of this code being written, the highest bits > + * of the CT label are not used for anything. Setting these > + * is most likely to minimize flushing a valid CT entry by > + * mistake. > + */ > + ovs_u128 ct_mask = { > + .u64.hi = 0x10000000, > + }; > + ovs_u128 ct_value = { > + .u64.hi = 0x10000000, > + }; > + struct ofp_ct_match match = { > + .labels = ct_value, > + .labels_mask = ct_mask, > + /* ICMP type 1 is unassigned. This should reduce the > + * risk of flushing legitimate CT entries by mistake > + */ > nit: The ip_proto should be set to IPPROTO_ICMP. > + .tuple_orig.icmp_type = 1, > + }; > + > + struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL, > + rconn_get_version(swconn)); > + const struct ofp_header *oh = msg->data; > + feature->xid = oh->xid; > + rconn_send(swconn, msg, NULL); > +} > + > +static bool > +ct_label_flush_handle_response(struct ovs_openflow_feature *feature, > + enum ofptype type, const struct ofp_header > *oh) > +{ > + if (type != OFPTYPE_ERROR) { > + log_unexpected_reply(feature, oh); > + } > + return false; > +} > + > +static bool > +ct_label_flush_handle_barrier(struct ovs_openflow_feature *feature > OVS_UNUSED) > +{ > + return true; > +} > + > static struct ovs_openflow_feature all_openflow_features[] = { > { > .value = OVS_DP_METER_SUPPORT, > @@ -289,7 +336,14 @@ static struct ovs_openflow_feature > all_openflow_features[] = { > .send_request = sample_with_reg_send_request, > .handle_response = sample_with_reg_handle_response, > .handle_barrier = sample_with_reg_handle_barrier, > - } > + }, > + { > + .value = OVS_CT_LABEL_FLUSH_SUPPORT, > + .name = "ct_label_flush", > + .send_request = ct_label_flush_send_request, > + .handle_response = ct_label_flush_handle_response, > + .handle_barrier = ct_label_flush_handle_barrier, > + }, > }; > > static bool > @@ -362,6 +416,7 @@ ovs_feature_is_valid(enum ovs_feature_value feature) > case OVS_DP_HASH_L4_SYM_SUPPORT: > case OVS_OF_GROUP_SUPPORT: > case OVS_SAMPLE_REG_SUPPORT: > + case OVS_CT_LABEL_FLUSH_SUPPORT: > return true; > default: > return false; > diff --git a/northd/en-global-config.c b/northd/en-global-config.c > index fff2aaa16..ffe462b7c 100644 > --- a/northd/en-global-config.c > +++ b/northd/en-global-config.c > @@ -383,6 +383,7 @@ northd_enable_all_features(struct > ed_type_global_config *data) > .ct_commit_to_zone = true, > .sample_with_reg = true, > .ct_next_zone = true, > + .ct_label_flush = true, > }; > } > > @@ -462,6 +463,15 @@ build_chassis_features(const struct > sbrec_chassis_table *sbrec_chassis_table, > chassis_features->ct_next_zone) { > chassis_features->ct_next_zone = false; > } > + > + bool ct_label_flush = > + smap_get_bool(&chassis->other_config, > + OVN_FEATURE_CT_LABEL_FLUSH, > + false); > + if (!ct_label_flush && > + chassis_features->ct_label_flush) { > + chassis_features->ct_label_flush = false; > + } > } > } > > diff --git a/northd/en-global-config.h b/northd/en-global-config.h > index 767810542..a95ba2a6c 100644 > --- a/northd/en-global-config.h > +++ b/northd/en-global-config.h > @@ -21,6 +21,7 @@ struct chassis_features { > bool ct_commit_to_zone; > bool sample_with_reg; > bool ct_next_zone; > + bool ct_label_flush; > }; > > struct global_config_tracked_data { > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 2bf8b53d0..f0bac1723 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3221,7 +3221,7 @@ check_ct_zone_max 22 > AS_BOX([Check LR snat requested zone 2]) > AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk > '/lr_snat/{print $2}') -eq 2]) > > -n_flush=$(grep -c -i flush hv1/ovs-vswitchd.log) > +n_flush=$(grep -c -i ct_flush_zone hv1/ovs-vswitchd.log) > check ovn-appctl -t ovn-controller exit --restart > # Make sure ovn-controller stopped before restarting it > OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" != > "running"]) > @@ -3230,7 +3230,7 @@ wait_for_ports_up > check ovn-nbctl --wait=hv sync > > # Check we do not have unexpected ct-flush restarting ovn-controller > -AT_CHECK([test $(grep -c -i flush hv1/ovs-vswitchd.log) -eq ${n_flush}]) > +AT_CHECK([test $(grep -c -i ct_flush_zone hv1/ovs-vswitchd.log) -eq > ${n_flush}]) > > AS_BOX([Check LR snat allowed requested zone 0]) > check ovn-nbctl set logical_router lr options:snat-ct-zone=0 > @@ -3240,7 +3240,7 @@ check_ct_zone_min 0 > check_ct_zone_max 22 > AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk > '/lr_snat/{print $2}') -eq 0]) > > -n_flush=$(grep -c -i flush hv1/ovs-vswitchd.log) > +n_flush=$(grep -c -i ct_flush_zone hv1/ovs-vswitchd.log) > check ovn-appctl -t ovn-controller exit --restart > # Make sure ovn-controller stopped before restarting it > OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" != > "running"]) > @@ -3249,7 +3249,7 @@ wait_for_ports_up > check ovn-nbctl --wait=hv sync > > # Check we do not have unexpected ct-flush restarting ovn-controller > -AT_CHECK([test $(grep -c -i flush hv1/ovs-vswitchd.log) -eq ${n_flush}]) > +AT_CHECK([test $(grep -c -i ct_flush_zone hv1/ovs-vswitchd.log) -eq > ${n_flush}]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > -- > 2.45.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Other than that it looks good. Acked-by: Ales Musil <[email protected]> Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
