roachprod: load balancer from pgurl command #123206
Conversation
|
This change is |
herkolategan
requested review from
DarrylWong and
renatolabs
and removed request for
a team
pkg/roachprod/install/nodes.go
Outdated
| if s == "all" { | ||
| // "L" is a special value that also returns all nodes, but implies a load | ||
| // balancer should be used when applicable. | ||
| if s == "all" || s == "L" { |
There was a problem hiding this comment.
Why not LB? L doesn't seem like a memorable mnemonic unlike all. Should it also be case-insensitive?
There was a problem hiding this comment.
I matched this to how the expanders also select the load balancer vs. node IPs. I can update both. I'll maybe add it to the command help as well to make it clear as an option.
herkolategan
force-pushed
the
hbl/roachprod-lb-pgurl-command
branch
from
ea28ee9
to
c31dbbe
Compare
pkg/roachprod/install/nodes.go
Outdated
| @@ -44,7 +44,9 @@ func ListNodes(s string, numNodesInCluster int) (Nodes, error) { | |||
| return nil, errors.AssertionFailedf("invalid number of nodes %d", numNodesInCluster) | |||
| } | |||
|
|
|||
| if s == "all" { | |||
| // "lb" is a special value that also returns all nodes, but implies a load | |||
| // balancer should be used when applicable. | |||
There was a problem hiding this comment.
Why "when applicable"? I don't see logic below to handle the case where a load balancer is not available (would we want that?)
There was a problem hiding this comment.
True, I'll update the comment to be more concise.
pkg/roachprod/roachprod.go
Outdated
| @@ -184,6 +184,13 @@ func newCluster( | |||
| return c, nil | |||
| } | |||
|
|
|||
| // preferLoadBalancer determines if the node selector section in the cluster | |||
There was a problem hiding this comment.
I guess this is a similar point, but prefer suggests that actually returning a LB IP is optional, but it seems we return an error below if we can't find a LB.
I actually prefer this semantics (error returned if LB requested but doesn't exist) unless there's strong reason to provide a fallback. In that case, maybe requestedLoadBalancer might be a better name.
| @@ -22,8 +22,8 @@ import ( | |||
| ) | |||
|
|
|||
| var parameterRe = regexp.MustCompile(`{[^{}]*}`) | |||
| var pgURLRe = regexp.MustCompile(`{pgurl(:[-,0-9]+|:L)?(:[a-z0-9\-]+)?(:[0-9]+)?}`) | |||
| var pgHostRe = regexp.MustCompile(`{pghost(:[-,0-9]+|:L)?(:[a-z0-9\-]+)?(:[0-9]+)?}`) | |||
| var pgURLRe = regexp.MustCompile(`{pgurl(:[-,0-9]+|:(?i)lb)?(:[a-z0-9\-]+)?(:[0-9]+)?}`) | |||
There was a problem hiding this comment.
At some point it would be nice to document these with examples, these regexes are getting complicated 😄
There was a problem hiding this comment.
True, these are becoming a bit over-encumbered.
herkolategan
force-pushed
the
hbl/roachprod-lb-pgurl-command
branch
from
c31dbbe
to
81d5a73
Compare
Previously, the load balancer IP or URL can only be retrieved using the expanders. This change adds the ability to also get the URL from the `roachprod` pgurl command by using the `lb` node selector similar to how it is done with expansion. Epic: None Release Note: None
herkolategan
force-pushed
the
hbl/roachprod-lb-pgurl-command
branch
2 times, most recently
from
613cb2b
to
390d1fc
Compare
Previously, `L` was used in the `roachprod` expanders and other place to select load balancers. This change updates it to `LB` or `lb` to be more explicit. The help for commands have also been updated to inform the user that an option exists to select a load balancer. Epic: None Release Note: None
herkolategan
force-pushed
the
hbl/roachprod-lb-pgurl-command
branch
from
390d1fc
to
dbd590f
Compare
|
TFTRs! bors r=srosenberg,renatolabs |
Previously, the load balancer IP or URL can only be retrieved using the expanders. This change adds the ability to also get the URL from the
roachprodpgurl command by using thelbnode selector similar to how it is done with expansion.Epic: None
Release Note: None