From eeb827665ac3d943412817277f6dee412b66875f Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Wed, 6 May 2026 18:42:35 +0200 Subject: [PATCH] Fix ifSpeed/ifHighSpeed for >1Gbps interfaces Two related bugs left interfaces faster than 1Gbps reporting wrong values to SNMP collectors: 1. ifSpeed (.5) was silently dropped for interfaces whose link speed exceeded 2.5Gbps. RFC 2863 requires ifSpeed to always be present and saturated at uint32 max (4294967295) when the real speed exceeds what fits in a Gauge32; ifHighSpeed carries the actual Mbps value. The row is now always emitted, capped at math.MaxUint32. 2. UpdateStats only rebuilt the MIB when the interface set changed. On startup, the stats routine connects to VPP and runs the first poll before the 1-second event-monitor poll has had a chance to call InitializeEventWatching, so the MIB is built from an empty interfaceDetails map and every row defaults to 1Gbps for life. Fix: filterValidInterfaces (which already calls GetAllInterfaceDetails to filter deleted interfaces from the stats segment) now also pushes those details through the MIB callback via a new InterfaceManager.NotifyDetails method. UpdateInterface- Details sets a staticFieldsDirty flag whenever Speed/MAC/MTU/ admin/oper/name actually change, and UpdateStats forces a rebuild on the next poll while that flag is set. Identical re-pushes do not dirty the cache, so AgentX session re-registration only happens on real change. Adds tests for the ifSpeed branches (0/nil/1G/10G/25G/100G) and for the dirty-flag transitions (fresh push, identical re-push, changed Speed). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/ifmib/ifmib.go | 59 +++++++++++++++++++------ src/ifmib/ifmib_test.go | 96 +++++++++++++++++++++++++++++++++++++++++ src/vpp/vpp_iface.go | 12 ++++++ src/vpp/vpp_stats.go | 7 +++ 4 files changed, 161 insertions(+), 13 deletions(-) diff --git a/src/ifmib/ifmib.go b/src/ifmib/ifmib.go index e3c5c46..19899df 100644 --- a/src/ifmib/ifmib.go +++ b/src/ifmib/ifmib.go @@ -3,7 +3,9 @@ package ifmib import ( + "bytes" "fmt" + "math" "os" "sync" "time" @@ -44,6 +46,14 @@ type InterfaceMIB struct { descriptions map[string]string interfaceDetails map[uint32]*vpp.InterfaceDetails + // staticFieldsDirty signals that interfaceDetails changed since the + // last rebuild and the next UpdateStats must rebuild the MIB even + // if the interface set looks unchanged. Without this flag the + // startup race between statsRoutine and eventMonitoringRoutine can + // rebuild the MIB before any details arrive, locking every row at + // the 1Gbps default forever. + staticFieldsDirty bool + // Counter items - direct references for fast updates counterItems map[string]*agentx.ListItem } @@ -104,9 +114,29 @@ func (m *InterfaceMIB) UpdateInterfaceDetails(details []vpp.InterfaceDetails) { defer m.mutex.Unlock() for _, detail := range details { - m.interfaceDetails[uint32(detail.SwIfIndex)] = &detail + d := detail + idx := uint32(d.SwIfIndex) + existing := m.interfaceDetails[idx] + if !sameStaticFields(existing, &d) { + m.staticFieldsDirty = true + } + m.interfaceDetails[idx] = &d } - logger.Debugf("Updated interface details for %d interfaces", len(details)) + logger.Debugf("Updated interface details for %d interfaces (dirty=%t)", len(details), m.staticFieldsDirty) +} + +// sameStaticFields reports whether b carries the same MIB-relevant fields +// as a; nil a/b counts as different so a fresh detail always dirties. +func sameStaticFields(a, b *vpp.InterfaceDetails) bool { + if a == nil || b == nil { + return false + } + return a.Speed == b.Speed && + a.AdminStatus == b.AdminStatus && + a.OperStatus == b.OperStatus && + a.MTU == b.MTU && + a.InterfaceName == b.InterfaceName && + bytes.Equal(a.MacAddress, b.MacAddress) } func (m *InterfaceMIB) UpdateStats(interfaceStats *api.InterfaceStats) { @@ -120,7 +150,7 @@ func (m *InterfaceMIB) UpdateStats(interfaceStats *api.InterfaceStats) { } // Simple comparison - rebuild if different - needsRebuild := len(newInterfaces) != len(m.currentInterfaces) + needsRebuild := m.staticFieldsDirty || len(newInterfaces) != len(m.currentInterfaces) if !needsRebuild { for idx := range newInterfaces { if !m.currentInterfaces[idx] { @@ -131,12 +161,13 @@ func (m *InterfaceMIB) UpdateStats(interfaceStats *api.InterfaceStats) { } if needsRebuild { - logger.Debugf("Interface set changed, rebuilding MIB") + logger.Debugf("Rebuilding MIB (set changed or details dirty)") if err := m.rebuildMIB(interfaceStats.Interfaces); err != nil { logger.Printf("Failed to rebuild MIB: %v", err) return } m.currentInterfaces = newInterfaces + m.staticFieldsDirty = false } else { // Fast path: just update counters logger.Debugf("Updating counters for %d interfaces", len(interfaceStats.Interfaces)) @@ -246,16 +277,18 @@ func (m *InterfaceMIB) addStaticFields(handler *agentx.ListHandler, iface *api.I item.Type = pdu.VariableTypeInteger item.Value = mtu - // ifSpeed (.5) - Only for speeds <= 2.5Gbps - if details != nil && details.Speed > 0 && details.Speed <= 2500000000 { - item = handler.Add(fmt.Sprintf("%s.5.%d", ifEntryOID, idx)) - item.Type = pdu.VariableTypeGauge32 - item.Value = uint32(details.Speed) - } else if details == nil || details.Speed == 0 { - item = handler.Add(fmt.Sprintf("%s.5.%d", ifEntryOID, idx)) - item.Type = pdu.VariableTypeGauge32 - item.Value = uint32(1000000000) + // ifSpeed (.5) - RFC 2863: cap at max uint32; ifHighSpeed carries the real value + ifSpeedVal := uint32(1000000000) + if details != nil && details.Speed > 0 { + if details.Speed >= math.MaxUint32 { + ifSpeedVal = math.MaxUint32 + } else { + ifSpeedVal = uint32(details.Speed) + } } + item = handler.Add(fmt.Sprintf("%s.5.%d", ifEntryOID, idx)) + item.Type = pdu.VariableTypeGauge32 + item.Value = ifSpeedVal // ifPhysAddress (.6) macAddr := "" diff --git a/src/ifmib/ifmib_test.go b/src/ifmib/ifmib_test.go index 04c55de..ace6838 100644 --- a/src/ifmib/ifmib_test.go +++ b/src/ifmib/ifmib_test.go @@ -3,10 +3,16 @@ package ifmib import ( + "fmt" + "math" "os" "testing" + "github.com/posteo/go-agentx" + "github.com/posteo/go-agentx/value" "go.fd.io/govpp/api" + + "govpp-snmp-agentx/vpp" ) func TestNewInterfaceMIB(t *testing.T) { @@ -149,6 +155,96 @@ func TestLoadVPPConfigInvalidYAML(t *testing.T) { } } +func TestAddStaticFieldsIfSpeed(t *testing.T) { + cases := []struct { + name string + details *vpp.InterfaceDetails + wantSpeed uint32 + wantHighSpeedMbps uint32 + }{ + {"unknown speed defaults to 1Gbps", &vpp.InterfaceDetails{Speed: 0}, 1000000000, 1000}, + {"nil details defaults to 1Gbps", nil, 1000000000, 1000}, + {"1Gbps reported verbatim", &vpp.InterfaceDetails{Speed: 1000000000}, 1000000000, 1000}, + {"10Gbps caps ifSpeed at uint32 max", &vpp.InterfaceDetails{Speed: 10000000000}, math.MaxUint32, 10000}, + {"25Gbps caps ifSpeed at uint32 max", &vpp.InterfaceDetails{Speed: 25000000000}, math.MaxUint32, 25000}, + {"100Gbps caps ifSpeed at uint32 max", &vpp.InterfaceDetails{Speed: 100000000000}, math.MaxUint32, 100000}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mib := NewInterfaceMIB() + handler := &agentx.ListHandler{} + iface := &api.InterfaceCounters{InterfaceIndex: 0, InterfaceName: "test0"} + idx := 1000 + + mib.addStaticFields(handler, iface, idx, tc.details) + + gotSpeed, ok := lookupGauge32(t, handler, fmt.Sprintf("%s.5.%d", ifEntryOID, idx)) + if !ok { + t.Fatalf("ifSpeed row was not registered for idx %d", idx) + } + if gotSpeed != tc.wantSpeed { + t.Errorf("ifSpeed = %d, want %d", gotSpeed, tc.wantSpeed) + } + + gotHigh, ok := lookupGauge32(t, handler, fmt.Sprintf("%s.15.%d", ifXTableOID, idx)) + if !ok { + t.Fatalf("ifHighSpeed row was not registered for idx %d", idx) + } + if gotHigh != tc.wantHighSpeedMbps { + t.Errorf("ifHighSpeed = %d, want %d", gotHigh, tc.wantHighSpeedMbps) + } + }) + } +} + +func lookupGauge32(t *testing.T, h *agentx.ListHandler, oid string) (uint32, bool) { + t.Helper() + parsed, err := value.ParseOID(oid) + if err != nil { + t.Fatalf("parse OID %q: %v", oid, err) + } + gotOID, _, val, err := h.Get(nil, parsed) + if err != nil || gotOID == nil { + return 0, false + } + v, ok := val.(uint32) + if !ok { + t.Fatalf("OID %s value type %T, want uint32", oid, val) + } + return v, true +} + +func TestUpdateInterfaceDetailsMarksDirty(t *testing.T) { + mib := NewInterfaceMIB() + + if mib.staticFieldsDirty { + t.Fatal("freshly constructed MIB should not be dirty") + } + + // First push: previously-unseen interface dirties the cache. + mib.UpdateInterfaceDetails([]vpp.InterfaceDetails{{SwIfIndex: 1, Speed: 25000000000}}) + if !mib.staticFieldsDirty { + t.Fatal("first push of details should dirty the static-field cache") + } + + // Simulate the rebuild that UpdateStats would do. + mib.staticFieldsDirty = false + + // Second push with identical fields: must not re-dirty (avoids + // thrashing the AgentX session reregister on every stats poll). + mib.UpdateInterfaceDetails([]vpp.InterfaceDetails{{SwIfIndex: 1, Speed: 25000000000}}) + if mib.staticFieldsDirty { + t.Fatal("identical re-push must not dirty the static-field cache") + } + + // Third push with a changed field: must dirty. + mib.UpdateInterfaceDetails([]vpp.InterfaceDetails{{SwIfIndex: 1, Speed: 10000000000}}) + if !mib.staticFieldsDirty { + t.Fatal("changed Speed must dirty the static-field cache") + } +} + func TestUpdateStatsBasic(t *testing.T) { mib := NewInterfaceMIB() diff --git a/src/vpp/vpp_iface.go b/src/vpp/vpp_iface.go index 2bc5b16..dac8cef 100644 --- a/src/vpp/vpp_iface.go +++ b/src/vpp/vpp_iface.go @@ -47,6 +47,18 @@ func (im *InterfaceManager) SetEventCallback(callback InterfaceEventCallback) { im.eventCallback = callback } +// NotifyDetails forwards an externally fetched interface list through +// the configured event callback. The stats path uses this so the MIB's +// static-field cache stays current on every poll, not just on +// asynchronous VPP interface events — without it the first stats poll +// can rebuild the MIB before InitializeEventWatching has populated any +// details, leaving every row stuck at the 1Gbps default until restart. +func (im *InterfaceManager) NotifyDetails(details []InterfaceDetails) { + if im.eventCallback != nil { + im.eventCallback(details) + } +} + // InitializeEventWatching starts event watching and retrieves initial interface details func (im *InterfaceManager) InitializeEventWatching() error { if !im.client.IsConnected() { diff --git a/src/vpp/vpp_stats.go b/src/vpp/vpp_stats.go index 83788bb..d82b6c3 100644 --- a/src/vpp/vpp_stats.go +++ b/src/vpp/vpp_stats.go @@ -187,6 +187,13 @@ func (sm *StatsManager) filterValidInterfaces(stats *api.InterfaceStats) (*api.I return stats, err } + // Push the freshly fetched details through the event callback so the + // MIB's static-field cache (Speed, MAC, MTU, ...) is in sync with the + // stats it's about to receive. Otherwise the very first UpdateStats + // races eventMonitoringRoutine and can rebuild from an empty details + // map. + sm.interfaceManager.NotifyDetails(currentInterfaces) + // Create a map of valid sw_if_index values validInterfaces := make(map[uint32]bool) for _, iface := range currentInterfaces {