⚡ discover assets in parallel #4973

afiune · 2024-12-11T22:50:28Z

Depends on #4970

This PR is using the new workerpool package to discover assets in parallel.

There were a few places that I detected race conditions that I have fixed, I
also added a race detector make command which is running as a CI job.

Testing

Using the github provider for testing, scanning an organization that has around 3k repositories.

Before (~15 Minutes)

TRC logger.FuncDur> func=explorer.discoverAssets took=890303.455042

After (~2 minutes)

TRC logger.FuncDur> func=explorer.discoverAssets took=124293.542166

Race Detector

You can now run make race/go to check for race conditions.

$ make race/go
go test -race go.mondoo.com/cnquery/v11/internal/workerpool
ok  	go.mondoo.com/cnquery/v11/internal/workerpool	16.417s
go test -race go.mondoo.com/cnquery/v11/explorer/scan
ok  	go.mondoo.com/cnquery/v11/explorer/scan	2.487s

Signed-off-by: Salim Afiune Maya <[email protected]>

This will help us submit as many requests as we want without knowing about the workers. Signed-off-by: Salim Afiune Maya <[email protected]>

github-actions · 2024-12-11T22:58:33Z

Test Results

3 169 tests ±0 3 168 ✅ ±0 1m 43s ⏱️ -1s
373 suites ±0 1 💤 ±0
28 files ±0 0 ❌ ±0

Results for commit ceb2585. ± Comparison against base commit 949f0ce.

♻️ This comment has been updated with latest results.

Signed-off-by: Salim Afiune Maya <[email protected]>

jaym · 2024-12-12T15:23:30Z

.vscode/launch.json

@@ -198,6 +198,18 @@
        "shell", "ssh", "[email protected]",
      ],
    },
