From 531e9fed11eab6db3b29fd06791f40434f098629 Mon Sep 17 00:00:00 2001 From: otroan Date: Mon, 8 Jun 2026 11:44:11 +0000 Subject: [PATCH 1/3] adapter/statsclient: refresh symlinks + expose epoch and symlink targets Three changes to support per-tick stats collection without re-resolving the whole directory each epoch: 1. UpdateDir now re-resolves symlink entries (updateStatOnIndex via CopyEntryData) so a PrepareDir-once + UpdateDir-per-tick loop keeps symlink data fresh. 2. StatEntry gains SymlinkTarget/SymlinkItem, populated from the v2 segment (GetSymlinkIndexes on the internal statSegment interface; v1 returns 0,0). 3. (*StatsClient).Epoch() exposes the current directory epoch. Adds statseg_symlink_test.go covering symlink refresh and target metadata. Co-Authored-By: Claude Opus 4.8 --- adapter/stats_api.go | 7 +++ adapter/statsclient/stat_segment_api.go | 5 ++ adapter/statsclient/statsclient.go | 42 ++++++++++++++--- adapter/statsclient/statseg_symlink_test.go | 52 +++++++++++++++++++++ adapter/statsclient/statseg_v1.go | 5 ++ adapter/statsclient/statseg_v2.go | 6 +++ 6 files changed, 111 insertions(+), 6 deletions(-) create mode 100644 adapter/statsclient/statseg_symlink_test.go diff --git a/adapter/stats_api.go b/adapter/stats_api.go index a8549974..c325b441 100644 --- a/adapter/stats_api.go +++ b/adapter/stats_api.go @@ -89,6 +89,13 @@ type StatEntry struct { Type StatType Data Stat Symlink bool + // SymlinkTarget is the directory index of the entry a symlink points to, and + // SymlinkItem the item (column) index within that target. They are only + // meaningful when Symlink is true (stats segment v2; zero otherwise). They let a + // caller read the backing vector directly and map its columns, instead of + // resolving every symlink individually. + SymlinkTarget uint32 + SymlinkItem uint32 } // Counter represents simple counter with single value, which is usually packet count. diff --git a/adapter/statsclient/stat_segment_api.go b/adapter/statsclient/stat_segment_api.go index af7ca71b..d417571e 100644 --- a/adapter/statsclient/stat_segment_api.go +++ b/adapter/statsclient/stat_segment_api.go @@ -93,6 +93,11 @@ 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 points to and the item (column) index within that target. + // Only meaningful for symlink segments; non-symlink segments / v1 return (0, 0). + GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32) + // 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 90a59587..15879085 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -306,6 +306,20 @@ func (sc *StatsClient) UpdateDir(dir *adapter.StatDir) (err error) { return nil } +// Epoch returns the current stats-segment epoch and whether an update is in progress. +// The epoch changes whenever the directory layout changes (counters added/removed), so +// a StatDir prepared under a different epoch is stale and must be re-prepared. Lets a +// caller pre-check staleness instead of relying on an UpdateDir error. +func (sc *StatsClient) Epoch() (epoch int64, inProgress bool, err error) { + sc.accessLock.RLock() + defer sc.accessLock.RUnlock() + if !sc.isConnected() { + return 0, false, adapter.ErrStatsDisconnected + } + epoch, inProgress = sc.GetEpoch() + return epoch, inProgress, nil +} + // checks the socket existence and waits for it for the designated // time if it is not available immediately func (sc *StatsClient) waitForSocket() error { @@ -540,7 +554,7 @@ func (sc *StatsClient) getStatEntriesOnIndex(vector dirVector, indexes ...uint32 if d != nil { t = d.Type() } - entries = append(entries, adapter.StatEntry{ + entry := adapter.StatEntry{ StatIdentifier: adapter.StatIdentifier{ Index: index, Name: dirName, @@ -548,7 +562,13 @@ func (sc *StatsClient) getStatEntriesOnIndex(vector dirVector, indexes ...uint32 Type: t, Data: d, Symlink: dirType == adapter.Symlink, - }) + } + if entry.Symlink { + // expose the backing (target, item) so callers can read the target + // vector directly instead of resolving each symlink every refresh. + entry.SymlinkTarget, entry.SymlinkItem = sc.GetSymlinkIndexes(dirPtr) + } + entries = append(entries, entry) } return entries, nil } @@ -629,10 +649,20 @@ 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, not data: its + // resolved Type never equals dirType, and UpdateEntryData would misread the + // union as a data pointer. Re-resolve through the symlink instead, which + // reads the current value of the backing counter. + 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_symlink_test.go b/adapter/statsclient/statseg_symlink_test.go new file mode 100644 index 00000000..1c14df86 --- /dev/null +++ b/adapter/statsclient/statseg_symlink_test.go @@ -0,0 +1,52 @@ +// Copyright (c) 2026 Cisco and/or its affiliates. +// +// 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 ( + "unsafe" + + . "github.com/onsi/gomega" + + "testing" +) + +// A v2 symlink directory entry packs (targetIndex, itemIndex) into its 8-byte union: +// the low 32 bits are the target directory index, the high 32 bits the item index. +func TestGetSymlinkIndexesV2(t *testing.T) { + RegisterTestingT(t) + + const targetIndex, itemIndex = uint32(0x1234), uint32(0x5678) + entry := statSegDirectoryEntryV2{ + directoryType: 6, // Symlink + unionData: uint64(targetIndex) | uint64(itemIndex)<<32, + } + + ss := &statSegmentV2{} + gotTarget, gotItem := ss.GetSymlinkIndexes(dirSegment(unsafe.Pointer(&entry))) + + Expect(gotTarget).To(Equal(targetIndex)) + Expect(gotItem).To(Equal(itemIndex)) +} + +// v1 has no symlinks; the accessor is a no-op returning zeroes. +func TestGetSymlinkIndexesV1(t *testing.T) { + RegisterTestingT(t) + + ss := &statSegmentV1{} + target, item := ss.GetSymlinkIndexes(nil) + + Expect(target).To(BeEquivalentTo(0)) + Expect(item).To(BeEquivalentTo(0)) +} diff --git a/adapter/statsclient/statseg_v1.go b/adapter/statsclient/statseg_v1.go index 134104b3..3dfea906 100644 --- a/adapter/statsclient/statseg_v1.go +++ b/adapter/statsclient/statseg_v1.go @@ -92,6 +92,11 @@ func (ss *statSegmentV1) GetEpoch() (int64, bool) { return sh.epoch, sh.inProgress != 0 } +// GetSymlinkIndexes is a no-op for stats segment v1, which has no symlinks. +func (ss *statSegmentV1) GetSymlinkIndexes(dirSegment) (uint32, uint32) { + return 0, 0 +} + 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..1111885b 100644 --- a/adapter/statsclient/statseg_v2.go +++ b/adapter/statsclient/statseg_v2.go @@ -580,6 +580,12 @@ 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. +func (ss *statSegmentV2) GetSymlinkIndexes(segment dirSegment) (targetIndex, itemIndex uint32) { + return ss.getSymlinkIndexes((*statSegDirectoryEntryV2)(segment)) +} + func (ss *statSegmentV2) getSymlinkIndexes(dirEntry *statSegDirectoryEntryV2) (index1, index2 uint32) { var b bytes.Buffer if err := binary.Write(&b, binary.LittleEndian, dirEntry.unionData); err != nil { From 40c8f0e2dd5f1fe7b60ea977e7e8d941e8585335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Tue, 9 Jun 2026 14:05:21 +0000 Subject: [PATCH 2/3] test/integration: cover symlink targets, Epoch, and UpdateDir refresh Add TestStatClientSymlinkRefresh to the live-VPP integration suite, exercising the adapter changes end-to-end against a running VPP: - StatEntry.SymlinkTarget/SymlinkItem are populated and each symlink resolves to a real (non-symlink) backing directory entry. - StatsClient.Epoch() returns the current epoch and reflects a directory-layout change (creating an interface bumps it). - UpdateDir refreshes a prepared dir under an unchanged epoch (re-resolving symlink entries), and returns ErrStatsDirStale for a dir prepared under a stale epoch. A loopback is created up front so per-interface symlink entries (/interfaces/* aliasing into /if/*) are present. Complements the statseg_symlink_test.go unit test, which covers the v1/v2 (target,item) bit-unpacking directly. Co-Authored-By: Claude Opus 4.8 --- test/integration/stats_test.go | 114 +++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/test/integration/stats_test.go b/test/integration/stats_test.go index ffa19157..62b3d75c 100644 --- a/test/integration/stats_test.go +++ b/test/integration/stats_test.go @@ -17,10 +17,16 @@ 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" ) +// statsSocket is the default VPP stats segment socket, matching the path the +// vpptesting harness launches VPP with. +const statsSocket = "/run/vpp/stats.sock" + func TestStatClientAll(t *testing.T) { test := vpptesting.SetupVPP(t) @@ -97,3 +103,111 @@ func TestStatClientNodeStatsAgain(t *testing.T) { t.Fatal("getting node stats failed:", err) } } + +// TestStatClientSymlinkRefresh exercises the lower-level statsclient adapter against +// a live VPP: the symlink-target metadata on StatEntry, the Epoch() accessor, and +// that UpdateDir refreshes a prepared dir (and rejects one prepared under a stale +// epoch). A loopback is created up front so per-interface symlinks (/interfaces/* +// aliasing into /if/*) are present. +func TestStatClientSymlinkRefresh(t *testing.T) { + test := vpptesting.SetupVPP(t) + + // create an interface so the directory carries per-interface symlink entries + test.MustCli("create loopback interface", "set interface state loop0 up") + + client := statsclient.NewStatsClient(statsSocket) + if err := client.Connect(); err != nil { + t.Fatal("connecting stats client failed:", err) + } + defer client.Disconnect() + + t.Run("Epoch", func(t *testing.T) { + epoch, _, err := client.Epoch() + if err != nil { + t.Fatal("Epoch failed:", err) + } + if epoch == 0 { + t.Fatal("expected a non-zero stats epoch") + } + t.Logf("stats epoch = %d", epoch) + }) + + t.Run("SymlinkTargetsResolve", func(t *testing.T) { + // A full dump lets us resolve each symlink's target index (a directory index, + // the same space as StatIdentifier.Index) back to a real backing entry. + all, err := client.DumpStats() + if err != nil { + t.Fatal("DumpStats failed:", err) + } + nameByIndex := make(map[uint32]string, len(all)) + symlinkByIndex := make(map[uint32]bool, len(all)) + var symlinks []adapter.StatEntry + for _, e := range all { + nameByIndex[e.Index] = string(e.Name) + symlinkByIndex[e.Index] = e.Symlink + if e.Symlink { + symlinks = append(symlinks, e) + } + } + if len(symlinks) == 0 { + t.Fatal("expected at least one symlink entry in the stats directory") + } + for _, e := range symlinks { + target, ok := nameByIndex[e.SymlinkTarget] + if !ok { + t.Fatalf("%s: symlink target index %d not present in directory", e.Name, e.SymlinkTarget) + } + if symlinkByIndex[e.SymlinkTarget] { + t.Fatalf("%s: symlink target %q is itself a symlink", e.Name, target) + } + } + t.Logf("validated %d symlink entries resolve to real backing vectors", len(symlinks)) + }) + + t.Run("UpdateDirRefresh", func(t *testing.T) { + dir, err := client.PrepareDir("/if", "/interfaces") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + // Under an unchanged epoch UpdateDir must succeed and re-resolve symlink + // entries (the change under test) rather than leaving them stale. + if err := client.UpdateDir(dir); err != nil { + t.Fatal("UpdateDir failed:", err) + } + for i := range dir.Entries { + if e := &dir.Entries[i]; e.Symlink && e.Data == nil { + t.Fatalf("%s: symlink entry has nil data after UpdateDir", e.Name) + } + } + }) + + t.Run("EpochChangeInvalidatesDir", func(t *testing.T) { + dir, err := client.PrepareDir("/if", "/interfaces") + if err != nil { + t.Fatal("PrepareDir failed:", err) + } + before, _, err := client.Epoch() + if err != nil { + t.Fatal("Epoch failed:", err) + } + + // Adding an interface changes the directory layout, which bumps the epoch. + test.MustCli("create loopback interface") + + after, _, err := client.Epoch() + if err != nil { + t.Fatal("Epoch failed:", err) + } + if after == before { + t.Fatalf("expected epoch to change after adding an interface (still %d)", before) + } + // A dir prepared under the old epoch is now stale. + if err := client.UpdateDir(dir); err != adapter.ErrStatsDirStale { + t.Fatalf("expected ErrStatsDirStale after epoch change, got %v", err) + } + // Re-preparing under the new epoch works again. + if _, err := client.PrepareDir("/if", "/interfaces"); err != nil { + t.Fatal("re-PrepareDir after epoch change failed:", err) + } + }) +} From 4d34bfdb61a3cf55af9a8b0c3739ec74a94ea910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Tr=C3=B8an?= Date: Tue, 9 Jun 2026 14:11:27 +0000 Subject: [PATCH 3/3] test/integration: check Disconnect error in deferred cleanup (errcheck) Co-Authored-By: Claude Opus 4.8 --- test/integration/stats_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/stats_test.go b/test/integration/stats_test.go index 62b3d75c..e9ef5d7f 100644 --- a/test/integration/stats_test.go +++ b/test/integration/stats_test.go @@ -119,7 +119,7 @@ func TestStatClientSymlinkRefresh(t *testing.T) { if err := client.Connect(); err != nil { t.Fatal("connecting stats client failed:", err) } - defer client.Disconnect() + defer func() { _ = client.Disconnect() }() t.Run("Epoch", func(t *testing.T) { epoch, _, err := client.Epoch()