ROX-13540: Allow Sensor Component `NodeScanHandler` to start max. once by vikin91 · Pull Request #3800 · stackrox/stackrox

vikin91 · 2022-11-14T11:13:10Z

Description

This is a follow-up PR to address review comments from #3533.
The original PR was merged by accident without addressing optional but
good suggestions.

This PR changes the NodeScan component to start maximally once.

It includes:

~~🗑️ Removing stoppedC leftover~~ (readded in d7353ae)
🐛 Handling a panic when two or more consecutive Start() calls happen without a Stop() call in-between
🧪 Adding a regression test for the two or more consecutive Start() calls
🐛 Fixing previously undiscovered race condition
💄 Removing one level of indentation in sendScan
💄 Using s.Assert() instead assert.
~~✨ (Extra) Making the component restartable~~ (removed in d7353ae)
Re-added Stopped() so that the tests can wait for the handler to finish work.

Extra

After the first comment about not closing the toCentral channel, I decided that the only clean solution to that challenge it to make the component restartable. I added the restartability in be4bb7c.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added

Not applies

Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

Testing Performed

CI
go test -race -count=1 -v -run ^TestNodeScanHandler$ ./sensor/common/compliance
go test -race -count=12 -v -run ^TestNodeScanHandler$ ./sensor/common/compliance (more executions at once)
Manual deployment of cluster locally, enabling the FF (ROX_RHCOS_NODE_SCANNING=true), and
confirming that the messages are sent to Sensor and Central. Also
killing sensor pod and making sure that it gets restarted and
self-heals.

openshift-ci · 2022-11-14T11:13:14Z

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 · 2022-11-14T11:13:22Z

Current dependencies on/for this PR:

master
- PR ROX-13540: Allow Sensor Component NodeScanHandler to start max. once #3800 👈
  - PR Rename NodeScanV2 to NodeInventory #3755
    - PR ROX-13722: Move NodeInventory into Node #3757
      - PR ROX-12975: Use new Scanner API for Node scanning #3940
        
        PR Port changes from mm/ROX-12967-real-nodescan #3941
      - PR ROX-12943: Sensor maps the NodeInventory to Node #3892

This comment was auto-generated by Graphite.

ghost · 2022-11-15T10:06:35Z

Images are ready for the commit at 5246c62.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.73.x-119-g5246c62090.

vikin91 · 2022-11-15T12:56:14Z

sensor/common/compliance/nodescan_handler_test.go

 				return
-			default:
-				nodeScans <- fakeNodeScanV2("Node")
+			case nodeScans <- fakeNodeScanV2("Node"):


Fix for the race: placing the blocking part in the case instead of default

vikin91 · 2022-11-15T12:57:39Z

sensor/common/compliance/nodescan_handler_test.go

+			case _, ok := <-h.ResponsesC():
+				if !ok {
+					return
+				}


Simplification: Instead of forwarding to central, we now receive and discard all msgs that are meant to go to central.

RTann · 2022-11-16T02:54:43Z

sensor/common/compliance/nodescan_handler_test.go