+    {
+      "name": "scan github org",


did you mean to keep this in here

Yes. I think it is useful for folks testing, specially now that we have https://github.com/hit-training.

I can remove it.

afiune · 2024-12-12T16:12:46Z

You can now run make race/go

$ make race/go
go test -race go.mondoo.com/cnquery/v11/internal/workerpool
ok  	go.mondoo.com/cnquery/v11/internal/workerpool	16.417s
go test -race go.mondoo.com/cnquery/v11/explorer/scan
ok  	go.mondoo.com/cnquery/v11/explorer/scan	2.487s

I'll add a CI job for it.

Signed-off-by: Salim Afiune Maya <[email protected]>

…el_asset_discovery Signed-off-by: Salim Afiune Maya <[email protected]>

We really don't need to do everything inside the workerpool, do we? Signed-off-by: Salim Afiune Maya <[email protected]>

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune · 2024-12-14T21:47:45Z

internal/workerpool/pool.go

@@ -92,7 +116,7 @@ func (p *Pool[R]) Close() {

 // Wait waits until all tasks have been processed.
 func (p *Pool[R]) Wait() {
-	ticker := time.NewTicker(100 * time.Millisecond)
+	ticker := time.NewTicker(10 * time.Millisecond)


This fixed the benchmark failure.

afiune · 2024-12-14T21:48:17Z

providers/github/connection/connection.go

@@ -74,6 +74,7 @@ func NewGithubConnection(id uint32, asset *inventory.Asset) (*GithubConnection,
 	ctx := context.WithValue(context.Background(), github.SleepUntilPrimaryRateLimitResetWhenRateLimited, true)

 	// perform a quick call to verify the token's validity.
+	// @afiune do we need to validate the token for every connection? can this be a "once" operation?


This is fixed with #4980

afiune · 2024-12-14T21:49:00Z

providers/runtime.go

 	r.Provider.Connection, r.Provider.ConnectionError = r.Provider.Instance.Plugin.Connect(req, &callbacks)
+	r.mu.Unlock()


The problem is the assignation here, not the actual gRPC call Connect(). I will change this.

✅ updated

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune added 3 commits December 10, 2024 23:08

⚡ workerpool package to submit parallel requests

42f0627

Signed-off-by: Salim Afiune Maya <[email protected]>

⚡ fetch org repositories in parallel

5ae3d75

Signed-off-by: Salim Afiune Maya <[email protected]>

⚙️ add a collector to the workerpool

93ca08d

This will help us submit as many requests as we want without knowing about the workers. Signed-off-by: Salim Afiune Maya <[email protected]>

afiune marked this pull request as draft December 11, 2024 23:01

🚨 fix race conditions

4765ace

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune force-pushed the afiune/parallel_asset_discovery branch from 35b1eff to f76eda1 Compare December 12, 2024 02:07

⚡ discover assets in parallel

8c98904

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune force-pushed the afiune/parallel_asset_discovery branch from f76eda1 to 8c98904 Compare December 12, 2024 07:28

🧪 decrease workerpool wait ticker to 10ms

b3dc6d8

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune marked this pull request as ready for review December 12, 2024 14:22

🐛 make DiscoveredAssets.AddError() thread safe

17f7beb

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune requested review from jaym and chris-rock December 12, 2024 15:14

jaym reviewed Dec 12, 2024

View reviewed changes

jaym approved these changes Dec 12, 2024

View reviewed changes

Base automatically changed from afiune/workerpool+github to main December 12, 2024 17:28

afiune added 2 commits December 12, 2024 09:29

🧵 add mutex when running provider.connect()

842e1e7

Signed-off-by: Salim Afiune Maya <[email protected]>

Merge branch 'main' of github.com:mondoohq/cnquery into afiune/parall…

e785c76

…el_asset_discovery Signed-off-by: Salim Afiune Maya <[email protected]>

afiune force-pushed the afiune/parallel_asset_discovery branch from 8364872 to e785c76 Compare December 12, 2024 17:32

⚙️ reduce the workerpool Task function

e3a58fd

We really don't need to do everything inside the workerpool, do we? Signed-off-by: Salim Afiune Maya <[email protected]>

afiune force-pushed the afiune/parallel_asset_discovery branch from 81de208 to e3a58fd Compare December 12, 2024 18:41

afiune requested a review from imilchev December 12, 2024 19:11

afiune mentioned this pull request Dec 12, 2024

⭐️ plugin cache + verify github conn once #4976

Closed

⚙️ return pool.Result as a combined struct

4ffb815

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune commented Dec 14, 2024

View reviewed changes

🏎️ fix more data race conditions

842f60e

Signed-off-by: Salim Afiune Maya <[email protected]>

🤖 Run race detector on CI

8badda7

Signed-off-by: Salim Afiune Maya <[email protected]>

afiune force-pushed the afiune/parallel_asset_discovery branch from 17495df to 8badda7 Compare December 15, 2024 09:41

⚙️ split plugin connect func and assignation

ceb2585

Signed-off-by: Salim Afiune Maya <[email protected]>

imilchev approved these changes Dec 16, 2024

View reviewed changes

afiune merged commit fee2739 into main Dec 16, 2024
17 checks passed

afiune deleted the afiune/parallel_asset_discovery branch December 16, 2024 09:12

github-actions bot locked and limited conversation to collaborators Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚡ discover assets in parallel #4973

⚡ discover assets in parallel #4973

afiune commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

jaym Dec 12, 2024

afiune Dec 12, 2024

afiune commented Dec 12, 2024

afiune Dec 14, 2024

afiune Dec 14, 2024

afiune Dec 14, 2024

afiune Dec 15, 2024

		r.Provider.Connection, r.Provider.ConnectionError = r.Provider.Instance.Plugin.Connect(req, &callbacks)
		r.mu.Unlock()

⚡ discover assets in parallel #4973

⚡ discover assets in parallel #4973

Conversation

afiune commented Dec 11, 2024 • edited Loading

Testing

Before (~15 Minutes)

After (~2 minutes)

Race Detector

github-actions bot commented Dec 11, 2024 • edited Loading

Test Results

jaym Dec 12, 2024

Choose a reason for hiding this comment

afiune Dec 12, 2024

Choose a reason for hiding this comment

afiune commented Dec 12, 2024

afiune Dec 14, 2024

Choose a reason for hiding this comment

afiune Dec 14, 2024

Choose a reason for hiding this comment

afiune Dec 14, 2024

Choose a reason for hiding this comment

afiune Dec 15, 2024

Choose a reason for hiding this comment

afiune commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading