On 1/28/25 3:49 PM, Mark Michelson wrote:
> On 1/28/25 05:36, Xavier Simonart wrote:
>> Hi Ales
>>
>> Thanks for the patch!
>>
>> On Tue, Jan 28, 2025 at 10:45 AM Ales Musil <amu...@redhat.com> wrote:
>>
>>> The registers used in actions and lflow created by controller are
>>> hardcoded, which causes upgrade issues. When the register are changed
>>> the "old" version might be still propagated by northd until upgraded.
>>>
>>> To prevent that add flag that will be populated only by northd that
>>> is using the "new" version of registers. The controller checks
>>> this flag and uses proper registers in lflows/actions.
>>>
>>> Reported-by: Xavier Simonart <xsimo...@redhat.com>
>>> Suggested-by: Dumitru Ceara <dce...@redhat.com>
>>> Signed-off-by: Ales Musil <amu...@redhat.com>
>>> ---
>>> controller/lflow.c | 52 +++++++++++++++++++++++-------------
>>> controller/lflow.h | 1 +
>>> controller/ovn-controller.c | 19 +++++++++++++
>>> include/ovn/actions.h | 4 +++
>>> include/ovn/logical-fields.h | 7 +++++
>>> lib/actions.c | 10 +++++--
>>> northd/en-global-config.c | 4 +++
>>> tests/test-ovn.c | 1 +
>>> 8 files changed, 78 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/controller/lflow.c b/controller/lflow.c
>>> index 856261f40..a77d07d22 100644
>>> --- a/controller/lflow.c
>>> +++ b/controller/lflow.c
>>> @@ -100,7 +100,8 @@ consider_logical_flow(const struct
>>> sbrec_logical_flow
>>> *lflow,
>>> static void
>>> consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
>>> const struct hmap *local_datapaths,
>>> - struct ovn_desired_flow_table *flow_table);
>>> + struct ovn_desired_flow_table *flow_table,
>>> + bool register_consolidation);
>>>
>>> static void add_port_sec_flows(const struct shash *binding_lports,
>>> const struct sbrec_chassis *,
>>> @@ -878,6 +879,7 @@ add_matches_to_flow_table(const struct
>>> sbrec_logical_flow *lflow,
>>> .lflow_uuid = lflow->header_.uuid,
>>> .dp_key = ldp->datapath->tunnel_key,
>>> .explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
>>> + .register_consolidation = l_ctx_in->register_consolidation,
>>>
>>> .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>>> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
>>> @@ -1645,7 +1647,8 @@ static void
>>> add_lb_vip_hairpin_flows(const struct ovn_controller_lb *lb,
>>> struct ovn_lb_vip *lb_vip,
>>> struct ovn_lb_backend *lb_backend,
>>> - struct ovn_desired_flow_table *flow_table)
>>> + struct ovn_desired_flow_table *flow_table,
>>> + bool register_consolidation)
>>> {
>>> uint64_t stub[1024 / 8];
>>> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
>>> @@ -1677,9 +1680,10 @@ add_lb_vip_hairpin_flows(const struct
>>> ovn_controller_lb *lb,
>>> if (!lb->hairpin_orig_tuple) {
>>> match_set_ct_nw_dst(&hairpin_match, vip4);
>>> } else {
>>> - match_set_reg(&hairpin_match,
>>> - MFF_LOG_LB_ORIG_DIP_IPV4 - MFF_LOG_REG0,
>>> - ntohl(vip4));
>>> + enum mf_field_id id = register_consolidation
>>> + ? MFF_LOG_LB_ORIG_DIP_IPV4
>>> + : MFF_LOG_LB_ORIG_DIP_IPV4_OLD;
>>> + match_set_reg(&hairpin_match, id - MFF_LOG_REG0,
>>> ntohl(vip4));
>>> }
>>>
>>> add_lb_vip_hairpin_reply_action(NULL, snat_vip4, lb->proto,
>>> @@ -1788,7 +1792,8 @@ static void
>>> add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb,
>>> const struct ovn_lb_vip *lb_vip,
>>> const struct hmap *local_datapaths,
>>> - struct ovn_desired_flow_table
>>> *flow_table)
>>> + struct ovn_desired_flow_table
>>> *flow_table,
>>> + bool register_consolidation)
>>> {
>>> uint64_t stub[1024 / 8];
>>> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
>>> @@ -1845,8 +1850,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
>>> ovn_controller_lb *lb,
>>> if (!lb->hairpin_orig_tuple) {
>>> match_set_ct_nw_dst(&match, vip4);
>>> } else {
>>> - match_set_reg(&match, MFF_LOG_LB_ORIG_DIP_IPV4 -
>>> MFF_LOG_REG0,
>>> - ntohl(vip4));
>>> + enum mf_field_id id = register_consolidation
>>> + ? MFF_LOG_LB_ORIG_DIP_IPV4
>>> + : MFF_LOG_LB_ORIG_DIP_IPV4_OLD;
>>> + match_set_reg(&match, id - MFF_LOG_REG0, ntohl(vip4));
>>> }
>>> } else {
>>> match_set_dl_type(&match, htons(ETH_TYPE_IPV6));
>>> @@ -1922,7 +1929,8 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
>>> ovn_controller_lb *lb,
>>> static void
>>> add_lb_ct_snat_hairpin_flows(const struct ovn_controller_lb *lb,
>>> const struct hmap *local_datapaths,
>>> - struct ovn_desired_flow_table *flow_table)
>>> + struct ovn_desired_flow_table *flow_table,
>>> + bool register_consolidation)
>>> {
>>> /* We must add a flow for each LB VIP. In the general case,
>>> this flow
>>> is added to the OFTABLE_CT_SNAT_HAIRPIN table. If it
>>> matches, we
>>> @@ -1956,14 +1964,15 @@ add_lb_ct_snat_hairpin_flows(const struct
>>> ovn_controller_lb *lb,
>>>
>>> for (int i = 0; i < lb->n_vips; i++) {
>>> add_lb_ct_snat_hairpin_vip_flow(lb, &lb->vips[i],
>>> local_datapaths,
>>> - flow_table);
>>> + flow_table,
>>> register_consolidation);
>>> }
>>> }
>>>
>>> static void
>>> consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
>>> const struct hmap *local_datapaths,
>>> - struct ovn_desired_flow_table *flow_table)
>>> + struct ovn_desired_flow_table *flow_table,
>>> + bool register_consolidation)
>>> {
>>> for (size_t i = 0; i < lb->n_vips; i++) {
>>> struct ovn_lb_vip *lb_vip = &lb->vips[i];
>>> @@ -1971,11 +1980,13 @@ consider_lb_hairpin_flows(const struct
>>> ovn_controller_lb *lb,
>>> for (size_t j = 0; j < lb_vip->n_backends; j++) {
>>> struct ovn_lb_backend *lb_backend = &lb_vip->backends[j];
>>>
>>> - add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend,
>>> flow_table);
>>> + add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend,
>>> flow_table,
>>> + register_consolidation);
>>> }
>>> }
>>>
>>> - add_lb_ct_snat_hairpin_flows(lb, local_datapaths, flow_table);
>>> + add_lb_ct_snat_hairpin_flows(lb, local_datapaths, flow_table,
>>> + register_consolidation);
>>> }
>>>
>>> /* Adds OpenFlow flows to flow tables for each Load balancer VIPs and
>>> @@ -1983,11 +1994,13 @@ consider_lb_hairpin_flows(const struct
>>> ovn_controller_lb *lb,
>>> static void
>>> add_lb_hairpin_flows(const struct hmap *local_lbs,
>>> const struct hmap *local_datapaths,
>>> - struct ovn_desired_flow_table *flow_table)
>>> + struct ovn_desired_flow_table *flow_table,
>>> + bool register_consolidation)
>>> {
>>> const struct ovn_controller_lb *lb;
>>> HMAP_FOR_EACH (lb, hmap_node, local_lbs) {
>>> - consider_lb_hairpin_flows(lb, local_datapaths, flow_table);
>>> + consider_lb_hairpin_flows(lb, local_datapaths, flow_table,
>>> + register_consolidation);
>>> }
>>> }
>>>
>>> @@ -2147,7 +2160,8 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
>>> lflow_ctx_out *l_ctx_out)
>>> l_ctx_out->flow_table);
>>> add_lb_hairpin_flows(l_ctx_in->local_lbs,
>>> l_ctx_in->local_datapaths,
>>> - l_ctx_out->flow_table);
>>> + l_ctx_out->flow_table,
>>> + l_ctx_in->register_consolidation);
>>> add_fdb_flows(l_ctx_in->fdb_table, l_ctx_in->local_datapaths,
>>> l_ctx_out->flow_table,
>>> l_ctx_in->sbrec_port_binding_by_key,
>>> @@ -2401,7 +2415,8 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
>>> *l_ctx_in,
>>> UUID_FMT, UUID_ARGS(&uuid_node->uuid));
>>> ofctrl_remove_flows(l_ctx_out->flow_table, &uuid_node->uuid);
>>> consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
>>> - l_ctx_out->flow_table);
>>> + l_ctx_out->flow_table,
>>> + l_ctx_in->register_consolidation);
>>> }
>>>
>>> UUIDSET_FOR_EACH (uuid_node, new_lbs) {
>>> @@ -2410,7 +2425,8 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
>>> *l_ctx_in,
>>> VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
>>> UUID_ARGS(&uuid_node->uuid));
>>> consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
>>> - l_ctx_out->flow_table);
>>> + l_ctx_out->flow_table,
>>> + l_ctx_in->register_consolidation);
>>> }
>>>
>>> return true;
>>> diff --git a/controller/lflow.h b/controller/lflow.h
>>> index 206328f9e..93a9f3b7e 100644
>>> --- a/controller/lflow.h
>>> +++ b/controller/lflow.h
>>> @@ -135,6 +135,7 @@ struct lflow_ctx_in {
>>> bool localnet_learn_fdb;
>>> bool localnet_learn_fdb_changed;
>>> bool explicit_arp_ns_output;
>>> + bool register_consolidation;
>>> };
>>>
>>> struct lflow_ctx_out {
>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>> index 910963e25..63c787bde 100644
>>> --- a/controller/ovn-controller.c
>>> +++ b/controller/ovn-controller.c
>>> @@ -3520,6 +3520,7 @@ struct ed_type_northd_options {
>>> * switch (i.e with localnet port) should
>>> * be tunnelled or sent via the localnet
>>> * port. Default value is 'false'. */
>>> + bool register_consolidation;
>>> };
>>>
>>>
>>> @@ -3557,6 +3558,12 @@ en_northd_options_run(struct engine_node *node,
>>> void *data)
>>> false)
>>> : false;
>>>
>>> + n_opts->register_consolidation =
>>> + sb_global
>>> + ? smap_get_bool(&sb_global->options, "register_consolidation",
>>> + false)
>>> + : false;
>>> +
>>> engine_set_node_state(node, EN_UPDATED);
>>> }
>>>
>>> @@ -3591,6 +3598,17 @@ en_northd_options_sb_sb_global_handler(struct
>>> engine_node *node, void *data)
>>> engine_set_node_state(node, EN_UPDATED);
>>> }
>>>
>>> + bool register_consolidation =
>>> + sb_global
>>> + ? smap_get_bool(&sb_global->options, "register_consolidation",
>>> + false)
>>> + : false;
>>> +
>>> + if (register_consolidation != n_opts->register_consolidation) {
>>> + n_opts->register_consolidation = register_consolidation;
>>> + engine_set_node_state(node, EN_UPDATED);
>>> + }
>>> +
>>> return true;
>>> }
>>>
>>> @@ -3820,6 +3838,7 @@ init_lflow_ctx(struct engine_node *node,
>>> l_ctx_in->localnet_learn_fdb_changed =
>>> rt_data->localnet_learn_fdb_changed;
>>> l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels;
>>> l_ctx_in->explicit_arp_ns_output = n_opts->explicit_arp_ns_output;
>>> + l_ctx_in->register_consolidation = n_opts->register_consolidation;
>>> l_ctx_in->nd_ra_opts = &fo->nd_ra_opts;
>>> l_ctx_in->dhcp_opts = &dhcp_opts->v4_opts;
>>> l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts;
>>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>> index db7342f1d..3c3d06570 100644
>>> --- a/include/ovn/actions.h
>>> +++ b/include/ovn/actions.h
>>> @@ -893,6 +893,10 @@ struct ovnact_encode_params {
>>> /* Indication if we should add explicit output after arp/nd_ns
>>> action. */
>>> bool explicit_arp_ns_output;
>>>
>>> + /* Indication if we should use the "old" version of registers. */
>>> + bool register_consolidation;
>>> +
>>> +
>>> /* OVN maps each logical flow table (ltable), one-to-one, onto a
>>> physical
>>> * OpenFlow flow table (ptable). A number of parameters
>>> describe this
>>> * mapping and data related to flow tables:
>>> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
>>> index 6a87fc386..f853b1f61 100644
>>> --- a/include/ovn/logical-fields.h
>>> +++ b/include/ovn/logical-fields.h
>>> @@ -64,6 +64,13 @@ enum ovn_controller_event {
>>> #define MFF_LOG_CT_ORIG_TP_DST_PORT MFF_REG2 /*
>>> REG_ORIG_TP_DPORT
>>> * (bits
>>> 0..15). */
>>>
>>> +/* Logical registers that are needed for backwards
>>> + * compatibility with older northd versions.
>>> + * XXX: All of them can be removed in 26.09. */
>>> +#define MFF_LOG_LB_ORIG_DIP_IPV4_OLD MFF_REG1
>>> +#define MFF_LOG_LB_AFF_MATCH_PORT_OLD MFF_REG8
>>> +#define MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD MFF_XXREG0
>>> +
>>> void ovn_init_symtab(struct shash *symtab);
>>>
>>> /* MFF_LOG_FLAGS_REG bit assignments */
>>> diff --git a/lib/actions.c b/lib/actions.c
>>> index 62e73b2c5..5dbcfe324 100644
>>> --- a/lib/actions.c
>>> +++ b/lib/actions.c
>>> @@ -5371,7 +5371,10 @@ encode_COMMIT_LB_AFF(const struct
>>> ovnact_commit_lb_aff *lb_aff,
>>> imm_backend_ip = (union mf_value) {
>>> .ipv6 = lb_aff->backend,
>>> };
>>> - ol_spec->dst.field = mf_from_id(MFF_LOG_LB_AFF_MATCH_IP6_ADDR);
>>> + enum mf_field_id id = !ep->register_consolidation && ep-
>>> >is_switch
>>> + ? MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD
>>> + : MFF_LOG_LB_AFF_MATCH_IP6_ADDR;
>>> + ol_spec->dst.field = mf_from_id(id);
>>> } else {
>>> ovs_be32 ip4 = in6_addr_get_mapped_ipv4(&lb_aff->backend);
>>> imm_backend_ip = (union mf_value) {
>>> @@ -5399,7 +5402,10 @@ encode_COMMIT_LB_AFF(const struct
>>> ovnact_commit_lb_aff *lb_aff,
>>> .be16 = htons(lb_aff->backend_port),
>>> };
>>>
>>> - ol_spec->dst.field = mf_from_id(MFF_LOG_LB_AFF_MATCH_PORT);
>>> + enum mf_field_id id = !ep->register_consolidation
>>> + ? MFF_LOG_LB_AFF_MATCH_PORT_OLD
>>> + : MFF_LOG_LB_AFF_MATCH_PORT;
>>> + ol_spec->dst.field = mf_from_id(id);
>>> ol_spec->dst_type = NX_LEARN_DST_LOAD;
>>> ol_spec->src_type = NX_LEARN_SRC_IMMEDIATE;
>>> ol_spec->dst.ofs = 0;
>>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
>>> index ce16c26f2..9aef9209f 100644
>>> --- a/northd/en-global-config.c
>>> +++ b/northd/en-global-config.c
>>> @@ -590,6 +590,10 @@ update_sb_config_options_to_sbrec(struct
>>> ed_type_global_config *config_data,
>>> * arp/nd_ns action. */
>>> smap_add(options, "arp_ns_explicit_output", "true");
>>>
>>> + /* Adds indication that northd has code with consolidated
>>> + * register usage. */
>>> + smap_add(options, "register_consolidation", "true");
>>> +
>>> if (!smap_equal(&sb->options, options)) {
>>> sbrec_sb_global_set_options(sb, options);
>>> }
>>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>>> index b097ec084..c310e197e 100644
>>> --- a/tests/test-ovn.c
>>> +++ b/tests/test-ovn.c
>>> @@ -1366,6 +1366,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx
>>> OVS_UNUSED)
>>> .meter_table = &meter_table,
>>> .collector_ids = &collector_ids,
>>> .explicit_arp_ns_output = true,
>>> + .register_consolidation = true,
>>>
>>> .pipeline = OVNACT_P_INGRESS,
>>> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
>>> --
>>> 2.48.1
>>>
>>> It looks good to me.
>> I also run some tests in an "upgrade-like" scenario (old ovn-northd with
>> new ovn-controller), and they look ok.
>>
>> Acked-by: Xavier Simonart <xsimo...@redhat.com>
>> Thanks
>> Xavier
>
> Thanks for this, Ales and Xavier. https://patchwork.ozlabs.org/project/
> ovn/list/?series=441693 depends on register consolidation to function
> properly. I'll add a note on the series not to merge it until a new
> version has been posted that takes the new register_consolidation
> boolean into effect.
>
Thanks, Ales, Xavier and Mark!
Applied to main.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
