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

fix: issues with prometheus kubernetes pod discovery #9605

Merged

Conversation

gracewehner
Copy link
Contributor

@gracewehner gracewehner commented Aug 9, 2021

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

Resolves #9600

  • Get the pod info from the event using event.Object.(*corev1.Pod) so that the pod isn't an empty struct. This is the equivalent of watcher.Next(pod) when the previous client package was used.

  • A go routine is already running that keeps calling the watchPod() function whenever it returns. Right now with the watchPod() function is calling its own go routine, the function returns and watchPod is called again. This continuously creates new threads and new watchers and re-registers pods instead of having one watcher running each time. Changed back to the previous functionality with a for loop that returns when ctx.Done()

  • Added defer watcher.Stop() which is the equivalent of defer watcher.Close() when the previous client package was used.

I'm not sure any unit tests can be added to detect these issues since the function relies on the kubernetes watch api call and discovering kubernetes pods.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 9, 2021
@gracewehner gracewehner changed the title Fix [prometheus input plugin]: Issues with prometheus kubernetes pod discovery Fix[prometheus input plugin]: Issues with prometheus kubernetes pod discovery Aug 9, 2021
@gracewehner gracewehner changed the title Fix[prometheus input plugin]: Issues with prometheus kubernetes pod discovery fix: issues with prometheus kubernetes pod discovery Aug 9, 2021
@sspaink sspaink self-requested a review August 10, 2021 07:10
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you for fixing this and describing the why/how so clearly.

@sspaink sspaink added fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Aug 10, 2021
Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I don't have a deep understanding of k8s.io/client-go/kubernetes but it sounds like you have identified valid problems and provided good fixes.

@reimda reimda merged commit fe144e7 into influxdata:master Aug 17, 2021
@jta jta mentioned this pull request Aug 17, 2021
3 tasks
reimda pushed a commit that referenced this pull request Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
3 participants