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

Include leaderelection in client-go; #39173

Merged
merged 4 commits into from
Jul 12, 2017
Merged

Include leaderelection in client-go; #39173

merged 4 commits into from
Jul 12, 2017

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Dec 22, 2016 •
edited
Loading

Fix #39117
Fix kubernetes/client-go#28

This PR:

  • includes the leaderelection to the staging client-go
  • to avoid conflict with golang's testing package, renames package /testing to /testutil, and renames cache/testing to cache/testframework
client-go now includes the leaderelection package

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 22, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@caesarxuchao caesarxuchao added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 22, 2016
@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 22, 2016
@caesarxuchao
Copy link
Member Author

@k8s-bot bazel test this

@caesarxuchao
Copy link
Member Author

@mikedanese do you know why the bazel build is failing? The "Details" doesn't provide any information.

@caesarxuchao
Copy link
Member Author

@k8s-bot bazel test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2016
@caesarxuchao caesarxuchao mentioned this pull request Dec 31, 2016
Closed
@ideahitme ideahitme mentioned this pull request Jan 2, 2017
Open
@deads2k
Copy link
Contributor

deads2k commented Jan 30, 2017

leader election currently requires access to endpoints which is too much power to grant generally. We shouldn't move leader election until we have a resource specifically for it which we can grant fine-grained access to.

@caesarxuchao caesarxuchao force-pushed the include-leaderelection branch from 6362908 to c1801fc Compare April 14, 2017 18:32
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2017
@caesarxuchao caesarxuchao force-pushed the include-leaderelection branch from c1801fc to 908ce9b Compare April 14, 2017 18:35
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 14, 2017
@caesarxuchao
Copy link
Member Author

caesarxuchao commented Jul 7, 2017 •
edited
Loading

@pmorie @hongchaodeng @deads2k could you take a look? Thanks.

Actually nvm, the componentconfig is not moved k8s.io/api, so the dependency cycle is still there. I'll try to workaround.

@caesarxuchao caesarxuchao force-pushed the include-leaderelection branch from 823a3e0 to 2c2ead7 Compare July 7, 2017 23:13
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2017
@caesarxuchao caesarxuchao force-pushed the include-leaderelection branch from 2c2ead7 to 0951384 Compare July 8, 2017 00:02
@caesarxuchao caesarxuchao force-pushed the include-leaderelection branch from 0951384 to 65cedee Compare July 8, 2017 02:04
caesarxuchao
caesarxuchao commented Jul 8, 2017
View reviewed changes
pkg/client/leaderelectionconfig/config.go
DefaultRetryPeriod = 2 * time.Second
)

func DefaultLeaderElectionConfiguration() componentconfig.LeaderElectionConfiguration {
Copy link
Member Author

Choose a reason for hiding this comment

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

I left the componentconfig part in kubernetes. Other parts are moved to client-go.

@caesarxuchao
Copy link
Member Author

Tests passed. @pmorie @hongchaodeng @deads2k PTAL. Thanks.

@hongchaodeng
Copy link
Contributor

/lgtm

Thanks @caesarxuchao for the work.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2017
@smarterclayton
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, hongchaodeng, smarterclayton

Associated issue: 39117

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2017
@caesarxuchao
Copy link
Member Author

/retest

@caesarxuchao
Copy link
Member Author

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 11, 2017 •
edited
Loading

@caesarxuchao: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
Jenkins Bazel Build bb04a39 link @k8s-bot bazel test this
Jenkins GCE e2e bb04a39 link @k8s-bot cvm gce e2e test this
Jenkins GCE etcd3 e2e bb04a39 link @k8s-bot gce etcd3 e2e test this
Jenkins kops AWS e2e bb04a39 link @k8s-bot kops aws e2e test this
Jenkins Kubemark GCE e2e bb04a39 link @k8s-bot kubemark e2e test this
Jenkins unit/integration bb04a39 link @k8s-bot unit test this
Jenkins non-CRI GCE e2e bb04a39 link @k8s-bot non-cri e2e test this
Jenkins GCI GCE e2e bb04a39 link @k8s-bot gci gce e2e test this
Jenkins verification bb04a39 link @k8s-bot verify test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fejta
Copy link
Contributor

fejta commented Jul 11, 2017

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 47232, 48625, 48613, 48567, 39173)

@k8s-github-robot k8s-github-robot merged commit b66be98 into kubernetes:master Jul 12, 2017
@bsalamat bsalamat mentioned this pull request Jul 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmorie pmorie

@hongchaodeng hongchaodeng

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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
No milestone
Development

Successfully merging this pull request may close these issues.