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

Assign specific, unique ports for pprof (Agent, Operator, Hubble Relay) #15441

Merged
merged 2 commits into from
Mar 27, 2021

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Mar 23, 2021

This allows users to enable pprof for both cilium-agent and
cilium-operator on the same machine without running into:

level=warning msg="Unable to serve pprof API" error="listen tcp
127.0.0.1:6060: bind: address already in use" subsys=pprof

It also provides users with the ability to specify the port via a flag.
Note that this flag is not exposed via Helm because it is a flag mainly
used for debugging, and shouldn't need to be exposed fully to the user.
A developer / advanced user can just as easily edit the daemonset or the
deployment object to append an extra arg. Also, given the fact that the
ports are unique across the components, that reduces the need to
provide a knob via Helm.

To avoid port collisions, the Agent, Operator, and Hubble Relay are all
assigned the following specific ports respectively: 6060, 6061, 6062.

Signed-off-by: Chris Tarazi chris@isovalent.com

@christarazi christarazi requested a review from a team March 23, 2021 19:17
@christarazi christarazi requested review from a team as code owners March 23, 2021 19:17
@christarazi christarazi requested a review from a team March 23, 2021 19:17
@christarazi christarazi requested a review from a team as a code owner March 23, 2021 19:17
@christarazi christarazi requested a review from a team March 23, 2021 19:17
@christarazi christarazi requested a review from a team as a code owner March 23, 2021 19:17
@christarazi christarazi added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Mar 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 23, 2021
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

It's worth mentioning that both the cilium-agent and cilium-operator
share the Cilium configmap so if a port was provided, then it will be
used for both, and will cause the same port collision issue. The user
must edit the cilium-operator deployment or the cilium-agent daemonset
directly in order to pass different ports to each component to properly
workaround the port collision.

Why not use different default ports?

@christarazi
Copy link
Member Author

It's worth mentioning that both the cilium-agent and cilium-operator
share the Cilium configmap so if a port was provided, then it will be
used for both, and will cause the same port collision issue. The user
must edit the cilium-operator deployment or the cilium-agent daemonset
directly in order to pass different ports to each component to properly
workaround the port collision.

Why not use different default ports?

That could work too. The reason I didn't change the defaults is because pprof is well-known to be at 6060. What do others think?

@borkmann
Copy link
Member

It's worth mentioning that both the cilium-agent and cilium-operator
share the Cilium configmap so if a port was provided, then it will be
used for both, and will cause the same port collision issue. The user
must edit the cilium-operator deployment or the cilium-agent daemonset
directly in order to pass different ports to each component to properly
workaround the port collision.

Why not use different default ports?

That could work too. The reason I didn't change the defaults is because pprof is well-known to be at 6060. What do others think?

For gops we also use different default ports, see #14329 and last commit documented them in Documentation/operations/system_requirements.rst.

@christarazi christarazi changed the title option, pprof: Allow user to provide port via flag Assign specific, unique ports for pprof (Agent, Operator, Hubble Relay) Mar 24, 2021
@aditighag aditighag removed their assignment Mar 24, 2021
@kaworu
Copy link
Member

kaworu commented Mar 25, 2021

@christarazi I don't think this is needed forf Hubble Relay as it run in its own netns.

EDIT: I'm fine adding the option to Relay, just wondering if that would be OK to keep the default 6060.

Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

👍

@aditighag
Copy link
Member

@pchaigno
Copy link
Member

pchaigno commented Mar 25, 2021

Closed and reopened as the GitHub actions were stuck.

test-me-please

This allows users to enable pprof for both cilium-agent and
cilium-operator on the same machine without running into:

```
level=warning msg="Unable to serve pprof API" error="listen tcp
127.0.0.1:6060: bind: address already in use" subsys=pprof
```

It also provides users with the ability to specify the port via a flag.
Note that this flag is not exposed via Helm because it is a flag mainly
used for debugging, and shouldn't need to be exposed fully to the user.
A developer / advanced user can just as easily edit the daemonset or the
deployment object to append an extra arg. Also, given the fact that the
ports are unique across the components, that reduces the need to
provide a knob via Helm.

To avoid port collisions, the Agent, Operator, and Hubble Relay are all
assigned the following specific ports respectively: 6060, 6061, 6062.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Following the commit to (re)assign the pprof default port, update the
documentation with the new information.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Copy link
Member Author

test-me-please

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks @aditighag!

@kaworu kaworu removed their assignment Mar 26, 2021
@joestringer joestringer merged commit 11d4f03 into cilium:master Mar 27, 2021
1.10.0 automation moved this from In progress to Done Mar 27, 2021
@christarazi christarazi deleted the pr/christarazi/pprof-port branch March 27, 2021 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants