ROX-30860: Limit parallel port use by processes #16628
Merged
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
|
Images are ready for the commit at ddb5b45. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16628 +/- ##
==========================================
+ Coverage 48.67% 49.03% +0.36%
==========================================
Files 2675 2691 +16
Lines 199760 201669 +1909
==========================================
+ Hits 97235 98897 +1662
- Misses 94918 95103 +185
- Partials 7607 7669 +62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
01545b1 to
f1977c0
Compare
12 tasks
67995be to
4a77577
Compare
502b8cf to
6c08ff8
Compare
4a77577 to
da9feb4
Compare
10 tasks
6c08ff8 to
b75539a
Compare
da9feb4 to
6a3376f
Compare
b75539a to
7e33a34
Compare
6a3376f to
d0c8150
Compare
Base automatically changed from
piotr/fake-workload-remove-globals
to
master
September 9, 2025 08:05
d0c8150 to
6a81807
Compare
be53969 to
72a20e4
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `sensor/kubernetes/fake/flows.go:59` </location>
<code_context>
+// <container1, 1.1.1.1:80, apache2>, then Sensor will keep the nginx-entry forever, as there was no 'close' message in between.
+//
+// The probability logic is explicit and configurable for different testing scenarios.
+func (oc *OriginatorCache) GetOrSetOriginator(endpointKey string, containerID string, openPortReuseProbability float32, processPool *ProcessPool) *storage.NetworkProcessUniqueKey {
+ // Use panic-safe read lock to check cache
+ originator, exists := concurrency.WithRLock2(&oc.lock, func() (*storage.NetworkProcessUniqueKey, bool) {
</code_context>
<issue_to_address>
Consider validating openPortReuseProbability input range.
Without an explicit range check, passing values outside [0.0, 1.0] could cause incorrect probability calculations. Please add validation or clamp the input to avoid such issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
I still haven't done the long run on a cluster, but I will trigger it tomorrow. Edit: running on |
Contributor
|
/retest |
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
Author
|
/retest-required |
Contributor
Author
|
@JoukoVirtanen PTAL again, I have observations from the long-running cluster that workload generation works fine. |
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Introduced configurable port reuse probability to limit unrealistic endpoint sharing behavior in fake workload generation.
Key Changes:
OriginatorCache: Implements probabilistic endpoint-to-originator caching with configurable reuse probabilityopenPortReuseProbabilityfield: Configurable parameter (default 0.05) controlling how often different processes reuse the same IP:port without closing endpointsWhy: Previous behavior (100% port reuse in releases ≤4.8) was unrealistic and caused memory pressure in Sensor's enrichment pipeline. Multiple processes listening on the same IP:port without proper endpoint closure created excessive deduplication overhead, as Sensor would retain stale entries indefinitely when no close messages were sent between different originators on the same endpoint.
The new approach simulates realistic container behavior where processes typically bind consistently to endpoints (95% default) while allowing occasional port reuse scenarios (5% for restarts/takeovers).
Impact:
User-facing documentation
Testing and quality
Automated testing
How I validated my change
The workload runs fine on the long running cluster.
Sensor is crashing every 12 hours on 8GB of memory (due to OOMKill), which is a known pattern, but that is not caused by this PR, but by the: (1) bug in the memory management that is currently on master, (2) giving Sensor only 8GB of memory for the experiment.
The following chart confirms the rates of objects being generated by fake workflows
This chart shows that the processesListening are being processed and sent to Central
