-
Notifications
You must be signed in to change notification settings - Fork 13
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 pprof server to fleet-manager and unlimited evals in testing&develop #1431
Conversation
@@ -3,6 +3,7 @@ package services | |||
import ( | |||
"context" | |||
"fmt" | |||
"github.com/stackrox/acs-fleet-manager/pkg/environments" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import should be in the block below no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sí, done
} | ||
|
||
func (p *PprofServer) Listen() (net.Listener, error) { | ||
return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the listener here for the pprof server ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Listen-func is used only for Listen
and not for starting the server. Only the API Server uses the Listen
function.
See docs here on the funs for more info:
acs-fleet-manager/pkg/server/api_server.go
Lines 158 to 187 in ac4af15
// Listen only starts the listener, not the server. | |
// Useful for breaking up ListenAndServer (Start) when you require the server to be listening before continuing | |
func (s *APIServer) Listen() (listener net.Listener, err error) { | |
l, err := net.Listen("tcp", s.serverConfig.BindAddress) | |
if err != nil { | |
return l, fmt.Errorf("starting the listener: %w", err) | |
} | |
return l, nil | |
} | |
// Start ... | |
func (s *APIServer) Start() { | |
go s.Run() | |
} | |
// Run Start listening on the configured port and start the server. This is a convenience wrapper for Listen() and Serve(listener Listener) | |
func (s *APIServer) Run() { | |
listener, err := s.Listen() | |
if err != nil { | |
glog.Fatalf("Unable to start API server: %s", err) | |
} | |
// Before we start processing requests, wait | |
// for the server to be ready to run. | |
for _, condition := range s.readyConditions { | |
condition.Wait() | |
} | |
s.Serve(listener) | |
} |
templates/service-template.yml
Outdated
- apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
name: fleet-manager-pprof | ||
labels: | ||
app: fleet-manager | ||
port: pprof | ||
spec: | ||
selector: | ||
app: fleet-manager | ||
ports: | ||
- port: 6060 | ||
targetPort: 6060 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about creating a service for it. Because fleet-manager runs with multiple replicas, we never know which one we hit. We should just do a port forward and use that rather than creating a service for it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I echo Ludovic's review
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ludydoo, SimonBaeumer 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 |
Description
create-central.sh
Checklist (Definition of Done)
Test manual
ROX-12345: ...
Test manual