Re: [PATCH v6 1/2] tools/xenstore: set oom score for xenstore daemon on Linux

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Juergen, Ian,

This patch broke gitlab-ci:

https://gitlab.com/xen-project/xen/-/jobs/1690080806

---
 * Executing: /lib/rc/sh/openrc-run.sh /lib/rc/sh/openrc-run.sh 
/etc/init.d/local start
 * Starting local ... *   Executing "/etc/local.d/xen.start" 
.../etc/xen/scripts/launch-xenstore: line 64: * 10: syntax error: operand 
expected (error token is "* 10")

illegal value daemon for XENSTORETYPE
---

See below about what the issue is and a potential fix.


On Tue, 12 Oct 2021, Juergen Gross wrote:
> Xenstored is absolutely mandatory for a Xen host and it can't be
> restarted, so being killed by OOM-killer in case of memory shortage is
> to be avoided.
> 
> Set /proc/$pid/oom_score_adj (if available) per default to -500 (this
> translates to 50% of dom0 memory size) in order to allow xenstored to
> use large amounts of memory without being killed.
> 
> The percentage of dom0 memory above which the oom killer is allowed to
> kill xenstored can be set via XENSTORED_OOM_MEM_THRESHOLD in
> xencommons.
> 
> Make sure the pid file isn't a left-over from a previous run delete it
> before starting xenstored.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> ---
> V2:
> - set oom score from launch script (Julien Grall)
> - split off open file descriptor limit setting (Julien Grall)
> V3:
> - make oom killer threshold configurable (Julien Grall)
> V4:
> - extend comment (Ian Jackson)
> ---
>  tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 9 +++++++++
>  tools/hotplug/Linux/launch-xenstore.in             | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in 
> b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> index 00cf7f91d4..b83101ab7e 100644
> --- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> +++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> @@ -48,6 +48,15 @@ XENSTORED_ARGS=
>  # Only evaluated if XENSTORETYPE is "daemon".
>  #XENSTORED_TRACE=[yes|on|1]
>  
> +## Type: integer
> +## Default: 50
> +#
> +# Percentage of dom0 memory size the xenstore daemon can use before the
> +# OOM killer is allowed to kill it.
> +# The specified value is multiplied by -10 and echoed to
> +# /proc/PID/oom_score_adj.
> +#XENSTORED_OOM_MEM_THRESHOLD=50
> +
>  ## Type: string
>  ## Default: @LIBEXEC@/boot/xenstore-stubdom.gz
>  #
> diff --git a/tools/hotplug/Linux/launch-xenstore.in 
> b/tools/hotplug/Linux/launch-xenstore.in
> index 019f9d6f4d..1747c96065 100644
> --- a/tools/hotplug/Linux/launch-xenstore.in
> +++ b/tools/hotplug/Linux/launch-xenstore.in
> @@ -59,11 +59,17 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . 
> @CONFIG_DIR@/@CONFIG_LEAF
>               echo "No xenstored found"
>               exit 1
>       }
> +     [ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
> +     XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))

These are the problematic lines. They don't work with busybox's bash
implementation. Originally I thought it was an issue with busybox bash
implementation but it looks like they don't even work with normal bash.
Specifically the first line is an issue, it should be:

if [ -z "$XENSTORED_OOM_MEM_THRESHOLD" ]
then
    XENSTORED_OOM_MEM_THRESHOLD=50
fi

Right?