fix: fix ethtool issue when trying to skip unsupported interface (#1296)

# 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

- [x] I have read the [contributing
documentation](https://retina.sh/docs/contributing).
- [x] I signed and signed-off the commits (`git commit -S -s ...`). See
[this
documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification)
on signing commits.
- [x] I have correctly attributed the author(s) of the code.
- [x] I have tested the changes locally.
- [x] I have followed the project's style guidelines.
- [x] I have updated the documentation, if necessary.
- [x] I have added tests, if applicable.

## Screenshots (if applicable) or Testing Completed

Please add any relevant screenshots or GIFs to showcase the changes
made.

Logs before fixing the issue:
<img width="851" alt="Before"
src="https://github.com/user-attachments/assets/edae5f1e-275e-4de6-9f74-23100dc49600"
/>
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):
<img width="848" alt="After"
src="https://github.com/user-attachments/assets/ffbff4da-431b-4349-bc67-bdb4d7818fb9"
/>


## Additional Notes

Add any additional notes or context about the pull request here.

---

Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more
information on how to contribute to this project.

Author: xiaozhiche320

pkg/plugin/linuxutil/ethtool_handle_linux.go

@@ -8,7 +8,6 @@ import (
 	lru "github.com/hashicorp/golang-lru/v2"
 
 	"github.com/microsoft/retina/pkg/log"
-	"go.uber.org/zap"
 )
 
 type CachedEthtool struct {
@@ -17,15 +16,10 @@ type CachedEthtool struct {
 	l           *log.ZapLogger
 }
 
-func NewCachedEthtool(ethHandle EthtoolInterface, opts *EthtoolOpts) *CachedEthtool {
-	cache, err := lru.New[string, struct{}](int(opts.limit))
-	if err != nil {
-		log.Logger().Error("failed to create LRU cache: ", zap.Error(err))
-	}
-
+func NewCachedEthtool(ethHandle EthtoolInterface, unsupportedInterfacesCache *lru.Cache[string, struct{}]) *CachedEthtool {
 	return &CachedEthtool{
 		EthtoolInterface: ethHandle,
-		unsupported:      cache,
+		unsupported:      unsupportedInterfacesCache,
 		l:                log.Logger().Named(string("EthtoolReader")),
 	}
 }

pkg/plugin/linuxutil/ethtool_stats_linux.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"strings"
 
+	lru "github.com/hashicorp/golang-lru/v2"
 	"github.com/microsoft/retina/pkg/log"
 	"github.com/microsoft/retina/pkg/metrics"
 	"github.com/microsoft/retina/pkg/utils"
@@ -21,7 +22,7 @@ type EthtoolReader struct {
 	ethHandle EthtoolInterface
 }
 
-func NewEthtoolReader(opts *EthtoolOpts, ethHandle EthtoolInterface) *EthtoolReader {
+func NewEthtoolReader(opts *EthtoolOpts, ethHandle EthtoolInterface, unsupportedInterfacesCache *lru.Cache[string, struct{}]) *EthtoolReader {
 	if ethHandle == nil {
 		var err error
 		ethHandle, err = ethtool.NewEthtool()
@@ -31,7 +32,7 @@ func NewEthtoolReader(opts *EthtoolOpts, ethHandle EthtoolInterface) *EthtoolRea
 		}
 	}
 	// Construct a cached ethtool handle
-	CachedEthHandle := NewCachedEthtool(ethHandle, opts)
+	CachedEthHandle := NewCachedEthtool(ethHandle, unsupportedInterfacesCache)
 	return &EthtoolReader{
 		l:         log.Logger().Named(string("EthtoolReader")),
 		opts:      opts,
@@ -58,8 +59,6 @@ func (er *EthtoolReader) readInterfaceStats() error {
 		return err
 	}
 
-	defer er.ethHandle.Close()
-
 	er.data = &EthtoolStats{
 		stats: make(map[string]uint64),
 	}
@@ -78,8 +77,10 @@ func (er *EthtoolReader) readInterfaceStats() error {
 		if err != nil {
 			if errors.Is(err, errskip) {
 				er.l.Debug("Skipping unsupported interface", zap.String("ifacename", i.Name))
+			} else if strings.Contains(err.Error(), "interface not supported while retrieving stats") {
+				er.l.Warn("Unsupported interface detected:", zap.String("ifacename", i.Name), zap.Error(err))
 			} else {
-				er.l.Error("Error while getting ethtool:", zap.String("ifacename", i.Name), zap.Error(err))
+				er.l.Error("Error while retrieving stats:", zap.String("ifacename", i.Name), zap.Error(err))
 			}
 			continue
 		}

pkg/plugin/linuxutil/ethtool_stats_linux_test.go

@@ -29,13 +29,17 @@ func TestNewEthtool(t *testing.T) {
 	opts := &EthtoolOpts{
 		errOrDropKeysOnly: false,
 		addZeroVal:        false,
-		limit:             10,
 	}
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	unsupportedInterfacesCache, err := lru.New[string, struct{}](10)
+	if err != nil {
+		t.Fatal("failed to create cache:", err)
+	}
+
 	ethHandle := NewMockEthtoolInterface(ctrl)
-	ethReader := NewEthtoolReader(opts, ethHandle)
+	ethReader := NewEthtoolReader(opts, ethHandle, unsupportedInterfacesCache)
 	assert.NotNil(t, ethReader)
 }
 
@@ -45,15 +49,19 @@ func TestNewEthtoolWithNil(t *testing.T) {
 	opts := &EthtoolOpts{
 		errOrDropKeysOnly: false,
 		addZeroVal:        false,
-		limit:             10,
 	}
 
-	ethReader := NewEthtoolReader(opts, nil)
+	unsupportedInterfacesCache, err := lru.New[string, struct{}](10)
+	if err != nil {
+		t.Fatal("failed to create cache:", err)
+	}
+
+	ethReader := NewEthtoolReader(opts, nil, unsupportedInterfacesCache)
 	assert.NotNil(t, ethReader)
 }
 
 func TestReadInterfaceStats(t *testing.T) {
-	globalCache, err := lru.New[string, struct{}](10)
+	unsupportedInterfacesCache, err := lru.New[string, struct{}](10)
 	if err != nil {
 		t.Fatal("failed to create LRU cache: ", err)
 	}
@@ -74,7 +82,6 @@ func TestReadInterfaceStats(t *testing.T) {
 			opts: &EthtoolOpts{
 				errOrDropKeysOnly: false,
 				addZeroVal:        false,
-				limit:             10,
 			},
 			statsReturn: map[string]uint64{
 				"rx_packets": 1,
@@ -90,32 +97,28 @@ func TestReadInterfaceStats(t *testing.T) {
 			opts: &EthtoolOpts{
 				errOrDropKeysOnly: false,
 				addZeroVal:        false,
-				limit:             10,
 			},
 			statsReturn: nil,
 			statErr:     errOther,
 			result:      nil,
 			wantErr:     true,
 		},
 		{
-			name: "test unsported interface",
+			name: "test unsupported interface",
 			opts: &EthtoolOpts{
 				errOrDropKeysOnly: false,
 				addZeroVal:        false,
-				limit:             10,
 			},
 			statsReturn: nil,
 			statErr:     errInterfaceNotSupported,
-
-			result:  nil,
-			wantErr: false,
+			result:      nil,
+			wantErr:     false,
 		},
 		{
 			name: "test skipped interface",
 			opts: &EthtoolOpts{
 				errOrDropKeysOnly: false,
 				addZeroVal:        false,
-				limit:             10,
 			},
 			statsReturn: nil,
 			statErr:     errInterfaceNotSupported,
@@ -132,15 +135,11 @@ func TestReadInterfaceStats(t *testing.T) {
 
 		ethHandle := NewMockEthtoolInterface(ctrl)
 
-		cachedEthHandle := NewCachedEthtool(ethHandle, tt.opts)
-		cachedEthHandle.unsupported = globalCache
-
-		ethReader := NewEthtoolReader(tt.opts, cachedEthHandle)
+		ethReader := NewEthtoolReader(tt.opts, ethHandle, unsupportedInterfacesCache)
 
 		assert.NotNil(t, ethReader)
 
 		ethHandle.EXPECT().Stats(gomock.Any()).Return(tt.statsReturn, tt.statErr).AnyTimes()
-		ethHandle.EXPECT().Close().Times(1)
 		InitalizeMetricsForTesting(ctrl)
 
 		if tt.statErr == nil {
@@ -160,13 +159,11 @@ func TestReadInterfaceStats(t *testing.T) {
 		}
 
 		if tt.statErr != nil && errors.Is(tt.statErr, errInterfaceNotSupported) {
-			assert.NotNil(t, cachedEthHandle.unsupported, "cache should not be nil")
-			assert.NotEqual(t, 0, cachedEthHandle.unsupported.Len(), "cache should contain interface")
+			assert.NotNil(t, unsupportedInterfacesCache, "cache should not be nil")
+			assert.NotEqual(t, 0, unsupportedInterfacesCache.Len(), "cache should contain interface")
 		} else if tt.statErr != nil && !errors.Is(tt.statErr, errInterfaceNotSupported) {
-			assert.Equal(t, 0, cachedEthHandle.unsupported.Len(), "cache should not add interface for other errors")
+			assert.Equal(t, 0, unsupportedInterfacesCache.Len(), "cache should not add interface for other errors")
 		}
-
-		globalCache = cachedEthHandle.unsupported
 	}
 }
 

pkg/plugin/linuxutil/linuxutil_linux.go

@@ -11,15 +11,15 @@ import (
 	"time"
 
 	hubblev1 "github.com/cilium/cilium/pkg/hubble/api/v1"
+	lru "github.com/hashicorp/golang-lru/v2"
 	kcfg "github.com/microsoft/retina/pkg/config"
 	"github.com/microsoft/retina/pkg/log"
 	"github.com/microsoft/retina/pkg/plugin/registry"
+	"github.com/pkg/errors"
 	"github.com/safchain/ethtool"
 	"go.uber.org/zap"
 )
 
-const defaultLimit = 2000
-
 func init() {
 	registry.Add(name, New)
 }
@@ -61,6 +61,14 @@ func (lu *linuxUtil) SetupChannel(chan *hubblev1.Event) error {
 
 func (lu *linuxUtil) run(ctx context.Context) error {
 	lu.l.Info("Running linuxutil plugin...")
+
+	// create a LRU cache to skip unsupported interfaces
+	unsupportedInterfacesCache, err := lru.New[string, struct{}](int(defaultLimit))
+	if err != nil {
+		lu.l.Error("failed to create global LRU cache", zap.Error(err))
+		return err
+	}
+
 	ticker := time.NewTicker(lu.cfg.MetricsInterval)
 	defer ticker.Stop()
 
@@ -93,20 +101,20 @@ func (lu *linuxUtil) run(ctx context.Context) error {
 			ethtoolOpts := &EthtoolOpts{
 				errOrDropKeysOnly: true,
 				addZeroVal:        false,
-				limit:             defaultLimit,
 			}
 
 			ethHandle, err := ethtool.NewEthtool()
 			if err != nil {
 				lu.l.Error("Error while creating ethHandle: %v\n", zap.Error(err))
-				return err
+				return fmt.Errorf("failed to create ethHandle: %w", err)
 			}
 
-			ethReader := NewEthtoolReader(ethtoolOpts, ethHandle)
+			ethReader := NewEthtoolReader(ethtoolOpts, ethHandle, unsupportedInterfacesCache)
 			if ethReader == nil {
 				lu.l.Error("Error while creating ethReader")
-				return fmt.Errorf("error while creating ethReader")
+				return errors.New("error while creating ethReader")
 			}
+
 			wg.Add(1)
 			go func() {
 				defer wg.Done()
@@ -117,6 +125,7 @@ func (lu *linuxUtil) run(ctx context.Context) error {
 			}()
 
 			wg.Wait()
+			ethHandle.Close()
 		}
 	}
 }

pkg/plugin/linuxutil/types_linux.go

@@ -9,6 +9,7 @@ import (
 )
 
 const name = "linuxutil"
+const defaultLimit = 2000
 
 //go:generate go run go.uber.org/mock/[email protected] -source=types_linux.go -destination=linuxutil_mock_generated_linux.go -package=linuxutil
 type linuxUtil struct {
@@ -102,9 +103,6 @@ type EthtoolOpts struct {
 
 	// when true will include all keys with value 0
 	addZeroVal bool
-
-	// Configurable limit for unsupported interfaces cache
-	limit uint
 }
 
 type EthtoolInterface interface {