| Message ID | 20171026152536.17072-1-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/10/17 16:25, Olaf Hering wrote: > An upcoming change in systemd will mount xenfs right away, along with > all other system mounts. This improves the detection of the > virtualization environment, which is currently racy. Some parts of > systemd rely on the presence of /proc/xen/capabilities, which will only > exist if xenfs is mounted. Since xenfs is mounted by the proc-xen.mount > unit, it will be processed very late. Other units may be processed > earlier, and if they make use of ConditionVirtualization*= failures may > occour. > > Unfortunately mounting xenfs by systemd as an API filesystem will lead > to errors when proc-xen.mount is processed. Since that mount point > already exists the unit is considered as failed, and other units that > depend on proc-xen.mount will not start. To avoid this the existing > proc-xen.mount will be converted into proc-xen.service, which just > mounts xenfs manually. All dependencies are updated by this change. > > The existing conditionals in proc-xen.mount will prevent failures with > existing systemd based installations: > ConditionPathExists=!/proc/xen/capabilities will prevent execution with > a new systemd that mounts xenfs. And this conditional, in combination > with ConditionPathExists=/proc/xen, will trigger execution with an old > systemd. > > An absolute path to the mount binary has to be used. /bin/mount is > expected to be universally available, nowaways it is a symlink to > /usr/bin/mount. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> Can't all information be obtained from /sys/hypervisor? If not, how hard would it be to make happen? What happens to all the software which currently has a dependency on proc-xen.mount ? Independently, how does this interact with having a xenfs entries in /etc/fstab, which might plausibly still exist for compatibility with other init systems? ~Andrew
On Thu, Oct 26, Andrew Cooper wrote: > Can't all information be obtained from /sys/hypervisor? If not, how > hard would it be to make happen? Likely not that hard. Not sure why that was not added in the first place. > What happens to all the software which currently has a dependency on > proc-xen.mount ? All software gets converted by this change. > Independently, how does this interact with having a xenfs entries in > /etc/fstab, which might plausibly still exist for compatibility with > other init systems? mount(1) will continue to consider them. Olaf
On 26/10/17 16:59, Olaf Hering wrote: > On Thu, Oct 26, Andrew Cooper wrote: > >> Can't all information be obtained from /sys/hypervisor? If not, how >> hard would it be to make happen? > Likely not that hard. Not sure why that was not added in the first place. I've never really understood why xenfs exists in the first place (although I expect the answer is "Because that is how someone did it in the past"), and I'm not aware of any other project which needs its own custom filesystem driver for device nodes. These days, /dev/xen/ is the preferred location for devices anyway. Either way, I don't mean to bikeshed the issue, but we should at least consider whether cleaning this up fully is the easiest course of action. > >> What happens to all the software which currently has a dependency on >> proc-xen.mount ? > All software gets converted by this change. Is it possible to express a dependency on proc-xen.mount || proc-xen.service? If not, then out-of-tree packages are going to have compatibility problems with this change. > >> Independently, how does this interact with having a xenfs entries in >> /etc/fstab, which might plausibly still exist for compatibility with >> other init systems? > mount(1) will continue to consider them. Right, but ISTR that Systemd deals with /etc/fstab by auto-generating *.mount targets, and from what is said here, it is the proc-xen.mount target which is now broken by the change in systemd behaviour. ~Andrew`
On Thu, Oct 26, Andrew Cooper wrote: > I've never really understood why xenfs exists in the first place > (although I expect the answer is "Because that is how someone did it in > the past"), and I'm not aware of any other project which needs its own > custom filesystem driver for device nodes. Perhaps in the early days, before udev, new nodes would not magically appear in /dev. It was likely easy to be compatible that way, just like claiming /dev/hda to please existing installation programs. > Is it possible to express a dependency on proc-xen.mount || > proc-xen.service? As ordering yes, an additional After=proc-xen.service line is needed. An existing Requires=proc-xen.mount can not be used anymore, I have not verified that. > If not, then out-of-tree packages are going to have compatibility > problems with this change. Only if they use Requires=proc-xen.mount. > Right, but ISTR that Systemd deals with /etc/fstab by auto-generating > *.mount targets, and from what is said here, it is the proc-xen.mount > target which is now broken by the change in systemd behaviour. No, existing fstab entries will continue to work. /dev/shm is automounted, and my own fstab entry for /dev/shm always worked. Olaf
On Thu, Oct 26, 2017 at 05:25:36PM +0200, Olaf Hering wrote: > An upcoming change in systemd will mount xenfs right away, along with > all other system mounts. This improves the detection of the > virtualization environment, which is currently racy. Some parts of > systemd rely on the presence of /proc/xen/capabilities, which will only > exist if xenfs is mounted. Since xenfs is mounted by the proc-xen.mount > unit, it will be processed very late. Other units may be processed > earlier, and if they make use of ConditionVirtualization*= failures may > occour. > > Unfortunately mounting xenfs by systemd as an API filesystem will lead > to errors when proc-xen.mount is processed. Since that mount point > already exists the unit is considered as failed, and other units that > depend on proc-xen.mount will not start. To avoid this the existing > proc-xen.mount will be converted into proc-xen.service, which just > mounts xenfs manually. All dependencies are updated by this change. > > The existing conditionals in proc-xen.mount will prevent failures with > existing systemd based installations: > ConditionPathExists=!/proc/xen/capabilities will prevent execution with > a new systemd that mounts xenfs. And this conditional, in combination > with ConditionPathExists=/proc/xen, will trigger execution with an old > systemd. > > An absolute path to the mount binary has to be used. /bin/mount is > expected to be universally available, nowaways it is a symlink to > /usr/bin/mount. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> The code itself looks OK at a glance. What is unsaid is that we need to backport this to older Xen releases so that old Xen can work with new systemd. Ian and Jan?
On Thu, Oct 26, 2017 at 04:45:38PM +0100, Andrew Cooper wrote: > On 26/10/17 16:25, Olaf Hering wrote: > > An upcoming change in systemd will mount xenfs right away, along with > > all other system mounts. This improves the detection of the > > virtualization environment, which is currently racy. Some parts of > > systemd rely on the presence of /proc/xen/capabilities, which will only > > exist if xenfs is mounted. Since xenfs is mounted by the proc-xen.mount > > unit, it will be processed very late. Other units may be processed > > earlier, and if they make use of ConditionVirtualization*= failures may > > occour. > > > > Unfortunately mounting xenfs by systemd as an API filesystem will lead > > to errors when proc-xen.mount is processed. Since that mount point > > already exists the unit is considered as failed, and other units that > > depend on proc-xen.mount will not start. To avoid this the existing > > proc-xen.mount will be converted into proc-xen.service, which just > > mounts xenfs manually. All dependencies are updated by this change. > > > > The existing conditionals in proc-xen.mount will prevent failures with > > existing systemd based installations: > > ConditionPathExists=!/proc/xen/capabilities will prevent execution with > > a new systemd that mounts xenfs. And this conditional, in combination > > with ConditionPathExists=/proc/xen, will trigger execution with an old > > systemd. > > > > An absolute path to the mount binary has to be used. /bin/mount is > > expected to be universally available, nowaways it is a symlink to > > /usr/bin/mount. > > > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > Can't all information be obtained from /sys/hypervisor? If not, how > hard would it be to make happen? It is not possible to tell from /sys/hypervisor at the moment. There is /sys/hypervisor/properties/features but that only contains feature bits from hypervisor. And XENFEAT_dom0 is not a reliable indicator for Dom0. I suppose it requires a small mount of work to make /sys/hypervisor contain such information, but that won't get propagated to older kernel so we need to deal with the problem at hand nonetheless.
On Thu, Oct 26, Olaf Hering wrote: > > If not, then out-of-tree packages are going to have compatibility > > problems with this change. > Only if they use Requires=proc-xen.mount. Any other objections to this change? How to proceed with this? Olaf
On Wed, Nov 08, 2017 at 05:24:14PM +0100, Olaf Hering wrote: > On Thu, Oct 26, Olaf Hering wrote: > > > > If not, then out-of-tree packages are going to have compatibility > > > problems with this change. > > Only if they use Requires=proc-xen.mount. > > Any other objections to this change? > > How to proceed with this? Regardless of the decision whether we should backport this to older branch, I think we should accept this patch going forward to avoid breakage. But is there really no way to ask nicely to see if systemd would accept a change in behaviour? That is, to make proc-xen.mount (or any attempt to mount API fs) a nop when xenfs is added to API file system.
On Wed, Nov 08, Wei Liu wrote: > But is there really no way to ask nicely to see if systemd would accept > a change in behaviour? That is, to make proc-xen.mount (or any attempt > to mount API fs) a nop when xenfs is added to API file system. I have considered that as well. If the failing unit is "proc-xen.mount" and /proc/xen exists, just ignore the error. I will check if and how that can be done. Olaf
Olaf Hering writes ("Re: [Xen-devel] [PATCH v1] tools/hotplug: convert proc-xen.mount to proc-xen.service"):
> On Wed, Nov 08, Wei Liu wrote:
> > But is there really no way to ask nicely to see if systemd would accept
> > a change in behaviour? That is, to make proc-xen.mount (or any attempt
> > to mount API fs) a nop when xenfs is added to API file system.
>
> I have considered that as well. If the failing unit is "proc-xen.mount"
> and /proc/xen exists, just ignore the error. I will check if and how
> that can be done.
It seems to me that this should be the case for all mount units.
Nothing here is Xen-specific, apart from the particular details of the
consequential lossage.
Ie, mount units should be idempotent even if something else has
already mounted them. Certainly mount units should be a no-op if
their functionality has been moved into the systemd internal mount
table.
Can we please see what systemd upstream think of that proposal ?
Ian.
diff --git a/tools/configure.ac b/tools/configure.ac index d1a3a78d87..7b18421fa0 100644 --- a/tools/configure.ac +++ b/tools/configure.ac @@ -441,7 +441,7 @@ AX_AVAILABLE_SYSTEMD() AS_IF([test "x$systemd" = "xy"], [ AC_CONFIG_FILES([ - hotplug/Linux/systemd/proc-xen.mount + hotplug/Linux/systemd/proc-xen.service hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service diff --git a/tools/hotplug/Linux/systemd/Makefile b/tools/hotplug/Linux/systemd/Makefile index a5d41d86ef..855ff3747f 100644 --- a/tools/hotplug/Linux/systemd/Makefile +++ b/tools/hotplug/Linux/systemd/Makefile @@ -3,10 +3,10 @@ include $(XEN_ROOT)/tools/Rules.mk XEN_SYSTEMD_MODULES = xen.conf -XEN_SYSTEMD_MOUNT = proc-xen.mount -XEN_SYSTEMD_MOUNT += var-lib-xenstored.mount +XEN_SYSTEMD_MOUNT = var-lib-xenstored.mount -XEN_SYSTEMD_SERVICE = xenstored.service +XEN_SYSTEMD_SERVICE = proc-xen.service +XEN_SYSTEMD_SERVICE += xenstored.service XEN_SYSTEMD_SERVICE += xenconsoled.service XEN_SYSTEMD_SERVICE += xen-qemu-dom0-disk-backend.service XEN_SYSTEMD_SERVICE += xendomains.service diff --git a/tools/hotplug/Linux/systemd/proc-xen.mount.in b/tools/hotplug/Linux/systemd/proc-xen.service.in similarity index 60% rename from tools/hotplug/Linux/systemd/proc-xen.mount.in rename to tools/hotplug/Linux/systemd/proc-xen.service.in index 64ebe7f9b1..76f0097b75 100644 --- a/tools/hotplug/Linux/systemd/proc-xen.mount.in +++ b/tools/hotplug/Linux/systemd/proc-xen.service.in @@ -4,7 +4,7 @@ ConditionPathExists=/proc/xen ConditionPathExists=!/proc/xen/capabilities RefuseManualStop=true -[Mount] -What=xenfs -Where=/proc/xen -Type=xenfs +[Service] +Type=oneshot +RemainAfterExit=true +ExecStart=/bin/mount -t xenfs xenfs /proc/xen diff --git a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in index 11a7d50edc..5d171f82e8 100644 --- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in +++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in @@ -1,7 +1,7 @@ [Unit] Description=mount xenstore file system -Requires=proc-xen.mount -After=proc-xen.mount +Requires=proc-xen.service +After=proc-xen.service ConditionPathExists=/proc/xen/capabilities RefuseManualStop=true diff --git a/tools/hotplug/Linux/systemd/xen-init-dom0.service.in b/tools/hotplug/Linux/systemd/xen-init-dom0.service.in index 3befadcea3..c560fbe1b7 100644 --- a/tools/hotplug/Linux/systemd/xen-init-dom0.service.in +++ b/tools/hotplug/Linux/systemd/xen-init-dom0.service.in @@ -1,7 +1,7 @@ [Unit] Description=xen-init-dom0, initialise Dom0 configuration (xenstore nodes, JSON configuration stub) -Requires=xenstored.service proc-xen.mount -After=xenstored.service proc-xen.mount +Requires=xenstored.service proc-xen.service +After=xenstored.service proc-xen.service ConditionPathExists=/proc/xen/capabilities [Service] diff --git a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in index f56775bc87..1b95104823 100644 --- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in +++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in @@ -1,7 +1,7 @@ [Unit] Description=qemu for xen dom0 disk backend -Requires=proc-xen.mount xenstored.service -After=proc-xen.mount xenstored.service xenconsoled.service +Requires=proc-xen.service xenstored.service +After=proc-xen.service xenstored.service xenconsoled.service Before=xendomains.service libvirtd.service libvirt-guests.service RefuseManualStop=true ConditionPathExists=/proc/xen/capabilities diff --git a/tools/hotplug/Linux/systemd/xen-watchdog.service.in b/tools/hotplug/Linux/systemd/xen-watchdog.service.in index 1eecd2a616..3b9aff5e3b 100644 --- a/tools/hotplug/Linux/systemd/xen-watchdog.service.in +++ b/tools/hotplug/Linux/systemd/xen-watchdog.service.in @@ -1,7 +1,7 @@ [Unit] Description=Xen-watchdog - run xen watchdog daemon -Requires=proc-xen.mount -After=proc-xen.mount xendomains.service +Requires=proc-xen.service +After=proc-xen.service xendomains.service ConditionPathExists=/proc/xen/capabilities [Service] diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in index 8e333b114e..2a01262ecd 100644 --- a/tools/hotplug/Linux/systemd/xenconsoled.service.in +++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in @@ -1,7 +1,7 @@ [Unit] Description=Xenconsoled - handles logging from guest consoles and hypervisor -Requires=proc-xen.mount xenstored.service -After=proc-xen.mount xenstored.service +Requires=proc-xen.service xenstored.service +After=proc-xen.service xenstored.service ConditionPathExists=/proc/xen/capabilities [Service] diff --git a/tools/hotplug/Linux/systemd/xendomains.service.in b/tools/hotplug/Linux/systemd/xendomains.service.in index c7bfb61eb4..f89460da35 100644 --- a/tools/hotplug/Linux/systemd/xendomains.service.in +++ b/tools/hotplug/Linux/systemd/xendomains.service.in @@ -1,7 +1,7 @@ [Unit] Description=Xendomains - start and stop guests on boot and shutdown -Requires=proc-xen.mount xenstored.service -After=proc-xen.mount xenstored.service xenconsoled.service xen-init-dom0.service +Requires=proc-xen.service xenstored.service +After=proc-xen.service xenstored.service xenconsoled.service xen-init-dom0.service After=network-online.target After=remote-fs.target ConditionPathExists=/proc/xen/capabilities diff --git a/tools/hotplug/Linux/systemd/xendriverdomain.service.in b/tools/hotplug/Linux/systemd/xendriverdomain.service.in index c39ec04182..c3b0a32147 100644 --- a/tools/hotplug/Linux/systemd/xendriverdomain.service.in +++ b/tools/hotplug/Linux/systemd/xendriverdomain.service.in @@ -1,7 +1,7 @@ [Unit] Description=Xen driver domain device daemon -Requires=proc-xen.mount -After=proc-xen.mount +Requires=proc-xen.service +After=proc-xen.service ConditionVirtualization=xen [Service] diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in index 80c1d408a5..da1f197d11 100644 --- a/tools/hotplug/Linux/systemd/xenstored.service.in +++ b/tools/hotplug/Linux/systemd/xenstored.service.in @@ -1,7 +1,7 @@ [Unit] Description=The Xen xenstore -Requires=proc-xen.mount var-lib-xenstored.mount -After=proc-xen.mount var-lib-xenstored.mount +Requires=proc-xen.service var-lib-xenstored.mount +After=proc-xen.service var-lib-xenstored.mount Before=libvirtd.service libvirt-guests.service RefuseManualStop=true ConditionPathExists=/proc/xen/capabilities @@ -15,5 +15,5 @@ ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore [Install] WantedBy=multi-user.target -Also=proc-xen.mount +Also=proc-xen.service Also=var-lib-xenstored.mount
An upcoming change in systemd will mount xenfs right away, along with all other system mounts. This improves the detection of the virtualization environment, which is currently racy. Some parts of systemd rely on the presence of /proc/xen/capabilities, which will only exist if xenfs is mounted. Since xenfs is mounted by the proc-xen.mount unit, it will be processed very late. Other units may be processed earlier, and if they make use of ConditionVirtualization*= failures may occour. Unfortunately mounting xenfs by systemd as an API filesystem will lead to errors when proc-xen.mount is processed. Since that mount point already exists the unit is considered as failed, and other units that depend on proc-xen.mount will not start. To avoid this the existing proc-xen.mount will be converted into proc-xen.service, which just mounts xenfs manually. All dependencies are updated by this change. The existing conditionals in proc-xen.mount will prevent failures with existing systemd based installations: ConditionPathExists=!/proc/xen/capabilities will prevent execution with a new systemd that mounts xenfs. And this conditional, in combination with ConditionPathExists=/proc/xen, will trigger execution with an old systemd. An absolute path to the mount binary has to be used. /bin/mount is expected to be universally available, nowaways it is a symlink to /usr/bin/mount. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- based on 4.10.0-rc2 Please run autogen.sh: tools/configure.ac | 2 +- tools/hotplug/Linux/systemd/Makefile | 6 +++--- .../Linux/systemd/{proc-xen.mount.in => proc-xen.service.in} | 8 ++++---- tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in | 4 ++-- tools/hotplug/Linux/systemd/xen-init-dom0.service.in | 4 ++-- tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 4 ++-- tools/hotplug/Linux/systemd/xen-watchdog.service.in | 4 ++-- tools/hotplug/Linux/systemd/xenconsoled.service.in | 4 ++-- tools/hotplug/Linux/systemd/xendomains.service.in | 4 ++-- tools/hotplug/Linux/systemd/xendriverdomain.service.in | 4 ++-- tools/hotplug/Linux/systemd/xenstored.service.in | 6 +++--- 11 files changed, 25 insertions(+), 25 deletions(-) rename tools/hotplug/Linux/systemd/{proc-xen.mount.in => proc-xen.service.in} (60%)