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.

Sign up for GitHub

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

Jump to bottom

Enable v3 Client as the default on UTs #30890

Merged
merged 2 commits into from
Aug 23, 2016
Merged

Enable v3 Client as the default on UTs #30890

merged 2 commits into from
Aug 23, 2016

Conversation

timothysc
Copy link
Member

@timothysc timothysc commented Aug 18, 2016 •
edited by k8s-oncall

Updates the default initialization to use clientv3 interface to etcd3, and fixes the UTs.

This PR includes a cherry-pick of #30634 so we can validate the tests, so do not merge until that PR is complete.

This change is Reviewable

@timothysc timothysc added area/etcd release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 18, 2016
@timothysc timothysc added this to the v1.4 milestone Aug 18, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2016
@timothysc timothysc removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 19, 2016
@timothysc timothysc removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2016
@timothysc
Copy link
Member Author

Hmm I can't seem to reproduce these UT issues locally. I'll take a look in the a.m.

wojtek-t
wojtek-t reviewed Aug 19, 2016
View reviewed changes
pkg/genericapiserver/options/server_run_options.go
@@ -415,7 +415,7 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
fs.MarkDeprecated("service-node-ports", "see --service-node-port-range instead")

fs.StringVar(&s.StorageConfig.Type, "storage-backend", s.StorageConfig.Type,
"The storage backend for persistence. Options: 'etcd2' (default), 'etcd3'.")
"The storage backend for persistence. Options: 'etcd2', 'etcd3' (default).")
Copy link
Member

Choose a reason for hiding this comment

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

As long as I'm probably fine with changing the default for tests in this PR, I don't think we should do it for binaries (since we still don't have rollback etc., so we may not able to launch it).

@lavalamp - thoughts?

@wojtek-t
Copy link
Member

I don't think we are ready to switch it on by default now (we don't have full migration, rollback, etc.). Though I'm probably fine with changing it for unit tests.

@lavalamp - thoughts?

@timothysc
Copy link
Member Author

I'll wait for the call on this one, and adjust the PR appropriately.

@lavalamp lavalamp added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 19, 2016
@lavalamp
Copy link
Member

Can we separate the unit test fixes into a separate PR?

@lavalamp
Copy link
Member

We definitely can't switch the global default like this right now. If you want to change the OSS setup scripts to start using etcd3, we can talk about that. But definitely we can't change the GKE setup at this point. Since different people need different settings, I think you have to plumb the setting through the setup code rather than actually flipping the flag default.

@timothysc
Copy link
Member Author

Can we separate the unit test fixes into a separate PR?

Yeah I can simply modify this PR.

@timothysc timothysc changed the title Enable v3 Client as the default Enable v3 Client as the default on UTs Aug 19, 2016
@timothysc timothysc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 19, 2016
@hongchaodeng hongchaodeng mentioned this pull request Aug 19, 2016
Closed
30 tasks
@wojtek-t
Copy link
Member

@timothysc - can you please take a look into test failures - these seem to be related.

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 22, 2016
@timothysc
Copy link
Member Author

@k8s-bot test this please, issue #IGNORE

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 22, 2016
Closed
@wojtek-t
Copy link
Member

OK - seems that test are passing now. Thanks!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016
@timothysc timothysc removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
@timothysc timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
@timothysc
Copy link
Member Author

@wojtek-t @hongchaodeng forced rebase found a minor bug in the client that I patched around.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2016
@xiang90 xiang90 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2016
@wojtek-t
Copy link
Member

LGTM

@timothysc
Copy link
Member Author

timothysc commented Aug 23, 2016 •
edited

@k8s-bot test this please, issue #IGNORE

@k8s-bot
Copy link

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit 99e0176.

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 23, 2016
Closed
@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 Aug 23, 2016

GCE e2e build/test passed for commit 99e0176.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 9484b2c into kubernetes:master Aug 23, 2016
This was referenced Aug 24, 2016
Closed
Closed
@lavalamp lavalamp mentioned this pull request Aug 24, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Aug 25, 2016
Merge pull request #31371 from kubernetes/revert-30890-etcd3_v3client
b70a758 
Automatic merge from submit-queue

Revert "Enable v3 Client as the default on UTs"

Reverts #30890

Fix flake problems (#31262)
@hongchaodeng hongchaodeng mentioned this pull request Nov 13, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Nov 14, 2016
Merge pull request #36712 from hongchaodeng/e5
38cc6df 
Automatic merge from submit-queue

cacher test: fix leftover v2 test server

I think this was dismissed in #30890

@timothysc @wojtek-t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@hongchaodeng hongchaodeng

@xiang90 xiang90

@wojtek-t wojtek-t

Labels
area/etcd 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@timothysc @wojtek-t @lavalamp @k8s-bot @k8s-github-robot @hongchaodeng @xiang90 @googlebot