ROX-13661: Refactor `Stopper` and use it more by msugakov · Pull Request #3931 · stackrox/stackrox

msugakov · 2022-11-28T21:47:09Z

Description

Refactor existing Stopper to be used in all places where we previously had stopC and stoppedC.
Induced by #3800

Note that I am not trying to fix things in this PR, just reduce the amount of different patterns. Some things look very questionable but I refrain from fixing them in the absence of unit tests.

It is best to review this PR by going through commits. This way the size isn't overwhelming.

Checklist

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

No need in the following:

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

Relying on CI.

ghost · 2022-11-28T22:03:39Z

Images are ready for the commit at 46adb80.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.73.x-331-g46adb80f44.

vikin91 · 2022-11-29T08:51:32Z

pkg/concurrency/stoppable.go

+// StoppableFlow represents an interface for goroutine to check and report status of a graceful shutdown.
+type StoppableFlow interface {
+	// StopWithError initiates a shutdown due to an error that happened during async processing in goroutine.
+	StopWithError(error)


Shouldn't StoppableClient.Stop and StoppableFlow.StopWithError be closer together in the same object?

This was intentional.

.Client().Stop() is called externally (outside of goroutine), and I haven't found any cases when non-nil error was provided as parameter. That's why .Client().Stop() doesn't take error parameters.

OTOH, I found goroutines self-stop due to error (e.g. a channel closed). That's why I added .Flow().StopWithError(error) for internal interface of goroutines (that's what I call StoppableFlow).

vikin91

Looks good so far (as still WIP). I am curious to see it in the NodeScanHandler (or NodeInventoryHandler after landing #3755)

vikin91 · 2022-11-29T12:39:17Z

sensor/common/compliance/command_handler_impl.go

-func (c *commandHandlerImpl) Stop(err error) {
-	c.stopC.SignalWithError(err)
+func (c *commandHandlerImpl) Stop(_ error) {
+	c.stopper.Client().Stop()


It looks like we are loosing information about the error passed here. As c.stopper.Flow().StopWithError(err) does not fit here, then maybe we need a replacement for sending a command to stop with error?

No, we are not loosing information because the error is always nil.

It is exactly the same topic as above - #3931 (comment)

I recommend you to check with IDE Find Usages what calls this Stop() method and whether there's any single place that gets non-nil error. I found zero.

I see, so the error was a "plan for later" that we never used

Probably. Something of this kind, or API was not separated for internal/external (from goroutine point of view) use cases and external functions were just a copy of internal needs. I guess we'd never know unless we go in archeology.

sensor/common/sensor/sensor.go

msugakov · 2022-11-29T21:08:24Z

sensor/common/compliance/nodescan_handler_test.go

 }

-func (s *NodeScanHandlerTestSuite) TestHandlerStoppedError() {
+func (s *NodeScanHandlerTestSuite) TestHandlerStopIgnoresError() {


I think TestHandlerStopIgnoresError test became redundant and can be removed. Would it be ok with you @vikin91 ?

sensor/admission-control/manager/manager_impl.go

msugakov · 2022-11-30T23:01:19Z

compliance/collection/auditlog/auditlog_impl.go

-	if s.stopC.IsDone() {
-		return false, errors.New("Cannot start reader because stopC is already done. Reader might have already been stopped")
-	}


This was redundant because nothing in the current code tries to stop the reader before it's started.

msugakov · 2022-11-30T23:02:39Z

compliance/collection/auditlog/auditlog_impl.go

-	return s.stopC.Signal()
+func (s *auditLogReaderImpl) StopReader() {
+	s.stopper.Client().Stop()
+	_ = s.stopper.Client().Stopped().Wait()


Absence of this .Wait() line lead to the following error appear from time to time in logs when running tests.

collection/auditlog: 2022/11/30 12:19:56.751189 auditlog_impl.go:158: Error: Audit log tailer stopped with an error no such file or directory

Looks like I fixed a bug.

msugakov · 2022-11-30T23:03:33Z

compliance/collection/auditlog/auditlog.go

 	// Returns true if the reader can be started (log exists and can be read). Log file missing is not considered an error.
 	StartReader(ctx context.Context) (bool, error)
-	// StopReader will stop the reader if it's started. Will return false if it was already stopped.
-	StopReader() bool


There was nothing that looks at the returned value of StopReader().

pkg/concurrency/stopper.go

sensor/common/sensor/central_communication.go

pkg/concurrency/stopper_test.go

fredrb

Code looks good. Some general comments and thoughts. No blockers.

The distinction between Client and Flow is more clear now on the second pass.

However, I have one last optional suggestion: I wonder if there's a way to enforce Client/Flow distinction (at least as a code pattern) in a way that avoid misuse of the Stopper interface inside the goroutine? For other dev that wants to build a new sensor component the distinction might not be so obvious.

We don't want the code inside the component's goroutine to call stopper.Client() functions, only stopper.Flow(), right? So maybe the goroutine should be anonymous and receive only what it should operate on?

Something like:

func (c *MyComponent) Start() {
    // do pre-work
    go func(flow concurrency.Flow, dependency T) {
           // Component main loop
    }(c.stopper.Flow(), someOtherDependency)
}

Maybe out of the scope for this PR. WDYT?

central/pruning/pruning.go

sensor/admission-control/manager/manager_impl.go

pkg/concurrency/stopper.go

fredrb · 2022-12-08T18:27:30Z

sensor/common/component.go

 type SensorComponent interface {
 	Start() error
-	Stop(err error)
+	Stop(err error) // TODO: get rid of err argument as it always seems to be effectively nil.


sensor/common/detector/detector.go

msugakov · 2022-12-21T21:50:16Z

@fredrb Sorry, I missed your suggestion previously.

I wonder if there's a way to enforce Client/Flow distinction (at least as a code pattern)

That's the ideal state I wanted to get it to but unfortunately couldn't find a way. Mainly the reason is that we use Stopper in a struct as a field and so every struct's function has access to everything in the Stopper. Even in the snippet you provide

func (c *MyComponent) Start() {
    // do pre-work
    go func(flow concurrency.Flow, dependency T) {
           // Component main loop
    }(c.stopper.Flow(), someOtherDependency)
}

there is c.stopper and goroutine can use that instead of flow. If some things can be done differently, humans will do them differently. The ideal state would be if there's only a single way and it hides things that should not be seen, but, as I mentioned, I did not find it.

We don't want the code inside the component's goroutine to call stopper.Client() functions, only stopper.Flow(), right?

For me it is actually the opposite. We don't want anything outside of goroutine to use stopper.Flow(). Not having stopper.Client() in goroutine would be nice, but there are some cases that violate that, e.g. sensor/common/sensor/central_receiver_impl.go.

msugakov · 2023-01-02T12:12:24Z

/retest

instead of its own stoppable implementation.

This has been a bit more involved and requires more thorough review.

although there's nothing that calls `Stop` except of tests and I'm not sure that counts.

msugakov requested review from jschnath and vikin91 November 28, 2022 21:47

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

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

msugakov requested a review from a team November 28, 2022 21:48

vikin91 reviewed Nov 29, 2022

View reviewed changes

msugakov requested a review from a team November 29, 2022 09:19

msugakov force-pushed the misha/stoppable-v2 branch from 9372aad to 6209b25 Compare November 29, 2022 10:58

msugakov changed the title ~~(WIP) Refactor existing Stopper to Stoppable~~ ROX-13661: (WIP) Refactor existing Stopper to Stoppable Nov 29, 2022

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

msugakov changed the title ~~ROX-13661: (WIP) Refactor existing Stopper to Stoppable~~ ROX-13661: (WIP) Refactor Stopper and use it more Nov 29, 2022

msugakov force-pushed the misha/stoppable-v2 branch from 6209b25 to fb528ff Compare November 29, 2022 11:50

vikin91 reviewed Nov 29, 2022

View reviewed changes

msugakov force-pushed the misha/stoppable-v2 branch 2 times, most recently from 32651d4 to dcb444a Compare November 29, 2022 18:29

msugakov commented Nov 29, 2022

View reviewed changes

sensor/common/sensor/sensor.go Show resolved Hide resolved

msugakov commented Nov 29, 2022

View reviewed changes

msugakov force-pushed the misha/stoppable-v2 branch from c2e3c6f to 5ea9da9 Compare November 30, 2022 10:38

github-actions bot added the area/central label Nov 30, 2022

msugakov force-pushed the misha/stoppable-v2 branch 3 times, most recently from 2f3dd15 to 3105f6a Compare November 30, 2022 22:57

github-actions bot added the area/admission-controller label Nov 30, 2022

msugakov commented Nov 30, 2022

View reviewed changes

msugakov requested a review from a team as a code owner December 1, 2022 00:06

msugakov force-pushed the misha/stoppable-v2 branch 2 times, most recently from 6b46a72 to 531b081 Compare December 1, 2022 00:32

fredrb reviewed Dec 1, 2022

View reviewed changes

pkg/concurrency/stopper.go Show resolved Hide resolved

sensor/common/sensor/central_communication.go Show resolved Hide resolved

pkg/concurrency/stopper_test.go Outdated Show resolved Hide resolved

msugakov force-pushed the misha/stoppable-v2 branch from 531b081 to 75bcb57 Compare December 1, 2022 18:41

fredrb reviewed Dec 8, 2022

View reviewed changes

msugakov requested a review from fredrb December 20, 2022 18:09

msugakov force-pushed the misha/stoppable-v2 branch from 597b26a to 18f666f Compare December 20, 2022 18:49

fredrb approved these changes Dec 22, 2022

View reviewed changes

msugakov force-pushed the misha/stoppable-v2 branch from 6c21226 to 2e17e81 Compare December 23, 2022 19:07

msugakov added 20 commits January 2, 2023 13:53

Refactor Stopper API

f17fe43

Add tests for Stopper

d09c9fc

Refresh code that's already using Stopper to the new API

237be30

Migrate Compliance command handler from stopC/stoppedC to Stopper

07ad2f0

Migrate AdmCtrlMsgForwarder to use Stopper instead of a signal

4afdef5

Migrate CentralCommunication to Stopper

fe7670c

Add a note that SensorComponent's Stop error parameter is unused

1e200ff

Migrate CentralReciever to use Stopper

a3f16c9

Migrate CentralSender to Stopper

7ffb274

Migrate Enforcer to Stopper

69941fb

Migrate NodeInventoryHandler tests to use Stopper

0a04b88

instead of its own stoppable implementation.

Migrate NodeInventoryHandler to use Stopper

591525e

Migrate auditlog Reader to Stopper

82bf205

This has been a bit more involved and requires more thorough review.

Migrate CVEUnsuppressLoop to Stopper

b014aaf

Migrate admission-control Manager to Stopper

a4b295d

Migrate reports Scheduler to Stopper

1957bd6

Migrate Prunning to Stopper

2a6ed0f

Migrate processindicator Datastore to Stopper

972ced0

although there's nothing that calls `Stop` except of tests and I'm not sure that counts.

Migrate vulnerabilityrequest Manager to Stopper

e2971c0

Make admission control manager start without error

46adb80

msugakov force-pushed the misha/stoppable-v2 branch from 2e17e81 to 46adb80 Compare January 2, 2023 12:53

msugakov merged commit b976089 into master Jan 2, 2023

msugakov deleted the misha/stoppable-v2 branch January 2, 2023 17:45

Conversation

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

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Nov 28, 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!

vikin91 left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredrb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

msugakov commented Dec 21, 2022

Uh oh!

msugakov commented Jan 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msugakov commented Nov 28, 2022 •

edited

Loading

ghost commented Nov 28, 2022 •

edited by ghost

Loading

msugakov Nov 30, 2022 •

edited

Loading

msugakov Nov 30, 2022 •

edited

Loading

fredrb left a comment •

edited

Loading