From 48e6a60b5a2557beb3d3ea13702582cebbeed9fc Mon Sep 17 00:00:00 2001 From: Yilin <76769345+xiaozhiche320@users.noreply.github.com> Date: Thu, 29 Aug 2024 18:57:49 +0000 Subject: [PATCH] 1.Change the caller level from warn to debug when delete the qdisc 2.Implement a dynamic temporary cache that tracks network interfaces that do not support the ethtool operation. 3.Add new test cases for the new changings --- pkg/plugin/linuxutil/ethtool_handle_linux.go | 50 +++++++++++ pkg/plugin/linuxutil/ethtool_stats_linux.go | 15 +++- .../linuxutil/ethtool_stats_linux_test.go | 88 +++++++++++++++++-- pkg/plugin/linuxutil/linuxutil_linux.go | 3 + pkg/plugin/linuxutil/types_linux.go | 3 + pkg/plugin/packetparser/packetparser_linux.go | 4 +- 6 files changed, 149 insertions(+), 14 deletions(-) create mode 100644 pkg/plugin/linuxutil/ethtool_handle_linux.go diff --git a/pkg/plugin/linuxutil/ethtool_handle_linux.go b/pkg/plugin/linuxutil/ethtool_handle_linux.go new file mode 100644 index 0000000000..a2e454d2c2 --- /dev/null +++ b/pkg/plugin/linuxutil/ethtool_handle_linux.go @@ -0,0 +1,50 @@ +package linuxutil + +import ( + "strings" + + "github.com/pkg/errors" + + lru "github.com/hashicorp/golang-lru/v2" + + "github.com/microsoft/retina/pkg/log" + "go.uber.org/zap" +) + +type CachedEthtool struct { + EthtoolInterface + unsupported *lru.Cache[string, 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)) + } + + return &CachedEthtool{ + EthtoolInterface: ethHandle, + unsupported: cache, + l: log.Logger().Named(string("EthtoolReader")), + } +} + +var errskip = errors.New("skip interface") + +func (ce *CachedEthtool) Stats(intf string) (map[string]uint64, error) { + // Skip unsupported interfaces + if _, ok := ce.unsupported.Get(intf); ok { + return nil, errskip + } + + ifaceStats, err := ce.EthtoolInterface.Stats(intf) + if err != nil { + if strings.Contains(err.Error(), "operation not supported") { + ce.unsupported.Add(intf, struct{}{}) + return nil, errors.Wrap(err, "interface not supported while retrieving stats") + } + return nil, errors.Wrap(err, "failed to retrieve interface stats") + } + return ifaceStats, nil +} diff --git a/pkg/plugin/linuxutil/ethtool_stats_linux.go b/pkg/plugin/linuxutil/ethtool_stats_linux.go index 32c42ea2fc..882bcc48e4 100644 --- a/pkg/plugin/linuxutil/ethtool_stats_linux.go +++ b/pkg/plugin/linuxutil/ethtool_stats_linux.go @@ -3,6 +3,7 @@ package linuxutil import ( + "errors" "net" "strings" @@ -28,11 +29,13 @@ func NewEthtoolReader(opts *EthtoolOpts, ethHandle EthtoolInterface) *EthtoolRea return nil } } + // Construct a cached ethtool handle + CachedEthHandle := NewCachedEthtool(ethHandle, opts) return &EthtoolReader{ l: log.Logger().Named(string("EthtoolReader")), opts: opts, data: &EthtoolStats{}, - ethHandle: ethHandle, + ethHandle: CachedEthHandle, } } @@ -48,8 +51,6 @@ func (er *EthtoolReader) readAndUpdate() error { } func (er *EthtoolReader) readInterfaceStats() error { - // ethtool section - ifaces, err := net.Interfaces() if err != nil { er.l.Error("Error while getting all interfaces: %v\n", zap.Error(err)) @@ -67,10 +68,15 @@ func (er *EthtoolReader) readInterfaceStats() error { if strings.Contains(i.Name, "lo") || strings.Contains(i.Name, "cbr0") { continue } + // Retrieve tx from eth0 ifaceStats, err := er.ethHandle.Stats(i.Name) if err != nil { - er.l.Error("Error while getting ethtool:", zap.String("ifacename", i.Name), zap.Error(err)) + if errors.Is(err, errskip) { + er.l.Debug("Skipping unsupported interface", zap.String("ifacename", i.Name)) + } else { + er.l.Error("Error while getting ethtool:", zap.String("ifacename", i.Name), zap.Error(err)) + } continue } @@ -79,6 +85,7 @@ func (er *EthtoolReader) readInterfaceStats() error { er.data.stats[i.Name] = tempMap er.l.Debug("Processed ethtool Stats ", zap.String("ifacename", i.Name)) + } return nil diff --git a/pkg/plugin/linuxutil/ethtool_stats_linux_test.go b/pkg/plugin/linuxutil/ethtool_stats_linux_test.go index 15278e6159..97ea262929 100644 --- a/pkg/plugin/linuxutil/ethtool_stats_linux_test.go +++ b/pkg/plugin/linuxutil/ethtool_stats_linux_test.go @@ -1,8 +1,11 @@ package linuxutil import ( + "errors" "testing" + lru "github.com/hashicorp/golang-lru/v2" + "github.com/microsoft/retina/pkg/log" "github.com/microsoft/retina/pkg/metrics" "github.com/prometheus/client_golang/prometheus" @@ -15,12 +18,18 @@ var ( MockCounterVec *metrics.MockICounterVec ) +var ( + errInterfaceNotSupported = errors.New("operation not supported") + errOther = errors.New("other error") +) + func TestNewEthtool(t *testing.T) { log.SetupZapLogger(log.GetDefaultLogOpts()) opts := &EthtoolOpts{ errOrDropKeysOnly: false, addZeroVal: false, + limit: 10, } ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -36,6 +45,7 @@ func TestNewEthtoolWithNil(t *testing.T) { opts := &EthtoolOpts{ errOrDropKeysOnly: false, addZeroVal: false, + limit: 10, } ethReader := NewEthtoolReader(opts, nil) @@ -43,6 +53,11 @@ func TestNewEthtoolWithNil(t *testing.T) { } func TestReadInterfaceStats(t *testing.T) { + globalCache, err := lru.New[string, struct{}](10) + if err != nil { + t.Fatal("failed to create LRU cache: ", err) + } + log.SetupZapLogger(log.GetDefaultLogOpts()) l := log.Logger().Named("ethtool test").Sugar() @@ -59,6 +74,7 @@ func TestReadInterfaceStats(t *testing.T) { opts: &EthtoolOpts{ errOrDropKeysOnly: false, addZeroVal: false, + limit: 10, }, statsReturn: map[string]uint64{ "rx_packets": 1, @@ -69,32 +85,88 @@ func TestReadInterfaceStats(t *testing.T) { }, wantErr: false, }, + { + name: "test other error not added to cache", + opts: &EthtoolOpts{ + errOrDropKeysOnly: false, + addZeroVal: false, + limit: 10, + }, + statsReturn: nil, + statErr: errOther, + result: nil, + wantErr: true, + }, + { + name: "test unsported interface", + opts: &EthtoolOpts{ + errOrDropKeysOnly: false, + addZeroVal: false, + limit: 10, + }, + statsReturn: nil, + statErr: errInterfaceNotSupported, + + result: nil, + wantErr: false, + }, + { + name: "test skipped interface", + opts: &EthtoolOpts{ + errOrDropKeysOnly: false, + addZeroVal: false, + limit: 10, + }, + statsReturn: nil, + statErr: errInterfaceNotSupported, + result: nil, + wantErr: false, + }, } for _, tt := range tests { l.Infof("Running TestReadInterfaceStats %s", tt.name) + ctrl := gomock.NewController(t) defer ctrl.Finish() ethHandle := NewMockEthtoolInterface(ctrl) - ethReader := NewEthtoolReader(tt.opts, ethHandle) + + cachedEthHandle := NewCachedEthtool(ethHandle, tt.opts) + cachedEthHandle.unsupported = globalCache + + ethReader := NewEthtoolReader(tt.opts, cachedEthHandle) + assert.NotNil(t, ethReader) - ethHandle.EXPECT().Stats(gomock.Any()).Return(tt.statsReturn, nil).AnyTimes() + ethHandle.EXPECT().Stats(gomock.Any()).Return(tt.statsReturn, tt.statErr).AnyTimes() ethHandle.EXPECT().Close().Times(1) InitalizeMetricsForTesting(ctrl) - testmetric := prometheus.NewGauge(prometheus.GaugeOpts{ - Name: "testmetric", - Help: "testmetric", - }) + if tt.statErr == nil { + testmetric := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "testmetric", + Help: "testmetric", + }) - MockGaugeVec.EXPECT().WithLabelValues(gomock.Any()).Return(testmetric).AnyTimes() + MockGaugeVec.EXPECT().WithLabelValues(gomock.Any()).Return(testmetric).AnyTimes() + } err := ethReader.readInterfaceStats() assert.Nil(t, err) - ethReader.updateMetrics() + if tt.statErr == nil { + ethReader.updateMetrics() + } + + 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") + } 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") + } + + globalCache = cachedEthHandle.unsupported } } diff --git a/pkg/plugin/linuxutil/linuxutil_linux.go b/pkg/plugin/linuxutil/linuxutil_linux.go index ec89eb254d..aedccfb7b7 100644 --- a/pkg/plugin/linuxutil/linuxutil_linux.go +++ b/pkg/plugin/linuxutil/linuxutil_linux.go @@ -18,6 +18,8 @@ import ( "go.uber.org/zap" ) +const defaultLimit = 2000 + // New creates a linuxutil plugin. func New(cfg *kcfg.Config) api.Plugin { return &linuxUtil{ @@ -87,6 +89,7 @@ func (lu *linuxUtil) run(ctx context.Context) error { ethtoolOpts := &EthtoolOpts{ errOrDropKeysOnly: false, addZeroVal: false, + limit: defaultLimit, } ethHandle, err := ethtool.NewEthtool() diff --git a/pkg/plugin/linuxutil/types_linux.go b/pkg/plugin/linuxutil/types_linux.go index 08f1af74e6..c0467360e4 100644 --- a/pkg/plugin/linuxutil/types_linux.go +++ b/pkg/plugin/linuxutil/types_linux.go @@ -102,6 +102,9 @@ 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 { diff --git a/pkg/plugin/packetparser/packetparser_linux.go b/pkg/plugin/packetparser/packetparser_linux.go index 3e0cb8da9e..4813962ba5 100644 --- a/pkg/plugin/packetparser/packetparser_linux.go +++ b/pkg/plugin/packetparser/packetparser_linux.go @@ -318,10 +318,10 @@ func (p *packetParser) clean(tcnl ITc, tcIngressObj *tc.Object, tcEgressObj *tc. // Warning, not error. Clean is best effort. if tcnl != nil { if err := getQdisc(tcnl).Delete(tcEgressObj); err != nil && !errors.Is(err, tc.ErrNoArg) { - p.l.Warn("could not delete egress qdisc", zap.Error(err)) + p.l.Debug("could not delete egress qdisc", zap.Error(err)) } if err := getQdisc(tcnl).Delete(tcIngressObj); err != nil && !errors.Is(err, tc.ErrNoArg) { - p.l.Warn("could not delete ingress qdisc", zap.Error(err)) + p.l.Debug("could not delete ingress qdisc", zap.Error(err)) } if err := tcnl.Close(); err != nil { p.l.Warn("could not close rtnetlink socket", zap.Error(err))