Bug 1221896

Cut/paste fail in the initial 'steps to reproduce' section, I wrote:

JAVA_OPTS="$JAVA_OPTS -Duser.language=en -Dfile.encoding=UTF-8 -Xms128M -Xmx512M -XX:PermSize=64M -XX:MaxPermSize=512M -Djavax.xml.transform.TransformerFactory=com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"
JAVA_OPTS="-Duser.language=en -Dfile.encoding=UTF-8 -Xms128M -Xmx512M -XX:PermSize=64M -XX:MaxPermSize=512M -Djavax.xml.transform.TransformerFactory=com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"

should be just:

JAVA_OPTS="$JAVA_OPTS -Duser.language=en -Dfile.encoding=UTF-8 -Xms128M -Xmx512M -XX:PermSize=64M -XX:MaxPermSize=512M -Djavax.xml.transform.TransformerFactory=com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"

(is there a way to edit the initial description for accuracy?)

As proposed solution you can set 
TOMCAT_CFG_LOADED=0
and TOMCAT_CFG=<your file with your java_opts extensions>

I believe we should rely on systemd environmentfile load for generic configuration, but provide an option to load addtional custom conviguration via shell.
So we are going to add something like $TOMCAT_SHELL_CFG which will be sourced in tomcat-preamble script.

Thanks for looking at this, and discussing it internally (I presume).

I cant see comments 3 to 12 inclusive, but from Ivans comment I dont think this is a good solution. From my perspective as a user the app fails to start when I do something that Is expected to work and does on other operating systems. 

With this proposed solution, the app still breaks in the same way. I would then need to understand that I need to put my config in another place just to get shell expansion. By the time I understand this, I might as well have just not used shell expansion in the first place. 

Perhaps comments at the top of /etc/sysconfig/tomcat explaining not to use shell syntax would be better?

Its not ideal, but it does not complicate further the already complicated bootstrap process by introducing more files which can contain more bits of config. Less is more IMHO.

The apps which broke at my institution when the Tomcat 7.0.42-8 updates were installed have since been configured without the need for shell expansion and are working. We would not use this proposed fix preferring to keep our config locations standard.

As I see it your best options in my order of preference are:

1) dont load the environment from systemd
2) modify systemd to provide a way to source its environment via a shell, and use that from tomcat.service
3) do nothing but document this shortcoming clearly at top of /etc/sysconfig/tomcat and anywhere guides or FAQ produced by redhat on this subject.

Exact the same behavior occurs when using /etc/tomcat/tomcat.conf instead of sytemd /etc/sysconfig/tomcat (and when using CATALINA_OPTS instead of JAVA_OPTS).

The sysconfig file is there to allow overrides (not additions to the variables) of the variables provided by the conf file and is loaded last (by systemd). The text of the sysconfig provided by RHEL 7 should be updated to reflect that and in Fedora the sysconfig looks like this http://pkgs.fedoraproject.org/cgit/rpms/tomcat.git/tree/tomcat-8.0.sysconfig?h=f23. However, with the conf file defined in the EnvironmentFile directive it is being loaded twice; once by systemd and then sourced by the preamble. Variables can be present in the conf file as long as sysconfig overrides them so that systemd doesn't try and load them, but if they aren't overridden systemd fails to start the service. I'm not sure why the conf file is loaded twice, but if I remove the EnvironmentFile directive (as stated above) which loads the conf file, then shell expansion works fine and everything is happy as long as sysconfig doesn't contain any variables requiring expansion.

I think that removing the EnvironmentFile for the conf file is the best move (assuming that sysconfig doesn't have variables) and then documenting that sysconfig is solely there to override the conf and doesn't support expansion. This is the smallest change that would still work without breaking the systemd design. I don't really want to the sysconfig load into the preamble or elsewhere because then we're not really using the systemd unit for anything (doing that is the same as the sysV scripts). I'm only proposing removing the conf file because its already being loaded in the preamble. An alternative to that might be to use the TOMCAT_CFG variable (not adding a new variable and config combination) to relocate the config for each instance, if you require shell expansion.

I'll continue looking into this and discussing internally, but I wanted to provide an update for some feedback from those involved here.

(In reply to Coty Sutherland from comment #16)
[..]
> I think that removing the EnvironmentFile for the conf file is the best move
> (assuming that sysconfig doesn't have variables) and then documenting that
> sysconfig is solely there to override the conf and doesn't support
> expansion. This is the smallest change that would still work without
> breaking the systemd design. 
[..]

Agree, removing the EnvironmentFile param results in the desired behavior and doesn't impact systemd behavior.

It is usual practice to add coment at the top of default config that it is not recommended to edit it. User is encouraged to add and edit his custom configs in special conf.d folder. So as an option we may add /etc/tomcat/conf.d folder and add shell code to source conf files from it.

(In reply to Ivan Afonichev from comment #18)
> configs in special conf.d folder. So as an option we may add
> /etc/tomcat/conf.d folder and add shell code to source conf files from it.

I think that would be OK also, but that doesn't affect the EnvironmentFile removal or update to sysconfig. So I'm still proposing that we pull in the sysconfig file from fc23's tomcat package (which is better documented for this issue) and removing EnvironmentFile for now. I'm also open to adding a conf.d dir, if you think that would help here. I'm of the mind that modifying the tomcat.conf would be sufficient though.

I believe we should source some default file using systemd. But let users source another one(s) using shell scripts.
Maybe we should set EnvironmentFile=/etc/sysconfig/tomcat  with all contents of current /etc/tomcat/tomcat.conf and left /etc/tomcat/tomcat.conf empty with comment "Feel free to add your overrides here. Shell invocations will work here."

Created attachment 1120897 [details]
proposed patch

I took Ivan's last comment and made a patch from it. I think that moving the default configuration into sysconfig and leaving the tomcat.conf for overrides with shell expansion further decouples the tomcat source from the service unit configuration. I amended the messages at the top of sysconfig/tomcat and tomcat.conf to further document the shell expansion issue. I think this is a good solution for now, but feedback is welcome.

Please note that per https://bugzilla.redhat.com/show_bug.cgi?id=1311771#c11 Bugzilla Bug #1311771 - Tomcat 8.0.32 update breaks FreeIPA and Dogtag installations, an error in the F22, F23, and F24 packages occurred.

The problematic change was reverted in http://koji.fedoraproject.org/koji/buildinfo?buildID=741084 tomcat-8.0.32-4.fc25 which fixed the problem:

    * https://bugzilla.redhat.com/show_bug.cgi?id=1311771#c10
    * https://bugzilla.redhat.com/show_bug.cgi?id=1311771#c11

Tested out the following F24 package:

    * http://koji.fedoraproject.org/koji/buildinfo?buildID=741325
      tomcat-8.0.32-4.fc24

Everything works as advertised!

Thanks,
-- Matt

The fix for this should align with Fedora's resolution of adding a conf.d directory for shell expansion needs while continuing to closely maintain the systemd unit. That solution was the commit here http://pkgs.fedoraproject.org/cgit/rpms/tomcat.git/commit/?id=7d21a72

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2016-2599.html