sqlproxyccl: statistically load balance across tenants

[chrisseto]

Previously, the SQLProxy's Directory implementation would always
select the first instance from the pods list. As part of the
SQLProxy's duty is load balancing, this was inadequate. This commit
upgrades the tenant dir to have pods report their CPU load. This
load is then used to perform a weighted distribution across all
running tenant pods.

Release note: None

[cockroach-teamcity]

This change is 

[andy-kimball]

, except for the RNG thread-safety issue. @darinpp should also take a look.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chrisseto)

---

pkg/ccl/sqlproxyccl/tenant/directory.go, line 175 at r1 (raw file):

			codes.NotFound, "tenant %d not in directory cache", tenantID.ToUint64())
	}
	tenantPods := entry.getPods()

UBER-NIT: Add a blank line before this. I treat blank lines like paragraph breaks; they break up code by cohesive "ideas". In this case, we're beginning a new idea, which is to get pod addresses. Doing this makes code easier to read, just as paragraphs do in writing.

---

pkg/ccl/sqlproxyccl/tenant/directory.go, line 341 at r1 (raw file):

			}

			podLoad := resp.Load

UBER-NIT: This is the reverse of the last comment: I'd remove the blank line after podLoad, b/c it's very much connected with the "idea", which is to extract and check pod fields.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 25 at r1 (raw file):

)

// tenantPod is a single instance of a tenant running at the specificed

NIT: I think we should break this struct and selectTenantPod into its own file. My general rule of thumb is to split out each struct into its own file unless it's trivial and doesn't force the main parts of the file (i.e. tenantEntry) so far down that I have to scroll down a ways to see it. Ideally, for a file called "entry", the "entry" struct is visible in the first page of an editor.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 85 at r1 (raw file):

	initError error

	rng *rand.Rand

randutil.NewPseudoRand is not safe for concurrent access, so it needs to be protected by the pods mutex when used.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 169 at r1 (raw file):

// are no pods available rather than blocking.
//
// TODO(andyk): Use better load-balancing algorithm once tenants can have more

NIT: Can remove this comment now.

---

pkg/ccl/sqlproxyccl/tenant/entry_test.go, line 36 at r1 (raw file):

	}

	// Assert that the distribution

NIT: comment is incomplete

[chrisseto]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @chrisseto)

---

pkg/ccl/sqlproxyccl/tenant/directory.go, line 175 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

UBER-NIT: Add a blank line before this. I treat blank lines like paragraph breaks; they break up code by cohesive "ideas". In this case, we're beginning a new idea, which is to get pod addresses. Doing this makes code easier to read, just as paragraphs do in writing.

Agreed, I'm not sure why I missed this line.

---

pkg/ccl/sqlproxyccl/tenant/directory.go, line 341 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

UBER-NIT: This is the reverse of the last comment: I'd remove the blank line after podLoad, b/c it's very much connected with the "idea", which is to extract and check pod fields.

Done.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 85 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

randutil.NewPseudoRand is not safe for concurrent access, so it needs to be protected by the pods mutex when used.

Done. Move the rng into the pods struct. I'm guessing it won't matter that much but we have added another point to lock just for the rng. Would it be better to give the rng its own mutex?

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 169 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

NIT: Can remove this comment now.

Done.

---

pkg/ccl/sqlproxyccl/tenant/entry_test.go, line 36 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

NIT: comment is incomplete

Done.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto)

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 85 at r1 (raw file):

Previously, chrisseto (Chris Seto) wrote…

Done. Move the rng into the pods struct. I'm guessing it won't matter that much but we have added another point to lock just for the rng. Would it be better to give the rng its own mutex?

I do have one suggestion below to avoid locking for longer than necessary.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 151 at r2 (raw file):

	}

	e.pods.Lock()

You only need a single random number for selectTenantPod. Perhaps generate it here and pass it in? Or maybe you could make selectTenantPod a member of tenantEntry so you have direct access to the rng and can put a bit of code directly into that routine:

func (e *tenantEntry) randFloat32() float32 {
  e.pods.Lock()
  defer e.pods.Unlock()
  return e.pods.rng.Float32()
}

