[closes #4378] Add keyspace visibility filtering to vtgates by setassociative · Pull Request #4420 · vitessio/vitess

setassociative · 2018-12-04T23:29:03Z

Summary

This was added in order to support running up a vtgate within a cell that has a limited access to the keyspaces inside that cell. A new flag was introduced instead of modifying the existing tablet_filters behavior because bringing along the |<shard-or-keyrange> syntax didn't make sense based on the desired behavior. tablet_filters and keyspaces_to_watch were made mutually exclusive to prevent situations where the flags required different keyspaces which would have suggested a non-obvious behavior (or had to fail anyway).

Implementation

Introduce a new filtering srvtopo.Server implementation that removes unwatched keyspaces from its results
This new filtering server does not expose access to its supporting topo.Server to prevent corrupting global SrvVSchema (c.f. this explainer from @demmer)
Updates callsites for GetSrvTopo to handle the error coming back

It additionally comes with a super thin implementation of a srvtopo.Server for tests.

Open Questions

With GetTopoServer now returning an error I updated vschema_manager.go to cache a single reference to the topo server instead of getting a new one on each use to minimize error handling.

In theory this could be problematic if the server pointer changes during this call but I wasn't able to identify any code that does that (and the get call isn't synchronized so it seems super unlikely).

Testing

Automated: Unit tests for the new filtering srvtopo.Server impl, vagrant-scriptsh/test.sh passes

Manual:

Using the vagrant setup I spun up a cluster with:

2 keyspaces: aaa_keyspace, test_keyspace
10 vttablets: 5 fronting each keyspace
1 vtgate, 1vtctld

when not watching a specific keyspace mysql cli showed both keyspaces on show vitess_keyspaces
when keyspaces_to_watch=test_keyspace was set only test_keyspace was returned for show vitess_keyspaces; select now() from dual; was functional

### Summary This was added in order to support spining up a vtgate within a cell that has a limited access to the keyspaces inside that cell. It was added instead of modifying the existing `filter_tablets` behavior because bringing in the `|<shard-or-keyrange>` syntax didn't make sense based on the desired behavior. `filter_tablets` and `keyspaces_to_watch` were made mutually exclusive to prevent situations where the flags required different keyspaces which would have implied a non-obvious behavior (or had to fail anyway). Implementation was relatively straight-forward: I added a new srvtopo.Server that can filter the keyspaces returned and passed to the callback for WatchSrvVSchema as well as tests. If a call is made to get the underlying topo.Server is returned, e.g., calls to a server Conn's ListDir will not be have any filtering applied. ### Testing Automated: Unit tests for the new filtering `srvtopo.Server` impl Manual: Using the vagrant setup I spun up a cluster with: 2 keyspaces: aaa_keyspace, test_keyspace 10 vttablets: 5 fronting each keyspace 1 vtgate, 1vtctld - when not watching a specific keyspace showed both keyspaces on `show vitess_keyspaces` - when `keyspaces_to_watch=test_keyspace` was set only `test_keyspace` was returned for `show vitess_keyspaces`; `select now() from dual;` was functional Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

…gate.go. Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

demmer

Overall looking good -- some suggestions on naming and organization and maybe a better approach to testing

go/vt/srvtopo/server.go

go/vt/vtgate/gateway/discoverygateway.go

go/vt/vtgate/filters/filters.go

go/vt/srvtopo/server.go

go/vt/srvtopo/server_test.go

…filtering

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

…ssthrough Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

…at instead of real code so that tests are easier to reason about Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

stop using init in filtering server test Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

demmer

Looking good overall - just the minor cleanup and one slightly more substantive change that I mentioned.

go/vt/srvtopo/keyspace_filtering_server_test.go

go/vt/srvtopo/srvtopotest/srvtopotest.go

go/vt/vtgate/gateway/gateway.go

go/vt/srvtopo/keyspace_filtering_server.go

…server. tl;dr is that the TopoServer is only used for vschema updates but also assumes that it has a full copy of the vschema so using it like that would result in losing other keyspaces. A full explanation as to why is in vitessio#4420 (comment) Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

…te being attached to a generic accessor function Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative · 2018-12-13T22:10:51Z

Pending CI passing this should be good to review again. I updated the summary to reflect the actual state of the change.

demmer

LGTM! Nice work

Richard Bailey added 2 commits December 4, 2018 14:28

move log message into vtgate impl

e821613

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative requested a review from sougou as a code owner December 4, 2018 23:29

Richard Bailey added 2 commits December 4, 2018 15:44

swap to a different context so that the linter is happy

44b5ded

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Clean up reference to selectedKeyspaces which is not in scope from vt…

7dcd85e

…gate.go. Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

demmer requested changes Dec 6, 2018

View reviewed changes

Richard Bailey added 2 commits December 5, 2018 18:38

Merge branch 'master' into setassociative-vitessio#4378-add-keyspace-…

b96d2f1

…filtering

PR feedback

893ca57

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative force-pushed the setassociative-#4378-add-keyspace-filtering branch from f3de7be to 950de57 Compare December 7, 2018 02:25

minor unit test cleanups, add test coverage for nil schema & error pa…

461e7a0

…ssthrough Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative force-pushed the setassociative-#4378-add-keyspace-filtering branch from 950de57 to 461e7a0 Compare December 7, 2018 02:26

Rename error to meet lint rules

1110a8c

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative closed this Dec 7, 2018

setassociative reopened this Dec 7, 2018

add super basic srvtopo.Server impl for use in tests and switch to th…

744c6b5

…at instead of real code so that tests are easier to reason about Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative force-pushed the setassociative-#4378-add-keyspace-filtering branch from c85e082 to 744c6b5 Compare December 7, 2018 21:16

Shift filter.KeyspacesToWatch into gateway package per chat w/ demmer

bef3eb5

stop using init in filtering server test Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

demmer requested changes Dec 13, 2018

View reviewed changes

Richard Bailey added 4 commits December 13, 2018 13:03

Updated error mesasge because the previous was awfully specific despi…

944d406

…te being attached to a generic accessor function Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Document the error behavior

fe3499c

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

Test that everything gets filtered and the return is empty, but not nil

a72b957

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>

setassociative force-pushed the setassociative-#4378-add-keyspace-filtering branch from 9de060f to a72b957 Compare December 13, 2018 21:52

demmer approved these changes Dec 14, 2018

View reviewed changes

demmer merged commit 21b226f into vitessio:master Dec 14, 2018

spark4 mentioned this pull request Feb 14, 2020

Healthcheck - limit healthchecks by keyspace #5815

Merged

setassociative mentioned this pull request Sep 7, 2021

VStream does not work when keyspaces_to_watch is set on vtgate #8772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[closes #4378] Add keyspace visibility filtering to vtgates#4420

[closes #4378] Add keyspace visibility filtering to vtgates#4420
demmer merged 14 commits intovitessio:masterfrom
tinyspeck:setassociative-#4378-add-keyspace-filtering

setassociative commented Dec 4, 2018 •

edited

Loading

Uh oh!

demmer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

demmer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

setassociative commented Dec 13, 2018 •

edited

Loading

Uh oh!

demmer left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

setassociative commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

Open Questions

Testing

Uh oh!

demmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

demmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

setassociative commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

demmer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

setassociative commented Dec 4, 2018 •

edited

Loading

setassociative commented Dec 13, 2018 •

edited

Loading