Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Sergey Vojtovich]

FYI: applied to MariaDB 10.0 
https://github.com/MariaDB/server/commit/8942824a5304e23f765b88d07498786d80092843

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Sergey Vojtovich]

Hi Lennart,

On Tue, Mar 08, 2016 at 04:25:06PM +, Lennart Weller wrote:
> March 8 2016 1:52 PM, "Sergey Vojtovich"  wrote:
> > I adjusted your patch a bit, it seem to work well for me. Could you please
> > verify if you're fine with the attached version and it works for you too?
> 
> Well you basically dropped all the safe guards. But in the end its a 
> logrotate script.
> Most people will never have the problem in the first place or know whatwent 
> wrong on
> their end when they see your version of the script. So yeah fine with me.
Nice. So I'll apply this patch to upstream 10.0.

> 
> > You mean "$($MYADMIN flush-logs)" doesn't return 1? I just tested it on my 
> > side
> > and got exit status 1.
> 
> I'll try to find a reproducible bug some other time. But it can happen. 
> EXIT_SUCCESS
> but error on stderr.
I went through relevant code and couldn't immediately find anything like that 
for
"flush-logs". But there's obvious exception for "ping": it may indeed return
success with "error on stderr".

Regards,
Sergey

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Lennart Weller]

March 8 2016 1:52 PM, "Sergey Vojtovich"  wrote:
> I adjusted your patch a bit, it seem to work well for me. Could you please
> verify if you're fine with the attached version and it works for you too?

Well you basically dropped all the safe guards. But in the end its a logrotate 
script.
Most people will never have the problem in the first place or know whatwent 
wrong on
their end when they see your version of the script. So yeah fine with me.

> You mean "$($MYADMIN flush-logs)" doesn't return 1? I just tested it on my 
> side
> and got exit status 1.

I'll try to find a reproducible bug some other time. But it can happen. 
EXIT_SUCCESS
but error on stderr.

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Sergey Vojtovich]

Hi Lennart,

I adjusted your patch a bit, it seem to work well for me. Could you please
verify if you're fine with the attached version and it works for you too?

On Mon, Mar 07, 2016 at 04:35:43PM +0100, Lennart Weller wrote:
> On Mon, Mar 07, 2016 at 06:37:49PM +0400, Sergey Vojtovich wrote:
> > Existence of pid-file is a sure sign that there's mysqld running, the only
> > exception is mysqld crash. What do you think about skipping this check?
> > 
> > I'd also suggest to turn things around and check for pid-file first and then
> > just run "MYADMIN flush-logs" allowing it to return error in case of 
> > failure.
> 
> Yep. Switched the order for this one. But mysqladmin does not return 1
> when it fails to connect. Did some additional testing. For now I put it
> back to the way it was in the original logrotate and just check if
> stdout/stderr of the command is null.
> This probably merits a second bug report.
You mean "$($MYADMIN flush-logs)" doesn't return 1? I just tested it on my side
and got exit status 1.

Thanks,
Sergey
diff --git a/debian/mariadb-server-10.0.mysql-server.logrotate 
b/debian/mariadb-server-10.0.mysql-server.logrotate
index 789ad35..a19e9ec 100644
--- a/debian/mariadb-server-10.0.mysql-server.logrotate
+++ b/debian/mariadb-server-10.0.mysql-server.logrotate
@@ -10,18 +10,11 @@
compress
sharedscripts
postrotate
-   test -x /usr/bin/mysqladmin || exit 0
+  test -x /usr/bin/mysqladmin || exit 0
 
-   # If this fails, check debian.conf! 
-   MYADMIN="/usr/bin/mysqladmin 
--defaults-file=/etc/mysql/debian.cnf"
-   if [ -z "`$MYADMIN ping 2>/dev/null`" ]; then
- # Really no mysqld or rather a missing debian-sys-maint user?
- # If this occurs and is not a error please report a bug.
- if ps cax | grep -q mysqld; then
-   exit 1
- fi 
-   else
- $MYADMIN flush-logs
-   fi
+  if [ -f `my_print_defaults --mysqld | grep -oP "pid-file=\K[^$]+"` 
]; then
+# If this fails, check debian.conf!
+mysqladmin --defaults-file=/etc/mysql/debian.cnf flush-logs
+  fi
endscript
 }

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Lennart Weller]

