fix: fix ethtool issue when trying to skip unsupported interface #1296
Conversation
| ethHandle, err := ethtool.NewEthtool() | ||
| if err != nil { | ||
| lu.l.Error("Error while creating ethHandle: %v\n", zap.Error(err)) | ||
| return fmt.Errorf("failed to create ethHandle: %w", err) | ||
| } | ||
| defer ethHandle.Close() | ||
|
|
||
| ethReader := NewEthtoolReader(ethtoolOpts, ethHandle) | ||
| if ethReader == nil { | ||
| lu.l.Error("Error while creating ethReader") | ||
| return errors.New("error while creating ethReader") | ||
| } |
There was a problem hiding this comment.
One of the reason for keeping the ethHandle inside the loop is to make sure we are not bringing down Retina agent for transient error. Do you think the ethHandle error is permanent - as in if it fails once, it cannot succeed on retry?
If you return error here, PluginManager will treat it as a fatal error.
There was a problem hiding this comment.
Brining this out is probably ok, but I would retry few time (with exponential backoff) before reporting failure.
The ethHandle opens an Unix socket and uses it to get stats about interfaces - https://github.com/safchain/ethtool/blob/c20939d9864b2df0943b7c0a2363f8cab34072c7/ethtool.go#L1029C13-L1029C24 .
While recreating it every time incurs some overhead, keeping a long running socket open can potentially consume resources (we are holding onto a socket on host for long time). Also, long-lived sockets might encounter issues like network changes or timeouts, requiring robust error handling. You have to add code to make sure the connection is alive, otherwise we will keep seeing errors and no metrics, and would require user intervention to restart the agent container.
There was a problem hiding this comment.
I think your cache shouldn't be tied to the lifecycle of the tool, it should persist for agent's lifecycle.
| @@ -58,8 +58,6 @@ func (er *EthtoolReader) readInterfaceStats() error { | |||
| return err | |||
| } | |||
|
|
|||
| defer er.ethHandle.Close() | |||
There was a problem hiding this comment.
I don't like closing this here, thanks for refactoring this.
Description
This PR addresses an issue where the plugin repeatedly logged the same operation not supported error for interfaces on every metrics collection cycle. The root cause ethtool handle is created and closed during each interval, which invalidated any cached (LRU) information on unsupported interfaces. Now, once an interface is identified as unsupported, it won’t repeatedly log the same error.
Related Issue
If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.
#1280
Checklist
git commit -S -s ...). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
Please add any relevant screenshots or GIFs to showcase the changes made.
Logs before fixing the issue:
Logs after fixing the issue, the unsupported cache will only log once now (add debug message for test and validate behaviour of LRU, which has been removed in PR):
Additional Notes
Add any additional notes or context about the pull request here.
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.