Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Liveness/readiness checks #383

Closed
wants to merge 7 commits into from
Closed

Liveness/readiness checks #383

wants to merge 7 commits into from

Conversation

kvosper
Copy link
Contributor

@kvosper kvosper commented Mar 7, 2019

Adds endpoints for checking:

  1. Styx is live (i.e. running)
  2. Styx is ready (to serve traffic)

@kvosper kvosper requested review from dvlato and mikkokar March 7, 2019 14:21
@bestokes
Copy link

@mikkokar please can you review?

Copy link
Contributor

@mikkokar mikkokar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose an alternative approach for the readiness checks: Introduce a StaticResponseHadler in the Styx proxy pipeline to serve the readiness responses. Then tag all readiness requests with a specific X-Styx-Readiness-Check header and divert them to the StaticResponseHandler. Let all other requests to pass through.

Then you could do all the readiness checks directly to Styx HTTP and HTTPS ports.

Something to think about.

@dvlato dvlato dismissed their stale review March 15, 2019 16:40

As per Mikko's review we are changing the approach.

@Override
public String toString() {
return "service:" + serviceName();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some observations:

  • Most of our toString methods conform to AbstractStyxService{name=<servicename>} format. Worth considering here?

  • Also print service status?

  • A service is a stateful object and styx can have two services with identical names. To distinguish between two instances it might be necessary to include object identity in the output.

@@ -115,6 +116,8 @@ public HttpServer build() {
httpRouter.add("/admin/configuration/logging", new LoggingConfigurationHandler(styxConfig.startupConfig().logConfigLocation()));
httpRouter.add("/admin/configuration/startup", new StartupConfigHandler(styxConfig.startupConfig()));

httpRouter.add("/admin/startup", new StartupStatusHandler(environment.configStore()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide an admin page link.


components.plugins().forEach(plugin -> components.environment().configStore().set("plugins." + plugin.name(), plugin));
StyxService adminServerService = new ServerService("adminServer", () ->
this.adminServer.store(createAdminServer(components)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it is necessary to an HttpServer instance and wrap it into a ServerReference object?

Why not to create an HttpServer instance and pass it as is to ServerService? Like so:

new ServerService(createAdminServer(components));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of the ServerService is to run the creation/start-up process of the server. If you do it before construction, then the service does nothing, and we would not be able to start the servers asynchronously.
The ServerReference is because the StyxServer needs to receive the server after it has been created asynchronously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createAdminServer() just creates a server object. It doesn't start it. The service is still be able to start it by invoking startAsync.

Okay. I see. The createProxyServer blocks until the plugins are loaded. This is somewhat confusing:

createProxyServer() returns an HttpServer instance which has startAsync method. Typical usage is:

HttpServer server = createProxyServer();
server.startAsync().awaitRunning();

Where does it block? On both lines. Surprisingly. The first line blocks at loading the plugins, the 2nd line blocks at waiting for the server to start.

IMO it would be more sensible to wait for plugins in the service startup method, rather than in HttpServer creation.

@kvosper
Copy link
Contributor Author

kvosper commented Mar 27, 2019

We have decided not to go ahead with this PR, however it did spawn two smaller pieces of work:

#389
#390

@kvosper kvosper closed this Mar 27, 2019
@kvosper kvosper deleted the liveness-readiness-2 branch May 23, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants