Add support for lease api to linearizability tests #15080

geetasg · 2023-01-11T05:23:23Z

Signed-off-by: Geeta Gharpure geetagh@amazon.com

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Signed-off-by: Geeta Gharpure <geetagh@amazon.com>

geetasg · 2023-01-11T05:25:28Z

Updated with review feedback from #15057. /cc @ahrtr @serathius

codecov-commenter · 2023-01-11T06:03:40Z

Codecov Report

Merging #15080 (5b84526) into main (a992fb5) will decrease coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #15080      +/-   ##
==========================================
- Coverage   74.78%   74.60%   -0.18%     
==========================================
  Files         415      415              
  Lines       34288    34288              
==========================================
- Hits        25642    25582      -60     
- Misses       7018     7062      +44     
- Partials     1628     1644      +16

Flag	Coverage Δ
all	`74.60% <ø> (-0.18%)`	⬇️

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

Impacted Files	Coverage Δ
client/pkg/v3/fileutil/purge.go	`68.85% <0.00%> (-6.56%)`	⬇️
server/storage/mvcc/kvstore_compaction.go	`95.65% <0.00%> (-4.35%)`	⬇️
client/v3/concurrency/session.go	`85.10% <0.00%> (-4.26%)`	⬇️
server/proxy/grpcproxy/watch.go	`92.48% <0.00%> (-4.05%)`	⬇️
client/v3/leasing/cache.go	`87.77% <0.00%> (-3.89%)`	⬇️
server/etcdserver/api/rafthttp/msgappv2_codec.go	`67.82% <0.00%> (-3.48%)`	⬇️
server/etcdserver/api/rafthttp/pipeline.go	`97.40% <0.00%> (-2.60%)`	⬇️
client/v3/concurrency/election.go	`79.68% <0.00%> (-2.35%)`	⬇️
server/etcdserver/api/v3rpc/auth.go	`79.34% <0.00%> (-2.18%)`	⬇️
server/etcdserver/api/rafthttp/peer.go	`85.06% <0.00%> (-1.95%)`	⬇️
... and 16 more

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

ahrtr · 2023-01-11T07:37:41Z

tests/linearizability/traffic.go

+		leaseId := lm.LeaseId(cid)
+		if leaseId != 0 {
+			err = c.LeaseRevoke(putCtx, leaseId)
+			//if LeaseRevoke has failed, do not remove the mapping.
+			if err == nil {
+				lm.RemoveLeaseId(cid)
+			}
+		}


Should you execute c.LeaseGrant as well if there is no a leaseId related to the clientId in this case (LeaseRevoke), just in the same way as you process PutWithLease? Otherwise the (readWriteSingleKey) Write will do nothing in this case.

The Delete operation might delete nothing if it happens before the related PUT operation. But there is always a communication with the etcdserver.

I agree that the existing implementation is simpler.

Anyway, I am OK to discuss this in a separate PR.

ahrtr

Overall looks good to me.

Thank you @geetasg

serathius · 2023-01-11T08:29:08Z

tests/linearizability/model.go

