Topo: Add version support to GetTopologyPath #15933

mattlord · 2024-05-13T23:02:02Z

Description

This PR adds the ability to view older/previous versions of topology keys. This can be used to follow changes over time and to revert to previous values.

ℹ️ Notes:

This is only supported in the etcd2topo implementation. This is because etcd is the only topo server implementation that stores multiple versions/revisions of a key and allows you to request a specific one. For ZooKeeper and Consul we display the value — which is a modification counter — and return an intuitive error if you use the flag:

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules --data-as-json --version=1
E0518 01:32:36.674734   45693 main.go:56] rpc error: code = Unknown desc = no such topology implementation GetVersion not supported in ZK2 topo

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules --data-as-json --version=1
E0518 01:33:55.596021   46502 main.go:56] rpc error: code = Unknown desc = no such topology implementation GetVersion not supported in consul topo

There is no way to "see all available versions of a node", so you can only step back in time and look at previous values for the node at previous keyspace revisions (see examples below)

Example usage:

pushd examples/local

./101_initial_cluster.sh

❯ vtctldclient ApplyKeyspaceRoutingRules --rules '{"rules":[{"from_keyspace":"commerce", "to_keyspace":"customer"}]}'
{
  "keyspace_routing_rules": {
    "rules": [
      {
        "from_keyspace": "commerce",
        "to_keyspace": "customer"
      }
    ]
  }
}

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules
{
  "name": "Rules",
  "path": "/global/routing_rules/keyspace/Rules",
  "data": "rules:{from_keyspace:\"commerce\" to_keyspace:\"customer\"}",
  "children": [],
  "version": "36"
}

❯ vtctldclient ApplyKeyspaceRoutingRules --rules '{}'
{
  "keyspace_routing_rules": {
    "rules": []
  }
}

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules --data-as-json
{}

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules
{
  "name": "Rules",
  "path": "/global/routing_rules/keyspace/Rules",
  "data": "",
  "children": [],
  "version": "39"
}

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules --data-as-json --version=38
{
  "rules": [
    {
      "from_keyspace": "commerce",
      "to_keyspace": "customer"
    }
  ]
}

❯ vtctldclient ApplyKeyspaceRoutingRules --rules "$(vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules --data-as-json --version=38)"
{
  "keyspace_routing_rules": {
    "rules": [
      {
        "from_keyspace": "commerce",
        "to_keyspace": "customer"
      }
    ]
  }
}

❯ vtctldclient GetTopologyPath /global/routing_rules/keyspace/Rules
{
  "name": "Rules",
  "path": "/global/routing_rules/keyspace/Rules",
  "data": "rules:{from_keyspace:\"commerce\" to_keyspace:\"customer\"}",
  "children": [],
  "version": "43"
}

❯ vtctldclient UpdateThrottlerConfig --enable --custom-query="select 1 from foo" --throttle-app="vplayer" commerce

❯ vtctldclient GetTopologyPath /global/keyspaces/commerce/Keyspace --data-as-json
{
  "durability_policy": "semi_sync",
  "sidecar_db_name": "_vt",
  "throttler_config": {
    "custom_query": "select 1 from foo",
    "enabled": true,
    "throttled_apps": {
      "vplayer": {
        "expires_at": {
          "nanoseconds": 774788000,
          "seconds": "1715964511"
        },
        "name": "vplayer",
        "ratio": 1
      }
    }
  }
}

Related Issue(s)

Fixes: Feature Request: Add support for getting older key values from the topo server #15968

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation PR: Add docs for GetTopologyPath website#1757

Signed-off-by: Matt Lord <[email protected]>

vitess-bot · 2024-05-13T23:02:05Z

Signed-off-by: Matt Lord <[email protected]>

codecov · 2024-05-17T15:52:49Z

Codecov Report

Attention: Patch coverage is 39.18919% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 68.23%. Comparing base (9911880) to head (359a25c).
Report is 12 commits behind head on main.

