[ovs-dev] [PATCH ovn] Create daemon pidfiles in ovn run dir.
Numan Siddique
numans at ovn.org
Thu Apr 23 07:24:25 UTC 2020
On Thu, Apr 23, 2020 at 1:10 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> 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?
>
Yes. I missed it v1.
Thanks for the review. I addressed the comments and submitted v2.
Thanks
Numan
> 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;
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list