Fix idle connection leak in prober

[dsimansk]

Changes

• Fix memory leak in prober with reusable transport

Per soak tests run on our Serverless distro with serving + net-kourier @maschmid observed a liner memory increase over time that may lead to OOM. It was traced down to idle connection pool steadily increasing.

In a serving-focused soak test with re-creating ksvcs, keeping around stable ~100 ksvcs on the cluster, net-kourier-controller with increased memory limit restarted 2 times on OOM during a 12h soak run.

/cc @maschmid @linkvt @dprotaso

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.34%. Comparing base (3cf3413) to head (61c8882).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
+ Coverage   93.33%   93.34%   +0.01%     
==========================================
  Files          35       35              
  Lines        1005     1007       +2     
==========================================
+ Hits          938      940       +2     
  Misses         53       53              
  Partials       14       14              

☔ 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.

[linkvt]

LGTM in general, minor comment about the fallback when context has no value set.

withProbeAddr could in theory be inlined but not really important.

[linkvt]

Not sure if we want to fallback here, panicking might be a good approach as we expect the probeaddr always to be set?

[dprotaso]

Any reason why we can't just enable DisableKeepAlives on the cloned transport ?

[dsimansk]

Any reason why we can't just enable DisableKeepAlives on the cloned transport ?

The soak test is running with the patch below. So far it more then halved the memory footprint.
Then hopefully comparing it to reused transport patch. Full cycle takes ~12h to complete. For sure we can start with as smaller DisableKeepAlives change. It'd definitely help.

 vendor/knative.dev/networking/pkg/status/status.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vendor/knative.dev/networking/pkg/status/status.go b/vendor/knative.dev/networking/pkg/status/status.go
index 4d7ebe515..5bfce6727 100644
--- a/vendor/knative.dev/networking/pkg/status/status.go
+++ b/vendor/knative.dev/networking/pkg/status/status.go
@@ -406,6 +406,7 @@ func (m *Prober) processWorkItem() bool {
 		prober.WithHeader(header.ProbeKey, header.ProbeValue),
 		prober.WithHeader(header.HashKey, header.HashValueOverride),
 		m.probeVerifier(item))
+	transport.CloseIdleConnections()
 
 	// In case of cancellation, drop the work item
 	select {
@@ -421,6 +422,7 @@ func (m *Prober) processWorkItem() bool {
 		item.logger.Errorf("Probing of %s failed, IP: %s:%s, ready: %t, error: %v (depth: %d)",
 			item.url, item.podIP, item.podPort, ok, err, m.workQueue.Len())
 	} else {
+		m.workQueue.Forget(obj)
 		m.onProbingSuccess(item.ingressState, item.podState)
 	}
 	return true

[dprotaso]

Curious - how does the soak test work? Is it creating a knative service or something every minute etc?

[dsimansk]

Curious - how does the soak test work? Is it creating a knative service or something every minute etc?

In a nutshell it should be maintaining a pool of ~100 ksvc in this case over long period of time. On this ksvc set performing periodic re-create with same name, add new revision, send request to scale from zero etc.

[dprotaso]

In a nutshell it should be maintaining a pool of ~100 ksvc in this case over long period of time. On this ksvc set performing periodic re-create with same name, add new revision, send request to scale from zero etc.

Do you have a script?

[dsimansk]

In a nutshell it should be maintaining a pool of ~100 ksvc in this case over long period of time. On this ksvc set performing periodic re-create with same name, add new revision, send request to scale from zero etc.

Do you have a script?

AFAIK there's a golang implementation of test framework that manages the soak loop scenario and collects evidence. @maschmid would need to fill in more gaps.

@dprotaso I've updated the PR to do a minimal change with .DisableKeepAlives property and .Forget call.

[dprotaso]

/lgtm
/approve

thanks!

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, dsimansk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [dprotaso,dsimansk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dsimansk]

/cherry-pick release-1.21
/cherry-pick release-1.22

[knative-prow-robot]

@dsimansk: new pull request created: #1141

Details

In response to this:

/cherry-pick release-1.21
/cherry-pick release-1.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[knative-prow-robot]

@dsimansk: new pull request created: #1142

Details

In response to this:

/cherry-pick release-1.21
/cherry-pick release-1.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.