+	s.Assert().NotPanics(func() {
+		defer close(nodeScans)
+		h := NewNodeScanHandler(nodeScans)
+		s.Assert().NoError(h.Start())


ah I found out a *suite is an *assert.Assertions, so no need to do s.Assert()..... You can just do s.NoError(...)

sensor/common/compliance/nodescan_handler_impl.go

vikin91 · 2022-11-16T16:19:31Z

Inviting additional Sensor experts to review if they find a time :) @fredrb and @lvalerom

RTann · 2022-11-18T01:42:27Z

sensor/common/compliance/nodescan_handler_impl.go

-	toCentral chan *central.MsgFromSensor
+	toCentral <-chan *central.MsgFromSensor
+	// toCentralMux prevents the race condition between Start() [writer] and ResponsesC() [reader]
+	toCentralMux *sync.Mutex


just make this a sync.Mutex, so you don't have to initialize it. The zero-value is an unlocked mutex

also, it looks like this mutex it used for more than just toCentral, but rather to ensure Start() and ResponseC do are not run in parallel/concurrently. Maybe this can just be called lock? (Also I think mux typically implies multiplexer instead of mutex)

RTann · 2022-11-18T05:27:17Z

sensor/common/compliance/nodescan_handler_impl.go

 	nodeScans <-chan *storage.NodeScanV2
-	toCentral chan *central.MsgFromSensor
+	toCentral <-chan *central.MsgFromSensor
+	// toCentralMux prevents the race condition between Start() [writer] and ResponsesC() [reader]


From what I can tell, we call Start() for each component before eventually calling ResponsesC() for each (see https://github.com/stackrox/stackrox/blob/3.72.2/sensor/common/sensor/sensor.go#L210. it looks like we call Start() for each component and then we eventually run ProcessMessage() and ResponsesC() concurrently). It definitely does not hurt at all to be defensive, but I don't think other sensor components have bothered.

Similarly, I wonder if we should just have some flag which indicates if Start() has been called and only allow ResponsesC continue once that happens? Or we even just comment that you must call ResponsesC after Start() completes and leave it up to the caller

I know that some usage patterns are not used in our setup, but my tests reported race or a leak and I wanted them to be clean. Moreover, we discussed the restartablilty of sensor components in the F2F meeting and we may be soon rely on it.

RTann · 2022-11-18T05:34:40Z

sensor/common/compliance/nodescan_handler_impl.go


 func (c *nodeScanHandlerImpl) Start() error {
-	// c.run closes chan toCentral on exit, so we do not want to close twice in case of a restart
+	c.toCentralMux.Lock()


The more I read this and look at other Sensor components, the more I wonder if this may be overkill and all my questions about how to use this library were not very relevant. This seems to be correct, but I also wonder if it's worth worrying so much about this since we know how these methods are called (well at least now I know because I finally looked into it). Not saying to remove it but I wanted to see what you think

+1 to not overkill. In my opinion, it will be sufficient to check whether a Start() was once attempted, and, if so, return an error. I don't think there's a need to make this component restartable provided that others don't take advantage of it.

I know this is an overkill a bit, but I think this investment is worth it. We may need to make all Sensor components restartable, so I thought that this may be also a good way to have the race & goroutine leak tests green.

RTann · 2022-11-18T05:40:19Z

havent read it all yet, but wanted to leave some comments in case i forget to come back to this in a timely manner

msugakov

Checked the main code mostly, skipped tests.

sensor/common/compliance/nodescan_handler_impl.go

sensor/common/compliance/nodescan_handler_test.go

sensor/common/compliance/nodescan_handler.go

msugakov · 2022-11-18T16:21:25Z

sensor/common/compliance/nodescan_handler_impl.go


 func (c *nodeScanHandlerImpl) Start() error {
-	// c.run closes chan toCentral on exit, so we do not want to close twice in case of a restart
+	c.toCentralMux.Lock()


+1 to not overkill. In my opinion, it will be sufficient to check whether a Start() was once attempted, and, if so, return an error. I don't think there's a need to make this component restartable provided that others don't take advantage of it.

sensor/common/compliance/nodescan_handler_impl.go

…ng first message

…and receiver receiving first message" This reverts commit 8d13af4.

vikin91 · 2022-11-28T17:44:30Z

@RTann @fredrb @lvalerom we are done with changes (provided that CI is happy), if you want to review again, then it may be a good time for it.

openshift-ci bot added the do-not-merge/work-in-progress label Nov 14, 2022

github-actions bot added the area/sensor label Nov 14, 2022

vikin91 force-pushed the pr/ROX-12835-followup-1 branch from 0880bd0 to c9416c7 Compare November 15, 2022 08:50

This was referenced Nov 15, 2022

Rename NodeScanV2 to NodeInventory #3755

Merged

ROX-13722: Move NodeInventory into Node #3757

Closed

vikin91 marked this pull request as ready for review November 15, 2022 09:17

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 15, 2022

vikin91 changed the title ~~Fix: Ensure that consecutive calls to nodeScanHandlerImpl.Start don't panic~~ ROX-13540: Ensure that consecutive calls to nodeScanHandlerImpl.Start don't panic Nov 15, 2022

vikin91 commented Nov 15, 2022

View reviewed changes

vikin91 requested review from RTann and msugakov November 15, 2022 12:58

RTann reviewed Nov 16, 2022

View reviewed changes

vikin91 marked this pull request as draft November 16, 2022 11:37

openshift-ci bot added the do-not-merge/work-in-progress label Nov 16, 2022

vikin91 force-pushed the pr/ROX-12835-followup-1 branch from 3f47d78 to be4bb7c Compare November 16, 2022 12:19

vikin91 changed the title ~~ROX-13540: Ensure that consecutive calls to nodeScanHandlerImpl.Start don't panic~~ ROX-13540: Make Sensor Component NodeScanHandler restartable Nov 16, 2022

vikin91 marked this pull request as ready for review November 16, 2022 12:30

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 16, 2022

vikin91 requested review from RTann, fredrb and lvalerom November 16, 2022 16:18

RTann reviewed Nov 18, 2022

View reviewed changes

fredrb added the team/maple label Nov 18, 2022

msugakov reviewed Nov 18, 2022

View reviewed changes

vikin91 added 21 commits November 28, 2022 18:14

Rephrase comment in TestStopHandler

38c94fe

Add more assertions to TestHandlerRegularRoutine

86dae6e

Ensure all test cases do h.Stopped().Wait()

c008157

WIP: Rework tests

283a43e

Reword comment

98a2509

Fix race in TestStopHandler between handler Stop and receiver receivi…

d9fcadc

…ng first message

Stop(nil) and skip stopAll in TestHandlerRegularRoutine

83f8613

Skip stopAll in TestHandlerStoppedError

706d1aa

Cleanup generateTestInputNoClose

cc6fd29

Improve TestRestartHandler

ae283da

Improve TestDoubleStartHandler and TestDoubleStopHandler

d948528

Temporarily revert "Fix race in TestStopHandler between handler Stop …

13befb8

…and receiver receiving first message" This reverts commit 8d13af4.

Remove the test context

dc50b12

Do not defer assertNoGoroutineLeaks

a4f7112

Correct stopAll code in TestStopHandler

8369b91

Remove redundant waiting in TestRestartHandler

ad836f8

Remove stopAll and fix race in TestStopHandler

06bc997

Fix hidden race in TestMultipleStartHandler

a4c6c54

Remove redundant TestRestartHandler

e22e35b

Remove assertions NotPanics

ed00521

Add coverage with test TestInputChannelClosed

e385c20

vikin91 force-pushed the pr/ROX-12835-followup-1 branch from 56dab50 to e385c20 Compare November 28, 2022 17:15

Fine details: comments and order of cleanup

5246c62

msugakov approved these changes Nov 28, 2022

View reviewed changes

msugakov mentioned this pull request Nov 28, 2022

ROX-13661: Refactor Stopper and use it more #3931

Merged

5 tasks

vikin91 merged commit 5fc5870 into master Nov 29, 2022

vikin91 deleted the pr/ROX-12835-followup-1 branch November 29, 2022 10:10

This was referenced Nov 29, 2022

ROX-12975: Use new Scanner API for Node scanning #3940

Closed

Port changes from mm/ROX-12967-real-nodescan #3941

Closed

Conversation

vikin91 commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Extra

Checklist

Not applies

Testing Performed

Uh oh!

openshift-ci bot commented Nov 14, 2022

Uh oh!

vikin91 commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 15, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vikin91 commented Nov 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RTann commented Nov 18, 2022

Uh oh!

msugakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vikin91 commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vikin91 commented Nov 14, 2022 •

edited

Loading

vikin91 commented Nov 14, 2022 •

edited

Loading

ghost commented Nov 15, 2022 •

edited by ghost

Loading