[darinpp]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto)

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 171 at r3 (raw file):

	for i, existing := range e.pods.pods {
		if existing.Addr == addr {
			// Clone pods to avoid modifiying entries that may have been

It isn't clear why the entries shouldn't be modified. This needs a better comment explaining why that is the case.

---

pkg/ccl/sqlproxyccl/tenant/pod.go, line 25 at r3 (raw file):

// incoming traffic. Pods are weighted by their reported CPU load. rand must be
// a pseudo random number within the bounds [0, 1).
func selectTenantPod(rand float32, pods []tenantPod) tenantPod {

This also need an explanation. It isn't clear at a first glance how rand is used to choose a pod . Perhaps put an example here.

[chrisseto]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @darinpp)

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 85 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I do have one suggestion below to avoid locking for longer than necessary.

Done.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 151 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

You only need a single random number for selectTenantPod. Perhaps generate it here and pass it in? Or maybe you could make selectTenantPod a member of tenantEntry so you have direct access to the rng and can put a bit of code directly into that routine:

func (e *tenantEntry) randFloat32() float32 {
  e.pods.Lock()
  defer e.pods.Unlock()
  return e.pods.rng.Float32()
}

Done.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 171 at r3 (raw file):

Previously, darinpp wrote…

It isn't clear why the entries shouldn't be modified. This needs a better comment explaining why that is the case.

entries are now a list of *Pod proto values and this no longer applies.

---

pkg/ccl/sqlproxyccl/tenant/pod.go, line 25 at r3 (raw file):

Previously, darinpp wrote…

This also need an explanation. It isn't clear at a first glance how rand is used to choose a pod . Perhaps put an example here.

Added a snippet and a suggestion to use rand.Float32()

[jaylim-crl]

Reviewed 9 of 9 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @chrisseto, and @darinpp)

---

pkg/ccl/sqlproxyccl/tenant/directory.proto, line 42 at r4 (raw file):

Quoted 5 lines of code…

  // UNKNOWN indicates that the pod values being reported are from a
  // potentially out of date source. UNKNOWN may be used to notify updates to
  // pod values when the pod's state may be out of date by the time the update
  // is processed.
  UNKNOWN = 3;

How do we know that the source is potentially out of date? Do you have an example of a scenario? In particular, I'm looking at the logic here: tenantdir/main.go#L163-L191 in CC.

Load reporters may elect to use the new UNKNOWN
state to indicate their updates should be ignored if the proxy is
unaware of a running pod at the given address.

What are the implications of using RUNNING in that case? I wonder if the new state is necessary, and whether we can piggy back on the existing states.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 159 at r4 (raw file):

Quoted 4 lines of code…

			pods := e.pods.pods
			e.pods.pods = make([]*Pod, len(pods))
			copy(e.pods.pods, pods)
			e.pods.pods[i] = pod

Why do we need to perform a copy here? Maybe a comment would be helpful. Same goes to the logic in UpdatePod.

[chrisseto]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @darinpp, and @jaylim-crl)

---

pkg/ccl/sqlproxyccl/tenant/directory.proto, line 42 at r4 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

  // UNKNOWN indicates that the pod values being reported are from a
  // potentially out of date source. UNKNOWN may be used to notify updates to
  // pod values when the pod's state may be out of date by the time the update
  // is processed.
  UNKNOWN = 3;

How do we know that the source is potentially out of date? Do you have an example of a scenario? In particular, I'm looking at the logic here: tenantdir/main.go#L163-L191 in CC.

Load reporters may elect to use the new UNKNOWN
state to indicate their updates should be ignored if the proxy is
unaware of a running pod at the given address.

What are the implications of using RUNNING in that case? I wonder if the new state is necessary, and whether we can piggy back on the existing states.

If a non-definitive source is sending load information (prometheus), there's always a chance that a pod could get deleted directly before a load update is sent to the proxy.

There's a test case in directory_test that show cases the example but concretely we may ship updated load metrics from a prometheus scrape without noticing that the pod was deleted. To avoid re-creating the in memory entry for a "zombie" state there needs to be some way to ensure that we only update the pod if it already exists.

We could check for pod existence before shipping over load information but there's still a very tiny race condition there. I also considered keeping around deleted pod entries so we could discard any modifications to them but that felt like a lot of effort compared to this.

---

pkg/ccl/sqlproxyccl/tenant/entry.go, line 159 at r4 (raw file):

Previously, jaylim-crl (Jay Lim) wrote…

			pods := e.pods.pods
			e.pods.pods = make([]*Pod, len(pods))
			copy(e.pods.pods, pods)
			e.pods.pods[i] = pod

Why do we need to perform a copy here? Maybe a comment would be helpful. Same goes to the logic in UpdatePod.

Comment added. In the long run we may want to switch to copy on read now that this isn't just a slice of strings 🤔

[jaylim-crl]

Just a heads up, we need to update the tenantdir to reflect the new proto changes in #67452 when we bump the vendor in CC.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @darinpp, and @jaylim-crl)

---

pkg/ccl/sqlproxyccl/tenant/directory.proto, line 42 at r4 (raw file):

Previously, chrisseto (Chris Seto) wrote…

If a non-definitive source is sending load information (prometheus), there's always a chance that a pod could get deleted directly before a load update is sent to the proxy.

There's a test case in directory_test that show cases the example but concretely we may ship updated load metrics from a prometheus scrape without noticing that the pod was deleted. To avoid re-creating the in memory entry for a "zombie" state there needs to be some way to ensure that we only update the pod if it already exists.

We could check for pod existence before shipping over load information but there's still a very tiny race condition there. I also considered keeping around deleted pod entries so we could discard any modifications to them but that felt like a lot of effort compared to this.

Ack. Thanks for the clarifications.

[chrisseto]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)