On Mon, Mar 07, 2016 at 06:37:49PM +0400, Sergey Vojtovich wrote:
> Existence of pid-file is a sure sign that there's mysqld running, the only
> exception is mysqld crash. What do you think about skipping this check?
> 
> I'd also suggest to turn things around and check for pid-file first and then
> just run "MYADMIN flush-logs" allowing it to return error in case of failure.

Yep. Switched the order for this one. But mysqladmin does not return 1
when it fails to connect. Did some additional testing. For now I put it
back to the way it was in the original logrotate and just check if
stdout/stderr of the command is null.
This probably merits a second bug report.
diff --git a/mariadb-server-10.0.mysql-server.logrotate.orig b/mariadb-server-10.0.mysql-server.logrotate
index 52f1292..30d8bec 100644
--- a/mariadb-server-10.0.mysql-server.logrotate.orig
+++ b/mariadb-server-10.0.mysql-server.logrotate
@@ -14,14 +14,11 @@
 
 		# If this fails, check debian.conf!
 		MYADMIN="/usr/bin/mysqladmin --defaults-file=/etc/mysql/debian.cnf"
-		if [ -z "`$MYADMIN ping 2>/dev/null`" ]; then
-		  # Really no mysqld or in incorrect authentication in /etc/mysql/debian.cnf user?
-		  # If this occurs and is not a error please report a bug.
-		  if ps cax | grep -q mysqld; then
- 		exit 1
-		  fi
-		else
-		  $MYADMIN flush-logs
+		PID=$(my_print_defaults mysqld | grep -oP "pid-file=\K[^$]+")
+		if [ -r $PID ]; then
+		  # If the pid-file exists and the process is mysqld it should be able to flush the logs
+		  [ "$(ps -p $(cat $PID) -o comm=)" = "mysqld" -a -n "$($MYADMIN flush-logs 2>&1)" ] && exit 1 
 		fi
+		exit 0
 	endscript
 }


signature.asc
Description: Digital signature

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Sergey Vojtovich]

Lennart,

On Mon, Mar 07, 2016 at 02:43:14PM +0100, Lennart Weller wrote:
...skip...

> diff --git a/mariadb-server-10.0.mysql-server.logrotate.orig 
> b/mariadb-server-10.0.mysql-server.logrotate
> index 52f1292..9a2050a 100644
> --- a/mariadb-server-10.0.mysql-server.logrotate.orig
> +++ b/mariadb-server-10.0.mysql-server.logrotate
> @@ -14,14 +14,13 @@
>  
>   # If this fails, check debian.conf!
>   MYADMIN="/usr/bin/mysqladmin 
> --defaults-file=/etc/mysql/debian.cnf"
> - if [ -z "`$MYADMIN ping 2>/dev/null`" ]; then
> -   # Really no mysqld or in incorrect authentication in 
> /etc/mysql/debian.cnf user?
> -   # If this occurs and is not a error please report a bug.
> -   if ps cax | grep -q mysqld; then
> - exit 1
> + if [ ! $($MYADMIN flush-logs 2>/dev/null) ]; then
> +   # If the pid-file exists and the process is mysqld it should 
> have flushed
> +   PID=$(my_print_defaults mysqld | grep -oP "pid-file=\K[^$]+")
Good. 

> +   if [ -r $PID ]; then
> + test "$(ps -p $(cat $PID) -o comm=)" = "mysqld" && exit 1 
Existence of pid-file is a sure sign that there's mysqld running, the only
exception is mysqld crash. What do you think about skipping this check?

I'd also suggest to turn things around and check for pid-file first and then
just run "MYADMIN flush-logs" allowing it to return error in case of failure.

If you like I can take your current patch and apply it upstream with some minor
corrections.

Regards,
Sergey

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Lennart Weller]

On Mon, Mar 07, 2016 at 04:15:54PM +0400, Sergey Vojtovich wrote:
> > March 3 2016 9:35 PM, "Otto Kekäläinen"  wrote:
> I consider it lesser evil. You may use my_print_defaults to get
> pid-file value.
>
> Worth to note that I don't see any value in executing "mysqladmin ping".

I attached a version of the patch using my_print_defaults with its
standard locations. Not specifying any configuration files in case they
change with 10.1. And the ping removed in favor of checking the exit
code of flush-logs.

I did some rudimentary testing. But I can't be sure that I caught all
possible versions of existing files/processes and names. So there might
be some corner cases with incorrect exit codes.

Lennart
diff --git a/mariadb-server-10.0.mysql-server.logrotate.orig b/mariadb-server-10.0.mysql-server.logrotate
index 52f1292..9a2050a 100644
--- a/mariadb-server-10.0.mysql-server.logrotate.orig
+++ b/mariadb-server-10.0.mysql-server.logrotate
@@ -14,14 +14,13 @@
 
 		# If this fails, check debian.conf!
 		MYADMIN="/usr/bin/mysqladmin --defaults-file=/etc/mysql/debian.cnf"
-		if [ -z "`$MYADMIN ping 2>/dev/null`" ]; then
-		  # Really no mysqld or in incorrect authentication in /etc/mysql/debian.cnf user?
-		  # If this occurs and is not a error please report a bug.
-		  if ps cax | grep -q mysqld; then
- 		exit 1
+		if [ ! $($MYADMIN flush-logs 2>/dev/null) ]; then
+		  # If the pid-file exists and the process is mysqld it should have flushed
+		  PID=$(my_print_defaults mysqld | grep -oP "pid-file=\K[^$]+")
+		  if [ -r $PID ]; then
+		test "$(ps -p $(cat $PID) -o comm=)" = "mysqld" && exit 1 
+		exit 0
 		  fi
-		else
-		  $MYADMIN flush-logs
 		fi
 	endscript
 }


signature.asc
Description: Digital signature

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Lennart Weller]

March 3 2016 9:35 PM, "Otto Kekäläinen"  wrote:
> Hello Lennart!
> 
> I asked core developers to review this and got this reply:
> 
> It's probably alright for 10.0. But it's not completely suitable for 10.1.
> As contributor mentioned himself that there's a problem with this patch:
> "When mysqld is called without mysqld_safe".
> 
> I'd rather simplify this script to something like (not tested):
> if [ -x /var/run/mysqld/mysqld.pid ]; then
> $MYADMIN flush-logs || exit 1
> fi
> 
> What do you think?

The line would result in some other issues. But the general idea was already 
tossed around
here before. Assume pid file location and then check if the process running on 
the pid
is named mysqld.

I don't really see an issue with my current version either though. In case that 
the mysqld_safe script
will be abandoned in the future a lot of the configuration files and init 
scripts/unit files
need to be updated anyhow. And the only change here would be removing one layer 
in the hierachy.

From:
PPID=$(ps -o ppid= -p $PID)
if [ $(ps -p $PPID -o comm=) = mysqld_safe ]; then
  test $(ps -o ppid= -p  $PPID) -eq 1 && exit 1
fi

To:
test $(ps -o ppid= -p $PID) -eq 1 && exit 1

But if you are fine with just checking the PID file at a static location 
something like the attached patch should be fine.


logrotate.patch
Description: Binary data

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Otto Kekäläinen]

Hello Lennart!

I asked core developers to review this and got this reply:

  It's probably alright for 10.0. But it's not completely suitable for 10.1.
  As contributor mentioned himself that there's a problem with this patch:
  "When mysqld is called without mysqld_safe".

  I'd rather simplify this script to something like (not tested):
  if [ -x /var/run/mysqld/mysqld.pid ]; then
$MYADMIN flush-logs || exit 1
  fi

What do you think?

I know your patch would fix an identified issue, but I am also afraid
of any regressions that might be introduced due to this very central
init file change, so I won't be including your patch just yet.

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Lennart Weller]

