From 9aad5a5a96bac423b05b5bb3afb7add2df44bba9 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 4 Feb 2016 09:48:54 -0800 Subject: [PATCH] ovs-vswitchd: Preserve datapath ports across graceful shutdown. 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.) Reported-by: Edgar Cantu Reported-at: http://openvswitch.org/pipermail/discuss/2016-January/020024.html Signed-off-by: Ben Pfaff Acked-by: Gurucharan Shetty --- 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 904cc2a5c..adfaeb681 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 3ba97d042..5fa03b590 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. @@ -896,12 +896,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 939cb3714..e1efedbbe 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 *); @@ -1573,7 +1573,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; @@ -1592,7 +1592,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) { @@ -2392,7 +2392,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 @@ -2478,11 +2478,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 b99f0cd9a..2d5a48148 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 c6fa44526..25a0663f1 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); } @@ -633,7 +633,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()); @@ -1713,7 +1713,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); } } @@ -2909,7 +2909,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, @@ -3194,7 +3194,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; @@ -3208,7 +3208,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.20.1