Vtgateproxy rework discovery and add metrics #296

demmer · 2024-04-12T13:44:35Z

Description

Refactor the discovery logic to have a single watcher for the parsed file, and then each resolver just gets callbacks from there as things change.

Clean up and consolidate the command line flags, and change a bit. The new set of options to run (tested in dev) are:

vtgateproxy \
   --logtostderr=1 \
   --port 15000 \ 
   --mysql_server_port 15306 \
   --mysql_auth_server_impl none \
   --grpc_auth_static_client_creds /etc/slack.d/vt_grpc_static_auth_client_creds.json \ 
   --vtgate_hosts_file /etc/vthosts/config_vtgate_hosts_rotor.json
   --num_connections=4 \
   --address_field nebula_address \
   --port_field grpc \
   --pool_type_field type \
   --affinity_field az_id

Implement a more efficient single pass shuffle scan because it seemed nifty.

Add some basic metrics for the discovery part:

# HELP vtgateproxy_json_discovery_build JSON host discovery rebuilt the host list
# TYPE vtgateproxy_json_discovery_build counter
vtgateproxy_json_discovery_build 1
# HELP vtgateproxy_json_discovery_host_affinity Count of hosts returned from discovery by AZ affinity
# TYPE vtgateproxy_json_discovery_host_affinity counter
vtgateproxy_json_discovery_host_affinity{affinity="local"} 3
vtgateproxy_json_discovery_host_affinity{affinity="remote"} 5
# HELP vtgateproxy_json_discovery_host_pool_type Count of hosts returned from discovery by pool type
# TYPE vtgateproxy_json_discovery_host_pool_type counter
vtgateproxy_json_discovery_host_pool_type{type="vifl"} 4
vtgateproxy_json_discovery_host_pool_type{type="web"} 4
# HELP vtgateproxy_json_discovery_unchanged JSON host discovery parsed and determined no change to the file
# TYPE vtgateproxy_json_discovery_unchanged counter
vtgateproxy_json_discovery_unchanged 0

This way we only have a single entity watching the file and dispatching out to the resolvers when things change rather than a bunch of tickers watching the file. Also cleaned up the code a bunch.

demmer · 2024-04-12T13:46:03Z

go/cmd/vtgateproxy/vtgateproxy.go

 	"vitess.io/vitess/go/exit"
 	"vitess.io/vitess/go/stats/prometheusbackend"
 	"vitess.io/vitess/go/vt/servenv"
 	"vitess.io/vitess/go/vt/vtgateproxy"
 )

 func init() {
-	rand.Seed(time.Now().UnixNano())


Pretty sure this is not needed since we never use the global rand.

demmer · 2024-04-12T13:46:59Z

go/vt/vtgateproxy/discovery.go

@@ -31,14 +30,8 @@ import (
 	"vitess.io/vitess/go/vt/log"
 )

-var (
-	jsonDiscoveryConfig = flag.String("json_config", "", "json file describing the host list to use fot vitess://vtgate resolution")


Seemed clearer to put all the flag definitions in the same place in the main vtgateproxy file.

demmer · 2024-04-12T13:49:15Z

go/vt/vtgateproxy/discovery.go

+				continue
+			}
+
+			// notify all the resolvers that the targets changed


This is the core of the change - the resolver "builder" is responsible for watching the file.

Each target then gets a separate resolver class which we track in a list, and notify whenever something changes in the file, at which point the resolver turns around and picks a new set of hosts out of the global list.

demmer · 2024-04-12T13:52:21Z

go/vt/vtgateproxy/discovery.go

 		}
-		hosts = candidates
+
+		// Handle both int and string values for port


This is somewhat regrettable but for silly consul template reasons, the grpc port is a string in our file format.

In talking to @jscheinblum about this, an upstream implementation could be simplified if we wanted to enforce a file format with hard-coded field names, which is more consistent with the rest of the package and then we wouldn't need all these xyzField command line flags.

It would of course mean that we would need to render a second file for vtgate hosts in the format that the proxy wants, or change webapp to use this other format.

Regardless, this approach seemed like the cleaner one for us for now.

demmer · 2024-04-12T13:57:27Z

go/vt/vtgateproxy/discovery.go

+	// the affinity matching (e.g. same az) hosts at the front and the non-matching ones
+	// at the end.
+	//
+	// Only need to do n-1 swaps since the last host is always in the right place.


I did some testing of this on the playground:
https://go.dev/play/p/a_nnDVEgkq-

demmer · 2024-04-12T13:58:12Z

go/vt/vtgateproxy/discovery.go

-func (r *JSONGateConfigResolver) Close() {
-	r.ticker.Stop()
+
+// Utilities


Alas this seems silly but it's apparently the "canonical" golang way to do it.

demmer · 2024-04-12T13:59:28Z

go/vt/vtgateproxy/vtgateproxy.go

-	affinityAttr = flag.String("affinity_attr", "", "Attribute (both mysql protocol connection and JSON file) used to specify the routing affinity , e.g. 'az_id'")
+	vtgateHostsFile = flag.String("vtgate_hosts_file", "", "json file describing the host list to use for vtgate:// resolution")
+	numConnections  = flag.Int("num_connections", 4, "number of outbound GPRC connections to maintain")
+	poolTypeField   = flag.String("pool_type_field", "", "Field name used to specify the target vtgate type and filter the hosts")


This is another change to the flags so we'll need to update the dockerfile again.

henryr · 2024-04-12T16:13:56Z

go/vt/vtgateproxy/discovery.go


-func (r *JSONGateConfigResolver) resolve() (*[]resolver.Address, []byte, error) {
+	// Start a config watcher
+	b.ticker = time.NewTicker(100 * time.Millisecond)


Ten times a second is probably overkill.

Probably. It's only a stat call however. Also this isn't changed from the current implementation (I don't think). But yeah maybe 1 / sec is fine?

The thing is, if there's an error, it's going to log ten times a second.

Smart. 1 / sec sound good to you? Alternatively we could stat faster and damp the error logs?

henryr · 2024-04-12T16:15:39Z

go/vt/vtgateproxy/discovery.go

+				log.Errorf("Error stat'ing config %v\n", err)
+				continue
+			}
+			isUnchanged := checkFileStat.Size() == fileStat.Size() || checkFileStat.ModTime() == fileStat.ModTime()


should this be &&? file sizes can stay constant, particularly if only hostnames are changed.

This is also just moved around, but you're definitely right. Will change.

OK I did this change and then tested in dev:

Touch the file (unchanged, no reparse)
Add whitespace to file (reparsed)
Single character change to file (reparsed)

henryr · 2024-04-12T17:04:35Z

go/vt/vtgateproxy/discovery.go

+	affinity := ""
+	if b.affinityField != "" {
+		affinity = attrs.Get(b.affinityField)


Do I have this right: grpc will call Build when a new client connection is created, which is identified by the vtgate://<type> portion of the URL.

So if we ask for the same <type> but with two different affinity attributes, we will only create one resolver corresponding to the first affinity.

This likely works in practice because why would we change the affinity from the local AZ? But it seems more correct to defer affinity to the actual subconn picking step (which would probably mean implementing a custom load balancer).

But it seems more correct to defer affinity to the actual subconn picking step (which would probably mean implementing a custom load balancer).

That's how @jscheinblum originally implemented it before the refactor that preceded this refactor.

The problem is that GRPC will connect to every host that you return from the discovery list. So you end up with all these actually unused TCP connections. So, we had to move the affinity check and the subsetting into the discovery phase and not the load balancing phase.

Do I have this right: grpc will call Build when a new client connection is created, which is identified by the vtgate:// portion of the URL.

Not quite -- Build will be called based on the full URL uniqueness. So even if you change things so there's a different az_id or we omit az_id altogether, it will get a new set of hosts for that target.

I just verified this in dev and tweaked the metrics a bit so that if we ever don't know the az_id of the target host we now track it as "unknown"

henryr · 2024-04-12T17:05:38Z

go/vt/vtgateproxy/discovery.go


-			hash = newHash
+	b.update(r)
+	b.resolvers = append(b.resolvers, r)


The reason we never clear this is because we expect to create exactly one resolver per target during the lifetime of the process, right?

Correct. We either reuse the existing resolver or create a new one, we never need to remove them.

* refactor a bit more to consolidate the command line flags * rework again to have a single parser and move around all the logic This way we only have a single entity watching the file and dispatching out to the resolvers when things change rather than a bunch of tickers watching the file. Also cleaned up the code a bunch. * redo the shuffle to be a single pass * actually use the builder rand not the global one * add some metrics * only stat once per second * split out counter for unknown affinity

demmer added 3 commits April 12, 2024 06:38

refactor a bit more to consolidate the command line flags

20785c7

rework again to have a single parser and move around all the logic

2622872

This way we only have a single entity watching the file and dispatching out to the resolvers when things change rather than a bunch of tickers watching the file. Also cleaned up the code a bunch.

redo the shuffle to be a single pass

ff32f97

demmer commented Apr 12, 2024

View reviewed changes

actually use the builder rand not the global one

9e7124f

demmer commented Apr 12, 2024

View reviewed changes

add some metrics

2f5ab37

demmer changed the title ~~Vtgateproxy more efficient shuffle 2~~ Vtgateproxy rework discovery and add stats Apr 12, 2024

demmer changed the title ~~Vtgateproxy rework discovery and add stats~~ Vtgateproxy rework discovery and add metrics Apr 12, 2024

henryr reviewed Apr 12, 2024

View reviewed changes

demmer added 2 commits April 12, 2024 10:10

only stat once per second

90b7e0f

split out counter for unknown affinity

247023a

henryr approved these changes Apr 12, 2024

View reviewed changes

demmer merged commit 5ea7ded into vtgateproxy Apr 15, 2024
152 of 241 checks passed

demmer deleted the vtgateproxy-more-efficient-shuffle-2 branch April 15, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vtgateproxy rework discovery and add metrics #296

Vtgateproxy rework discovery and add metrics #296

demmer commented Apr 12, 2024 •

edited

Loading

demmer Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

henryr Apr 12, 2024

demmer Apr 12, 2024

henryr Apr 12, 2024

demmer Apr 12, 2024

henryr Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

henryr Apr 12, 2024

demmer Apr 12, 2024

demmer Apr 12, 2024

henryr Apr 12, 2024

demmer Apr 12, 2024

Vtgateproxy rework discovery and add metrics #296

Vtgateproxy rework discovery and add metrics #296

Conversation

demmer commented Apr 12, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demmer commented Apr 12, 2024 •

edited

Loading