Re: [ovs-dev] [PATCH] vswitchd: Add 'appctl quit' command
On Tue, Apr 25, 2017 at 8:43 PM, Ben Pfaff wrote: > On Tue, Apr 25, 2017 at 05:02:14PM -0700, Andy Zhou wrote: >> vswitchd can gracefully shutdown after receiving the 'appctl exit' >> command. But the datapath resources vswitchd created can >> linger on. This is not a problem, and in fact desireable when the >> shutdown is transient in nature. Since restarted vswitchd usually >> sees a similar configuration. By keeping the datapath resources >> minimize the perturbation to the running data traffic. >> >> However, when vswitchd shutdown is permanent, there should be a >> way to release the all datapath resources, so that they won't >> be linger on forever. Add 'appctl quit' command for such purpose. >> >> Signed-off-by: Andy Zhou > > Thanks for working on this. > > I have two categories of comments here. The first category is a general > kind of unease over what problems this solves. I think it's better if > we don't have to make the distinction between two ways to exit, and so > I'd like to hear more specifically about the problems that this solves. The specific use case that triggered this patch was a user wanted to shut down OVS in the root name space, then restart OVS in a specific name space with the same or very similar configuration. They found that having the same bridge and ports existing in both the root name space and another name space to be confusing. They are also concerned about potential kernel resource leak in the root name space. They could have solved this by explicitly remove all bridges before stopping OVS services, but they chose not to rely on this approach (I don't know the details). They are mostly interested in having a whole sale way of releasing all kernel resources. Ideally, they'd like this to be provided via the the distribution scripts. They worked around this issue by issuing the 'ovs-dpctl del-dp ovs-system' command after stopping OVS services. It solves their problem at hand. While 'ovs-dpctl del-dp ovs-system' will remove all resources known to OVS kernel module, OVS userspace may create additional kernel resources outside the OVS kernel module down the road, For example: - light weight tunnel devices created by rtnetlink messages. - (future) reatlimiter created by NFT netlink messages. - CT/NAT states - TC Queues. Given that, It seems a better model would be for vswitchd to manage DP resource deletion, although this patch does not address most, if at all, of the issues listed above, this command can be expanded down the road. What do you think? I am definitely certainly open any comments and suggestions. > > The second category is just about naming. The two names "quit" and > "exit" don't suggest different operations and so I think that they are > likely to lead to confusion. I know that I, personally, will never > remember which is which. I'd suggest, instead, making the existing > "exit" take an option to explain what kind of shutdown is desired. > That's a fair point. May be we can add an option to exit, e.g. 'appctl exit clean-dp'? > Thanks, > > Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] vswitchd: Add 'appctl quit' command
On Tue, Apr 25, 2017 at 05:02:14PM -0700, Andy Zhou wrote: > vswitchd can gracefully shutdown after receiving the 'appctl exit' > command. But the datapath resources vswitchd created can > linger on. This is not a problem, and in fact desireable when the > shutdown is transient in nature. Since restarted vswitchd usually > sees a similar configuration. By keeping the datapath resources > minimize the perturbation to the running data traffic. > > However, when vswitchd shutdown is permanent, there should be a > way to release the all datapath resources, so that they won't > be linger on forever. Add 'appctl quit' command for such purpose. > > Signed-off-by: Andy Zhou Thanks for working on this. I have two categories of comments here. The first category is a general kind of unease over what problems this solves. I think it's better if we don't have to make the distinction between two ways to exit, and so I'd like to hear more specifically about the problems that this solves. The second category is just about naming. The two names "quit" and "exit" don't suggest different operations and so I think that they are likely to lead to confusion. I know that I, personally, will never remember which is which. I'd suggest, instead, making the existing "exit" take an option to explain what kind of shutdown is desired. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] vswitchd: Add 'appctl quit' command
On Tue, 2017-04-25 at 17:02 -0700, Andy Zhou wrote: > vswitchd can gracefully shutdown after receiving the 'appctl exit' > command. But the datapath resources vswitchd created can > linger on. This is not a problem, and in fact desireable when the > shutdown is transient in nature. Since restarted vswitchd usually > sees a similar configuration. By keeping the datapath resources > minimize the perturbation to the running data traffic. > > However, when vswitchd shutdown is permanent, there should be a > way to release the all datapath resources, so that they won't > be linger on forever. Add 'appctl quit' command for such purpose. Otherwise looks fine to me but s/quiting/quitting/ Thanks, - Greg > > Signed-off-by: Andy Zhou > --- > NEWS | 1 + > ofproto/ofproto-dpif.c | 11 +++ > ofproto/ofproto-provider.h | 2 +- > ofproto/ofproto.c | 2 +- > vswitchd/bridge.c | 4 ++-- > vswitchd/bridge.h | 4 +++- > vswitchd/ovs-vswitchd.8.in | 3 +++ > vswitchd/ovs-vswitchd.c| 7 --- > 8 files changed, 22 insertions(+), 12 deletions(-) > > diff --git a/NEWS b/NEWS > index ea97d84a2dea..3f65654f5124 100644 > --- a/NEWS > +++ b/NEWS > @@ -26,6 +26,7 @@ Post-v2.7.0 > * Bundles now support hashing by just nw_src or nw_dst. > * The "learn" action now supports a "limit" option (see ovs-ofctl(8)). > * The port status bit OFPPS_LIVE now reflects link aliveness. > + - Add the command 'ovs-appctl quit' (see ovs-vswitchd(8)). > > v2.7.0 - 21 Feb 2017 > - > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index c73c2738c91c..bd2eaa60d36b 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -645,7 +645,7 @@ dealloc(struct ofproto *ofproto_) > } > > static void > -close_dpif_backer(struct dpif_backer *backer) > +close_dpif_backer(struct dpif_backer *backer, bool del) > { > ovs_assert(backer->refcount > 0); > > @@ -661,6 +661,9 @@ close_dpif_backer(struct dpif_backer *backer) > shash_find_and_delete(&all_dpif_backers, backer->type); > free(backer->type); > free(backer->dp_version_string); > +if (del) { > +dpif_delete(backer->dpif); > +} > dpif_close(backer->dpif); > free(backer); > } > @@ -772,7 +775,7 @@ open_dpif_backer(const char *type, struct dpif_backer > **backerp) > if (error) { > VLOG_ERR("failed to listen on datapath of type %s: %s", > type, ovs_strerror(error)); > -close_dpif_backer(backer); > +close_dpif_backer(backer, false); > return error; > } > > @@ -1452,7 +1455,7 @@ add_internal_flows(struct ofproto_dpif *ofproto) > } > > static void > -destruct(struct ofproto *ofproto_) > +destruct(struct ofproto *ofproto_, bool del) > { > struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); > struct ofproto_async_msg *am; > @@ -1505,7 +1508,7 @@ destruct(struct ofproto *ofproto_) > > seq_destroy(ofproto->ams_seq); > > -close_dpif_backer(ofproto->backer); > +close_dpif_backer(ofproto->backer, del); > } > > static int > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index b7b12cdfd5f4..ef993d0afc4d 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -828,7 +828,7 @@ struct ofproto_class { > */ > struct ofproto *(*alloc)(void); > int (*construct)(struct ofproto *ofproto); > -void (*destruct)(struct ofproto *ofproto); > +void (*destruct)(struct ofproto *ofproto, bool del); > void (*dealloc)(struct ofproto *ofproto); > > /* Performs any periodic activity required by 'ofproto'. It should: > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index ca0f3e49bd67..7bc7b7f99d0d 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -1651,7 +1651,7 @@ ofproto_destroy(struct ofproto *p, bool del) > free(usage); > } > > -p->ofproto_class->destruct(p); > +p->ofproto_class->destruct(p, del); > > /* We should not postpone this because it involves deleting a listening > * socket which we may want to reopen soon. 'connmgr' may be used by > other > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index ebb6249416fa..e741f34f19ec 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -496,14 +496,14 @@ bridge_init(const char *remote) > } > > void > -bridge_exit(void) > +bridge_exit(bool delete_datpath) > { > struct bridge *br, *next_br; > > if_notifier_destroy(ifnotifier); > seq_destroy(ifaces_changed); > HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) { > -bridge_destroy(br, false); > +bridge_destroy(br, delete_datpath); > } > ovsdb_idl_destroy(idl); > } > diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h > index 3783a21e3b4c..7835611568cf 100644 > --- a/vswitchd/bridge.h > +++ b/vswitchd/bridge.h > @@ -16,10 +16,12 @@ >
[ovs-dev] [PATCH] vswitchd: Add 'appctl quit' command
vswitchd can gracefully shutdown after receiving the 'appctl exit' command. But the datapath resources vswitchd created can linger on. This is not a problem, and in fact desireable when the shutdown is transient in nature. Since restarted vswitchd usually sees a similar configuration. By keeping the datapath resources minimize the perturbation to the running data traffic. However, when vswitchd shutdown is permanent, there should be a way to release the all datapath resources, so that they won't be linger on forever. Add 'appctl quit' command for such purpose. Signed-off-by: Andy Zhou --- NEWS | 1 + ofproto/ofproto-dpif.c | 11 +++ ofproto/ofproto-provider.h | 2 +- ofproto/ofproto.c | 2 +- vswitchd/bridge.c | 4 ++-- vswitchd/bridge.h | 4 +++- vswitchd/ovs-vswitchd.8.in | 3 +++ vswitchd/ovs-vswitchd.c| 7 --- 8 files changed, 22 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index ea97d84a2dea..3f65654f5124 100644 --- a/NEWS +++ b/NEWS @@ -26,6 +26,7 @@ Post-v2.7.0 * Bundles now support hashing by just nw_src or nw_dst. * The "learn" action now supports a "limit" option (see ovs-ofctl(8)). * The port status bit OFPPS_LIVE now reflects link aliveness. + - Add the command 'ovs-appctl quit' (see ovs-vswitchd(8)). v2.7.0 - 21 Feb 2017 - diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index c73c2738c91c..bd2eaa60d36b 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -645,7 +645,7 @@ dealloc(struct ofproto *ofproto_) } static void -close_dpif_backer(struct dpif_backer *backer) +close_dpif_backer(struct dpif_backer *backer, bool del) { ovs_assert(backer->refcount > 0); @@ -661,6 +661,9 @@ close_dpif_backer(struct dpif_backer *backer) shash_find_and_delete(&all_dpif_backers, backer->type); free(backer->type); free(backer->dp_version_string); +if (del) { +dpif_delete(backer->dpif); +} dpif_close(backer->dpif); free(backer); } @@ -772,7 +775,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) if (error) { VLOG_ERR("failed to listen on datapath of type %s: %s", type, ovs_strerror(error)); -close_dpif_backer(backer); +close_dpif_backer(backer, false); return error; } @@ -1452,7 +1455,7 @@ add_internal_flows(struct ofproto_dpif *ofproto) } static void -destruct(struct ofproto *ofproto_) +destruct(struct ofproto *ofproto_, bool del) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); struct ofproto_async_msg *am; @@ -1505,7 +1508,7 @@ destruct(struct ofproto *ofproto_) seq_destroy(ofproto->ams_seq); -close_dpif_backer(ofproto->backer); +close_dpif_backer(ofproto->backer, del); } static int diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index b7b12cdfd5f4..ef993d0afc4d 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -828,7 +828,7 @@ struct ofproto_class { */ struct ofproto *(*alloc)(void); int (*construct)(struct ofproto *ofproto); -void (*destruct)(struct ofproto *ofproto); +void (*destruct)(struct ofproto *ofproto, bool del); void (*dealloc)(struct ofproto *ofproto); /* Performs any periodic activity required by 'ofproto'. It should: diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index ca0f3e49bd67..7bc7b7f99d0d 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1651,7 +1651,7 @@ ofproto_destroy(struct ofproto *p, bool del) free(usage); } -p->ofproto_class->destruct(p); +p->ofproto_class->destruct(p, del); /* We should not postpone this because it involves deleting a listening * socket which we may want to reopen soon. 'connmgr' may be used by other diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index ebb6249416fa..e741f34f19ec 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -496,14 +496,14 @@ bridge_init(const char *remote) } void -bridge_exit(void) +bridge_exit(bool delete_datpath) { struct bridge *br, *next_br; if_notifier_destroy(ifnotifier); seq_destroy(ifaces_changed); HMAP_FOR_EACH_SAFE (br, next_br, node, &all_bridges) { -bridge_destroy(br, false); +bridge_destroy(br, delete_datpath); } ovsdb_idl_destroy(idl); } diff --git a/vswitchd/bridge.h b/vswitchd/bridge.h index 3783a21e3b4c..7835611568cf 100644 --- a/vswitchd/bridge.h +++ b/vswitchd/bridge.h @@ -16,10 +16,12 @@ #ifndef VSWITCHD_BRIDGE_H #define VSWITCHD_BRIDGE_H 1 +#include + struct simap; void bridge_init(const char *remote); -void bridge_exit(void); +void bridge_exit(bool delete_datpath); void bridge_run(void); void bridge_wait(void); diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in index 8496aa68af97..bb855861992b 100644 --- a/vswitchd/ovs-vswitchd.8.in +++ b/vswitchd/ov