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

Add support for configurable modes for cloudwatch agent CR to support Prometheus monitoring with Target Allocator #86

Open
wants to merge 11 commits into
base: target-allocator
Choose a base branch
from

Conversation

mitali-salvi
Copy link
Contributor

@mitali-salvi mitali-salvi commented Aug 22, 2024

Description of changes:

  1. Add support for configuring multiple agent workloads
  2. Add support for modes (daemon-set, deployment, statefulset) for the cloudwatch-agent custom resource
  3. Add support for prometheus monitoring via cloudwatch-agent
  4. Add support for target allocator for prometheus monitoring
    Add support for prometheusCR monitoring

Testing - manually tested on cluster with the following config

agents:
  - name: prometheus-cloudwatch-agent
    mode: statefulset
    prometheus:
      config:
        global:
          scrape_interval: 60s
        scrape_configs:
          - job_name: 'cadvisor'
            scrape_interval: 15s
            static_configs:
              - targets: [ 'cadvisor:8080' ]
      targetAllocator:
        enabled: true

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lisguo
Copy link
Contributor

lisguo commented Aug 26, 2024

Helm chart integ tests should still pass right? Is the agent running as expected?

@mitali-salvi mitali-salvi changed the title [WIP] Add support for configurable modes for cloudwatch agent CR Add support for configurable modes for cloudwatch agent CR Sep 9, 2024
@mitali-salvi mitali-salvi changed the base branch from main to target-allocator September 13, 2024 19:54
Copy link

@okankoAMZ okankoAMZ left a comment

Choose a reason for hiding this comment

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

Could you update the desc, with an example on how to have multiple workloads

@mitali-salvi mitali-salvi changed the title Add support for configurable modes for cloudwatch agent CR Add support for configurable modes for cloudwatch agent CR to support Prometheus monitoring with Target Allocator Oct 4, 2024
Copy link

@okankoAMZ okankoAMZ left a comment

Choose a reason for hiding this comment

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

Minor comments

@@ -1,7 +1,7 @@
build
.tmp
*.iml
.DS_Store
*.DS_Store

Choose a reason for hiding this comment

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

Why do we have this?

strategy:
nodeSelector:
kubernetes.io/os: linux
image:

Choose a reason for hiding this comment

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

Just noting that this image does not exist yet*

@@ -545,8 +545,14 @@ admissionWebhooks:
## Secret labels
secretLabels: { }

## Agent workloads list
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Agent workloads list
## List of AmazonCloudWatchAgent workloads to install & manage, each representing an independent installation of the AmazonCloudWatchAgent CustomResource. Each entry in this list uses the schema & the defaults from $.agent, so only provide any overrides here.

agent:
name:
mode: # represents the mode the cloudwatch-agent will run in (deployment, daemonset or statefulset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt this default to daemonset?

agent:
name:
mode: # represents the mode the cloudwatch-agent will run in (deployment, daemonset or statefulset)
replicas: # The total number non-terminated pods targeted by this cloudwatch-agent's deployment or statefulSet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe set this to -1 as the default to make it obvious its not applicable by default and anything using it will fail unless overriden.

@@ -602,6 +608,37 @@ agent:
}
}
}
windowsDefaultConfig:
Copy link
Contributor

Choose a reason for hiding this comment

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

This content actually now looks identical to the linux default above.
It might actually be cleaner to now have cloudwatch-agent-windows in the agents list above and drop this windowsDefaultConfig field here. We can instead add an os field thats either linux or windows and spin up all other resources based on that flag.

So agents would look like:

agents:
- name: cloudwatch-agent
- name: cloudwatch-agent-windows
  os: windows

Copy link
Contributor

Choose a reason for hiding this comment

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

Its fine if we do this in a separate PR.

}
prometheus:
config:
targetAllocator:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we'll have more TLS related stuff here, but those can come slightly later

@@ -51,3 +51,6 @@ rules:
- apiGroups: [ "route.openshift.io" ]
resources: [ "routes", "routes/custom-host" ]
verbs: [ "create","delete","get","list","patch","update","watch" ]
- apiGroups: [ "policy" ]
resources: [ "poddisruptionbudgets" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the latest operator creating PDBs for the deployment mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants