roachprod: support for dynamic admin url port #123619
Conversation
nameisbhaskar
requested review from
herkolategan and
DarrylWong
and removed request for
a team
|
This change is |
|
The prom-helper-service: https://github.com/cockroachlabs/prom-helper-service |
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
from
dfa0df8 to
e0c588a
Compare
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
from
e0c588a to
038d539
Compare
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
2 times, most recently
from
caa21e7 to
59123f6
Compare
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
3 times, most recently
from
688d4be to
b30b6a6
Compare
|
Thanks @srosenberg for your review. I have made certain changes as per your comments and would be good if you can a look again. |
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
2 times, most recently
from
5ecf56a to
5b66816
Compare
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
from
5b66816 to
f05ea18
Compare
In order to support multiple tenants on the same host, a unique, custom port can be assigned for DefaultAdminUIPort. This breaks because the port is hard-coded in prometheus config for scraping. The change is to have a http server running in prometheus server that can dynamically update the configuration when a new cluster is brought up. More details of the solution is explained in the page - https://cockroachlabs.atlassian.net/wiki/spaces/~7120207825326fb5e546c194029506f2c5335e/pages/3458531376/Dynamic+Scrape+Configs+on+Prometheus+for+Roachprod Fixes: cockroachdb#117125 Epic: none
nameisbhaskar
force-pushed
the
user/bhaskar/dynamic_prom_port
branch
from
f05ea18 to
d290435
Compare
|
bors r=@srosenberg,@renatolabs,@herkolategan |
|
Thanks @srosenberg , @herkolategan, @renatolabs, @DarrylWong for reviewing this change!! |
123943: roachprod: fix prom target panic r=srosenberg a=msbutler As of #123619, roachtests that start up a cockroach cluster on a subset of roachtest nodes on gce panics (i.e. all PCR roachtests) because `updatePrometheusTargets` assumes the roachtest starts up all nodes in the roachprod cluster. This patch relaxes this assumption. Epic: none Release note: none Co-authored-by: Michael Butler <[email protected]>
There was a problem hiding this comment.
Sorry for the late review, I only had a chance to take a closer look at this PR now!
Most comments are just suggestions for future improvements that can be done separately or when we are making changes to the code in the future.
That said, I think a few changes should be done earlier rather than later (the command line help string, the check for len(nodeIPPorts) > 0, and maybe logging of errors in Delete to make it easier for us to understand failures when they happen in the future).
| ` | ||
|
|
||
| // createClusterConfigFile creates the cluster config file per node | ||
| func buildCreateRequest(nodes []string) (io.Reader, error) { |
There was a problem hiding this comment.
This would likely be simpler if we used the yaml package to handle the serialization for us. Then we would be able to go from data structure -> YAML without relying on Go templates which is IMO a little easier to understand.
In order to support multiple tenants on the same host, a unique, custom port can be assigned for DefaultAdminUIPort. This breaks because the port is hard-coded in prometheus config for scraping.
The change is to have a http server running in prometheus server that can dynamically update the configuration when a new cluster is brought up. More details of the solution is explained in the page - https://cockroachlabs.atlassian.net/wiki/spaces/~7120207825326fb5e546c194029506f2c5335e/pages/3458531376/Dynamic+Scrape+Configs+on+Prometheus+for+Roachprod
Fixes: #117125
Epic: none