Bug#961511: [Pkg-xen-devel] Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-22 Thread Elliott Mitchell
On Tue, Sep 22, 2020 at 02:39:09PM +0200, Hans van Kranenburg wrote:
> How did you test it and how did you get a working process without the --?

By reading the man page, noticing there was no mention of "--" and then
trying `choom -n +5 sleep 5` and found that worked.  When you sent this
message I checked and GNU `sleep` does have "--version", thus I tried
`choom -n +5 sleep 5 --version` and found *that* failed.

A "--" seemed natural, but documentation omitting crucial details is a
problem.  Never mind.

Nice find, I did at one point have the oom-killer get the wrong process
and saw *problems*.


-- 
(\___(\___(\__  --=> 8-) EHM <=--  __/)___/)___/)
 \BS (| ehem+sig...@m5p.com  PGP 87145445 |)   /
  \_CS\   |  _  -O #include  O-   _  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445



Bug#961511: [Pkg-xen-devel] Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-22 Thread Hans van Kranenburg
notfixed 961511 xen/4.14.0-1~exp1
thanks

Right... so in the end I made an off-by-one error while rebasing and
totally lost that commit. It's not actually in 4.14.0-1~exp1 now. That's
bad.

On 9/21/20 3:50 AM, Elliott Mitchell wrote:
> This is fun.  Actually isn't too difficult to trigger, simply slowly
> reduce the memory Xen allocates to Dom0 and eventually the oom-killer is
> likely to trigger (having tried to shrink Dom0 as far as possible,
> believe me, I know).  I had been wondering which of the Xen daemons could
> be safely restarted since it is handy to restart daemons instead of whole
> machine for security updates...
> 
> Interestingly running `xenstored --help` mentions:
>   -I, --internal-db   store database in memory, not on disk
> 
> There is a run/xenstored/tdb file so I end up wondering if newer versions
> are in fact storing everything in a file and restarting isn't so bad.

Not by default, and I don't know if it's actually considered best
practice. I could not find any info about this yet. I suspect it's not
recommended.

oxenstored has the following option in /etc/xen/oxenstored.conf:

# Activate filed base backend
persistent = false

When enabling this, the file /run/xenstored/db gets rewritten a lot and
I also see it's out of sync with what's in xenstore-ls after doing some
things. So, it might me inconsistent when the process is oom-killed.

> The patch switches the arguments from:
> --exec "$try_xenstored" -- ...
> to:
> --exec /usr/bin/choom -- -n -1000 "$try_xenstored" -- ...
> 
> I'm pretty sure start-stop-daemon is consuming the "--" and the second
> "--" shouldn't be there.

Well, I tested it and found out that it's needed...

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/bin/choom -- -n -1000 \
   /usr/lib/xen-4.14/bin/oxenstored --pid-file "/run/xenstore.pid"
/usr/bin/choom: unrecognized option '--pid-file'
Try 'choom --help' for more information.

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/lib/xen-4.14/bin/oxenstored --test
Would start /usr/lib/xen-4.14/bin/oxenstored .

and with the extra separator:

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/bin/choom -- -n -1000 \
   /usr/lib/xen-4.14/bin/oxenstored -- --pid-file "/run/xenstore.pid"

-# grep . /proc/$(pidof /usr/lib/xen-4.14/bin/oxenstored)/oom_*
/proc/363043/oom_adj:-17
/proc/363043/oom_score:0
/proc/363043/oom_score_adj:-1000

-# cat /proc/$(pidof /usr/lib/xen-4.14/bin/oxenstored)/cmdline
/usr/lib/xen-4.14/bin/oxenstored--pid-file/run/xenstore.pid

How did you test it and how did you get a working process without the --?

