[WFCORE-6935] Improve systemd service units #6179

yersan · 2024-09-18T10:37:19Z

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 · 2024-10-04T15:39:11Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+    # unzip -d /opt wildfly-34.0.0.Final.zip
+    # ln -s /opt/wildfly-34.0.0.Final /opt/wildfly


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.

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

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.

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

core-feature-pack/common/src/main/resources/content/bin/systemd/wildfly-domain.service

yersan

Thanks for the review @jamezp , added some comments related

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

core-feature-pack/common/src/main/resources/content/bin/systemd/wildfly-domain.service

yersan · 2024-10-08T16:29:58Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+    # unzip -d /opt wildfly-34.0.0.Final.zip
+    # ln -s /opt/wildfly-34.0.0.Final /opt/wildfly


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 · 2024-10-09T14:45:22Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+    # unzip -d /opt wildfly-34.0.0.Final.zip
+    # ln -s /opt/wildfly-34.0.0.Final /opt/wildfly


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.

core-feature-pack/common/src/main/resources/content/bin/systemd/wildfly-domain.service

rpelisse · 2024-10-15T12:11:52Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+
+== Start and enable WildFly
+
+    # systemctl start wildfly-$MODE.service


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

rpelisse · 2024-10-15T12:12:41Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+
+== Configure systemd
+
+    # cd /opt/wildfly/bin/systemd


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

rpelisse · 2024-10-15T12:13:40Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+
+    # cd /opt/wildfly/bin/systemd
+    # cp wildfly-$MODE.conf /etc/sysconfig/
+    # cp wildfly-$MODE.service $(pkg-config systemd --variable=systemdsystemunitdir)


To follow bash's best practices:

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

rpelisse · 2024-10-15T12:14:23Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+
+== Remove WildFly systemd service
+
+    # rm -f $(pkg-config systemd --variable=systemdsystemunitdir)/wildfly-$MODE.service


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

rpelisse · 2024-10-15T12:14:59Z

core-feature-pack/common/src/main/resources/content/bin/systemd/README

+
+    # rm -f $(pkg-config systemd --variable=systemdsystemunitdir)/wildfly-$MODE.service
+    # rm -f /etc/sysconfig/wildfly-$MODE.conf
+    # systemctl daemon-reload


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

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 · 2024-10-15T12:15:42Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+JBOSS_HOME="$(realpath "$SCRIPT_DIR/../..")"
+JBOSS_SYSTEMD_MODE="${1}"
+
+if [ -z "$JBOSS_SYSTEMD_MODE" ]; then


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

for consistency on top of best practice

rpelisse · 2024-10-15T12:16:09Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+  exit 0;
+fi
+
+SYSTEMD_FILE="$SCRIPT_DIR/wildfly-${JBOSS_SYSTEMD_MODE}.service"


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

rpelisse · 2024-10-15T12:16:53Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+
+SYSTEMD_FILE="$SCRIPT_DIR/wildfly-${JBOSS_SYSTEMD_MODE}.service"
+JBOSS_SYSTEMD_USER="${2:-wildfly}"
+JBOSS_SYSTEMD_GROUP="${3:-wildfly}"


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'}"

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

rpelisse · 2024-10-15T12:18:27Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+JBOSS_SYSTEMD_GROUP="${3:-wildfly}"
+
+echo "INFO: Systemd Unit File to generate: ${SYSTEMD_FILE}"
+echo "INFO: Using JBOSS_HOME: $JBOSS_HOME"


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}"

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 · 2024-10-15T12:19:38Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+echo "INFO: User: ${JBOSS_SYSTEMD_USER}"
+echo "INFO: Group: ${JBOSS_SYSTEMD_GROUP}"
+
+while true; do


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)

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?

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)

@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

rpelisse · 2024-10-15T12:20:16Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+  esac
+done
+
+sed -e "s|@@@JBOSS_SYSTEMD_SERVER_HOME@@@|$JBOSS_HOME|g" \


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}"

rpelisse · 2024-10-15T12:20:30Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+    -e "s|@@@JBOSS_SYSTEMD_GROUP@@@|$JBOSS_SYSTEMD_GROUP|g" \
+    "${SYSTEMD_FILE}.template" > "$SYSTEMD_FILE"
+
+systemd-analyze verify --recursive-errors=no "$SYSTEMD_FILE"


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

rpelisse · 2024-10-15T12:20:48Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+echo ""
+echo "INFO: Systemd Unit File generated."
+echo "INFO: The ${JBOSS_SYSTEMD_USER}:${JBOSS_SYSTEMD_GROUP} are the user:group configured to launch the server. You have to ensure this user and group exist and have the necessary permissions to read and launch the server."
+echo "INFO: Use $JBOSS_HOME/bin/systemd/wildfly-${JBOSS_SYSTEMD_MODE}.conf to configure the server environment."


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

rpelisse · 2024-10-15T12:22:04Z

core-feature-pack/common/src/main/resources/content/bin/systemd/generate_systemd_unit.sh

