api/etcdserverpb/rpc.proto: add leader_id response header, populate defragment API response header - #22327
api/etcdserverpb/rpc.proto: add leader_id response header, populate defragment API response header#22327gyuho wants to merge 6 commits into
Conversation
|
Hi @gyuho. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
The commit requires author to signoff by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 443 files with indirect coverage changes @@ Coverage Diff @@
## main #22327 +/- ##
==========================================
+ Coverage 69.69% 73.03% +3.34%
==========================================
Files 448 448
Lines 38074 31585 -6489
==========================================
- Hits 26534 23067 -3467
+ Misses 10117 8515 -1602
+ Partials 1423 3 -1420 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
43ef676 to
575d780
Compare
|
I think the change is useful by itself. One example in etcd-operator, we need to know the leaderId so that we can move leader before we recreate a POD. For completeness, let's also populate the header for etcd/server/etcdserver/api/v3rpc/maintenance.go Lines 119 to 130 in fd8581e |
Defragment was the only maintenance RPC that returned an empty response header, so a client defragmenting through the v3 API got no member_id, raft_term, or leader_id back. Fill the header after the defrag returns: a defrag can run long enough for leadership to move, and callers (e.g. etcd-operator moving leadership before recreating a pod) want the current leader, not the one from when the call started. ref. etcd-io#22327 Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
575d780 to
28140e3
Compare
Add leader_id (field 5) to the ResponseHeader proto and populate it at every server-side fill site from RaftStatusGetter.Leader(). That interface is already wired at each site. The field is advisory: 0 means the responding member does not know who the leader is. It is not a fencing token and is not atomic with raft_term. Old clients ignore the field; new clients treat 0 as "no hint." Covers all v3 RPC responses through fillWithoutRevision / fill, newResponseHeader (watch), ClusterServer.header, EtcdServer.newHeader, and applierV3backend.newHeader. ref. etcd-io#22268 Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
Defragment was the only maintenance RPC that returned an empty response header, so a client defragmenting through the v3 API got no member_id, raft_term, or leader_id back. Fill the header after the defrag returns: a defrag can run long enough for leadership to move, and callers (e.g. etcd-operator moving leadership before recreating a pod) want the current leader, not the one from when the call started. Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
28140e3 to
a930242
Compare
|
/retest |
TestCurlV3MaintenanceDefragment asserted the POST /v3/maintenance/defragment
response body is exactly "{}". Now that Defragment populates the response
header (cluster_id, member_id, revision, raft_term, leader_id), the gateway
returns a non-empty body and the expectation times out.
Assert on the leader_id field instead, which is the field this change
introduces.
Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
|
@ahrtr PTAL. |
|
LGTM, please also add a changelog item for 3.8 https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.8.md#etcd-server |
Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
|
/retest-required |
|
/retest-required |
|
Please signoff the last commit, either your sigature or mine (below), |
Co-authored-by: Benjamin Wang <benjamin.ahrtr@gmail.com> Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
7e77eb3 to
04590e1
Compare
|
@ahrtr PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, gyuho The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| - [Remove flag `--max-snapshots` and `--v2-deprecation`](https://github.com/etcd-io/etcd/pull/22306) | ||
| - [Cleanup the legacy v2 snapshot files on bootstrap](https://github.com/etcd-io/etcd/pull/22336) | ||
| - [Cleanup the legacy v2 snapshot source code and cleanup orphaned defragmentation files on bootstrap](https://github.com/etcd-io/etcd/pull/22341) | ||
| - [Add `LeaderId` (`leader_id`) to `ResponseHeader` and populate it in every response, including `DefragmentResponse`](https://github.com/etcd-io/etcd/pull/22327) |
There was a problem hiding this comment.
Do we need to support Snapshot/MoveLeader as well?
| // leader_id is the member ID of the raft leader that the responding | ||
| // member believes is the current leader, or 0 if the responding member | ||
| // does not know who the leader is. | ||
| uint64 leader_id = 5 [(versionpb.etcd_version_field)="3.8"]; |
There was a problem hiding this comment.
We need to update this file as well -
fuweid
left a comment
There was a problem hiding this comment.
Left two comments and overall it looks good to me.
Signed-off-by: Gyuho Lee <gyuhol@nvidia.com>
|
@gyuho: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
(/cc @SaranBalaji90 @jamesmthompson)
Adds
leader_id(field 5) to the v3ResponseHeaderand fills it at every server-side header site from the already-wiredRaftStatusGetter.Leader(). The field is optional:0means "no hint," and it is not a fencing token. Old clients ignore it; new clients treat0from old servers as "unknown leader." Cost is at most ~11 bytes per response for a value the server already holds in memory.Today a client must poll
Statusto learn the leader; this field puts that leader member ID in every Put/Get/Watch response, which is what Phase 2's opt-in leader-aware balancer builds on.A new common-suite test (
TestResponseHeaderLeaderId, integration and e2e) verifiesleader_idis nonzero once a leader is elected, matches each member'sstatus.leader, agrees across members, and appears on Put and Get, with mixed-version clusters checked per current-version endpoint.c.f., #22268