Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[datadog] Add optional kube-state-metrics pod #1011

Merged
merged 6 commits into from
Jun 20, 2017
Merged

[datadog] Add optional kube-state-metrics pod #1011

merged 6 commits into from
Jun 20, 2017

Conversation

hkaj
Copy link
Collaborator

@hkaj hkaj commented May 5, 2017

Optionally deploy kube-state-metrics to get its (awesome!) metrics out of box. It is automatically discovered by the agent running on the same node so no further configuration is required.

This chart is awesome y'all! Thanks for creating it. We will likely list it as the recommended way to install the agent on Kubernetes.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @hkaj. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2017
@hkaj
Copy link
Collaborator Author

hkaj commented May 22, 2017

Can I get a review here, team?

@hkaj hkaj force-pushed the hkaj/datadog-kube-state-metrics branch from 061e254 to edbc984 Compare May 22, 2017 13:19
@hkaj hkaj force-pushed the hkaj/datadog-kube-state-metrics branch from edbc984 to 0c15770 Compare May 22, 2017 13:21
@elafarge
Copy link
Contributor

Great addition @hkaj . Can't wait for that to get merged :)

@roobert
Copy link

roobert commented May 23, 2017 •
edited
Loading

@hkaj: I'm somewhat new to Helm so please excuse me if this doesn't make sense.

For pods like kube-state-metrics that are likely dependencies for multiple charts, should they be bundled into individual application charts? The alternative is that we have multiple instances of the same pods running across our infrastructure which seems like a waste of resources.

Cheers,

edit: maybe this would be better off as a separate chart and a requirement?

edit 2: looks like there's on open PR for a kube state metrics chart here.

@hkaj
Copy link
Collaborator Author

hkaj commented May 24, 2017

@roobert you're right, it's better as another chart. We can switch to a requirement as soon as that PR is merged. And I agree, the risk here is that several charts end up shipping kube-state-metrics (some people use prometheus and datadog, and both will have a kube-state-metrics deployment), that's why I made it optional. This way if you already deploy it another way you can leave it out in the datadog deployment (the datadog agent will still find it and report its metrics),

@roobert
Copy link

roobert commented May 24, 2017

Nice one @hkaj, going to leave a reference here for #662.

@pierreozoux pierreozoux mentioned this pull request May 26, 2017
Closed
@mgoodness mgoodness self-assigned this May 26, 2017
@mgoodness
Copy link
Contributor

#662 has been merged. There are some differences between the kube-state-metrics chart and what's proposed here. I recommend opening a new PR to resolve those differences, then update this PR to use the stand-alone KSM chart as a dependency. We'll get both merged ASAP.

@hkaj hkaj force-pushed the hkaj/datadog-kube-state-metrics branch 3 times, most recently from b86fb9c to ef7fef7 Compare June 8, 2017 12:52
@hkaj hkaj force-pushed the hkaj/datadog-kube-state-metrics branch from ef7fef7 to bcef080 Compare June 8, 2017 12:53
@hkaj
Copy link
Collaborator Author

hkaj commented Jun 8, 2017

thanks for the feedback @mgoodness
Updated the PR using requirements (also removed a bunch of changes that became useless without the kube-state-metrics deployment).

@hkaj
Copy link
Collaborator Author

hkaj commented Jun 19, 2017

@mgoodness Resolved the conflicts, can I get a quick review before new ones pop up?

gtaylor
gtaylor reviewed Jun 19, 2017
View reviewed changes
stable/datadog/Chart.yaml Outdated
@@ -13,3 +13,5 @@ sources:
maintainers:
- name: Greg Taylor
email: gtaylor@gc-taylor.com
- name: Haissam Kaj
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have the bandwidth to review the rest of this, but wanted to give this the 👍 as the current and only maintainer. The situation should improve greatly in having upstream involved!

@prydonius
Copy link
Member

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 20, 2017
@prydonius prydonius added code reviewed lgtm Indicates that a PR is ready to be merged. UX reviewed labels Jun 20, 2017
@prydonius
Copy link
Member

thanks a lot @hkaj :)

@prydonius prydonius merged commit 1f9bc91 into helm:master Jun 20, 2017
@hkaj
Copy link
Collaborator Author

hkaj commented Jun 20, 2017

thanks guys!

@hkaj hkaj deleted the hkaj/datadog-kube-state-metrics branch June 20, 2017 17:00
lachie83 added a commit to lachie83/charts that referenced this pull request Jul 10, 2017
@lachie83
Merge remote-tracking branch 'upstream/master'
0623dd5 
* upstream/master: (52 commits)
  [redmine] add ingress (helm#1336)
  [stable/jenkins] Use imageTag as version in config map (helm#1333)
  - Bump to latest Minio release (helm#1304)
  Update k8s-dashboard note when ingress is enabled (helm#1339)
  Update etcd-operator to latest release (helm#1248)
  [stable/nginx-ingress] Add hostNetwork option (helm#1250)
  Update the CONTRIBUTING.md to reflect the new #helm-users and #helm-dev channels (helm#1315)
  Submit stable Voyager chart (helm#954)
  [datadog] Add optional kube-state-metrics pod (helm#1011)
  [stable/prestashop] Release 0.4.10 (helm#1267)
  [stable/wordpress] Release 0.6.5 (helm#1270)
  [stable/phabricator] Release 0.4.9 (helm#1281)
  [stable/drupal] Release 0.6.2 (helm#1268)
  Bump aws-cluster-autoscaler to latest. (helm#1101)
  Use memcached modern recommended options instead (helm#1221)
  Update NOTES.txt (helm#1316)
  Rename MARIADB_PORT env var to MARIADB_PORT_NUMBER (helm#1210)
  Rename POSTGRESQL_PORT env var to POSTGRESQL_PORT_NUMBER (helm#1189)
  Kubernetes Dashboard Chart (helm#808)
  Add best practices to requirements (helm#1305)
  ...
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
@hkaj @prydonius
[datadog] Add optional kube-state-metrics pod (helm#1011)
60733ca 
* [datadog] Add optional kube-state-metrics pod

* use new kube-state-metrics chart as requirement

* Bump to 0.6.0

* bump version and use github ids
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gtaylor gtaylor gtaylor left review comments

Assignees

@mgoodness mgoodness

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. code reviewed lgtm Indicates that a PR is ready to be merged. size/medium UX reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hkaj @k8s-ci-robot @elafarge @roobert @mgoodness @prydonius @gtaylor