+echo "sudo systemctl start $(basename $SYSTEMD_FILE)"
+echo ""
+echo "INFO: In case of issues, you can check the service logs with:"
+echo "sudo journalctl -u $(basename $SYSTEMD_FILE)"


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.

rpelisse · 2024-10-15T12:23:14Z

core-feature-pack/common/src/main/resources/content/bin/systemd/launch.sh

@@ -0,0 +1,26 @@
+#!/bin/sh


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.

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.

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.

Jira issue: https://issues.redhat.com/browse/WFCORE-6935 [WFCORE-6935] Minor fix for README

yersan

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

wildfly-ci · 2024-10-15T22:12:30Z

Core -> Full Integration Build 14278 outcome was FAILURE using a merge of bcf9528
Summary: Tests failed: 1 (1 new), passed: 5882, ignored: 118 Build time: 04:03:27

Failed tests

TestSuite: org.eclipse.microprofile.rest.client.tck.sse.ReactiveStreamsPublisherTckTest.stochastic_spec103_mustSignalOnMethodsSequentially: java.lang.NullPointerException
	at org.reactivestreams.tck.PublisherVerification$5$1.run(PublisherVerification.java:257)
	at org.reactivestreams.tck.PublisherVerification.activePublisherTest(PublisherVerification.java:1135)
	at org.reactivestreams.tck.PublisherVerification$5.apply(PublisherVerification.java:251)
	at org.reactivestreams.tck.PublisherVerification$5.apply(PublisherVerification.java:248)
	at org.reactivestreams.tck.PublisherVerification.stochasticTest(PublisherVerification.java:1204)
	at org.reactivestreams.tck.PublisherVerification.stochastic_spec103_mustSignalOnMethodsSequentially(PublisherVerification.java:248)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:136)
	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:658)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:219)
	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:923)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:192)
	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.testng.TestRunner.privateRun(TestRunner.java:808)
	at org.testng.TestRunner.run(TestRunner.java:603)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:429)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:423)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:383)
	at org.testng.SuiteRunner.run(SuiteRunner.java:326)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1249)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1169)
	at org.testng.TestNG.runSuites(TestNG.java:1092)
	at org.testng.TestNG.run(TestNG.java:1060)
------- Stderr: -------
[AbstractSseTest] [ERROR] launchServer caughtException 
java.lang.RuntimeException: Failed to start SSE HTTP server
	at org.eclipse.microprofile.rest.client.tck.sse.HttpSseServer.start(HttpSseServer.java:46)
	at org.eclipse.microprofile.rest.client.tck.sse.AbstractSseTest.lambda$launchServer$0(AbstractSseTest.java:48)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.io.IOException: Failed to bind to ::/0:0:0:0:0:0:0:0:10000
	at org.eclipse.jetty.server.ServerConnector.openAcceptChannel(ServerConnector.java:349)
	at org.eclipse.jetty.server.ServerConnector.open(ServerConnector.java:310)
	at org.eclipse.jetty.server.AbstractNetworkConnector.doStart(AbstractNetworkConnector.java:80)
	at org.eclipse.jetty.server.ServerConnector.doStart(ServerConnector.java:234)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73)
	at org.eclipse.jetty.server.Server.doStart(Server.java:401)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73)
	at org.eclipse.microprofile.rest.client.tck.sse.HttpSseServer.start(HttpSseServer.java:43)
	... 6 more

yersan added Feature This PR adds a new feature to WildFly missing-reqs This PR is missing external requirements before it can be merged labels Sep 18, 2024

yersan force-pushed the WFCORE-6935 branch 3 times, most recently from 0af8db3 to 5e7a85b Compare September 18, 2024 15:08

This comment was marked as off-topic.

Sign in to view

jamezp reviewed Oct 4, 2024

View reviewed changes

yersan commented Oct 8, 2024

View reviewed changes

jamezp approved these changes Oct 9, 2024

View reviewed changes

rpelisse reviewed Oct 15, 2024

View reviewed changes

[WFCORE-6935] Improve systemd service units

96f6ca3

Jira issue: https://issues.redhat.com/browse/WFCORE-6935 [WFCORE-6935] Minor fix for README

yersan force-pushed the WFCORE-6935 branch from 5e7a85b to 96f6ca3 Compare October 15, 2024 15:34

yersan commented Oct 15, 2024

View reviewed changes

yersan requested a review from rpelisse October 15, 2024 15:37

[WFCORE-6935] Correct naming for systemd, unit, file

bcf9528

		# unzip -d /opt wildfly-34.0.0.Final.zip
		# ln -s /opt/wildfly-34.0.0.Final /opt/wildfly


		== Start and enable WildFly

		# systemctl start wildfly-$MODE.service


		== Remove WildFly systemd service

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

[WFCORE-6935] Improve systemd service units #6179

Are you sure you want to change the base?

[WFCORE-6935] Improve systemd service units #6179

Conversation

yersan commented Sep 18, 2024

This comment was marked as off-topic.

jamezp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamezp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yersan left a comment

Choose a reason for hiding this comment

wildfly-ci commented Oct 15, 2024

Failed tests