Bug 1121783

Summary: systemd ExecStart handling is confusing
Product: Red Hat Enterprise Linux 7 Reporter: Karl Hastings <kasmith>
Component: systemdAssignee: systemd-maint
Status: CLOSED NOTABUG QA Contact: qe-baseos-daemons
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 7.0CC: jonstanley, lnykryn, msekleta, systemd-maint-list
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-08-25 11:43:59 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Karl Hastings 2014-07-21 21:11:53 UTC 
Description of problem:
The way systemd handles duplicate ExecStart lines is confusing.  Instead of throwing an error, later entries should override earlier entries.

I.e.

~~~
root@localhost system# cat docker.service 
.include /usr/lib/systemd/system/docker.service

[Service]
ExecStart=/usr/bin/docker -d --selinux-enabled -H fd:// $DOCKER_OPTS
root@localhost system# systemctl start docker.service
Jul 10 20:23:10 localhost systemd[1]: docker.service has more than one ExecStart setting, which is only allowed for Type=oneshot services. Refusing.
~~~

The workaround is to clear the entry first, and then set it:
~~~
.include /usr/lib/systemd/system/docker.service

[Service]
ExecStart=
ExecStart=/usr/bin/docker -d --selinux-enabled -H fd:// $DOCKER_OPTS
~~~

But this seems unnecessary (and imho somewhat confusing) since it's not valid to have multiple ExecStart entries in this type of unit.

Version-Release number of selected component (if applicable):
systemd-208-11.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Attempt to overwrite an ExecStart line as above

Actual results:
service refuses to run

Expected results:
service runs with overwritten options.

Additional info:
Comment 2 Michal Sekletar 2014-07-22 08:30:00 UTC 
I don't think we want to change current behaviour. Altering ExecStart option for service really changes vendor supplied semantics substantially thus in such cases copying the whole thing to /etc is advised. I am tempted to close as NOTABUG.
Comment 3 Jon Stanley 2014-07-22 12:57:25 UTC 
While there is an error message that gives some indication of what went wrong, I personally think that since the operation makes no sense whatsoever, the overridden attribute should take precedence.

It breaks the promise of systemd that "configuration in /etc overrides configuration in /usr" to not do this. Of course, I could just not .include the system-provided unit and write my own complete unit, but then you lose the maintainability value that including the system-provided unit gives you.
 
An argument could also possibly be made for what I'll call "non-accretive" values in /etc. Whereas the following is perfectly valid in any unit:

[Service]
Environment=FOO=bar
Environment=BAZ=bar

One could quite reasonably expect that if the first line was in a unit in /usr, and the second in an overriding unit in /etc which included the first unit, then $FOO wouldn't be set in the resultant environment - i.e. the content of the second unit really DOES override the content of the first.

I realize that this results in special handling of .include'd units, possibly adding something like '.includeoverride' or something of the sort so that people can opt-in to this change in behavior.
Comment 4 Jon Stanley 2014-07-22 13:03:30 UTC 
(In reply to Jon Stanley from comment #3)

> unit, then $FOO wouldn't be set in the resultant environment - i.e. the
> content of the second unit really DOES override the content of the first.

I really should clarify this and say that "identical keys in the second included unit override the values in the first, and reset them"

Let me know if this is unclear, I realize my first wording of this was about as clear as mud :)
Comment 5 Lukáš Nykrýn 2014-08-25 11:43:59 UTC 
Closed for reason mention in Comment 2. Changing ExecStart is substantial change how the unit works. ALso this stanza for example uses variables given by Environment= , this could break easily during update.

Btw using .include is deprecated, you should use drop-in files. For the service foo.service create foo.service.d/SOMETHING.conf.