Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snyk: wrap snyk invocation by timeout(1) by default #127

Closed
wants to merge 1 commit into from

Conversation

kdudka
Copy link
Member

@kdudka kdudka commented Oct 6, 2023

... set to 5 hours so that Snyk does not hang forever and does not block OpenScanHub scanning queue. If Snyk does not deliver results within the first 5 hours, it usually hangs forever.

The amount of time can be tweaked by the --snyk-timeout option of csmock or disabled by --snyk-timeout=0.

Related: https://issues.redhat.com/browse/OSH-301

@kdudka kdudka requested a review from jperezdealgaba October 6, 2023 06:32
@kdudka kdudka self-assigned this Oct 6, 2023
kdudka added a commit to kdudka/csmock that referenced this pull request Oct 6, 2023
... set to 5 hours so that Snyk does not hang forever and does not block
OpenScanHub scanning queue.  If Snyk does not deliver results within the
first 5 hours, it usually hangs forever.

The amount of time can be tweaked by the `--snyk-timeout` option of csmock
or disabled by `--snyk-timeout=0`.

Related: https://issues.redhat.com/browse/OSH-301
Closes: csutils#127
... set to 5 hours so that Snyk does not hang forever and does not block
OpenScanHub scanning queue.  If Snyk does not deliver results within the
first 5 hours, it usually hangs forever.

The amount of time can be tweaked by the `--snyk-timeout` option of csmock
or disabled by `--snyk-timeout=0`.

Related: https://issues.redhat.com/browse/OSH-301
Closes: csutils#127
@@ -32,6 +32,9 @@

FILTER_CMD = f"csgrep '%s' --mode=json --prepend-path-prefix={SNYK_SCAN_DIR}/ > '%s'"

# default value for the maximum amount of time taken by invocation of Snyk (5 hours)
DEFAULT_SNYK_TIMEOUT=18000
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdudka Hey Kamil. I don't know if because of the workers, the scans are slower. But when I have been testing this, more than 2 hours means it is going to get stuck. Just in case you want to reduce this value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was my observation as well when I tested Snyk initially (although the scans were never hanging when I was experimenting with the Snyk client interactively). But now we are increasing the load on the 3rd party scanning service. Theoretically up to 40 Snyk scans can now run in parallel in OpenScanHub. So I wanted to reserve some extra time to avoid failing OSH tasks just because a scan is temporarily queued on the remote service.

The 5 hours timeout is really a workaround to avoid manual intervention in the edge case where the Snyk client keeps hanging forever. This should not happen and the vendor should fix the client to terminate itself in such cases because the client has more information to decide whether it makes sense to wait longer or not. For example, if the remote scanning process crashes, it would be wasteful to wait even for 2 hours before we terminate the client. However, if the scan is just temporarily queued on the remote service, then waiting for 3 hours might be better for us than failing the OSH task and having to manually investigate what happened.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool then. I have used the same timout for cspodman

Copy link
Collaborator

@jperezdealgaba jperezdealgaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@kdudka kdudka closed this in fc15afb Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants