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

Update lease renewer logic #4090

Merged
merged 6 commits into from
Mar 19, 2018
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 66 additions & 15 deletions api/renewer.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ var (
ErrRenewerNotRenewable = errors.New("secret is not renewable")
ErrRenewerNoSecretData = errors.New("returned empty secret data")

// DefaultRenewerGrace is the default grace period
DefaultRenewerGrace = 15 * time.Second

// DefaultRenewerRenewBuffer is the default size of the buffer for renew
// messages on the channel.
DefaultRenewerRenewBuffer = 5
Expand Down Expand Up @@ -111,9 +108,6 @@ func (c *Client) NewRenewer(i *RenewerInput) (*Renewer, error) {
}

grace := i.Grace
if grace == 0 {
grace = DefaultRenewerGrace
}

random := i.Rand
if random == nil {
Expand Down Expand Up @@ -184,6 +178,9 @@ func (r *Renewer) renewAuth() error {
return ErrRenewerNotRenewable
}

priorDuration := time.Duration(r.secret.LeaseDuration) * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Should this be r.secret.Auth.LeaseDuration instead of r.secret.LeaseDuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Good catch.

r.calculateGrace(priorDuration)

client, token := r.client, r.secret.Auth.ClientToken

for {
Expand Down Expand Up @@ -216,13 +213,28 @@ func (r *Renewer) renewAuth() error {
return ErrRenewerNotRenewable
}

// Grab the lease duration and sleep duration - note that we grab the auth
// lease duration, not the secret lease duration.
// Grab the lease duration
leaseDuration := time.Duration(renewal.Auth.LeaseDuration) * time.Second
sleepDuration := r.sleepDuration(leaseDuration)

// If we are within grace, return now.
if leaseDuration <= r.grace || sleepDuration <= r.grace {
// We keep evaluating a new grace period so long as the lease is
// extending. Once it stops extending, we've hit the max and need to
// rely on the grace duration.
if leaseDuration > priorDuration {
Copy link
Member

Choose a reason for hiding this comment

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

Should we calculate the new grace if the lease duration doesn't change, which could be a likely common thing? In other words, should this be leaseDuration >= priorDuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the lease duration doesn't change, the grace period would be within the same parameters. Recalculating it would just shift the amount of random jitter, which if it's truly random won't either help or hurt, so can be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

r.calculateGrace(leaseDuration)
}
priorDuration = leaseDuration

// The sleep duration is set to 2/3 of the current lease duration plus
// 1/3 of the current grace period, which adds jitter.
sleepDuration := time.Duration(float64(leaseDuration.Nanoseconds())*2/3 + float64(r.grace.Nanoseconds()*1/3))
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: The argument to time.Duration is of the form a + b. In that a has different parenthesis order in comparison with b. Can it be the same just for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this was subtly wrong, good catch.


// If we are within grace, return now; or, if the amount of time we
// would sleep would land us in the grace period. This helps with short
// tokens; for example, you don't want a current lease duration of 4
// seconds, a grace period of 3 seconds, and end up sleeping for more
// than three of those seconds and having a very small budget of time
// to renew.
if leaseDuration <= r.grace || leaseDuration-sleepDuration <= r.grace {
return nil
}

Expand All @@ -241,6 +253,9 @@ func (r *Renewer) renewLease() error {
return ErrRenewerNotRenewable
}

priorDuration := time.Duration(r.secret.LeaseDuration) * time.Second
r.calculateGrace(priorDuration)

client, leaseID := r.client, r.secret.LeaseID

for {
Expand Down Expand Up @@ -273,12 +288,28 @@ func (r *Renewer) renewLease() error {
return ErrRenewerNotRenewable
}

// Grab the lease duration and sleep duration
// Grab the lease duration
leaseDuration := time.Duration(renewal.LeaseDuration) * time.Second
sleepDuration := r.sleepDuration(leaseDuration)

// If we are within grace, return now.
if leaseDuration <= r.grace || sleepDuration <= r.grace {
// We keep evaluating a new grace period so long as the lease is
// extending. Once it stops extending, we've hit the max and need to
// rely on the grace duration.
if leaseDuration > priorDuration {
r.calculateGrace(leaseDuration)
}
priorDuration = leaseDuration

// The sleep duration is set to 2/3 of the current lease duration plus
// 1/3 of the current grace period, which adds jitter.
sleepDuration := time.Duration(float64(leaseDuration.Nanoseconds())*2/3 + float64(r.grace.Nanoseconds()*1/3))

// If we are within grace, return now; or, if the amount of time we
// would sleep would land us in the grace period. This helps with short
// tokens; for example, you don't want a current lease duration of 4
// seconds, a grace period of 3 seconds, and end up sleeping for more
// than three of those seconds and having a very small budget of time
// to renew.
if leaseDuration <= r.grace || leaseDuration-sleepDuration <= r.grace {
return nil
}

Expand Down Expand Up @@ -307,3 +338,23 @@ func (r *Renewer) sleepDuration(base time.Duration) time.Duration {

return time.Duration(sleep)
}

// calculateGrace calculates the grace period based on a reasonable set of
// assumptions given the total lease time; it also adds some jitter to not have
// clients be in sync. We calculate this continuously so long as the new lease
Copy link
Contributor

Choose a reason for hiding this comment

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

We calculate this continuously so long as the new lease
 +// duration is greater than the previous; no change means we don't need to
 +// recalculate, and if the lease duration keeps decreasing we've hit max and
 +// want to be able to rely on this.

This is actually done by the callers so shouldn't be on this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed that part of the comment.

// duration is greater than the previous; no change means we don't need to
// recalculate, and if the lease duration keeps decreasing we've hit max and
// want to be able to rely on this.
func (r *Renewer) calculateGrace(leaseDuration time.Duration) {
if leaseDuration == 0 {
r.grace = 0
return
}

leaseNanos := float64(leaseDuration.Nanoseconds())
jitterMax := 0.1 * leaseNanos

// For a given lease duration, we want to allow 80-90% of that to elapse,
// so the remaining amount is the grace period
r.grace = time.Duration(leaseNanos*0.1) + time.Duration(uint64(r.random.Int63())%uint64(jitterMax))
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the initialized jitterMax var as argument to the time.Duration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}