Bug 957135

I think it would be cool if we could do this conditionally for virtual machines
running on VMware.

Just stick
ConditionVirtualization=vmware
in the [Unit] section of the service file.

(In reply to comment #2)
> Just stick
> ConditionVirtualization=vmware
> in the [Unit] section of the service file.

We have it included in the service file already.

This bug is assigned to generic group. Is someone going to take it over and
make the necessary changes? Is it something I can fix as well?

Can you explain why you are putting Restart=always instead of Restart=on-failure in the unit file?

The requirement for vmtoolsd service is to be always up unless someone manually
stops it. Basically, we don't differentiate between on-failure and on-abort.

What I have noticed while testing it is vmtoolsd is not started by systemd if I
stop vmtoolsd using 'systemctl stop vmtoolsd'. So, Restart=always is providing
the behavior we are looking for.

You cant manually stop it with Restart=always 

And if you are going to hit the daemon with a hammer after one sec you can just as well do it immediately or patch the bug in the dameon that prevents it from cleanly existing

[Unit]
Description=Service for virtual machines hosted on VMware
Documentation=http://open-vm-tools.sourceforge.net/about.php
ConditionVirtualization=vmware

[Service]
Type=forking
PIDFile=/run/vmtoolsd.pid
ExecStart=/usr/bin/vmtoolsd -b /run/vmtoolsd.pid
Restart=on-failure
KillMode=process
KillSignal=SIGKILL

[Install]
WantedBy=multi-user.target

And what about other units like? 

mnt-hgfs.mount

[Unit]
Description=Load VMware shared folders
ConditionPathExists=.host:/
ConditionVirtualization=vmware

[Mount]
What=.host:/
Where=/mnt/hgfs
Type=vmhgfs
Options=defaults,noatime

[Install]
WantedBy=multi-user.target

mnt-hgfs.automount

[Unit]
Description=Load VMware shared folders
ConditionPathExists=.host:/
ConditionVirtualization=vmware

[Automount]
Where=/mnt/hgfs

[Install]
WantedBy=multi-user.target

How are the vmware modules being loaded?
( did not spot any /etc/modules-load.d/ conf files for vmblock, vmhgfs, vmsync, vmci, vsock in the spec file)

(In reply to comment #6)
> You cant manually stop it with Restart=always 
> 
> And if you are going to hit the daemon with a hammer after one sec you can
> just as well do it immediately or patch the bug in the dameon that prevents
> it from cleanly existing
> 
> [Unit]
> Description=Service for virtual machines hosted on VMware
> Documentation=http://open-vm-tools.sourceforge.net/about.php
> ConditionVirtualization=vmware
> 
> [Service]
> Type=forking
> PIDFile=/run/vmtoolsd.pid
> ExecStart=/usr/bin/vmtoolsd -b /run/vmtoolsd.pid
> Restart=on-failure
> KillMode=process
> KillSignal=SIGKILL
> 
> [Install]
> WantedBy=multi-user.target
> 
> And what about other units like? 
> 
> mnt-hgfs.mount
> 
> [Unit]
> Description=Load VMware shared folders
> ConditionPathExists=.host:/
> ConditionVirtualization=vmware
> 
> [Mount]
> What=.host:/
> Where=/mnt/hgfs
> Type=vmhgfs
> Options=defaults,noatime
> 
> [Install]
> WantedBy=multi-user.target
> 
> mnt-hgfs.automount
> 
> [Unit]
> Description=Load VMware shared folders
> ConditionPathExists=.host:/
> ConditionVirtualization=vmware
> 
> [Automount]
> Where=/mnt/hgfs
> 
> [Install]
> WantedBy=multi-user.target
> 

Hmm, I'd expect HGFS mounting be done by whatever package that provides vmhgfs module, not this package.

(In reply to comment #6)
> How are the vmware modules being loaded?
> ( did not spot any /etc/modules-load.d/ conf files for vmblock, vmhgfs,
> vmsync, vmci, vsock in the spec file)

vmci should load automatically since there is backing PCI device; vsock will be loaded on demand when userspace tries to create AF_VSOCK socket; vmblock is not shipped (vmblock-fuse is used instead); vmsync is not shipped (Fedora has FIFREEZE/FITHAW). I think the only module that might need loading is vmhgfs.

Jóhann, I don't know which version of service definition are you referring to.
The service definition we have packaged 'vmtoolsd.service' is given below.

> You cant manually stop it with Restart=always

I have tested this multiple times on Fedora 18 and it works just fine for me.
Here is the log of recent attempt:

# cat /etc/systemd/system/multi-user.target.wants/vmtoolsd.service
[Unit]
Description=Service for virtual machines hosted on VMware
Documentation=http://open-vm-tools.sourceforge.net/about.php
ConditionVirtualization=vmware

[Service]
Type=simple
ExecStart=/usr/bin/vmtoolsd
Restart=always
TimeoutStopSec=2

[Install]
WantedBy=multi-user.target

# systemctl status vmtoolsd
vmtoolsd.service - Service for virtual machines hosted on VMware
          Loaded: loaded (/usr/lib/systemd/system/vmtoolsd.service; enabled)
          Active: active (running) since Fri, 2013-04-26 17:34:31 EDT; 13s ago
            Docs: http://open-vm-tools.sourceforge.net/about.php
        Main PID: 1833 (vmtoolsd)
          CGroup: name=systemd:/system/vmtoolsd.service
                  â 1833 /usr/bin/vmtoolsd

Apr 26 17:34:31 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Started Service ...
# systemctl stop vmtoolsd
# systemctl status vmtoolsd
vmtoolsd.service - Service for virtual machines hosted on VMware
          Loaded: loaded (/usr/lib/systemd/system/vmtoolsd.service; enabled)
          Active: failed (Result: signal) since Fri, 2013-04-26 17:35:04 EDT; 4s ago
            Docs: http://open-vm-tools.sourceforge.net/about.php
         Process: 1833 ExecStart=/usr/bin/vmtoolsd (code=killed, signal=KILL)
          CGroup: name=systemd:/system/vmtoolsd.service

Apr 26 17:34:31 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Started Service ...
Apr 26 17:35:02 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopping Service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: vmtoolsd.service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopped Service ...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Unit vmtoolsd.se...
# systemctl status vmtoolsd
vmtoolsd.service - Service for virtual machines hosted on VMware
          Loaded: loaded (/usr/lib/systemd/system/vmtoolsd.service; enabled)
          Active: failed (Result: signal) since Fri, 2013-04-26 17:35:04 EDT; 39s ago
            Docs: http://open-vm-tools.sourceforge.net/about.php
         Process: 1833 ExecStart=/usr/bin/vmtoolsd (code=killed, signal=KILL)
          CGroup: name=systemd:/system/vmtoolsd.service

Apr 26 17:34:31 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Started Service ...
Apr 26 17:35:02 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopping Service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: vmtoolsd.service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopped Service ...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Unit vmtoolsd.se...
# date
Fri Apr 26 17:35:48 EDT 2013
# systemctl status vmtoolsd
vmtoolsd.service - Service for virtual machines hosted on VMware
          Loaded: loaded (/usr/lib/systemd/system/vmtoolsd.service; enabled)
          Active: failed (Result: signal) since Fri, 2013-04-26 17:35:04 EDT; 50s ago
            Docs: http://open-vm-tools.sourceforge.net/about.php
         Process: 1833 ExecStart=/usr/bin/vmtoolsd (code=killed, signal=KILL)
          CGroup: name=systemd:/system/vmtoolsd.service

Apr 26 17:34:31 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Started Service ...
Apr 26 17:35:02 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopping Service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: vmtoolsd.service...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Stopped Service ...
Apr 26 17:35:04 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Unit vmtoolsd.se...
# systemctl start vmtoolsd
# systemctl status vmtoolsd
vmtoolsd.service - Service for virtual machines hosted on VMware
          Loaded: loaded (/usr/lib/systemd/system/vmtoolsd.service; enabled)
          Active: active (running) since Fri, 2013-04-26 17:36:14 EDT; 2s ago
            Docs: http://open-vm-tools.sourceforge.net/about.php
        Main PID: 1864 (vmtoolsd)
          CGroup: name=systemd:/system/vmtoolsd.service
                  â 1864 /usr/bin/vmtoolsd

Apr 26 17:36:14 prme-elab3-dhcp82.eng.vmware.com systemd[1]: Started Service ...
#

> And if you are going to hit the daemon with a hammer after one sec you can
> just as well do it immediately or patch the bug in the dameon that prevents
> it from cleanly existing

Yes, we need to patch the daemon.

> I think the only module that might need loading is vmhgfs.

Thanks Dmitry for answering the questions. I think vmhgfs will require manual
changes to the system because we are not packaging vmhgfs in this package.

It's a bug in systemd if Restart=always can be manually stopped

(In reply to comment #9)
> It's a bug in systemd if Restart=always can be manually stopped

To be clear, by "manual stop" I mean "systemctl stop <service>". Somehow,
I'm not very convinced this is a bug because systemd knows when a service
has been stopped manually. So, it can differentiate between systemd killing
a service to stop it vs someone else killing it without systemd's knowledge.
I could be wrong but I would need your help understand why is it a bug?

Also, how do I specify multiple conditions for restart?
"Restart=on-failure|on-abort"?

I am curious to know where from you referenced service unit files in your
earlier comment? I'm asking this because your comment does not match with the
service unit file provided by open-vm-tools package.

(In reply to comment #10)
> (In reply to comment #9)
> > It's a bug in systemd if Restart=always can be manually stopped
> 
> To be clear, by "manual stop" I mean "systemctl stop <service>". Somehow,
> I'm not very convinced this is a bug because systemd knows when a service
> has been stopped manually. So, it can differentiate between systemd killing
> a service to stop it vs someone else killing it without systemd's knowledge.
> I could be wrong but I would need your help understand why is it a bug?

Because Restart=always means restart always regardless if someone manually shutdown the unit or not and that is the expected behaviour 

Restart always is only used in special circumstances and vmtoolsd is not special enough. 

Unfortunately the internet spreads ignorance and people have blindly added this to their units and thought that was a smart thing. 

> 
> Also, how do I specify multiple conditions for restart?
> "Restart=on-failure|on-abort"?

Well you would list it twice if it was supported

Restart=on-failure
Restart=on-abort

But that's not something you do you only use either or and the proper way is to add Restart=on-failure followed by three restart tries and some interval between them.

> 
> I am curious to know where from you referenced service unit files in your
> earlier comment? I'm asking this because your comment does not match with the
> service unit file provided by open-vm-tools package.

I dont know who migrated that one and afairc upstream does not ship systemd units or legacy sysv initscript so I simply looked at the init script debian/suse and other distro have been shipping ( which all start the vmtoolsd in the background with pidfile followed by a hack killing the daemon ) and migrated that to native systemd unit along with looking into our and others community forums and see what users where struggling with and generally doing to be able to use open-vm-tools properly. ( the mount units and question about the loading the modules comes from that digging )

btw what's the difference between open-vm-tools vs vmwares own tool ( which probably requires us having to have conflict in the unit with whatever vmware ships ) ?

(In reply to comment #11)
> Because Restart=always means restart always regardless if someone manually
> shutdown the unit or not and that is the expected behaviour 
> 
> Restart always is only used in special circumstances and vmtoolsd is not
> special enough. 

Ok, I tested Restart=on-failure and it seems to work. So, I can change it.

> But that's not something you do you only use either or and the proper way is
> to add Restart=on-failure followed by three restart tries and some interval
> between them.

I believe systemd's default retries and interval values are good enough.

> I dont know who migrated that one and afairc upstream does not ship systemd
> units or legacy sysv initscript

Yes, upstream does not ship service scripts.

> so I simply looked at the init script
> debian/suse and other distro have been shipping ( which all start the
> vmtoolsd in the background with pidfile followed by a hack killing the
> daemon ) and migrated that to native systemd unit

Yes, both the forms of service definitions work for vmtoolsd. The one I have
packaged is much simpler.

> btw what's the difference between open-vm-tools vs vmwares own tool ( which
> probably requires us having to have conflict in the unit with whatever
> vmware ships ) ?

VMware Tools use SysV Init scripts and installation of VMware Tools requires
open-vm-tools to be uninstalled first. So, this is not a conflict at present.

In what way is it simpler and why should distribution switch to that then what they have already tried and tested in their legacy sysv initscript?

(In reply to comment #13)
> In what way is it simpler and why should distribution switch to that then
> what they have already tried and tested in their legacy sysv initscript?

I'm not sure who is "they", are you referring to debian/suse or VMware Tools?

If you are referring to VMware Tools, then I would like to mention that we had
SysV init script originally, but we were asked to provide a systemd unit because
that seems to be the new standard for the packages. Please refer the comment:
https://bugzilla.redhat.com/show_bug.cgi?id=905255#c1 (point #6)

Simone has used init script for distros where systemd is not applicable.

If you are referring to debian/suse, then I would like to mention we had "forking"
type service and during the review, we were asked to use "simple" service because
"simple" is preferable over "forking" type. Please refer review comment here:
https://bugzilla.redhat.com/show_bug.cgi?id=905255#c17

My current plan is to push systemd script open-vm-tools upstream, so that we can
remove it from packaging.

Considering different platforms, please let me know, if you have a different
suggestion.

(In reply to comment #14)
> (In reply to comment #13)
> > In what way is it simpler and why should distribution switch to that then
> > what they have already tried and tested in their legacy sysv initscript?
> 
> I'm not sure who is "they", are you referring to debian/suse or VMware Tools?

Debian/Suse 

> If you are referring to VMware Tools, then I would like to mention that we
> had
> SysV init script originally, but we were asked to provide a systemd unit
> because
> that seems to be the new standard for the packages. Please refer the comment:
> https://bugzilla.redhat.com/show_bug.cgi?id=905255#c1 (point #6)
> 
> Simone has used init script for distros where systemd is not applicable.
> 
> If you are referring to debian/suse, then I would like to mention we had
> "forking"
> type service and during the review, we were asked to use "simple" service
> because
> "simple" is preferable over "forking" type. Please refer review comment here:
> https://bugzilla.redhat.com/show_bug.cgi?id=905255#c17

"Daemons should not be forking if possible, they should "stay" in the foreground as systemd takes care of that. Can the "Type=forking" line be removed?"

Afaikt this review does not have a clue what he's talking about when it comes to systemd and units and our packing guidelines mention nothing about removing "Type=forking".

it however mentions that " Type=simple may simply omit the Type line altogether. "

http://fedoraproject.org/wiki/Packaging:Systemd#.5BService.5D

> 
> My current plan is to push systemd script open-vm-tools upstream, so that we
> can
> remove it from packaging.
> 
> Considering different platforms, please let me know, if you have a different
> suggestion.

I suggest you use the type forking which is consistent with how other distribution have been doing it with their legacy sysv init script which should prevent them for having the need to do downstream changes to the upstream unit file.

Again what's the difference between the open vm tool and the vmtool that vmware ships and what's the gain of shipping and enable it by default if vmware itself recommends that it be removed and replaced with their own tools which indicates using open-vm-tools breaks vmware support?

http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1013096

(In reply to comment #15)
> Again what's the difference between the open vm tool and the vmtool that
> vmware ships

Ideally they should be the same; currently open-vm-tools do not support "unity" mode of workstation.

(In reply to comment #15)
> and what's the gain of shipping and enable it by default if
> vmware itself recommends that it be removed and replaced with their own
> tools which indicates using open-vm-tools breaks vmware support?
> 
> http://kb.vmware.com/selfservice/microsites/search.
> do?language=en_US&cmd=displayKC&externalId=1013096

This stance is changing and VMware will be supporting open-vm-tools packages that are packaged with our involvement. The level of support will depend on the overall guest support tier (i.e. RHEL vs Fedora). The same goes for SLES/OpenSUSE.

(In reply to comment #15)
> > I suggest you use the type forking which is consistent with how other
> distribution have been doing it with their legacy sysv init script which
> should prevent them for having the need to do downstream changes to the
> upstream unit file.

open-vm-tools deliberately omits all unit/init/script files so that distributions package the product in the way most suitable for that particular distribution. So that if distribution's primary init is SysV they can package proper SysV init script and if they are using upstart they can have upstart version of the module, etc, etc.

sysvinit scripts have too many variations, so that policy makes sense but for systemd, the unit file should be standardized across distros (that use systemd of course), so it makes it easier to include it upstream and that is what a lot of components do these days.  man daemon has the autoconf foo requires to make it install the unit file only if the distro is using systemd.

> I suggest you use the type forking which is consistent with how other 
> distribution have been doing it with their legacy sysv init script which should 
> prevent them for having the need to do downstream changes to the upstream unit 
> file.

I don't know why Debian/Suse are using "forking" type. Could you please help me
understand it? If we can run the service as simple and without depending on one
more file for PID, I find that neat, simpler and cool.

Regarding downstream, why not change downstream if there is a better way to do
something? Also, if we can't change downstream that should be OK. I would
imagine future versions of Debian/Suse could follow the approach we are using in
Fedora.

If you are not convinced about using "simple" type, I could change it to
"forking". To be frank, I don't see a convincing explanation for using "forking"
service type in this case.

(In reply to comment #19)
> > I suggest you use the type forking which is consistent with how other 
> > distribution have been doing it with their legacy sysv init script which should 
> > prevent them for having the need to do downstream changes to the upstream unit 
> > file.
> 
> I don't know why Debian/Suse are using "forking" type. Could you please help
> me
> understand it? 

Not really involved with those project so I cant say why they are doing things they are doing within their project 

>If we can run the service as simple and without depending on
> one
> more file for PID, I find that neat, simpler and cool.

Putting your feelings aside this is what is recommended when writing systemd daemons and vmware should consider if the plan is to rework their own daemon(s) or write new one(s).

We ask daemon writers not to fork or even double fork in their processes, but run their event loop from the initial process systemd starts for you. Also, don't call setsid().
    
Don't drop user privileges in the daemon itself, leave this to systemd and configure it in systemd service configuration files. (There are exceptions here. For example, for some daemons there are good reasons to drop privileges inside the daemon code, after an initialization phase that requires elevated privileges.)
    
Don't write PID files
    
Grab a name on the bus
    
You may rely on systemd for logging, you are welcome to log whatever you need to log to stderr.
    
Let systemd create and watch sockets for you, so that socket activation works. Hence, interpret $LISTEN_FDS and $LISTEN_PID as described above.

Use SIGTERM for requesting shut downs from your daemon.

http://0pointer.de/blog/projects/systemd.html
http://0pointer.de/blog/projects/socket-activation.html
http://0pointer.de/blog/projects/socket-activation2.html
http://0pointer.de/blog/projects/journal-submit.html


> 
> Regarding downstream, why not change downstream if there is a better way to
> do
> something? Also, if we can't change downstream that should be OK. I would
> imagine future versions of Debian/Suse could follow the approach we are
> using in
> Fedora.

We have a cross distribution, cross collaboration effort between distribution using systemd as their init system going on, which involves syncing and upstream systemd units between distribution and getting rid of needless difference in them as was present in legacy sysv init script. 

The unit file used on Fedora/RHEL/Debian/Suse/OpenSuse/Arch/Mageia/Gentoo etc should be the one and the same. 

If upstream is not shipping systemd units it should be safe for any distribution planning to to integrate and use systemd in their distribution to pick/clone the unit from any of the distribution that have already chosen systemd for their init system and expect at the same time they be up to speed to systemd changes and implemented with best practices in mind. 


> If you are not convinced about using "simple" type, I could change it to
> "forking". To be frank, I don't see a convincing explanation for using
> "forking"
> service type in this case.

The only personal opinion from me is not to use Restart=always but to use Restart=on-failure instead and the vmware related daemons rewritten to be socket activated where applicable.

(In reply to comment #20)
> Putting your feelings aside this is what is recommended when writing systemd
> daemons and vmware should consider if the plan is to rework their own
> daemon(s) or write new one(s).
> 
> We ask daemon writers not to fork or even double fork in their processes,
> but run their event loop from the initial process systemd starts for you.
> Also, don't call setsid().
>     
> Don't drop user privileges in the daemon itself, leave this to systemd and
> configure it in systemd service configuration files. (There are exceptions
> here. For example, for some daemons there are good reasons to drop
> privileges inside the daemon code, after an initialization phase that
> requires elevated privileges.)
>     
> Don't write PID files
>     
> Grab a name on the bus
>     
> You may rely on systemd for logging, you are welcome to log whatever you
> need to log to stderr.
>     
> Let systemd create and watch sockets for you, so that socket activation
> works. Hence, interpret $LISTEN_FDS and $LISTEN_PID as described above.
> 
> Use SIGTERM for requesting shut downs from your daemon.
> 
> http://0pointer.de/blog/projects/systemd.html
> http://0pointer.de/blog/projects/socket-activation.html
> http://0pointer.de/blog/projects/socket-activation2.html
> http://0pointer.de/blog/projects/journal-submit.html
> 

I think we are in pretty good shape on many points. But, we might require some
work on SIGTERM and socket activation. I can't commit to this at the moment,
but we can definitely evaluate it and keep it on the list of things to do in
the future.

> We have a cross distribution, cross collaboration effort between
> distribution using systemd as their init system going on, which involves
> syncing and upstream systemd units between distribution and getting rid of
> needless difference in them as was present in legacy sysv init script. 
> 
> The unit file used on Fedora/RHEL/Debian/Suse/OpenSuse/Arch/Mageia/Gentoo
> etc should be the one and the same. 
> 
> If upstream is not shipping systemd units it should be safe for any
> distribution planning to to integrate and use systemd in their distribution
> to pick/clone the unit from any of the distribution that have already chosen
> systemd for their init system and expect at the same time they be up to
> speed to systemd changes and implemented with best practices in mind. 

Given that "forking" type and PIDFile are not really needed for vmtoolsd. I
think it makes sense to fix the unit file in Debian/Suse at some point.

> The only personal opinion from me is not to use Restart=always but to use
> Restart=on-failure instead and the vmware related daemons rewritten to be
> socket activated where applicable.

I have fixed Restart=on-failure. I have increased the TimeoutStopSec to 5 sec.
I really want to keep it because I think giving vmtoolsd a chance to shutdown
cleanly will help when it starts handling SIGTERM properly. We could also
remove TimeoutStopSec at that point. FWIW, existing VMware Tools script also
waits for 10 sec after sending SIGTERM and then sends SIGKILL.

Can we do socket activation later in the future?

(In reply to comment #21)
> (In reply to comment #20)
> > Putting your feelings aside this is what is recommended when writing systemd
> > daemons and vmware should consider if the plan is to rework their own
> > daemon(s) or write new one(s).
> > 
> > We ask daemon writers not to fork or even double fork in their processes,
> > but run their event loop from the initial process systemd starts for you.
> > Also, don't call setsid().
> >     
> > Don't drop user privileges in the daemon itself, leave this to systemd and
> > configure it in systemd service configuration files. (There are exceptions
> > here. For example, for some daemons there are good reasons to drop
> > privileges inside the daemon code, after an initialization phase that
> > requires elevated privileges.)
> >     
> > Don't write PID files
> >     
> > Grab a name on the bus
> >     
> > You may rely on systemd for logging, you are welcome to log whatever you
> > need to log to stderr.
> >     
> > Let systemd create and watch sockets for you, so that socket activation
> > works. Hence, interpret $LISTEN_FDS and $LISTEN_PID as described above.
> > 
> > Use SIGTERM for requesting shut downs from your daemon.
> > 
> > http://0pointer.de/blog/projects/systemd.html
> > http://0pointer.de/blog/projects/socket-activation.html
> > http://0pointer.de/blog/projects/socket-activation2.html
> > http://0pointer.de/blog/projects/journal-submit.html
> > 
> 
> I think we are in pretty good shape on many points. But, we might require
> some
> work on SIGTERM and socket activation. I can't commit to this at the moment,
> but we can definitely evaluate it and keep it on the list of things to do in
> the future.

Given that the same code base is supposed to run on older Linux distributions (pre systemd) as well as Solaris and FreeBSD, I do not believe we will have dependencies on systemd-provided facilities on open-vm-tools for now.

Also socket activation does not quite make sense for us given the fact that hypervisor may communicate with the daemon via hypervisor port.

Thanks,
Dmitry

you don't have hard dependencies on systemd.  the systemd specific stuff becomes a no-op on other distros if you follow the process outlined in the documents above.  that is just a suggestion and we don't mandate it however.

I've now pushed an update to documentation for Restart= to git and to http://www.freedesktop.org/software/systemd/man/systemd.service.html.

The policy for Fedora regarding Restart= was discussed during during a sprint in February. The consensus was that there's no one setting which fits all cases, and must be decided on the discretion of individual maintainers. Restart=on-failure or Restart=on-abort makes sense for daemons which are (a) buggy and fail intermittently, (b) depend on resources which can fail transiently, usually network-related, but don't deal with that. Restart=always makes only sense for daemons which do (a) or (b), and in addition, falsely return 0, or are something like a cron job that needs to run repeatedly. Like Jóhann said, Restart=always almost never is the right choice. Before setting Restart=on-failure you should check if vmtoolsd can hit an error condition that can go away "by itself", i.e. not a config or compatibility error. And even if there's such a condition, e.g. a race condition in driver loading, it would be better to fix the real bug where it happens. If such conditions are not pertinent to vmtoolsd, then enabling automatic restart is just annoying to the user and a potential attack vector.

> Before setting Restart=on-failure you should check if vmtoolsd can hit an error
> condition that can go away "by itself", i.e. not a config or compatibility
> error. And even if there's such a condition, e.g. a race condition in driver
> loading, it would be better to fix the real bug where it happens.

For vmtoolsd, automatic restart is only useful in cases where service might
crash due to a coding bug. Given that code has been very stable over last few
years, it is going to be some rare untested case only. So, restart setting is
not really that useful and can definitely be removed, but I think we should
keep it to avoid the users having to restart the service manually in case of
unforeseen vmtoolsd service crashes.

> If such conditions are not pertinent to vmtoolsd, then enabling automatic
> restart is just annoying to the user and a potential attack vector.

Sorry, I did not get this. Users can always issue a 'systemclt stop vmtoolsd' 
if they really want to stop this service.

Ravindra,

I do not believe that our "other" tools, at least on Linux and other Unices, restart vmtoolsd if it dies so I think we should keep the same behavior in open--vm-tools.

Thanks,
Dmitry

(In reply to comment #25)
> > Before setting Restart=on-failure you should check if vmtoolsd can hit an error
> > condition that can go away "by itself", i.e. not a config or compatibility
> > error. And even if there's such a condition, e.g. a race condition in driver
> > loading, it would be better to fix the real bug where it happens.
> 
> For vmtoolsd, automatic restart is only useful in cases where service might
> crash due to a coding bug. Given that code has been very stable over last few
> years, it is going to be some rare untested case only. So, restart setting is
> not really that useful and can definitely be removed, but I think we should
> keep it to avoid the users having to restart the service manually in case of
> unforeseen vmtoolsd service crashes.
OK, makes sense.

> > If such conditions are not pertinent to vmtoolsd, then enabling automatic
> > restart is just annoying to the user and a potential attack vector.
> 
> Sorry, I did not get this.
By "a potential attack vector" I mean a situation where a crash can be exploited in e.g. 1% of attempts, and restarting the service allows the attacker to repeat attempts.

By "annoying" I mean a situation where a crash, e.g. due to config issue, is
repeated (a few times, or many times), where it's obvious that the error will be the same every time. This drains resources and fill up logs.

That's why it's good to have the server return different exit codes on different class of issues and restart only on some of them.

> Users can always issue a 'systemclt stop
> vmtoolsd' if they really want to stop this service.
Sure.

Based on all the discussions above, I have removed the "Restart" directive from
the service definition. Could you please add this service to F19 and Rawhide
presets now?

(In reply to comment #17)
> (In reply to comment #15)
> > > I suggest you use the type forking which is consistent with how other
> > distribution have been doing it with their legacy sysv init script which
> > should prevent them for having the need to do downstream changes to the
> > upstream unit file.
> 
> open-vm-tools deliberately omits all unit/init/script files so that
> distributions package the product in the way most suitable for that
> particular distribution. So that if distribution's primary init is SysV they
> can package proper SysV init script and if they are using upstart they can
> have upstart version of the module, etc, etc.

Well, it is explicitly our intention with systemd to get these unit files included upstream because they should be the same for all distros that use systemd (which are the big majority now).

To add these units to the preset file I'd have to know their name. Which units precisely should be enabled by default?

open-vm-tools.service

http://pkgs.fedoraproject.org/cgit/open-vm-tools.git/tree/

I have added this to the preset files of f18 and f19, didn't rebuild the packages with it though. THis will be included in the next rebuilds however.

Hello, the name of the service is vmtoolsd.service and not open-vm-tools.service.

That is only the source name, as per the package guidelines it starts with the package name.

either this is open-vm-tools.service and you have a dummy link for backwards compatability to vmtoolsd.service ( since that name should not be changed after install ) or it should be simply called vmtoolsd.service instead of that current hack in the spec file

Which(In reply to comment #34)
> either this is open-vm-tools.service and you have a dummy link for backwards
> compatability to vmtoolsd.service ( since that name should not be changed
> after install ) or it should be simply called vmtoolsd.service instead of
> that current hack in the spec file

Which hack are you referring to? It's simply installing the suorce file to vmtoolsd.service (line 97).

Wut? You are naming the file differently in the fedpkg repo than in the system? Please don't do this. THis is highly confusing....

Where do the packaging guideliness propose any renaming like that?

Fixing name makes sense. Backwards compatibility part is not clear to me.
The service name is same i.e. 'vmtoolsd' across f17 (SysV init), f18 and
f19. So, I'm not able to follow backwards compatibility issue. Could you
please elaborate more on this?

I remember a page in the packaging guidelines that states that the source files must begin with %{name}; but I might totally be wrong as I can't find it. Maybe it was for the name of patches?

Ravindra, can you change the spec file to have the source file name as vmtoolsd.service? I will change accordingly in the other branches.

Working on it.

If "vmtoolsd" is the only name you ever used for the service, then you do not need any backwards compat. 

(In reply to comment #38)
> I remember a page in the packaging guidelines that states that the source
> files must begin with %{name}; but I might totally be wrong as I can't find
> it. Maybe it was for the name of patches?

I certainly never did that for any of my packages and rpmlint never complained, and nobody else wither...

> Ravindra, can you change the spec file to have the source file name as
> vmtoolsd.service? I will change accordingly in the other branches.

I will make the change in the preset file now to name it "vmtoolsd.service"

(In reply to comment #37)
> Fixing name makes sense. Backwards compatibility part is not clear to me.
> The service name is same i.e. 'vmtoolsd' across f17 (SysV init), f18 and
> f19. So, I'm not able to follow backwards compatibility issue. Could you
> please elaborate more on this?

If you rename a service/unit you would need to provide a backwards compatibility to the old name and you do so via symlink from the old name to the new name of the unit. 

Sometime in the future we hopefully manage to sync needless difference in service/unit names between distribution ( for example apache on debian vs httpd on fedora ) and when we do distribution will need to solve it via symlink during the migration phaze as I mention above

Ok, I'm not sure if we need to change anything here. The service file is called
"open-vm-tools.service" inside the package. But, when rpm is installed, it is
always installed as "vmtoolsd.service".

So, I think the preset change Lennart has made should just work fine and I
believe there is no change needed in the package. Am I missing something here?

Here is Lennart's change:
http://pkgs.fedoraproject.org/cgit/systemd.git/commit/?h=f19

Never mind. Got it clarified from Simone in a separate mail. It is going to
work normally as it stands now. Just the source file name change is needed
which is mostly a cosmetic change now. I will make the change and push a new
update.

(In reply to comment #41)
> If you rename a service/unit you would need to provide a backwards
> compatibility to the old name and you do so via symlink from the old name to
> the new name of the unit. 

This is basically a brand new package.  Well .. it's been in
the distro for maybe 2 weeks, but I think we don't need to worry
about backwards compatibility of the service file.

systemd-201-2.fc18.6 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/FEDORA-2013-5452/systemd-201-2.fc18.6

Package systemd-201-2.fc18.6:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing systemd-201-2.fc18.6'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-5452/systemd-201-2.fc18.6
then log in and leave karma (feedback).

systemd-201-2.fc18.6 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.