[ovs-dev] 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 <azhou@ovn.org>
---
 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(-)

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 <azhou@ovn.org>
> ---
>  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 <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..bb855861992b 100644
> --- a/vswitchd/ovs-vswitchd.8.in
> +++ b/vswitchd/ovs-vswitchd.8.in
> @@ -111,6 +111,9 @@ how to configure Open vSwitch.
>  .SS "GENERAL COMMANDS"
>  .IP "\fBexit\fR"
>  Causes \fBovs\-vswitchd\fR to gracefully terminate.
> +.IP "\fBquit\fR"
> +Similar to "\fBexit\fR", but also release datapath resources.
> +.
>  .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..10953bb1bd47 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -66,7 +66,7 @@ main(int argc, char *argv[])
>      char *unixctl_path = NULL;
>      struct unixctl_server *unixctl;
>      char *remote;
> -    bool exiting;
> +    bool exiting, quiting;
>      int retval;
>  
>      set_program_name(argv[0]);
> @@ -93,6 +93,7 @@ main(int argc, char *argv[])
>          exit(EXIT_FAILURE);
>      }
>      unixctl_command_register("exit", "", 0, 0, ovs_vswitchd_exit, &exiting);
> +    unixctl_command_register("quit", "", 0, 0, ovs_vswitchd_exit, &quiting);
>  
>      bridge_init(remote);
>      free(remote);
> @@ -120,11 +121,11 @@ main(int argc, char *argv[])
>              poll_immediate_wake();
>          }
>          poll_block();
> -        if (should_service_stop()) {
> +        if (quiting || should_service_stop()) {
>              exiting = true;
>          }
>      }
> -    bridge_exit();
> +    bridge_exit(quiting);
>      unixctl_server_destroy(unixctl);
>      service_stop();
>

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 <azhou@ovn.org>

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.

On Tue, Apr 25, 2017 at 8:43 PM, Ben Pfaff <blp@ovn.org> 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 <azhou@ovn.org>
>
> 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.