Files	Patch %	Lines
go/cmd/vtctldclient/command/topology.go	18.18%	9 Missing ⚠️
go/vt/topo/stats_conn.go	0.00%	9 Missing ⚠️
go/vt/topo/etcd2topo/file.go	0.00%	8 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	75.86%	7 Missing ⚠️
go/vt/topo/decode.go	50.00%	4 Missing ⚠️
go/vt/topo/consultopo/file.go	0.00%	2 Missing ⚠️
go/vt/topo/faketopo/faketopo.go	0.00%	2 Missing ⚠️
go/vt/topo/memorytopo/file.go	33.33%	2 Missing ⚠️
go/vt/topo/zk2topo/file.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15933      +/-   ##
==========================================
- Coverage   68.45%   68.23%   -0.22%     
==========================================
  Files        1562     1562              
  Lines      197057   197221     +164     
==========================================
- Hits       134891   134573     -318     
- Misses      62166    62648     +482

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Matt Lord <[email protected]>

mattlord · 2024-05-17T16:59:30Z

go/vt/topo/decode.go

+	case CommonRoutingRulesFile:
+		switch path.Base(dir) {
+		case "keyspace":
+			p = new(vschemapb.KeyspaceRoutingRules)
+		}


This was needed as a follow-up to: #15807

This PR is also what allows you to easily revert to the previous value as we had toyed with the idea of returning the old and new values from the ApplyKeyspaceRoutingRules command for this purpose.

deepthi · 2024-05-17T17:10:17Z

I don't like the idea of doing something that only works with etcd.
Also, how do you know what the available versions are?
What happens if someone is running with zookeeper (which we still support), and tries to use this feature?

mattlord · 2024-05-17T17:19:58Z

I don't like the idea of doing something that only works with etcd.

ZooKeeper does not support this feature generally AFAIK. It's also worth noting that etcd2 is the only implementation that is officially / fully supported AFAIK. IMO it's a nice feature to support where it's possible.

Also, how do you know what the available versions are?

The etcd keyspace has a new revision each time the keyspace is changed. So you would see what the current version is for the key and you can step back from there. That's what I did in the examples in the PR description. So e.g. if you just made a change, you would use current version - 1 to see what value of the node/key was at the previous keyspace revision.

What happens if someone is running with zookeeper (which we still support), and tries to use this feature?

You'll get a an err: GetVersion not supported in ZK2 topo: https://github.com/vitessio/vitess/pull/15933/files#diff-05359f832e44437ddcaef9416e6f0c0fd0997dc84104b166c0948c7570a2a15a

deepthi · 2024-05-17T17:23:25Z

Ok, this seems reasonable given the answers to my questions. Will review.

Signed-off-by: Matt Lord <[email protected]>

mattlord · 2024-05-17T17:58:12Z

@deepthi actually... it seems like ZooKeeper does support versions — there are virtually no docs so I didn't find anything, but... (after fixing the local example in: e5bef58):

export TOPO=zk2
❯ ./101_initial_cluster.sh
...

❯ vtctldclient GetTopologyPath /global/keyspaces/commerce/Keyspace
{
  "name": "Keyspace",
  "path": "/global/keyspaces/commerce/Keyspace",
  "data": "durability_policy:\"semi_sync\" sidecar_db_name:\"_vt\"",
  "children": [],
  "version": "0"
}

❯ vtctldclient UpdateThrottlerConfig --enable --custom-query="select 1 from foo" --throttle-app="vplayer" commerce

❯ vtctldclient GetTopologyPath /global/keyspaces/commerce/Keyspace
{
  "name": "Keyspace",
  "path": "/global/keyspaces/commerce/Keyspace",
  "data": "durability_policy:\"semi_sync\" throttler_config:{enabled:true custom_query:\"select 1 from foo\" throttled_apps:{key:\"vplayer\" value:{name:\"vplayer\" ratio:1 expires_at:{seconds:1715971901 nanoseconds:349687000}}}} sidecar_db_name:\"_vt\"",
  "children": [],
  "version": "1"
}

❯ vtctldclient UpdateThrottlerConfig --enable --custom-query="select 1 from foobar" --throttle-app="vplayer" commerce

❯ vtctldclient GetTopologyPath /global/keyspaces/commerce/Keyspace
{
  "name": "Keyspace",
  "path": "/global/keyspaces/commerce/Keyspace",
  "data": "durability_policy:\"semi_sync\" throttler_config:{enabled:true custom_query:\"select 1 from foobar\" throttled_apps:{key:\"vplayer\" value:{name:\"vplayer\" ratio:1 expires_at:{seconds:1715971909 nanoseconds:518421000}}}} sidecar_db_name:\"_vt\"",
  "children": [],
  "version": "2"
}

