feature: restart and health #1366

subnetmarco · 2016-07-07T06:23:29Z

Adds the restart and health CLI commands.

The stop command accepts an optional --graceful parameter.

Tieske · 2016-07-07T08:43:07Z

shouldn't this be graceful by default, and have an option --force, as that's the more common case.

or possibly a timeout, after which it is forced.

subnetmarco · 2016-07-07T16:56:31Z

My first thought was to have both quit and stop, but maybe that would be overkill. Then I thought that if we only keep stop, it assumes an hard stop. When you stop something you want to TERM-inate it.

Of course it can work both ways. Either a stop that sends a QUIT signal by default (instead of TERM) or the other way around. Just to me stop == TERM, and stop --graceful == QUIT.

thibaultcha · 2016-07-07T17:58:38Z

feature/restart

thibaultcha · 2016-07-07T18:24:28Z

kong/cmd/health.lua

+local kill = require "kong.cmd.utils.kill"
+local pl_stringx = require "pl.stringx"
+local pl_path = require "pl.path"
+local pl_tablex = require "pl.tablex"


Please order those (I know it sounds silly but so far all new modules are ordered and as a result among many other efforts, the code is more readable).

See: https://github.com/Mashape/kong/blob/refactor/cli/kong/cmd/stop.lua#L1-L7

subnetmarco · 2016-07-07T18:28:31Z

Also @Tieske the graceful QUIT signal returns immediately and doesn't really terminate nginx right away. If you run kong stop --graceful and then you run ps aux | grep nginx, nginx may still be running because it's still processing existing requests.

I think that a stop command that by default does not stop the service right away is very misleading.

thibaultcha · 2016-07-07T18:43:49Z

kong/cmd/health.lua

+local function is_running(pid_path)
+  if not pl_path.exists(pid_path) then return nil end
+  local code = kill(pid_path, "-0")
+  return code == 0


No need to for the code local.

Tieske · 2016-07-07T19:40:14Z

@thefosk a forcefull stop should never be the default, quitting an app half way transactions, with open files, and connections etc. is a last resort, not a default.

Default should be gracefull and blocking, with a --timeout=xxx option (which could be 0). Result is either ok, or timeout error. The --force option would quit immediately.

thibaultcha · 2016-07-07T23:49:50Z

Just pushed a revised version of those with more tests.

thibaultcha · 2016-07-07T23:50:36Z

(Of health and restart. I agree to make quit graceful by default too)

subnetmarco · 2016-07-08T02:35:39Z

a forceful stop should never be the default

I disagree (nginx also disagrees, stop is a SIGTERM). With a graceful-by-default stop you have this behavior which is very wrong:

$ bin/kong stop
$ ps aux | grep nginx
marco           33552   0.0  0.0  2618256   6380   ??  S     7:28PM   0:00.02 nginx: worker process
marco           33551   0.0  0.0  2610704   5480   ??  S     7:28PM   0:00.01 nginx: worker process
marco           33550   0.0  0.0  2607528    544   ??  Ss    7:28PM   0:00.00 nginx: master process /usr/local/openresty/nginx/sbin/nginx -p /usr/local/kong -c nginx.conf

Which would mean that stop, doesn't really stop. It only stops later when the connections are done.

I am not even arguing that stop should be a SIGTERM, that it's the standard behavior of nginx itself (and of every service that I know of), my point another one and it was in the number of commands that we offer in the CLI: should we introduce kong quit (one more command to learn and document), or just use kong stop --graceful?

I am fine to have both kong stop (which correctly sends a SIGTERM) and kong quit (which properly sends a SIGQUIT), but absolutely I don't agree with a default kong stop to send a SIGQUIT.

subnetmarco · 2016-07-08T03:43:48Z

I have added kong quit. So now we have:

Graceful: kong quit and kong reload
Forceful: kong stop and kong restart

thibaultcha · 2016-07-08T06:47:59Z

spec/02-integration/01-cmd/07-cluster_spec.lua

+  it("force-leaves a node", function()
+    assert(helpers.kong_exec("start --conf "..helpers.test_conf_path))
+    local _, _, stdout = assert(helpers.kong_exec("cluster force-leave 127.0.0.1 --conf "..helpers.test_conf_path))
+    assert.matches("left node 127.0.0.1", stdout)


Should be

assert.matches("left node 127.0.0.1", stdout, nil, true)

Because arg #1 will be interpreted as a pattern otherwise, which means . can be any character. Better to have sane examples to not lead to errors in the future.

Tieske · 2016-07-08T07:13:51Z

Which would mean that stop, doesn't really stop. It only stops later when the connections are done.

true, that's why I said;

Default should be gracefull _and blocking, with a --timeout=xxx option (which could be 0)_.

personally I find having both stop and quit commands confusing.

As an example; git commandline, where ever things might get dangerous, defaults are safe and --force overrides. Also, I don't think nginx sets the standard on "intuitive" configuration...

Tieske · 2016-07-08T07:22:11Z

I have added kong quit. So now we have:

Graceful: kong quit and kong reload

Forceful: kong stop and kong restart

I think the graceful versions should still get a --timeout=xxx option. Such that one could script as;

kong quit --timeout=3
if error {
  kong stop
}

Commands expecting a running Kong should not accept a --conf argument

- abstract away `is_running()` in `kill.lua` util - add missing `kong health` to `kong --help` message - some linting inherited from the base branch of this branch

restart

f0f61b0

subnetmarco force-pushed the refactor/restart branch 2 times, most recently from 7be85a0 to 2c3710d Compare July 7, 2016 06:28

health

2c3710d

thibaultcha changed the title ~~restart and health~~ feature: restart and health Jul 7, 2016

thibaultcha reviewed Jul 7, 2016
View reviewed changes

refactor

6c2820c

thibaultcha force-pushed the refactor/restart branch from 6c2820c to 959c809 Compare July 7, 2016 23:49

subnetmarco and others added 2 commits July 7, 2016 16:50

feat(cli) kong health + restart commands

cef1baa

refactor(cli) better health/restart impl + tests

c55c981

thibaultcha force-pushed the refactor/restart branch from 959c809 to c55c981 Compare July 7, 2016 23:50

subnetmarco added 2 commits July 7, 2016 20:35

kong quit and tests

de03293

quit and tests

fcf7470

thibaultcha reviewed Jul 8, 2016
View reviewed changes

better assert.matches

3cd3ece

fix(cli) health command only accepts --prefix arg

80906ce

Commands expecting a running Kong should not accept a --conf argument

thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jul 11, 2016

subnetmarco mentioned this pull request Jul 11, 2016

feat(cli) kong quit (graceful shutdown) with --timeout #1377

Merged

thibaultcha and others added 7 commits July 11, 2016 17:59

feat(cli) implement 'kong quit' with --timeout

ccea6d3

- abstract away `is_running()` in `kill.lua` util - add missing `kong health` to `kong --help` message - some linting inherited from the base branch of this branch

restart

7217798

health

30bb41c

refactor

e8cb830

refactor(cli) better health/restart impl + tests

c566b76

kong quit and tests

6e00b81

better assert.matches

5d35ee2

thibaultcha force-pushed the refactor/restart branch from 3cd3ece to 5d35ee2 Compare July 12, 2016 01:15

Merge branch 'proper-stop' into refactor/restart

82f52d6

thibaultcha removed the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jul 12, 2016

thibaultcha merged commit e17c192 into refactor/cli Jul 12, 2016

thibaultcha deleted the refactor/restart branch July 12, 2016 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: restart and health #1366

feature: restart and health #1366

subnetmarco commented Jul 7, 2016 •

edited

Loading

Tieske commented Jul 7, 2016

subnetmarco commented Jul 7, 2016 •

edited

Loading

thibaultcha commented Jul 7, 2016

thibaultcha Jul 7, 2016

subnetmarco commented Jul 7, 2016 •

edited

Loading

thibaultcha Jul 7, 2016

Tieske commented Jul 7, 2016

thibaultcha commented Jul 7, 2016

thibaultcha commented Jul 7, 2016

subnetmarco commented Jul 8, 2016 •

edited

Loading

subnetmarco commented Jul 8, 2016

thibaultcha Jul 8, 2016

Tieske commented Jul 8, 2016

Tieske commented Jul 8, 2016

feature: restart and health #1366

feature: restart and health #1366

Conversation

subnetmarco commented Jul 7, 2016 • edited Loading

Tieske commented Jul 7, 2016

subnetmarco commented Jul 7, 2016 • edited Loading

thibaultcha commented Jul 7, 2016

thibaultcha Jul 7, 2016

Choose a reason for hiding this comment

subnetmarco commented Jul 7, 2016 • edited Loading

thibaultcha Jul 7, 2016

Choose a reason for hiding this comment

Tieske commented Jul 7, 2016

thibaultcha commented Jul 7, 2016

thibaultcha commented Jul 7, 2016

subnetmarco commented Jul 8, 2016 • edited Loading

subnetmarco commented Jul 8, 2016

thibaultcha Jul 8, 2016

Choose a reason for hiding this comment

Tieske commented Jul 8, 2016

Tieske commented Jul 8, 2016

subnetmarco commented Jul 7, 2016 •

edited

Loading

subnetmarco commented Jul 7, 2016 •

edited

Loading

subnetmarco commented Jul 7, 2016 •

edited

Loading

subnetmarco commented Jul 8, 2016 •

edited

Loading