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 and packaging (RPM) improvments from discussion in #213 #212

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@remicollet
Copy link
Contributor

remicollet commented Jan 15, 2019

forking is old deprecated mode.
simple (or even better notify, when supported) should be preferred.

@VBart VBart requested a review from defanator Jan 15, 2019

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Jan 15, 2019

Notice that current version use an hardcoded PIDFile, while this path is provided by $UNITD_OPTIONS (from sysconfig file) which may result to very strange issues. With Type=simple, this pid file is no more used by systemd. Thus also fixing a possible issue.

Improve systemd dependencies
Also use systemd on Fedora
Drop /etc/sysconfig/unit configuration file for systemd distributions
Switch from EnvironmentFile to Environment
Document how to create a oveeride file (systemctl edit unit)

@remicollet remicollet changed the title switch systemd unit type from forking to simple systemd improvments from discussion in #213 Jan 15, 2019

@@ -1,5 +1,5 @@
# distribution specific definitions
%define use_systemd (0%{?rhel} && 0%{?rhel} >= 7) || (0%{?suse_version} >= 1315)
%define use_systemd 0%{?rhel} >= 7 || 0%{?fedora} >= 19 || 0%{?suse_version} >= 1315

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

brackets needed, fixed in mext commit

[Unit]
Description=NGINX Unit
Wants=network-online.target
After=network-online.target

[Service]
Type=simple
EnvironmentFile=-/etc/sysconfig/unit
Environment="UNITD_OPTIONS=--log /var/unit/unit.log --pid /run/unit.pid"

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

Typo there... fixed in next

@@ -16,9 +16,11 @@ After=network-online.target

[Service]
Type=simple
Environment="UNITD_OPTIONS=--log /var/log/unit/unit.log --pid /run/unit.pid"
Environment="UNITD_OPTIONS=--log /var/log/unit/unit.log --pid /run/unit/unit.pid"

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

only /run on systemd distro

@@ -1 +1 @@
UNITD_OPTIONS="--log /var/log/unit/unit.log --pid /run/unit.pid"
UNITD_OPTIONS="--log /var/log/unit/unit.log --pid /var/run/unit/unit.pid"

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

but /var/run on non-systemd distro (where this file is used)

%dir %{_sysconfdir}/unit
%if %{use_systemd}
%{_unitdir}/unit.service
%dir %attr(0755,root,root) %ghost %{_localstatedir}/run/unit

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

ghosted directory is enough, will be created during service startup by systemd

%dir %{_sysconfdir}/unit
%if %{use_systemd}
%{_unitdir}/unit.service
%else
%config(noreplace) %{_sysconfdir}/sysconfig/unit

This comment has been minimized.

@defanator

defanator Jan 15, 2019

Contributor

@remicollet it seems like this one is mistakenly reverted here from
575d6d4 ?

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

Nop, simply move lower (to avoid multiple conditions)

@@ -98,7 +98,7 @@ endif
CONFIGURE_ARGS=\
--prefix=/usr \
--state=%{_sharedstatedir}/unit \
--control="unix:/var/run/control.unit.sock" \
--control="unix:/var/run/unit/control.sock" \

This comment has been minimized.

@remicollet

remicollet Jan 15, 2019

Author Contributor

This may raised issue when unit launch outside of systemd (e.g. in docker).
Reverted in next commit

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Jan 15, 2019

I think everything doen, so ready for review .

@@ -98,7 +98,7 @@ endif
CONFIGURE_ARGS=\
--prefix=/usr \
--state=%{_sharedstatedir}/unit \
--control="unix:/var/run/unit/control.sock" \
--control="unix:/var/run/control.unit.sock" \

This comment has been minimized.

@VBart

VBart Jan 15, 2019

Member

That may confuse users. Looking into unitd --help, they will see /var/run/control.unit.sock, while the actual path is overridden in systemd configuration. So, they will have to figure out the real path either via ps or checking unit.service file.

This comment has been minimized.

@remicollet

remicollet Jan 16, 2019

Author Contributor

ok, so let revert latest commit

@remicollet

This comment has been minimized.

Copy link
Contributor Author

remicollet commented Jan 16, 2019

Notice: latest commit fix a issue with non-systemd packages.

In pkg/rpm/rpmbuild/SOURCES/unit.init

prog="unitd"
[ -e /etc/sysconfig/$prog ] && . /etc/sysconfig/$prog

The currently provided file is ignored, and thus needed options

@remicollet remicollet changed the title systemd improvments from discussion in #213 systemd and packaging (RPM) improvments from discussion in #213 Jan 17, 2019

@defanator

This comment has been minimized.

Copy link
Contributor

defanator commented Jan 24, 2019

@remicollet committed in a single patch here: b78ed44, with a minor change to logrotate script (added check for pid file existence).

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment