tests/common/lease: Wait for correct lease list response

[endocrimes]

We don't consistently reach the same etcd server during the lifetime of
a test and in some cases, this means that this test will flake if an
etcd server was slow to update its state and the test hits the outdated
server.

Here we switch to using an Eventually case which will wait upto a
second for the expected result before failing - with a 10ms gap between
invocations.

[tests(dani/leasefix)] $ gotestsum -- ./common -tags integration -count 100 -timeout 15m -run TestLeaseGrantAndList
✓  common (2m26.71s)

DONE 1600 tests in 147.258s

[serathius]

Shouldn't LeaseList be a Linearizable read (preserve read commited)?

[endocrimes]

@serathius not as implemented in v3 afaict -

etcd/server/etcdserver/v3_server.go

Lines 364 to 371 in 0d5c1dc

	func (s *EtcdServer) LeaseLeases(ctx context.Context, r *pb.LeaseLeasesRequest) (*pb.LeaseLeasesResponse, error) {
	ls := s.lessor.Leases()
	lss := make([]*pb.LeaseStatus, len(ls))
	for i := range ls {
	lss[i] = &pb.LeaseStatus{ID: int64(ls[i].ID)}
	}
	return &pb.LeaseLeasesResponse{Header: newHeader(s), Leases: lss}, nil
	}

[ahrtr]

I think we should make the LeaseList as a Linearizable request, so this test case successfully detected the unexpected behavior. cc @ptabor

[endocrimes]

LeaseLeases is the only Lease API we have that doesn't get forwarded to the leader in some way, so that would make sense.

I assume it wasn't in the first place because it's a mostly informational api that's easy to overlook, but I can't find more discussion around it in the initial PR than this (#8358 (comment)).

---

As for how to make this Linearizable, it looks like that should be fairly easy, just a little bit weird because the LeaseHTTP package has a couple of different ways of doing things. I can open a PR if that's what we'd rather do instead (but that might have to wait for 3.6)

[ptabor]

Agree that intuitively LeaseLeasesRequest should be 'linearizable'

I think it's pretty serious change to make it as part of stable v3.* line and violating formal backward compatibility guarantees to change the default.

I would:

1. Add bool linearizable = 1 [(versionpb.etcd_version_field)="3.6"]; to the LeaseLeases method (default false)
2. Mark it as 'deprecated'
3. Add a new method ListLeasesmethod with serializable field (default false):
   

   etcd/api/etcdserverpb/rpc.proto

   Line 453 in 3d3c437

   bool serializable = 7;

This way we would cleanup also the naming mistake (I assume) and have safe default going forward.

[endocrimes]

@ptabor Sounds good - I can open a PR for that soon. Do we wanna merge this to stabilize CI in the meantime (given that I assume a feature PR will take longer to land, and this is currently the flakiest etcd test)

[ahrtr]

I think it makes more sense to skip the test case for now instead of delivering a fix for an unexpected behavior. Once the formal fix/enhancement is ready, we can enable it again.

[serathius]

@ptabor Does adding linearizable field to LeaseLeases benefit us in any way when we will still introduce ListLeases? In both cases users will still take an action to get linealizable read (either set field or change method used). I think that adding the field will slow down support of new method. I think that we can skip adding linearizable field.

@endocrimes @ahrtr For now we are only deprecating LeaseLeases, we need to maintain the tests until we remove it. As so we still need to make this test stable.

[serathius]

Please use revision in eventually check

[codecov-commenter]

Codecov Report

Merging #13868 (f71196d) into main (6c974a3) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13868      +/-   ##
==========================================
+ Coverage   72.51%   72.56%   +0.05%     
==========================================
  Files         468      468              
  Lines       38255    38255              
==========================================
+ Hits        27739    27760      +21     
+ Misses       8734     8718      -16     
+ Partials     1782     1777       -5     

Flag	Coverage Δ
all	72.56% <ø> (+0.05%)	⬆️

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

Impacted Files	Coverage Δ
server/proxy/grpcproxy/register.go	69.76% <0.00%> (-9.31%)	⬇️
server/etcdserver/api/v3rpc/interceptor.go	77.60% <0.00%> (-5.21%)	⬇️
client/v3/concurrency/session.go	88.63% <0.00%> (-4.55%)	⬇️
server/storage/mvcc/watchable_store.go	85.14% <0.00%> (-3.99%)	⬇️
api/etcdserverpb/raft_internal_stringer.go	81.72% <0.00%> (-2.16%)	⬇️
server/etcdserver/util.go	86.17% <0.00%> (-2.13%)	⬇️
server/etcdserver/api/v3rpc/watch.go	87.24% <0.00%> (-1.35%)	⬇️
server/proxy/grpcproxy/lease.go	89.14% <0.00%> (ø)
client/v3/leasing/kv.go	90.69% <0.00%> (+0.33%)	⬆️
pkg/proxy/server.go	61.01% <0.00%> (+0.33%)	⬆️
... and 13 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 6c974a3...f71196d. Read the comment docs.

[ptabor]

@ptabor Does adding linearizable field to LeaseLeases benefit us in any way when we will still introduce ListLeases? In both cases users will still take an action to get linealizable read (either set field or change method used). I think that adding the field will slow down support of new method. I think that we can skip adding linearizable field.

Seems good.

@endocrimes @ahrtr For now we are only deprecating LeaseLeases, we need to maintain the tests until we remove it. As so we still need to make this test stable.