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.

Sign up for GitHub

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

Jump to bottom

munin: add a check for existing PID files #1363

Merged

kou merged 25 commits into master from detect-pid-file

May 30, 2022

Member

komainu8 commented May 27, 2022 •

edited

Loading

Because protocols that have not exist PID files are invalid.

The result of execution is the following.

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | http                                   
groonga_cpu_time_          | yes  | http                                   
groonga_disk_              | yes  | http                                   
groonga_memory_            | yes  | http                                   
groonga_n_records_         | yes  | http                                   
groonga_query_performance_ | yes  | http                                   
groonga_status_            | yes  | http                                   
groonga_throughput_        | yes  | http

kou reviewed

View reviewed changes

data/munin/groonga_n_records_ Outdated

Comment on lines 193 to 197

+                pid_file_exist = false
+                pid_paths.each do |pid_path|
+                  next unless File.exist?(pid_path)
+                  pid_file_exist = true
+                end

Member

kou May 27, 2022

We can return false here when no PID file exist.

Member

kou May 27, 2022

We need similar check in suggest too. If http_pid_path doesn't exist, suggest doesn't print http. Without similar check, suggest will print http when http://127.0.0.1/d/... is accesible by groonga-httpd not groonga --protocol http.

Member Author

komainu8 May 27, 2022

Thank you for your coments.
I've fixed them at aecf24d.

komainu8 marked this pull request as ready for review

May 29, 2022 14:54

kou reviewed

View reviewed changes

data/munin/groonga_disk_ Outdated Show resolved Hide resolved

data/munin/groonga_n_records_ Outdated

@@ @@ -146,18 +146,21 @@ end @@
               def setup_http
                 @http_host = ENV["http_host"] || @default_host
                 @http_port = (ENV["http_port"] || @default_http_port).to_i
+                @http_pid_path = File.exist?(ENV["http_pid_path"]) || nil

Member

kou May 30, 2022

File.exist?(ENV["http_pid_path"]) isn't a PID path. How about @http_running = File.exist?(ENV["http_pid_path"]) or something?

BTW, can we use /run/groonga/groonga-http.pid as the default PID path?

Member Author

komainu8 May 30, 2022

What does BTW, can we use /run/groonga/groonga-http.pid as the default PID path? mean?
Sorry, I don't understand this.

Member

kou May 30, 2022

http_hostやhttp_portみたいにデフォルト値を設定できませんか？
この変更が入ると必ずPIDパスを設定しないと動きませんが、適切なデフォルト値があれば以前のように設定なしでも（デフォルトの設定のままgroonga --protocol http -sを使っていれば）動く気がします。

data/munin/groonga_n_records_ Outdated

@@ @@ -169,7 +172,39 @@ def setup @@
                 setup_gqtp
               end
+              def pid_files_esists?

Member

kou May 30, 2022

We should use exist? not exists? like File.exist?. (File.exists? is deprecated.)

Member Author

komainu8 May 30, 2022

Oh.. I see.

data/munin/groonga_n_records_ Outdated

Comment on lines 177 to 178

		"pid_path",
		"pid_file", # For backward compatibility. Remove me when 5.0.0.

Member

kou May 30, 2022

If we support them, setup_XXX should also support them.

data/munin/groonga_n_records_ Outdated

Comment on lines 206 to 207

		exit(false) unless pid_files_esists?

Member

kou May 30, 2022

Do we need this?

Can we check @http_running instead?

Member Author

komainu8 May 30, 2022

No, this is needless.
I've deleted this.

data/munin/groonga_n_records_ Outdated

Comment on lines 240 to 241

		exit(false) unless pid_files_esists?

Member

kou May 30, 2022

Do we need this?

Member Author

komainu8 May 30, 2022

No, this is needless.
I've deleted this.

Member Author

komainu8 commented May 30, 2022

Thank you for your comment. I've fixed them.

kou reviewed

View reviewed changes

data/munin/groonga_disk_ Outdated Show resolved Hide resolved

data/munin/groonga_n_records_ Outdated Show resolved Hide resolved

data/munin/groonga_query_performance_ Outdated

		@@ -10,7 +10,39 @@ label = ENV["label"]

		command = ARGV.shift

		def pid_files_esists?

Member

kou May 30, 2022

