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

Bump up etcd dependency to fix data race #24022

Merged
merged 3 commits into from
Apr 17, 2016

Conversation

hongchaodeng
Copy link
Contributor

ref: #23694

What this PR does

  • Bumping up the godep of etcd to fix data race in etcd watcher. Without this change, watcher PR builds will fail in race detection.
  • Small changes to fix builds after upgrade

@hongchaodeng
Copy link
Contributor Author

/cc @wojtek-t @timothysc @xiang90

This is prereq to the watcher implementation.
It also provides the API of event types as requested by Tim.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 8, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit df41d88abcae75608997e0f40b14012ecc70dbaf.

@k8s-bot
Copy link

k8s-bot commented Apr 8, 2016

GCE e2e build/test passed for commit 71b46f3.

@timothysc
Copy link
Member

Looks like the tests are green, but just so I'm clear this change to the server looks to affect both clients? and is this an officially released version of etcd?

We typically only bump the dep on sanctioned releases.

@xiang90
Copy link
Contributor

xiang90 commented Apr 8, 2016

and is this an officially released version of etcd?

This is not. Hongchao is working on v3 so he was tracking the master.

@timothysc
Copy link
Member

k, then we should either close this pull or mark as WIP to revisit at a later date.

@hongchaodeng
Copy link
Contributor Author

The v2 client change just remove the outdated code because we are not using go1.4 anymore.

The changes to v2 client include:

@hongchaodeng
Copy link
Contributor Author

@timothysc
I think the changes on v2 client are pretty minor, actually good for go1.6. Do you still have any concern I can help answer?

@timothysc
Copy link
Member

@lavalamp @wojtek-t ping on policy here?
Do we want to allow godep from non-released versions?

@lavalamp
Copy link
Member

lavalamp commented Apr 8, 2016

I don't care that much about the client as long as it works. This doesn't change the version of etcd that we run, which I do care about. However it also changes the version of etcd that we build for kube2sky (?maybe? @ArtfulCoder). However we're going to stop doing that in the near future anyway.

I guess my only concern would be if this causes our tests to test something that is not the same as what we run against in production.

@hongchaodeng
Copy link
Contributor Author

I want to point out that we ensure it is as close to production env as possible by using real etcd binaries in testing, i.e. e2e and integration testing, not the imported version for unit testing.

etcd also runs a bunch of CI to ensure API behavior doesn't change, including regression, of both v2 and v3.

@timothysc timothysc added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 9, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 10, 2016

GCE e2e build/test passed for commit 71b46f3.

@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit 71b46f3.

@k8s-bot
Copy link

k8s-bot commented Apr 14, 2016

GCE e2e build/test passed for commit 71b46f3.

@xiang90
Copy link
Contributor

xiang90 commented Apr 15, 2016

/cc @k8s-oncall Can we get this merged?

@mikedanese mikedanese added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 16, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 16, 2016

GCE e2e build/test passed for commit 71b46f3.

@hongchaodeng
Copy link
Contributor Author

Something seems have changed in godep licenses. Updated

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 16, 2016
@hongchaodeng
Copy link
Contributor Author

Can you help re-add 'lgtm' label?
@timothysc @mikedanese @yifan-gu

@k8s-bot
Copy link

k8s-bot commented Apr 16, 2016

GCE e2e build/test passed for commit 66bd277db6a78129862977be2fe14a63da20645d.

@hongchaodeng
Copy link
Contributor Author

@k8s-bot unit test this please github issue: #24298

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 66bd277db6a78129862977be2fe14a63da20645d.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 3c2c906.

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 17, 2016

GCE e2e build/test passed for commit 3c2c906.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5f3f06f into kubernetes:master Apr 17, 2016
@hongchaodeng hongchaodeng deleted the dep branch April 17, 2016 21:10
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Nov 26, 2019
Bug 1719686: Fix ceph expansion

Origin-commit: 0b8be0c706c82dd106af405702bde33fbc7a6e7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants