✨ Export HTTP server manager runnable implementation #2473
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/M
joelanford
changed the title
pkg/manager/server.go
Outdated
| // among multiple servers. | ||
| Kind string | ||
|
|
||
| // Log is the logger used by the server. If not set, a logger will be derived from the context passed to Start. |
There was a problem hiding this comment.
Why not only have one way to pass it in?
There was a problem hiding this comment.
I didn't want to require a Server struct to be setup with a logger in advance. I don't have a strong opinion here though.
There was a problem hiding this comment.
Could we remove the Log here, and just inherit the logger from Start?
There was a problem hiding this comment.
This was already present in the unexported server struct, so I wasn't going to change it.
There is existing code in the pprof and health probe server setup that explicitly sets the servers' logger to the controller manager's logger (which comes from manager.Options.Logger).
There's a bunch of context plumbing that makes it difficult to tell which contexts are used where, but I can't find anywhere that injects the controllerManager's logger into the context that is used to start the http servers.
We could potentially make a custom context with the logger injected and use that context just with the HTTPServers group to preserve existing behavior.
WDYT?
joelanford
force-pushed
the
export-runnable-server
branch
from
9e72ade
to
77b08a8
Compare
| // NeedLeaderElection returns true if the server should only be started when the manager is the leader. | ||
| func (s *Server) NeedLeaderElection() bool { | ||
| return s.OnlyServeWhenLeader | ||
| } |
There was a problem hiding this comment.
The use case I have in mind is when there is a strong coupling between what is being reconciled and what is being served. One example might be an API that represents OCI images, where creating an instance of that API makes the OCI image contents available as a tar archive via the HTTP server.
k8s-ci-robot
added
the
needs-rebase
|
@joelanford Are you still interested in this PR? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
joelanford
force-pushed
the
export-runnable-server
branch
from
77b08a8
to
84f4ebd
Compare
k8s-ci-robot
added
size/L
joelanford
force-pushed
the
export-runnable-server
branch
from
84f4ebd
to
7ac9a14
Compare
k8s-ci-robot
removed
the
needs-rebase
…able Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
joelanford
force-pushed
the
export-runnable-server
branch
from
7ac9a14
to
d86d5f8
Compare
joelanford
removed
the
lifecycle/stale
|
/lgtm Will fix the nit in a follow up |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 5e942e32c0322a50d35fbf4b19a3e60ef8e056b3
|
In operator-framework, we have run into a few situations where we need to run a custom HTTP server inside a controller process. In order to decouple from assumptions made by the metrics server (which allows extra handlers to be added), we need a fully separate server. Since controller-runtime already has a generic HTTP server runnable for use with health probes and pprof handlers, it seems useful to export this implementation to users as well so that they can run their own HTTP servers without having to worry about implementing the
manager.Runnableinterface correctly.