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

introduce unrecoverable error type, and logs to show Fluentd process/worker started #1359

Merged
merged 4 commits into from
Dec 14, 2016

Conversation

tagomoris
Copy link
Member

Currently, unrecoverable errors (which serverengine doesn't try to restart workers about) are only configuration errors.
But some other situation cannot be retried. For example:

  • per-worker root directory (will be introduced next) exists but it is a file
  • failed to bind an address/port in server plugin helper
  • failed to read a file from in_tail by permission error
  • or many others

We are using ConfigError for such cases, but it's nonsense in fact.

This change also adds unrecoverable cases for scripting errors while loading plugin files.
Fluentd is now restarting for such cases, but it's just wrong behavior.

…worker started

Currently, unrecoverable errors (which serverengine doesn't try to restart workers about) are only configuration errors.
But some other situation cannot be retried. For example:
* per-worker root directory (will be introduced next) exists but it is a file
* failed to bind an address/port in server plugin helper
* failed to read a file from in_tail by permission error
* or many others

We are using ConfigError for such cases, but it's nonsense in fact.

This change also adds unrecoverable cases for scripting errors while loading plugin files.
Fluentd is now restarting for such cases, but it's just wrong behavior.
@tagomoris tagomoris added enhancement Feature request or improve operations v0.14 labels Dec 8, 2016
@tagomoris tagomoris requested a review from repeatedly December 8, 2016 11:58
@tagomoris
Copy link
Member Author

Actually, the biggest improvement in this change is testing about:

  • Fluentd process started correctly
  • Fluentd process failed to start correctly

@tagomoris
Copy link
Member Author

@repeatedly could you review this change?

@tagomoris tagomoris self-assigned this Dec 8, 2016
def main_process(&block)
Process.setproctitle("worker:#{@process_name}") if @process_name

configuration_error = false
unrecoverable_error = false

begin
block.call
rescue Fluent::ConfigError
Copy link
Member

@repeatedly repeatedly Dec 9, 2016

Choose a reason for hiding this comment

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

Use Fluent::ConfigError => e and replace $! with e.

log.debug_backtrace
end
unrecoverable_error = true
rescue Fluent::UnrecoverableError
Copy link
Member

Choose a reason for hiding this comment

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

ditto

log.error_backtrace
end
unrecoverable_error = true
rescue ScriptError # LoadError, NotImplementedError, SyntaxError
Copy link
Member

Choose a reason for hiding this comment

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

ditto


sub_test_case 'configuration to load plugin which raises unrecoverable error in #start' do
test 'failed to start' do
script = "require 'fluent/plugin/input'\n"
Copy link
Member

Choose a reason for hiding this comment

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

Use heredocument is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Writing ruby code in here document breaks indentation on my environment... :P

@repeatedly
Copy link
Member

Others are LGTM and adding start test sis awesome.
BTW, test on Windows is unstable...

@tagomoris tagomoris force-pushed the introduce-unrecoverable-errors branch 18 times, most recently from fa4f46a to 1abe61a Compare December 13, 2016 09:51
@tagomoris tagomoris force-pushed the introduce-unrecoverable-errors branch from 1abe61a to af43194 Compare December 14, 2016 03:13
Ruby 2.1/2.2 executes worker process via shell (command prompt) with previous code.
Ruby 2.3 executes in different way (without shell), but we need to support 2.1/2.2.
This fix forces to execute worker processes without shell.
@tagomoris tagomoris force-pushed the introduce-unrecoverable-errors branch 5 times, most recently from c08a2c8 to cb3b3a0 Compare December 14, 2016 08:13
Especially on Windows, test code cannot find Fluentd process id from the result of spawn,
because "exec" system call cannot be used on Windows, and we got the process id of "bundle exec"
by "io.pid".
It is also useful to investigate supervisor-worker relation from supervisor pid and ppid of workers.
@tagomoris tagomoris force-pushed the introduce-unrecoverable-errors branch from 5eb053e to 17a4880 Compare December 14, 2016 08:30
@tagomoris
Copy link
Member Author

I added some fixes about executing worker processes and showing process ids in log.
@repeatedly could you review these changes again?

@repeatedly
Copy link
Member

LGTM and good fix for windows environment 👍

@tagomoris tagomoris merged commit dc23d35 into master Dec 14, 2016
@tagomoris tagomoris deleted the introduce-unrecoverable-errors branch December 14, 2016 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants