[ovs-dev] [PATCH 2/2] ovs-appctl: Allow ovnnb_db and ovnsb_db as targets

[Numan Siddique]

On Wed, Nov 8, 2017 at 8:39 AM, Ben Pfaff <blp at ovn.org> wrote:

> On Tue, Nov 07, 2017 at 09:01:27PM +0530, nusiddiq at redhat.com wrote:
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > ovn-ctl starts OVN DB servers with the options "--unixctl=ovnnb_db"
> > and "--unixctl=ovnsb_db". When "ovs-appctl -t ovnnb_db" is run the
> > below error is seen
> >
> > 2017-11-07T10:18:17Z|00001|unixctl|WARN|failed to connect to
> > /var/run/openvswitch/ovnnb_db.29536.ctl.
> >
> > This patch allows the targets "ovnnb_db" and ovndb_db" by not
> > appending the pid to the socket path.
> >
> > This issue has broken the openvswitch logrotate script because of
> > the invalid socket path and results in the OVN DB log files not
> > generated properly.
> >
> > Other approach to fix is to not pass "--unixctl" option, in which case
> > the ctl socket will be created as "ovsdb-server-<pid>.ctl". This would
> > break the functions 'stop_nb_ovsdb', 'stop_sb_ovsdb', 'demote_ovnnb'
> > and others. And we will not be able to use "ovnnb_db" and "ovnsd_db"
> > as target options to 'ovs-appctl'.
> >
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > ---
> >  utilities/ovs-appctl.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> > index 8f87cc4f6..b5a1a218b 100644
> > --- a/utilities/ovs-appctl.c
> > +++ b/utilities/ovs-appctl.c
> > @@ -210,8 +210,18 @@ connect_to_target(const char *target)
> >              ovs_fatal(-pid, "cannot read pidfile \"%s\"", pidfile_name);
> >          }
> >          free(pidfile_name);
> > -        socket_name = xasprintf("%s/%s.%ld.ctl",
> > -                                ovs_rundir(), target, (long int) pid);
> > +
> > +        /* If the target is ovnnb_db or ovnsb_db, then do not append
> the pid
> > +         * to the socket_name. When OVN DB servers are started using
> ovn-ctl
> > +         * they are started with the option
> > +         * "--unixctl=ovnnb_db.ctl/ovnsb_db.ctl". */
> > +        if (!strcmp(target, "ovnnb_db") || !strcmp(target, "ovnsb_db"))
> {
> > +            socket_name = xasprintf("%s/%s.ctl", ovs_rundir(), target);
> > +        } else {
> > +            socket_name = xasprintf("%s/%s.%ld.ctl",
> > +                                    ovs_rundir(), target, (long int)
> pid);
> > +        }
>
> This is extraordinarily specific to a particular use case.  It would be
> better, I think, to have the ovs-appctl invocation pass the absolute
> path to the target, in which case the filename will be used as-is,
> without appending anything.
>


Thanks Ben. Does it makes sense to fix logrotate script here -
https://github.com/openvswitch/ovs/blob/master/rhel/etc_logrotate.d_openvswitch
(and debian version)
to pass absolute path ?

Thanks
Numan