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

ROX-21387: batch insert vulns #1345

Merged
merged 1 commit into from
Dec 12, 2023
Merged

ROX-21387: batch insert vulns #1345

merged 1 commit into from
Dec 12, 2023

Conversation

RTann
Copy link
Collaborator

@RTann RTann commented Dec 8, 2023

$ go test -run=^$ -bench=^BenchmarkLoadOSVulnsFromDump$ -benchmem ./pkg/vulndump/
goos: darwin
goarch: amd64
pkg: github.com/stackrox/scanner/pkg/vulndump
cpu: Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
BenchmarkLoadOSVulnsFromDump-8   	       1	4358242218 ns/op	1314951824 B/o 8285803 allocs/op
--- BENCH: BenchmarkLoadOSVulnsFromDump-8
    loader_test.go:26: Loaded 134043 vulns
PASS
ok  	github.com/stackrox/scanner/pkg/vulndump	5.990s



$ go test -run=^$ -bench=^BenchmarkOSLoader$ -benchmem ./pkg/vulndump/
goos: darwin
goarch: amd64
pkg: github.com/stackrox/scanner/pkg/vulndump
cpu: Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
BenchmarkOSLoader-8   	       1	4075758627 ns/op	702698568 B/op	 8285718 allocs/op
--- BENCH: BenchmarkOSLoader-8
    batch_loader_test.go:57: Loaded 134043 vulns
PASS
ok  	github.com/stackrox/scanner/pkg/vulndump	5.450s

The batched implementation uses a little over half of the memory/op than the original implementation.

I also ran an experiment where I ran the current implementation and the implementation from this PR. In each separate cluster, I ran StackRox in offline-mode, and I uploaded the latest "offline" vuln dump to Central. From there I found the current implementation stayed at around 3.5G for about 20 minutes, while this PR's implementation hovered around 700MB for a few minutes before going back down to about 500MB, which is its steady-state. This is a rather significant memory reduction.

@ghost
Copy link

ghost commented Dec 8, 2023

Images are ready for the commit at 748bc9e.

To use the images, use the tag 2.31.x-64-g748bc9ee93.

@RTann RTann force-pushed the ross/batch-insert branch 6 times, most recently from 6cc4f18 to 748bc9e Compare December 12, 2023 06:52
Copy link

@ludydoo ludydoo left a comment

Choose a reason for hiding this comment

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

Amazing! 🥇

@RTann RTann changed the title chore: batch insert vulns ROX-21387: batch insert vulns Dec 12, 2023
@RTann
Copy link
Collaborator Author

RTann commented Dec 12, 2023

Current Scanner loading all vulns (offline-mode):
control

This implementation loading all vulns (offline-mode):
experiment

You'll find the current Scanner requires about 3.5G while this implementation only requires ~700MB.

Copy link
Contributor

@dcaravel dcaravel left a comment

Choose a reason for hiding this comment

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

LGTM, wondering if its worth running some queries against the DB just to double check that the # of vulns loaded before/after this fix are the same, and perhaps also query the table for total size / bytes consumed - if they are the same, ship it!

@RTann RTann force-pushed the ross/batch-insert branch from 748bc9e to 58d5753 Compare December 12, 2023 21:54
@RTann
Copy link
Collaborator Author

RTann commented Dec 12, 2023

I added a benchmark test for the offline dumps, too:

$ go test -run=^$ -bench=^BenchmarkLoadOSVulnsFromDump -benchmem ./pkg/vulndump/
goos: darwin
goarch: amd64
pkg: github.com/stackrox/scanner/pkg/vulndump
cpu: Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
BenchmarkLoadOSVulnsFromDump-8           	       1	4300235287 ns/op	1315103992 B/op	 8293226 allocs/op
--- BENCH: BenchmarkLoadOSVulnsFromDump-8
    loader_test.go:38: Loaded 134212 vulns
BenchmarkLoadOSVulnsFromDump_Offline-8   	       1	13470219395 ns/op	4659625192 B/op	26622319 allocs/op
--- BENCH: BenchmarkLoadOSVulnsFromDump_Offline-8
    loader_test.go:38: Loaded 415338 vulns
PASS
ok  	github.com/stackrox/scanner/pkg/vulndump	22.328s


$ go test -run=^$ -bench=^BenchmarkOSLoader -benchmem ./pkg/vulndump/
goos: darwin
goarch: amd64
pkg: github.com/stackrox/scanner/pkg/vulndump
cpu: Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
BenchmarkOSLoader-8           	       1	3906161039 ns/op	702851656 B/op	 8293140 allocs/op
--- BENCH: BenchmarkOSLoader-8
    batch_loader_test.go:109: Loaded 134212 vulns
BenchmarkOSLoader_Offline-8   	       1	12385573250 ns/op	2255503144 B/op	26622223 allocs/op
--- BENCH: BenchmarkOSLoader_Offline-8
    batch_loader_test.go:109: Loaded 415338 vulns
PASS
ok  	github.com/stackrox/scanner/pkg/vulndump	20.754s

This also prints out the number of vulnerabilities we read from the JSON file, and they both match. I will merge with CI

@RTann RTann enabled auto-merge (squash) December 12, 2023 21:57
Copy link

openshift-ci bot commented Dec 12, 2023

@RTann: 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/e2e-tests 748bc9e link false /test e2e-tests

Full PR test history. Your PR dashboard.

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

@RTann RTann force-pushed the ross/batch-insert branch from 58d5753 to 6e461eb Compare December 12, 2023 22:07
@RTann RTann merged commit eeb37b5 into master Dec 12, 2023
7 of 11 checks passed
@RTann RTann deleted the ross/batch-insert branch December 12, 2023 22:22
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.

3 participants