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

Use monotonic time in lease #6888

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

fanminshi
Copy link
Member

lease uses monotimer to calculate its expiration. In this way, changing system time won't affect in lease expiration.

FIX #6700

@@ -35,8 +35,8 @@ etcd_build() {
if [ -n "${BINDIR}" ]; then out="${BINDIR}"; fi
toggle_failpoints
# Static compilation is useful when etcd is run in a container
CGO_ENABLED=0 go build $GO_BUILD_FLAGS -installsuffix cgo -ldflags "$GO_LDFLAGS" -o ${out}/etcd ${REPO_PATH}/cmd/etcd || return
Copy link
Contributor

@gyuho gyuho Nov 22, 2016

Choose a reason for hiding this comment

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

Is there anyway that we can enabled CGO only for darwin?
Maybe detect architecture in build script or make it configurable.

Linux still works fine with CGO disabled, as far as I know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Try if [[ "$OSTYPE" == "darwin"* ]]; then

@fanminshi fanminshi added the WIP label Nov 22, 2016
@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 152dcdb to 86f0bfd Compare November 22, 2016 01:19
@@ -22,6 +22,7 @@ import (
"sync"
"time"

"github.com/ScaleFT/monotime"
Copy link
Contributor

Choose a reason for hiding this comment

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

let's put a space between external dependency and internal dependency (coreos/etcd/xxx)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@xiang90
Copy link
Contributor

xiang90 commented Nov 23, 2016

@fanminshi The author suggests us to use https://github.com/aristanetworks/goarista/blob/master/monotime/nanotime.go instead of his library.

@fanminshi
Copy link
Member Author

@xiang90 the library that the author suggested imports unsafe. Not sure if that will cause a problem.
@heyitsanthony what do you think?

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 86f0bfd to 7d30c8a Compare November 23, 2016 19:13
@fanminshi
Copy link
Member Author

I sent a PR(ScaleFT/monotime#1) ScaleFT to use aristanetworks/goarista/monotime as source of monotime.

@heyitsanthony
Copy link
Contributor

@fanminshi the scaleft package already imports unsafe anyway. The vendoring for all the w32 stuff seems like overkill. Just pulling in nanotime() / using the arista library is probably better

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch 2 times, most recently from 3db0b96 to e99e48a Compare November 28, 2016 22:40
@fanminshi
Copy link
Member Author

fanminshi commented Nov 28, 2016

@heyitsanthony the scaleft guy decides to use "golang.org/x/sys/windows" for windows part. ScaleFT/monotime#2

@fanminshi
Copy link
Member Author

All fixed. PLTAL @xiang90 @xiang90 @heyitsanthony

@heyitsanthony
Copy link
Contributor

@fanminshi vendor updates should be separate commits, but is there any reason not to use the arista code? The windows deps still seem excessive for what this does.

@fanminshi
Copy link
Member Author

fanminshi commented Nov 28, 2016

@heyitsanthony I'll make separate commits for vendored stuff. My original PR to scaleFT uses arista code for windows,ScaleFT/monotime#1. However, I think scaleFT guy decided to use "golang.org/x/sys/windows". In this case, if I want to use ScaleFT, I have to import the windows deps. is there a way to get around that. I could just use our own version of their code like https://github.com/fanminshi/monotime

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch 2 times, most recently from 2968469 to 584b431 Compare November 29, 2016 00:30
@fanminshi fanminshi removed the WIP label Nov 29, 2016
@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch 4 times, most recently from 37260f6 to 20841b7 Compare November 29, 2016 00:40
@fanminshi
Copy link
Member Author

All fixed. PTAL @xiang90 @heyitsanthony @gyuho

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package monotime
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line above? Otherwise it will break godoc?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@gyuho gyuho Nov 29, 2016

Choose a reason for hiding this comment

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

I still see

+// See the License for the specific language governing permissions and
+// limitations under the License.
+package monotime

?

Can you double-check?

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package monotime

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fixed now

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 20841b7 to 2ccf41b Compare November 29, 2016 00:47
return Now()
}

func (t *monoTimer) Since(start uint64) time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need separate Since 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.

It seems to me Since() could be useful function. I could make it a private helper function.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this is a method on monoTimer since it doesn't use anything from monoTimer

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok keeping it sounds good to me.

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 2ccf41b to e78ebb5 Compare November 29, 2016 01:06
// Use of this source code is governed by the Apache License 2.0
// that can be found in the COPYING file.

// Package monotime provides a fast monotonic clock source.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment

// that can be...

package monotime

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change

@gyuho
Copy link
Contributor

gyuho commented Nov 29, 2016

lgtm. defer to @xiang90 @heyitsanthony

return Now()
}

func (t *monoTimer) Since(start uint64) time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this is a method on monoTimer since it doesn't use anything from monoTimer

// expiry time in unixnano
expiry time.Time
// expiry is time when lease should expire
expiry time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the wrong type; Durations aren't deadlines. Probably have a type monotime.Time uint64 (to indicate it's not a time based on the system time) or something?

"time"
)

type Timer interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this interface wasn't going to be used? Why are monotime.Now() and monotime.Since(uint64) not good enough? I don't think wrapping Now/Since with this really buys anything

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, monotime.Now() and monotime.Since(uint64) should be sufficient. Will change to that

// Since returns duration between two points of time
Since(uint64) time.Duration
// SinceBegining returns duration between now and when timer is created
SinceBegining() time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

Elapsed

forever = time.Unix(math.MaxInt64>>1, 0)

monoTimer = monotime.New()
forever = time.Duration(1>>63 - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

math.MaxInt64

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from e78ebb5 to 3a8847f Compare November 29, 2016 19:47
@fanminshi
Copy link
Member Author

All fixed. PTAL @heyitsanthony @gyuho

@xiang90
Copy link
Contributor

xiang90 commented Nov 29, 2016

@fanminshi

CI fails with

pkg/monotime/monotime.go.21: // time represents a point in monotonic time (godoc-export: time -> Time?)

"time"
)

// time represents a point in monotonic time
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah let's change this to Time... go vet complains

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I just changed that.

@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 3a8847f to 49a73be Compare November 29, 2016 20:20
// Use of this source code is governed by the Apache License 2.0
// that can be found in the COPYING file.

// Package monotime provides a fast monotonic clock source.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

// Package monotime provides a fast monotonic clock source.
package monotime

Otherwise godoc won't catch it.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

// Copyright (C) 2016 Arista Networks, Inc.
// Use of this source code is governed by the Apache License 2.0

// Package monotime provides a fast monotonic clock source.
Copy link
Contributor

@gyuho gyuho Nov 29, 2016

Choose a reason for hiding this comment

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

And this description is duplicate?

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

lease uses monotimer to calculate its expiration. In this way, changing system time won't affect in lease expiration.

FIX etcd-io#6700
@fanminshi fanminshi force-pushed the use_monotonic_time_for_lease branch from 49a73be to e7f4010 Compare November 29, 2016 20:31
@fanminshi
Copy link
Member Author

All fixed. PTAL @xiang90 @heyitsanthony @gyuho

@heyitsanthony
Copy link
Contributor

lgtm

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

Successfully merging this pull request may close these issues.

4 participants