Hans



Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-20 Thread Elliott Mitchell
This is fun.  Actually isn't too difficult to trigger, simply slowly
reduce the memory Xen allocates to Dom0 and eventually the oom-killer is
likely to trigger (having tried to shrink Dom0 as far as possible,
believe me, I know).  I had been wondering which of the Xen daemons could
be safely restarted since it is handy to restart daemons instead of whole
machine for security updates...

Interestingly running `xenstored --help` mentions:
  -I, --internal-db   store database in memory, not on disk

There is a run/xenstored/tdb file so I end up wondering if newer versions
are in fact storing everything in a file and restarting isn't so bad.

The patch switches the arguments from:
--exec "$try_xenstored" -- ...
to:
--exec /usr/bin/choom -- -n -1000 "$try_xenstored" -- ...

I'm pretty sure start-stop-daemon is consuming the "--" and the second
"--" shouldn't be there.


-- 
(\___(\___(\__  --=> 8-) EHM <=--  __/)___/)___/)
 \BS (| ehem+sig...@m5p.com  PGP 87145445 |)   /
  \_CS\   |  _  -O #include  O-   _  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445



Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-07 Thread Ian Jackson
~Hans van Kranenburg writes ("[PATCH] d/xen-utils-common.xen.init: disable oom 
killer for xenstored"):
> In case of oom killer terminating some process, we'd rather not see
> xenstored go. Xenstored has an in-memory database, and when starting the
> process again, it would be empty, which is very inconvenient. Xenstored
> should already score quite low and have a fairly low memory footprint,
> but according to the user report, it happened.
> 
> Closes: #961511
> Suggested-by: Samuel Thibault 
> Signed-off-by: Hans van Kranenburg 

Acked-by: Ian Jackson 



Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-07 Thread Hans van Kranenburg
tag -1 + pending
thanks

On 9/7/20 12:40 PM, Ian Jackson wrote:
> ~Hans van Kranenburg writes ("[PATCH] d/xen-utils-common.xen.init: disable 
> oom killer for xenstored"):
>> In case of oom killer terminating some process, we'd rather not see
>> xenstored go. Xenstored has an in-memory database, and when starting the
>> process again, it would be empty, which is very inconvenient. Xenstored
>> should already score quite low and have a fairly low memory footprint,
>> but according to the user report, it happened.
>>
>> Closes: #961511
>> Suggested-by: Samuel Thibault 
>> Signed-off-by: Hans van Kranenburg 
> 
> Acked-by: Ian Jackson 

Thanks, added.

Hans



Bug#961511: [PATCH] d/xen-utils-common.xen.init: disable oom killer for xenstored

2020-09-06 Thread Hans van Kranenburg
In case of oom killer terminating some process, we'd rather not see
xenstored go. Xenstored has an in-memory database, and when starting the
process again, it would be empty, which is very inconvenient. Xenstored
should already score quite low and have a fairly low memory footprint,
but according to the user report, it happened.

Closes: #961511
Suggested-by: Samuel Thibault 
Signed-off-by: Hans van Kranenburg 
---
Cc: Ian Jackson 
---
This is in my knorrie/4.14-extra branch now. I think we should do this.
---
 debian/xen-utils-common.xen.init | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/debian/xen-utils-common.xen.init b/debian/xen-utils-common.xen.init
index 54aaba89d320..2a4c09fa3f71 100644
--- a/debian/xen-utils-common.xen.init
+++ b/debian/xen-utils-common.xen.init
@@ -226,7 +226,8 @@ xenstored_start()
eval "try_xenstored=\$$try_xenstored_var"
if [ -x $try_xenstored ]; then
if start-stop-daemon --start --quiet \
-   --pidfile "$XENSTORED_PIDFILE" --exec 
"$try_xenstored" -- \
+   --pidfile "$XENSTORED_PIDFILE" \
+   --exec /usr/bin/choom -- -n -1000 
"$try_xenstored" -- \
$XENSTORED_ARGS --pid-file 
"$XENSTORED_PIDFILE"; then
started_xenstored=$try_xenstored
break
-- 
2.20.1