@@ -197,6 +224,24 @@ func initState(request EtcdRequest, response EtcdResponse) EtcdState {
 		case Put:
 			state.KeyValues[op.Key] = op.Value
 		case Delete:
+		case PutWithLease:
+			if _, ok := state.Leases[op.LeaseID]; ok {


This should never happen as state.Leases should be empty.

updated in #15093

ptabor · 2023-01-11T08:29:50Z

tests/linearizability/model.go

+					panic("old lease id found at init")
+				}
+				//attach to new lease id
+				state.KeyLeases[op.Key] = op.LeaseID


How about calling attachToNewLease here ?

removed the code in init - now putwithlease at init is nop since the lease wont be there. updated in #15093

serathius · 2023-01-11T08:29:53Z

tests/linearizability/model.go

+			if _, ok := state.Leases[op.LeaseID]; ok {
+				state.KeyValues[op.Key] = op.Value
+				//detach from old lease id but we dont expect that at init
+				if _, ok := state.KeyLeases[op.Key]; ok {


Just skip it

updated in #15093

serathius · 2023-01-11T08:30:48Z

tests/linearizability/model.go

 	return s, EtcdResponse{Result: opResp, Revision: s.Revision}
 }
+
+func detachFromOldLease(s EtcdState, op EtcdOperation) EtcdState {


Suggested change

func detachFromOldLease(s EtcdState, op EtcdOperation) EtcdState {

func detachFromOldLease(s EtcdState, key string) EtcdState {

updated in #15093

ptabor

Looks good to me, but I think @serathius opinion is crucial here.

serathius · 2023-01-11T08:32:31Z

tests/linearizability/model.go

 				opResp[i].Deleted = 1
 			}
+		case PutWithLease:
+			if _, ok := s.Leases[op.LeaseID]; ok {


Question, does etcd allows to make a Put with lease that doesn't exist? I expect that it returns an client error, but maybe I remember wrong.

Adding test result for quick ref -
Put with non existent lease -

{"level":"warn","ts":"2023-01-12T15:39:35.877-0800","caller":"txn/util.go:47","msg":"failed to apply request","took":"18.335µs","request":"header:<ID:7587867905305039876 > put:<key:"x" value_size:1 lease:4 >","response":"","error":"lease not found"}

client (etcdctl in my case) -
macbook % ./etcdctl put x y --lease=4
{"level":"warn","ts":"2023-01-12T15:39:35.878-0800","logger":"etcd-client","caller":"v3@v3.6.0-alpha.0/retry_interceptor.go:65","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0xc00017e000/127.0.0.1:2379","method":"/etcdserverpb.KV/Put","attempt":0,"error":"rpc error: code = NotFound desc = etcdserver: requested lease not found"}
Error: etcdserver: requested lease not found

serathius · 2023-01-11T08:34:28Z

tests/linearizability/model.go

+	return s
+}
+
+func attachToNewLease(s EtcdState, op EtcdOperation) EtcdState {


Suggested change

func attachToNewLease(s EtcdState, op EtcdOperation) EtcdState {

func attachKeyToLease(s EtcdState, leaseID int64, key string) EtcdState {

updated in #15093

serathius · 2023-01-11T08:40:35Z

tests/linearizability/model_test.go

+			},
+		},
+		{
+			name: "Deleting a leased key - revoke should not increment revision",


One additional scenario would be useful, "Overriding a leased key should remove lease"

there is a scenario for Put following PutWithLease. https://github.com/etcd-io/etcd/blob/main/tests/linearizability/model_test.go#L435. Do you mean something else? Can you please clarify what you mean by override? thanks

serathius · 2023-01-11T08:46:30Z

tests/linearizability/lease_ids.go

+	"sync"
+)
+
+type clientId2LeaseIdMapper interface {


What's the reason to map leases to clients? Would it be more interesting if we allowed every client to touch every lease? Changes needed:

We just store available lease ids. Possibly with some limit to avoid explosion on to many LeaseGrant.

When client needs to use lease it will take it out from storage to avoid racing with other clients. When client is done with a lease it can return it to storage, assuming it didn't revoke it.

serathius · 2023-01-11T08:48:08Z

Looks very good, my comments are mostly nits to make code simpler. I'm impressed.

serathius · 2023-01-11T14:34:31Z

Let's merge it to avoid conflicts with #15078
@geetasg can you address the comments in future PR?

geetasg · 2023-01-11T17:21:58Z

Thanks much for the review @serathius @ahrtr and @ptabor . I will create a new PR to address the review comments in this one.

Add support for lease api to linearizability tests

5b84526

Signed-off-by: Geeta Gharpure <geetagh@amazon.com>

geetasg mentioned this pull request Jan 11, 2023

Add support for lease api to linearizability tests #15057

Closed

ahrtr reviewed Jan 11, 2023

View reviewed changes

ahrtr approved these changes Jan 11, 2023

View reviewed changes

serathius reviewed Jan 11, 2023

View reviewed changes

ptabor reviewed Jan 11, 2023

View reviewed changes

serathius reviewed Jan 11, 2023

View reviewed changes

ptabor approved these changes Jan 11, 2023

View reviewed changes

serathius reviewed Jan 11, 2023

View reviewed changes

serathius approved these changes Jan 11, 2023

View reviewed changes

serathius merged commit 3306639 into etcd-io:main Jan 11, 2023

This comment was marked as off-topic.

Sign in to view

This was referenced Jan 12, 2023

Lease support for linearizability tests : address review feedback #15093

Merged

Introduce etcd linearizability tests #14045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for lease api to linearizability tests #15080

Add support for lease api to linearizability tests #15080

geetasg commented Jan 11, 2023

geetasg commented Jan 11, 2023

codecov-commenter commented Jan 11, 2023

ahrtr Jan 11, 2023

ahrtr left a comment

serathius Jan 11, 2023

geetasg Jan 12, 2023

ptabor Jan 11, 2023

geetasg Jan 12, 2023

serathius Jan 11, 2023

geetasg Jan 12, 2023

serathius Jan 11, 2023

geetasg Jan 12, 2023

ptabor left a comment

serathius Jan 11, 2023

geetasg Jan 12, 2023

serathius Jan 11, 2023

geetasg Jan 12, 2023

serathius Jan 11, 2023

geetasg Jan 12, 2023

serathius Jan 11, 2023

serathius commented Jan 11, 2023

serathius commented Jan 11, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

geetasg commented Jan 11, 2023

	func detachFromOldLease(s EtcdState, op EtcdOperation) EtcdState {
	func detachFromOldLease(s EtcdState, key string) EtcdState {

	func attachToNewLease(s EtcdState, op EtcdOperation) EtcdState {
	func attachKeyToLease(s EtcdState, leaseID int64, key string) EtcdState {

Add support for lease api to linearizability tests #15080

Add support for lease api to linearizability tests #15080

Conversation

geetasg commented Jan 11, 2023

geetasg commented Jan 11, 2023

codecov-commenter commented Jan 11, 2023

Codecov Report

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptabor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented Jan 11, 2023

serathius commented Jan 11, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

geetasg commented Jan 11, 2023