[vtadmin] gate/tablet/vtctld FQDNs #7886
Conversation
There was a problem hiding this comment.
Thanks so much for doing this. It's going to be so useful, especially for "internal deprecation" reasons. 😼
Just a couple of minor comments + questions but otherwise this looks great! Go templates make this so much nicer than the equivalent front-end implementation.
| if *vtgateFQDNTmplStr != "" { | ||
| disco.vtgateFQDNTmpl, err = template.New("consul-vtgate-fqdn-template-" + cluster.Id).Parse(*vtgateFQDNTmplStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failde to parse vtgate FQDN template %s: %w", *vtgateFQDNTmplStr, err) |
There was a problem hiding this comment.
Typo: "failde" (there's another instance lower in the file)
There was a problem hiding this comment.
it's the Olde English of "failed" (thank you!)
| if c.vtgateFQDNTmpl != nil { | ||
| vtgate.FQDN, err = textutil.ExecuteTemplate(c.vtgateFQDNTmpl, vtgate) | ||
| if err != nil { | ||
| return nil, err // TODO: wrap |
| } | ||
|
|
||
| func (c *ConsulDiscovery) discoverVTGates(_ context.Context, tags []string) ([]*vtadminpb.VTGate, error) { | ||
| func (c *ConsulDiscovery) discoverVTGates(_ context.Context, tags []string, executeFQDNTemplate bool) ([]*vtadminpb.VTGate, error) { |
There was a problem hiding this comment.
Despite looking all at the callsites, I'm not sure I follow why the executeFQDNTemplate boolean is necessary, or when I'd want to use true vs. false. 🤔 Maybe a comment would help? (Same comment about discoverVtctld's parameter.)
There was a problem hiding this comment.
In practice it's an optimization for Discover{Vtctld,VTGate}Addr. If we didn't have this flag to thread through, then we would make one ExecuteTemplate call per gate (read also "vtctld") as part of the discovery (to generate the FQDN), pick one gate, and make a final ExecuteTemplate call to generate the dialable address, for a total of N+1 calls to ExecuteTemplate.
Since everything that's available to ExecuteTemplate when we're generating the address is also available when generating the FQDN, we can force the user to (potentially*) be a little redundant in the config and save the N calls for generating FQDNs and make a single ExecuteTemplate call for the final address.
* "potentially" only because it's redundant if your dial address is something like "FQDN + gRPC port", then you would need a config of:
-fqdn-tmpl "{{ .Hostname }}.example.com"
-addr-tmpl "{{ .Hostname }}.example.com:15000"
whereas if we didn't do this optimization then you could specify something like
-fqdn-tmpl "{{ .Hostname }}.example.com"
-addr-tmpl "{{ .FQDN }}:15000"
There was a problem hiding this comment.
I seeee. Thank you for explaining!
go/vt/vtadmin/cluster/cluster.go
Outdated
| return nil, fmt.Errorf("DiscoverVTGates(cluster = %s): %w", c.ID, err) | ||
| } | ||
|
|
||
| // Not every discovery implementation will necessarily populate the Cluster |
There was a problem hiding this comment.
This is pretty nitpicky so feel free to leave it, but a more straightforward phrasing might be:
// This overwrites any "Cluster" field populated by the discovery implementation
I know that the code is right there, but the current comment (to me) more readily implies that the "Cluster" field will be populated iff it doesn't already exist from the discovery implementation. Like I said... nitpicky. 😎
There was a problem hiding this comment.
No this distinction makes sense to me! Thank you!
Signed-off-by: Andrew Mason <amason@slack-corp.com>
staticfile disco doesn't need this, because it can just read this from the config. **Note**: the main downside is that the json key is "FQDN" rather than "fqdn" which 🤷 ? Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188
force-pushed
the
am_vtadmin_full_urls
branch
from
2be0592
to
6cee666
Compare
ajm188
added
Component: VTAdmin
Description
This PR introduces an optional FQDN field to gates/tablets/vtctld proto message types, along with options to specify a go template for generating an FQDN from the rest of the struct fields in a gate/tablet/vtctld.
For gates and vtctlds, it's up to the particular discovery implementations to populate those fields, and for tablets it is handled by the
Cluster. Consequently, the gate/vtctld template flags are pushed down into theConsulDiscoveryimplementation (andStaticFileDiscoverycan just read the "FQDN" field directly from the static file, so I did not include an option), while the tablet template flag is parsed as part of the cluster DSN.One note: I named the field "FQDN" rather than "fqdn" in the proto file, so that it would generate the horrid "Fqdn" Go field name; the side effect of me doing this is that when marshaling to JSON, the key is in all-caps rather than all-lowercase like the rest of the fields. If we feel that json field casing consistency is more valuable, then I can bear with the
Fqdn.I also added a generic function to
package textutilto remove the duplicated "create buffer; execute template into buffer; return resulting string" that we have in like 6 places now.Final note: I'm really bad at typing "fqdn" apparently, and kept making the typo for "fdqn", so something to keep an eye out for when reviewing.
Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: