| Message ID | 1493246299-4387-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
Some types noted below, otherwise: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Apr 26, 2017, at 3:38 PM, Andy Zhou <azhou@ovn.org> wrote: > > 'appctl exit' stops the running vswitchd daemon, without releasing > the datapath resources (such as bridges and ports) that vswitchd > has created. This is expected when vswitchd is to be relaunched, to > reduce the perturbation of exiting traffic and connections. > > However, when vswitchd is intended to be shutdown permanently, it > is desirable not to leak datapath resources. In theory, this can be > achieved by removing the corresponding configurations from > OVSDB before shutting down vswitchd. However it is not always > possible in practice. Sometimes it is convenient and robust for > vswitchd to release all datapath resources that it has configured. > Add 'appctl exit --cleanup' option for this use case. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > --- > v1->v2: > remove 'appctl quit', Change to 'appctl exit --cleanup' > Add more details to the commit message. > --- > 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 | 7 +++++-- > vswitchd/ovs-vswitchd.c | 23 ++++++++++++++++------- > 8 files changed, 36 insertions(+), 18 deletions(-) > > diff --git a/NEWS b/NEWS > index ea97d84a2dea..ee50c6660468 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 --cleanup option to command 'ovs-appctl exit' (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) ->”delete_datapath” > { > 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); here too. > } > 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 <stdbool.h> > + > struct simap; > > void bridge_init(const char *remote); > -void bridge_exit(void); > +void bridge_exit(bool delete_datpath); ditto. > > void bridge_run(void); > void bridge_wait(void); > diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in > index 8496aa68af97..9fb1f774a474 100644 > --- a/vswitchd/ovs-vswitchd.8.in > +++ b/vswitchd/ovs-vswitchd.8.in > @@ -109,8 +109,11 @@ configuration. > described below. The command descriptions assume an understanding of > how to configure Open vSwitch. > .SS "GENERAL COMMANDS" > -.IP "\fBexit\fR" > -Causes \fBovs\-vswitchd\fR to gracefully terminate. > +.IP "\fBexit\fR \fI--cleanup\fR" > +Causes \fBovs\-vswitchd\fR to gracefully terminate. If \fI--cleanup\fR > +is specified, release datapath resources configured by \fBovs\-vswitchd\fR. > +Otherwise, datapath resources remains as is. ->”Otherwise, datapath flows and other resources remain undeleted.” > +. > .IP "\fBqos/show-types\fR \fIinterface\fR" > Queries the interface for a list of Quality of Service types that are > configurable via Open vSwitch for the given \fIinterface\fR. > diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c > index bed3fb5c374d..7de6d89abbea 100644 > --- a/vswitchd/ovs-vswitchd.c > +++ b/vswitchd/ovs-vswitchd.c > @@ -60,13 +60,19 @@ static unixctl_cb_func ovs_vswitchd_exit; > static char *parse_options(int argc, char *argv[], char **unixctl_path); > OVS_NO_RETURN static void usage(void); > > +struct ovs_vswitchd_exit_args { > + bool *exiting; > + bool *cleanup; > +}; > + > int > main(int argc, char *argv[]) > { > char *unixctl_path = NULL; > struct unixctl_server *unixctl; > char *remote; > - bool exiting; > + bool exiting, cleanup; > + struct ovs_vswitchd_exit_args exit_args = {&exiting, &cleanup}; > int retval; > > set_program_name(argv[0]); > @@ -92,12 +98,14 @@ main(int argc, char *argv[]) > if (retval) { > exit(EXIT_FAILURE); > } > - unixctl_command_register("exit", "", 0, 0, ovs_vswitchd_exit, &exiting); > + unixctl_command_register("exit", "[--cleanup]", 0, 1, > + ovs_vswitchd_exit, &exit_args); > > bridge_init(remote); > free(remote); > > exiting = false; > + cleanup = false; > while (!exiting) { > memory_run(); > if (memory_should_report()) { > @@ -124,7 +132,7 @@ main(int argc, char *argv[]) > exiting = true; > } > } > - bridge_exit(); > + bridge_exit(cleanup); > unixctl_server_destroy(unixctl); > service_stop(); > > @@ -265,10 +273,11 @@ usage(void) > } > > static void > -ovs_vswitchd_exit(struct unixctl_conn *conn, int argc OVS_UNUSED, > - const char *argv[] OVS_UNUSED, void *exiting_) > +ovs_vswitchd_exit(struct unixctl_conn *conn, int argc, > + const char *argv[], void *exit_args_) > { > - bool *exiting = exiting_; > - *exiting = true; > + struct ovs_vswitchd_exit_args *exit_args = exit_args_; > + *exit_args->exiting = true; > + *exit_args->cleanup = argc == 2 && !strcmp(argv[1], "--cleanup"); > unixctl_command_reply(conn, NULL); > } > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Apr 28, 2017 at 5:00 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > Some types noted below, otherwise: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> Thanks Jarno for the review. Pushed to master with fixes suggested. Will backport to branch 2.7 soon.
diff --git a/NEWS b/NEWS index ea97d84a2dea..ee50c6660468 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 --cleanup option to command 'ovs-appctl exit' (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 <stdbool.h> + 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..9fb1f774a474 100644 --- a/vswitchd/ovs-vswitchd.8.in +++ b/vswitchd/ovs-vswitchd.8.in @@ -109,8 +109,11 @@ configuration. described below. The command descriptions assume an understanding of how to configure Open vSwitch. .SS "GENERAL COMMANDS" -.IP "\fBexit\fR" -Causes \fBovs\-vswitchd\fR to gracefully terminate. +.IP "\fBexit\fR \fI--cleanup\fR" +Causes \fBovs\-vswitchd\fR to gracefully terminate. If \fI--cleanup\fR +is specified, release datapath resources configured by \fBovs\-vswitchd\fR. +Otherwise, datapath resources remains as is. +. .IP "\fBqos/show-types\fR \fIinterface\fR" Queries the interface for a list of Quality of Service types that are configurable via Open vSwitch for the given \fIinterface\fR. diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c index bed3fb5c374d..7de6d89abbea 100644 --- a/vswitchd/ovs-vswitchd.c +++ b/vswitchd/ovs-vswitchd.c @@ -60,13 +60,19 @@ static unixctl_cb_func ovs_vswitchd_exit; static char *parse_options(int argc, char *argv[], char **unixctl_path); OVS_NO_RETURN static void usage(void); +struct ovs_vswitchd_exit_args { + bool *exiting; + bool *cleanup; +}; + int main(int argc, char *argv[]) { char *unixctl_path = NULL; struct unixctl_server *unixctl; char *remote; - bool exiting; + bool exiting, cleanup; + struct ovs_vswitchd_exit_args exit_args = {&exiting, &cleanup}; int retval; set_program_name(argv[0]); @@ -92,12 +98,14 @@ main(int argc, char *argv[]) if (retval) { exit(EXIT_FAILURE); } - unixctl_command_register("exit", "", 0, 0, ovs_vswitchd_exit, &exiting); + unixctl_command_register("exit", "[--cleanup]", 0, 1, + ovs_vswitchd_exit, &exit_args); bridge_init(remote); free(remote); exiting = false; + cleanup = false; while (!exiting) { memory_run(); if (memory_should_report()) { @@ -124,7 +132,7 @@ main(int argc, char *argv[]) exiting = true; } } - bridge_exit(); + bridge_exit(cleanup); unixctl_server_destroy(unixctl); service_stop(); @@ -265,10 +273,11 @@ usage(void) } static void -ovs_vswitchd_exit(struct unixctl_conn *conn, int argc OVS_UNUSED, - const char *argv[] OVS_UNUSED, void *exiting_) +ovs_vswitchd_exit(struct unixctl_conn *conn, int argc, + const char *argv[], void *exit_args_) { - bool *exiting = exiting_; - *exiting = true; + struct ovs_vswitchd_exit_args *exit_args = exit_args_; + *exit_args->exiting = true; + *exit_args->cleanup = argc == 2 && !strcmp(argv[1], "--cleanup"); unixctl_command_reply(conn, NULL); }
'appctl exit' stops the running vswitchd daemon, without releasing the datapath resources (such as bridges and ports) that vswitchd has created. This is expected when vswitchd is to be relaunched, to reduce the perturbation of exiting traffic and connections. However, when vswitchd is intended to be shutdown permanently, it is desirable not to leak datapath resources. In theory, this can be achieved by removing the corresponding configurations from OVSDB before shutting down vswitchd. However it is not always possible in practice. Sometimes it is convenient and robust for vswitchd to release all datapath resources that it has configured. Add 'appctl exit --cleanup' option for this use case. Signed-off-by: Andy Zhou <azhou@ovn.org> --- v1->v2: remove 'appctl quit', Change to 'appctl exit --cleanup' Add more details to the commit message. --- 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 | 7 +++++-- vswitchd/ovs-vswitchd.c | 23 ++++++++++++++++------- 8 files changed, 36 insertions(+), 18 deletions(-)