Fixes #29507 - Add puma-status for reporting #7578

sthirugn · 2020-04-08T21:37:56Z

Note the foreman packaging change is still pending to include puma-status gem as rpm

theforeman-bot · 2020-04-08T21:38:07Z

sthirugn · 2020-04-08T21:41:22Z

this PR requires packaging of https://rubygems.org/gems/puma-status/versions/1.0 in https://github.com/theforeman/foreman-packaging

config/puma/production.rb

bundler.d/service.rb

config/puma/production.rb

script/foreman-puma-status

bundler.d/service.rb

config/puma/production.rb

sthirugn · 2020-04-09T19:01:12Z

I squashed all the commits, test and the output is:

# systemctl restart foreman

# journalctl -u foreman -f
Apr 09 18:57:39 centos7-katello-nightly.apex.example.com systemd[1]: Started Foreman.
Apr 09 18:57:42 centos7-katello-nightly.apex.example.com foreman[7802]: /usr/share/foreman/lib/foreman.rb:8: warning: already initialized constant Foreman::UUID_REGEXP
Apr 09 18:57:42 centos7-katello-nightly.apex.example.com foreman[7802]: /usr/share/foreman/lib/foreman.rb:8: warning: previous definition of UUID_REGEXP was here
Apr 09 18:57:43 centos7-katello-nightly.apex.example.com foreman[7802]: => Booting Puma
Apr 09 18:57:43 centos7-katello-nightly.apex.example.com foreman[7802]: => Rails 5.2.1 application starting in production
Apr 09 18:57:43 centos7-katello-nightly.apex.example.com foreman[7802]: => Run `rails server -h` for more startup options
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] Puma starting in cluster mode...
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Version 4.3.3 (ruby 2.5.5-p157), codename: Mysterious Traveller
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Min threads: 0, max threads: 16
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Environment: production
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Process workers: 2
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Phased restart available
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Activated tcp://127.0.0.1:3000
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] Use Ctrl-C to stop
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] * Starting control server on unix:///var/run/foreman/foreman-pumactl.sock
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] - Worker 0 (pid: 7842) booted, phase: 0
Apr 09 18:57:55 centos7-katello-nightly.apex.example.com foreman[7802]: [7802] - Worker 1 (pid: 7846) booted, phase: 0

# ll /var/run/foreman/pids
total 12
-rw-r--r--. 1 foreman foreman   5 Apr  9 18:57 puma.pid
-rw-r--r--. 1 foreman foreman 124 Apr  9 18:57 puma.state
-rw-r--r--. 1 foreman foreman   4 Apr  9 18:57 server.pid

# cat /var/run/foreman/pids/puma.state
---
pid: 7802
control_url: unix:///var/run/foreman/foreman-pumactl.sock
control_auth_token: f498f3f689f023ee6afe6e82699f36c

# cat /var/run/foreman/pids/puma.pid
7802

# /bin/bash /usr/share/foreman/script/foreman-puma-status 
7802 (/var/run/foreman/pids/puma.state) Uptime:  2m17s | Phase: 0 | Load: 0[░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░]32
 └  7842 CPU:   0.0% Mem:  294 MB Uptime:  2m17s | Load: 0[░░░░░░░░░░░░░░░░]16
 └  7846 CPU:   0.0% Mem:  294 MB Uptime:  2m17s | Load: 0[░░░░░░░░░░░░░░░░]16

script/foreman-puma-status

ekohl

The use of /var/run is generally deprecated since systemd introduced /run. It is also a symlink on all platforms we support now. In packaging we ensure this directory exists by creating /usr/lib/tmpfiles.d/foreman.conf.

On a related, note, we still symlink /usr/share/foreman/tmp to /var/run/foreman instead of /run/foreman. A brief look tells me we should be using RuntimeDirectory=foreman in foreman.service but then we also need to take a look at the cache directory. The more I looked into this, the more things I saw to improve. https://www.freedesktop.org/software/systemd/man/systemd.exec.html#RuntimeDirectory= has a nice list but Rails doesn't have a variable to change the tmp directory and just uses Rails.root.join('tmp') all over the place. That's why packaging symlinks it.

