Convert Cortex components to services

[pstibrany]

This PR is a supplement to proposal document to revamp service model used by Cortex.

The document and this PR are based on the concept of "Service", as used in Google Guava library: Service is an object with operational state, with methods to start/stop the service, well-defined and observable state, methods for waiting for state transitions. Google Guava is a Java library, here we use reimplementation of Services concept in Go. Update: now part of Cortex, as of PR #2188.

In this PR, Cortex components like ingester, ruler, ring or lifecycler are converted to such services.
During startup Cortex collects all the services from modules that are supposed to run (based on target module), and then starts them in parallel. Module can either contribute a service directly, or use so-called "wrapped" service – wrapped services will make sure that services are still started and stopped in a correct order based on module dependencies.

Important change in module initialization code is that services are returned in "New" state – they are not supposed to start any background tasks until started. That means that initialization is faster.

Main Cortex Run method now simply starts all services, and then waits until "Server" service stops or any service fails, and then initiates shutdown. "Server" service is special because it reacts on SIGTERM signal (this is how Cortex always worked). It also runs HTTP and gRPC servers.

Not all benefits of services model are implemented, but some are: for example Blocks-based StoreQueryable now fetches indexes in the background, while other services start (most importantly "Server" itself with HTTP server, for collecting metrics). Until StoreQueryable is done, Cortex doesn't transition to "Started" phase, but since HTTP server already runs, we can collect its metrics.

WAL replays in ingester are not done in background in this PR... while it would be trivial to do this change, ingester would need more changes to check if it's running already or not, and it seemed like out of scope for this PR.

Another possible area for cleanups is Lifecycler -- now it has a way to return errors, instead of just doing os.Exit. We should use that.

Please familiarize yourself with services concept, Go implementation (Service interface, BasicService implementation and ServiceManager. This PR touches many places, but "conversion" to services was pretty straightforward.

This is an alternative to PR #2119.

[sandeepsukhani]

I would say overall you did a very good job @pstibrany!
I did a first pass through the new repo and how it is used in Cortex. I have added some comments about design decisions. Some of the things that I pointed out are subjective so its not necessary that you follow them all.

While the services repo looks solid I was thinking while we are at it maybe we should offload dependency management to the new package like starting and stopping the dependencies in order. Cortex or any other project using services repo would register a bunch of services as ordered dependencies with each service and that code should take care of it. What do you think?

[sandeepsukhani]

Maybe it is just me but I would prefer these functions to be in an interface? For each service type, there can be a different interface. What do you think?

[pstibrany]

In general, there is one Service interface, and currently one BasicService implementation.

These functions are specific to the implementation. BasicService requires all of them, but not all services need to use them all. These functions are not related to different types of services, single service often uses all three.

[sandeepsukhani]

Yes, right. I was suggesting to define an interface for each service type with the methods that the caller needs to define to create a service like for InitIdleService function make an interface as below:

type IdleService interface {
	type StartingFn func(serviceContext context.Context) error
	type StoppingFn func() error
}

[pstibrany]

But even "idle" service may not require starting or stopping function, and using interface would force it to have one (which does nothing).

Furthermore, by using interface, one would need to have type implementing the interface, but that's often not practical. For example, initQuerier now returns idle service without needing any extra type: https://github.com/pstibrany/cortex/blob/5c9d7faad7121d982ed16cd11a601d693e258b99/pkg/cortex/modules.go#L323

And if for example Ingester implemented this interface directly, those StartingFn and StoppingFn would become part of its public API, which is what I want to avoid.

[sandeepsukhani]

I think we can get rid of goto here by adding an else block or maybe move the code from goto block to some other function and call it maybe cleanupService() which is then called in a defer before b.serviceContext.Err(). What do you think?

[pstibrany]

Originally I had service lifecycle implemented in several methods, but decided to keep it in one for simplicity. It think it is easier to follow the logic this way, and one goto when starting succeeds, but context is canceled so we skip Running, is fine.

Alternatively, we can remove this completely, and just go to Running, which should then find out that it needs to stop. Skipping Running state is my go-specific "invention", and I'm not 100% it is the correct one.

[pstibrany]

You're right that else would work too

[pstibrany]

Thank you for your review Sandeep!

While the services repo looks solid I was thinking while we are at it maybe we should offload dependency management to the new package like starting and stopping the dependencies in order. Cortex or any other project using services repo would register a bunch of services as ordered dependencies with each service and that code should take care of it. What do you think?

In general I'd like to keep the package as small as possible and close to Guava, to ease adoption for people who know it already. Especially Service interface, service lifecycle, ServicesManager and listeners are basically set to stone. But this can be a useful extension to include, if implemented well.

In Cortex, modules that don't share dependencies already start concurrently (this is new in this PR), so basically modules on each line start at the same time (assuming they all would start):

• Server, RuntimeConfig and MemberlistKV
• Ring, Overrides, TableManager, Configs, AlertManager, Compactor
• Distributor, Store, QueryFrontend
• StoreQueryable
• Ingester, Ruler

This may be tricky to express nicely in a general way, to be included in general package, but perhaps.

[pracucci]

Good job @pstibrany! I can't say this PR is easy to review: it's pretty large and unfortunately a good candidate for rebasing pain in the future and took me about 3h to review. As a retrospective, we could have tried to work on a smaller initial step (ie. progressively adding Service to components first). Let's see if we can get this merged in a reasonable time.

Good job on code comments. They helped me a lot understanding the state and thus reviewing the code 👏

Question: what's about the /ready probe? Is it mandatory to have one configured for each Cortex component once this PR will be merged, otherwise we may have some endpoints hit ahead of time (ie. querier)?

[pstibrany]

Good job @pstibrany! I can't say this PR is easy to review: it's pretty large and unfortunately a good candidate for rebasing pain in the future and took me about 3h to review. As a retrospective, we could have tried to work on a smaller initial step (ie. progressively adding Service to components first). Let's see if we can get this merged in a reasonable time.

I have tried that. And this is the result :-( I realize it's not small at all, but doing it for single module only didn't make much sense to me.

I will split this PR into two, one to introduce services package to Cortex, and one with the rest of the work.

Question: what's about the /ready probe? Is it mandatory to have one configured for each Cortex component once this PR will be merged, otherwise we may have some endpoints hit ahead of time (ie. querier)?

Querier already has /ready probe, but it doesn't react on services status, it just always returns 200. This is definitely something to add! I would probably go as far as checking the state of entire Cortex service manager, and report Ready only if all services are Running. In case of microservices, it would reflect the main service, in case of single binary, it's the state of entire Cortex binary. What do you think?

[pracucci]

Querier already has /ready probe, but it doesn't react on services status, it just always returns 200. This is definitely something to add! I would probably go as far as checking the state of entire Cortex service manager, and report Ready only if all services are Running. In case of microservices, it would reflect the main service, in case of single binary, it's the state of entire Cortex binary. What do you think?

I don't see any other way to do it. In single binary mode you can't have a per-internal-service readiness probe, so /ready should return 200 only after all services are ready.

[pstibrany]

I think this is now ready for another review.

Here is list of follow-up work when this PR is merged

• /services should be nicer, and documented in docs/apis.md
• /services should include sub-services in the output, see status.go (I'm thinking via new interface which would give access to sub-services in a generic way, but services then need to make sure they return correct results in race-free way)
• querier.New should implement a service, so that other components can wait for it to be ready. Also storeQueryable should be a service, and querier should then reflect on it.
• ruler should wait until querier is running, before doing anything
• Ingester: move more stuff into Starting (WAL replays). Requires that Ingester checks its running state on pushes and queries.
• Ingester v2: move opening of TSDBs into Starting
• Ingester WAL - convert to service, manage via subservices
• cleanup Lifecycler, return errors from methods instead of calling os.Exit

[pracucci]

Fixes #784

[pracucci]

Thanks @pstibrany for addressing my feedback! ❤️

As discussed offline, in this PR we should also:

• mention configuring /ready probe for ALL services
• mention breaking changes when vendoring
• check querier changes in current PR and see if we return correct errors

Could you also rebase master, please? I would like to see the new querier integration tests running too.

[pstibrany]

• mention configuring /ready probe for ALL services

Done

• mention breaking changes when vendoring

Done

• check querier changes in current PR and see if we return correct errors

I've modified code so that it returns 500 instead of 422 (indicates client error) from API when server is not yet ready:

$ curl -v http://localhost:8000/api/prom/api/v1/query?query=up 
> GET /api/prom/api/v1/query?query=up HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 500 Internal Server Error
< Content-Type: application/json
< Date: Fri, 28 Feb 2020 18:25:15 GMT
< Content-Length: 91
< 

{"status":"error","errorType":"internal","error":"BlockQueryable is not running: Starting"}

We could also wrap API handler with a check if server is ready and return 503 if it isn't. We may want to do that in a separate PR.

Could you also rebase master, please? I would like to see the new querier integration tests running too.

Done.

Thanks for your review!

[pracucci]

Thanks @pstibrany for this amazing work! LGTM!

[codesome]

This is not checking any new error (rather the err from t, err := cortex.New(cfg)). If you intended to check for error from t.Run(), you need to have err = t.Run() outside of the if.

[pstibrany]

You are correct. I've now removed this line completely. Everything now happens in Run anyway.

[gouthamve]

Nice work Peter! LGTM!

Though this touches a lot of things and its hard to get things right. But now that we are running it in our dev and staging environments successfully, I think this is ready to go!

[michaeljoy]

Just curious, why not implement the basic health checks that Prometheus does along with the vast majority of exporters for simplicity and consistency? Found myself deep diving the source code to try and reverse engineer a health check as none was readily available, nor documented, and it didn't follow the prometheus or kube conventions (best not to reinvent the wheel these days for something as simple as a health check).

Prometheus provides a set of management API to ease automation and integrations.

Health check
GET /-/healthy
This endpoint always returns 200 and should be used to check Prometheus health.

Readiness check
GET /-/ready
This endpoint returns 200 when Prometheus is ready to serve traffic (i.e. respond to queries).

[pracucci]

Readiness check
GET /-/ready
This endpoint returns 200 when Prometheus is ready to serve traffic (i.e. respond to queries).

I think this is a very good feedback. We already had /ready for ingesters so we kept compatibility, but nothing prevent us from adding support to /-/ready. @michaeljoy are you willing to submit a PR? Should be easy to add the support (see where we register /ready in pkg/cortex/cortex.go).

[pstibrany]

I think we can also add /healthy (as server with only New/Starting/Running/Stopping services, but no Failed or Terminated ones). What do you think?

Other than being compatible with Prometheus (which we aren’t in many other ways), what’s the point of having both /-/ready and /ready?

[michaeljoy]

I agree having both is mostly pointless. Having 1 endpoint that returns a 200 is sufficient. Typically you don't want these to be heavyweight checks and if they are, you cache the results for some time anyways to prevent DOS'ing the more costly layer checks. I think the only reason for having both is if you are exposing some heavyweight check that you don't want your load balancer hitting and or exposing externally. Which in this case is needless duplication I agree.

The primary purpose in this case is to give the container scheduler (ECS, Fargate, et al) the ability to terminate a container that is no longer able to serve requests, and only consider swapping containers once a container is ready to service requests successfully. Also, to expose it in a way that doesn't require prior knowledge of the API to implement at a systems level.