-
Notifications
You must be signed in to change notification settings - Fork 306
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
shut down pipeline when finish register fails #1151
shut down pipeline when finish register fails #1151
Conversation
…asticsearch into stop_pipeline_if_post_register_fail # Conflicts: # lib/logstash/outputs/elasticsearch.rb # lib/logstash/plugin_mixins/elasticsearch/common.rb
It would be better to add integration test in core rather than in plugin to test pipeline shutdown. Test against Logstash 6 fails. Believe we can stop testing 6.x
|
@logger.error("Failed to bootstrap. Pipeline \"#{execution_context.pipeline_id}\" is going to shut down", | ||
{ message: e.message, exception: e.class, backtrace: e.backtrace }) |
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.
@logger.error("Failed to bootstrap. Pipeline \"#{execution_context.pipeline_id}\" is going to shut down", | |
{ message: e.message, exception: e.class, backtrace: e.backtrace }) | |
details = { message: e.message, exception: e.class } | |
details[:backtrace] = backtrace: e.backtrace if @logger.debug? | |
@logger.error("Failed to bootstrap. Pipeline \"#{execution_context.pipeline_id}\" is going to shut down", details) |
if respond_to?(:execution_context) && execution_context.respond_to?(:pipeline_id) && | ||
execution_context.respond_to?(:agent) && execution_context.agent.respond_to?(:stop_pipeline) |
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.
do we need all these checks? is it because during tests we may not have all these available?
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 see similar checking in dlq and the stop_pipeline
function is added in recent two years. pipeline_id
and agent
are not necessary
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.
Tracing back the history of execution_context.rb
agent and pipeline_id was not there at the beginning.
Considering the execution_context was there since 2017, do you think we can safely remove and only check execution_context.agent.respond_to?(:stop_pipeline)
? I can see other plugins do not check respond_to?(:execution_context)
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.
@jsvd ☝️
…asticsearch into stop_pipeline_if_post_register_fail # Conflicts: # CHANGELOG.md # logstash-output-elasticsearch.gemspec
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.
LGTM
Prior to this change, the errors in the post-register phase (finish_register) behaved differently: template manager logged failure; ILM setup loop printed error messages. Continuing the pipeline would not be meaningful.
With this commit, ConfigurationError is introduced for critical bootstrap failures, such as when the template path is not found. This change stops the pipeline when it gets ConfigurationError or 4xx status code calling template or ILM related API. For HTTP 429 too many request, finish_register retries the bootstrap