I also see that /run/foreman/sockets exists, but is empty. It appears this is a Rails convention and we should be making use of that.

You're now only configuring this for production, but shouldn't this be a general config? development also uses puma and it'd be nice to test the same tools there.

ekohl · 2020-04-14T16:44:45Z

config/puma/production.rb

+pid_dir = '/var/run/foreman/pids'
+
+# Store the pid of the server.
+pidfile "#{pid_dir}/puma.pid"
+
+# Store server info state.
+state_path "#{pid_dir}/puma.state"


Suggested change

pid_dir = '/var/run/foreman/pids'

# Store the pid of the server.

pidfile "#{pid_dir}/puma.pid"

# Store server info state.

state_path "#{pid_dir}/puma.state"

run_dir = Rails.root.join('tmp')

# Store the pid of the server.

pidfile File.join(run_dir, 'pids', 'puma.pid')

# Store server info state.

state_path File.join(run_dir, 'puma.state')

thanks @ekohl , to make it even simpler, should we perhaps just use
run_dir = "/run/foreman"

config/puma/production.rb

ekohl · 2020-04-14T16:45:25Z

script/foreman-puma-status

@@ -0,0 +1,2 @@
+#!/bin/bash
+puma-status /var/run/foreman/pids/puma.state


Suggested change

puma-status /var/run/foreman/pids/puma.state

puma-status /run/foreman/puma.state

ekohl · 2020-04-14T17:51:02Z

config/puma/production.rb

+pid_dir = '/var/run/foreman/pids'
+
+# Store the pid of the server.
+pidfile "#{pid_dir}/puma.pid"


Reading further about this, the default pid is tmp/pids/server.pid and I don't see the need to change this. I also think that rails server has a --pid option to change this and there's no real reason to hardcode it. In systemd we don't even care about pids at all since it's all managed for us. Am I missing something?

you are right, we are concerned about the state_path option only and there is no need to change pidfile option. I changed it to make sure the pid and state files live together, but it doesn't matter. I will remove this entry.

sthirugn · 2020-04-15T16:48:30Z

You're now only configuring this for production, but shouldn't this be a general config? development also uses puma and it'd be nice to test the same tools there.

I can submit a change for dev environment, which repository should this change be submitted to?

config/puma/production.rb

script/foreman-puma-status

ekohl · 2020-04-16T14:12:41Z

Are there packaging PRs (both deb and RPM) to make sure foreman-puma-status is installed to the correct location?

sthirugn · 2020-04-16T14:29:15Z

Are there packaging PRs (both deb and RPM) to make sure foreman-puma-status is installed to the correct location?

I see this PR theforeman/foreman-packaging#5031 manipulates foreman-puma-status. What other changes are needed?

ehelms · 2020-04-20T15:47:25Z

Packaging change for RPM:

theforeman/foreman-packaging#5031

ehelms · 2020-04-21T19:43:25Z

Any further comments or concerns with this? With packaging PRs open, I believe this is GTG.

ekohl

Approving if my assumption is correct.

ekohl · 2020-04-22T13:49:34Z

config/puma/production.rb

@@ -17,3 +22,6 @@
  dynflow = ::Rails.application.dynflow
  dynflow.initialize! unless dynflow.config.lazy_initialization
 end
+
+# === Puma control rack application ===
+activate_control_app "unix://#{run_dir}/sockets/pumactl.sock"


I haven't checked the permissions on this, but I'm assuming it's foreman:foreman and 0644 which should prevent from everyone writing to it. Correct?

This is how it looks now:

# ll -d /run/foreman/ /run/foreman/sockets drwxr-xr-x. 5 foreman foreman 200 Apr 22 14:30 /run/foreman/ drwxr-xr-x. 2 foreman foreman 60 Apr 22 14:23 /run/foreman/sockets # ll /run/foreman/sockets total 0 srwxrwxrwx. 1 foreman foreman 0 Apr 22 14:23 pumactl.sock

Does that mean that any user can connect to the socket and restart the server?

Are these determined by packaging?

yes, looks like a postgres user is able to restart the puma server if right control_url and control_auth_token is known. Any suggestions on how to lock this down further?

