Reimplement REST support with libevent

[tristan957]

Previously HSE used to use libmicrohttpd for its REST server. For
various reason that may or may not have been accurate, it was determined
that libmicrohttpd was not a good fit for what we were tring to
accomplish with a REST server in HSE.

Enter libevent. libevent is primarily an event loop library. It just so
happens that libevent also has support for an HTTP server. libevent is a
little bit more lighterweight than libmicrohttpd when it comes to
dependencies. libevent is also more useful than just an HTTP server. At
some point in time, we may want to integrate an event loop within HSE.
Who knows? But it is a possibility, and we wouldn't even need another
dependency for one.

I evaluated an alternative like libuv, which I happened to think might be
more maintained due to its use in technologies like node.js, but libuv
doesn't have an HTTP server, and instead only implements TCP. While the
HTTP 1.1 spec is fairly minimal, I don't have the time to implement it,
nor do I want other people to have to read it to contribute in this area
of the code.

I have created a libhse-rest.a for use within the rest of HSE. This
library provides the interfaces to register HTTP routes, create HTTP
request handlers, and lifecycle functions for an HTTP server. Many of
the interfaces are inspired by Go's HTTP support. libevent's structures
and APIs are hidden away from the users of the library should we ever
come to the determination that libevent no longer suits our needs. It
would be a fairly easy swap out of code contained with libhse-rest.a.

The hse_util/rest_client files have also been removed. These files made
HSE take a dependency on libcurl when the interfaces in
hse_util/rest_client weren't even used within the library. They were
used only in the CLI and maybe some tools. In order to replace the
functionality that was removed, I added a REST client into libhse-cli.a
in addition to REST API wrappers for C. The REST client is modeled a
little bit off of JavaScript's fetch() API.

The previous REST server was extremely under-documented. Routes,
methods, and parameters were fairly unknown. I believe you could query a
running REST server for some help documentation, but it just wasn't
cutting it. With this commit, I have added a docs/openapi.yaml file that
is a full description (or at least aims to be) of what the REST server
does and how to interact with it. OpenAPI is a fairly popular standard
for documenting HTTP APIs, so it was a fairly easy choice. In addition,
also added a GitHub Actions workflow for validating it in the event it
ever changes.

Many of the REST interfaces along with being re-implemented were cleaned
up. For instance, many used YAML. They all use JSON now. We set the
"Content-Type" header as appropriate. We return status codes which
describe the operations and their potential errors. This was previously
impossible in the previous iteration of the REST server. Some of the
output format changed, but all REST output has been thoroughly
documented with the OpenAPI description I have provided.

Signed-off-by: Tristan Partin tpartin@micron.com

[tristan957]

This is pretty much everything except, replacing kvdb_rest_test, replacing rest_api_test, and some of the routes that get registered in platform.c. I didn't want to look at those code paths anymore...Greg, want to help? :)

I created a static library libhse-rest, that doesn't leak anything about libevent to the rest of HSE. I have wrapped libevent through the use of opaque pointers where needed. This means we could in theory sub out libevent and revert back to libmicrohttpd, or some other HTTP sevrer library, at a moment's notice. Pretty simple.

I like where this has gone quite a bit, and I think it is a big step up over the design of the previous REST interfaces. I took a lot of inspiration from Go's HTTP support.

I am not entirely sure how confident I am in the project management of libevent considering there hasn't been a release in 2 years and pull requests seem to stall out quite a bit. But for what we are currently doing, libevent should be fine.

I also evaluated libuv, but it doesn't implement HTTP support, only TCP support, and I don't really feel like reading and implementing the HTTP spec. I think libuv is probably the better event loop library however since more professional projects probably rely on it.

I evaluated libsoup from the GNOME ecosystem, but didn't want to bring GLib as a transitive dep. Would much rather use GLib for testing infrastructure though.

I evaluated libwebsockets, but it felt like it was going to drag in a lot of unnecessary stuff.

I did find some C++ solutions like https://github.com/facebook/proxygen, but they are C++, although with the libhse-rest.a interface, C++ would basically be an implementation detail if you expose a C API.

I have also been toying with the idea of making the REST support optional to build in HSE.

I kept the notion of non-exact routes around even though I don't like them. I think using a regex engine like PCRE2 would be a lot better, but I didn't feel like adding another dependency.

Oh and you'll notice, I pretty much finished the docs/openapi.yaml file minus the aforementioned unimplemented routes. What this gets you is a document that can be rendered like so:

I added linting to the CI for this document too. You can drill down to read more about these routes like:

Overall, pretty cool. Getting to pull a lot on my previous web development experience.

Edit: Just another thought while we are thinking about HSE's dependencies, we should remove cURL from a dependency of libhse, and make it a dependency of the CLI and any tools that need it. I think rest_client_test as it currently exists is not really worth maintaining.

[tristan957]

So as it stands right now, there is a race condition when rest_start_server() is called, which spawns the thread to start the event loop, and rest_stop_server() is called right after. We may call out to exit the event loop prior to the event loop being started. This is noticeable in the tests I have been working on. Only solution is to throw a usleep() around from what I can tell.

I don't think libevent allows you to know when the event loops has started. I opened an issue on the libevent repo, but overall I don't have much faith in libevent if I can't do this workflow properly. Maybe other people have ideas.

[tristan957]

Fixed the race condition. Nice. Feel a lot better about it now.

