From 18960a5dad6a64010f620eb0f70285e97cbff57c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Wed, 12 Aug 2026 12:30:36 +0000 Subject: [PATCH 1/3] adapter/statsclient: refresh symlink entries in UpdateDir updateStatOnIndex skips an entry whose directory type no longer matches the type recorded at PrepareDir. For a symlink those never match: the directory type stays Symlink while entry.Type is the resolved type of the counter it aliases. So every symlink in a prepared dir was silently left at its PrepareDir value, and a PrepareDir-once + UpdateDir-per-tick loop over, say, "/interfaces" reported the same numbers forever. Re-resolve symlinks through CopyEntryData instead. That allocates, where the non-symlink path updates in place, because a resolved item has no stable backing slice to write into - noted in a comment so callers refreshing large numbers of symlinks know to expect it. Adds a synthetic v2 stats segment to test against, laid out as VPP lays out the real one, so the refresh can be shown to pick up a changed backing counter without needing a running VPP to generate traffic. Co-Authored-By: Claude Opus 5 (1M context) --- adapter/statsclient/statsclient.go | 21 +- adapter/statsclient/statseg_v2_fake_test.go | 267 ++++++++++++++++++++ 2 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 adapter/statsclient/statseg_v2_fake_test.go diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index 90a59587..a5331ccc 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -1,4 +1,5 @@ // Copyright (c) 2019 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -629,10 +630,22 @@ func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVec return fmt.Errorf("stat entry index %d out of dir vector length (%d)", entry.Index, dirLen) } dirPtr, dirName, dirType := sc.GetStatDirOnIndex(vector, entry.Index) - if len(dirName) == 0 || - !bytes.Equal(dirName, entry.Name) || - dirType != entry.Type || - entry.Data == nil { + // Identity is the name; if it no longer matches, the directory changed under us + // (the epoch check in UpdateDir normally catches this first). + if len(dirName) == 0 || !bytes.Equal(dirName, entry.Name) || entry.Data == nil { + return nil + } + if dirType == adapter.Symlink { + // A symlink's directory entry holds (target, item) indexes rather than a data + // pointer, so its resolved Type never equals dirType and the type check below + // would skip it, leaving the entry frozen at its PrepareDir value forever. + // Re-resolve through the symlink instead. This allocates, unlike the in-place + // UpdateEntryData path, because the resolved item does not have a stable + // backing slice to write into. + entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0)) + return nil + } + if dirType != entry.Type { return nil } if err := sc.UpdateEntryData(dirPtr, &entry.Data); err != nil { diff --git a/adapter/statsclient/statseg_v2_fake_test.go b/adapter/statsclient/statseg_v2_fake_test.go new file mode 100644 index 00000000..dd0cca8d --- /dev/null +++ b/adapter/statsclient/statseg_v2_fake_test.go @@ -0,0 +1,267 @@ +// Copyright (c) 2026 Meter, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package statsclient + +import ( + "sync/atomic" + "testing" + "unsafe" + + "go.fd.io/govpp/adapter" +) + +// A synthetic v2 stats segment, laid out exactly as VPP lays out the real one, so +// the client's unsafe pointer walking can be exercised without a running VPP. It +// mirrors the shape VPP uses for error counters - one vector, plus a symlink naming +// each of its items: +// +// index 0: /sys/fake-scalar filler, see fakeTargetIndex +// index 1: /node/errors simple counter vector, one thread +// index 2+: /err/fake-node/ symlinks, one per item of that vector +// +// Pointers stored inside the segment are VPP-side addresses (fakeBase + offset), +// which is what adjust() expects to translate back into the mapped region. +const fakeBase = uint64(0x7f0000000000) + +// v2 stat segment directory types, per dirTypeMapping. +const ( + fakeTypeScalarIndex = 1 + fakeTypeSimpleCounterVector = 2 + fakeTypeSymlink = 6 +) + +// fakeTargetIndex is the directory index of /node/errors. It is deliberately not 0: +// CopyEntryData treats a directory entry whose union data is zero as having no data, +// so a symlink to item 0 of directory index 0 is unrepresentable. Real VPP never +// lands there either, but a fake that did would fail for that reason alone. +const fakeTargetIndex = 1 + +// shared header field offsets, per sharedHeaderV2. +const ( + fakeOffVersion = 0 + fakeOffBase = 8 + fakeOffEpoch = 16 + fakeOffInProgress = 24 + fakeOffDirVector = 32 + fakeOffErrorVector = 40 +) + +type fakeSegment struct { + buf []byte + // counters is the offset of the backing counter data for thread 0. + counters int +} + +// newFakeSegment builds a segment holding a /node/errors vector with the given +// counter values, plus one /err/fake-node/rN symlink per value, in reverse order so +// that a symlink's own directory index is never its item index (which would let an +// off-by-one confusion pass unnoticed). +func newFakeSegment(t *testing.T, values []uint64) *fakeSegment { + t.Helper() + + const ( + hdrSize = 64 // sharedHeaderV2 rounded up + vecHdr = 8 // vector length precedes the data + ptrSize = 8 + threads = 1 + trailer = 8 // adjust() rejects pointers to the very last byte + dirEntLen = int(unsafe.Sizeof(statSegDirectoryEntryV2{})) + ) + nDir := fakeTargetIndex + 1 + len(values) // filler + /node/errors + one symlink per value + + dirLenOff := hdrSize + dirOff := dirLenOff + vecHdr + ptLenOff := dirOff + nDir*dirEntLen // per-thread vector of pointers + ptOff := ptLenOff + vecHdr + ctLenOff := ptOff + threads*ptrSize // thread 0 counter vector + ctOff := ctLenOff + vecHdr + total := ctOff + len(values)*ptrSize + trailer + + f := &fakeSegment{buf: make([]byte, total), counters: ctOff} + + // Shared header. errorVector stays zero: adjust() then rejects it, which is how + // the client decides a segment uses the modern (non-legacy) type mapping. + f.putU64(fakeOffVersion, 2) + f.putU64(fakeOffBase, fakeBase) + f.putU64(fakeOffEpoch, 1) + f.putU64(fakeOffInProgress, 0) + f.putU64(fakeOffDirVector, fakeBase+uint64(dirOff)) + f.putU64(fakeOffErrorVector, 0) + + // Vector lengths. + f.putU64(dirLenOff, uint64(nDir)) + f.putU64(ptLenOff, threads) + f.putU64(ctLenOff, uint64(len(values))) + + // Counter data, and the per-thread vector pointing at it. + f.putU64(ptOff, fakeBase+uint64(ctOff)) + for i, v := range values { + f.putU64(ctOff+i*ptrSize, v) + } + + // Directory entry 0: filler, so the backing vector is not at index 0. + f.putDirEntry(dirOff, 0, fakeTypeScalarIndex, 7, "/sys/fake-scalar") + + // Directory entry 1: the backing vector. + f.putDirEntry(dirOff, fakeTargetIndex, fakeTypeSimpleCounterVector, fakeBase+uint64(ptOff), "/node/errors") + + // The remaining entries are symlinks into it, named in reverse item order. + for i := range values { + item := uint32(len(values) - 1 - i) + union := uint64(fakeTargetIndex) | uint64(item)<<32 + f.putDirEntry(dirOff, fakeTargetIndex+1+i, fakeTypeSymlink, union, fakeErrName(item)) + } + return f +} + +func fakeErrName(item uint32) string { + return "/err/fake-node/r" + string(rune('a'+item)) +} + +// fakeErrItem is the inverse of fakeErrName, so a test can tell which counter a +// symlink entry should be showing without relying on the API under test. +func fakeErrItem(t *testing.T, name []byte) uint32 { + t.Helper() + for item := uint32(0); item < 32; item++ { + if fakeErrName(item) == string(name) { + return item + } + } + t.Fatalf("%s is not a fake symlink name", name) + return 0 +} + +func (f *fakeSegment) putU64(off int, v uint64) { + *(*uint64)(unsafe.Pointer(&f.buf[off])) = v +} + +func (f *fakeSegment) putDirEntry(dirOff, index int, typ dirType, union uint64, name string) { + e := (*statSegDirectoryEntryV2)(unsafe.Pointer(&f.buf[dirOff+index*int(unsafe.Sizeof(statSegDirectoryEntryV2{}))])) + e.directoryType = typ + e.unionData = union + copy(e.name[:], name) + e.name[len(name)] = 0 +} + +// setCounter changes a backing counter value, as VPP would between two reads. +func (f *fakeSegment) setCounter(item int, v uint64) { + f.putU64(f.counters+item*8, v) +} + +// bumpEpoch simulates a directory re-layout. +func (f *fakeSegment) bumpEpoch() { + f.putU64(fakeOffEpoch, *(*uint64)(unsafe.Pointer(&f.buf[fakeOffEpoch]))+1) +} + +// client returns a StatsClient reading this segment, without a socket. +func (f *fakeSegment) client() *StatsClient { + sc := &StatsClient{statSegment: newStatSegmentV2(f.buf, int64(len(f.buf)))} + atomic.StoreUint32(&sc.connected, 1) + return sc +} + +// symlinkValue returns the single counter value an entry resolved through a symlink +// carries. +func symlinkValue(t *testing.T, e adapter.StatEntry) uint64 { + t.Helper() + s, ok := e.Data.(adapter.SimpleCounterStat) + if !ok { + t.Fatalf("%s: expected SimpleCounterStat, got %T", e.Name, e.Data) + } + if len(s) != 1 || len(s[0]) != 1 { + t.Fatalf("%s: expected a single resolved item, got %v", e.Name, s) + } + return uint64(s[0][0]) +} + +// UpdateDir must re-resolve symlink entries. Before the fix the type check in +// updateStatOnIndex skipped them - a symlink's directory type never equals the +// resolved type of its data - so a prepared dir kept returning its PrepareDir values. +func TestUpdateDirRefreshesSymlinks(t *testing.T) { + values := []uint64{1, 2, 3} + f := newFakeSegment(t, values) + sc := f.client() + + dir, err := sc.PrepareDir() + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + + // Change every backing counter, exactly as VPP would while counting. + updated := []uint64{100, 200, 300} + for i, v := range updated { + f.setCounter(i, v) + } + + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + var seen int + for i := range dir.Entries { + e := dir.Entries[i] + if !e.Symlink { + continue + } + seen++ + item := fakeErrItem(t, e.Name) + if got, want := symlinkValue(t, e), updated[item]; got != want { + t.Errorf("%s: value after UpdateDir = %d, want %d (stale value was %d)", + e.Name, got, want, values[item]) + } + } + if seen != len(values) { + t.Fatalf("expected %d symlink entries in the prepared dir, got %d", len(values), seen) + } +} + +// The non-symlink path must keep working, in place, as before. +func TestUpdateDirRefreshesCounterVector(t *testing.T) { + f := newFakeSegment(t, []uint64{1, 2, 3}) + sc := f.client() + + dir, err := sc.PrepareDir("^/node/errors$") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + if len(dir.Entries) != 1 { + t.Fatalf("expected one entry, got %d", len(dir.Entries)) + } + f.setCounter(1, 42) + if err := sc.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + s, ok := dir.Entries[0].Data.(adapter.SimpleCounterStat) + if !ok { + t.Fatalf("expected SimpleCounterStat, got %T", dir.Entries[0].Data) + } + if got := uint64(s[0][1]); got != 42 { + t.Errorf("counter after UpdateDir = %d, want 42", got) + } +} + +func TestUpdateDirStaleEpoch(t *testing.T) { + f := newFakeSegment(t, []uint64{1, 2, 3}) + sc := f.client() + + dir, err := sc.PrepareDir() + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + f.bumpEpoch() + if err := sc.UpdateDir(dir); err != adapter.ErrStatsDirStale { + t.Fatalf("UpdateDir after epoch change = %v, want %v", err, adapter.ErrStatsDirStale) + } +} From e1718f5295fa589d43a3e56939429fe2aa6d5547 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Wed, 12 Aug 2026 12:30:59 +0000 Subject: [PATCH 2/3] adapter/statsclient: check the prepared dir against the epoch access starts on UpdateDir read the epoch once for the staleness check and then let accessStart read it again. If the directory is re-laid-out between the two reads, the staleness check passes against the old epoch while the entries are resolved against the new directory - and accessEnd then confirms that same new epoch, so nothing catches it and the caller gets values read against a directory its entry indexes no longer describe. Drop the separate read and compare dir.Epoch against the epoch accessStart settled on, which is the one accessEnd validates. Also return an error when the directory vector is nil, rather than the nil named return, which reported success. Co-Authored-By: Claude Opus 5 (1M context) --- adapter/statsclient/statsclient.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index a5331ccc..93dd8e20 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -283,18 +283,21 @@ func (sc *StatsClient) UpdateDir(dir *adapter.StatDir) (err error) { return adapter.ErrStatsDisconnected } - epoch, _ := sc.GetEpoch() - if dir.Epoch != epoch { - return adapter.ErrStatsDirStale - } - + // Compare the prepared dir against the epoch accessStart settled on, not against + // a separately read one: with two reads the directory can be re-laid-out in + // between, in which case the staleness check passes against the old epoch while + // the entries are resolved against the new directory - and accessEnd then + // confirms that same new epoch, so nothing catches it. accessEpoch := sc.accessStart() if accessEpoch == 0 { return adapter.ErrStatsAccessFailed } + if dir.Epoch != accessEpoch { + return adapter.ErrStatsDirStale + } dirVector := sc.GetDirectoryVector() if dirVector == nil { - return err + return fmt.Errorf("failed to update dir: directory vector is nil") } for i := 0; i < len(dir.Entries); i++ { if err := sc.updateStatOnIndex(&dir.Entries[i], dirVector); err != nil { From d2dc5cf2094d87a9f0bdd87be2ac759e37a044a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Wed, 12 Aug 2026 12:32:05 +0000 Subject: [PATCH 3/3] adapter: add ListSymlinks to map a counter vector's items to names VPP exposes some counters only as one vector plus a set of symlinks naming its items. /node/errors is the case that hurts: it is a single counter vector, and every /err// is a symlink into one item of it. Reading the vector once is far cheaper than resolving thousands of symlinks - but then the item names are recoverable only from the symlinks, and an item's position is not derivable from anything a caller can compute, because vlib_register_errors allocates it from a heap and reuses freed holes. VPP exports /sys/node/names, but nothing equivalent for error reasons, and StatEntry.Index for a symlink is its own directory index, not the item it aliases. ListSymlinks reports that mapping: for each symlink, the entry it aliases (index and name) and the item within it. Like ListStats it walks names and indexes only and copies no counter data, so it is cheap enough to rebuild whenever the epoch changes - which is the only time the mapping can change. The (target, item) pair stays off StatEntry: a caller wanting labels does not want the per-symlink data copy that DumpStats does, and a caller wanting data has no use for the indexes. The internal statSegment accessor reports ok=false for non-symlink entries and for v1, which has no symlink target encoding, so neither can be mistaken for a symlink with target 0 item 0. Co-Authored-By: Claude Opus 5 (1M context) --- adapter/mock/mock_stats_adapter.go | 6 + adapter/stats_api.go | 26 ++++ adapter/statsclient/stat_segment_api.go | 7 + adapter/statsclient/statsclient.go | 82 ++++++++++- adapter/statsclient/statseg_v1.go | 7 + adapter/statsclient/statseg_v2.go | 12 ++ adapter/statsclient/statseg_v2_fake_test.go | 82 +++++++++++ test/integration/stats_test.go | 143 ++++++++++++++++++++ 8 files changed, 364 insertions(+), 1 deletion(-) diff --git a/adapter/mock/mock_stats_adapter.go b/adapter/mock/mock_stats_adapter.go index f2378f37..97f956e2 100644 --- a/adapter/mock/mock_stats_adapter.go +++ b/adapter/mock/mock_stats_adapter.go @@ -57,6 +57,12 @@ func (a *StatsAdapter) ListStats(patterns ...string) ([]adapter.StatIdentifier, return statNames, nil } +// ListSymlinks mocks symlink listing. The mock holds no symlink metadata, so it +// reports none. +func (a *StatsAdapter) ListSymlinks(patterns ...string) ([]adapter.SymlinkEntry, error) { + return nil, nil +} + // DumpStats mocks all stat entries dump. func (a *StatsAdapter) DumpStats(patterns ...string) ([]adapter.StatEntry, error) { return a.entries, nil diff --git a/adapter/stats_api.go b/adapter/stats_api.go index a8549974..2f8cd4a1 100644 --- a/adapter/stats_api.go +++ b/adapter/stats_api.go @@ -1,4 +1,5 @@ // Copyright (c) 2019 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -41,6 +42,9 @@ type StatsAPI interface { // ListStats lists indexed names for stats matching patterns. ListStats(patterns ...string) (indexes []StatIdentifier, err error) + // ListSymlinks lists symlink entries matching patterns, resolved to the + // entry and item each of them aliases. + ListSymlinks(patterns ...string) (symlinks []SymlinkEntry, err error) // DumpStats dumps all stat entries. DumpStats(patterns ...string) (entries []StatEntry, err error) @@ -91,6 +95,28 @@ type StatEntry struct { Symlink bool } +// SymlinkEntry describes a symlink directory entry and the counter it aliases. +// +// VPP exposes some counters only as one vector plus a set of symlinks naming its +// items: /node/errors is a single counter vector, and every /err// +// is a symlink to one item of it. Reading the vector once is far cheaper than +// resolving thousands of symlinks, but then the item names are recoverable only +// from the symlinks, and an item's position is not derivable from anything else +// (vlib_register_errors allocates it from a heap, and reuses freed holes). +// +// ListSymlinks reports that mapping, so a caller reading a vector directly can +// label its items. +type SymlinkEntry struct { + // StatIdentifier holds the name and directory index of the symlink itself. + StatIdentifier + // TargetIndex is the directory index of the aliased entry, in the same index + // space as StatIdentifier.Index, and TargetName is its name. + TargetIndex uint32 + TargetName []byte + // ItemIndex is the position within the aliased vector that the symlink names. + ItemIndex uint32 +} + // Counter represents simple counter with single value, which is usually packet count. type Counter uint64 diff --git a/adapter/statsclient/stat_segment_api.go b/adapter/statsclient/stat_segment_api.go index af7ca71b..680c8b3c 100644 --- a/adapter/statsclient/stat_segment_api.go +++ b/adapter/statsclient/stat_segment_api.go @@ -1,4 +1,5 @@ // Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -93,6 +94,12 @@ type statSegment interface { // Use ^uint32(0) as an empty index (since 0 is a valid value). CopyEntryData(segment dirSegment, index uint32) adapter.Stat + // GetSymlinkIndexes returns, for a symlink directory segment, the directory + // index of the entry it aliases and the item index within that entry. + // ok is false if the segment is not a symlink, or if the segment version has + // no notion of symlinks. + GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32, ok bool) + // UpdateEntryData accepts pointer to a directory segment with data, and stat // segment to update UpdateEntryData(segment dirSegment, s *adapter.Stat) error diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index 93dd8e20..84c062e9 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -187,6 +187,36 @@ func (sc *StatsClient) ListStats(patterns ...string) (entries []adapter.StatIden return entries, nil } +// ListSymlinks lists the symlinks among the entries matching patterns, resolved to +// the entry and item each aliases. Entries that are not symlinks are skipped, so +// patterns may be as broad as convenient. +// +// Like ListStats it reads names and indexes only - no counter data is copied - which +// makes it cheap enough to rebuild the mapping on every epoch change. See +// adapter.SymlinkEntry for what the mapping is good for. +func (sc *StatsClient) ListSymlinks(patterns ...string) (symlinks []adapter.SymlinkEntry, err error) { + sc.accessLock.RLock() + defer sc.accessLock.RUnlock() + + if !sc.isConnected() { + return nil, adapter.ErrStatsDisconnected + } + accessEpoch := sc.accessStart() + if accessEpoch == 0 { + return nil, adapter.ErrStatsAccessFailed + } + + symlinks, err = sc.getSymlinkEntries(patterns...) + if err != nil { + return nil, err + } + + if !sc.accessEnd(accessEpoch) { + return nil, adapter.ErrStatsDataBusy + } + return symlinks, nil +} + func (sc *StatsClient) DumpStats(patterns ...string) (entries []adapter.StatEntry, err error) { sc.accessLock.RLock() defer sc.accessLock.RUnlock() @@ -576,6 +606,54 @@ func (sc *StatsClient) getIdentifierEntriesOnIndex(vector dirVector, indexes ... return identifiers, nil } +// getSymlinkEntries retrieves the symlinks among the entries matching desired +// patterns, or among all entries if no pattern is provided. +func (sc *StatsClient) getSymlinkEntries(patterns ...string) (symlinks []adapter.SymlinkEntry, err error) { + vector := sc.GetDirectoryVector() + if vector == nil { + return nil, fmt.Errorf("failed to get symlink entries: directory vector is nil") + } + indexes, err := sc.listIndexes(vector, patterns...) + if err != nil { + return nil, err + } + return sc.getSymlinkEntriesOnIndex(vector, indexes...) +} + +// getSymlinkEntriesOnIndex resolves the symlinks among indexes to the entry and item +// each of them aliases. Indexes that are not symlinks are skipped. +func (sc *StatsClient) getSymlinkEntriesOnIndex(vector dirVector, indexes ...uint32) (symlinks []adapter.SymlinkEntry, err error) { + dirLen := *(*uint32)(vectorLen(vector)) + for _, index := range indexes { + if index >= dirLen { + return nil, fmt.Errorf("stat entry index %d out of dir vector length (%d)", index, dirLen) + } + dirPtr, dirName, _ := sc.GetStatDirOnIndex(vector, index) + if len(dirName) == 0 { + continue + } + targetIndex, itemIndex, ok := sc.GetSymlinkIndexes(dirPtr) + if !ok { + continue + } + if targetIndex >= dirLen { + debugf("symlink %s aliases out of range index %d", dirName, targetIndex) + continue + } + _, targetName, _ := sc.GetStatDirOnIndex(vector, targetIndex) + symlinks = append(symlinks, adapter.SymlinkEntry{ + StatIdentifier: adapter.StatIdentifier{ + Index: index, + Name: dirName, + }, + TargetIndex: targetIndex, + TargetName: targetName, + ItemIndex: itemIndex, + }) + } + return symlinks, nil +} + // listIndexes lists indexes for all stat entries that match any of the regex patterns. func (sc *StatsClient) listIndexes(vector dirVector, patterns ...string) (indexes []uint32, err error) { if len(patterns) == 0 { @@ -644,7 +722,9 @@ func (sc *StatsClient) updateStatOnIndex(entry *adapter.StatEntry, vector dirVec // would skip it, leaving the entry frozen at its PrepareDir value forever. // Re-resolve through the symlink instead. This allocates, unlike the in-place // UpdateEntryData path, because the resolved item does not have a stable - // backing slice to write into. + // backing slice to write into; callers refreshing large numbers of symlinks + // on a tick are better served reading the backing vector and mapping it with + // ListSymlinks. entry.Data = sc.CopyEntryData(dirPtr, ^uint32(0)) return nil } diff --git a/adapter/statsclient/statseg_v1.go b/adapter/statsclient/statseg_v1.go index 134104b3..b5d3f7ac 100644 --- a/adapter/statsclient/statseg_v1.go +++ b/adapter/statsclient/statseg_v1.go @@ -1,4 +1,5 @@ // Copyright (c) 2019 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -92,6 +93,12 @@ func (ss *statSegmentV1) GetEpoch() (int64, bool) { return sh.epoch, sh.inProgress != 0 } +// GetSymlinkIndexes is unsupported for stats segment v1, which does not encode +// symlink target indexes. +func (ss *statSegmentV1) GetSymlinkIndexes(dirSegment) (uint32, uint32, bool) { + return 0, 0, false +} + func (ss *statSegmentV1) CopyEntryData(segment dirSegment, _ uint32) adapter.Stat { dirEntry := (*statSegDirectoryEntryV1)(segment) typ := getStatType(dirEntry.directoryType, true) diff --git a/adapter/statsclient/statseg_v2.go b/adapter/statsclient/statseg_v2.go index 01bd5f70..8f8b16ca 100644 --- a/adapter/statsclient/statseg_v2.go +++ b/adapter/statsclient/statseg_v2.go @@ -1,4 +1,5 @@ // Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -580,6 +581,17 @@ func (ss *statSegmentV2) getErrorVector() dirVector { return ss.adjust(dirVector(&header.errorVector)) } +// GetSymlinkIndexes returns the target directory index and item index encoded in a +// symlink directory segment's union data, or ok false if the segment is not a symlink. +func (ss *statSegmentV2) GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32, ok bool) { + dirEntry := (*statSegDirectoryEntryV2)(segment) + if getStatType(dirEntry.directoryType, ss.getErrorVector() != nil) != adapter.Symlink { + return 0, 0, false + } + targetIndex, itemIndex = ss.getSymlinkIndexes(dirEntry) + return targetIndex, itemIndex, true +} + func (ss *statSegmentV2) getSymlinkIndexes(dirEntry *statSegDirectoryEntryV2) (index1, index2 uint32) { var b bytes.Buffer if err := binary.Write(&b, binary.LittleEndian, dirEntry.unionData); err != nil { diff --git a/adapter/statsclient/statseg_v2_fake_test.go b/adapter/statsclient/statseg_v2_fake_test.go index dd0cca8d..1de38a01 100644 --- a/adapter/statsclient/statseg_v2_fake_test.go +++ b/adapter/statsclient/statseg_v2_fake_test.go @@ -186,6 +186,64 @@ func symlinkValue(t *testing.T, e adapter.StatEntry) uint64 { return uint64(s[0][0]) } +func TestListSymlinks(t *testing.T) { + values := []uint64{10, 20, 30} + f := newFakeSegment(t, values) + sc := f.client() + + symlinks, err := sc.ListSymlinks() + if err != nil { + t.Fatal("ListSymlinks failed:", err) + } + if len(symlinks) != len(values) { + t.Fatalf("expected %d symlinks, got %d", len(values), len(symlinks)) + } + for _, s := range symlinks { + if got, want := string(s.TargetName), "/node/errors"; got != want { + t.Errorf("%s: target name = %q, want %q", s.Name, got, want) + } + if s.TargetIndex != fakeTargetIndex { + t.Errorf("%s: target index = %d, want %d", s.Name, s.TargetIndex, fakeTargetIndex) + } + // The item index is the whole point: it must name the counter this symlink + // aliases, independent of the symlink's own directory index. + if got, want := string(s.Name), fakeErrName(s.ItemIndex); got != want { + t.Errorf("item index %d resolved to %q, want %q", s.ItemIndex, want, got) + } + } +} + +// The mapping ListSymlinks reports must agree with what resolving the symlink +// individually yields - otherwise a caller reading the backing vector directly and +// labelling it from ListSymlinks would mislabel every item. +func TestListSymlinksAgreesWithResolvedValues(t *testing.T) { + values := []uint64{11, 22, 33, 44} + f := newFakeSegment(t, values) + sc := f.client() + + symlinks, err := sc.ListSymlinks() + if err != nil { + t.Fatal("ListSymlinks failed:", err) + } + entries, err := sc.DumpStats("^/err/") + if err != nil { + t.Fatal("DumpStats failed:", err) + } + byName := make(map[string]adapter.StatEntry, len(entries)) + for _, e := range entries { + byName[string(e.Name)] = e + } + for _, s := range symlinks { + e, ok := byName[string(s.Name)] + if !ok { + t.Fatalf("%s: not returned by DumpStats", s.Name) + } + if got, want := symlinkValue(t, e), values[s.ItemIndex]; got != want { + t.Errorf("%s: resolved value %d, but item index %d holds %d", s.Name, got, s.ItemIndex, want) + } + } +} + // UpdateDir must re-resolve symlink entries. Before the fix the type check in // updateStatOnIndex skipped them - a symlink's directory type never equals the // resolved type of its data - so a prepared dir kept returning its PrepareDir values. @@ -265,3 +323,27 @@ func TestUpdateDirStaleEpoch(t *testing.T) { t.Fatalf("UpdateDir after epoch change = %v, want %v", err, adapter.ErrStatsDirStale) } } + +// v1 has no symlink target encoding, so nothing must be reported for it. +func TestGetSymlinkIndexesV1(t *testing.T) { + ss := &statSegmentV1{} + if _, _, ok := ss.GetSymlinkIndexes(nil); ok { + t.Error("statSegmentV1 reported symlink indexes") + } +} + +// A non-symlink segment must not be reported as one, even though its union data +// would decode into a plausible-looking pair of indexes. +func TestGetSymlinkIndexesNonSymlink(t *testing.T) { + f := newFakeSegment(t, []uint64{1}) + ss := newStatSegmentV2(f.buf, int64(len(f.buf))) + + vector := ss.GetDirectoryVector() + segment, name, _ := ss.GetStatDirOnIndex(vector, fakeTargetIndex) + if string(name) != "/node/errors" { + t.Fatalf("index %d is %q, want /node/errors", fakeTargetIndex, name) + } + if _, _, ok := ss.GetSymlinkIndexes(segment); ok { + t.Error("counter vector entry reported as a symlink") + } +} diff --git a/test/integration/stats_test.go b/test/integration/stats_test.go index ffa19157..55ed0976 100644 --- a/test/integration/stats_test.go +++ b/test/integration/stats_test.go @@ -1,4 +1,5 @@ // Copyright (c) 2022 Cisco and/or its affiliates. +// Copyright (c) 2026 Meter, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -17,6 +18,8 @@ package integration import ( "testing" + "go.fd.io/govpp/adapter" + "go.fd.io/govpp/adapter/statsclient" "go.fd.io/govpp/api" "go.fd.io/govpp/test/vpptesting" ) @@ -97,3 +100,143 @@ func TestStatClientNodeStatsAgain(t *testing.T) { t.Fatal("getting node stats failed:", err) } } + +// TestStatClientSymlinks exercises ListSymlinks against a live VPP: that the +// reported (target, item) mapping is self-consistent, and that it actually agrees +// with what resolving each symlink individually yields - which is what makes it safe +// to read a backing vector once and label its items from the mapping. +// +// /err// is the case that motivates the API: every one of them is a +// symlink into a single /node/errors vector, and an item's position there comes from +// a heap allocation in vlib_register_errors, so it is not derivable from anything a +// caller can compute. +// +// The unit tests in adapter/statsclient cover the pointer walking and the UpdateDir +// symlink refresh deterministically, against a synthetic segment; this test is about +// agreeing with a real VPP's directory layout. +func TestStatClientSymlinks(t *testing.T) { + test := vpptesting.SetupVPP(t) + + // create an interface so the directory carries per-interface symlink entries + // (/interfaces/* aliasing into /if/*) alongside the /err/* ones + test.MustCli("create loopback interface", "set interface state loop0 up") + + client := statsclient.NewStatsClient("") + if err := client.Connect(); err != nil { + t.Fatal("connecting stats client failed:", err) + } + defer func() { _ = client.Disconnect() }() + + symlinks, err := client.ListSymlinks() + if err != nil { + t.Fatal("ListSymlinks failed:", err) + } + if len(symlinks) == 0 { + t.Fatal("expected at least one symlink entry in the stats directory") + } + + all, err := client.DumpStats() + if err != nil { + t.Fatal("DumpStats failed:", err) + } + byIndex := make(map[uint32]adapter.StatEntry, len(all)) + for _, e := range all { + byIndex[e.Index] = e + } + + var errSymlinks int + for _, s := range symlinks { + target, ok := byIndex[s.TargetIndex] + if !ok { + t.Fatalf("%s: target index %d not present in directory", s.Name, s.TargetIndex) + } + if string(target.Name) != string(s.TargetName) { + t.Fatalf("%s: target index %d is %q, but ListSymlinks reported %q", + s.Name, s.TargetIndex, target.Name, s.TargetName) + } + // A symlink must alias a real counter, never another symlink. + if target.Symlink { + t.Fatalf("%s: target %q is itself a symlink", s.Name, target.Name) + } + + // The value the mapping points at must equal the value obtained by resolving + // the symlink itself. This is the property a caller relies on when it reads + // the backing vector directly and labels its items from ListSymlinks. + resolved, ok := byIndex[s.Index] + if !ok { + t.Fatalf("%s: symlink index %d not present in directory", s.Name, s.Index) + } + want, ok := itemValue(t, target, s.ItemIndex) + if !ok { + continue // target type carries no per-item counters to compare + } + got, ok := itemValue(t, resolved, 0) + if !ok { + t.Fatalf("%s: resolved to unexpected type %T", s.Name, resolved.Data) + } + if got != want { + t.Fatalf("%s: resolves to %d, but %s item %d holds %d", + s.Name, got, target.Name, s.ItemIndex, want) + } + + if string(target.Name) == "/node/errors" { + errSymlinks++ + } + } + if errSymlinks == 0 { + t.Fatal("expected /err/* symlinks aliasing /node/errors") + } + t.Logf("validated %d symlinks (%d of them error counters)", len(symlinks), errSymlinks) +} + +// itemValue returns the item at index of a counter vector entry, summed over threads, +// and whether the entry has such items at all. +func itemValue(t *testing.T, e adapter.StatEntry, index uint32) (uint64, bool) { + t.Helper() + switch d := e.Data.(type) { + case adapter.SimpleCounterStat: + if len(d) == 0 || int(index) >= len(d[0]) { + return 0, false + } + return adapter.ReduceSimpleCounterStatIndex(d, int(index)), true + case adapter.CombinedCounterStat: + if len(d) == 0 || int(index) >= len(d[0]) { + return 0, false + } + return adapter.CombinedCounter(adapter.ReduceCombinedCounterStatIndex(d, int(index))).Packets(), true + } + return 0, false +} + +// TestStatClientUpdateDirStaleEpoch checks that a dir prepared under one directory +// layout is rejected once the layout changes, and can be re-prepared afterwards. +func TestStatClientUpdateDirStaleEpoch(t *testing.T) { + test := vpptesting.SetupVPP(t) + + test.MustCli("create loopback interface", "set interface state loop0 up") + + client := statsclient.NewStatsClient("") + if err := client.Connect(); err != nil { + t.Fatal("connecting stats client failed:", err) + } + defer func() { _ = client.Disconnect() }() + + dir, err := client.PrepareDir("/if", "/interfaces") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + // Under an unchanged layout UpdateDir must succeed. + if err := client.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + + // Adding an interface changes the directory layout, which bumps the epoch. + test.MustCli("create loopback interface") + + if err := client.UpdateDir(dir); err != adapter.ErrStatsDirStale { + t.Fatalf("UpdateDir after layout change = %v, want %v", err, adapter.ErrStatsDirStale) + } + if _, err := client.PrepareDir("/if", "/interfaces"); err != nil { + t.Fatal("re-PrepareDir after layout change failed:", err) + } +}