Add range --count-only and --keys-only mutual exclusivity check#21802
Add range --count-only and --keys-only mutual exclusivity check#21802nwnt wants to merge 1 commit into
Conversation
Signed-off-by: Nont <9658731+nwnt@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nwnt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 31 files with indirect coverage changes @@ Coverage Diff @@
## main #21802 +/- ##
==========================================
+ Coverage 70.08% 70.20% +0.12%
==========================================
Files 426 426
Lines 35245 35247 +2
==========================================
+ Hits 24701 24745 +44
+ Misses 9157 9113 -44
- Partials 1387 1389 +2 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@nwnt: The following test 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. |
| ErrGRPCCanceled = status.Error(codes.Canceled, "etcdserver: request canceled") | ||
| ErrGRPCDeadlineExceeded = status.Error(codes.DeadlineExceeded, "etcdserver: context deadline exceeded") | ||
|
|
||
| ErrGRPCRangeKeysAndCountOptions = status.Error(codes.InvalidArgument, "etcdserver: both KeysOnly and CountOnly cannot be true at the same time") |
There was a problem hiding this comment.
What happens currently? Do we have a test for that?
There was a problem hiding this comment.
From what I can see:
- on the client side, the validation is on
etcdctlget command, but the same check is not performed by the client package for the Get operation. - on the server side, the current behavior is if --count-only takes the priority where
storeTxnCommon.rangeKeysreturns early.
There are no tests that validate the latter behavior. The --keys-only tests I added previously avoided this case because I ran into the validation error returned by etcdctl (used by the e2e tests), but at that time I incorrectly assumed the client (used by the integration tests) also did it.
I'm thinking maybe what we should do instead is to adjust the behavior of etcdctl to stop validating this case and document it that --count-only takes precedence and we can adjust the test cases accordingly.
There was a problem hiding this comment.
Even if client validates and server doesn't, we still need to avoid breaking server behavior. We need a server only test using clientv3 instead of etcdctl. That's why we use common package to write a test that can be executed on both paths client and command line. Please take a look how to best do that.
Adding the mutual exclusivity check for Range on the server side.
Addressing the comment #21791 (comment).