And we don't in fact get an error when trying to get the version:

❯ vtctldclient GetTopologyPath /global/keyspaces/commerce/Keyspace --version 1
{
  "name": "Keyspace",
  "path": "/global/keyspaces/commerce/Keyspace",
  "data": "",
  "children": [],
  "version": "0"
}

So that's not right... I'll put this back in draft then and try to add zk2 and consul support. And at the very least, return an error as expected if it's not supported.

mattlord · 2024-05-18T04:57:04Z

Confirmed that ZooKeeper does not support getting a specific version of a node/key as it does not actually store multiple revisions/versions of a key: https://github.com/z-division/go-zookeeper/blob/v1.0.0/zk/conn.go#L1129-L1166

Rather the version in ZK is merely a counter that is incremented on each Set: https://github.com/z-division/go-zookeeper/blob/v1.0.0/zk/structs.go#L36

So for ZK2 we can only provide a clear and meaningful error from the client.

The same is true for Consul. It does not store multiple versions/revisions of keys/nodes so it has no way to request them on a get: https://github.com/hashicorp/consul/blob/v1.18.2/api/kv.go#L69-L89

It only has the key's ModifyIndex which is also a change counter that is used for CAS: https://github.com/hashicorp/consul/blob/v1.18.2/api/kv.go#L25-L28

Signed-off-by: Matt Lord <[email protected]>

rohit-nayak-ps

❤️

go/vt/vtctl/grpcvtctldserver/server.go

This also brings all of the vtctldclient docs up to date with: export COBRADOC_VERSION_PAIRS="get_topo_path_version:20.0" export VITESS_DIR=~/git/vitess make vtctldclient-docs Signed-off-by: Matt Lord <[email protected]>

Signed-off-by: Matt Lord <[email protected]>

mattlord · 2024-05-22T21:42:46Z

Thank you for the review, @rohit-nayak-ps ! I've addressed your comments here: 3ff727c

Thank you for the nudge! I wasn't happy with that function either and it gave me the push to refactor it. 🙂

Signed-off-by: Matt Lord <[email protected]>

deepthi · 2024-05-31T16:33:10Z

go/vt/vtctl/grpcvtctldserver/server.go

