balancer_by_lua for streams

[rshriram]

Hi @agentzh
here is the PR for balancer_by_lua based on @splitice's work (#7).

Changes on top of his commits:

• Fixed styling issues reported by ngx-style.pl

• Fixed bugs in the test suite (t/138-balancer.t that was ported from ngx_http_lua_module)

  The tests pertaining to balancer_by_lua are passing: https://travis-ci.org/rshriram/stream-lua-nginx-module/jobs/151088688
  However, an unrelated test suite (069-null.t) is failing due to missing dependency cjson.lua.
  If you want me to remove the .travis.yml file let me know. Somehow, GH seems to show that the .travis.yml is a new addition to your existing repo.

Note: I found that ngx.exit does not work inside the balancer_by_lua block. It throws an exception saying attempt to yield across C-call boundary. Given that this is a TCP stream block, ngx.exit does not really make sense, and so the above error seems like a valid one. I added a test to this effect. However, this behavior is not documented in the main README.

[splitice]

Good work :)

[agentzh]

@rshriram Thanks for your work on this. I'll have a look as soon as I can manage :)

[agentzh]

@rshriram I've just committed a quick fix for travis ci to the master branch. Please rebase to it. The recent system changes in travis ci exposes a bug in our existing .travis.yml files.

[agentzh]

@rshriram ngx.exit should still be supported in this context. It's useful to abort the whole request by running ngx.exit(ngx.ERROR) even in this stream module. The error you are seeing is due to the lack of changes in the ngx.exit function implementation for the balancer context. You can check out the http lua module for how to do this exactly.

[agentzh]

Style: extra blank lines should get removed.

[agentzh]

@rshriram Will you also create a separate PR to allow the ngx.balancer Lua module of lua-resty-core to work with this? Without the Lua API provided by ngx.balancer, the balancer_by_lua* directives are not really useful, I'm afraid :)

[agentzh]

The git master branch already has this file. Please rebase to it.

[rshriram]

you mean remove the entire .travis.yml file? I can certainly do it.. I just kept it around for my personal testing.

[rshriram]

@agentzh
Could you please give me some pointer on what exactly you mean by allowing ngx.balancer to work with this? Sorry, I whipped up this PR without going into much of the code base. So, I don't really understand what is needed here.

If you could provide me with a pointer or something, that would be very useful.

[agentzh]

@rshriram I mean the git master branch already has the .travis.yml file:

https://github.com/openresty/stream-lua-nginx-module/blob/master/.travis.yml

And the travis ci tests are all passing on the master branch. So your branch should too. And you should not add your own .travis.yml changes as seen in this diff.

[agentzh]

@rshriram I'm not suggesting removing the .travis.yml file. Instead I'm suggesting that the master branch already has it and you should not add your own version without good reasons.

[splitice]

@rshriram Theres another PR in resty core for the ngx.balancer side.

[agentzh]

@rshriram ngx.exit() should work in the http balancer_by_lua* context. You can check out its implementation for details. If it does not work in the http subsystem, please file a bug.

The existing ngx_http_lua test suite does not test cases utilizing ngx.exit in the context of balancer_by_lua*:

$ grep ngx.exit t/*-balancer.t
            ngx.exit(403)
            ngx.exit(ngx.OK)

[agentzh]

@rshriram Because you are currently working on the stream balancer set timeouts feature, it is your responsibility to add that upstream timeout field patch for the stream subsystem as part of your pull requests. Your change is the first user that makes use of it. But before that, it makes no sense for me to add that since I don't even have ways to actually test it.

[agentzh]

@rshriram If you look at the ngx_http_lua_ngx_exit C function, then you should see the relevant part dealing specifically for the balancer_by_lua* context:

    if (ctx->context & (NGX_HTTP_LUA_CONTEXT_HEADER_FILTER
                        | NGX_HTTP_LUA_CONTEXT_BALANCER))
    {
        return 0;
    }

This avoids yielding from within this function for that balancer by lua context.

[rshriram]

Thanks for the pointer. I added the exit support.

[rshriram]

Hi @agentzh
I have fixed a bunch of bugs and the code in lua-resty-core as well. Could you please take a look and let me know?

thanks

[agentzh]

@rshriram Thanks for your hard work on this! Will have another look as soon as I can manage. Thanks for your patience and understanding!

[rshriram]

Hi @agentzh
Any updates on this?

[agentzh]

@rshriram Sorry, been busy with my nginx.conf talk and some other stuff. Will have a look as soon as I can manage. Thanks for your hard work on this!

[doujiang24]

r should be s ? compiler error on my side.

[rshriram]

That's odd. How did the code even compile and pass Travis tests? Anyway good catch. Will fix it

[doujiang24]

@rshriram Any update?

[doujiang24]

@rshriram Thanks for your hard work, we'd better port more test case like ngx.var, lua_code_cache off and etc.

Regarding the balancer.set_timeouts, I have check the nginx stream code base, seems we have to patch for nginx (connect_timeout only in ngx_stream_proxy_srv_conf_t).
And it now only support proxy_connect_timeout and proxy_timeout (no proxy_read_timeout and proxy_send_tiemout like http system does).
May be we can leave it alone for now ? (may be the stream system may change in the future)

[rshriram]

I ported whatever I could from http balancer. A whole bunch of them had to be removed because there was no equivalent in stream subsystem. I don't remember seeing tests related to lua code cache. Will check again.

With regard to timeouts, I was also pointing to the fact that stream upstream server structures were different from http upstream structures. We could skip thst whole part now and slowly add that feature.

What is more pressing is the support for variables and stuff in the stream subsystem. It's annoying that only a few types of variables are supported in stream subsystem. But I guess that's a patch for the stream subsystem as a whole.

[doujiang24]

lua_code_cache off case: https://github.com/openresty/lua-nginx-module/blob/master/t/138-balancer.t#L308

Yes, only a few types of variables are supported, still deserved a test case. https://github.com/openresty/stream-lua-nginx-module#nginx-api-for-lua

[felipejfc]

Any updates here? I have an use case here where I need to dynamically add upstreams and balancer_by_lua would be very handy

[agentzh]

@felipejfc Agreed. Our OpenResty official team developer @doujiang24 will shortly look into this.

[misiek08]

Any news here?

I didn't saw this PR today and started to port balancer_by_* to stream module, but we don't have upstream functionalities, so we need to implement them or drop everything using upstream.

While working on my own code for PR I found this: https://github.com/tarantool/lua-stream-upstream-nginx-module. Maybe it will be much easier to include someting like this into OpenResty and port balancer_by_* to stream module using upstream functionality?

I can help with this if you are interested @agentzh.

[dndx]

No longer needed.