http: allow all forks to process HTTP requests

This MR allows the http module to run on all forks.

added 1 commit

• 57944f9f - modules/http: allow passing server options to http configuration

Compare with previous version

added 10 commits

• 57944f9f...9bdfbdd8 - 9 commits from branch master
• 1f9d5826 - modules/http: allow passing server options to http configuration

Compare with previous version

added feature label

resolved all discussions

Hmm, I've played a little bit with the 1f9d5826 and found a problem related to multiple forks.

The old version executed using -f 4 shows 4 instances as expected:

New version shows only one instance:

WTF?

changed title from modules/http: allow passing server options to http configuration to WIP: modules/http: allow passing server options to http configuration

The old version allowed only one fork to serve HTTP (the one with the worker.id = 0). Now any fork can serve HTTP (because of reuseport), which is good for supporting things like DNS over HTTPS etc., but I think the command fan-out doesn't work if it's running on on non-primary fork. Let me fix that.

added 1 commit

• e6988214 - wip

Compare with previous version

So there are two problems before all forks can fully serve HTTP:

• Only the first fork can send commands to the other forks, which means that if any other fork gets the HTTP request, it won't "see" the rest of the forks
• Handling of the remote commands is synchronous, so if two forks send each other commands, they will deadlock

I will try to rewrite most of the the remote commands functionality to Lua so that it's possible to make it non-blocking with cqueues (just like the HTTP module).

Hmm, this is a problem because cqueues are not packaged in Fedora/EPEL and other distros. Do you see a way to keep it optional dependency?

It doesn't change dependencies much, as map() is currently used from the http module (that already requires cqueues). It would be nice to package cqueues anyway (Debian stable has it) to get full features out of resolver.

@vavrusam: the point is that the http module can be easily omitted in case some distro/developer/... doesn't want to bother packaging/obtaining cqueues.

In particular, "we" would have to package it for Turris Omnia as well ;-)

added 19 commits

• e6988214...b18846d9 - 5 commits from branch master
• bb2f1c46 - daemon: added callback for resolve() query initialisation
• 00cb5245 - daemon: fixed memory leaking from wrk_resolve on some input errors
• b88714db - lib/resolve: add support for per-request logging
• 80abd563 - lib/rplan: remember request context in each query
• 937ec9a6 - lib: added support for trace_log for verbose messages
• 17d9bd86 - modules/http: added /trace endpoint for request log tracing, added tests
• 3f56c970 - modules/http: updated format of the /trace log
• 6f750103 - clean up on module test success, fix priming query failing predict test
• 6cc0b704 - ci: added lua-http dependency to build, fixed test for http
• 98f3284a - main: close loop after it's finished to please valgrind
• 07ef83cb - kres: added support for NULL type
• d66d1563 - modules/http: added selected records to /trace endpoint
• 3255c2a0 - modules/http: allow passing server options to http configuration
• e06ad114 - Implement worker coroutines for asynchronous background processing

Compare with previous version

changed target branch from master to query-trace

changed title from WIP: modules/http: allow passing server options to http configuration to WIP: Implement worker coroutines for asynchronous background processing, all forks able to process HTTP requests

changed the description

I tried to make cqueues as optional as possible, but it doesn't make sense to me to make IPC work correctly without it (because it's inherently synchronous, so two forks talking to each other would deadlock). I tried to provide some reasoning in the commit message and MR header, so please let me know if that's acceptable. The daemon will still work without the library, but it's not going to be as nice.

added 1 commit

• 65fc7f56 - Implement worker coroutines for asynchronous background processing

Compare with previous version

Added tests.

added 1 commit

• b5c2df3e - Implement worker coroutines for asynchronous background processing

Compare with previous version

added 1 commit

• d78095d5 - daemon/worker: allow large responses for outbound over TCP

Compare with previous version

added 45 commits

• d78095d5...c75b1176 - 42 commits from branch query-trace
• b91ff55a - modules/http: allow passing server options to http configuration
• d2c1aa04 - Implement worker coroutines for asynchronous background processing
• 55fc6c7e - daemon/worker: allow large responses for outbound over TCP

Compare with previous version

added 4 commits

• a3d38b58 - 1 commit from branch query-trace
• 231dfab8 - modules/http: allow passing server options to http configuration
• b9329e86 - Implement worker coroutines for asynchronous background processing
• b8a480ae - daemon/worker: allow large responses for outbound over TCP

Compare with previous version

added 4 commits

• c34ad084 - added knot_dname_t * to Lua string wireformat conversion function
• 0146a88f - daemon: unified query completion callback with trace callback in resolve
• 13debc30 - lib: added kr_rplan_last() function to get last processed query
• c9f5878a - tests/config: all tests can now be asynchronous with worker.coroutine

Compare with previous version

added 3 commits

• 2066de68 - daemon: unified query completion callback with trace callback in resolve
• f455bbce - lib: added kr_rplan_last() function to get last processed query
• 49087d4a - tests/config: all tests can now be asynchronous with worker.coroutine

Compare with previous version

mentioned in merge request !405 (merged)

I don't think there's anything left in this MR after query-trace merge, right?

added 446 commits

• 49087d4a...fa4ea355 - 446 commits from branch query-trace

Compare with previous version

unmarked as a Work In Progress

changed title from WIP: Implement worker coroutines for asynchronous background processing, all forks able to process HTTP requests to http: allow all forks to process HTTP requests

changed the description

changed target branch from query-trace to master

@vcunat there's one commit left to allow http module to run on all forks, I've rebased the branch and updated the MR to reflect that.

assigned to @gdemidov

@vavrusam Could you rebase please this branch over master? I will merge it then.

added 232 commits

• fa4ea355...bba85538 - 231 commits from branch master
• 165e1411 - modules/http: allow passing server options to http configuration

Compare with previous version

Rebased

Hmm, the rebase broke tests:

ok 1 - creates server instance
ok 2 - binds to an interface
not ok 3 - static page return 200 OK
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 36
not ok 4 - static page has non-empty body
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 37
not ok 5 - static page has text/html content type
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 38
ok 6 - custom page return 200 OK
ok 7 - custom page has non-empty body
ok 8 - custom page has custom content type
ok 9 - non-existent page returns 404
not ok 10 - /stats page return 200 OK
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 49
not ok 11 - /stats page has non-empty body
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 50
not ok 12 - /stats page has correct content type
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 51
not ok 13 - /metrics page return 200 OK
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 54
not ok 14 - /metrics page has non-empty body
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 55
not ok 15 - /metrics page has correct content type
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 56
not ok 16 - /trace page return 200 OK
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 59
not ok 17 - /trace page has non-empty body
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 60
not ok 18 - /trace page has correct content type
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 61
not ok 19 - /trace checks type
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 64
not ok 20 - /trace requires name
# Trouble in /builds/knot/knot-resolver/modules/http/http.test.lua around line 66
1..20
Expected return code '0' got '14'.
tests/config/test_config.mk:20: recipe for target 'modules/http/http.test.lua' failed

assigned to @vavrusam

marked as a Work In Progress

added 24 commits

• 165e1411...d0e32c6f - 23 commits from branch master
• b2cefdcf - modules/http: allow passing server options to http configuration

Compare with previous version

Rebased and fixed tests.

unmarked as a Work In Progress

merged

mentioned in commit 4eedc70c

Thank you for patience!

mentioned in commit 111590ad