Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

endocrimes
Copy link
Contributor

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
Copy link
Member

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

@endocrimes
Copy link
Contributor Author

@serathius not as implemented in v3 afaict -

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
Copy link
Member

ahrtr commented Apr 1, 2022

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

@endocrimes
Copy link
Contributor Author

endocrimes commented Apr 2, 2022

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
Copy link
Contributor

ptabor commented Apr 3, 2022

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):
    bool serializable = 7;

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

@endocrimes
Copy link
Contributor Author

@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
Copy link
Member

ahrtr commented Apr 3, 2022

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
Copy link
Member

@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.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use revision in eventually check

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
```
@codecov-commenter
Copy link

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
Copy link
Contributor

ptabor commented Apr 4, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants