Topo: Add version support to GetTopologyPath

[mattlord]

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

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

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.

[mattlord]

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]

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]

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]

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

[mattlord]

@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]

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

[rohit-nayak-ps]

❤️

[mattlord]

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. 🙂

[deepthi]

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.

[mattlord]

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.