| Message ID | 20171107153127.7755-1-nusiddiq@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN DB log fixes | expand |
I'm not sure what to think of this one. This fixes a problem when using ovn-ctl to start the databases. But I'm concerned about this change causing breakage for deployments that don't use ovn-ctl. So I guess the question is, does anyone ever actually start the OVN databases with something other than ovn-ctl? Am I concerned for a non-existent userbase :) ? "ovnsb_db" is misspelled twice in the commit message, but it's not worth posting another version of the patch to fix those. Whoever commits this change can edit those when merging. On Tue, Nov 7, 2017 at 9:32 AM <nusiddiq@redhat.com> wrote: > From: Numan Siddique <nusiddiq@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@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); > + } > + > #else > /* On windows, if the 'target' contains ':', we make an assumption > that > * it is an absolute path. */ > -- > 2.13.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks Mark for the comments. On Wed, Nov 8, 2017 at 2:30 AM, Mark Michelson <mmichels@redhat.com> wrote: > I'm not sure what to think of this one. This fixes a problem when using > ovn-ctl to start the databases. But I'm concerned about this change causing > breakage for deployments that don't use ovn-ctl. > When ovn-ctl is used, ovnnb_db.pid and ovnsb_db.pid files are created in /var/run/openvswitch. When the log rotate script [1] runs it would call "ovs-appctl -t ovnnb_db .." and "ovs-appctl -t ovnsb_db .." and this fails. I don't think it would break any thing for usecases which dont use ovn-ctl. ovs-appctl supports providing the target as complete "socket path" which can be used. We could probably make this patch more generic so that when OVN db servers are started, any value can be provided to "--unixctl". Right now it is limited to just "ovnnb_db" and "ovnsb_db". Eg. User can start ovsdb-server for NB DB as " ovsdb-server ... --unixctl=nbdbctl" and we can allow "ovs-appctl -t nbdbctl ...". Right now it is limited to just "ovnnb_db" and "ovnsb_db". But I am not sure if this is really required. Thanks Numan [1] - https://github.com/openvswitch/ovs/blob/master/rhel/etc_logrotate.d_openvswitch So I guess the question is, does anyone ever actually start the OVN > databases with something other than ovn-ctl? Am I concerned for a > non-existent userbase :) ? > > "ovnsb_db" is misspelled twice in the commit message, but it's not worth > posting another version of the patch to fix those. Whoever commits this > change can edit those when merging. > > On Tue, Nov 7, 2017 at 9:32 AM <nusiddiq@redhat.com> wrote: > >> From: Numan Siddique <nusiddiq@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@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); >> + } >> + >> #else >> /* On windows, if the 'target' contains ':', we make an assumption >> that >> * it is an absolute path. */ >> -- >> 2.13.5 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On Tue, Nov 07, 2017 at 09:01:27PM +0530, nusiddiq@redhat.com wrote: > From: Numan Siddique <nusiddiq@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@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.
On Wed, Nov 8, 2017 at 8:39 AM, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Nov 07, 2017 at 09:01:27PM +0530, nusiddiq@redhat.com wrote: > > From: Numan Siddique <nusiddiq@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@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
On Wed, Nov 08, 2017 at 08:48:25AM +0530, Numan Siddique wrote: > On Wed, Nov 8, 2017 at 8:39 AM, Ben Pfaff <blp@ovn.org> wrote: > > > On Tue, Nov 07, 2017 at 09:01:27PM +0530, nusiddiq@redhat.com wrote: > > > From: Numan Siddique <nusiddiq@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@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 ? I think that we solve the problem by changing it to something like this: for ctl in /var/run/openvswitch/*.ctl; do ovs-appctl -t "$ctl" vlog/reopen 2>/dev/null || : done Are you able to test that change? If so, then it seems like a better solution to me.
On Wed, Nov 8, 2017 at 9:28 AM, Ben Pfaff <blp@ovn.org> wrote: > On Wed, Nov 08, 2017 at 08:48:25AM +0530, Numan Siddique wrote: > > On Wed, Nov 8, 2017 at 8:39 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > On Tue, Nov 07, 2017 at 09:01:27PM +0530, nusiddiq@redhat.com wrote: > > > > From: Numan Siddique <nusiddiq@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@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 ? > > I think that we solve the problem by changing it to something like this: > > for ctl in /var/run/openvswitch/*.ctl; do > ovs-appctl -t "$ctl" vlog/reopen 2>/dev/null || : > done > > Are you able to test that change? If so, then it seems like a better > solution to me. > Agree. This should work. I will test it out and respin v2. Thanks Numan
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); + } + #else /* On windows, if the 'target' contains ':', we make an assumption that * it is an absolute path. */