Fix member response header missing revision

[chaochn47]

Fix issue raised in #14020 (comment)

Before

$ etcdctl member list -w json | jq .
{
  "header": {
    "cluster_id": 14841639068965180000,
    "member_id": 10276657743932975000,
    "raft_term": 13
  },
  "members": [
    {
      "ID": 10276657743932975000,
      "name": "default",
      "peerURLs": [
        "http://localhost:2380"
      ],
      "clientURLs": [
        "http://localhost:2379"
      ]
    }
  ]
}

After

$ etcdctl member list -w json | jq .
{
  "header": {
    "cluster_id": 14841639068965180000,
    "member_id": 10276657743932975000,
    "revision": 1,
    "raft_term": 14
  },
  "members": [
    {
      "ID": 10276657743932975000,
      "name": "default",
      "peerURLs": [
        "http://localhost:2380"
      ],
      "clientURLs": [
        "http://localhost:2379"
      ]
    }
  ]
}

[codecov-commenter]

Codecov Report

Merging #14026 (0051cbf) into main (b7be91f) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #14026      +/-   ##
==========================================
- Coverage   74.96%   74.73%   -0.24%     
==========================================
  Files         450      450              
  Lines       37173    37173              
==========================================
- Hits        27868    27781      -87     
- Misses       7532     7602      +70     
- Partials     1773     1790      +17     

Flag	Coverage Δ
all	74.73% <100.00%> (-0.24%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/etcdserver/api/v3rpc/member.go	93.54% <100.00%> (-3.23%)	⬇️
server/proxy/grpcproxy/register.go	69.76% <0.00%> (-20.94%)	⬇️
server/auth/simple_token.go	80.00% <0.00%> (-8.47%)	⬇️
client/pkg/v3/fileutil/lock_linux.go	72.22% <0.00%> (-8.34%)	⬇️
client/pkg/v3/tlsutil/tlsutil.go	83.33% <0.00%> (-8.34%)	⬇️
client/v3/experimental/recipes/queue.go	58.62% <0.00%> (-6.90%)	⬇️
server/etcdserver/api/v3rpc/interceptor.go	72.39% <0.00%> (-5.21%)	⬇️
server/storage/wal/file_pipeline.go	90.69% <0.00%> (-4.66%)	⬇️
server/proxy/grpcproxy/watch.go	92.48% <0.00%> (-4.05%)	⬇️
client/pkg/v3/testutil/recorder.go	76.27% <0.00%> (-3.39%)	⬇️
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7be91f...0051cbf. Read the comment docs.

[ahrtr]

The change looks good to me.

Two additional verification:

1. On client side (etcdctl), there are different printers, please make sure all of them handle this new field correctly. It should be good, just to double confirm.
2. Please verify whether this change has any impact on the K8s.

[serathius]

This goes into recent discussion about meaning of revision in lease response requests that was raised during community meeting #13989. Please remember that revision only relates to key value store, even though revision field is present in all etcd rpc responses. Current situation is not satisfactory as usage of revision is inconsistent and not properly documented.

Before we merge this PR we should make a decision of meaning of revision in all rpc responses and document it. We should look into how revision is used currently in APIs, propose changes and analyse impact if we would make changes.

[ahrtr]

Please remember that revision only relates to key value store,

This might not be correct. I think revision is related to any data which needs raft consensus. Membership info definitely needs raft consensus.

Lease is a special case, which we can discuss separately.

[serathius]

I think you mistook revision for raft index. It's raft index that relates to all data which needs raft concensus. Revision is global version number used for key value store. Etcd implements MVCC by using btree with revision as key.

You see this if you look how bbolt is used, we have multiple buckets used to stored different values:

• key values are stored under key bucket with keys being revisions
• leases are stored under `lease bucket with keys being revision id
• membership info is stored under two buckets members and members_removed both use member id as key

So if you look at this at low level, only KV store uses revision to version changes, however leases and membership is not versioned and totally unaware of revision.

[ahrtr]

Yes, you are right. The key for the lease is lease ID.

This could be the reason why member related commands do not have revision in the header.

But all responses share the same header struct, so we might need to hide the revision field if it's 0. For example printer_fields.go#L39

[ptabor]

+1 to hiding in etcdctl and not exposing revision for these methods.

[chaochn47]

Okay. The missing revision behavior sounds like intended not to reflect unrelated mvcc global revision.