-
Notifications
You must be signed in to change notification settings - Fork 25
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 #261
Target allocator #261
Conversation
Co-authored-by: Mitali Salvi <[email protected]>
* Ta https server (#2921) * Added https server, tests, secret marshalling --------- Co-authored-by: ItielOlenick <[email protected]>
…247) * edited workflow
* Reconciler now removes un-used managed resources for CWA collector
* added one-service per TA
* TLS tests
* Injecting Prom path if it doesn't exist
* Adding support for NodeJS auto instrumentation and integ tests (#220) * Support configurable resources for NodeJS. (#225) * Supporting JMX annotations (#240) * Add support for a supplemental YAML configuration for the CloudWatchAgent (#241) * Changed naming for OTLP container ports from agent JSON (#252) * Updated Release Notes for 1.8.0 (#251) * Adjust EKS add-on integration test service count expectations (#256) * Add integration tests for JMX. (#250) * Implemented Target Allocator Container (#214) * Implemented TargetAllocator resource deployments. (#208) * Update cmd/amazon-cloudwatch-agent-target-allocator/config/config.go Co-authored-by: Musa <[email protected]> * Update internal/config/main.go Co-authored-by: Musa <[email protected]> --------- Co-authored-by: Parampreet Singh <[email protected]> Co-authored-by: Musa <[email protected]> Co-authored-by: Mitali Salvi <[email protected]> Co-authored-by: Jeffrey Chien <[email protected]>
d59fed3
to
a1a0f2c
Compare
a1a0f2c
to
50e29f0
Compare
cmd/amazon-cloudwatch-agent-target-allocator/allocation/allocator.go
Outdated
Show resolved
Hide resolved
@@ -9,6 +9,16 @@ func ConfigMap(otelcol string) string { | |||
return DNSName(Truncate("%s", 63, otelcol)) | |||
} | |||
|
|||
// TAConfigMap returns the name for the config map used in the TargetAllocator. | |||
func TAConfigMap(otelcol string) string { | |||
return DNSName(Truncate("%s-target-allocator", 63, otelcol)) |
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.
nit: %s-targetallocator
to be consistent with upstream
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.
we are following the cloudwatch-agent format here but I am open to changing
|
||
// PrometheusConfigMap returns the name for the prometheus config map. | ||
func PrometheusConfigMap(otelcol string) string { | ||
return DNSName(Truncate("%s-prometheus-config", 63, otelcol)) |
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.
nit: %s-prometheus
is probably good enough to be consistent with the other names and its better to keep these short to avoid hitting the max length of 64.
) | ||
|
||
// Labels return the common labels to all TargetAllocator objects that are part of a managed AmazonCloudWatchAgent. | ||
func Labels(instance v1alpha1.AmazonCloudWatchAgent, name string) map[string]string { |
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.
nit: Do we need this?
Cant we just call manifestutils.Labels
with a new component name like ComponentAmazonCloudWatchAgentTargetAllocator
?
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.
We don't need it, we can remove it and use manifestutils.Labels
instead if that's preferable.
Namespace: owner.Namespace, | ||
LabelSelector: labels.SelectorFromSet(selector), | ||
} | ||
// Define lists for different Kubernetes resources |
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.
why just these? are we only looking at the type of resources created for TA currently?
since the controller can create other resources too such as hpsa, pdbs, routes, ingresses etc.
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.
Same question
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.
Do we have any integration tests which validate the new component being added ?
@@ -87,6 +90,9 @@ func (c CollectorWebhook) defaulter(r *AmazonCloudWatchAgent) error { | |||
if r.Spec.Replicas == nil { | |||
r.Spec.Replicas = &one | |||
} | |||
if r.Spec.TargetAllocator.Enabled && r.Spec.TargetAllocator.Replicas == nil { | |||
r.Spec.TargetAllocator.Replicas = &one |
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.
Wondering if this needs to be documented or made configurable later ? This means TA is not scalable at the moment
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.
Yeah I think this can be an enhancement.
@@ -163,6 +169,32 @@ func (c CollectorWebhook) validate(r *AmazonCloudWatchAgent) (admission.Warnings | |||
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 { | |||
warnings = append(warnings, fmt.Sprintf("The Amazon CloudWatch Agent mode is set to %s, we do not recommend enabling Target Allocator when not running as a StatefulSet", r.Spec.Mode)) |
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.
nit: could be updated with The Amazon CloudWatch Agent mode is set to %s, which does not support enabling Target Allocator
The mode could be Deployment and the error message would say StatefulSet which can be confusing
Namespace: owner.Namespace, | ||
LabelSelector: labels.SelectorFromSet(selector), | ||
} | ||
// Define lists for different Kubernetes resources |
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.
Same question
Enhanced CloudWatch Agent Operator for Kubernetes
Summary
This PR introduces significant improvements to the CloudWatch Agent Operator for Kubernetes, addressing limitations in the previous Daemonset implementation and enhancing scalability and efficiency.
Key Changes
Detailed Description
Background
The Amazon CloudWatch Agent allows customers to collect and publish metrics in Prometheus format across various compute environments, including Kubernetes clusters. The CloudWatch Agent Operator simplifies the onboarding process for Prometheus scraping.
Previous Limitations
New Features
Statefulset Deployment:
Target Allocator Integration:
Benefits:
Automatic Updates
The operator automatically applies changes to the scrape configuration, updating both the Target Allocator and CloudWatch Agent instances.
Testing
TBA
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.