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.

Sign up for GitHub

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

Jump to bottom

etcd-tester: check expired lease with -1 TTL #7415

Merged
merged 1 commit into from
Mar 3, 2017
Merged

etcd-tester: check expired lease with -1 TTL #7415

merged 1 commit into from
Mar 3, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Mar 3, 2017 •
edited
Loading

Following the change at
2ca1823

This fixes the error that we are getting

[round#6 case#0] functional-tester returning with error (tt.checkConsistency error (lease 5264526043634585873 expiration mismatch (lease expired=false, keys expired=true), lease 8791688976795817481 expiration mismatch (lease expired=false, keys expired=true), lease 1772547482562197041 expiration mismatch (lease expired=false, keys expired=true), lease 7369958869430294037 expiration mismatch (lease expired=false, keys expired=true), lease 8847421022184526416 expiration mismatch (lease expired=false, keys expired=true)))

heyitsanthony
heyitsanthony suggested changes Mar 3, 2017
View reviewed changes
tools/functional-tester/etcd-tester/checks.go Outdated
@@ -133,7 +132,7 @@ func (lc *leaseChecker) checkShortLivedLease(ctx context.Context, leaseID int64)
var resp *pb.LeaseTimeToLiveResponse
for i := 0; i < retries; i++ {
resp, err = lc.getLeaseByID(ctx, leaseID)
if rpctypes.Error(err) == rpctypes.ErrLeaseNotFound {
if err == nil && resp.TTL == -1 { // lease not found
Copy link
Contributor

Choose a reason for hiding this comment

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

if (err == nil && resp.TTL == -1) || (err != nil && rpctypes.Error(err) == rpctypes.ErrLeaseNotFound)?

tools/functional-tester/etcd-tester/checks.go Outdated
}
if rpctypes.Error(err) == rpctypes.ErrLeaseNotFound {
return true, nil
return resp.TTL == -1, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

if err != nil {
    if rpctypes.Error(err) == rpctypes.ErrLeaseNotFound {
        return true, nil
    }
} else {
    return resp.TTL == -1, nil
}

@gyuho gyuho force-pushed the etcd-tester-lease-check-with-ttl branch from 15e4a4a to 15b9052 Compare March 3, 2017 19:32
fanminshi
fanminshi reviewed Mar 3, 2017
View reviewed changes
tools/functional-tester/etcd-tester/checks.go Outdated
@@ -133,7 +133,8 @@ func (lc *leaseChecker) checkShortLivedLease(ctx context.Context, leaseID int64)
var resp *pb.LeaseTimeToLiveResponse
for i := 0; i < retries; i++ {
resp, err = lc.getLeaseByID(ctx, leaseID)
if rpctypes.Error(err) == rpctypes.ErrLeaseNotFound {
// lease not found when it's follower or leader
if (err == nil && resp.TTL == -1) || (err != nil && rpctypes.Error(err) == rpctypes.ErrLeaseNotFound) {
Copy link
Member

@fanminshi fanminshi Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think TimeToLive() ever returns ErrLeaseNotFound err via #7305

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does when it's leader. Now fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

was mostly concerned about compat with older clusters, but I guess it only returned ErrLeaseNotFound in the rc's for 3.1?

@gyuho
etcd-tester: check expired lease with -1 TTL
fb81fb4 
Following the change at etcd-io@2ca1823

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@gyuho gyuho force-pushed the etcd-tester-lease-check-with-ttl branch from 15b9052 to fb81fb4 Compare March 3, 2017 19:42
@fanminshi
Copy link
Member

lgtm

@gyuho
Copy link
Contributor Author

gyuho commented Mar 3, 2017

@fanminshi @heyitsanthony I added comments on v3.1 compatibility, since TTL -1 only lives on master branch. Will make sure we highlight those in release note.

@gyuho gyuho merged commit 2831b9d into etcd-io:master Mar 3, 2017
@gyuho gyuho deleted the etcd-tester-lease-check-with-ttl branch March 3, 2017 20:50
@codecov-io
Copy link

codecov-io commented Mar 4, 2017 •
edited
Loading

Codecov Report

Merging #7415 into master will increase coverage by 0.42%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7415      +/-   ##
==========================================
+ Coverage    70.7%   71.12%   +0.42%     
==========================================
  Files         236      236              
  Lines       21093    21093              
==========================================
+ Hits        14914    15003      +89     
+ Misses       5047     4970      -77     
+ Partials     1132     1120      -12
Impacted Files Coverage Δ
rafthttp/peer.go 90.07% <0%> (-1.53%) ❌
etcdserver/server.go 79.38% <0%> (-0.36%) ❌
clientv3/client.go 77.45% <0%> (+0.4%) ✅
raft/node.go 90.17% <0%> (+0.44%) ✅
etcdserver/v3_server.go 62.93% <0%> (+0.87%) ✅
clientv3/lease.go 92% <0%> (+0.88%) ✅
etcdmain/etcd.go 48.14% <0%> (+1.23%) ✅
lease/leasehttp/http.go 62.59% <0%> (+1.52%) ✅
pkg/adt/interval_tree.go 88.51% <0%> (+1.68%) ✅
rafthttp/msgappv2_codec.go 71.3% <0%> (+1.73%) ✅
... and 7 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 e16db33...fb81fb4. Read the comment docs.

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

@fanminshi fanminshi fanminshi approved these changes

@heyitsanthony heyitsanthony heyitsanthony approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gyuho @fanminshi @codecov-io @heyitsanthony