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

DiscoveryConfig Status: update even when no resources are found #49588

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcoandredinis
Copy link
Contributor

The DiscoveryService was not updating the DiscoveryConfigStatus when its matchers didn't discovered any resource.

This would lead the user to think that there the DiscoveryConfig was not being processed, when in fact it was.

Demo:
$ tctl get discovery_config | yq .status

discovered_resources: 0
last_sync_time: "2024-11-29T16:45:54.51056Z"
state: DISCOVERY_CONFIG_STATE_SYNCING
---
discovered_resources: 0
last_sync_time: "2024-11-29T17:17:59.196886Z"
state: DISCOVERY_CONFIG_STATE_SYNCING
---
discovered_resources: 0
last_sync_time: "2024-11-29T17:17:59.196586Z"
state: DISCOVERY_CONFIG_STATE_SYNCING
---
discovered_resources: 0
last_sync_time: "2024-11-29T16:45:54.760152Z"
state: DISCOVERY_CONFIG_STATE_SYNCING

@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 labels Nov 29, 2024
@github-actions github-actions bot requested review from atburke and r0mant November 29, 2024 17:19
@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch from d292bd2 to a07f1bb Compare December 2, 2024 10:59
@marcoandredinis
Copy link
Contributor Author

@atburke @r0mant Can you please take a look when you get a chance?

lib/utils/slices/slices.go Outdated Show resolved Hide resolved
@@ -97,7 +97,7 @@ func NewAzureWatcher(ctx context.Context, fetchersFn func() []Fetcher, opts ...O
}

// MatchersToAzureInstanceFetchers converts a list of Azure VM Matchers into a list of Azure VM Fetchers.
func MatchersToAzureInstanceFetchers(matchers []types.AzureMatcher, clients azureClientGetter) []Fetcher {
func MatchersToAzureInstanceFetchers(matchers []types.AzureMatcher, clients azureClientGetter, discoveryConfig string) []Fetcher {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tbh I'm not a huge fan of the fact that we need to propagate this discovery config as an extra parameter everywhere. Seems very tightly coupled. Is there a cleaner way to achieve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DiscoveryConfig contains Matchers, which create Fetchers.
Fetchers find resources and enrolls them. When it fails, we must update the DiscoveryConfigStatus / UserTasks and we need the DiscoveryConfig name.

So, what I'm doing is adding the DiscoveryConfig name into the Fetcher, so that when it tries to auto-enroll resources and it fails, it is able to identify which DiscoveryConfig was used.

I don't think there's a way to achieve this without passing the DiscoveryConfig name into the Fetcher.
We can create a new type that includes the DiscoveryConfigName as well as the Matcher. Something like this:

type MatcherFromDiscoveryConfig struct {
    DiscoveryConfigName string
    AzureMatcher types.AzureMatcher
    AWSMatcher types.AWSMatcher
    GCPMatcher types.GCPMatcher
}

And when creating the Fetcher, we would get the DiscoveryConfigName from this new type, instead of as a param.
I'm not too fond of this, and would rather keep it as is.
Please let me know what you think.

lib/srv/server/azure_watcher.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch from a07f1bb to 5d4aa50 Compare December 5, 2024 10:05
@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch from 26f8d4f to 826f17a Compare December 6, 2024 11:05
lib/srv/discovery/database_watcher.go Outdated Show resolved Hide resolved
lib/utils/slices/slices_test.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch 2 times, most recently from e649705 to f022c26 Compare December 10, 2024 11:37
@marcoandredinis
Copy link
Contributor Author

@r0mant Can I please get another pass on this one? (next week is fine)

@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch from f022c26 to df03a6e Compare December 16, 2024 10:07
The DiscoveryService was not updating the DiscoveryConfigStatus when its
matchers didn't discovered any resource.

This would lead the user to think that there the DiscoveryConfig was not
being processed, when in fact it was.
@marcoandredinis marcoandredinis force-pushed the marco/discoveryconfig_status_report_nonmatching_rules branch from df03a6e to ac1d330 Compare December 16, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 discovery no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants