Skip to content

Commit f093369

Browse files
committed
[review-fix 1] Preserve baseline selections across windows
- Replace expired one-shot contexts during admission so the next baseline window cannot lose an identical selection before flush cleanup. - Remove the E2E package-install fallback and verify the local test service before generating traffic with the existing Python runtime. - Use explicit Dynamic Tests state and consistent empty metric tags for clearer test and telemetry semantics. Source: review feedback and full-diff self-review Validation: Bazel pathteststore tests and netpath dynamic-tests build
1 parent 25ed7aa commit f093369

4 files changed

Lines changed: 62 additions & 25 deletions

File tree

comp/networkpath/npcollector/impl/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func TestNetworkPathCollectorEnabled(t *testing.T) {
2828
}
2929
assert.True(t, config.networkPathCollectorEnabled())
3030

31-
config.dynamicTestsState = 0
31+
config.dynamicTestsState = npconfig.DynamicTestsOff
3232
assert.False(t, config.networkPathCollectorEnabled())
3333

3434
config.netflowMonitoringEnabled = true

comp/networkpath/npcollector/impl/pathteststore/pathteststore.go

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ func (f *Store) Flush() []*PathtestContext {
164164
f.statsdClient.Incr(networkPathStoreMetricPrefix+"pathtest_never_run", []string{}, 1) //nolint:errcheck
165165
}
166166
if ptConfigCtx.Pathtest.DynamicTestProfile == payload.DynamicTestProfileBaseline {
167-
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_expired", nil, 1) //nolint:errcheck
167+
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_expired", []string{}, 1) //nolint:errcheck
168168
}
169169
continue
170170
}
@@ -178,7 +178,7 @@ func (f *Store) Flush() []*PathtestContext {
178178
pathtestsToFlush = append(pathtestsToFlush, ptConfigCtx.snapshot())
179179
if ptConfigCtx.Pathtest.OneShot {
180180
if ptConfigCtx.Pathtest.DynamicTestProfile == payload.DynamicTestProfileBaseline {
181-
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_dispatched", nil, 1) //nolint:errcheck
181+
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_dispatched", []string{}, 1) //nolint:errcheck
182182
}
183183
delete(f.contexts, key)
184184
continue
@@ -205,16 +205,30 @@ func (f *Store) Add(pathtestToAdd *common.Pathtest) {
205205
if ok {
206206
if pathtestToAdd.OneShot {
207207
// A baseline slot is consumed before admission. Deduplication must not
208-
// extend or recur an already selected one-shot context.
208+
// extend or recur a live one-shot context. Replace an expired context
209+
// immediately so a flush racing with the next window cannot discard the
210+
// new selection along with the old one.
211+
now := f.timeNowFn()
212+
expired := pathtestCtx.runUntil.Before(now) || (pathtestCtx.Pathtest.OneShot && pathtestCtx.runUntil.Equal(now))
213+
if !expired {
214+
return
215+
}
216+
delete(f.contexts, hash)
217+
if pathtestCtx.lastFlushTime.IsZero() {
218+
f.statsdClient.Incr(networkPathStoreMetricPrefix+"pathtest_never_run", []string{}, 1) //nolint:errcheck
219+
}
220+
if pathtestCtx.Pathtest.DynamicTestProfile == payload.DynamicTestProfileBaseline {
221+
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_expired", []string{}, 1) //nolint:errcheck
222+
}
223+
} else {
224+
// Refresh attribution from the latest admission without creating a second
225+
// context for the same path.
226+
pathtestCtx.Pathtest.TestConfigID = pathtestToAdd.TestConfigID
227+
pathtestCtx.Pathtest.TestConfigSource = pathtestToAdd.TestConfigSource
228+
pathtestCtx.Pathtest.Tags = slices.Clone(pathtestToAdd.Tags)
229+
pathtestCtx.runUntil = f.timeNowFn().Add(f.config.TTL)
209230
return
210231
}
211-
// Refresh attribution from the latest admission without creating a second
212-
// context for the same path.
213-
pathtestCtx.Pathtest.TestConfigID = pathtestToAdd.TestConfigID
214-
pathtestCtx.Pathtest.TestConfigSource = pathtestToAdd.TestConfigSource
215-
pathtestCtx.Pathtest.Tags = slices.Clone(pathtestToAdd.Tags)
216-
pathtestCtx.runUntil = f.timeNowFn().Add(f.config.TTL)
217-
return
218232
}
219233

220234
if len(f.contexts) >= f.config.ContextsLimit {

comp/networkpath/npcollector/impl/pathteststore/pathteststore_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,24 @@ func TestPathtestStoreOneShotExpiresBeforeDispatch(t *testing.T) {
224224
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_expired"].Sum)
225225
}
226226

227+
func TestPathtestStoreReplacesExpiredOneShotBeforeFlush(t *testing.T) {
228+
now := time.Date(2026, 8, 15, 12, 0, 0, 0, time.UTC)
229+
stats := &teststatsd.Client{}
230+
store := NewPathtestStore(Config{ContextsLimit: 10}, logmock.New(t), stats, func() time.Time { return now })
231+
store.Add(&common.Pathtest{Hostname: "baseline", OneShot: true, ExecutionDeadline: now.Add(30 * time.Minute), DynamicTestProfile: payload.DynamicTestProfileBaseline})
232+
now = now.Add(30 * time.Minute)
233+
newDeadline := now.Add(30 * time.Minute)
234+
store.Add(&common.Pathtest{Hostname: "baseline", OneShot: true, ExecutionDeadline: newDeadline, DynamicTestProfile: payload.DynamicTestProfileBaseline})
235+
236+
replacement := store.contexts[(&common.Pathtest{Hostname: "baseline"}).GetHash()]
237+
require.NotNil(t, replacement)
238+
assert.Equal(t, newDeadline, replacement.runUntil)
239+
assert.Len(t, store.Flush(), 1, "the next window's replacement must remain dispatchable")
240+
assert.Zero(t, store.GetContextsCount())
241+
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_expired"].Sum)
242+
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_dispatched"].Sum)
243+
}
244+
227245
func TestPathtestStoreRecurringDispatchesAtTTLBoundary(t *testing.T) {
228246
now := time.Date(2026, 8, 15, 12, 0, 0, 0, time.UTC)
229247
store := NewPathtestStore(Config{

test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ const (
4040
hostTrafficDNSPIDPath = "/tmp/host_traffic_dns.pid"
4141
hostTrafficResolverBackupPath = "/tmp/host_traffic_resolv.conf.backup"
4242
hostTrafficResolverLinkPath = "/tmp/host_traffic_resolv.conf.link"
43-
hostTrafficCurlLogPath = "/tmp/host_traffic_dynamic_path_curl.log"
44-
hostTrafficCurlPIDPath = "/tmp/host_traffic_dynamic_path_curl.pid"
43+
hostTrafficGeneratorLogPath = "/tmp/host_traffic_dynamic_path_generator.log"
44+
hostTrafficGeneratorPIDPath = "/tmp/host_traffic_dynamic_path_generator.pid"
4545
hostTrafficHTTPBinComposeYAML = `version: '3.9'
4646
services:
4747
httpbin:
@@ -124,8 +124,8 @@ func hostTrafficHTTPBinCompose() docker.ComposeInlineManifest {
124124
}
125125

126126
func (s *hostTrafficDynamicPathBaseSuite) setupHostTraffic() {
127-
s.ensureCurlInstalled()
128127
s.startHostTrafficDNSServer()
128+
s.assertHostTrafficServiceReady()
129129
s.configureAgentResolver()
130130
s.assertHostTrafficDomainResolves()
131131
}
@@ -139,13 +139,18 @@ func (s *hostTrafficDynamicPathBaseSuite) tearDownHostTraffic() {
139139
func (s *hostTrafficDynamicPathBaseSuite) AfterTest(suiteName, testName string) {
140140
if s.T().Failed() {
141141
s.logRemoteFile(s.Env().HTTPBinHost, hostTrafficDNSLogPath)
142-
s.logRemoteFile(s.Env().RemoteHost, hostTrafficCurlLogPath)
142+
s.logRemoteFile(s.Env().RemoteHost, hostTrafficGeneratorLogPath)
143143
}
144144
s.BaseSuite.AfterTest(suiteName, testName)
145145
}
146146

147-
func (s *hostTrafficDynamicPathBaseSuite) ensureCurlInstalled() {
148-
s.Env().RemoteHost.MustExecute("if ! command -v curl >/dev/null 2>&1; then sudo apt-get update && sudo apt-get install -y curl; fi")
147+
func (s *hostTrafficDynamicPathBaseSuite) assertHostTrafficServiceReady() {
148+
s.Env().HTTPBinHost.MustExecute(`i=0; while [ "$i" -lt 30 ]; do
149+
python3 -c 'import urllib.request; urllib.request.urlopen("http://127.0.0.1/", timeout=5).read()' && exit 0
150+
sleep 2
151+
i=$((i+1))
152+
done
153+
exit 1`)
149154
}
150155

151156
func (s *hostTrafficDynamicPathBaseSuite) startHostTrafficDNSServer() {
@@ -234,23 +239,23 @@ func (s *hostTrafficDynamicPathBaseSuite) assertHostTrafficDomainResolves() {
234239

235240
func (s *hostTrafficDynamicPathBaseSuite) startHostTrafficGenerator(duration time.Duration) {
236241
seconds := int(duration.Seconds())
237-
trafficCommand := fmt.Sprintf(
238-
"i=0; while [ \"$i\" -lt %d ]; do curl -4 -fsS --max-time 5 %s >/dev/null || true; sleep 2; i=$((i+2)); done",
242+
trafficScript := fmt.Sprintf(
243+
"import time, urllib.request\nurl = %q\nend = time.monotonic() + %d\nwhile time.monotonic() < end:\n try:\n urllib.request.urlopen(url, timeout=5).read()\n except Exception:\n pass\n time.sleep(2)\n",
244+
hostTrafficURL(hostTrafficRemoteConfigDomain),
239245
seconds,
240-
shellQuote(hostTrafficURL(hostTrafficRemoteConfigDomain)),
241246
)
242-
s.Env().RemoteHost.MustExecute(fmt.Sprintf("nohup sh -c %s >%s 2>&1 & echo $! >%s",
243-
shellQuote(trafficCommand),
244-
shellQuote(hostTrafficCurlLogPath),
245-
shellQuote(hostTrafficCurlPIDPath),
247+
s.Env().RemoteHost.MustExecute(fmt.Sprintf("nohup python3 -c %s >%s 2>&1 & echo $! >%s",
248+
shellQuote(trafficScript),
249+
shellQuote(hostTrafficGeneratorLogPath),
250+
shellQuote(hostTrafficGeneratorPIDPath),
246251
))
247252
}
248253

249254
func (s *hostTrafficDynamicPathBaseSuite) stopHostTrafficGenerator() {
250255
if s.Env().RemoteHost == nil {
251256
return
252257
}
253-
_, err := s.Env().RemoteHost.Execute(fmt.Sprintf(`if [ -f %s ]; then kill "$(cat %s)" || true; fi`, shellQuote(hostTrafficCurlPIDPath), shellQuote(hostTrafficCurlPIDPath)))
258+
_, err := s.Env().RemoteHost.Execute(fmt.Sprintf(`if [ -f %s ]; then kill "$(cat %s)" || true; fi`, shellQuote(hostTrafficGeneratorPIDPath), shellQuote(hostTrafficGeneratorPIDPath)))
254259
if err != nil {
255260
s.T().Logf("failed to stop host traffic generator: %v", err)
256261
}

0 commit comments

Comments
 (0)