Modules can now start additional startup tasks, while server is starting #2119

pstibrany · 2020-02-12T10:05:02Z

Modules can now have start function for starting additional tasks on startup.

Start functions are called concurrently, and only after all modules are initialized. Server's HTTP service starts while modules are still starting, so modules must be able to handle that. Both ingester and queriers will return error.

This is currently used by blocks-based ingester (to replay WAL for locally stored TSDBs) and querier (to do initial fetch of blocks).

This is alternative to #2104.

Note: this is not ideal, but is relatively simple change. I think we should overhaul the lifecycle of our modules, and have separate NEW (instantiated, but nothing started yet), STARTING (replaying WALs, fetching blocks, whatever needs to be done for service to be ready) and RUNNING (service is running) states and make those states observable by other modules (for dependencies and for global view on the server, plus make those states observable via metrics / simple HTML page), but this PR doesn't go that far. I'll write a design document for that first.

Signed-off-by: Peter Štibraný [email protected]

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

thorfour

LGTM! This looks like a great qol change thank you!

jtlisi

LGTM

It might be worth noting the new startup behavior in Blocks documentation. If I'm not mistaken, the user must be aware that the startup function needs complete before a transfer can take place and tune accordingly?

jtlisi · 2020-02-13T04:21:25Z

pkg/cortex/cortex.go

+func (t *Cortex) start(target moduleName) error {
+	deps := orderedDeps(target)
+
+	wg := sync.WaitGroup{}


Since you are only returning errors you should consider using a sync.errgroup instead of the waitgroup and channel.

Thanks, looks exactly like what I need.

jtlisi

Withdrawing approval til integration tests are sorted

pstibrany · 2020-02-13T11:07:27Z

Before approving this, I will check return values from gRPC methods to make sure we send the correct errors and status codes back.

pstibrany · 2020-02-13T11:08:27Z

Withdrawing approval til integration tests are sorted

Integration tests found a real bug (start was only called on dependencies, but not target module itself), which is now fixed. I didn't catch it in my testing, as I was only running with All target which doesn't have start function.

…startup. Start functions are called concurrently, and only after all modules are initialized. Server's HTTP service starts while modules are still starting, so modules must be able to handle that. This is currently used by blocks-based ingester and querier. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

…d yet. codes.Unavailable seems to be the correct code, and client is expected to retry later (with some backoff). Signed-off-by: Peter Štibraný <[email protected]>

pstibrany · 2020-02-13T14:29:04Z

Before approving this, I will check return values from gRPC methods to make sure we send the correct errors and status codes back.

After some reading, I believe returning status.Error(codes.Unavailable, message) is the right thing to do from gRPC handlers, when ingester or querier are not ready yet. When client receives Unavailable, it is expected to retry (with some backoff), which is exactly what we want to express here.

I've updated the code accordingly.

pstibrany · 2020-02-13T14:42:31Z

After some reading, I believe returning status.Error(codes.Unavailable, message) is the right thing to do from gRPC handlers, when ingester or querier are not ready yet. When client receives Unavailable, it is expected to retry (with some backoff), which is exactly what we want to express here.

I've updated the code accordingly.

Of course, ingester doesn't even register itself into the Ring, until it is ready.

With blocks store situation is more complicated... both querier and ruler that use it are started immediately, and don't wait for "block store" to be ready first. All requests to the block store will however fail until it is. This may cause troubles and I will investigate more how to solve it. Ideally they should observe it's component for readiness before using it.

codesome

This is exactly what I was looking for atm. Perfect timing!

codesome · 2020-02-14T09:12:13Z

go.mod

@@ -67,7 +67,9 @@ require (
 	github.com/weaveworks/common v0.0.0-20200201141823-27e183090ab1
 	go.etcd.io/bbolt v1.3.3
 	go.etcd.io/etcd v0.0.0-20190709142735-eb7dd97135a5
+	go.uber.org/atomic v1.5.0


Instead of this additional dependency, can we use https://golang.org/pkg/sync/atomic/ for atomic loads and stores?

We already have this dependency, although under old name (github.com/uber-go/atomic). I've removed old version in PR #2127.

gouthamve · 2020-02-14T09:48:28Z

@pstibrany Is the querier and ruler using the ingester only a problem in single binary mode? Can we not register the ingester in the ring (or mark it pending) until it starts up?

pstibrany · 2020-02-17T09:22:20Z

@pstibrany Is the querier and ruler using the ingester only a problem in single binary mode? Can we not register the ingester in the ring (or mark it pending) until it starts up?

Ruler isn't using ingester. Querier is, but only indirectly, via gRPC calls.

Problem that this PR is solving for QuerierChunkStore is that blocks-based QuerierChunkStore needs to fetch blocks metadata on startup, which can take a while. However, Ruler is using QuerierChunkStore and has no way to see whether QuerierChunkStore has finished its fetching and is now ready to use, or not.

I'd like to solve this in a general way, and will send new PR showing how to do that. I'd suggest to keep this PR open for now, so that we can compare both approaches and see which one works better. (I don't really like approach in this PR, to be honest)

pstibrany · 2020-02-20T20:05:22Z

I have sent alternative PR to this one, #2166. It revamps services module and supports everything that we need to correctly implement in this PR, in a much better way. If you have time, please take a look.

gouthamve · 2020-03-04T13:36:39Z

Closing this in favour of #2166

thorfour approved these changes Feb 12, 2020

View reviewed changes

jtlisi approved these changes Feb 13, 2020

View reviewed changes

jtlisi suggested changes Feb 13, 2020

View reviewed changes

pstibrany added 5 commits February 13, 2020 14:04

Use errgroup.

bc3ee7e

Signed-off-by: Peter Štibraný <[email protected]>

Make sure to call target's start function as well.

1d23c9b

Signed-off-by: Peter Štibraný <[email protected]>

lint check

4441937

Signed-off-by: Peter Štibraný <[email protected]>

Send back codes.Unavailable code if ingester or querier aren't starte…

c579ba5

…d yet. codes.Unavailable seems to be the correct code, and client is expected to retry later (with some backoff). Signed-off-by: Peter Štibraný <[email protected]>

codesome approved these changes Feb 14, 2020

View reviewed changes

codesome mentioned this pull request Feb 14, 2020

Flusher target to flush WAL #2075

Merged

3 tasks

pstibrany mentioned this pull request Feb 20, 2020

Convert Cortex components to services #2166

Merged

gouthamve closed this Mar 4, 2020

Modules can now start additional startup tasks, while server is starting #2119

Modules can now start additional startup tasks, while server is starting #2119

Uh oh!

Conversation

pstibrany commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thorfour left a comment

Choose a reason for hiding this comment

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

jtlisi Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

pstibrany Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany commented Feb 13, 2020

Uh oh!

pstibrany commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstibrany commented Feb 13, 2020

Uh oh!

pstibrany commented Feb 13, 2020

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

codesome Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

pstibrany Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouthamve commented Feb 14, 2020

Uh oh!

pstibrany commented Feb 17, 2020

Uh oh!

pstibrany commented Feb 20, 2020

Uh oh!

gouthamve commented Mar 4, 2020

Uh oh!

Uh oh!

pstibrany commented Feb 12, 2020 •

edited

Loading

pstibrany commented Feb 13, 2020 •

edited

Loading

pstibrany Feb 14, 2020 •

edited

Loading