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.

Sign up for GitHub

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

Jump to bottom

"Terminate Session" from All Clusters / cluster-name / Sessions doesn't end the SQL cli session #70832

Closed
kevin-v-ngo opened this issue Sep 28, 2021 · 3 comments
Closed

"Terminate Session" from All Clusters / cluster-name / Sessions doesn't end the SQL cli session #70832

kevin-v-ngo opened this issue Sep 28, 2021 · 3 comments
Assignees
@matthewtodd
Labels
A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@kevin-v-ngo
Copy link

kevin-v-ngo commented Sep 28, 2021 •
edited by exalate-issue-sync bot
Loading

Additional context. This behavior differs from CockroachDB self hosted where the sessions page should be cleared.

Epic CC-5060
@kevin-v-ngo kevin-v-ngo added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console labels Sep 28, 2021
@matthewtodd
Copy link
Contributor

And "Terminate Statement" doesn't work, either.

When I click these buttons in the UI, I don't see any requests in the inspector, suggesting a redux wiring issue.

On the server side, the tenantStatusServer does offer CancelQuery and CancelSession, but they're not in the list of authorized methods for tenants, suggesting we'll need to open them up there, too.

I'll do the server-side work first, since it's more urgent for backporting.

@matthewtodd
Copy link
Contributor

In the frontend, requests aren't being fired off because, while we we import the terminateSaga, we neglect to include it in the list here.

This was referenced Sep 30, 2021
Closed
Merged
craig bot pushed a commit that referenced this issue Oct 1, 2021
@matthewtodd
Merge #70955
6961f28 
70955: cluster-ui: hide terminate session / query buttons r=matthewtodd a=matthewtodd

Temporarily mitigates #70832

Currently, these buttons do not work for tenant clusters, both because
of a redux oversight and because the we still need to build out the
endpoints they would hit in the tenant status server.

Until we can get to that work, let's just turn them off.

Release justification: Category 2: Bug fixes and low-risk updates to new
functionality.

Release note (ui): The terminate session and terminate query buttons
have been temporarily disabled in the cluster ui.

Co-authored-by: Matthew Todd <[email protected]>
blathers-crl bot pushed a commit that referenced this issue Oct 1, 2021
@matthewtodd
cluster-ui: hide terminate session / query buttons
ec80b88 
Temporarily mitigates #70832

Currently, these buttons do not work for tenant clusters, both because
of a redux oversight and because the we still need to build out the
endpoints they would hit in the tenant status server.

Until we can get to that work, let's just turn them off.

Release justification: Category 2: Bug fixes and low-risk updates to new
functionality.

Release note (ui change): The terminate session and terminate query
buttons have been temporarily disabled in the cluster ui.
@blathers-crl blathers-crl bot mentioned this issue Oct 1, 2021
Merged
@matthewtodd matthewtodd mentioned this issue Oct 5, 2021
Merged
andy-kimball pushed a commit that referenced this issue Oct 6, 2021
@matthewtodd @andy-kimball
cluster-ui: hide terminate session / query buttons
e0b20f2 
Temporarily mitigates #70832

Currently, these buttons do not work for tenant clusters, both because
of a redux oversight and because the we still need to build out the
endpoints they would hit in the tenant status server.

Until we can get to that work, let's just turn them off.

Release justification: Category 2: Bug fixes and low-risk updates to new
functionality.

Release note (ui change): The terminate session and terminate query
buttons have been temporarily disabled in the cluster ui.
craig bot pushed a commit that referenced this issue Oct 12, 2021
@matthewtodd @otan
Merge #71174 #71427
171587f 
71174: server: Tenant pods fan out to cancel SQL sessions r=matthewtodd a=matthewtodd

Partially addresses #70832.
    
Previously, the tenant `/_status/cancel_session` endpoint was not
available over HTTP and could not cancel a session not running locally.
This change opens it up, so that we can call it from the cloud console,
and makes sure that that we can cancel sessions when a tenant has more
than one SQL pod running.
    
Release note (api change): The tenant `/_status/cancel_session` endpoint
was made available over HTTP and augmented to fan out requests in a
multi-pod world.

71427: tree: add enum array comparator ops r=rafiss a=otan

Refs #71388

Release note (sql change): Arrays of enums can now be compared.

Co-authored-by: Matthew Todd <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
@matthewtodd matthewtodd mentioned this issue Oct 13, 2021
Merged
@matthewtodd
Copy link
Contributor

matthewtodd commented Oct 13, 2021 •
edited
Loading

Remaining work here:

craig bot pushed a commit that referenced this issue Oct 19, 2021
@dhartunian @matthewtodd
@yuzefovich @tbg
Merge #70792 #71503 #71677 #71698
c13e449 
70792: server: expect duplicate session cookies on HTTP requests r=catj-cockroach,knz a=dhartunian

Previously, the golang HTTP library was used to retrieve a cookie with
the specific name we use to store our authentication token. This API
method retrieves the *first* matching cookie in the header and ignores
the rest. This can cause issues with requests that contain multiple
matching cookies with the same name (if other applications are
potentially running on the same domain).

This change adjusts the HTTP API we use to retrieve cookies (we now
prefer to call `req.Cookies()` and iterate through the entire list
ourselves, instead of relying on the API to give us the matching cookie
since it only supports returning a single result.

Resolves #70376.

Release note (security update): Authenticated HTTP requests to nodes can
now contain additional cookies with the same name as the one we use
("session"). Duplicates like this are permitted by the HTTP spec and our
code will now attempt to parse all cookies with a matching name before
giving up. This can resolve issues with customers who run other services
on the same domain as their CRDB nodes.

71503: server: Tenant pods fan out to cancel SQL queries r=matthewtodd a=matthewtodd

Partially addresses #70832.

Previously, the tenant `/_status/cancel_query` endpoint was not
available over HTTP and could not cancel a query not running locally.
This change opens it up, so that we can call it from the cloud console,
and makes sure that that we can cancel queries when a tenant has more
than one SQL pod running.

Release note (api change): The tenant `/_status/cancel_query` endpoint
was made available over HTTP and augmented to fan out requests in a
multi-pod world.

71677: row: pass in account instead of monitor for KV fetcher r=yuzefovich a=yuzefovich

This commit switches the KV fetcher to expect a memory account rather
than a memory monitor. It additionally creates a separate memory account
for the KV fetcher when it is used by the cFetcher (previously, we were
reusing the monitor from the allocator).

Note that we don't need to update multiple places where we call methods
on possibly nil memory account because all methods have been taught to
be noops when the receiver is `nil`.

Fixes: #55481.

Release note: None

71698: roachtest: add kv95/enc=false/nodes=3/tracing r=[dhartunian,stevendanna] a=tbg

We don't have a good grasp on what it means for performance to turn on
tracing globally. A kv95 (read-mostly) workload is a good baseline here
as we expect it to be among the most severely impacted.

Touches #71694.
Touches #58610.

Release note: None


Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Matthew Todd <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
@matthewtodd matthewtodd mentioned this issue Jan 4, 2022
Merged
craig bot pushed a commit that referenced this issue Jan 6, 2022
@matthewtodd
Merge #74408
31a339c 
74408: cluster-ui: enable terminate session / query buttons r=matthewtodd a=matthewtodd

Addresses #70832.

The backend endpoints to support these actions were backported to the
release-21.2 branch in cbe3347 and first released in version 21.2.2:

```
$ git tag --contains cbe3347
@cockroachlabs/[email protected]
v21.2.2
v21.2.3
```

Cockroach Cloud UI code may selectively enable these buttons by setting
`pages.{sessions,sessionDetails}.showTerminateActions` in the UI Config
slice. The default is `false`, continuing to not show the buttons.

Release note (ui change): The terminate session and terminate query
buttons are again available to be enabled in the cluster ui.

Co-authored-by: Matthew Todd <[email protected]>
blathers-crl bot pushed a commit that referenced this issue Jan 6, 2022
@matthewtodd
cluster-ui: enable terminate session / query buttons
eb5f47f 
Addresses #70832.

The backend endpoints to support these actions were backported to the
release-21.2 branch in cbe3347 and first released in version 21.2.2:

```
$ git tag --contains cbe3347
@cockroachlabs/[email protected]
v21.2.2
v21.2.3
```

Cockroach Cloud UI code may selectively enable these buttons by setting
`pages.{sessions,sessionDetails}.showTerminateActions` in the UI Config
slice. The default is `false`, continuing to not show the buttons.

Release note (ui change): The terminate session and terminate query
buttons are again available to be enabled in the cluster ui.
@blathers-crl blathers-crl bot mentioned this issue Jan 6, 2022
Merged
@matthewtodd matthewtodd mentioned this issue Jan 20, 2022
Merged
craig bot pushed a commit that referenced this issue Jan 21, 2022
@matthewtodd
ui: unconditionally enable the terminate buttons
5b03155 
Follow-on to #70832.

Clusters running the 21.2 release series had to be careful, because the
necessary endpoints weren't available until 21.2.2.

Now that master is headed for 22.1, we can remove the conditions.

Release note: None
craig bot pushed a commit that referenced this issue Jan 22, 2022
@andreimatei @matthewtodd @nicktrav
Merge #74752 #75217 #75244 #75253
b88739e 
74752: sql: fix handling of errors on Sync r=rafiss a=andreimatei

In #74242 we've changed the conn_executor to sometimes commit
transactions on a Sync protocol message. Errors coming from such commits
have to be handled differently from any other errors because they don't
require us to skip further messages until another Sync. However, the
code was not giving them any special treatment, and so we were skipping
whatever the client was sending next. This causes the client to get
descynchronized and deadlock. This patch fixes it by manipulating the
session advancing instructions on Sync.

Release note: None

75217: ui: unconditionally enable the terminate buttons r=matthewtodd a=matthewtodd

Follow-on to #70832.

Clusters running the 21.2 release series had to be careful, because the
necessary endpoints weren't available until 21.2.2.

Now that master is headed for 22.1, we can remove the conditions.

Release note: None

75244: workload: Ignore transaction restart errors. r=miretskiy a=miretskiy

Update pgx helpers to ignore "restart transactions" errors.
Previsouly, these were sent via a "msg" portion of the Log call.
It appears that these messages may also arrive in the "data".

Release Notes: None

75253: vendor: bump Pebble to ec6c21662e65 r=jbowens a=nicktrav

ec6c2166 sstable: add range key support to BlockIntervalCollector
586718d8 cmd/pebble: support testkeys comparer
b58a7bdc docs/rfcs: pebble table format versions
f3d164d8 db: save level iter debug output before closing
7f1a70dc sstable: reorganize sstable buffers and structs

Update the existing `sstable.NewBlockIntervalCollector` call site to add
the additional required parameter for the range-key collector.

Release note: none

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Matthew Todd <[email protected]>
Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Co-authored-by: Nick Travers <[email protected]>
gtr pushed a commit to gtr/cockroach that referenced this issue Jan 24, 2022
@matthewtodd
ui: unconditionally enable the terminate buttons
179ef67 
Follow-on to cockroachdb#70832.

Clusters running the 21.2 release series had to be careful, because the
necessary endpoints weren't available until 21.2.2.

Now that master is headed for 22.1, we can remove the conditions.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matthewtodd matthewtodd

Labels
A-sql-console-general SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@matthewtodd @kevin-v-ngo