ROX-30390: Don't queue open enriched-entities forever by vikin91 · Pull Request #16532 · stackrox/stackrox

vikin91 · 2025-08-26T10:00:05Z

🐑 Shepherd of this stack is @lvalerom and @rhybrillou (when back)

Description

Here, we change the rule for removing the already enriched enriched-entities (EEs) from the queue:

func (c *connStatus) checkRemoveCondition(useLegacy, isConsumed bool) bool {
	// Legacy UpdateComputer requires keeping all open entities in the enrichment queue until they are closed.
	if useLegacy {
		return c.rotten || (c.isClosed() && isConsumed)
	}
	return c.rotten || isConsumed
}

The rest of the PR is:

FF to switch between legacy and transition-based
boilerplate
unit-tests

Change in `checkRemoveCondition`

Before that, we kept the open EEs in hostConns for as long as they were open. After this change, we can treat the hostConns to be a real enrichment queue, so that it holds only those items that have not been enriched yet or the enrichment must be retried.

I did a careful investigation why we were keeping the open connections in hostConns forever and it was only for the correct calculation of the updates being sent to Central using the legacy update computer. After switching to the categorized implementation, this change can be introduced without consequences to the feature.

User-facing documentation

CHANGELOG.md is updated OR update is not needed
documentation PR is created and is linked above OR is not needed

Testing and quality

the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
CI results are inspected

Automated testing

How I validated my change

CI
Manually on a Long-running cluster & looking at the metrics

openshift-ci · 2025-08-26T10:00:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

vikin91 · 2025-08-26T10:00:31Z

This change is part of the following stack:

ROX-30390: Prefactor: Extract indicators into pkg #16447
- ROX-30390: Extract Update Computer #16452
  - ROX-30390: Add indicator key functions #16621
    - ROX-30390: Refactor netFlowManager metrics #16624
      - ROX-30390: Add TransitionBased update computer #16522
        
        ROX-30390: Don't queue open enriched-entities forever #16532 ◀
        
        ROX-30390: Remove gauge metric for update types #16769
        
        ROX-30390: Leftovers from the prototype & code instrumentation #16238

_{Change managed by git-spice.}

rhacs-bot · 2025-08-26T10:19:09Z

Images are ready for the commit at 48c3ec7.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.9.x-745-g48c3ec70ae.

codecov · 2025-08-26T10:37:12Z

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.80%. Comparing base (9678d9b) to head (dd5b1bb).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...sor/common/networkflow/updatecomputer/interface.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16532      +/-   ##
==========================================
- Coverage   48.81%   48.80%   -0.01%     
==========================================
  Files        2679     2680       +1     
  Lines      200331   200342      +11     
==========================================
- Hits        97791    97783       -8     
- Misses      94935    94951      +16     
- Partials     7605     7608       +3

Flag	Coverage Δ
go-unit-tests	`48.80% <61.53%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

Consider centralizing the UpdateComputer selection based on the NetworkFlowUseLegacyUpdateComputer flag into a dedicated factory/helper to reduce duplication between createManager and CreateSensor.
Instead of reading the environment flag directly within manager methods, inject the chosen UpdateComputer via constructor to improve clarity and testability of the enrichment pipeline logic.
Add targeted unit tests for the new checkRemoveCondition function to validate both legacy and non-legacy removal behaviors and ensure they match the intended logic.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Consider centralizing the UpdateComputer selection based on the `NetworkFlowUseLegacyUpdateComputer` flag into a dedicated factory/helper to reduce duplication between `createManager` and `CreateSensor`.
- Instead of reading the environment flag directly within manager methods, inject the chosen UpdateComputer via constructor to improve clarity and testability of the enrichment pipeline logic.
- Add targeted unit tests for the new `checkRemoveCondition` function to validate both legacy and non-legacy removal behaviors and ensure they match the intended logic.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

vikin91 · 2025-09-11T09:15:05Z

Consider centralizing the UpdateComputer selection based on the NetworkFlowUseLegacyUpdateComputer flag into a dedicated factory/helper to reduce duplication between createManager and CreateSensor.

Done in 54711e1

Instead of reading the environment flag directly within manager methods, inject the chosen UpdateComputer via constructor to improve clarity and testability of the enrichment pipeline logic.

Done in 54711e1

Add targeted unit tests for the new checkRemoveCondition function to validate both legacy and non-legacy removal behaviors and ensure they match the intended logic.

Done in 6cb18bf

rhacs-bot · 2025-09-11T09:41:39Z

Images are ready for the commit at dd5b1bb.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.9.x-751-gdd5b1bbd65.

lvalerom

LGTM

sensor/common/networkflow/manager/enrichment.go

rhacs-bot · 2025-09-11T11:41:10Z

/retest

openshift-ci · 2025-09-11T12:58:24Z

@vikin91: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocp-4-19-qa-e2e-tests	`dd5b1bb`	link	false	`/test ocp-4-19-qa-e2e-tests`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

rhacs-bot · 2025-09-11T13:10:05Z

/retest

openshift-ci bot added the do-not-merge/work-in-progress label Aug 26, 2025

This comment was marked as off-topic.

Sign in to view

github-actions bot added area/sensor ai-review labels Aug 26, 2025

This was referenced Aug 26, 2025

ROX-30390: Add TransitionBased update computer #16522

Merged

ROX-30390: Leftovers from the prototype & code instrumentation #16238

Closed

ROX-30390: Extract Update Computer #16452

Merged

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 39a7266 to 0d2e13e Compare August 26, 2025 10:59

vikin91 force-pushed the piotr/ROX-30390-categorized branch from 6140a99 to 75a4b41 Compare August 27, 2025 10:33

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 0d2e13e to 16f47ce Compare August 27, 2025 10:33

vikin91 force-pushed the piotr/ROX-30390-categorized branch from cf2fd11 to f42e378 Compare August 29, 2025 13:21

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 16f47ce to 399b027 Compare August 29, 2025 13:22

vikin91 force-pushed the piotr/ROX-30390-categorized branch from dbfcf08 to 3fd11bf Compare August 29, 2025 14:20

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 399b027 to 4b2b0a5 Compare August 29, 2025 14:20

vikin91 force-pushed the piotr/ROX-30390-categorized branch from 3fd11bf to 67d9d64 Compare August 29, 2025 14:42

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch 2 times, most recently from 6f2d24e to 49bdb60 Compare August 29, 2025 15:19

vikin91 force-pushed the piotr/ROX-30390-categorized branch from 3b07945 to 4dfb3c0 Compare August 29, 2025 16:19

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 49bdb60 to 727558f Compare August 29, 2025 16:19

vikin91 force-pushed the piotr/ROX-30390-categorized branch from 4dfb3c0 to fd59e5a Compare September 1, 2025 08:54

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch 4 times, most recently from 9b9dd17 to 140f9db Compare September 2, 2025 08:12

vikin91 force-pushed the piotr/ROX-30390-categorized branch from 92e8fb4 to c8fc930 Compare September 2, 2025 08:25

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 140f9db to d9cb951 Compare September 2, 2025 08:25

vikin91 requested a review from a team as a code owner September 11, 2025 07:32

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 11, 2025

vikin91 requested a review from lvalerom September 11, 2025 07:32

vikin91 added the ci-all-qa-tests Tells CI to run all API tests (not just BAT). label Sep 11, 2025

sourcery-ai bot reviewed Sep 11, 2025

View reviewed changes

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 48c3ec7 to 6cb18bf Compare September 11, 2025 09:14

Base automatically changed from piotr/ROX-30390-categorized to master September 11, 2025 09:32

vikin91 added 10 commits September 11, 2025 11:34

Empty commit to create a branch with gs

6c9b160

Do not store open enriched-entities in queue forever

c4cc348

Remove TODO comments

bd00f2f

Update unit test

0bedb53

Add env var to control the check-remove behavior

36ccc10

Add env switch to fallback to the legacy update computer

d8d0100

Style: Remove unused imports

6f156a9

Fix style again

4c107f3

Add factory helper method for uc construction

c06b370

Add unit-test for checkRemoveCondition

367cc38

vikin91 force-pushed the piotr/ROX-30390-dont-queue-open-ee branch from 6cb18bf to 367cc38 Compare September 11, 2025 09:34

lvalerom approved these changes Sep 11, 2025

View reviewed changes

sensor/common/networkflow/manager/enrichment.go Show resolved Hide resolved

vikin91 mentioned this pull request Sep 11, 2025

ROX-30390: Remove gauge metric for update types #16769

Merged

9 tasks

vikin91 removed the ci-all-qa-tests Tells CI to run all API tests (not just BAT). label Sep 11, 2025

Empty commit to trigger the stuck GH actions CI

dd5b1bb

vikin91 enabled auto-merge (squash) September 11, 2025 11:51

vikin91 merged commit 382e24c into master Sep 11, 2025
89 of 96 checks passed

vikin91 deleted the piotr/ROX-30390-dont-queue-open-ee branch September 11, 2025 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-30390: Don't queue open enriched-entities forever#16532

ROX-30390: Don't queue open enriched-entities forever#16532
vikin91 merged 11 commits intomasterfrom
piotr/ROX-30390-dont-queue-open-ee

vikin91 commented Aug 26, 2025 •

edited

Loading

Uh oh!

openshift-ci bot commented Aug 26, 2025

Uh oh!

This comment was marked as off-topic.

vikin91 commented Aug 26, 2025 •

edited

Loading

Uh oh!

rhacs-bot commented Aug 26, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 26, 2025 •

edited

Loading

Uh oh!

sourcery-ai bot left a comment

Uh oh!

vikin91 commented Sep 11, 2025

Uh oh!

rhacs-bot commented Sep 11, 2025 •

edited

Loading

Uh oh!

lvalerom left a comment

Uh oh!

Uh oh!

rhacs-bot commented Sep 11, 2025

Uh oh!

openshift-ci bot commented Sep 11, 2025

Uh oh!

rhacs-bot commented Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

vikin91 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Change in checkRemoveCondition

User-facing documentation

Testing and quality

Automated testing

How I validated my change

Uh oh!

openshift-ci bot commented Aug 26, 2025

Uh oh!

This comment was marked as off-topic.

vikin91 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhacs-bot commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

vikin91 commented Sep 11, 2025

Uh oh!

rhacs-bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lvalerom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhacs-bot commented Sep 11, 2025

Uh oh!

openshift-ci bot commented Sep 11, 2025

Uh oh!

rhacs-bot commented Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vikin91 commented Aug 26, 2025 •

edited

Loading

Change in `checkRemoveCondition`

vikin91 commented Aug 26, 2025 •

edited

Loading

rhacs-bot commented Aug 26, 2025 •

edited

Loading

codecov bot commented Aug 26, 2025 •

edited

Loading

rhacs-bot commented Sep 11, 2025 •

edited

Loading