diff mbox series

[v2,2/2] tools/xenstore: set open file descriptor limit for xenstored

Message ID 20210608055839.10313-3-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools/xenstore: set resource limits of xenstored | expand

Commit Message

Juergen Gross June 8, 2021, 5:58 a.m. UTC
Add a configuration item for the maximum number of domains xenstored
should support and set the limit of open file descriptors accordingly.

For HVM domains there are up to 5 socket connections per domain (2 by
the xl daemon process, and 3 by qemu). So set the ulimit for xenstored
to 5 * XENSTORED_MAX_DOMAINS + 100 (the "+ 100" is for some headroom,
like logging, event channel device, etc.).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- set ulimit form launch script (Julien Grall)
- split off from original patch (Julien Grall)
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 7 +++++++
 tools/hotplug/Linux/launch-xenstore.in             | 3 +++
 2 files changed, 10 insertions(+)

Comments

Olaf Hering June 8, 2021, 4:39 p.m. UTC | #1
Am Tue,  8 Jun 2021 07:58:39 +0200
schrieb Juergen Gross <jgross@suse.com>:

> +#XENSTORED_MAX_N_DOMAINS=32768

This will break fillup.
Provide an empty variable like it is done for a few others in that file.

Olaf
Juergen Gross June 11, 2021, 5:01 a.m. UTC | #2
On 08.06.21 18:39, Olaf Hering wrote:
> Am Tue,  8 Jun 2021 07:58:39 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> +#XENSTORED_MAX_N_DOMAINS=32768
> 
> This will break fillup.

Why? You realize that above is a comment just documenting the default?

> Provide an empty variable like it is done for a few others in that file.

I'm following the pattern of basically all variables in that file, BTW.


Juergen
Olaf Hering June 11, 2021, 5:46 a.m. UTC | #3
Am Fri, 11 Jun 2021 07:01:31 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Why? You realize that above is a comment just documenting the default?

That depends on the context. See https://bugzilla.opensuse.org/show_bug.cgi?id=1185682 for a reason why it should become an empty variable. But yes, we can patch that one too.

Olaf
Juergen Gross June 11, 2021, 7:07 a.m. UTC | #4
On 11.06.21 07:46, Olaf Hering wrote:
> Am Fri, 11 Jun 2021 07:01:31 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> Why? You realize that above is a comment just documenting the default?
> 
> That depends on the context. See https://bugzilla.opensuse.org/show_bug.cgi?id=1185682 for a reason why it should become an empty variable. But yes, we can patch that one too.
Isn't that a bug in fillup or the related rpm-macro? A variable set in
the existing /etc/sysconfig/xencommons file only should be preserved.

In general I think we should be consistent in the file.

In case there is no downside for other distributions I'd recommend to
switch all variables to your suggested pattern.

If there are disadvantages for others we should keep the current
pattern as changing it now would break existing installations.

Any thoughts?


Juergen
Olaf Hering June 11, 2021, 7:28 a.m. UTC | #5
Am Fri, 11 Jun 2021 09:07:24 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Isn't that a bug in fillup or the related rpm-macro? 

No. Fillup expects a certain pattern: a bunch of comments and a single key=var right after that. With such format it might be able to adjust the comment and leave the key=var as it is. Without key=var it will see it as a stale comment, and removes the entire section of comments during the next package update.

Olaf
diff mbox series

Patch

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 00cf7f91d4..516cd97092 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -32,6 +32,13 @@ 
 # Changing this requires a reboot to take effect.
 #XENSTORED=@XENSTORED@
 
+## Type: integer
+## Default: 32768
+#
+# Select maximum number of domains supported by xenstored.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_N_DOMAINS=32768
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 3ad71e3d08..89149f98ee 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -54,12 +54,14 @@  test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 
 [ "$XENSTORETYPE" = "daemon" ] && {
 	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
+	[ -z "$XENSTORED_MAX_N_DOMAINS" ] && XENSTORED_MAX_N_DOMAINS=32768
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
 		exit 1
 	}
 	rm -f @XEN_RUN_DIR@/xenstored.pid
+	N_FILES=$(($XENSTORED_MAX_N_DOMAINS * 5 + 100))
 
 	echo -n Starting $XENSTORED...
 	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
@@ -67,6 +69,7 @@  test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
 	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
 	echo -500 >/proc/$XS_PID/oom_score_adj
+	prlimit --pid $XS_PID --nofile=$N_FILES
 
 	exit 0
 }