@@ -2259,24 +2260,26 @@ func (s *VtctldServer) GetTopologyPath(ctx context.Context, req *vtctldatapb.Get
 	span, ctx := trace.NewSpan(ctx, "VtctldServer.GetTopology")
 	defer span.Finish()

-	// handle toplevel display: global, then one line per cell.
-	if req.Path == "/" {


nit: proto fields are all public by design, and it is fine to access them directly. There is no need to change all accesses to use a getter instead.

I've gotten into the habit of using the proto getters as they are nil safe. In this case, for example, if req is nil it will return "" rather than have a nil pointer error.

This also brings all of the vtctldclient docs up to date with: export COBRADOC_VERSION_PAIRS="get_topo_path_version:20.0" export VITESS_DIR=~/git/vitess make vtctldclient-docs Signed-off-by: Matt Lord <[email protected]>

Add version support to GetTopologyPath

1d0deed

Signed-off-by: Matt Lord <[email protected]>

github-actions bot added this to the v20.0.0 milestone May 13, 2024

mattlord added Component: Topology Type: Enhancement Logical improvement (somewhere between a bug and feature) and removed NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels May 13, 2024

mattlord added 8 commits May 13, 2024 19:14

Add json option

7ddc0bf

Signed-off-by: Matt Lord <[email protected]>

Add data-as-json flag

a6ce9b6

Signed-off-by: Matt Lord <[email protected]>

Merge remote-tracking branch 'origin/main' into get_topo_path_version

b46f112

Signed-off-by: Matt Lord <[email protected]>

Post merge fixup

7f2fd79

Signed-off-by: Matt Lord <[email protected]>

Add support for KeyspaceRoutingRules in GetTopologyPath

864ae5b

Signed-off-by: Matt Lord <[email protected]>

Add snake case conversion

b49ccb5

Signed-off-by: Matt Lord <[email protected]>

Test fixes

48affa6

Signed-off-by: Matt Lord <[email protected]>

Add unit test

7c7cb0e

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the get_topo_path_version branch from f6b9100 to 7c7cb0e Compare May 17, 2024 15:51

mattlord changed the title ~~Add version support to GetTopologyPath~~ Topo: Add version support to GetTopologyPath for etcd2topo May 17, 2024

mattlord changed the title ~~Topo: Add version support to GetTopologyPath for etcd2topo~~ Topo: Add version support to GetTopologyPath May 17, 2024

mattlord removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request labels May 17, 2024

mattlord added 2 commits May 17, 2024 12:32

Changes after self review

98ba421

Signed-off-by: Matt Lord <[email protected]>

Handle protojson formatting on server side

edb127e

Signed-off-by: Matt Lord <[email protected]>

mattlord commented May 17, 2024

View reviewed changes

mattlord marked this pull request as ready for review May 17, 2024 17:04

mattlord requested review from deepthi and harshit-gangal as code owners May 17, 2024 17:04

mattlord requested review from ajm188 and notfelineit as code owners May 17, 2024 17:04

mattlord requested review from rohit-nayak-ps and removed request for harshit-gangal May 17, 2024 17:05

Fix zk and consul local examples

e5bef58

Signed-off-by: Matt Lord <[email protected]>

mattlord marked this pull request as draft May 17, 2024 17:59

Fix error handling and use int64

81ef344

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the get_topo_path_version branch from 17481ed to 81ef344 Compare May 18, 2024 05:40

Minor change from self review

b9b567a

Signed-off-by: Matt Lord <[email protected]>

mattlord marked this pull request as ready for review May 18, 2024 06:05

rohit-nayak-ps approved these changes May 22, 2024

View reviewed changes

go/vt/vtctl/grpcvtctldserver/server.go Outdated Show resolved Hide resolved

go/vt/vtctl/grpcvtctldserver/server.go Outdated Show resolved Hide resolved

mattlord removed the NeedsWebsiteDocsUpdate What it says label May 22, 2024

Address review comments

3ff727c

Signed-off-by: Matt Lord <[email protected]>

mattlord added 2 commits May 22, 2024 18:13

Adjust memorytopo behavior on "dirs" to match other topo impls

9a2df94

Signed-off-by: Matt Lord <[email protected]>

Correct comments

359a25c

Signed-off-by: Matt Lord <[email protected]>

deepthi mentioned this pull request May 31, 2024

Add docs for GetTopologyPath vitessio/website#1757

Merged

deepthi approved these changes May 31, 2024

View reviewed changes

deepthi merged commit c5e6e9b into vitessio:main May 31, 2024
96 checks passed

deepthi deleted the get_topo_path_version branch May 31, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topo: Add version support to GetTopologyPath #15933

Topo: Add version support to GetTopologyPath #15933

mattlord commented May 13, 2024 •

edited

Loading

vitess-bot bot commented May 13, 2024

codecov bot commented May 17, 2024 •

edited

Loading

mattlord May 17, 2024

deepthi commented May 17, 2024

mattlord commented May 17, 2024 •

edited

Loading

deepthi commented May 17, 2024

mattlord commented May 17, 2024

mattlord commented May 18, 2024 •

edited

Loading

rohit-nayak-ps left a comment

mattlord commented May 22, 2024

deepthi May 31, 2024

mattlord May 31, 2024

Topo: Add version support to GetTopologyPath #15933

Topo: Add version support to GetTopologyPath #15933

Conversation

mattlord commented May 13, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented May 13, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

mattlord May 17, 2024

Choose a reason for hiding this comment

deepthi commented May 17, 2024

mattlord commented May 17, 2024 • edited Loading

deepthi commented May 17, 2024

mattlord commented May 17, 2024

mattlord commented May 18, 2024 • edited Loading

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

mattlord commented May 22, 2024

deepthi May 31, 2024

Choose a reason for hiding this comment

mattlord May 31, 2024

Choose a reason for hiding this comment

mattlord commented May 13, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

mattlord commented May 17, 2024 •

edited

Loading

mattlord commented May 18, 2024 •

edited

Loading