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

Bumping up etcd dep to fix data race #23808

Closed
wants to merge 2 commits into from

Conversation

hongchaodeng
Copy link
Contributor

ref: #23694

What this PR does

  • Bumping up the godep of etcd to fix data race in etcd watcher.

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@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 4, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 4, 2016

GCE e2e build/test failed for commit 34908ac9b4145ab2992ef20d3794ca251c9396a4.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Apr 4, 2016

GCE e2e build/test failed for commit eff39aa3cd8c60dd86850e965b91e431618cd7ef.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Apr 4, 2016

GCE e2e build/test failed for commit 812e120a100c7f49cb7e479c43a9322825acbd38.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@hongchaodeng
Copy link
Contributor Author

It seemed like etcd master dropped support for go 1.4

@eparis eparis assigned hongchaodeng and unassigned eparis Apr 4, 2016
@eparis
Copy link
Contributor

eparis commented Apr 4, 2016

I assigned the PR to you. You should 'unassign' the PR when you are ready. Or ping me/re-assign to me.

@hongchaodeng
Copy link
Contributor Author

Blocked on #22149

@hongchaodeng
Copy link
Contributor Author

Rebased after the merged Go 1.6 PR.
I'm just bumping up the godep of etcd. But I'm adding the watcher commit here to test if the data race is fixed. Will remove it after the build pass.

/cc @wojtek-t @gmarek @xiang90

@k8s-bot
Copy link

k8s-bot commented Apr 6, 2016

GCE e2e build/test passed for commit 1c11fc65e021c275e05c7aa7978c3e8e499ca1a9.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@k8s-bot
Copy link

k8s-bot commented Apr 6, 2016

GCE e2e build/test passed for commit f9af323.

@gmarek
Copy link
Contributor

gmarek commented Apr 6, 2016

@hongchaodeng please squash commits.

@Random-Liu
Copy link
Member

/cc @ixdy

@ixdy
Copy link
Member

ixdy commented Apr 6, 2016

Should probably be explicitly noted somewhere that this PR ratchets us into Go 1.6+, assuming I'm understanding correctly.

@Random-Liu
Copy link
Member

@ixdy
This PR build failed because

Godeps/_workspace/src/github.com/coreos/etcd/client/cancelreq.go:13:
req.Cancel undefined (type *http.Request has no field or method Cancel)

The Cancel field is added after go 1.5 (See here), while it's not there in go 1.4 (See here).

@ixdy
Copy link
Member

ixdy commented Apr 6, 2016

OK, though due to #20656 Kubernetes basically doesn't support go 1.5.

@ixdy
Copy link
Member

ixdy commented Apr 6, 2016

My basic worry is thus: we just merged the Go 1.6 PR yesterday, and we've been seeing some failures in CI that we can't fully explain away yet. I'm hesitant to lock us into Go 1.5+ until we're sure that things are not broken and we're not going to roll back.

cc @kubernetes/goog-testing

@hongchaodeng
Copy link
Contributor Author

Closing this to transfer discussion to #23915.

Once go version is resolved, will open up another PR.

@wojtek-t
Copy link
Member

wojtek-t commented Apr 6, 2016

@ixdy - one point from me about Go 1.6 to note - we don't observe any performance issues with it, so from performance perspective the upgrade is fine.

@ixdy
Copy link
Member

ixdy commented Apr 7, 2016

all Google-run Jenkins VMs should be using Go 1.6 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants