-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ISSUE-1830: Adding support for stopping the scan #98
Conversation
@@ -68,9 +68,11 @@ func StartStatusReporter(ctx context.Context, scanCtx *scan.ScanContext) chan er | |||
threshold := *opts.InactiveThreshold | |||
go func() { | |||
defer stopScanJob() | |||
ticker := time.NewTicker(30 * time.Second) | |||
ticker := time.NewTicker(1 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want to be that quick about cancelling, keeping 30 seconds would simplify the following logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I increased the granularity to 1 second as this gives us a chance to check the "Stopped" flag more frequently
Else we will be checking this flag once per 30 seconds .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, my point is I don't see any problem with use checking once per 30 seconds?
It simplifies the code and free some CPU usage
jobs/status.go
Outdated
return | ||
} | ||
if err != nil { | ||
} else if scanCtx.Stopped.Load() == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have abort
but it does not seem to be used. Can we remove the if abort != nil
entirely?
If not, might be good to merge it with scanCtx.Stopped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have these 2 flags on the scan job as a signal for 2 different activities:
-
Abort: This signals the case when the scan has been aborted due to lack of activity.
We set this in the StatusChecker routine to signal that we need to exit the scan.
We use this flag in the below to exit the running scan:
SecretScanner/scan/process_image.go
Line 825 in 60dfd9c
if scanCtx.Aborted.Load() == true { -
Stopped: This signals that the scan was stopped by api.
We set this in the "StopScan" grpc request
We use this flag in the below to exit the running scan.
SecretScanner/scan/process_image.go
Line 829 in 60dfd9c
} else if scanCtx.Stopped.Load() == true {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I have made a mistake in the if checks in the end of StatusChecker routine... fixed and pushed the change.
464341b
to
b2f6760
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, as discussed raised points will be solved in upcoming PR
88b8760
to
c72d36a
Compare
Changes to add support for Stopping the scans.
These changes are done for the below issue:
https://github.com/deepfence/enterprise-roadmap/issues/1830