# ll /var/run/foreman/puma.state -rw-r--r--. 1 foreman foreman 120 Apr 22 15:23 /var/run/foreman/puma.state # cat /var/run/foreman/puma.state --- pid: 1672 control_url: unix:///run/foreman/sockets/pumactl.sock control_auth_token: 6c445af0312fff70fe0f190a65644cf # sudo -u postgres scl enable tfm 'pumactl --control-url 'unix:///var/run/foreman/sockets/pumactl.sock' --control-token 6c445af0312fff70fe0f190a65644cf restart' Command restart sent success

What are the permissions on viewing the control token?

see the first line of my post above. the control token is present in puma.state file which has permissions 644

All I can think is having something to adjust the permissions after creation but really the answer is to get a change into Puma to allow configuring the state file permissions.

Is it possible to put the state file in a directory that is not globally readable?

All I can think is having something to adjust the permissions after creation but really the answer is to get a change into Puma to allow configuring the state file permissions.

I am working to get a fix to the puma project.

Submitted patch to puma project puma/puma#2238 to address the state file security issue.

tbrisker · 2020-05-03T09:54:58Z

@sthirugn @ekohl What's the status here?

ekohl · 2020-05-03T13:02:49Z

Blocked on the linked PR (puma/puma#2238). There's good progress on it.

sthirugn · 2020-05-07T04:23:11Z

Note that puma/puma#2238 is merged now. So I added a new commit to this PR to add state_permission of 0644

ekohl · 2020-05-07T09:08:45Z

Note that puma/puma#2238 is merged now. So I added a new commit to this PR to add state_permission of 0644

Note that merged doesn't mean available. You'll need to wait for a gem to include it and require at least puma x.y.z.

ehelms · 2020-05-28T18:33:01Z

The related Puma fix is merged and released in Puma.

ekohl

This should now require the right puma version but other than that 👍

tbrisker · 2020-06-17T08:06:49Z

closing in favor of gh-7735

theforeman-bot added Needs testing Not yet reviewed labels Apr 8, 2020

sthirugn commented Apr 9, 2020

View reviewed changes

config/puma/production.rb Outdated Show resolved Hide resolved

sthirugn commented Apr 9, 2020

View reviewed changes

bundler.d/service.rb Outdated Show resolved Hide resolved

ekohl reviewed Apr 9, 2020

View reviewed changes

config/puma/production.rb Outdated Show resolved Hide resolved

script/foreman-puma-status Outdated Show resolved Hide resolved

bundler.d/service.rb Outdated Show resolved Hide resolved

theforeman-bot added the Waiting on contributor label Apr 9, 2020

houndci-bot reviewed Apr 9, 2020

View reviewed changes

config/puma/production.rb Outdated Show resolved Hide resolved

config/puma/production.rb Outdated Show resolved Hide resolved

config/puma/production.rb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Apr 9, 2020

sthirugn commented Apr 9, 2020

View reviewed changes

config/puma/production.rb Outdated Show resolved Hide resolved

sthirugn force-pushed the puma_status branch from 133ff18 to d25a01a Compare April 9, 2020 19:03

theforeman-bot removed the Waiting on contributor label Apr 9, 2020

ehelms reviewed Apr 13, 2020

View reviewed changes

script/foreman-puma-status Outdated Show resolved Hide resolved

sthirugn force-pushed the puma_status branch from d25a01a to 7f7fc8f Compare April 13, 2020 21:13

ehelms requested review from ekohl and timogoebel April 14, 2020 13:56

ekohl reviewed Apr 14, 2020

View reviewed changes

theforeman-bot added the Waiting on contributor label Apr 15, 2020

ekohl reviewed Apr 15, 2020

View reviewed changes

config/puma/production.rb Outdated Show resolved Hide resolved

ehelms requested a review from ekohl April 16, 2020 14:03

timogoebel reviewed Apr 16, 2020

View reviewed changes

script/foreman-puma-status Show resolved Hide resolved

Fixes #29507 - Add puma-status for reporting

8eda3e4

sthirugn dismissed ehelms’s stale review via 8eda3e4 April 16, 2020 14:58

sthirugn force-pushed the puma_status branch from ecf8836 to 8eda3e4 Compare April 16, 2020 14:58

sthirugn requested a review from ehelms April 17, 2020 19:34

ehelms previously approved these changes Apr 22, 2020

View reviewed changes

ekohl previously approved these changes Apr 22, 2020

View reviewed changes

ekohl mentioned this pull request Apr 27, 2020

Add optional state file permissions puma/puma#2238

Merged

8 tasks

tbrisker added the Blocked by another PR label May 6, 2020

Add permission configuration for state file

5e210d5

sthirugn dismissed stale reviews from ekohl and ehelms via 5e210d5 May 7, 2020 04:21

theforeman-bot added the Waiting on contributor label May 7, 2020

ehelms mentioned this pull request May 28, 2020

Update puma to 4.3.5 theforeman/foreman-packaging#5317

Merged

ehelms removed the Blocked by another PR label May 28, 2020

ekohl requested changes May 28, 2020

View reviewed changes

ehelms mentioned this pull request Jun 9, 2020

Fixes #29507 - Add puma-status for reporting #7735

Merged

tbrisker closed this Jun 17, 2020

evgeni mentioned this pull request Apr 27, 2021

Deploy foreman-puma-status script when installing foreman-service theforeman/foreman-packaging#6322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #29507 - Add puma-status for reporting #7578

Fixes #29507 - Add puma-status for reporting #7578

sthirugn commented Apr 8, 2020 •

edited

Loading

theforeman-bot commented Apr 8, 2020

sthirugn commented Apr 8, 2020

sthirugn commented Apr 9, 2020 •

edited

Loading

ekohl left a comment

ekohl Apr 14, 2020

sthirugn Apr 15, 2020

ekohl Apr 14, 2020

ekohl Apr 14, 2020

sthirugn Apr 15, 2020

sthirugn commented Apr 15, 2020

ekohl commented Apr 16, 2020

sthirugn commented Apr 16, 2020

ehelms commented Apr 20, 2020

ehelms commented Apr 21, 2020

ekohl left a comment

ekohl Apr 22, 2020

sthirugn Apr 22, 2020 •

edited

Loading

ekohl Apr 22, 2020

ehelms Apr 22, 2020

sthirugn Apr 22, 2020

ehelms Apr 22, 2020

sthirugn Apr 22, 2020 •

edited

Loading

ehelms Apr 22, 2020 •

edited

Loading

sthirugn Apr 23, 2020

sthirugn Apr 27, 2020

tbrisker commented May 3, 2020

ekohl commented May 3, 2020

sthirugn commented May 7, 2020

ekohl commented May 7, 2020

ehelms commented May 28, 2020

ekohl left a comment

tbrisker commented Jun 17, 2020

		@@ -0,0 +1,2 @@
		#!/bin/bash
		puma-status /var/run/foreman/pids/puma.state

	puma-status /var/run/foreman/pids/puma.state
	puma-status /run/foreman/puma.state

Fixes #29507 - Add puma-status for reporting #7578

Fixes #29507 - Add puma-status for reporting #7578

Conversation

sthirugn commented Apr 8, 2020 • edited Loading

theforeman-bot commented Apr 8, 2020

sthirugn commented Apr 8, 2020

sthirugn commented Apr 9, 2020 • edited Loading

ekohl 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

sthirugn commented Apr 15, 2020

ekohl commented Apr 16, 2020

sthirugn commented Apr 16, 2020

ehelms commented Apr 20, 2020

ehelms commented Apr 21, 2020

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthirugn Apr 22, 2020 • edited Loading

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

sthirugn Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

ehelms Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker commented May 3, 2020

ekohl commented May 3, 2020

sthirugn commented May 7, 2020

ekohl commented May 7, 2020

ehelms commented May 28, 2020

ekohl left a comment

Choose a reason for hiding this comment

tbrisker commented Jun 17, 2020

sthirugn commented Apr 8, 2020 •

edited

Loading

sthirugn commented Apr 9, 2020 •

edited

Loading

sthirugn Apr 22, 2020 •

edited

Loading

sthirugn Apr 22, 2020 •

edited

Loading

ehelms Apr 22, 2020 •

edited

Loading