Add a new field hash_revision into HashKVResponse

[ahrtr]

Added one new field "HashRevision" into HashKVResponse. The field is populated when responding to leader or client's HashKV requests. It's the revision up to which the returned hash is calculated.

Two reasons why adding this field:

1. Users/clients may get inconsistent hashes (which might be expected) and accordingly take wrong action. Please see etcdserver: update corruption check #14510 (comment)
2. The KeyValueHash has three fields (Hash, CompactRevision and Revision), and they are an integral whole. It would be better to return all the three fields to client or other member (e.g. leader).

cc @serathius @spzala @ptabor

[codecov-commenter]

Codecov Report

Merging #14537 (f77b8a7) into main (4c82446) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #14537      +/-   ##
==========================================
- Coverage   75.61%   75.51%   -0.11%     
==========================================
  Files         457      457              
  Lines       37320    37325       +5     
==========================================
- Hits        28220    28186      -34     
- Misses       7331     7362      +31     
- Partials     1769     1777       +8     

Flag	Coverage Δ
all	75.51% <100.00%> (-0.11%)	⬇️

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

Impacted Files	Coverage Δ
server/etcdserver/api/v3rpc/maintenance.go	73.50% <100.00%> (+0.72%)	⬆️
server/etcdserver/corrupt.go	89.57% <100.00%> (+0.03%)	⬆️
client/pkg/v3/testutil/leak.go	60.17% <0.00%> (-7.08%)	⬇️
client/v3/namespace/watch.go	87.87% <0.00%> (-6.07%)	⬇️
client/v3/concurrency/session.go	88.63% <0.00%> (-4.55%)	⬇️
server/etcdserver/api/v3rpc/watch.go	81.90% <0.00%> (-4.45%)	⬇️
client/pkg/v3/testutil/recorder.go	76.27% <0.00%> (-3.39%)	⬇️
server/etcdserver/api/v3rpc/util.go	70.96% <0.00%> (-3.23%)	⬇️
client/v3/experimental/recipes/key.go	75.34% <0.00%> (-2.74%)	⬇️
client/v3/experimental/recipes/double_barrier.go	68.83% <0.00%> (-2.60%)	⬇️
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[serathius]

As mentioned in #14510 (comment) adding the field itself will not improve the user expirience much. It would just change:

+------------------------+------------+
|        ENDPOINT        |    HASH    |
+------------------------+------------+
|  http://127.0.0.1:2379 | 12345      |
| http://127.0.0.1:22379 | 54321      |
| http://127.0.0.1:32379 | 34521      |
+------------------------+------------+

to

+------------------------+------------+----------+
|        ENDPOINT        |    HASH    | REVISION |
+------------------------+------------+----------+
|  http://127.0.0.1:2379 | 12345      | 132      |
| http://127.0.0.1:22379 | 54321      | 133      |
| http://127.0.0.1:32379 | 34521      | 134      |
+------------------------+------------+----------+

Still each member returns different hash.

If we really want to improve user experience. Let's rethink how etcdctl endpoint hashkv -w table --cluster should work to make it useful.

[ahrtr]

The etcdctl side change is the next step I will do.

[ahrtr]

Still each member returns different hash.

This isn't a problem. The point is just to let users know the rev for each hash. Of course, we need to enhance etcdctl in a following PR.

[ptabor]

The etcdctl side change is the next step I will do.

What's your plan for this ? Pre-fetching revisions and double-lookup on min ?

[ahrtr]

The etcdctl side change is the next step I will do.

What's your plan for this ? Pre-fetching revisions and double-lookup on min ?

The existing logic will not be changed. I will just add the HashRevision into the output, just similar to #14537 (comment) pointed out.

If users clearly set a revision when getting the hashKV, such as etcdctl endpoint hashkv -w table --cluster --rev 10000, then the responses will just echo the rev.

If users do not set rev when getting the hashKV, then it defaults to 0 in the request. etcdserver will just current latest revision to calculate the hash. Since etcdctl just gets members' hashes one by one, so the responses for different member might have different revisions.

[ahrtr]

After second thought, I would like to partially adopt @ptabor 's comment above #14537 (comment) .

The solution to enhance etcdctl (in a separate PR) would be:

1. If users explicitly set a revision such as etcdctl endpoint hashkv -w table --cluster --rev 10000, then etcdctl just gets the hash of each member using the provided rev.
2. Otherwise if rev isn't specified, then pre-fetching revisions and double-lookup on min(rev).

Please let me know if you have any concern or comment.

[spzala]

@ahrtr thanks, lgtm but I am wondering if you think good idea to add a test coverage for the changes in the corrupt_test.go? Doing it separately is okay too.

[ahrtr]

@ahrtr thanks, lgtm but I am wondering if you think good idea to add a test coverage for the changes in the corrupt_test.go? Doing it separately is okay too.

Thanks @spzala . Yes, it makes sense to me. Will do it separately.

[ahrtr]

Just rebased this PR in case there are any potential conflicts.

[spzala]

@ahrtr thanks, lgtm but I am wondering if you think good idea to add a test coverage for the changes in the corrupt_test.go? Doing it separately is okay too.

Thanks @spzala . Yes, it makes sense to me. Will do it separately.

Thanks @ahrtr