logging: Attempt to recover logmon failures #5577
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
endocrimes
force-pushed
the
dani/b-logmon-unrecoverable
branch
from
7d24952
to
21c3970
Compare
Currently, when logmon fails to reattach, we will retry reattachment to the same pid until the task restart specification is exhausted. Because we cannot clear hook state during error conditions, it is not possible for us to signal to a future restart that it _shouldn't_ attempt to reattach to the plugin. Here we revert to explicitly detecting reattachment seperately from a launch of a new logmon, so we can recover from scenarios where a logmon plugin has failed. This is a net improvement over the current hard failure situation, as it means in the most common case (the pid has gone away), we can recover. Other reattachment failure modes where the plugin may still be running could potentially cause a duplicate process, or a subsequent failure to launch a new plugin. If there was a duplicate process, it could potentially cause duplicate logging. This is better than a production workload outage. If there was a subsequent failure to launch a new plugin, it would fail in the same (retry until restarts are exhausted) as the current failure mode.
endocrimes
force-pushed
the
dani/b-logmon-unrecoverable
branch
from
21c3970
to
269e2c0
Compare
schmichael
changed the title
schmichael
approved these changes
Apr 18, 2019
nickethier
reviewed
Apr 19, 2019
| // We did not reattach to a plugin and one is still not running. | ||
| if h.logmonPluginClient == nil || h.logmonPluginClient.Exited() { | ||
| if err := h.launchLogMon(nil); err != nil { | ||
| // Retry errors launching logmon as logmon may have crashed on start and |
There was a problem hiding this comment.
Ugh I wrote this comment and I can't make out what it means. Could you rewrite this into something intelligible. We want to convey that subsequent attempts to launch logmon will likely error at this point so a recoverable error is returned so the task can reattempt to start.
Co-Authored-By: notnoop <mahmood@notnoop.com>
endocrimes
added a commit
that referenced
this pull request
Apr 23, 2019
This was referenced Apr 25, 2019
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when logmon fails to reattach, we will retry reattachment to
the same pid until the task restart specification is exhausted.
Because we cannot clear hook state during error conditions, it is not
possible for us to signal to a future restart that it shouldn't
attempt to reattach to the plugin.
Here we revert to explicitly detecting reattachment seperately from a
launch of a new logmon, so we can recover from scenarios where a logmon
plugin has failed.
This is a net improvement over the current hard failure situation, as it
means in the most common case (the pid has gone away), we can recover.
Other reattachment failure modes where the plugin may still be running
could potentially cause a duplicate process, or a subsequent failure to launch
a new plugin.
If there was a duplicate process, it could potentially cause duplicate
logging. This is better than a production workload outage.
If there was a subsequent failure to launch a new plugin, it would fail
in the same (retry until restarts are exhausted) as the current failure
mode.
A future improvement would be to wrap logmon reattachment failures and
attempt to ensure that the process has been destroyed, however this
would require care in pid-rotation heavy environments and on windows.