-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[check command] Add --instance-filter
option
#15034
Changes from 13 commits
d640c30
9e76794
9556540
9f45c3b
a989b57
40fe201
723f9cc
9c2894d
1df1f6f
7ac006a
937bd99
8027fec
a0ca08e
89534fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ package common | |
|
||
import ( | ||
"context" | ||
"fmt" | ||
"time" | ||
|
||
"go.uber.org/atomic" | ||
|
@@ -18,6 +19,7 @@ import ( | |
"github.com/DataDog/datadog-agent/pkg/autodiscovery/scheduler" | ||
"github.com/DataDog/datadog-agent/pkg/config" | ||
confad "github.com/DataDog/datadog-agent/pkg/config/autodiscovery" | ||
"github.com/DataDog/datadog-agent/pkg/util/jsonquery" | ||
"github.com/DataDog/datadog-agent/pkg/util/log" | ||
) | ||
|
||
|
@@ -211,14 +213,14 @@ func (sf schedulerFunc) Stop() { | |
// | ||
// If the context is cancelled, then any accumulated, matching changes are | ||
// returned, even if that is fewer than discoveryMinInstances. | ||
func WaitForConfigsFromAD(ctx context.Context, checkNames []string, discoveryMinInstances int) (configs []integration.Config) { | ||
return waitForConfigsFromAD(ctx, false, checkNames, discoveryMinInstances) | ||
func WaitForConfigsFromAD(ctx context.Context, checkNames []string, discoveryMinInstances int, instanceFilter string) (configs []integration.Config, lastError error) { | ||
return waitForConfigsFromAD(ctx, false, checkNames, discoveryMinInstances, instanceFilter) | ||
} | ||
|
||
// WaitForAllConfigsFromAD waits until its context expires, and then returns | ||
// the full set of checks scheduled by AD. | ||
func WaitForAllConfigsFromAD(ctx context.Context) (configs []integration.Config) { | ||
return waitForConfigsFromAD(ctx, true, []string{}, 0) | ||
func WaitForAllConfigsFromAD(ctx context.Context) (configs []integration.Config, lastError error) { | ||
return waitForConfigsFromAD(ctx, true, []string{}, 0, "") | ||
} | ||
|
||
// waitForConfigsFromAD waits for configs from the AD scheduler and returns them. | ||
|
@@ -234,7 +236,7 @@ func WaitForAllConfigsFromAD(ctx context.Context) (configs []integration.Config) | |
// If wildcard is true, this gathers all configs scheduled before the context | ||
// is cancelled, and then returns. It will not return before the context is | ||
// cancelled. | ||
func waitForConfigsFromAD(ctx context.Context, wildcard bool, checkNames []string, discoveryMinInstances int) (configs []integration.Config) { | ||
func waitForConfigsFromAD(ctx context.Context, wildcard bool, checkNames []string, discoveryMinInstances int, instanceFilter string) (configs []integration.Config, returnErr error) { | ||
configChan := make(chan integration.Config) | ||
|
||
// signal to the scheduler when we are no longer waiting, so we do not continue | ||
|
@@ -265,10 +267,24 @@ func waitForConfigsFromAD(ctx context.Context, wildcard bool, checkNames []strin | |
} | ||
} | ||
|
||
stopChan := make(chan struct{}) | ||
// add the scheduler in a goroutine, since it will schedule any "catch-up" immediately, | ||
// placing items in configChan | ||
go AC.AddScheduler("check-cmd", schedulerFunc(func(configs []integration.Config) { | ||
for _, cfg := range configs { | ||
if instanceFilter != "" { | ||
instances, err := filterInstances(cfg.Instances, instanceFilter) | ||
if err != nil { | ||
returnErr = err | ||
stopChan <- struct{}{} | ||
break | ||
} | ||
if len(instances) == 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we want to exclude config with no instances here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Kaderinho thanks for the review. In this case, note that we are here inside the If there is no instances left after the filter being applied I think there is no need to return the config with no instances since the goal for Maybe I'm missing something, please let me know :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
AFAIK, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay I get it sorry, I missed the fact that you use instanceFilter = "" in |
||
continue | ||
} | ||
cfg.Instances = instances | ||
} | ||
|
||
if match(cfg) && waiting.Load() { | ||
configChan <- cfg | ||
} | ||
|
@@ -279,9 +295,25 @@ func waitForConfigsFromAD(ctx context.Context, wildcard bool, checkNames []strin | |
select { | ||
case cfg := <-configChan: | ||
configs = append(configs, cfg) | ||
case <-stopChan: | ||
return | ||
case <-ctx.Done(): | ||
return | ||
} | ||
} | ||
return | ||
} | ||
|
||
func filterInstances(instances []integration.Data, instanceFilter string) ([]integration.Data, error) { | ||
var newInstances []integration.Data | ||
for _, instance := range instances { | ||
exist, err := jsonquery.YAMLCheckExist(instance, instanceFilter) | ||
if err != nil { | ||
return nil, fmt.Errorf("instance filter error: %v", err) | ||
} | ||
if exist { | ||
newInstances = append(newInstances, instance) | ||
} | ||
} | ||
return newInstances, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Unless explicitly stated otherwise all files in this repository are licensed | ||
// under the Apache License Version 2.0. | ||
// This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
// Copyright 2023-present Datadog, Inc. | ||
|
||
package jsonquery | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
|
||
"github.com/DataDog/datadog-agent/pkg/autodiscovery/integration" | ||
) | ||
|
||
func TestYAMLExistQuery(t *testing.T) { | ||
exist, err := YAMLCheckExist(integration.Data("{\"ip_address\": \"127.0.0.50\"}"), ".ip_address == \"127.0.0.50\"") | ||
assert.NoError(t, err) | ||
assert.True(t, exist) | ||
|
||
exist, err = YAMLCheckExist(integration.Data("{\"ip_address\": \"127.0.0.50\"}"), ".ip_address == \"127.0.0.99\"") | ||
assert.NoError(t, err) | ||
assert.False(t, exist) | ||
|
||
exist, err = YAMLCheckExist(integration.Data("{\"ip_address\": \"127.0.0.50\"}"), ".ip_address") | ||
assert.EqualError(t, err, "filter query must return a boolean: yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `127.0.0.50` into bool") | ||
assert.False(t, exist) | ||
|
||
exist, err = YAMLCheckExist(integration.Data("{}"), ".ip_address == \"127.0.0.99\"") | ||
assert.NoError(t, err) | ||
assert.False(t, exist) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# Each section from every release note are combined when the | ||
# CHANGELOG.rst is rendered. So the text needs to be worded so that | ||
# it does not depend on any information only available in another | ||
# section. This may mean repeating some details, but each section | ||
# must be readable independently of the other. | ||
# | ||
# Each section note must be formatted as reStructuredText. | ||
--- | ||
enhancements: | ||
- | | ||
Add an ``--instance-filter`` option to the Agent check command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to get a list of error, and return an aggregated view of the errors.
we are using in several place https://pkg.go.dev/k8s.io/apimachinery/pkg/util/errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clamoriniere Thanks for the suggestion. I modified the code to return an aggregated view of the errors here: 89534fd
Let me know if there is anything else to improve.