Handling plugin extraction errors #125
Comments
|
Good catch. Not sure what's the best option atm, for sure I will take a look. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
falcosecurity#125) Various sinsp logic sets m_tid_to_remove, to record the fact that a thread has been identified as ready-to-be-removed from the threadtable. And if automatic threadtable purging is configured, sinsp::next() takes care of removing these threads by calling remove_thread(), then clearing m_tid_to_remove. But remove_thread() may itself set m_tid_to_remove in certain situations. The current sinsp::next() logic loses track of this request; as a result, these threads will languish in the threadtable until the next remove_inactive_threads() interval, default value 20 minutes. This fix changes the sinsp::next() logic to recognize and handle the case where remove_thread() records a thread for removal. Signed-off-by: Joseph Pittman <[email protected]> Signed-off-by: Joseph Pittman <[email protected]>
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
/milestone 0.11.0 |
|
/milestone 0.12.0 |
|
We have had lots of plugins refactors, is this still relevant? |
|
I believe this is still relevant. @jasondellaluce to confirm. However, I think it would be best to extend this discussion to the plugin domain and all field extraction mechanisms, including those built-in sinsp. I know Jason has some thoughts in this regard, so I'm eager to hear from him. That being said, I don't think this is a top priority. However, it is still an improvement that is worth tackling. Just my 2 cents |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
/help |
|
@leogr: Please ensure the request meets the requirements listed here. If this request no longer meets these requirements, the label can be removed In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
Motivation
Currently, errors are ignored during field extraction in the plugin framework. In theory, a plugin might fail extracting a field for two main reasons:
ss_plugin_extract_field.field_presentflag is set to falseextract_fieldsexported plugin function encounters some error and returns a code different thanSS_PLUGIN_SUCCESS.In the current implementation, in both cases the filtercheck returns a NULL pointer, which is interpreted as a not-available field. This is visible here 👇🏼
libs/userspace/libsinsp/plugin.cpp
Line 630 in 83f460c
libs/userspace/libsinsp/plugin.cpp
Line 304 in 83f460c
Although this is semantically correct, the two failure paths have a quite different meaning. In the second case, the plugin returns a failure code and the framework silently ignores it to maintain a non-blocking extraction flow. This is makes error handling efforts useless for plugin developers, and generally makes it harder to debug plugins at runtime.
Feature
I propose to catch the error and make it visible somehow.
I agree that maintaining field extraction non-blocking might be a priority here, so maybe throwing an exception might not be a viable option. We can consider some weaker error propagation methods, or maybe logging to
stderr. To the bare minimum, we might log the error if a debug mode is enabled.Alternatives
Keep things as they are, and just ignore plugin failures for
extract_fields.The text was updated successfully, but these errors were encountered: