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 functionality to scrape pods per kubernetes node #6969

Merged
merged 9 commits into from
Mar 3, 2020

Conversation

mg03
Copy link
Contributor

@mg03 mg03 commented Feb 1, 2020

Required for all PRs:

  • [ y] Signed CLA.
  • [ y] Associated README.md updated.
  • [ y] Has appropriate unit tests.

Fixes #6950

@mg03 mg03 requested review from danielnelson and glinton February 1, 2020 22:50
@mg03 mg03 changed the title add functionaloty to scrape pods on a kubernetes node add functionality to scrape pods on a kubernetes node Feb 1, 2020
@mg03
Copy link
Contributor Author

mg03 commented Feb 1, 2020

@danielnelson : please review my PR, thank you

@mg03
Copy link
Contributor Author

mg03 commented Feb 4, 2020

@jackzampolin : Hi, Im tagging you since I see you are the primary moderator on community.influxdata.com and I thought you might have some suggestion/s . Would you know who could possibly help with PR review ?

@mg03 mg03 removed the request for review from glinton February 8, 2020 15:34
@mg03 mg03 changed the title add functionality to scrape pods on a kubernetes node add functionality to scrape pods per kubernetes node Feb 12, 2020
@mg03
Copy link
Contributor Author

mg03 commented Feb 12, 2020

@danielnelson : hi would it be possible for you to take a look at this PR and let me know your thoughts on the same.

@danielnelson
Copy link
Contributor

Code looks good but I do feel that this method is very specific to your case and that we should try to generalize it more.

I'm not a Kubernetes expert, but perhaps we could provide access to the labelSelector option when starting the watch. With this we could watch any custom subset of pods and as a bonus it should perform better since it doesn't need to skip over unmatched pods. However, this would require having the right labels set, maybe this technique can work: https://stackoverflow.com/questions/40131682/how-to-set-label-to-kubernetes-node-at-creation-time

@danielnelson danielnelson added area/k8s area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Feb 12, 2020
@mg03
Copy link
Contributor Author

mg03 commented Feb 14, 2020

@danielnelson : thank you for the feedback.

  1. Agree the code is specific
  2. Labels on nodes do not help filter pods during a watch. Only pod attributes can be used to filter during a watch.
  3. Assuming we go with filtering on pod label selector, still we cannot hone in on filtering by pod running on a specific node, the reason is that the nodename attribute is not part of pod labels. Labels are under metadata parent field. the nodename is under the spec parent field.
  4. Due to above may I suggest if we can have both label selectors and field selectors, so that users can filter on both or any or none of the filters during watch cycle.
    Let me know if you are ok with my suggestion in point 4) above and I can work towards it ....

@danielnelson
Copy link
Contributor

That sounds great, thanks.

@mg03
Copy link
Contributor Author

mg03 commented Feb 17, 2020

@danielnelson : As per our discussion above, Ive updated the code to support Kubernetes label and field selectors. Please review and let me know your thoughts.

@mg03
Copy link
Contributor Author

mg03 commented Feb 20, 2020

@danielnelson : would it be possible for you to take a look at the PR, which is updated as per our conversation about using Kubernetes label and field selectors?

@mg03
Copy link
Contributor Author

mg03 commented Feb 25, 2020

@danielnelson : The functionality implemented here is important for adoption within our org, as such please provide guidance on the path forward on this PR.

@danielnelson danielnelson added this to the 1.14.0 milestone Feb 27, 2020
plugins/inputs/prometheus/kubernetes.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/kubernetes.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
@mg03 mg03 requested a review from danielnelson February 28, 2020 08:37
@mg03
Copy link
Contributor Author

mg03 commented Feb 28, 2020

@danielnelson : I have made the changes as requested, please review.

plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved
@mg03
Copy link
Contributor Author

mg03 commented Mar 3, 2020

@danielnelson : updated the PR with changes as requested, please review.

@mg03 mg03 requested a review from danielnelson March 3, 2020 02:30
@danielnelson danielnelson merged commit dd1ace7 into influxdata:master Mar 3, 2020
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
@bamvenkatesh
Copy link

@mg03 - Hi, I was just curious why choose DaemonSet instead of a deployment for Telegraf.

Like @russorat asked, Is there a specific reason you don't want to run a single telegraf to handle the scraping?

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/k8s area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus scrape metrics from pods on same node
3 participants