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

✨ Offer /metrics and /debug/pprof from controller and agent #68

Merged
merged 1 commit into from
May 29, 2024

Conversation

MikeSpreitzer
Copy link
Contributor

@MikeSpreitzer MikeSpreitzer commented May 24, 2024

Summary

This PR fills out the matrix of offering /metrics and /debug/pprof from the controller and agent. Previously the only thing offered was /metrics from the agent.

After #67 merges there will be more work to do about /metrics and /debug/pprof in the Helm chart.

Related issue(s)

This is part of addressing kubestellar/kubestellar#2094 and kubestellar/kubestellar#2158

Copy link
Contributor

@ezrasilvera ezrasilvera left a comment

Choose a reason for hiding this comment

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

  1. Should we add an option to disable (at list the pprof data) ?
  2. Why do we need the pprof at different server/port ? can't we just use a different path in the same base URL ?

@MikeSpreitzer
Copy link
Contributor Author

MikeSpreitzer commented May 29, 2024

@ezrasilvera:

  1. We should have a discussion of security design. It will probably include options to enable/disable both ports as well as additional stuff. I think it is beyond the immediate needs.
  2. When using controller-runtime, the support for those two endpoints opens two distinct sockets, so they have to be on different ports. I opted to follow the same pattern for the other controllers for the sake of consistency.

Copy link
Collaborator

@pdettori pdettori left a comment

Choose a reason for hiding this comment

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

/lgtm

@ezrasilvera
Copy link
Contributor

@MikeSpreitzer
(1) - Sounds good
(2) _ I think the plan was always to convert the controller-runtime, not sure adopting the controller-runtime on the other controllers is reasonable. So overall how many sockets do we open now on (across all our controllers) ?

/hold

Copy link
Contributor

@ezrasilvera ezrasilvera left a comment

Choose a reason for hiding this comment

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

Added "request changes" just to make sure we resolve the second Q..

@MikeSpreitzer
Copy link
Contributor Author

@ezrasilvera: yes, my understanding also is that we plan to switch away from controller-runtime. But that is beyond the scope of this PR.

Regarding number of sockets, remember that each Pod has its own set of sockets. I do not think we are anywhere near having a problem with the number of sockets open on a given Pod.

@MikeSpreitzer MikeSpreitzer merged commit 52ef690 into kubestellar:main May 29, 2024
7 checks passed
@MikeSpreitzer MikeSpreitzer deleted the add-observability branch May 29, 2024 19:33
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.

3 participants