Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlproxyccl: Add codeUnavailable to the list of error codes #77442

Merged

Conversation

alyshanjahani-crl
Copy link
Collaborator

@alyshanjahani-crl alyshanjahani-crl commented Mar 7, 2022

Release justification: fixes for high-priority bug in existing functionality

Previously, if a tenant cluster had maxPods set to 0 the error returned by
directory.EnsureTenantAddr was not treated as a non-retryable error.

The tenant directory implementation used in CockroachCloud now identifies
this situation and returns a status error with codes.FailedPrecondition and
a descriptive message.

This patch adds a check for the FailedPrecondition error in
connector.lookupAddr.

Release note: None

@alyshanjahani-crl alyshanjahani-crl requested a review from a team as a code owner March 7, 2022 17:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@@ -319,7 +319,11 @@ func (c *connector) lookupAddr(ctx context.Context) (string, error) {
if c.Directory != nil {
addr, err := c.Directory.EnsureTenantAddr(ctx, c.TenantID, c.ClusterName)
if err != nil {
if status.Code(err) != codes.NotFound {
if status.Code(err) == codes.FailedPrecondition {
if st, ok := status.FromError(err); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are unable to retrieve the status, we should still return a codeUnavailable error and break from the loop. I'm worried this case could trigger an infinite loop.

@@ -319,7 +319,11 @@ func (c *connector) lookupAddr(ctx context.Context) (string, error) {
if c.Directory != nil {
addr, err := c.Directory.EnsureTenantAddr(ctx, c.TenantID, c.ClusterName)
if err != nil {
if status.Code(err) != codes.NotFound {
if status.Code(err) == codes.FailedPrecondition {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it would be nice to flatten this logic. I'm thinking something like

switch {
case err == nil:
  return add, nil
case status.Code(err) == codes.FailedPrecondition:
  message = "unavailable"
  if st, ok := status.FromError(err); ok {
    message = st.Message()
  }
  return "", newErrorf(codeUnavailable, st.Message()
case status.Code(err) != codes.NotFound:
  return "", markAsRetriableConnectionError(err)
default: 
  /* fallback to old resolution rule */
}

Copy link
Collaborator Author

@alyshanjahani-crl alyshanjahani-crl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how I go about generating errorcode_string.go it says it is generated by "stringer". Since i've added codeUnavailable i need to re-generate this file.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)


pkg/ccl/sqlproxyccl/connector.go, line 323 at r1 (raw file):

Previously, JeffSwenson (Jeff Swenson) wrote…

If we are unable to retrieve the status, we should still return a codeUnavailable error and break from the loop. I'm worried this case could trigger an infinite loop.

Done.

Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

You can use dev to regenerate errcode_string.go

./dev generate

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)

@alyshanjahani-crl alyshanjahani-crl force-pushed the proxy-add-unavailable-err branch 6 times, most recently from 8e83ae5 to 897df56 Compare March 13, 2022 18:03
Copy link
Collaborator

@jaylim-crl jaylim-crl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: without the release note. SQL proxy is internal only for now, so no release notes are needed.

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

@alyshanjahani-crl
Copy link
Collaborator Author

SQL proxy is internal only for now, so no release notes are needed.

Ack, amended the commit msg to remove release note.

TFTR

Copy link
Collaborator

@jaylim-crl jaylim-crl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would sill want Release note: None 😄 Same to PR message I think.

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

Release justification: fixes for high-priority bug in existing functionality

Previously, if a tenant cluster had maxPods set to 0 the error returned by
directory.EnsureTenantAddr was not treated as a non-retryable error.

The tenant directory implementation used in CockroachCloud now identifies
this situation and returns a status error with codes.FailedPrecondition and
a descriptive message.

This patch adds a check for the FailedPrecondition error in
connector.lookupAddr.

Release note: None
@alyshanjahani-crl
Copy link
Collaborator Author

haha, thx for the quick response ;)
Done.

@alyshanjahani-crl
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 14, 2022

Build succeeded:

@craig craig bot merged commit 51cbdce into cockroachdb:master Mar 14, 2022
@blathers-crl
Copy link

blathers-crl bot commented Mar 14, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating backport branch refs/heads/blathers/backport-release-21.2-77442: POST https://api.github.com/repos/cockroachlabs/cockroach/git/refs: 403 Resource not accessible by integration []

Backport to branch 21.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@alyshanjahani-crl
Copy link
Collaborator Author

blathers backport 21.2

@blathers-crl
Copy link

blathers-crl bot commented Mar 15, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating backport branch refs/heads/blathers/backport-release-21.2-77442: POST https://api.github.com/repos/cockroachlabs/cockroach/git/refs: 403 Resource not accessible by integration []

Backport to branch 21.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants