sqlproxy: fix flakiness and other small issues

[andy-kimball]

• Fix flakiness of the TestDirectoryConnect test.
• Fix minor deleteEntry race condition.
• Consolidate EventType and PodState enumerations.
• Do not add DRAINING pod addresses to directory.
• Avoid locking to check tenantEntry initialization.

Fixes #67406

Release note: None

[cockroach-teamcity]

This change is 

[jaylim-crl]

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

---

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

  Pod pod = 1;

Note that we should only bump the version of sqlproxy in the CC codebase once tenantdir has been updated to reflect these changes as well. Changing the ordering is fine since both tenantdir and sqlproxy will start up at the same time.

---

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

Quoted 4 lines of code…

	// If Initialize has already been successfully called, nothing to do.
	if e.initialized.Load() {
		return nil
	}

Hm, I wonder if it's cleaner to use sync.Once here.

[andy-kimball]

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

---

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

Previously, jaylim-crl (Jay Lim) wrote…

  Pod pod = 1;

Note that we should only bump the version of sqlproxy in the CC codebase once tenantdir has been updated to reflect these changes as well. Changing the ordering is fine since both tenantdir and sqlproxy will start up at the same time.

Ack.

---

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

Previously, jaylim-crl (Jay Lim) wrote…

	// If Initialize has already been successfully called, nothing to do.
	if e.initialized.Load() {
		return nil
	}

Hm, I wonder if it's cleaner to use sync.Once here.

Yeah, it definitely is cleaner - I was just reimplementing it here. I switched to use it.

[jaylim-crl]

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

---

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

func (e *tenantEntry) RefreshPods(ctx context.Context, client DirectoryClient) error {
	if !e.initialized.Load() {
		return errors.AssertionFailedf("entry for tenant %d is not initialized", e.TenantID)

Now that we've removed these assertions, what happens if we invoke RefreshPods / ChoosePodAddr whenever the entry hasn't been initialized?

---

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

	// If Initialize has already been successfully called, nothing to do.
	e.initialized.Do(func() {
		tenantResp, err := client.GetTenant(ctx, &GetTenantRequest{TenantID: e.TenantID.ToUint64()})

Do we also need to synchronize access to to the Directory service through e.calls here?

[andy-kimball]

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

---

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

Previously, jaylim-crl (Jay Lim) wrote…

Now that we've removed these assertions, what happens if we invoke RefreshPods / ChoosePodAddr whenever the entry hasn't been initialized?

This was just here to catch bugs. But the bugs are quite unlikely and these have never fired. I don't think it's worth adding back an initialized boolean just to catch this very unlikely bug.

---

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

Previously, jaylim-crl (Jay Lim) wrote…

Do we also need to synchronize access to to the Directory service through e.calls here?

No, because this is always called first before anything else. Any other threads will wait on the sync.Once until this initial call is complete.

[andy-kimball]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)