We need to update this file too.

Member Author

komainu8 commented May 30, 2022

Thank you for your comment. I've fixed them.

kou reviewed

View reviewed changes

data/munin/groonga_query_performance_ Outdated

		def autoconf
		exit(false) unless (@http_running or @httpd_running or @gqtp_running)

Member

kou May 30, 2022

Do we need this?

Member Author

komainu8 May 30, 2022

Ah... This is needless.
I've removed it.

data/munin/groonga_query_performance_ Outdated

@@ @@ -46,13 +56,13 @@ def suggest @@
                   query_log_path = ENV[variable_name]
                   query_log_path and File.exist?(query_log_path)
                 end
-                if exist_p.call("http_query_log_path")
+                if exist_p.call("http_query_log_path") and @http_running

Member

kou May 30, 2022

How about using the same implementation in data/munin/groonga_disk_?

Member Author

komainu8 commented May 30, 2022

Thank you for your comments.
I've fixed them.

kou reviewed

View reviewed changes

data/munin/groonga_query_performance_ Outdated

Comment on lines 17 to 20

+              @http_running = File.exist?(ENV["http_pid_path"] || @default_http_pid_path)
+              @httpd_running = File.exist?(ENV["httpd_pid_path"] || @default_httpd_pid_path)
+              @gqtp_running = File.exist?(ENV["gqtp_pid_path"] || @default_gqtp_pid_path)

Member

kou May 30, 2022

Can we remove them?

Member Author

komainu8 May 30, 2022

I've removed them.

data/munin/groonga_query_performance_ Outdated

Comment on lines 13 to 15

+              @default_http_pid_path = "/run/groonga/groonga-http.pid"
+              @default_httpd_pid_path = "/run/groonga/groonga-httpd.pid"
+              @default_gqtp_pid_path = "/run/groonga/groonga-gptp.pid"

Member

kou May 30, 2022

Can we use them?

Member Author

komainu8 commented May 30, 2022

Thank you for your comments.
I've fixed them.

kou reviewed

View reviewed changes

data/munin/groonga_disk_ Outdated

-                  database_path = ENV[variable_name]
-                  database_path and File.exist?(database_path)
+                exist_p = lambda do |database_path_name, pid_file_path_name|
+                  File.exist?(ENV[database_path_name]) and File.exist?(ENV[pid_file_path_name])

Member

kou May 30, 2022

Ah, this will raise an error when it's not specified:

$ ruby -e 'File.exist?(nil)'
-e:1:in `exist?': no implicit conversion of nil into String (TypeError)
	from -e:1:in `<main>'

kou reviewed

View reviewed changes

data/munin/groonga_disk_ Outdated

Comment on lines 160 to 167

+                exist_p = lambda do |database_path_name, pid_file_path_name|
+                  database_path = ENV[database_path_name]
+                  pid_file_path = ENV[pid_file_path_name]
+                  (database_path and File.exist?(ENV[database_path_name])) and (pid_file_path and File.exist?(ENV[pid_file_path_name]))
                 end

Member

kou May 30, 2022

How about creating a helper lambda?

env_path_exist_p = lambda do |variable_name|
  path = ENV[variable_name]
  path and File.exist?(path)
end
exist_p = lambda do |database_path_name, pid_file_path_name|
  env_path_exist_p.call(database_path_name) and
    env_path_exist_p.call(pid_file_path_name)
end

kou approved these changes

View reviewed changes

Member

kou left a comment

