Hi! The OVN project recommends upgrading controllers first, then
updating ovn-northd and ovsdb-servers. However, some deployments might
upgrade ovn-northd before ovn-controllers, which can cause downtime in
large-scale environments if older controllers don’t understand new
logical flows or actions from ovn-northd. I made a patch to validate
currently supported actions in the SB, but I don’t like storing them in
the sb_global table. I’ve thought of three options: 1) Keep the current
approach (storing in sb_global)—pro: uses existing string-array
structures, con: requires schema changes; 2) Store them in options —con:
awkward to parse smap; or 3) Create a dedicated SB table, which feels
more extensible. Ideally, we’d validate not just actions but their
arguments too, though I haven’t settled on a clean design for that: what
is the best way to implement the argument signature. A separate table
seems better for future flexibility. We should also validate
ingress/egress OpenFlow table numbering if the NB changes. I’d love
feedback on these options!
On 28.05.2025 23:59, Alexandra Rukomoinikova wrote:
> According to OVN, northd should be updated before controllers
> since northd may introduce new actions that older controllers don't support.
> This commit adds an optional validation step to ensure controllers are
> compatible with northd actions before proceeding with upgrades.
>
> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud>
> ---
> controller/ovn-controller.c | 73 +++++++++++++++++++++++++++++++++++++
> include/ovn/actions.h | 9 +++++
> northd/en-global-config.c | 21 +++++++++++
> ovn-sb.ovsschema | 5 ++-
> ovn-sb.xml | 4 ++
> tests/ovn.at | 28 ++++++++++++++
> 6 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 89207bb4b..0b665f28a 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -173,6 +173,8 @@ struct pending_pkt {
> /* Registered ofctrl seqno type for nb_cfg propagation. */
> static size_t ofctrl_seq_type_nb_cfg;
>
> +static bool conntroller_validate_actions = false;
> +
> static void
> remove_newline(char *s)
> {
> @@ -203,6 +205,57 @@ static char *get_file_system_id(void)
> free(filename);
> return ret;
> }
> +
> +static void
> +log_actions_diff(const void *arg OVS_UNUSED,
> + const char *item, bool northd_supported)
> +{
> + VLOG_WARN("Validate actions: action %s supported by %s "
> + "but not supported by %s.", item,
> + northd_supported ? "northd" : "controller",
> + northd_supported ? "controller" : "northd");
> +}
> +
> +static void
> +validate_compatibility_with_northd(const struct sbrec_sb_global *sb,
> + bool sb_global_upgraded)
> +{
> + static bool executed = false;
> +
> + if (executed) {
> + return;
> + }
> +
> + if (!sb || !sb->n_supported_actions) {
> + VLOG_WARN("Validate actions: %s",
> + !sb ? "No SB Global record found." :
> + "No northd actions available in SB Global.");
> + return;
> + }
> + struct svec controller_action_entries = SVEC_EMPTY_INITIALIZER;
> + ovn_action_get_names(&controller_action_entries);
> +
> + struct sorted_array controller_actions =
> + sorted_array_from_svec(&controller_action_entries);
> +
> + struct sorted_array northd_actions =
> + sorted_array_create((const char **) sb->supported_actions,
> + sb->n_supported_actions,
> + false);
> +
> + sorted_array_apply_diff(&northd_actions,
> + &controller_actions,
> + log_actions_diff, NULL);
> +
> + VLOG_WARN("Validate actions: All northd actions are validated. ");
> +
> + executed = sb_global_upgraded ? false : true;
> +
> + sorted_array_destroy(&northd_actions);
> + sorted_array_destroy(&controller_actions);
> + svec_destroy(&controller_action_entries);
> +}
> +
> /* Only set monitor conditions on tables that are available in the
> * server schema.
> */
> @@ -755,6 +808,18 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl
> *ovnsb_idl,
> if (monitor_all_p) {
> *monitor_all_p = monitor_all;
> }
> +
> + /* Check actions compatibility when updating SB_DB. */
> + conntroller_validate_actions =
> + get_chassis_external_id_value_bool(
> + &cfg->external_ids, chassis_id,
> + "validate-northd-actions", false);
> +
> + if (conntroller_validate_actions) {
> + const struct sbrec_sb_global *sb = sbrec_sb_global_first(ovnsb_idl);
> + validate_compatibility_with_northd(sb, false);
> + }
> +
> if (reset_ovnsb_idl_min_index && *reset_ovnsb_idl_min_index) {
> VLOG_INFO("Resetting southbound database cluster state");
> engine_set_force_recompute();
> @@ -3619,6 +3684,10 @@ en_northd_options_run(struct engine_node *node, void
> *data)
> true)
> : true;
>
> + if (conntroller_validate_actions) {
> + validate_compatibility_with_northd(sb_global, false);
> + }
> +
> return EN_UPDATED;
> }
>
> @@ -3676,6 +3745,10 @@ en_northd_options_sb_sb_global_handler(struct
> engine_node *node, void *data)
> result = EN_HANDLED_UPDATED;
> }
>
> + if (conntroller_validate_actions) {
> + validate_compatibility_with_northd(sb_global, false);
> + }
> +
> return result;
> }
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 055d3b26c..3b3912152 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -23,6 +23,7 @@
> #include "expr.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/hmap.h"
> +#include "lib/svec.h"
> #include "openvswitch/ofp-actions.h"
> #include "openvswitch/uuid.h"
> #include "util.h"
> @@ -152,6 +153,14 @@ enum {
> #undef OVNACT
> };
>
> +static inline void
> +ovn_action_get_names(struct svec *ovnacts)
> +{
> + #define OVNACT(ENUM, STRUCT) svec_add(ovnacts, #ENUM);
> + OVNACTS
> + #undef OVNACT
> +}
> +
> /* Header for an action.
> *
> * Each action is a structure "struct ovnact_*" that begins with "struct
> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
> index ee5adfd3b..19b05f7e8 100644
> --- a/northd/en-global-config.c
> +++ b/northd/en-global-config.c
> @@ -26,6 +26,7 @@
> #include "en-global-config.h"
> #include "en-sampling-app.h"
> #include "include/ovn/features.h"
> +#include "include/ovn/actions.h"
> #include "ipam.h"
> #include "lib/ovn-nb-idl.h"
> #include "lib/ovn-sb-idl.h"
> @@ -51,6 +52,7 @@ static void update_sb_config_options_to_sbrec(struct
> ed_type_global_config *,
> const struct sbrec_sb_global
> *);
> static bool is_vxlan_mode(const struct smap *nb_options,
> const struct sbrec_chassis_table *);
> +static void set_supported_actions_to_sbrec(const struct sbrec_sb_global *sb);
>
> void *
> en_global_config_init(struct engine_node *node OVS_UNUSED,
> @@ -179,6 +181,8 @@ en_global_config_run(struct engine_node *node , void
> *data)
> /* Set up SB_Global (depends on chassis features). */
> update_sb_config_options_to_sbrec(config_data, sb);
>
> + set_supported_actions_to_sbrec(sb);
> +
> return EN_UPDATED;
> }
>
> @@ -713,3 +717,20 @@ is_vxlan_mode(const struct smap *nb_options,
> }
> return false;
> }
> +
> +static void
> +set_supported_actions_to_sbrec(const struct sbrec_sb_global *sb)
> +{
> + struct svec ovnacts = SVEC_EMPTY_INITIALIZER;
> +
> + ovn_action_get_names(&ovnacts);
> +
> + struct sorted_array ovnacts_sorted =
> + sorted_array_from_svec(&ovnacts);
> +
> + sbrec_sb_global_set_supported_actions(sb, ovnacts_sorted.arr,
> + ovnacts_sorted.n);
> +
> + sorted_array_destroy(&ovnacts_sorted);
> + svec_destroy(&ovnacts);
> +}
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 4c24f5b51..c956ef888 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> "version": "21.2.0",
> - "cksum": "29145795 34859",
> + "cksum": "2732310741 35034",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -18,6 +18,9 @@
> "type": {"key": {"type": "uuid",
> "refTable": "SSL"},
> "min": 0, "max": 1}},
> + "supported_actions" : {"type": {"key": "string",
> + "min": 0,
> + "max": "unlimited"}},
> "options": {
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"}},
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index db5faac66..d4924c3f8 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -165,6 +165,10 @@
>
> <column name="options">
> </column>
> +
> + <column name="supported_actions">
> + Logical flow actions supported by northd.
> + </column>
> </group>
>
> <group title="Common options">
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 46a0f09bc..1f9a1fa88 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -43500,3 +43500,31 @@ AT_CHECK([grep -q "Postponing virtual lport sw0-vir"
> hv3/ovn-controller.log], [1
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Controller actions validating])
> +ovn_start
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl add-port br-int vif1 -- \
> + set Interface vif1 external-ids:iface-id=lp1 \
> + options:tx_pcap=hv1/vif1-tx.pcap options:rxq_pcap=hv1/vif1-rx.pcap
> \
> + ofport-request=1
> +
> +# Since from the tests we can't check the situation when the controller
> doesn't support all
> +# northd actions - we will check the opposite situation.
> +check ovn-sbctl remove SB_G . supported_actions MOVE
> +
> +check ovs-vsctl set Open_Vswitch . external_ids:validate-northd-actions=true
> +
> +AT_CHECK([grep -o "action MOVE supported by controller but not supported by
> northd." hv1/ovn-controller.log], [0], [dnl
> +action MOVE supported by controller but not supported by northd.
> +])
> +AT_CLEANUP
> +])
> +
--
regards,
Alexandra.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
