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

server: supplement (*statusServer).iterateNodes() with a new iterateSQLPods() #64477

Closed
knz opened this issue Apr 30, 2021 · 1 comment · Fixed by #66675
Closed

server: supplement (*statusServer).iterateNodes() with a new iterateSQLPods() #64477

knz opened this issue Apr 30, 2021 · 1 comment · Fixed by #66675
Assignees
@dhartunian
Labels
A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-server CLI commands that pertain to CockroachDB server processes A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security X-server-triaged-202105

Comments

@knz
Copy link
Contributor

knz commented Apr 30, 2021 •
edited by elinorgarcia
Loading

The (*statusServer).iterateNodes() function exists for KV-level APIs that do RPC fan-outs across all KV nodes.

This is inadequate to implement APIs for SQL data, which need to iterate over SQL pods instead.

For this we need a new/different function .iterateSQLPods() which provides a similar interface but iterates over SQL servers intead:

Care must be taken in the implementation to pass the right parameters to the RPC context, since we are likely to use different TLS configs (#64476)

Epic CRDB-2576
@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-multitenancy Related to multi-tenancy A-cli-server CLI commands that pertain to CockroachDB server processes labels Apr 30, 2021
@knz
Copy link
Contributor Author

knz commented Apr 30, 2021

cc @dhartunian

@knz knz mentioned this issue Apr 30, 2021
Closed
4 tasks
@dhartunian dhartunian self-assigned this Jun 8, 2021
@jlinder jlinder added the T-server-and-security DB Server & Security label Jun 16, 2021
@dhartunian dhartunian mentioned this issue Jun 21, 2021
Merged
dhartunian added a commit to dhartunian/cockroach that referenced this issue Jun 21, 2021
@dhartunian
server: fan-out tenant pods impl for statements API
ef1c947 
Previously, fan-out for requests like the Statements call on the status
server was implemented across nodes. We would like to preservet this
functionality while adding the ability for tenant pods to also fan-out
such a request to all instances for the caller tenant in order to
replicate the statements API endpoint functionality on tenant pods.

The `statusServer` struct is enhanced with `dialPod` and `iteratePods`
methods that support the fan-out for tenant pods. These implementations
are copied from the `dialNode` and `iterateNodes` functions for now with
modifications to handle the different `instanceID` type.

The `statusServer.rpcCtx` now contains an `InstanceID` field that will
contain the current pod's ID.

Open questions:
- Some of this code uses a `NodeID` as an instance ID to keep the
  API from splitting too much. Is it important to avoid this? The
  types both alias the same int32 type.
- The `iteratePods` implementation is copied over mostly from the
  `iterateNodes` function. Should efforts be taken to support both
  options in a single function call? I held off on that until we know
  more about how much the working implementations will differ.

Remaining TODOs:
[ ] - Reconcile this work w/ upcoming instance ID API implementation in
[ ] - Reconcile this work w/ upcoming support for status server
initialization on tenant pods
[ ] - Reoncile this work w/ upcoming support for tenant-tenant RPC
connections from server team

Resolves: cockroachdb#64477

Release note: None (TODO if this PR ends up enabling the endpoint for
this feature for tenant pods)
dhartunian added a commit to dhartunian/cockroach that referenced this issue Jul 22, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
11febcb 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

REVIEWER NOTE: This change is based on cockroachdb#66600 which is still in
progress, please only review the final commit. Once cockroachdb#66600 is merged,
only the final commit will remain on rebase.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Jul 27, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
ad724ac 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

REVIEWER NOTE: This change is based on cockroachdb#66600 which is still in
progress, please only review the final commit. Once cockroachdb#66600 is merged,
only the final commit will remain on rebase.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Jul 28, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
770ccfe 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

REVIEWER NOTE: This change is based on cockroachdb#66600 which is still in
progress, please only review the final commit. Once cockroachdb#66600 is merged,
only the final commit will remain on rebase.
@knz knz added the A-cc-enablement Pertains to current CC production issues or short-term projects label Jul 29, 2021
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 2, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
73090bd 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

REVIEWER NOTE: This change is based on cockroachdb#66600 which is still in
progress, please only review the final commit. Once cockroachdb#66600 is merged,
only the final commit will remain on rebase.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 5, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
a2d58a4 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

REVIEWER NOTE: This change is based on cockroachdb#66600 which is still in
progress, please only review the final commit. Once cockroachdb#66600 is merged,
only the final commit will remain on rebase.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 9, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
dc375b7 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 9, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
2ceab23 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.
craig bot pushed a commit that referenced this issue Aug 9, 2021
@dhartunian @nvanbenschoten @fqazi
Merge #66675 #68497 #68601
764a1a9 
66675: server: pod-to-pod fanout for statements api on tenant  r=dhartunian a=dhartunian

Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves #64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.

68497: kv: don't load applied index twice during Raft snapshot r=nvanbenschoten a=nvanbenschoten

This was harmless, but it was also unnecessary and looked quite
alarming. It would be a major problem if the Raft applied index stored
in a `SnapshotMetadata` was ever incoherent with the applied state in
the snapshot. However, we're reading from a consistent enging snapshot
in `snapshot`, so the Raft applied index can't change. Still, better to
avoid questions.

The "members of the Range struct" comment was very stale, dating back
to acc41dd.

68601: sql: drop view cascade does not handle deps being dropped earlier r=ajwerner a=fqazi

Fixes: #68600

Previously, drop view cascade would only check if a dependency
was dropped when initially going over the list. It did not check
before executing the drop logic, so an earlier dependency could
cause the current object to move to a dropped state. This was
inadequate because cascaded drops of views could fail due to this
reason. To address this, this patch skips dropping dependencies
if at a later stage they are found to be dropped.

Release note (bug fix): Cascaded drop of views could run into
'table ...is already being dropped' errors incorrectly.

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
sajjadrizvi pushed a commit to sajjadrizvi/cockroach that referenced this issue Aug 10, 2021
@dhartunian
server: pod-to-pod fanout for statements api on tenant
722172e 
Previously, the Statements endpoint implementation was purely local in
its execution and reported only in-memory data on the current tenant.
This change initializes a gRPC and gRPC-gateway server on the tenant,
much like we do on the hosts already, and uses the newly added
InstanceID subsystem for tenants to implement a gRPC-based fanout for
the Statements endpoint implementation.

The fanout is done much in the same manner as on hosts and we
expose the status server endpoints via HTTP on the tenant as well.

This change is necessary to support serverless observability features
and provide our UIs access to the Statements endpoint. Future work may
move this API to SQL-only

Resolves cockroachdb#64477

Release note (api change): tenant pods now expose the Statements API at
`/_status/statements` on their HTTP port.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dhartunian dhartunian

Labels
A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-server CLI commands that pertain to CockroachDB server processes A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security X-server-triaged-202105
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server: pod-to-pod fanout for statements api on tenant dhartunian/cockroach
3 participants
@jlinder @knz @dhartunian