We can merge this once we confirm all patterns by hand. (We don't have tests for this yet.)

komainu8 and others added 17 commits

May 30, 2022 17:08


          munin: add a check for existing PID files

dd50bc4

Because protocols that have not exist PID files are invalid.


          munin: add a check for existing PID files in "suggest"

06bb174


          munin: remove a needless expression

de82e9b


          munin: remove needless expressions

0f1ef62


          munin: add an expression of detection of PID file

275cc74


          munin: add a expression for detecting the PID file

9c4a671


          munin: remove needless "elsif"

da568c0


          munin: add a expression for detecting PID file

534f71c


          munin: fix wrong argument names

0ed9439

Because these block variables are not the path itself. They are variable names.

Co-authored-by: Sutou Kouhei <kou@clear-code.com>


          munin: fix wrong variable names

ff76e9b

Because "File.exist?(ENV["http_pid_path"])" isn't a PID path.


          munin: fix wrong variable names

5860a67

Because "File.exist?(ENV["http_pid_path"])" isn't a PID path.


          munin: remove needless method

ea11159

We can check @http_running and @httpd_running and, @gqtp_running instead.


          munin: set default values for PID path

00dde13

Because if default values for the PID path don't exist, these plugins
don't work.
If default values for the PID path exist, these plugins work without
setting the PID path.


          munin: fix wrong variable names

c7ead65

Co-authored-by: Sutou Kouhei <kou@clear-code.com>


          munin: fix a wrong condition

96fe8ef

Co-authored-by: Sutou Kouhei <kou@clear-code.com>


          munin: fix a wrong condition

08634c8


          munin: remove needless method

7d61dbc

We can check @http_running and @httpd_running and, @gqtp_running instead.

komainu8 added 8 commits

May 30, 2022 17:08


          munin: fix a typo

3f8f227


          munin: fix a wrong condition

e0008c5


          munin: remove a needless condition

0cb220a

Because the confirmation the query log path is valid or not would be
sufficient.


          munin: use the same implementation in groonga_disk_

ecfafca


          munin: remove useless variables

1c4408f


          munin: add a nil check

ff3a0b6


          munin: add a helper lambda

da864ac


          munin: fix a typo

023593e

komainu8 force-pushed the detect-pid-file branch from db18b1f to 023593e Compare

May 30, 2022 08:08

Member Author

komainu8 commented May 30, 2022

Use groonga-server-http

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | http                                   
groonga_cpu_time_          | yes  | http                                   
groonga_disk_              | yes  | http                                   
groonga_memory_            | yes  | http                                   
groonga_n_records_         | yes  | http                                   
groonga_query_performance_ | yes  | http                                   
groonga_status_            | yes  | http                                   
groonga_throughput_        | yes  | http

Member Author

komainu8 commented May 30, 2022

Use groonga-httpd

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | httpd                                  
groonga_cpu_time_          | yes  | httpd                                  
groonga_disk_              | yes  | httpd                                  
groonga_memory_            | yes  | httpd                                  
groonga_n_records_         | yes  | httpd                                  
groonga_query_performance_ | yes  | httpd                                  
groonga_status_            | yes  | httpd                                  
groonga_throughput_        | yes  | httpd

Member Author

komainu8 commented May 30, 2022

Use gqtp

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | gqtp                                   
groonga_cpu_time_          | yes  | gqtp                                   
groonga_disk_              | yes  | gqtp                                   
groonga_memory_            | yes  | gqtp                                   
groonga_n_records_         | yes  | gqtp                                   
groonga_query_performance_ | yes  | gqtp                                   
groonga_status_            | yes  | gqtp                                   
groonga_throughput_        | yes  | gqtp

Member Author

komainu8 commented May 30, 2022

Use groonga-httpd and groonga-gqtp

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | gqtp httpd                             
groonga_cpu_time_          | yes  | gqtp httpd                             
groonga_disk_              | yes  | gqtp httpd                             
groonga_memory_            | yes  | gqtp httpd                             
groonga_n_records_         | yes  | gqtp httpd                             
groonga_query_performance_ | yes  | gqtp httpd                             
groonga_status_            | yes  | gqtp httpd                             
groonga_throughput_        | yes  | gqtp httpd

Use groonga-server-http and groonga-gqtp

$ sudo munin-node-configure |grep groonga
groonga_cpu_load_          | yes  | gqtp http                              
groonga_cpu_time_          | yes  | gqtp http                              
groonga_disk_              | yes  | gqtp http                              
groonga_memory_            | yes  | gqtp http                              
groonga_n_records_         | yes  | gqtp http                              
groonga_query_performance_ | yes  | gqtp http                              
groonga_status_            | yes  | gqtp http                              
groonga_throughput_        | yes  | gqtp http

Member Author

komainu8 commented May 30, 2022

We have confirmed all patterns by hand.

kou merged commit 2182721 into master

kou deleted the detect-pid-file branch

May 30, 2022 08:57

github-actions bot mentioned this pull request

nginx: update bundles version 1.23.4 komainu8/groonga#1

Closed

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