Use monotonic clock in lease manager skew calculations #6940

Merged
merged 1 commit into from Feb 9, 2017

Conversation

Projects
None yet
3 participants
Owner

wallyworld commented Feb 8, 2017

Description of change

A monotonic clock is introduced to the lease manager so that when it calculates skew, there is no possibility of time going backwards and causing an error.

QA steps

bootstrap and run list-models to smoke test

Bug reference

https://bugs.launchpad.net/juju/+bug/1662202

state/lease/client.go
if err := clockDoc.validate(); err != nil {
return nil, errors.Annotatef(err, "corrupt clock document")
}
// Create skew entries for each known writer...
- skews, err := clockDoc.skews(beginning, end)
+ readTime := endRead - startRead
+ skews, err := clockDoc.skews(beginning, beginning.Add(readTime*time.Nanosecond))
@axw

axw Feb 8, 2017

Member

no need for *time.Nanosecond

state/lease/client_race_test.go
err := f.Client.Refresh()
c.Assert(err, jc.ErrorIsNil)
}
-func (s *ClientNTPSuite) TestTimeGoesBackwardsALittle(c *gc.C) {
+func (s *ClientNTPSuite) TestTimeGoesBackwards(c *gc.C) {
@axw

axw Feb 8, 2017

Member

is this one of the tests you have to fix? because I don't think it makes sense anymore. we're only using the wall clock for the initial time stamp, so the step doesn't even come into play

@wallyworld

wallyworld Feb 8, 2017

Owner

yep, who suit deleted

state/lease/schema.go
@@ -172,13 +172,8 @@ func (doc clockDoc) skews(beginning, end time.Time) (map[string]Skew, error) {
// If it isn't, it could be ntp rolling the clock back slowly, so we add
@axw

axw Feb 8, 2017

Member

this comment and check are irrelevant now? we're using a monotonic clock, so by definition this cannot happen

@wallyworld

wallyworld Feb 8, 2017

Owner

missed deleting this first time, done now

@axw

axw Feb 8, 2017

Member

you didn't delete the check. it's not harmful, except it's second-guessing the definition of monotonicity

@wallyworld wallyworld changed the title from Allow for greater clock skew than 10ms in lease manager set up to Use monotonic clock in lease manager skew calculations Feb 8, 2017

axw approved these changes Feb 8, 2017

LGTM. Would prefer if we didn't check end<beginning now, but I'll leave that up to you.

Owner

wallyworld commented Feb 8, 2017

$$merge$$

Contributor

jujubot commented Feb 8, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Feb 9, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10246

Owner

wallyworld commented Feb 9, 2017

$$merge$$

Contributor

jujubot commented Feb 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 4f81885 into juju:2.1 Feb 9, 2017

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