Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Fri, Dec 12, Ian Campbell wrote: Seems ok. I wonder if the wrapper ought to source @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself rather than relying on the initscript and unit file to do so. Especially in the initscript case it looks a bit ugly to have to manually propagate things. It seems all that wrapping is of no use because SELinux can not deal with it. I will see if ExecStart=/bin/ary --no-fork $ENVVAR can be used to pass additional arguments. If so, the current XENSTORED_TRACE handling has to be removed in favour of XENSTORED_ARGS=. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Fri, Dec 12, Olaf Hering wrote: This works: ExecStart=@XENSTORED@ --no-fork $XENSTORED_ARGS This fails: ExecStart=$XENSTORED --no-fork $XENSTORED_ARGS But this will likely work: ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS Let me know how to proceed... Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Tue, Dec 09, Ian Jackson wrote: Bottom line: as relevant maintainer, I'm afraid I'm going to insist that this script be in /etc. I dont agree with the reasoning, but to get this done: Which place would that be, XEN_SCRIPT_DIR? I don't think this script wants to contain an option parser! How should it handle exec vs. no-exec? Just a single yes/no knob, so essentially sysv vs systemd? I was imagining a named parameter as SuS calls them. One or both the sites which run this wrapper script would pass an environment variable. Something like this in the script: $xenstored_do_exec $XENSTORED $@ $XENSTORED_TRACE_ARGS $XENSTORED_ARGS where the systemd unit simply sets `xenstored_do_exec=exec'. Ok, I will implement this. In this case that means I think you should find out whether the lack of this xenstore-read polling loop is actually a bug in the existing systemd unit. If (as I suspect) this is not a bug, then your code motion should not change this aspect of the operation under systemd. I think any daemon needs some time until its operational. In case of xenstored there are users which expect its functionality right away, such as xen-init-dom0 and the other tools launched by xencommons. Thats likely the reason why the loop is there, and the commit msg of f706d9e9a confirms that. With systemd we can only hope that it handles this properly with its socket passing functionality. If not, the startup code could be done like I did in my patch do avoid code duplication in a to-be-added ExecStartPost=. And the is xenstored ready? check in my current implementation would not work anyway because once the shell does exec it will not run the following code which does the xenstore-read -s. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Wed, 2014-12-10 at 10:15 +0100, Olaf Hering wrote: I was imagining a named parameter as SuS calls them. One or both the sites which run this wrapper script would pass an environment variable. Something like this in the script: $xenstored_do_exec $XENSTORED $@ $XENSTORED_TRACE_ARGS $XENSTORED_ARGS where the systemd unit simply sets `xenstored_do_exec=exec'. Ok, I will implement this. I'm not sure why we don't want to exec in both cases, making this whole problem moot. In this case that means I think you should find out whether the lack of this xenstore-read polling loop is actually a bug in the existing systemd unit. If (as I suspect) this is not a bug, then your code motion should not change this aspect of the operation under systemd. I think any daemon needs some time until its operational. In case of xenstored there are users which expect its functionality right away, such as xen-init-dom0 and the other tools launched by xencommons. Thats likely the reason why the loop is there, and the commit msg of f706d9e9a confirms that. With systemd we can only hope that it handles this properly with its socket passing functionality. I think we should assume that socket activation is working properly, or else what is the point of socket activation? If it is buggy then we should fix it, not add hacks to the wrapper or unit files. That results in a wrapper which unconditionally execs, the systemd unit just calls that while the sysv script runs the wrapper and then does the xenstore-read -s loop. If not, the startup code could be done like I did in my patch do avoid code duplication in a to-be-added ExecStartPost=. And the is xenstored ready? check in my current implementation would not work anyway because once the shell does exec it will not run the following code which does the xenstore-read -s. Separately from the above I wonder if it might be worth moving the xenstore readiness check into the xen-init-dom0 helper and having most things which currently depend on xenstore actually depend on the dom0-is-ready unit, which itself depends on xenstored, qemu-dom0 and whatever else is supposed to be ready in a dom0? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Wed, Dec 10, Ian Campbell wrote: Separately from the above I wonder if it might be worth moving the xenstore readiness check into the xen-init-dom0 helper and having most things which currently depend on xenstore actually depend on the dom0-is-ready unit, which itself depends on xenstored, qemu-dom0 and whatever else is supposed to be ready in a dom0? You mean something like this chain of depencencies? xenstored xen-init-dom0 xenconsoled | qemu-dom0 xendomains Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
Olaf Hering writes (Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in
systemd):
On Fri, Dec 05, Ian Jackson wrote:
I think the only way to make this work properly is to factor the
necessary parts out of init.d/xencommons into a new script which can
be used by both xencommons and systemd. I'm not sure such a patch
would be appropriate for 4.5 at this stage.
I came up with this, it appears to work in my testing. Will do more
testing later today.
Thanks. I think this is going in roughly the right direction.
But: I think the script is rather over-engineered, and that it ought
to be in /etc.
+ $(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
Sysadmins might want to edit the script to do something we haven't
thought of. So it should be in /etc where they can do so.
diff --git a/tools/hotplug/Linux/xenstored.sh.in
b/tools/hotplug/Linux/xenstored.sh.in
new file mode 100644
index 000..11caf25
--- /dev/null
+++ b/tools/hotplug/Linux/xenstored.sh.in
...
+case $1 in
+--exec)
+do_exec=exec
+;;
+--opt)
+opts=(${opts[@]} $2)
+shift
+;;
+esac
+shift
+done
I don't think this script wants to contain an option parser!
+. @XEN_SCRIPT_DIR@/hotplugpath.sh
...
+test -f $xencommons_config/xencommons . $xencommons_config/xencommons
...
+# Wait for xenstored to actually come up, timing out after 30 seconds
+while [ $time -lt $timeout ] ! `${BINDIR}/xenstore-read -s /
/dev/null 21` ; do
I would have expected this new script to contain only the
functionality which was previously both in (a) the systemd unit and
(b) the traditional init script, and which you are removing from both
those places. So I think you should be able to say no ultimate
functional change in your commit message.
The systemd unit doesn't currently contain anything messing about with
xenstore-read to detect when xenstored is working. Is that a bug that
was previously in the systemd unit, or is it a mistake in your patch
that this is added here ?
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Tue, Dec 09, Ian Jackson wrote: Olaf Hering writes (Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd): On Fri, Dec 05, Ian Jackson wrote: I think the only way to make this work properly is to factor the necessary parts out of init.d/xencommons into a new script which can be used by both xencommons and systemd. I'm not sure such a patch would be appropriate for 4.5 at this stage. I came up with this, it appears to work in my testing. Will do more testing later today. Thanks. I think this is going in roughly the right direction. But: I think the script is rather over-engineered, and that it ought to be in /etc. Why should the wrapper be in /etc?! xendomains isnt in /etc either. + $(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN) Sysadmins might want to edit the script to do something we haven't thought of. So it should be in /etc where they can do so. So they should mail here if they find substantial functionality missing. Until then they continue to not enable xencommons and run their own startup script. I don't think this script wants to contain an option parser! How should it handle exec vs. no-exec? Just a single yes/no knob, so essentially sysv vs systemd? The systemd unit doesn't currently contain anything messing about with xenstore-read to detect when xenstored is working. Is that a bug that was previously in the systemd unit, or is it a mistake in your patch that this is added here ? No idea how long it takes to have a functional xenstored after running it. Perhaps the forking has some overhead and it returns earlier than it can process requests. If thats the reason why the loop exists in the sysv runlevel script then that loop should be used only without --no-fork. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
Olaf Hering writes (Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd): On Tue, Dec 09, Ian Jackson wrote: But: I think the script is rather over-engineered, and that it ought to be in /etc. Why should the wrapper be in /etc?! Because it is the last chance the admin has to adjust how xenstored is run, which they ought to be able to do. xendomains isnt in /etc either. xendomains is rather different. + $(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN) Sysadmins might want to edit the script to do something we haven't thought of. So it should be in /etc where they can do so. So they should mail here if they find substantial functionality missing. That is no good because it doesn't allow them to make the change immediately on their own system, in a way that won't be overwritten by their system's package manager. Until then they continue to not enable xencommons and run their own startup script. That is not a practical suggestion. Bottom line: as relevant maintainer, I'm afraid I'm going to insist that this script be in /etc. I don't think this script wants to contain an option parser! How should it handle exec vs. no-exec? Just a single yes/no knob, so essentially sysv vs systemd? I was imagining a named parameter as SuS calls them. One or both the sites which run this wrapper script would pass an environment variable. Something like this in the script: $xenstored_do_exec $XENSTORED $@ $XENSTORED_TRACE_ARGS $XENSTORED_ARGS where the systemd unit simply sets `xenstored_do_exec=exec'. The systemd unit doesn't currently contain anything messing about with xenstore-read to detect when xenstored is working. Is that a bug that was previously in the systemd unit, or is it a mistake in your patch that this is added here ? No idea how long it takes to have a functional xenstored after running it. Perhaps the forking has some overhead and it returns earlier than it can process requests. If thats the reason why the loop exists in the sysv runlevel script then that loop should be used only without --no-fork. I think it is up to you as the person proposing a change to explain what the situation is and why your change is justified. It's not really satisfactory for a maintainer or reviewer to ask questions of a submitter and get simply `I'm not sure ... perhaps ...' without some kind of acknowledgement that more information (and perhaps research) is needed before we can establish how the code should look. In this case that means I think you should find out whether the lack of this xenstore-read polling loop is actually a bug in the existing systemd unit. If (as I suspect) this is not a bug, then your code motion should not change this aspect of the operation under systemd. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
Olaf Hering writes ([PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in
systemd):
The sysv runlevel script handles the boolean variable XENSTORED_TRACE
from sysconfig.xencommons to enable tracing. Recognize this also to
the systemd service file.
...
-ExecStart=/bin/sh -c exec $XENSTORED --no-fork $XENSTORED_ARGS
+ExecStart=/bin/sh -c 'if test -n ${XENSTORED_TRACE} ; then
XENSTORED_ARGS=-T /var/log/xen/xenstored-trace.log ; fi ; exec $XENSTORED
--no-fork $$XENSTORED_ARGS'
I'm afraid I'm not happy with the way that this duplicates logic
already found in /etc/init.d/xencommons.
Nacked-by: Ian Jackson ian.jack...@eu.citrix.com
I think the only way to make this work properly is to factor the
necessary parts out of init.d/xencommons into a new script which can
be used by both xencommons and systemd. I'm not sure such a patch
would be appropriate for 4.5 at this stage.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Fri, Dec 05, Ian Jackson wrote: I think the only way to make this work properly is to factor the necessary parts out of init.d/xencommons into a new script which can be used by both xencommons and systemd. I'm not sure such a patch would be appropriate for 4.5 at this stage. Yes, a helper script to launch just xenstored would help. But which part would do the final exec? Perhaps the sysv script has to fork a shell like its done above. I will have a look at this. Are you opposed to the idea to support XENSTORED_TRACE for systemd right in 4.5.0? Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
On Fri, Dec 05, Ian Jackson wrote: Can systemd not launch these daemons by running the existing xencommons et al init scripts ? Obviously that won't give you all of systemd's shiny features but IMO it ought to work. I think the point was to let systemd pass the file descriptors. Thats why the service file does the exec xenstored. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
