On Thu, Aug 24, 2017 at 2:13 PM, Mark Michelson <mmich...@redhat.com> wrote:
> OVN is lenient with the types of logical switch ports. Maybe too
> lenient. This patch attempts to solve this problem on two fronts:
>
> 1) In ovn-nbctl, if you attempt to set the port type to an unknown
> type, the command will not end up setting the type.
> 2) In northd, when copying the port type from the northbound database to
> the corresponding port-binding in the southbound database, a warning
> will be issued if the port is of an unknown type.
>
> Signed-off-by: Mark Michelson <mmich...@redhat.com>
> ---
> ovn/lib/ovn-util.c | 30 +++++++++++++++++++
> ovn/lib/ovn-util.h | 2 ++
> ovn/northd/ovn-northd.c | 4 +++
> ovn/utilities/ovn-nbctl.c | 7 ++++-
> tests/ovn-nbctl.at | 73
> +++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 115 insertions(+), 1 deletion(-)
>
Did you look into whether it was possible to use an enum in the db
schema? In particular, what would happen on an upgrade of an existing
database? and what happens if the existing db has an invalid type in
it already?
I just want to make sure we clearly rule that out first.
> diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
> index 037d0798a..883ce4ccb 100644
> --- a/ovn/lib/ovn-util.c
> +++ b/ovn/lib/ovn-util.c
> @@ -299,3 +299,33 @@ default_sb_db(void)
> }
> return def;
> }
> +
> +/* l3gateway, chassisredirect, and patch
> + * are not in this list since they are
> + * only set in the SB DB by northd
> + */
> +const char *OVN_NB_LSP_TYPES[] = {
I would add "static" here.
> + "router",
> + "localnet",
> + "localport",
> + "l2gateway",
> + "vtep",
> +};
> +
> +bool
> +ovn_is_known_nb_lsp_type(const char *type)
> +{
> + int i;
> +
> + if (!type || !type[0]) {
> + return true;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(OVN_NB_LSP_TYPES); ++i) {
> + if (!strcmp(OVN_NB_LSP_TYPES[i], type)) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
> index b3d2125a3..9b456426d 100644
> --- a/ovn/lib/ovn-util.h
> +++ b/ovn/lib/ovn-util.h
> @@ -67,4 +67,6 @@ char *alloc_nat_zone_key(const struct uuid *key, const char
> *type);
> const char *default_nb_db(void);
> const char *default_sb_db(void);
>
> +bool ovn_is_known_nb_lsp_type(const char *type);
> +
> #endif
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 49e4ac338..177df5a1e 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1962,6 +1962,10 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> }
> sbrec_port_binding_set_options(op->sb, &options);
> smap_destroy(&options);
> + if (!ovn_is_known_nb_lsp_type(op->nbsp->type)) {
> + VLOG_WARN("Unknown port type '%s' set on logical switch
> '%s'. "
> + "Using anyway.", op->nbsp->type, op->nbsp->name);
> + }
> sbrec_port_binding_set_type(op->sb, op->nbsp->type);
> } else {
> const char *chassis = NULL;
I tend to think we should ignore the port as if it didn't exist in
this case since it's an invalid configuration.
--
Russell Bryant
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
