Upgrade unipi to paf-le-chien #4
Conversation
|
So an implementation of |
|
I don't know. What is the goal? This PR adds quite some boilerplate to the codebase :/ Will this remove the cohttp and conduit dependency entirely? if not, can we get to this point (easily?) -- maybe if let's encrypt specifies their own module type for the HTTP client used (to avoid the cohttp dependency)? |
I think indeed we can do something better on this side. The main problem is the But I'm not sure that the sub-module
The only remaining module is the
It can be a nicer solution indeed! Again, we already talk about On the other side, |
|
PS: I can try to run some benchmark to between this version and the version with |
|
So I did a large stress-test between
Such test is done over TLS for both. This is the plain text of the benchmark ( dinosaure@turbine:~$ hey -n 1000000 -c 24 https://unipi.egar.im/
Summary:
Total: 73.8878 secs
Slowest: 0.2575 secs
Fastest: 0.0010 secs
Average: 0.0018 secs
Requests/sec: 13533.8195
Total data: 22999632 bytes
Size/request: 23 bytes
Response time histogram:
0.001 [1] |
0.027 [999926] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
0.052 [47] |
0.078 [9] |
0.104 [0] |
0.129 [0] |
0.155 [0] |
0.181 [0] |
0.206 [0] |
0.232 [0] |
0.258 [1] |
Latency distribution:
10% in 0.0013 secs
25% in 0.0014 secs
50% in 0.0015 secs
75% in 0.0017 secs
90% in 0.0021 secs
95% in 0.0024 secs
99% in 0.0103 secs
Details (average, fastest, slowest):
DNS+dialup: 0.0000 secs, 0.0010 secs, 0.2575 secs
DNS-lookup: 0.0000 secs, 0.0000 secs, 0.0183 secs
req write: 0.0000 secs, 0.0000 secs, 0.0021 secs
resp wait: 0.0017 secs, 0.0009 secs, 0.2575 secs
resp read: 0.0000 secs, 0.0000 secs, 0.0025 secs
Status code distribution:
[200] 999984 responsesAnd dinosaure@turbine:~$ hey -n 1000000 -c 24 https://unipi.egar.im/
Summary:
Total: 427.5950 secs
Slowest: 0.2574 secs
Fastest: 0.0011 secs
Average: 0.0102 secs
Requests/sec: 2338.6243
Total data: 22999632 bytes
Size/request: 23 bytes
Response time histogram:
0.001 [1] |
0.027 [828281] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
0.052 [170993] |■■■■■■■■
0.078 [681] |
0.104 [4] |
0.129 [0] |
0.155 [0] |
0.180 [0] |
0.206 [0] |
0.232 [0] |
0.257 [24] |
Latency distribution:
10% in 0.0019 secs
25% in 0.0024 secs
50% in 0.0028 secs
75% in 0.0040 secs
90% in 0.0451 secs
95% in 0.0465 secs
99% in 0.0495 secs
Details (average, fastest, slowest):
DNS+dialup: 0.0000 secs, 0.0011 secs, 0.2574 secs
DNS-lookup: 0.0000 secs, 0.0000 secs, 0.0019 secs
req write: 0.0000 secs, 0.0000 secs, 0.0011 secs
resp wait: 0.0020 secs, 0.0010 secs, 0.0456 secs
resp read: 0.0081 secs, 0.0000 secs, 0.0680 secs
Status code distribution:
[200] 999984 responsesThe tool |
|
Thanks @dinosaure, the benchmark looks convincing. About let's encrypt: I don't know what the state of mirage-http is. I'd be fine to have a HTTP Client module type in letsencrypt directly. |
That's a surprisingly large difference. There are some http benchmarks at https://github.com/ocaml-multicore/retro-httpaf-bench and there we see httpaf being "only" about twice as fast as cohttp:
That graph is from wrk2 with 1000 open connections repeatedly requesting a single static page (which fits in one packet). All servers were configured to use a single core for this test. |
Yeah, I think it's not a fair benchmark for many reasons:
For these points, it's why I said that the "benchmark" is not reproductible and show-up (with |
|
So https://unipi.egar.im/ is alive for a long time so I believe that I'm ready to cut a release of |
|
Thanks again @dinosaure -- there's still the outstanding question of the dependency cone in respect to letsencrypt (which currently has a hard dependency on cohttp, and paf has a dependency on letsencrypt). I'd appreciate if we can conclude:
WDYT? |
|
Yes, let me sometimes to shape all of that, I still need to think to revive |
|
So, now that letsencrypt 0.3.0 is in opam-repository, we could proceed with (a) adapting paf and (b) removing cohttp from unipi. This would clean up the dependency cone drastically :) (and reduce the binary size). |
|
I updated the PR with last changes on:
However, we still need to |
|
Just waiting the release of |
|
Thanks for your PR. I pushed a cleanup commit on top. I'll raise some questions via the code comment / review system. |
| Logs.info (fun f -> f "requested %s" path); | ||
| match Astring.String.cuts ~sep:"/" ~empty:false path with | ||
| | [ h ] when String.equal hook_url h -> | ||
| begin | ||
| hookf () >>= function | ||
| | Ok data -> Http.respond ~status:`OK ~body:(`String data) () | ||
| Lwt.async @@ fun () -> hookf () >>= function |
There was a problem hiding this comment.
I'm not really comfortable with this Lwt.async -- what is the resource / connection / task story for Httpaf here?
There was a problem hiding this comment.
By type, http/af requires that the request_handler (in that case, dispatch) returns unit instead of unit Lwt.t. Then, httpaf has an internal queue which care about callbacks and execute these functions (unit -> unit) which can emits the request to Read/Write or Error into the socket (via respond_with_*) via the given reqd.
Concurrently, paf processes such tasks with mirage-tcpip on the other side via a server connection. So, resources (such as buffers, internal states, etc.) are shared between the server connection and the given reqd. As long as the reqd exists, internal resources exists.
In that case, and it's the case for the other branch, we require resources which are only available via Lwt. If you are scare about what it can happens into the Lwt.async (such as an exception), may be we can/should add an Lwt.catch inside and call then Reqd.report_exn to be sure that in any way, the resource will be free whatever happens the process.
| let headers = Httpaf.Headers.of_list | ||
| [ "content-length", string_of_int (String.length data) ] in | ||
| let resp = Httpaf.Response.create ~headers `OK in | ||
| Httpaf.Reqd.respond_with_string reqd resp data ; |
There was a problem hiding this comment.
This one does actually send out data, does it not? but it does not seem to be in the Lwt monad -- what is the story here?
There was a problem hiding this comment.
As I said above, respond_with_string will just fill an internal buffer into the reqd which is shared with a server connection owned by paf (for instance). Concurrently, paf launched an unit Lwt.t which cares about Read/Write operations.
Then, an internal queue of callbacks exists in httpaf which will execute them one per one and emits syscalls action in on side (via server connection) and consume what the user wants on the other side (via the given reqd).
A question can subsist about data race condition, but the computation model of lwt and the global GC lock ensure (due to mutation of the internal queue) ensure that everything is safe (I believed).
| Http.respond ~status:`Internal_server_error ~body:(`String msg) () | ||
| let headers = Httpaf.Headers.of_list | ||
| [ "content-length", string_of_int (String.length msg) ] in | ||
| let resp = Httpaf.Response.create ~headers `Internal_server_error in |
There was a problem hiding this comment.
the above three lines can be factored out together with 109 - 111.
| Httpaf.Reqd.respond_with_string reqd resp data ; | ||
| Lwt.return_unit | ||
|
|
||
| let redirect port _ reqd = |
There was a problem hiding this comment.
could the second argument now be removed? what is provided here?
There was a problem hiding this comment.
The second argument is the Ipaddr.t * int peer/the client. You can not remove it because it is given by paf.
| let dispatch store hookf hook_url request _body = | ||
| let p = Uri.path (Cohttp.Request.uri request) in | ||
| let path = if String.equal p "/" then "index.html" else p in | ||
| let dispatch store hookf hook_url _conn reqd = |
There was a problem hiding this comment.
_conn is the Ipaddr.t * int too. You can not remove it (because the caller pass it to the callback).
| let port = if port = 443 then None else Some port in | ||
| let new_uri = Uri.with_port new_uri port in | ||
| let path = request.Httpaf.Request.target in | ||
| let new_uri = Uri.make ~scheme:"https" ?host:(Key_gen.hostname ()) ?port ~path () in |
There was a problem hiding this comment.
is this the only remaining use of uri? can we drop that dependency? (I'm fine working out the string stuff to get the "new url" ;)
There was a problem hiding this comment.
I don't have strong opinion about that, feel free to remove it 👍.
| ; LE.account_seed = Key_gen.account_seed () | ||
| ; LE.account_key_type = `ED25519 | ||
| ; LE.account_key_bits = Some 4096 | ||
| ; LE.hostname = Key_gen.hostname () |> Option.get |> Domain_name.of_string_exn |> Domain_name.host_exn } |
There was a problem hiding this comment.
the previous defaults for account key type and certificate key type should be used here -- also there should be command line arguments for the key types (and bit sizes)
|
with this branch, @hb9cwp reported successful builds (on Unix and hvt) on OpenBSD. But the hvt unikernel does not serve web pages -- maybe related to the httpaf/mimic/paf semantics of Lwt.async that were discussed above? @dinosaure did you do functional tests with a hvt unikernel (that it actually serves web pages)? |
|
@hannesm @dinosaure Good news: with this branch, I have got both |
|
@hb9cwp great! This means your unix and hvt unikernels deliver data via HTTP(S) to clients that send requests (in contrast too your earlier comment that there's no content being delivered)? |
|
@hannesm Yes, exactly. So far, I tested serving simple .html pages and .png images with HTTP though, HTTPS clients to come. Also, Unipi's hook works and triggers it to refresh from Github using HTTPS, SSH to come. P.S. Also, Unipi unikernels answer requests on both their IPv4 and IPv6 addresses of their |
|
@hb9cwp great, thanks for your confirmation.
indeed :)
yes :) also using DNS-over-TLS by default (to anycast.uncensoreddns.org)
sadly not yet AFAICT (lack of using happy-eyeballs in the git client code) |
|
merged manually into main, thanks for the PR! |
|
It has been some months after merging this, though there is some regression:
|
It's a draft, at least it compiles but the TLS path seems buggy and it's hard to understand why. I will try to investigate more deeper on this side soon. But it's a proposal, feel free to share your opinion on it and improve this unikernel 👍.