fix(cli) handle serf shutdown correctly

[bungle]

Summary

With LuaJIT Lua 5.2 compatibility the error code 256 is not returned from os.execute, but instead code % 256, and in this case 0.

This PR fixes the code handling, and usually serf:leave is all that needs to be done. This is also related to a patch to Penlight. Without this PR the Serf Agent pid file will not be deleted even if the Serf Agent is not running anymore when using Lua 5.2 compatibility.

Related Pull-Requests

Related Fix lunarmodules/Penlight/pull/244

[thibaultcha]

Sweet, looking forward to not having to disable Lua 5.2 compat anymore. Am I right to believe this PR only makes sense once lunarmodules/Penlight#244 is merged and released though?

[bungle]

It will work with and without Penlight patch as the actual kill is probably never needed, as Serf will shutdown the agent on leave. (but it obviously is not working without this patch with Lua > 5.1 — even with Penlight patch)

[thibaultcha]

It will work with and without Penlight patch as the actual kill is probably never needed, as Serf will shutdown the agent on leave.

Indeed. But shouldn't we hold on until re-enabling support for Lua 5.2 compat anyways before merging that, in case it does ever happen? We can mark this PR as ready but leave it open before it needs some other work (it's the goal of the pr/ready label).

[bungle]

I agree, this might not be needed if the serf replacement comes before this, but this would make probably it a bit easier for developers to start working with Kong development (as right now) as all the default OpenRestys come with 5.2 compatibility these days (also might be related to new packaging as well).

E.g. I work with macOS and I do brew install openresty. Then I grab the Kong sources, and well I start to hit these errors. Then I need to rebuild OpenResty from sources to overcome this. Maybe others have different issues. I just fixed this for myself so that I can use with or without Lua 5.2 compatibility. One less wall that I can hit when setting up a dev env.

[thibaultcha]

Sorry, I am not mentioning the removal of Serf, and I too wish to merge this patch prior to the Serf removal. But it seemed to me like this patch is not compatible with our current recommended --without-luajit-lua52 flag. If you confirm, then I believe we need to first take action to remove this flag (in the distributions scripts/packages + in the website against a next branch there), before merging this. Correct?

[bungle]

@thibaultcha, well, the thing here is that in my testing all we need to stop Serf is this:

local ok, err = serf:leave()

The only problem without this patch is that the .pid file stays there with 5.2 compatibility mode. And causes a lot of problems, e.g. Kong thinking that Serf is still running while it isn't.

I have never entered to this block because I guarded it with kill.is_running(kong_config.serf_pid):

local code = kill.kill(kong_config.serf_pid, "-15")

But I'm not sure if it was there for a reason. So the else block is there just to try kill Serf if the leave didn't do its job. Well, we are still trying just with SIGTERM, maybe we need to have SIGKILL as well?

The code you are afraid of is this:

if code ~= 0 then

Yes we could change it to:

if code ~= 0 and code ~= 256 then

The thing here is that no "if this is perfect" but it sucks badly to leave .pid files in case the process was shutdown. This is the only place where we actually check for 256.

E.g. Nginx stop uses this:

local code = kill.kill(kong_conf.nginx_pid, "-s "..signal)
if code ~= 0 then return nil, "could not send signal" end

But here it doesn't matter because we don't really care the code as Nginx deletes its own pid file (at least we don't do it).

Also for nginx we send TERM but for Serf we send -15 (inconsistent).

I think we should not worry about that code too much. The main a problem is make test* / pain in development with compatibility mode on (as the servers are frequently started and stopped).

[p0pr0ck5]

Just curious, do we care now that 0.11 is out-ish?

[bungle]

@p0pr0ck5, yes, I don't care this anymore unless we want to have this for 0.10. 0.11 with serf removal will get rid of this as well.

[bungle]

Btw. The Penlight was released that contains the patches I did send to them.

[Tieske]

so shouldn't this PR be against master then so it can land in 0.10.4?

Also: do we want to reenable the 52 compatibility? and if so, only for next (0.11.x), or also for master (0.10.4)?

[bungle]

Good questions. Definitely not in next anymore. I have no strong feelings about it.

[thibaultcha]

I don't think we need this anymore:

• 0.10.4 will still use the 0.10 family compilation flags and OpenResty 1.11.2.2
• 0.11.0 will use OpenResty 1.11.2.4 with Lua 5.2 compat enabled, but doesn't use Serf anymore

Mind closing this PR thus?