-
Notifications
You must be signed in to change notification settings - Fork 818
Flush mode for ingester #1747
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
Flush mode for ingester #1747
Conversation
1fc5f35
to
a5af8ea
Compare
a5af8ea
to
d1c3880
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's early to review but I took a look and made a couple of comments.
cb5956b
to
fc24cce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to refactor this code so the shutdown is initiated by the ingester module?
That way the main.go should not need any changes, and the IsIngesterJob
function could be removed?
pkg/ingester/ingester.go
Outdated
if cfg.FlushMode { | ||
// Flush and don't update the ring. | ||
i.Flush() | ||
return i, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we initiate the shutdown here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been staring at this for a while and I might need some pointers on how cortex can be shut down within ingester. I can only see signals as an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, let me see. Can we pass t.Stop
to the ingester when initiating and then just call that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try that! Should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Causes an initialization loop :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think it's not safe to stop all modules while we are in the process of initing all the modules (additionally, the server is started after all modules are inited).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried another modification of my earlier attempt - the modules propagate the info of shutting down via a bool instead of cortex explicitly knowing about the ingester job.
cmd/cortex/main.go
Outdated
|
||
runtime.KeepAlive(ballast) | ||
err = t.Stop() | ||
|
||
if t.IsIngesterJob() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to refactor this logic into the ingester module?
One blocker that I faced was that the server was started by the cortex package, and the |
I have a clarification to make here: The flushing happens even before the server starts. But would the metrics be served via the default registry even before the server starts? If not, then the code needs to be refactored so that the server starts before flushing. |
Yea we are going to need the server to be running if we want metrics from this job. |
pkg/ingester/ingester.go
Outdated
if cfg.FlushMode { | ||
// Flush and don't update the ring. | ||
i.Flush() | ||
return i, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, let me see. Can we pass t.Stop
to the ingester when initiating and then just call that?
feac537
to
7c362db
Compare
I am curious how can we scrape the metrics here even if we have the server running. Reason: The flushing happens in |
I tested this with #1103 and seems to be working fine. |
This is my attempt to simplify things compared to cortexproject#1747 /cc @jtlisi @codesome Signed-off-by: Goutham Veeramachaneni <[email protected]>
@codesome Ideally we use something like a push gateway for something like this. However, that may take a significant bit of refactoring. Otherwise we could add a wait period before shutting down. |
Signed-off-by: Ganesh Vernekar <[email protected]>
d5efd1d
to
212ebd3
Compare
I have now rebased this PR on top of #1885 with a small fix |
Signed-off-by: Ganesh Vernekar <[email protected]>
Tested it. Worked fine. |
Sorry to come up with another suggestion after so many other discussions, but it seemed cleaner to me: how about doing it from a different |
This is my attempt to simplify things compared to cortexproject#1747 /cc @jtlisi @codesome Signed-off-by: Goutham Veeramachaneni <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the most ideal but should do the job, @codesome could you test it again?
Superseded by #2075 |
This is as per this design. Ingester would be run in flush mode in case someone forgets to hit the shutdown endpoint as described in #1746 before scaling down the ingesters with WAL.
Still in draft mode, as I have to check about the ingester quitting gracefully and not just stopping the operations.