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

🧹 providers coordinator v2 #3218

Closed
wants to merge 5 commits into from
Closed

🧹 providers coordinator v2 #3218

wants to merge 5 commits into from

Conversation

imilchev
Copy link
Member

@imilchev imilchev commented Feb 6, 2024

The goal is to allow creating local coordinators with a parent coordinator. The way the code is setup is:

  • providers are always managed by the parent coordinator. This means eventually all providers should end up in the global coordinator
  • the local coordinator only keeps a reference to the started providers but are not managing them
  • from the perspective of the global coordinator any provider started by a local coordinator is ephemeral. This means the local coordinators have dedicated providers for them. This allows scans to run independently of each other
  • there is no direct dependency from the global coordinator to its children

Signed-off-by: Ivan Milchev <[email protected]>
Signed-off-by: Ivan Milchev <[email protected]>
Copy link
Contributor

github-actions bot commented Feb 6, 2024

Test Results

762 tests  +1   762 ✅ +1   23s ⏱️ -1s
 85 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit 7f1d985. ± Comparison against base commit c4adb50.

♻️ This comment has been updated with latest results.

Signed-off-by: Ivan Milchev <[email protected]>
@imilchev imilchev force-pushed the ivan/coordinator-v2 branch from 298e1c7 to bf90bcd Compare February 6, 2024 15:34
@@ -30,13 +36,26 @@ type AssetWithError struct {

type DiscoveredAssets struct {
platformIds map[string]struct{}
Assets []*AssetWithRuntime
Errors []*AssetWithError
// Assets []*AssetWithRuntime
Copy link
Contributor

Choose a reason for hiding this comment

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

should this line be removed

RootAssets map[*inventory.Asset]*RootAsset
}

func (d *DiscoveredAssets) AddRoot(root *inventory.Asset, coordinator providers.Coordinator, runtime *providers.Runtime) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

runtime isn't used. is that expected?

log.Debug().Str("asset", discoveredAssets.Assets[0].Asset.Name).Msg("Overriding asset name with --asset-name flag")
discoveredAssets.Assets[0].Asset.Name = invAssets[0].Name
}
// if len(discoveredAssets.Assets) == 1 && invAssets[0].Name != "" && invAssets[0].Name != discoveredAssets.Assets[0].Asset.Name {
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code

@arlimus
Copy link
Member

arlimus commented Feb 6, 2024

Thank you for taking a stab at this! We are motivated by random provider shutdowns across the system and by supporting multiple scans at the same time. We'll have a separate call to go over a discussion of this, so we will keep this PR as draft until then.

@arlimus arlimus marked this pull request as draft February 6, 2024 21:30
@imilchev
Copy link
Member Author

we are fixing this by getting rid of ephemeral providers and making sure we have a more robust provider management system. Closing this one

@imilchev imilchev closed this Feb 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants