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

[target-allocator] Introduce "per node" allocation strategy to target allocator #2430

Merged
merged 38 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
6defa78
Add node name to Collector struct
matej-g Dec 5, 2023
318fd6c
Add per node allocation algorithm
matej-g Dec 5, 2023
d298b69
More docs
matej-g Dec 7, 2023
886a80c
Add tests
matej-g Dec 7, 2023
23089e3
Update APIs and docs with new strategy
matej-g Dec 7, 2023
7f82f69
Add changelog
matej-g Dec 8, 2023
959c3c5
Fix lint
matej-g Dec 8, 2023
082e678
Add more labels to match node name
matej-g Dec 8, 2023
2d29ae6
Better handling of unassignable targets / jobs
matej-g Dec 18, 2023
c08f560
Adjust webhook validation
matej-g Dec 18, 2023
8c0f9e6
Add E2E test; put TA E2E tests into separate target
matej-g Dec 18, 2023
5ad0079
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Dec 18, 2023
acf7644
Format; fix mistakenly changed test image
matej-g Dec 18, 2023
57d23bd
Use node name instead of collector name for collector map
matej-g Dec 19, 2023
0e09fc8
Add TA E2E tests to GH action
matej-g Dec 19, 2023
0c8239e
Merge branch 'main' into per-node-strategy
matej-g Dec 19, 2023
9c6c6e6
Bump kuttl timeout
matej-g Dec 19, 2023
6d08c78
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Dec 20, 2023
94b336e
Validate mode and strategy
matej-g Jan 4, 2024
023ce4b
Handle cases of missing node name when (re)adding collector
matej-g Jan 4, 2024
113f6bb
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Jan 8, 2024
5976307
Adjust test cases after node name fix
matej-g Jan 8, 2024
fe5e28d
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Jan 9, 2024
3ac78c7
Adjust E2E tests
matej-g Jan 9, 2024
85c66e5
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Jan 9, 2024
0c3fbdc
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Jan 15, 2024
0b3363e
Put TA config validation into separate method
matej-g Jan 15, 2024
02ee921
Adjust logging and add metric for jobs with unassigned targets
matej-g Jan 15, 2024
d2d46c9
Merge branch 'main' into per-node-strategy
matej-g Jan 15, 2024
485d9d6
Merge branch 'main' into per-node-strategy
matej-g Jan 15, 2024
b8befdf
Address feedback
matej-g Jan 18, 2024
f7eeb5e
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Jan 18, 2024
253b340
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Feb 1, 2024
9075a84
Simplify flow according to feedback; correct docs
matej-g Feb 6, 2024
339ed5c
Add more unit tests for edge cases
matej-g Feb 6, 2024
7defcdc
Merge remote-tracking branch 'origin/main' into per-node-strategy
matej-g Feb 6, 2024
5d31f67
Fix test case assert file
matej-g Feb 6, 2024
4f195d5
Merge branch 'main' into per-node-strategy
matej-g Feb 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 43 additions & 31 deletions apis/v1alpha1/collector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,40 +228,14 @@ func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollecto
return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'AdditionalContainers'", r.Spec.Mode)
}

// validate target allocation
if r.Spec.TargetAllocator.Enabled && (r.Spec.Mode != ModeStatefulSet && r.Spec.Mode != ModeDaemonSet) {
return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the target allocation deployment", r.Spec.Mode)
}

if r.Spec.TargetAllocator.Enabled && (r.Spec.Mode == ModeDaemonSet && r.Spec.TargetAllocator.AllocationStrategy != OpenTelemetryTargetAllocatorAllocationStrategyPerNode) {
return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which must be used with target allocation strategy %s ", r.Spec.Mode, OpenTelemetryTargetAllocatorAllocationStrategyPerNode)
}

if r.Spec.TargetAllocator.AllocationStrategy == OpenTelemetryTargetAllocatorAllocationStrategyPerNode && r.Spec.Mode != ModeDaemonSet {
return warnings, fmt.Errorf("target allocation strategy %s is only supported in OpenTelemetry Collector mode %s", OpenTelemetryTargetAllocatorAllocationStrategyPerNode, ModeDaemonSet)
}

// validate Prometheus config for target allocation
// validate target allocator configs
if r.Spec.TargetAllocator.Enabled {
promCfg, err := ta.ConfigToPromConfig(r.Spec.Config)
if err != nil {
return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
err = ta.ValidatePromConfig(promCfg, r.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled())
taWarnings, err := c.validateTargetAllocatorConfig(ctx, r)
if err != nil {
return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
return warnings, err
}
err = ta.ValidateTargetAllocatorConfig(r.Spec.TargetAllocator.PrometheusCR.Enabled, promCfg)
if err != nil {
return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
// if the prometheusCR is enabled, it needs a suite of permissions to function
if r.Spec.TargetAllocator.PrometheusCR.Enabled {
if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil {
return warnings, fmt.Errorf("unable to check rbac rules %w", err)
} else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed {
warnings = append(warnings, warningsGroupedByResource(deniedReviews)...)
}
if taWarnings != nil {
warnings = append(warnings, taWarnings...)
}
}

Expand Down Expand Up @@ -368,6 +342,44 @@ func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollecto
return warnings, nil
}

func (c CollectorWebhook) validateTargetAllocatorConfig(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) {
if r.Spec.Mode != ModeStatefulSet && r.Spec.Mode != ModeDaemonSet {
return nil, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the target allocation deployment", r.Spec.Mode)
}

if r.Spec.Mode == ModeDaemonSet && r.Spec.TargetAllocator.AllocationStrategy != OpenTelemetryTargetAllocatorAllocationStrategyPerNode {
return nil, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which must be used with target allocation strategy %s ", r.Spec.Mode, OpenTelemetryTargetAllocatorAllocationStrategyPerNode)
}

if r.Spec.TargetAllocator.AllocationStrategy == OpenTelemetryTargetAllocatorAllocationStrategyPerNode && r.Spec.Mode != ModeDaemonSet {
return nil, fmt.Errorf("target allocation strategy %s is only supported in OpenTelemetry Collector mode %s", OpenTelemetryTargetAllocatorAllocationStrategyPerNode, ModeDaemonSet)
}

// validate Prometheus config for target allocation
promCfg, err := ta.ConfigToPromConfig(r.Spec.Config)
if err != nil {
return nil, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
err = ta.ValidatePromConfig(promCfg, r.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled())
if err != nil {
return nil, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
err = ta.ValidateTargetAllocatorConfig(r.Spec.TargetAllocator.PrometheusCR.Enabled, promCfg)
if err != nil {
return nil, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
}
// if the prometheusCR is enabled, it needs a suite of permissions to function
if r.Spec.TargetAllocator.PrometheusCR.Enabled {
if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil {
return nil, fmt.Errorf("unable to check rbac rules %w", err)
} else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed {
return warningsGroupedByResource(deniedReviews), nil
}
}

return nil, nil
}

func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error {
if autoscaler.Behavior != nil {
if autoscaler.Behavior.ScaleDown != nil && autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds != nil &&
Expand Down
8 changes: 2 additions & 6 deletions cmd/otel-allocator/allocation/per_node.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,8 @@ func (allocator *perNodeAllocator) handleTargets(diff diff.Changes[*target.Item]

// Check for unassigned targets
if len(unassignedTargetsForJobs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can just become an integer that's incremented on line 186, i don't think there's a need for a map, unless we care about deduping (which should happen already in the filter strategy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if there can be a scenario where we have multiple targets per job, that cannot be assigned. But I see the point, I simplified this to just count the number of targets, that should hopefully give enough info when debugging.

jobs := make([]string, 0, len(unassignedTargetsForJobs))
for j := range unassignedTargetsForJobs {
jobs = append(jobs, j)
}

allocator.log.Info("Could not assign targets for the following jobs due to missing node labels", "jobs", jobs)
allocator.log.Info("Could not assign targets for some jobs due to missing node labels", "jobs", unassignedTargetsForJobs)
RecordJobsWithUnassignedTargets(unassignedTargetsForJobs)
}
}

Expand Down
8 changes: 8 additions & 0 deletions cmd/otel-allocator/allocation/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ var (
Name: "opentelemetry_allocator_targets_remaining",
Help: "Number of targets kept after filtering.",
})
jobsUnassigned = promauto.NewCounter(prometheus.CounterOpts{
Name: "opentelemetry_allocator_jobs_with_unassigned_targets",
Help: "Number of jobs with targets that could not be assigned due to missing node label.",
})
)

type AllocationOption func(Allocator)
Expand All @@ -67,6 +71,10 @@ func RecordTargetsKept(targets map[string]*target.Item) {
targetsRemaining.Add(float64(len(targets)))
}

func RecordJobsWithUnassignedTargets(jobs map[string]struct{}) {
jobsUnassigned.Add(float64(len(jobs)))
}

func New(name string, log logr.Logger, opts ...AllocationOption) (Allocator, error) {
if p, ok := registry[name]; ok {
return p(log.WithValues("allocator", name), opts...), nil
Expand Down