[ovs-dev] [PATCH ovn] Create daemon pidfiles in ovn run dir.

Dumitru Ceara dceara at redhat.com
Wed Apr 22 19:40:31 UTC 2020


On 4/21/20 2:41 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> If an OVN service is started with --pidfile option, the pidfile
> is created in the ovs rundir. This patch fixes it by using the ovn rundir
> if either the pidfile is not specified or if specified, it is not
> absolute path.
> 
> Signed-off-by: Numan Siddique <numans at ovn.org>

Hi Numan,

Overall, looks good to me, however shouldn't we use
OVN_DAEMON_OPTION_ENUMS in ovn-trace and ovn-ic too?

A few minor comments inline.

Thanks,
Dumitru

> ---
>  controller-vtep/ovn-controller-vtep.c |  6 +--
>  controller/ovn-controller.c           |  6 +--
>  lib/ovn-util.c                        | 26 ++++++++++++
>  lib/ovn-util.h                        | 61 +++++++++++++++++++++++++++
>  northd/ovn-northd.c                   |  6 +--
>  utilities/ovn-nbctl.c                 | 10 ++---
>  6 files changed, 101 insertions(+), 14 deletions(-)
> 
> diff --git a/controller-vtep/ovn-controller-vtep.c b/controller-vtep/ovn-controller-vtep.c
> index 253a709ab..c13280bc0 100644
> --- a/controller-vtep/ovn-controller-vtep.c
> +++ b/controller-vtep/ovn-controller-vtep.c
> @@ -169,7 +169,7 @@ parse_options(int argc, char *argv[])
>          OPT_PEER_CA_CERT = UCHAR_MAX + 1,
>          OPT_BOOTSTRAP_CA_CERT,
>          VLOG_OPTION_ENUMS,
> -        DAEMON_OPTION_ENUMS,
> +        OVN_DAEMON_OPTION_ENUMS,
>          SSL_OPTION_ENUMS,
>      };
>  
> @@ -179,7 +179,7 @@ parse_options(int argc, char *argv[])
>          {"help", no_argument, NULL, 'h'},
>          {"version", no_argument, NULL, 'V'},
>          VLOG_LONG_OPTIONS,
> -        DAEMON_LONG_OPTIONS,
> +        OVN_DAEMON_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
>          {"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
>          {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
> @@ -212,7 +212,7 @@ parse_options(int argc, char *argv[])
>              exit(EXIT_SUCCESS);
>  
>          VLOG_OPTION_HANDLERS
> -        DAEMON_OPTION_HANDLERS
> +        OVN_DAEMON_OPTION_HANDLERS
>          STREAM_SSL_OPTION_HANDLERS
>  
>          case OPT_PEER_CA_CERT:
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 4d21ba0fd..6ff897325 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2268,7 +2268,7 @@ parse_options(int argc, char *argv[])
>          OPT_PEER_CA_CERT = UCHAR_MAX + 1,
>          OPT_BOOTSTRAP_CA_CERT,
>          VLOG_OPTION_ENUMS,
> -        DAEMON_OPTION_ENUMS,
> +        OVN_DAEMON_OPTION_ENUMS,
>          SSL_OPTION_ENUMS,
>      };
>  
> @@ -2276,7 +2276,7 @@ parse_options(int argc, char *argv[])
>          {"help", no_argument, NULL, 'h'},
>          {"version", no_argument, NULL, 'V'},
>          VLOG_LONG_OPTIONS,
> -        DAEMON_LONG_OPTIONS,
> +        OVN_DAEMON_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
>          {"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
>          {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
> @@ -2301,7 +2301,7 @@ parse_options(int argc, char *argv[])
>              exit(EXIT_SUCCESS);
>  
>          VLOG_OPTION_HANDLERS
> -        DAEMON_OPTION_HANDLERS
> +        OVN_DAEMON_OPTION_HANDLERS
>          STREAM_SSL_OPTION_HANDLERS
>  
>          case OPT_PEER_CA_CERT:
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 514e2489f..353e8ab2d 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -15,6 +15,7 @@
>  #include <config.h>
>  #include <unistd.h>
>  
> +#include "daemon.h"
>  #include "ovn-util.h"
>  #include "ovn-dirs.h"
>  #include "openvswitch/vlog.h"
> @@ -393,6 +394,31 @@ get_abs_unix_ctl_path(const char *path)
>      return abs_path;
>  }
>  
> +void
> +ovn_set_pidfile(const char *name)
> +{
> +    char *pidfile_name = NULL;
> +
> +#ifndef _WIN32
> +    pidfile_name = name ? abs_file_name(ovn_rundir(), name)
> +                        : xasprintf("%s/%s.pid", ovn_rundir(), program_name);
> +#else
> +    if (name) {
> +        if (strchr(name, ':')) {
> +            pidfile_name = xstrdup(name);
> +        } else {
> +            pidfile_name = xasprintf("%s/%s", ovn_rundir(), name);
> +        }
> +    } else {
> +        pidfile_name = xasprintf("%s/%s.pid", ovn_rundir(), program_name);
> +    }
> +#endif
> +
> +    /* Call openvswitch lib function. */
> +    set_pidfile(pidfile_name);
> +    free(pidfile_name);
> +}
> +
>  /* l3gateway, chassisredirect, and patch
>   * are not in this list since they are
>   * only set in the SB DB by northd
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 11238f61c..400457618 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -112,6 +112,7 @@ uint32_t ovn_allocate_tnlid(struct hmap *set, const char *name, uint32_t min,
>                              uint32_t max, uint32_t *hint);
>  
>  char *ovn_chassis_redirect_name(const char *port_name);
> +void ovn_set_pidfile(const char *name);
>  
>  /* An IPv4 or IPv6 address */
>  struct v46_ip {
> @@ -124,4 +125,64 @@ struct v46_ip {
>  bool ip46_parse_cidr(const char *str, struct v46_ip *prefix,
>                       unsigned int *plen);
>  bool ip46_equals(const struct v46_ip *addr1, const struct v46_ip *addr2);
> +
> +
> +/* OVN daemon options. Taken from ovs/lib/daemon.h. */
> +#define OVN_DAEMON_OPTION_ENUMS                     \
> +    OVN_OPT_DETACH,                                 \
> +    OVN_OPT_NO_SELF_CONFINEMENT,                    \
> +    OVN_OPT_NO_CHDIR,                               \
> +    OVN_OPT_OVERWRITE_PIDFILE,                      \
> +    OVN_OPT_PIDFILE,                                \
> +    OVN_OPT_MONITOR,                                \
> +    OVN_OPT_USER_GROUP
> +
> +#define OVN_DAEMON_LONG_OPTIONS                                              \
> +        {"detach",            no_argument, NULL, OVN_OPT_DETACH},            \
> +        {"no-self-confinement", no_argument, NULL, \
> +         OVN_OPT_NO_SELF_CONFINEMENT}, \

Nit: I'd align the "\" with the rest of the macro definition.

> +        {"no-chdir",          no_argument, NULL, OVN_OPT_NO_CHDIR},          \
> +        {"pidfile",           optional_argument, NULL, OVN_OPT_PIDFILE},     \
> +        {"overwrite-pidfile", no_argument, NULL, OVN_OPT_OVERWRITE_PIDFILE}, \
> +        {"monitor",           no_argument, NULL, OVN_OPT_MONITOR},           \
> +        {"user",              required_argument, NULL, OVN_OPT_USER_GROUP}
> +
> +#define OVN_DAEMON_OPTION_HANDLERS                  \
> +        case OVN_OPT_DETACH:                        \
> +            set_detach();                       \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_NO_SELF_CONFINEMENT:           \
> +            daemon_disable_self_confinement();  \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_NO_CHDIR:                      \
> +            set_no_chdir();                     \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_PIDFILE:                       \
> +            ovn_set_pidfile(optarg);                \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_OVERWRITE_PIDFILE:             \
> +            ignore_existing_pidfile();          \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_MONITOR:                       \
> +            daemon_set_monitor();               \
> +            break;                              \
> +                                                \
> +        case OVN_OPT_USER_GROUP:                    \
> +            daemon_set_new_user(optarg);        \
> +            break;
> +> +#define OVN_DAEMON_OPTION_CASES                     \
> +        case OVN_OPT_DETACH:                        \
> +        case OVN_OPT_NO_SELF_CONFINEMENT:           \
> +        case OVN_OPT_NO_CHDIR:                      \
> +        case OVN_OPT_PIDFILE:                       \
> +        case OVN_OPT_OVERWRITE_PIDFILE:             \
> +        case OVN_OPT_MONITOR:                       \
> +        case OVN_OPT_USER_GROUP:
> +

Nit: Same here, I'd align the "\" with the rest of the macro definition.

>  #endif
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 324835080..cab4db7a5 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -11627,7 +11627,7 @@ static void
>  parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
>  {
>      enum {
> -        DAEMON_OPTION_ENUMS,
> +        OVN_DAEMON_OPTION_ENUMS,
>          VLOG_OPTION_ENUMS,
>          SSL_OPTION_ENUMS,
>      };
> @@ -11638,7 +11638,7 @@ parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
>          {"help", no_argument, NULL, 'h'},
>          {"options", no_argument, NULL, 'o'},
>          {"version", no_argument, NULL, 'V'},
> -        DAEMON_LONG_OPTIONS,
> +        OVN_DAEMON_LONG_OPTIONS,
>          VLOG_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
>          {NULL, 0, NULL, 0},
> @@ -11654,7 +11654,7 @@ parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
>          }
>  
>          switch (c) {
> -        DAEMON_OPTION_HANDLERS;
> +        OVN_DAEMON_OPTION_HANDLERS;
>          VLOG_OPTION_HANDLERS;
>          STREAM_SSL_OPTION_HANDLERS;
>  
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 18bf91e32..db460d04f 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -324,7 +324,7 @@ enum {
>      OPT_NO_SHUFFLE_REMOTES,
>      OPT_BOOTSTRAP_CA_CERT,
>      MAIN_LOOP_OPTION_ENUMS,
> -    DAEMON_OPTION_ENUMS,
> +    OVN_DAEMON_OPTION_ENUMS,
>      VLOG_OPTION_ENUMS,
>      TABLE_OPTION_ENUMS,
>      SSL_OPTION_ENUMS,
> @@ -428,7 +428,7 @@ get_all_options(void)
>          {"version", no_argument, NULL, 'V'},
>          {"unixctl", required_argument, NULL, 'u'},
>          MAIN_LOOP_LONG_OPTIONS,
> -        DAEMON_LONG_OPTIONS,
> +        OVN_DAEMON_LONG_OPTIONS,
>          VLOG_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
>          {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
> @@ -460,7 +460,7 @@ has_option(const struct ovs_cmdl_parsed_option *parsed_options, size_t n,
>  static bool
>  will_detach(const struct ovs_cmdl_parsed_option *parsed_options, size_t n)
>  {
> -    return has_option(parsed_options, n, OPT_DETACH);
> +    return has_option(parsed_options, n, OVN_OPT_DETACH);
>  }
>  
>  static char * OVS_WARN_UNUSED_RESULT
> @@ -547,7 +547,7 @@ apply_options_direct(const struct ovs_cmdl_parsed_option *parsed_options,
>              printf("DB Schema %s\n", nbrec_get_db_version());
>              exit(EXIT_SUCCESS);
>  
> -        DAEMON_OPTION_HANDLERS
> +        OVN_DAEMON_OPTION_HANDLERS
>          VLOG_OPTION_HANDLERS
>          TABLE_OPTION_HANDLERS(&table_style)
>          STREAM_SSL_OPTION_HANDLERS
> @@ -6598,7 +6598,7 @@ nbctl_client(const char *socket_name,
>          case OPT_NO_SHUFFLE_REMOTES:
>          case OPT_BOOTSTRAP_CA_CERT:
>          STREAM_SSL_CASES
> -        DAEMON_OPTION_CASES
> +        OVN_DAEMON_OPTION_CASES
>              VLOG_INFO("using ovn-nbctl daemon, ignoring %s option",
>                        po->o->name);
>              break;
> 



More information about the dev mailing list