Implement /-/reload #493
Implement /-/reload #493
Conversation
|
I've moved the handling of the server recreation logic to a new package, which helped a bit. I'll do a little more thinking about if it can be simplified tomorrow, and then test it one more time before opening this up for review. |
There was a problem hiding this comment.
Tested with tls and had some feedback on documentation. Ran through the code and tried calling the reload config endpoint with a variety of valid and invalid configs without issues. Did parallels calls to reload on both the primary and reload-port around 10k times with no observed memory leak or issues.
docs/api.md
Outdated
| server, as restarting it will prevent an HTTP client from reading the response | ||
| of the reload. Instead, use the `--reload-addr` and `--reload-port` flags | ||
| supported by the Agent. That will launch a secondary HTTP server that only | ||
| responds to `/-/reload` and can be used to safely reload the system. |
There was a problem hiding this comment.
Does it make sense to disable reload on the "primary" server if --reload-* is passed in?
There was a problem hiding this comment.
🤷♂️ I'm okay with leaving them both open and just recommend not using the primary server. I can see both arguments here though.
There was a problem hiding this comment.
Going to leave this as-is for now and let's come back later to see if we want to restrict it.
* implement /-/reload * move reloading server to a new server util package * documentation * more docs * fix bug in util/server * move unregisterer to util/server * document logger propagation * remove weird log propagation hack * remove go-cmp and defer to yaml comparison * document reload server constraints
github-actions
bot
added
the
frozen-due-to-age
PR Description
Adds
/-/reload. Supporting this change required ensuring that Entrypoint.Run doesn't exit when re-creating the server.Which issue(s) this PR fixes
Closes #147 (finally!)
Notes to the Reviewer
PR Checklist