[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