Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Systemd service should use /bin/karaf instead of start.sh #72

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

BClark09
Copy link
Member

Closes #70

Signed-off-by: Ben Clark ben@benjyc.uk

@BClark09
Copy link
Member Author

BClark09 commented Jul 13, 2017

@ThomDietrich if it helps for testing, the above changes are available as a full pacakge in the linuxpkg-testing repo:

echo 'deb https://openhab.jfrog.io/openhab/linuxpkg-testing unstable main' | sudo tee /etc/apt/sources.list.d/openhab2.list

@BClark09
Copy link
Member Author

OPENHAB_USERDATA=WHEREAMI

Before PR:
image

After PR:
image

Seems to have gone well!

@ThomDietrich
Copy link
Member

Excellent! I'm sadly not able to do any tests today. I'm curious how the error would look like if there is no java runtime available

@BClark09
Copy link
Member Author

BClark09 commented Jul 14, 2017

There's a weird issue with this:

When a program managed by systemd sources a script, the stout buffer from that script seems delayed. In this case, when karaf calls our setenv script the echo 'Java version higher than 1.8...' does not get the chance to print when exit 1; follows.

To see this error message in systemd, we would have to edit 'setenv' to output with:

echo "Error message" && /bin/true

Which is otherwise completely redundant.

@ThomDietrich
Copy link
Member

ThomDietrich commented Jul 19, 2017

@BClark09 If it is like you suggested and the buffered stdout is cut off too early at the exit, did you find a discussion on the issue? This one looks alright: http://systemd-devel.freedesktop.narkive.com/suwZf7s7/unbuffered-stderr-for-my-systemd-service

Your proposed solution does work because the otherwise useless && /bin/true will delay following commands? Is there a cleaner and more intuitive way to solve this? sd_notify would be an option for systemd. Another option might be sleep but that's hack'y...

@BClark09
Copy link
Member Author

BClark09 commented Jul 19, 2017

There are a few scattered around (this for example), I think this is the systemd bug to watch for this issue though.

@BClark09
Copy link
Member Author

I think this is okay to merge now. I'll wait a bit for feedback in the above thread to see if the ulimit restriction has been adjusted.

@@ -1,5 +1,5 @@
[Unit]
Description=openHAB 2 - empowering the smart home
Description=openHAB 2 - Empowering the smart home
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed un-natural at it was. My intention was to capitalise the S and H in Smart Home (as it is in the docs), but I must have misplaced that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanted to do that a few months ago but got scolded by Kai for it.

http://docs.openhab.org/images/logo.png

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... I'll change it back then.

@@ -29,3 +29,8 @@ EXTRA_JAVA_OPTS=""
#
#OPENHAB_USER=openhab
#OPENHAB_GROUP=openhab
#
# The startmode for the openHAB runtime. Only available for systemctl/systemd systems.
# Defaults to daemon when unset here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we list the options here?

Copy link
Member Author

@BClark09 BClark09 Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't so sure, options for karaf are:

clean   - Deletes $KARAF_DATA contents (/var/lib/openhab2)
debug   - Turns on java debugging options
status  - Checks if running
stop    - Stops instance
console - ???
server  - Starts remote shell and runs the karaf server
daemon  - Same as server, extra exec and return details.
client  - Starts the client to an existing instance

Out of them, we could really only recommend using daemon, debug or debug daemon with systemd. Definitely don't want to mention clean!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Signed-off-by: Ben Clark <ben@benjyc.uk>
@BClark09
Copy link
Member Author

BClark09 commented Jul 21, 2017

@kaikreuzer, @theoweiss, just an FYI: I noticed there was some discussion at openhab/openhab-distro#138 about how to start using systemd. I have changed this to use runtime/bin/karaf daemon so that systemd recognises java as the main process to follow. It is also the recommended way using the karaf service template by the looks of things.

Is there any reason, or can you remember any reason for this being a bad idea? Tests on Debian and Fedora have worked well, so will merge this PR soon.

@kaikreuzer
Copy link
Member

I am not aware of reason against it, but @theoweiss is indeed probably the much better person to ask about it :-)

@ThomDietrich
Copy link
Member

ThomDietrich commented Jul 22, 2017

@BClark09 Instead of the && /bin/true trick we could use systemd-notify/sd_notify. Did you look into them?

@BClark09
Copy link
Member Author

BClark09 commented Jul 22, 2017

I couldn't find a way of incorporating it reliably, it seems to suffer from the same race condition.

Also, should we really add platform specific Linux commands to openHAB's start scripts?

@ThomDietrich
Copy link
Member

I thought about it and couldn't see a reason why not. Of course one would need to properly check for systemd beforehand.

it seems to suffer from the same race condition

Well nice...

@BClark09
Copy link
Member Author

Actually, I may have been wrong in that, was looking in the wrong place. The systemd service needed a bit of tweaking to be of "notify" type.

image

Unfortunately, this still isn't a solution, openHAB would need an implementation of sd_notify to pass down that the service has started successfully. Otherwise, the service start command would keep waiting for one and timeout.

@ThomDietrich
Copy link
Member

You mean this would need to happen inside openHAB or Karaf, not inside the bash script!?

@BClark09
Copy link
Member Author

If you're using sd_notify, then you'll need to pass down a message that the service has started successfully, which should only happen inside the java process started by Karaf if you want to certain that everything is working correctly.

@BClark09 BClark09 closed this Jul 23, 2017
@BClark09 BClark09 reopened this Jul 23, 2017
@BClark09
Copy link
Member Author

BClark09 commented Jul 23, 2017

To summarise:

  • This PR makes sure that some information is available.
  • Additionally, the service now has a larger file limit as suggested by the Karaf template.
  • Since I've re-enabled automatic restarting (with some delay now) these messages only appear when the user uses journalctl -u openhab2 -b (report all openhab2 service messages since last boot). This is what we should suggest to users for debugging.
  • The service still requires some editing of setenv to obtain all output.
  • I've got to stop pressing the close button instead of the comment button.

I'd suggest we merge now and I can make a PR in the distro if using && /bin/true is an acceptable workaround.

@BClark09 BClark09 closed this Jul 23, 2017
@BClark09 BClark09 reopened this Jul 23, 2017
@theoweiss
Copy link
Member

Sorry guys for not participating in this discussion but remodelling our new house and working for my paid job is still eating my whole time. May be I'm back in late autumn. @BClark09 is doing a great job here!!

@BClark09
Copy link
Member Author

No problem @theoweiss! Hope it's all going well!

@BClark09 BClark09 merged commit 953678b into openhab:master Jul 24, 2017
@BClark09 BClark09 deleted the systemd-fix branch July 24, 2017 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants