On Fri, Nov 20, 2020 at 7:53 AM Mark Michelson <[email protected]> wrote:
>
> Hi Numan,
>
> I have one minor nit below. If you either address it or can explain it
> away, then:
>
> Acked-by: Mark Michelson <[email protected]>
>
> On 11/18/20 9:14 AM, [email protected] wrote:
> > From: Numan Siddique <[email protected]>
> >
> > OVN recommends updating/upgrading ovn-controllers first and then
> > ovn-northd and OVN DB ovsdb-servers. This is to ensure that any
> > new functionality specified by the database or logical flows created
> > by ovn-northd is understood by ovn-controller.
> >
> > However certain deployments may upgrade ovn-northd services first and
> > then ovn-controllers. In a large scal deployment, this can result in
> > downtime during upgrades as old ovn-controllers may not understand
> > new logical flows or new actions added by ovn-northd.
> >
> > Even upgrading ovn-controllers first can result in ovn-controllers
> > rejecting some of the logical flows if an existing OVN action is
> > changed. One such example is ct_commit action which recently was updated
> > to take new arguments.
> >
> > To avoid such downtimes during upgrades, this patch adds the
> > functionality of pinning ovn-controller and ovn-northd to a specific
> > version. An internal OVN version is generated and this version is stored
> > by ovn-northd in the Southbound SB_Global table's
> > options:northd_internal_version.
> >
> > When ovn-controller notices that the internal version has changed, it
> > stops handling the database changes - both Southbound and OVS. All the
> > existing OF flows are preserved. When ovn-controller is upgraded to the
> > same version as ovn-northd services, it will process the database
> > changes.
> >
> > This feature is made optional and disabled by default. A CMS can
> > enable it by configuring the OVS local database with the option -
> > ovn-match-northd-version=true.
> >
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
> >
> > v2 -> v3
> > -----
> > * Addressed the review comments from Dumitru.
> > * Added comments in actions.h and submitting-patches.rst about the
> > changes that needs to be done when existing actions are updated.
> >
> > v1 -> v2
> > ----
> > * Added test cases and missing documentation.
> > * Renamed the ovsdb option from ignore_northd_verison
> > to ovn-pin-to-northd.
> > * Added NEWS item.
> >
> >
> > .../contributing/submitting-patches.rst | 12 ++
> > NEWS | 3 +
> > controller/ovn-controller.8.xml | 11 ++
> > controller/ovn-controller.c | 52 ++++++-
> > include/ovn/actions.h | 4 +
> > lib/ovn-util.c | 17 ++
> > lib/ovn-util.h | 4 +
> > northd/ovn-northd.c | 18 ++-
> > tests/ovn.at | 147 ++++++++++++++++++
> > 9 files changed, 263 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/internals/contributing/submitting-patches.rst
> > b/Documentation/internals/contributing/submitting-patches.rst
> > index 0a9de58669..31a3ca747b 100644
> > --- a/Documentation/internals/contributing/submitting-patches.rst
> > +++ b/Documentation/internals/contributing/submitting-patches.rst
> > @@ -397,6 +397,18 @@ Remember to follow-up and actually remove the feature
> > from OVN codebase once
> > deprecation grace period has expired and users had opportunity to use at
> > least
> > one OVN release that would have informed them about feature deprecation!
> >
> > +OVN upgrades
> > +------------
> > +
> > +If the patch introduces any new OVN actions or updates existing OVN
> > actions,
> > +then make sure to check the function ovn_get_internal_version() in
> > +lib/ovn-util.c and increment the macro - OVN_INTERNAL_MINOR_VER.
> > +
> > +Adding new OVN actions or changing existing OVN actions can have datapath
> > +disruptions during OVN upgrades. To minimize disruptions, OVN supports
> > +version matching between ovn-northd and ovn-controller and it is important
> > +to update the internal OVN version when the patch introduces such changes.
> > +
> > Comments
> > --------
> >
> > diff --git a/NEWS b/NEWS
> > index 6010230679..bb95724b36 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -8,6 +8,9 @@ Post-v20.09.0
> > server.
> > - Support other_config:vlan-passthru=true to allow VLAN tagged incoming
> > traffic.
> > + - Support version pinning between ovn-northd and ovn-controller as an
> > + option. If the option is enabled and the versions don't match,
> > + ovn-controller will not process any DB changes.
> >
> > OVN v20.09.0 - 28 Sep 2020
> > --------------------------
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index 16bc47b205..b5c0800f0b 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -233,6 +233,17 @@
> > The boolean flag indicates if the chassis is used as an
> > interconnection gateway.
> > </dd>
> > +
> > + <dt><code>external_ids:ovn-match-northd-version</code></dt>
> > + <dd>
> > + The boolean flag indicates if <code>ovn-controller</code> needs to
> > + check <code>ovn-northd</code> version. If this
> > + flag is set to true and the <code>ovn-northd's</code> version
> > (reported
> > + in the Southbound database) doesn't match with the
> > + <code>ovn-controller's</code> internal version, then it will stop
> > + processing the southbound and local Open vSwitch database changes.
> > + The default value is considered false if this option is not
> > defined.
> > + </dd>
> > </dl>
> >
> > <p>
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 25de0c72fd..7f48f694ec 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2195,6 +2195,49 @@ struct ovn_controller_exit_args {
> > bool *restart;
> > };
> >
> > +/* Returns false if the northd internal version stored in SB_Global
> > + * and ovn-controller internal version don't match.
> > + */
> > +static bool
> > +check_northd_version(struct ovsdb_idl *ovs_idl, struct ovsdb_idl
> > *ovnsb_idl,
> > + const char *version)
> > +{
> > + static bool version_mismatch;
> > +
> > + const struct ovsrec_open_vswitch *cfg =
> > ovsrec_open_vswitch_first(ovs_idl);
> > + if (!cfg || !smap_get_bool(&cfg->external_ids,
> > "ovn-match-northd-version",
> > + false)) {
> > + version_mismatch = false;
> > + return true;
> > + }
> > +
> > + const struct sbrec_sb_global *sb = sbrec_sb_global_first(ovnsb_idl);
> > + if (!sb) {
> > + version_mismatch = true;
> > + return false;
> > + }
> > +
> > + const char *northd_version =
> > + smap_get_def(&sb->options, "northd_internal_version", "");
> > +
> > + if (strcmp(northd_version, version)) {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > + VLOG_WARN_RL(&rl, "controller version - %s mismatch with northd "
> > + "version - %s", version, northd_version);
> > + version_mismatch = true;
> > + return false;
> > + }
> > +
> > + /* If there used to be a mismatch and ovn-northd got updated, force a
> > + * full recompute.
> > + */
> > + if (version_mismatch) {
> > + engine_set_force_recompute(true);
> > + }
> > + version_mismatch = false;
> > + return true;
> > +}
> > +
> > int
> > main(int argc, char *argv[])
> > {
> > @@ -2488,6 +2531,9 @@ main(int argc, char *argv[])
> > .enable_lflow_cache = true
> > };
> >
> > + char *ovn_version = ovn_get_internal_version();
> > + VLOG_INFO("OVN internal version is : [%s]", ovn_version);
> > +
> > /* Main loop. */
> > exiting = false;
> > restart = false;
> > @@ -2541,7 +2587,9 @@ main(int argc, char *argv[])
> >
> > engine_set_context(&eng_ctx);
> >
> > - if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl)) {
> > + if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
> > + check_northd_version(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
> > + ovn_version)) {
> > /* Contains the transport zones that this Chassis belongs to
> > */
> > struct sset transport_zones =
> > SSET_INITIALIZER(&transport_zones);
> > sset_from_delimited_string(&transport_zones,
> > @@ -2830,6 +2878,7 @@ loop_done:
> > }
> > }
> >
> > + free(ovn_version);
> > unixctl_server_destroy(unixctl);
> > lflow_destroy();
> > ofctrl_destroy();
> > @@ -2882,6 +2931,7 @@ parse_options(int argc, char *argv[])
> >
> > case 'V':
> > ovs_print_version(OFP15_VERSION, OFP15_VERSION);
> > + printf("SB DB Schema %s\n", sbrec_get_db_version());
> > exit(EXIT_SUCCESS);
> >
> > VLOG_OPTION_HANDLERS
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index b4e5acabb9..1ebdb9ae89 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -48,6 +48,10 @@ struct ovn_extend_table;
> > * action. Its first member must have type "struct ovnact" and name
> > * "ovnact". The structure must have a fixed length, that is, it may
> > not
> > * end with a flexible array member.
> > + *
> > + * These OVNACTS are used to generate the OVN internal version. See
> > + * ovn_get_internal_version() in lib/ovn-util.c. If any OVNACT is updated,
> > + * increment the OVN_INTERNAL_MINOR_VER macro in lib/ovn-util.c.
> > */
> > #define OVNACTS \
> > OVNACT(OUTPUT, ovnact_null) \
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index f0fb796b4e..4ec1bcc7e9 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -20,6 +20,7 @@
> > #include <unistd.h>
> >
> > #include "daemon.h"
> > +#include "include/ovn/actions.h"
> > #include "openvswitch/ofp-parse.h"
> > #include "openvswitch/vlog.h"
> > #include "ovn-dirs.h"
> > @@ -720,3 +721,19 @@ ip_address_and_port_from_lb_key(const char *key, char
> > **ip_address,
> > *addr_family = ss.ss_family;
> > return true;
> > }
> > +
> > +#define N_OVN_ACTIONS N_OVNACTS
> > +BUILD_ASSERT_DECL(N_OVN_ACTIONS == 47);
>
> I don't understand the need for this BUID_ASSERT_DECL. Each time that a
> new OVN action is added, N_OVN_ACTIONS will go up, and that will be
> reflected in the OVN internal version. I don't understand why developers
> also need to update this BUILD_ASSERT_DECL each time that a new OVN
> actions is added.
Thanks for the review. I agree with you. Probably the assert is not required.
So I removed the assert with the below changes and applied this patch to master.
******
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 4ec1bcc7e9..97d27efa3f 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -722,9 +722,6 @@ ip_address_and_port_from_lb_key(const char *key,
char **ip_address,
return true;
}
-#define N_OVN_ACTIONS N_OVNACTS
-BUILD_ASSERT_DECL(N_OVN_ACTIONS == 47);
-
/* Increment this for any logical flow changes or if existing OVN action is
* modified. */
#define OVN_INTERNAL_MINOR_VER 0
@@ -735,5 +732,5 @@ ovn_get_internal_version(void)
{
return xasprintf("%s-%s-%d.%d", OVN_PACKAGE_VERSION,
sbrec_get_db_version(),
- N_OVN_ACTIONS, OVN_INTERNAL_MINOR_VER);
+ N_OVNACTS, OVN_INTERNAL_MINOR_VER);
}
*****
Thanks
Numan
>
> > +
> > +/* Increment this for any logical flow changes or if existing OVN action is
> > + * modified. */
> > +#define OVN_INTERNAL_MINOR_VER 0
> > +
> > +/* Returns the OVN version. The caller must free the returned value. */
> > +char *
> > +ovn_get_internal_version(void)
> > +{
> > + return xasprintf("%s-%s-%d.%d", OVN_PACKAGE_VERSION,
> > + sbrec_get_db_version(),
> > + N_OVN_ACTIONS, OVN_INTERNAL_MINOR_VER);
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index ea82265711..280bcc3e4f 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -222,4 +222,8 @@ char *str_tolower(const char *orig);
> > bool ip_address_and_port_from_lb_key(const char *key, char **ip_address,
> > uint16_t *port, int *addr_family);
> >
> > +/* Returns the internal OVN version. The caller must free the returned
> > + * value. */
> > +char *ovn_get_internal_version(void);
> > +
> > #endif
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 8ce756c8a1..920dfc9fa6 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -12072,7 +12072,8 @@ ovnnb_db_run(struct northd_context *ctx,
> > struct ovsdb_idl_loop *sb_loop,
> > struct hmap *datapaths, struct hmap *ports,
> > struct ovs_list *lr_list,
> > - int64_t loop_start_time)
> > + int64_t loop_start_time,
> > + const char *ovn_internal_version)
> > {
> > if (!ctx->ovnsb_txn || !ctx->ovnnb_txn) {
> > return;
> > @@ -12149,6 +12150,8 @@ ovnnb_db_run(struct northd_context *ctx,
> > smap_replace(&options, "max_tunid", max_tunid);
> > free(max_tunid);
> >
> > + smap_replace(&options, "northd_internal_version",
> > ovn_internal_version);
> > +
> > nbrec_nb_global_verify_options(nb);
> > nbrec_nb_global_set_options(nb, &options);
> >
> > @@ -12756,7 +12759,8 @@ ovnsb_db_run(struct northd_context *ctx,
> > static void
> > ovn_db_run(struct northd_context *ctx,
> > struct ovsdb_idl_index *sbrec_chassis_by_name,
> > - struct ovsdb_idl_loop *ovnsb_idl_loop)
> > + struct ovsdb_idl_loop *ovnsb_idl_loop,
> > + const char *ovn_internal_version)
> > {
> > struct hmap datapaths, ports;
> > struct ovs_list lr_list;
> > @@ -12766,7 +12770,8 @@ ovn_db_run(struct northd_context *ctx,
> >
> > int64_t start_time = time_wall_msec();
> > ovnnb_db_run(ctx, sbrec_chassis_by_name, ovnsb_idl_loop,
> > - &datapaths, &ports, &lr_list, start_time);
> > + &datapaths, &ports, &lr_list, start_time,
> > + ovn_internal_version);
> > ovnsb_db_run(ctx, ovnsb_idl_loop, &ports, start_time);
> > destroy_datapaths_and_ports(&datapaths, &ports, &lr_list);
> > }
> > @@ -13123,6 +13128,9 @@ main(int argc, char *argv[])
> > unixctl_command_register("sb-connection-status", "", 0, 0,
> > ovn_conn_show, ovnsb_idl_loop.idl);
> >
> > + char *ovn_internal_version = ovn_get_internal_version();
> > + VLOG_INFO("OVN internal version is : [%s]", ovn_internal_version);
> > +
> > /* Main loop. */
> > exiting = false;
> > state.had_lock = false;
> > @@ -13164,7 +13172,8 @@ main(int argc, char *argv[])
> > }
> >
> > if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> > - ovn_db_run(&ctx, sbrec_chassis_by_name, &ovnsb_idl_loop);
> > + ovn_db_run(&ctx, sbrec_chassis_by_name, &ovnsb_idl_loop,
> > + ovn_internal_version);
> > if (ctx.ovnsb_txn) {
> > check_and_add_supported_dhcp_opts_to_sb_db(&ctx);
> > check_and_add_supported_dhcpv6_opts_to_sb_db(&ctx);
> > @@ -13226,6 +13235,7 @@ main(int argc, char *argv[])
> > }
> > }
> >
> > + free(ovn_internal_version);
> > unixctl_server_destroy(unixctl);
> > ovsdb_idl_loop_destroy(&ovnnb_idl_loop);
> > ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 9a9b8a5079..512781c87e 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -22827,3 +22827,150 @@ AT_CHECK_UNQUOTED([grep -c
> > "table=65.*metadata=${sw_meta}.*load:${ro_snat_zone}-
> >
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check ovn-northd and ovn-controller version pinning])
> > +ovn_start
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.10
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-p1
> > +check ovn-nbctl lsp-add sw0 sw0-p2
> > +
> > +as hv1
> > +ovs-vsctl \
> > + -- add-port br-int vif1 \
> > + -- set Interface vif1 external_ids:iface-id=sw0-p1 \
> > + ofport-request=1
> > +ovs-vsctl \
> > + -- add-port br-int vif2 \
> > + -- set Interface vif2 external_ids:iface-id=sw0-p2 \
> > + ofport-request=2
> > +
> > +# Wait for port to be bound.
> > +wait_row_count Chassis 1 name=hv1
> > +ch=$(fetch_column Chassis _uuid name=hv1)
> > +wait_row_count Port_Binding 1 logical_port=sw0-p1 chassis=$ch
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 chassis=$ch
> > +
> > +northd_version=$(ovn-sbctl get SB_Global . options:northd_internal_version
> > | sed s/\"//g)
> > +echo "northd version = $northd_version"
> > +AT_CHECK([grep -c $northd_version hv1/ovn-controller.log], [0], [1
> > +])
> > +
> > +# Stop ovn-northd so that we can modify the northd_version.
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as northd-backup
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +check ovn-sbctl set SB_Global . options:northd_internal_version=foo
> > +
> > +as hv1
> > +check ovs-vsctl set interface vif2 external_ids:iface-id=foo
> > +
> > +# ovn-controller should release the lport sw0-p2 since
> > ovn-match-northd-version
> > +# is not true.
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 'chassis=[[]]'
> > +
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [1], [dnl
> > +0
> > +])
> > +
> > +echo
> > +echo "__file__:__line__: Pin ovn-controller to ovn-northd version."
> > +
> > +as hv1
> > +check ovs-vsctl set open . external_ids:ovn-match-northd-version=true
> > +
> > +OVS_WAIT_UNTIL(
> > + [test 1 = $(grep -c "controller version - $northd_version mismatch
> > with northd version - foo" hv1/ovn-controller.log)
> > +])
> > +
> > +as hv1
> > +check ovs-vsctl set interface vif2 external_ids:iface-id=sw0-p2
> > +
> > +# ovn-controller should not claim sw0-p2 since there is version mismatch
> > +as hv1 ovn-appctl -t ovn-controller recompute
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 'chassis=[[]]'
> > +
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [1], [dnl
> > +0
> > +])
> > +
> > +check ovn-sbctl set SB_Global .
> > options:northd_internal_version=$northd_version
> > +
> > +# It should claim sw0-p2
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 chassis=$ch
> > +
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [0], [dnl
> > +1
> > +])
> > +
> > +as hv1
> > +ovn_remote=$(ovs-vsctl get open . external_ids:ovn-remote | sed s/\"//g)
> > +ovs-vsctl set open . external_ids:ovn-remote=unix:foo
> > +check ovs-vsctl set interface vif2 external_ids:iface-id=foo
> > +
> > +# ovn-controller is not connected to the SB DB. Even though it
> > +# releases sw0-p2, it will not delete the OF flows.
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [0], [dnl
> > +1
> > +])
> > +
> > +# Change the version to incorrect one and reconnect to the SB DB.
> > +check ovn-sbctl set SB_Global . options:northd_internal_version=bar
> > +
> > +as hv1
> > +check ovs-vsctl set open . external_ids:ovn-remote=$ovn_remote
> > +
> > +sleep 1
> > +
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [0], [dnl
> > +1
> > +])
> > +
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 chassis=$ch
> > +
> > +# Change the ovn-remote to incorrect and set the correct northd version
> > +# and then change back to the correct ovn-remote
> > +as hv1
> > +check ovs-vsctl set open . external_ids:ovn-remote=unix:foo
> > +
> > +check ovn-sbctl set SB_Global .
> > options:northd_internal_version=$northd_version
> > +
> > +as hv1
> > +check ovs-vsctl set open . external_ids:ovn-remote=$ovn_remote
> > +
> > +wait_row_count Port_Binding 1 logical_port=sw0-p2 'chassis=[[]]'
> > +as hv1 ovs-ofctl dump-flows br-int table=0 > offlows_table0.txt
> > +AT_CAPTURE_FILE([offlows_table0.txt])
> > +AT_CHECK_UNQUOTED([grep -c "in_port=2" offlows_table0.txt], [1], [dnl
> > +0
> > +])
> > +
> > +as hv1
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +AT_CLEANUP
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
