[WFCORE-6935] Improve systemd service units

[yersan]

Jira issue: https://issues.redhat.com/browse/WFCORE-6935

[jamezp]

Overall I think this looks good. Just a minor suggestion and a couple questions.

[jamezp]

What we might want to do here is use ${project.version} and allow processing of this resource to replace the properties. We could probably do something for wildfly too so it's generic.

[yersan]

I need to check but not sure if that would be fine taking into account that this file contains some other linux commands, I don't want to make them be replaced by maven applying environment properties across this file

[jamezp]

That is a good point. I don't see any others with the pattern ${property.name}, but it's definitely something to consider since that is the linux way.

I was mostly trying to think of a way where we don't have to keep updating the version, but maybe it doesn't matter.

[yersan]

Thanks for the review @jamezp , added some comments related

[yersan]

I need to check but not sure if that would be fine taking into account that this file contains some other linux commands, I don't want to make them be replaced by maven applying environment properties across this file

[jamezp]

This looks good to me. We can figure out the version thing later and really maybe it doesn't make sense to keep it up to date. After all that is the full version and this is core.

[jamezp]

That is a good point. I don't see any others with the pattern ${property.name}, but it's definitely something to consider since that is the linux way.

I was mostly trying to think of a way where we don't have to keep updating the version, but maybe it doesn't matter.

[rpelisse]

To respect Bash's best practises:
# systemctl start "wildfly-${MODE}.service"

[yersan]

Done

[rpelisse]

To avoid hard coding /opt/wildfly everywhere, I would suggest defining a WILDFLY_HOME=/opt/wildfly at the beginning.

[yersan]

Done

[rpelisse]

To follow bash's best practices:

    # cp "wildfly-${MODE}.conf" /etc/sysconfig/
    # cp "wildfly-${MODE}.service" $(pkg-config systemd --variable=systemdsystemunitdir)

[yersan]

Done

[rpelisse]

rm -f "$(pkg-config systemd --variable=systemdsystemunitdir)/wildfly-${MODE}.service"

[yersan]

Done

[rpelisse]

Should we not add systemctl status wildfy and systemctl start wildfly with an example of the output?

[yersan]

At this point, this is the section that describes how to remove the service.

The start / enable / status commands are described in the previous section. There is no sample of status output, though

[rpelisse]

if [ -z "${JBOSS_SYSTEMD_MODE}" ]; then

for consistency on top of best practice

[yersan]

Done

[rpelisse]

SYSTEMD_FILE="${SCRIPT_DIR}/wildfly-${JBOSS_SYSTEMD_MODE}.service"

[yersan]

Done

[rpelisse]

My preference is to add single quote around literals to make it clear to the reader that it's supposed to be fixed characters strings:
JBOSS_SYSTEMD_GROUP="${3:-'wildfly'}"

[yersan]

That would move the quotes to the Systemd unit file as well, is that still valid?

[rpelisse]

As we have JBOSS_HOME here, we should use it at the beginning, to not repeat /opt/wildfly all the time.

Also:
echo "INFO: Using JBOSS_HOME: ${JBOSS_HOME}"

[yersan]

As we have JBOSS_HOME here, we should use it at the beginning, to not repeat /opt/wildfly all the time.

JBOSS_HOME is initialized at the beginning of the script and there is no "/opt/wildfly" path hardcoded in this script, so I don't follow you here.

Also:
echo "INFO: Using JBOSS_HOME: ${JBOSS_HOME}"

Done

[rpelisse]

Interactive shell is a bad idea. I would suggest using opts so that the user can pass a parameters if he wants to generate the systemd file.
We should also aimed at making this script idempotent. (I'll do another review in this regard, after this one)

[yersan]

Interactive shell is a bad idea.

It is not expected this file be invoked by a third-party tool, this is just a utility that is expected to be used by a user to make his life easier when he needs to replace the JBOSS_HOME, user:group in the systemd unit files. Asking interactively for confirmation here allows the user to review what he is going to configure, so I think it is useful here.

Would that convince you to leave it as it is?

[rpelisse]

No :) I really think it's a bad idea.

Bear in mind that this will be used by Ops (or DevOps), they won't expect an interactive shell and will be quite annoyed with it. Also, they are most likely going to automate around this (I've done so in the past, at customers). They may want to automate the deployment of Wildfly based and this and have script (or a tool like Ansible) automatically generate the file on the target system....

And right now, it's impossible to because the script is interactive by default. So if you really want to keep it, at least you should provide a mecanism around it.

IMHO, the best approach, that would match what people do in this kind of script, would be to test if the var is defined and act according to it:

if  [ -z ${SYSTEMD_FILE} ]; then
   echo "No systemd file provided, not generating one."
   exit 0
fi

(last point if somehow you really, really want to keep this interaction thing, I would move into a function, because it's big chunk of code)

[yersan]

@rpelisse Ok, you bring more experience to this from Ansible automation, so I'll remove the interactive question or add a confirm flag for skipping it

[yersan]

@rpelisse I've added a new argument c,--confirm to avoid the interactive mode.

[rpelisse]

sed -e "s|@@@JBOSS_SYSTEMD_SERVER_HOME@@@|${JBOSS_HOME}|g" \
    -e "s|@@@JBOSS_SYSTEMD_USER@@@|${JBOSS_SYSTEMD_USER}|g" \
    -e "s|@@@JBOSS_SYSTEMD_GROUP@@@|${JBOSS_SYSTEMD_GROUP}|g" \
    "${SYSTEMD_FILE}.template" > "${SYSTEMD_FILE}"

[yersan]

Done

[rpelisse]

systemd-analyze verify --recursive-errors=no "${SYSTEMD_FILE}"

[yersan]

Done

[rpelisse]

echo "INFO: Use ${JBOSS_HOME}/bin/systemd/wildfly-${JBOSS_SYSTEMD_MODE}.conf to configure the server environment."

[yersan]

Done

[rpelisse]

Be mindful of where to use quote:
echo sudo journalctl -u $(basename "${SYSTEMD_FILE}")
This approach will yield a more comprehensible error message if, somehow, a space has been inserted in the variable.

[yersan]

Done

[rpelisse]

I would suggest /bin/bash so that we can use bash specific mechanism. It also better because with /bin/sh we should test the script with all sort of shell interpreter.

[yersan]

We use /sh to maximize the portability now and in the future, it is the same shell used by the launch scripts. Using bash opens the door to not be compatible in the future.

[rpelisse]

Yeah, but do you ensure that CI test the script with different flavor of shell? Unlikely... By using /sh you are opening the door to people filling issue because it does not work with "not Bash". It's generally consider best practises to use explicit /bin/bash for this very reason. As this is a new script, I would take the chance to reduce the scope of incoming issue. That being said, it's not a big deal.

[yersan]

Thanks for the review @rpelisse , I fixed most of the suggestion and added some minor comments

[yersan]

Hello @rpelisse could you review this again?

[rpelisse]

@yersan I'm super sorry to have not reviewed this PR (again) more quickly... I did find a couple of areas I think we should look into before we merge.

[rpelisse]

Comparing to the service configuration we use for the Ansible collection for Wildfly, there is a few options you might to consider adding:

1. Creating a PIDFile and giving it to systemd will sensibly enhance its ability to manage the server:

PIDFile=/opt/wildfly/standalone/wildfly.pid

2. Adding the sucessful exit statuses will prevent Systemd to misinterpret when the server is successfully stopping:

SuccessExitStatus = 0 143

3. As Java services tends to start significantly slower than native ones, configuring a timeout will prevent a scenario where apps deployment on the service boot causes Wildfly to be tagged as "fail to start":

TimeoutSec=600

4. That more of a nice to have, but you could configure to automatically restart the service in case of failure:

RestartSec = 120
Restart = on-failure

[rpelisse]

If you the configuration suggested above to the service conf, you should probably update the template accordingly.

[rpelisse]

AFAICS the ony difference between domain and standlone is this variable. I would suggest to avoid duplication, and use some bash savyness to avoid duplicating the files (and this have only one template and one file):

Environment="WILDFLY_SERVER_CONFIG=${CONFIG_FILE:-'standalone.xml'}" # Then we just need to ensure that CONFIG_FILE defaults to domain.xml in the launch script.

[rpelisse]

OK, looking at the next file I reallized, we might simply be able to WILDFLY_SERVER_CONFIG which defaults to 'standalone.xml'.

[rpelisse]

SCRIPT_NAME=$(basename "$0") to SCRIPT_NAME=$(basename "${0}")

[rpelisse]

SCRIPT_DIR="$(dirname "$(realpath "$0")")" to SCRIPT_DIR="$(dirname "$(realpath "${0}")")"

[rpelisse]

Getting into super nickpicking zone :)

Technically by using "" you are telling the script interpreter to look for cmd substitution or variable to be replaced by their content into the string. So it would be better to have 'standalone' and 'domain' instead of "standalone" and "domain".

I did say it was nickpicking, right? ;)

[rpelisse]

I would not echo this variable, as it will only be used for interactive mode. Having this value in logfile of automated install will be ... strange, and might confused ops ("Wait, why my JBOSS is not confirmed? What does it means???".
If you intend to echo the value for debugging purpose, remember that simply running the script with the -x option will echo all the variable values.

[rpelisse]

Oh cool! I'm so going to steal that for the Ansible collection for Wildfly! :)

[rpelisse]

case "$choice" in to case "${choice}" in

Also, as I suspect choice will not change value after the script starts, I would suggest to go uppercase to cleary display that (so rename it to CHOICE).

Ideally, I would prefix any vars declaration by readonly, but I'm not sure if this supported across all interpreter or just a bash feature. It's a good practise to use readonly, because it prevent some already defined variable in the shell to override a value in the script.

[rpelisse]

I'm not sure most ops are going to be thrilled by this output. You may want to consider displaying it only if JBOSS_CONFIRM is set to true? (the more I think about it, the more I think a less confusing name would be better for this variable).

Also, you might want to move this to a separate file and a do a sort of template as you did for the configuratiion file. It might be easier to maintain on the long run.