[tristan957]

Can you provide a list of signals which you would like me to block?

Presumably I need to use the pthread_sigmask(2) interface.

[tristan957]

what is the 0 in http://localhost/csched/0? compaction job id?

[beckerg]

That zero is the alias passed into sp3_create().
As a library all our threads must block all signals, we cannot intercept signals that the app might be expecting.

[tristan957]

As a library all our threads must block all signals, we cannot intercept signals that the app might be expecting.

Sure, but I didn't know how to do it. Looking at workqueue code, I found my answer. New to signal blocking 😄

    sigfillset(&sigset);
    pthread_sigmask(SIG_BLOCK, &sigset, NULL);

[beckerg]

Same here, just bitwise-or the results of all json calls and check the result at the end, will reduce LOC by 3/4 and eliminate all the gotos.

[beckerg]

Wow, every one of these calls does a malloc, the old code didn't do any mallocs nor anything that might potentially sleep while holding the lock. I see that cjson has a provision for the caller to provide an allocator, so at least there's a potential to make this more efficient and minimize lock hold time. Ugh...

[tristan957]

Your idea about the bitwise is nice. Will look into it.

[beckerg]

Yeah, I use that in contexts where errors are extremely unlikely, can really eliminate a lot of mindless boilerplate. Some people would write a horrid macro to do it, leveraging local vars, ugh, I wouldn't go there! Just so long as you can keep calling into cjson having had a failure, looks like there is no issue based on my casual look at it...

I was thinking another way to do it is to build vectors of names and args then do it in a loop. Would take maybe 7 or 8 lines, but requires that all calls be the same..

[alexttx]

Or one giant if expression since they share the error value:

if (!cJSON_this() ||
    !cJSON_that() ||
    !cJSON_blahblah)
{
    status = ;
    goto end;
}

[tristan957]

Already went with Greg's bitwise idea. About to push updates.

[beckerg]

My suggestion eliminates all the compares since short-circuit evaluation isn't needed here, so should be less code and faster than the big-if statement. Would be interesting to compare the optimized assembly from both approaches...

[beckerg]

Ok, but seems outside the scope of this patch..

[beckerg]

Do we now emit it?

[tristan957]

Seems like not if you are referring to the original comment. I can carry that forward if you would like.

[tristan957]

Went ahead and copied it back in to my PR.

[beckerg]

Was just asking, we could eliminate all that cpp stuff if so...

[tristan957]

Oh, I see. Yea I don't really know what to do here. Seems like emitting the root is pointless anyways.

[beckerg]

Given a typical ivl_cnt of 31, hat's gotta be at least 155 mallocs per bucket, times the number of perf counters, ouch...

[beckerg]

With kvt and it's (1262 * 29 * 5) -> 182,990 mallocs just to build the histograms in json, and another (1262 * 7) mallocs for the other fields in those counters, so 191,824, yikes. Worst case, if we actually had 255 kvs', then it's would be something like 8 times that... And that doesn't include any of the non-distribution type counters...

[beckerg]

OMG, it's even worse than that, each call to AddNumberToObject calls strdup() on the name string, so almost 400,000 mallocs to dump the stats...

Also note that these "AddNumber" functions convert the 64-bit number to a double, which mean we lose 8-bits of precision in the actual number once the number goes over 52-bits in size. Not a problem for most use cases, but gtk if you're counting something like bytes-written and you go over 4PB and are wondering why your count is off..

[tristan957]

cJSON is not very good. I have talked to Alex about replacing it potentially next cycle and he seems to be on the same page.

cJSON does have a function called cJSON_AddItemToObjectCS() where the CS stands for constant string. The API for it kind of blows, but someone could push me to use it.

[beckerg]

Micron Confidential If you remove the tester then you have no direct way test if one’s “fixes” to the allocator improve or hurt things. As-is, it can test the allocator while the system is under a real load which yields very different results than testing on an otherwise idle machine… Micron Confidential From: Tristan Partin ***@***.***> Sent: Tuesday, July 19, 2022 12:57 AM To: hse-project/hse ***@***.***> Cc: Greg Becker (gbecker) ***@***.***>; Review requested ***@***.***> Subject: [EXT] Re: [hse-project/hse] Replace libmicrohttpd with libevent (PR #316) CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless you recognize the sender and were expecting this message. /ps renamed to /workqueues, but I think it has been implemented. Next is /kmc/vmstat. I think I will remove /kmc/test since it seems like it just testing allocators. I'll just let someone yell at me if this is wrong 😄! From there, onto the data tree (kill me) + cn tree stats! — Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/hse-project/hse/pull/316*issuecomment-1188630569__;Iw!!KZTdOCjhgt4hgw!8ul1O7W4y-N_s17OT6FCRWGwfDhh21XMLDGI7lo8aVu7lM9EMO1S1K8ka8_nJGQelm48X40TDBCW8zw2R6j91es$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ACTIJVKSGIVYLTVSM4Z74JTVUY7SBANCNFSM52LHUHUQ__;!!KZTdOCjhgt4hgw!8ul1O7W4y-N_s17OT6FCRWGwfDhh21XMLDGI7lo8aVu7lM9EMO1S1K8ka8_nJGQelm48X40TDBCW8zw2TMN56TQ$>. You are receiving this because your review was requested.Message ID: ***@***.******@***.***>>