Add Closer on Runner interface by mathetake · Pull Request #5527 · envoyproxy/gateway

mathetake · 2025-03-17T23:50:56Z

What type of PR is this?

chore: add Close method interface on Runner interface

What this PR does / why we need it:

Previously, the runner "shudown" was not existent, so the main goroutine doesn't wait for the cleanup that is not synchronous to the context closure or channel. That includes virtually all the operations after the context cancelation running in non main goroutines. One notable example is that when running with the host provider, Envoy processes are spawned and managed from non-main goroutines, so without the "synchronous" closure hook on Runner, it is impossible to ensure the exit of Envoy processes. Without this fix, for example, standalone mode will leave all the Envoy processes as orphans after the CLI exits with SIGINT or SIGTERM.

As a side effect, this also fixes a bug in the host provider which is that its CreateOrUpdateProxyInfra blocks forever until the Envoy process is running. That is not the expected semantics of infra.Manager interface.

Release Notes: No

mathetake · 2025-03-17T23:55:12Z

obviously i need to write a test

codecov · 2025-03-18T00:01:47Z

Codecov Report

Attention: Patch coverage is 39.02439% with 25 lines in your changes missing coverage. Please review.

Project coverage is 65.18%. Comparing base (7b8bed0) to head (0743eaa).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/infrastructure/host/proxy_infra.go	55.17%	12 Missing and 1 partial ⚠️
internal/cmd/server.go	0.00%	4 Missing ⚠️
internal/infrastructure/runner/runner.go	0.00%	2 Missing ⚠️
internal/gatewayapi/runner/runner.go	0.00%	1 Missing ⚠️
internal/globalratelimit/runner/runner.go	0.00%	1 Missing ⚠️
internal/infrastructure/kubernetes/infra.go	0.00%	1 Missing ⚠️
internal/provider/runner/runner.go	0.00%	1 Missing ⚠️
internal/xds/server/runner/runner.go	0.00%	1 Missing ⚠️
internal/xds/translator/runner/runner.go	0.00%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (39.02%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5527      +/-   ##
==========================================
- Coverage   65.19%   65.18%   -0.02%     
==========================================
  Files         213      213              
  Lines       34059    34080      +21     
==========================================
+ Hits        22204    22214      +10     
- Misses      10529    10538       +9     
- Partials     1326     1328       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake · 2025-03-18T23:58:16Z

ok ready

mathetake · 2025-03-19T00:05:20Z

internal/cmd/server.go

+	for _, r := range runners {
+		if err := r.runner.Close(); err != nil {
+			cfg.Logger.Error(err, "failed to close runner", "name", r.runner.Name())
+		}
+	}


this is the main change on the main routine besides the host infra closure

mathetake · 2025-03-19T00:42:30Z

/retest

mathetake · 2025-03-19T00:42:56Z

oh man the command exists here as well!

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

internal/infrastructure/host/proxy_infra.go

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake · 2025-03-19T01:36:12Z

internal/infrastructure/host/proxy_infra.go

+	// This is ad-hoc sleep to wait for the Envoy process to start up to avoid canceling the context too early,
+	// which also results in the Envoy process being orphaned.
+	time.Sleep(1 * time.Second)


i think this seems an issue of func-e, not here and should be fixed there

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake · 2025-03-19T01:55:10Z

lol we encountered a race in func-e

==================
WARNING: DATA RACE
Read at 0x00c000580218 by goroutine 251:
  os.(*ProcessState).ExitCode()
      /opt/hostedtoolcache/go/1.24.1/x64/src/os/exec_posix.go:145 +0xbf2
  github.com/tetratelabs/func-e/internal/envoy.(*Runtime).Run()
      /home/runner/go/pkg/mod/github.com/tetratelabs/func-e@v1.1.5-0.20250318224823-b69d1c096c7a/internal/envoy/run.go:95 +0xbb4
  github.com/tetratelabs/func-e/internal/cmd.NewRunCmd.func2()
      /home/runner/go/pkg/mod/github.com/tetratelabs/func-e@v1.1.5-0.20250318224823-b69d1c096c7a/internal/cmd/run.go:87 +0x129d
  github.com/urfave/cli/v2.(*Command).Run()
      /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.6/command.go:276 +0x15a1
  github.com/urfave/cli/v2.(*Command).Run()
      /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.6/command.go:269 +0x1464
  github.com/urfave/cli/v2.(*App).RunContext()
      /home/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.27.6/app.go:333 +0x1228
  github.com/tetratelabs/func-e/api.Run.func1()
      /home/runner/go/pkg/mod/github.com/tetratelabs/func-e@v1.1.5-0.20250318224823-b69d1c096c7a/api/run.go:115 +0x84

Previous write at 0x00c000580218 by goroutine 256:
  os.(*Process).pidfdWait()
      /opt/hostedtoolcache/go/1.24.1/x64/src/os/pidfd_linux.go:116 +0x5aa
  os.(*Process).wait()
      /opt/hostedtoolcache/go/1.24.1/x64/src/os/exec_unix.go:27 +0x4d
  os.(*Process).Wait()
      /opt/hostedtoolcache/go/1.24.1/x64/src/os/exec.go:358 +0xaf
  os/exec.(*Cmd).Wait()
      /opt/hostedtoolcache/go/1.24.1/x64/src/os/exec/exec.go:922 +0x8f
  github.com/tetratelabs/func-e/internal/envoy.(*Runtime).Run.func1()
      /home/runner/go/pkg/mod/github.com/tetratelabs/func-e@v1.1.5-0.20250318224823-b69d1c096c7a/internal/envoy/run.go:64 +0x6a

mathetake · 2025-03-19T02:00:25Z

marking as a draft for now

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake · 2025-03-19T21:05:57Z

go.mod

 	github.com/stretchr/testify v1.10.0
 	github.com/telepresenceio/watchable v0.0.0-20220726211108-9bb86f92afa7
-	github.com/tetratelabs/func-e v1.1.5-0.20240822223546-c85a098d5bf0
+	github.com/tetratelabs/func-e v1.1.5-0.20250319205758-07e93e08850f


this is needed to have tetratelabs/func-e#458

mathetake · 2025-03-19T21:14:26Z

great, passing with the latest func-e tetratelabs/func-e@ca8702e

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

arkodg

LGTM thanks !

Add Closer on Runner interface

f846c7d

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake force-pushed the runnerclose branch from 97377a6 to f846c7d Compare March 18, 2025 23:49

mathetake marked this pull request as ready for review March 18, 2025 23:55

mathetake requested a review from a team as a code owner March 18, 2025 23:55

mathetake commented Mar 19, 2025

View reviewed changes

mathetake mentioned this pull request Mar 19, 2025

cli: scaffolds 'aigw run' command envoyproxy/ai-gateway#498

Merged

Ensure

29510b9

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake commented Mar 19, 2025

View reviewed changes

internal/infrastructure/host/proxy_infra.go Outdated Show resolved Hide resolved

more

7cefcd0

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake commented Mar 19, 2025

View reviewed changes

more

9f836db

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake marked this pull request as draft March 19, 2025 02:00

mathetake added 2 commits March 19, 2025 14:02

finally

857cc33

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

finally

04cf8ef

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mathetake mentioned this pull request Mar 19, 2025

fixes race condition in Run by decoupling singal handling from the impl tetratelabs/func-e#458

Merged

mathetake commented Mar 19, 2025

View reviewed changes

mathetake added 2 commits March 19, 2025 14:15

Updates func-e to the latest

670d92d

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Merge remote-tracking branch 'origin/main' into runnerclose

0743eaa

mathetake marked this pull request as ready for review March 19, 2025 21:16

arkodg approved these changes Mar 19, 2025

View reviewed changes

arkodg requested review from a team and shawnh2 March 20, 2025 00:04

arkodg requested a review from a team March 20, 2025 00:04

mathetake mentioned this pull request Mar 20, 2025

cli: initial implementation of aigw run envoyproxy/ai-gateway#458

Merged

zirain approved these changes Mar 20, 2025

View reviewed changes

arkodg merged commit 896ca89 into envoyproxy:main Mar 20, 2025
23 of 25 checks passed

mathetake deleted the runnerclose branch March 20, 2025 00:24

mathetake mentioned this pull request Mar 20, 2025

Wait for the runners to close #5560

Merged

Conversation

mathetake commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathetake commented Mar 17, 2025

Uh oh!

codecov bot commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mathetake commented Mar 18, 2025

Uh oh!

mathetake Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake commented Mar 19, 2025

Uh oh!

mathetake commented Mar 19, 2025

Uh oh!

Uh oh!

mathetake Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake commented Mar 19, 2025

Uh oh!

mathetake commented Mar 19, 2025

Uh oh!

mathetake Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

mathetake commented Mar 19, 2025

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mathetake commented Mar 17, 2025 •

edited

Loading

codecov bot commented Mar 18, 2025 •

edited

Loading