Hey Otto,
Sorry I totally forgot about this. Yes, I do have an updated patch.
I actually found there to be some issues with containers which had
embedded mysqld forked from the start script so I added an additional
check to the 2) version.

Following problems might exist in the future with this version:
 - When mysqld is called without mysqld_safe
 - Somebody managed to start mysqld as init

So it shouldn't be too much of an issue in general.

Lennart

On Sun, Feb 21, 2016 at 03:48:38PM +0200, Otto Kekäläinen wrote:
> Hello Lennart!
> 
> Do you intend to make one more version of the patch so that it is
> perfect and it is safe for me to include it in the next upload?
> 
> Thanks for your help!
> 
> 2016-01-26 11:13 GMT+02:00 Otto Kekäläinen :
> >> 2) Check the parent process id being 1
> >>In this case parent of the parent because of mysqld_safe
> >># test $(ps -o ppid= -p $(ps -o ppid= -p $PID)) -eq 1
> >>This would work in most cases I can think of. mysqld run by a user
> >>or a container would not be started by the init. But seems like a
> >>rather complex solution to a fairly simple problem.
> >
> > Option 2 seems OK. Alternatively we could check processed owned by user 
> > 'mysql'.
> >
> > Yes, the solution might be a bit complex, but I am fine with it as
> > long as the bash code is well documented and as clean as possible, so
> > that potential regressions/bugs in future are easy to track down and
> > new people are confident in tweaking the code. Try avoiding long
> > oneliners and short-cut code style (to the extend possible, it is
> > after all bash scripting).
diff --git a/mysql-server b/mysql-server.new
index 52f1292..19b1614 100644
--- a/mysql-server
+++ b/mysql-server
@@ -16,10 +16,14 @@
 		MYADMIN="/usr/bin/mysqladmin --defaults-file=/etc/mysql/debian.cnf"
 		if [ -z "`$MYADMIN ping 2>/dev/null`" ]; then
 		  # Really no mysqld or in incorrect authentication in /etc/mysql/debian.cnf user?
-		  # If this occurs and is not a error please report a bug.
-		  if ps cax | grep -q mysqld; then
- 		exit 1
-		  fi
+		  for PID in $(pgrep mysqld); do
+		# If the parent of the parent (mysqld_safe) PID is 1 we can assume it's the system
+		# mysqld and it should have responded to the ping
+		PPID=$(ps -o ppid= -p $PID)
+		if [ $(ps -p $PPID -o comm=) = mysqld_safe ]; then
+		  test $(ps -o ppid= -p  $PPID) -eq 1 && exit 1
+		fi
+		  done
 		else
 		  $MYADMIN flush-logs
 		fi


signature.asc
Description: Digital signature

Bug#810968: [debian-mysql] Bug#810968: Bug#810968: mariadb-server-10.0: Logrotate exists 1 if a non-debian mysqld is running (e.g. containerized mysqld)

[Otto Kekäläinen]

Hello Lennart!

Do you intend to make one more version of the patch so that it is
perfect and it is safe for me to include it in the next upload?

Thanks for your help!

2016-01-26 11:13 GMT+02:00 Otto Kekäläinen :
>> 2) Check the parent process id being 1
>>In this case parent of the parent because of mysqld_safe
>># test $(ps -o ppid= -p $(ps -o ppid= -p $PID)) -eq 1
>>This would work in most cases I can think of. mysqld run by a user
>>or a container would not be started by the init. But seems like a
>>rather complex solution to a fairly simple problem.
>
> Option 2 seems OK. Alternatively we could check processed owned by user 
> 'mysql'.
>
> Yes, the solution might be a bit complex, but I am fine with it as
> long as the bash code is well documented and as clean as possible, so
> that potential regressions/bugs in future are easy to track down and
> new people are confident in tweaking the code. Try avoiding long
> oneliners and short-cut code style (to the extend possible, it is
> after all bash scripting).
>
> ___
> pkg-mysql-maint mailing list
> pkg-mysql-ma...@lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-mysql-maint



-- 
Check out our blog at http://seravo.fi/blog
and follow @ottokekalainen