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

Rework sysconfig file/startup environment #6328

Merged
merged 3 commits into from Jun 5, 2018
Merged

Rework sysconfig file/startup environment #6328

merged 3 commits into from Jun 5, 2018

Conversation

Crunsher
Copy link
Contributor

The sysconifg file now only contains the defaults as comments, changes
made there are given to the init.d script, prepare-dirs, safe-reload and
Icinga 2 itself. If nothing is set in the sysconfig file (as is the
default as all lines are commented out) the defaults are used.

fixes #6255

@Crunsher
Copy link
Contributor Author

Crunsher commented May 23, 2018

TODO: Windows, as always

Testing:

  • Install and run on a init.d system
  • Validate changes in the sysconfig file are read and used (user and filelimit are easy to test)
  • Test running in foreground
  • Install and run on a systemd system
  • Validate changes in the sysconfig file are read and used (user and filelimit are easy to test)
  • Test running in foreground

@dnsmichi
Copy link
Contributor

getenv() also is available on Windows, needs proper tests though.

@Crunsher Crunsher self-assigned this May 23, 2018
@Crunsher Crunsher added this to the 2.9.0 milestone May 23, 2018
@Crunsher Crunsher added the area/setup Installation, systemd, sample files label May 23, 2018
Michael Friedrich and others added 3 commits May 23, 2018 15:18
Users kept asking about it, still it is just an "information"
that this isn't needed yet.
The sysconfig file now only contains the defaults as comments, changes
made there are given to the init.d script, prepare-dirs, safe-reload and
Icinga 2 itself. If nothing is set in the sysconfig file (as is the
default as all lines are commented out) the defaults are used.

fixes #6255
Logging breaks autocomplete, with this patch we should only log when
something is going terribly wrong and execution can't be guaranteed or
is destined to fail.
This commit depends on the previous one which is why it's not in its own
pull request.

fixes #6256
@Crunsher
Copy link
Contributor Author

While windows can use getenv, we never set the environment and the user we store at a different place. So this is a different issue.

@Crunsher Crunsher changed the title [WIP] Rework sysconfig file Rework sysconfig file/startup environment May 23, 2018
@dnsmichi
Copy link
Contributor

True that, still we should keep it in mind when it comes to changes related to environments (where getenv() comes in handy).

@dnsmichi
Copy link
Contributor

It seems that this PR cherry-picked a commit of mine and is not fast forward.

4a6320d

@N-o-X
Copy link
Contributor

N-o-X commented May 29, 2018

I couldn't get the expected result while testing this on Ubuntu 18.04.
It seems like Icinga isn't using the values.

Tested with:
ICINGA2_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/icinga51241322.log
And:
ICINGA2_ERROR_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/erro515125135r.log

Both logs were still written to icinga2.log and error.log

I've also tried removing the file to validate that it is in the correct location:
Sysconfig file '/etc/sysconfig/icinga2' cannot be read. Using default values.

@dnsmichi
Copy link
Contributor

ICINGA2_LOG=@CMAKE_INSTALL_FULL_LOCALSTATEDIR@/log/icinga2/icinga51241322.log

that's from a git source, how does your sysconfig file actually look like on the system you're testing on?

The wrong location for Debian-ized sysconfig files needs to be dealt with in a separate patch, hiding this warning. Maybe you're using the wrong branch to test, which commit are you on?

@N-o-X
Copy link
Contributor

N-o-X commented May 30, 2018

Whoops… didn't see that CMAKE variable there :D
I used the file from git source. The package didn't create it. Might be because the directory /etc/sysconfig doesn't exist on Debian derivatives, like you said.

And you're right I used https://build.icinga.com/job/icinga2-dev/job/deb-ubuntu-bionic-1binary/arch=x86_64/1/, because @Crunsher said that that build is based on branch sysconfig-env but is actually based on master. Sorry for that, will test it again.

@Crunsher
Copy link
Contributor Author

The dev package was built from the sysconfig-env. If the result is of the wrong branch, our dev pipeline is broken

@N-o-X
Copy link
Contributor

N-o-X commented May 30, 2018

@Crunsher you're right, it build from sysconfig-env and did what it should.
My sysconfig currently looks like this:

ICINGA2_PID_FILE=/var/log/icinga2/icinga218.pid
ICINGA2_ERROR_LOG=/var/log/icinga2/erro515125135r.log
ICINGA2_LOG=/var/log/icinga2/icinga51241322.log

But it still doesn't affect Icinga.

@dnsmichi
Copy link
Contributor

Keep in mind that ICINGA2_ERROR_LOG is taken into account by the Systemd/init script itself, which passes --errorlog as argument. We should discuss whether this is now obsoleted.

Same applies to the other settings above. Looking at the patch I can only see that USER/GROUP and RTSTACK limits are properly read and used.

@dnsmichi dnsmichi self-requested a review May 30, 2018 11:56
Copy link
Contributor

@dnsmichi dnsmichi left a comment

Choose a reason for hiding this comment

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

See my notes above, all environment variables should have an effect on Icinga 2, even without Systemd/init.d.

@dnsmichi dnsmichi self-assigned this Jun 5, 2018
@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 5, 2018

Review

As discussed offline, this issue is solely to fix USER, GROUP and RLIMIT settings which have been introduced with 2.8.2. No other focus with environment variables here.

Set environment variable

michi@mbmif ~/coding/icinga/icinga2 (sysconfig-env<>) $ sudo ICINGA2_USER=abc icinga2 daemon -C
critical/cli: Invalid user specified: abc
michi@mbmif ~/coding/icinga/icinga2 (sysconfig-env<>) $ sudo ICINGA2_USER=icinga icinga2 daemon -C
[2018-06-05 11:22:52 +0200] information/cli: Icinga application loader (version: v2.8.4-731-g2b5f95060; debug)
...
[2018-06-05 11:22:52 +0200] information/cli: Finished validating the configuration file(s).

RSTACK

Requires Linux, doesn't work on macOS. Updates soon from inside Vagrant.

@dnsmichi dnsmichi merged commit bf0737d into master Jun 5, 2018
@dnsmichi dnsmichi deleted the sysconfig-env branch June 5, 2018 11:27
dnsmichi pushed a commit that referenced this pull request Jun 5, 2018
@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 5, 2018

Manual Stacksize on Linux

[root@icinga2 ~]# ICINGA2_RLIMIT_STACK=16 icinga2 daemon -C
[2018-06-05 14:19:08 +0200] warning/Application: The user-specified value for RLimitStack cannot be smaller than the default value (262144). Using the default value instead.

Systemd

[root@icinga2 ~]# for p in $(pidof icinga2); do cat /proc/$p/limits | grep files; done
Max open files            16384                16384                files
Max open files            16384                16384                files
[root@icinga2 ~]# vim /etc/sysconfig/icinga2
...
ICINGA2_RLIMIT_FILES=50000

[root@icinga2 ~]# systemctl restart icinga2
[root@icinga2 ~]# for p in $(pidof icinga2); do cat /proc/$p/limits | grep files; done
Max open files            50000                50000                files
Max open files            50000                50000                files

@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 5, 2018

Initscripts pass the environment variables as expected, there's not much difference here involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/setup Installation, systemd, sample files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On debian based systems /etc/default/icinga2 is not read/used
3 participants