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

feat(leaderelection): impl leader election for HA Deployment #722

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

Dentrax
Copy link
Contributor

@Dentrax Dentrax commented Feb 11, 2022

Fixes #720

TODO:

  • update README
  • pass namespace dynamically (used BindLeaderElectionFlags)
  • pass durations dynamically from cmdOptions (used LeaderElectionFlag set)
  • better id handle: POD-NAME_UUID (used os.Hostname + UUID)
  • add e2e test
  • update Helm Chart (Pod Static 1)
    • add leaderElection config
    • add podAntiAffinity example
    • enforce replica should not greater than 1 if leaderElection is not set

Signed-off-by: Furkan furkan.turkal@trendyol.com
Co-authored-by: Emin emin.aktas@trendyol.com
Co-authored-by: Yasin yasintaha.erol@trendyol.com

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 11, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @Dentrax. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2022
@Dentrax Dentrax changed the title [W.I.P]: feat(leaderelection): impl leader election [W.I.P]: feat(leaderelection): impl leader election for HA Deployment Feb 11, 2022
@Dentrax Dentrax force-pushed the feature/leaderelection branch 2 times, most recently from 1c0fc48 to 998b557 Compare February 11, 2022 11:56
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2022
@eminaktas eminaktas force-pushed the feature/leaderelection branch 11 times, most recently from a25edab to 01ff31a Compare February 18, 2022 09:34
@Dentrax Dentrax changed the title [W.I.P]: feat(leaderelection): impl leader election for HA Deployment feat(leaderelection): impl leader election for HA Deployment Feb 18, 2022
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

/ok-to-test
I had mostly nits on docs and logging, but the overall idea looks pretty straightforward. Thanks for the contribution @Dentrax !

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
charts/descheduler/templates/deployment.yaml Outdated Show resolved Hide resolved
pkg/descheduler/descheduler.go Outdated Show resolved Hide resolved
pkg/descheduler/descheduler.go Outdated Show resolved Hide resolved
pkg/descheduler/descheduler.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 18, 2022
@damemi
Copy link
Contributor

damemi commented Feb 18, 2022

@Dentrax forgot to mention this, could you please squash your commits down too?

@ingvagabund
Copy link
Contributor

@Dentrax @eminaktas would you mind extending the e2e test suite with a test validating only a single instance is active at a time? E.g.

  • create 2 namespaces (e.g. A, B)
  • create e.g. 5 pods in each
  • wait for 5 or more seconds, configure both deschedulers to evict pods running longer than 5 seconds, each descheduler targeting a specific namespace (includeNamespaces)
  • validate only pods from a single namespace (A xor B) are evicted.

This will be a bit tricky as you will need to invoke Run from the pkg/descheduler/descheduler.go or some higher function so you are running both deschedulers from within the same binary.

@Dentrax
Copy link
Contributor Author

Dentrax commented Mar 17, 2022

@Dentrax @eminaktas would you mind extending the e2e test suite with a test validating only a single instance is active at a time?

We've tried to write a simple test by following your instructions, here is test output:

=== RUN   TestLeaderElection
    e2e_leaderelection_test.go:45: Creating testing namespace e2e-testleaderelection-a
    e2e_leaderelection_test.go:52: Creating testing namespace e2e-testleaderelection-b
    e2e_leaderelection_test.go:175: Creating deployment leaderelection for namespace e2e-testleaderelection-a
    e2e_leaderelection_test.go:175: Creating deployment leaderelection for namespace e2e-testleaderelection-b
    e2e_leaderelection_test.go:203: Pod leaderelection-76dbf88fff-kpv7d not running yet, is Pending instead
    e2e_leaderelection_test.go:203: Pod leaderelection-76dbf88fff-v98g4 not running yet, is Pending instead
    e2e_leaderelection_test.go:94: starting deschedulers
I0317 12:40:08.240823   75207 leaderelection.go:248] attempting to acquire leader lease kube-system/descheduler...
I0317 12:40:08.270859   75207 leaderelection.go:258] successfully acquired lease kube-system/descheduler
I0317 12:40:11.232282   75207 leaderelection.go:248] attempting to acquire leader lease kube-system/descheduler...
    e2e_leaderelection_test.go:135: Total evicted Pods count for e2e-testleaderelection-a: is 5 and e2e-testleaderelection-b: is 14
--- PASS: TestLeaderElection (43.24s)
PASS
ok      sigs.k8s.io/descheduler/test/e2e        44.618s

We also had to put two new policy files since we couldn't pass a PolicyConfig into DeschedulerServer struct, so we bring the following files:

test/e2e/policy_leaderelection_a.yaml
test/e2e/policy_leaderelection_b.yaml

Eventually, we're asserting that only pods from a single namespace are evicted:

singleNamespaceEvicted := (podsA == 5 && podsB != 5) || (podsA != 5 && podsB == 5)

Thanks, @ingvagabund! I think we've resolved the following above reviews. I'm not sure whether it's the best way to test leader election, but waiting your thoughts.

@Dentrax Dentrax force-pushed the feature/leaderelection branch 4 times, most recently from acdfeac to c3b1ddc Compare March 17, 2022 12:32
@a7i
Copy link
Contributor

a7i commented Mar 17, 2022

Re: RBAC, I would have expected the Role definition to include the following:

  - apiGroups: ["coordination.k8s.io"]
    resources: ["leases"]
    verbs: ["create"]
  - apiGroups: ["coordination.k8s.io"]
    resources: ["leases"]
    resourceNames: ["descheduler"]
    verbs: ["get", "patch", "delete"]

@eminaktas eminaktas force-pushed the feature/leaderelection branch 2 times, most recently from 4512e7d to 474a492 Compare March 17, 2022 20:32
@eminaktas
Copy link
Contributor

/retest

@ingvagabund
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
test/e2e/e2e_leaderelection_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
pkg/descheduler/leaderelection.go Outdated Show resolved Hide resolved
cmd/descheduler/app/options/options.go Outdated Show resolved Hide resolved
test/e2e/e2e_leaderelection_test.go Outdated Show resolved Hide resolved
@ingvagabund
Copy link
Contributor

ingvagabund commented Mar 25, 2022

Few more nits I noticed. Durations are the most important here. We need to set those wisely to reduce the traffic between the descheduler instances and the apiserver.

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Signed-off-by: eminaktas <eminaktas34@gmail.com>
Co-authored-by: Emin <emin.aktas@trendyol.com>
Co-authored-by: Yasin <yasintaha.erol@trendyol.com>
@ingvagabund
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Dentrax, ingvagabund

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2022
@ingvagabund
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2022
@k8s-ci-robot k8s-ci-robot merged commit 238eebe into kubernetes-sigs:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HA Mode for Deployment using Leader Election
6 participants