On 4 February 2016 at 09:49, Ben Pfaff <b...@ovn.org> wrote:
> Until now, asking ovs-vswitchd to shut down gracefully, e.g. with
> "ovs-appctl exit", would cause it to first remove all the ports from
> kernel-based datapaths. This has the unfortunate side effect that IP
> addresses on any removed "internal" ports are lost, even if the ports are
> added again when ovs-vswitchd is restarted. This is long-standing
> behavior, but it only became important when the OVS control scripts were
> changed to try to do graceful shutdown first instead of using a signal.
>
> This commit changes graceful shutdown so that it leaves ports in the
> datapath, fixing the problem.
>
> Fixes: 9b5422a98f8 (ovs-lib: Try to call exit before killing.)
> Signed-off-by: Ben Pfaff <b...@ovn.org>
>
Acked-by: Gurucharan Shetty <g...@ovn.org>
> ---
> ofproto/ofproto-dpif.c | 4 ++--
> ofproto/ofproto-provider.h | 28 ++++++++++++++++++++++++++--
> ofproto/ofproto.c | 12 ++++++------
> ofproto/ofproto.h | 4 ++--
> vswitchd/bridge.c | 14 +++++++-------
> 5 files changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 89e06aa..2b75c76 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1802,7 +1802,7 @@ port_construct(struct ofport *port_)
> }
>
> static void
> -port_destruct(struct ofport *port_)
> +port_destruct(struct ofport *port_, bool del)
> {
> struct ofport_dpif *port = ofport_dpif_cast(port_);
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
> @@ -1817,7 +1817,7 @@ port_destruct(struct ofport *port_)
>
> dp_port_name = netdev_vport_get_dpif_port(port->up.netdev, namebuf,
> sizeof namebuf);
> - if (dpif_port_exists(ofproto->backer->dpif, dp_port_name)) {
> + if (del && dpif_port_exists(ofproto->backer->dpif, dp_port_name)) {
> /* The underlying device is still there, so delete it. This
> * happens when the ofproto is being destroyed, since the caller
> * assumes that removal of attached ports will happen as part of
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index b6aac0a..004335b 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira,
> Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -899,12 +899,36 @@ struct ofproto_class {
> * initialization, and construct and destruct ofports to reflect
> all of
> * the changes.
> *
> + * - On graceful shutdown, the base ofproto code will destruct all
> + * the ports.
> + *
> * ->port_construct() returns 0 if successful, otherwise a positive
> errno
> * value.
> + *
> + *
> + * ->port_destruct()
> + * =================
> + *
> + * ->port_destruct() takes a 'del' parameter. If the provider
> implements
> + * the datapath itself (e.g. dpif-netdev, it can ignore 'del'. On the
> + * other hand, if the provider is an interface to an external datapath
> + * (e.g. to a kernel module that implement the datapath) then 'del'
> should
> + * influence destruction behavior in the following way:
> + *
> + * - If 'del' is true, it should remove the port from the underlying
> + * implementation. This is the common case.
> + *
> + * - If 'del' is false, it should leave the port in the underlying
> + * implementation. This is used when Open vSwitch is performing a
> + * graceful shutdown and ensures that, across Open vSwitch
> restarts,
> + * the underlying ports are not removed and recreated. That
> makes an
> + * important difference for, e.g., "internal" ports that have
> + * configured IP addresses; without this distinction, the IP
> address
> + * and other configured state for the port is lost.
> */
> struct ofport *(*port_alloc)(void);
> int (*port_construct)(struct ofport *ofport);
> - void (*port_destruct)(struct ofport *ofport);
> + void (*port_destruct)(struct ofport *ofport, bool del);
> void (*port_dealloc)(struct ofport *ofport);
>
> /* Called after 'ofport->netdev' is replaced by a new netdev object.
> If
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index bba30ae..6d21155 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -217,7 +217,7 @@ static void learned_cookies_flush(struct ofproto *,
> struct ovs_list *dead_cookie
>
> /* ofport. */
> static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex);
> -static void ofport_destroy(struct ofport *);
> +static void ofport_destroy(struct ofport *, bool del);
>
> static int update_port(struct ofproto *, const char *devname);
> static int init_ports(struct ofproto *);
> @@ -1580,7 +1580,7 @@ ofproto_destroy_defer__(struct ofproto *ofproto)
> }
>
> void
> -ofproto_destroy(struct ofproto *p)
> +ofproto_destroy(struct ofproto *p, bool del)
> OVS_EXCLUDED(ofproto_mutex)
> {
> struct ofport *ofport, *next_ofport;
> @@ -1599,7 +1599,7 @@ ofproto_destroy(struct ofproto *p)
>
> ofproto_flush__(p);
> HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) {
> - ofport_destroy(ofport);
> + ofport_destroy(ofport, del);
> }
>
> HMAP_FOR_EACH_SAFE (usage, next_usage, hmap_node, &p->ofport_usage) {
> @@ -2399,7 +2399,7 @@ ofport_remove(struct ofport *ofport)
> {
> connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
> OFPPR_DELETE);
> - ofport_destroy(ofport);
> + ofport_destroy(ofport, true);
> }
>
> /* If 'ofproto' contains an ofport named 'name', removes it from
> 'ofproto' and
> @@ -2485,11 +2485,11 @@ ofport_destroy__(struct ofport *port)
> }
>
> static void
> -ofport_destroy(struct ofport *port)
> +ofport_destroy(struct ofport *port, bool del)
> {
> if (port) {
> dealloc_ofp_port(port->ofproto, port->ofp_port);
> - port->ofproto->ofproto_class->port_destruct(port);
> + port->ofproto->ofproto_class->port_destruct(port, del);
> ofport_destroy__(port);
> }
> }
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 7504027..ebeb590 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira,
> Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -239,7 +239,7 @@ void ofproto_type_wait(const char *datapath_type);
>
> int ofproto_create(const char *datapath, const char *datapath_type,
> struct ofproto **ofprotop);
> -void ofproto_destroy(struct ofproto *);
> +void ofproto_destroy(struct ofproto *, bool del);
> int ofproto_delete(const char *name, const char *type);
>
> int ofproto_run(struct ofproto *);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index f8324a2..3f3054e 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -227,7 +227,7 @@ static bool ifaces_changed = false;
> static void add_del_bridges(const struct ovsrec_open_vswitch *);
> static void bridge_run__(void);
> static void bridge_create(const struct ovsrec_bridge *);
> -static void bridge_destroy(struct bridge *);
> +static void bridge_destroy(struct bridge *, bool del);
> static struct bridge *bridge_lookup(const char *name);
> static unixctl_cb_func bridge_unixctl_dump_flows;
> static unixctl_cb_func bridge_unixctl_reconnect;
> @@ -500,7 +500,7 @@ bridge_exit(void)
>
> if_notifier_destroy(ifnotifier);
> HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) {
> - bridge_destroy(br);
> + bridge_destroy(br, false);
> }
> ovsdb_idl_destroy(idl);
> }
> @@ -635,7 +635,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> VLOG_ERR("failed to create bridge %s: %s", br->name,
> ovs_strerror(error));
> shash_destroy(&br->wanted_ports);
> - bridge_destroy(br);
> + bridge_destroy(br, true);
> } else {
> /* Trigger storing datapath version. */
> seq_change(connectivity_seq_get());
> @@ -1714,7 +1714,7 @@ add_del_bridges(const struct ovsrec_open_vswitch
> *cfg)
> br->cfg = shash_find_data(&new_br, br->name);
> if (!br->cfg || strcmp(br->type, ofproto_normalize_type(
> br->cfg->datapath_type))) {
> - bridge_destroy(br);
> + bridge_destroy(br, true);
> }
> }
>
> @@ -2910,7 +2910,7 @@ bridge_run(void)
> (long int) getpid());
>
> HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) {
> - bridge_destroy(br);
> + bridge_destroy(br, false);
> }
> /* Since we will not be running system_stats_run() in this process
> * with the current situation of multiple ovs-vswitchd daemons,
> @@ -3195,7 +3195,7 @@ bridge_create(const struct ovsrec_bridge *br_cfg)
> }
>
> static void
> -bridge_destroy(struct bridge *br)
> +bridge_destroy(struct bridge *br, bool del)
> {
> if (br) {
> struct mirror *mirror, *next_mirror;
> @@ -3209,7 +3209,7 @@ bridge_destroy(struct bridge *br)
> }
>
> hmap_remove(&all_bridges, &br->node);
> - ofproto_destroy(br->ofproto);
> + ofproto_destroy(br->ofproto, del);
> hmap_destroy(&br->ifaces);
> hmap_destroy(&br->ports);
> hmap_destroy(&br->iface_by_name);
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
