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

Leader election defaults were updated #203

Merged
merged 1 commit into from
Dec 21, 2018
Merged

Leader election defaults were updated #203

merged 1 commit into from
Dec 21, 2018

Conversation

prashanth26
Copy link
Contributor

  • Leader election now acquires the lock before main begins
  • Updated leader election lease duration 15s --> 60s and renewal deadline 10s --> 30s
  • Shorter default leader election renewal deadline of 10s was causing failures while renewing the lease (on Azure)

What this PR does / why we need it:
Shorter default leader election renewal deadline of 10s was causing failures while renewing the lease (on Azure)

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Leader election defaults were updated

@prashanth26 prashanth26 added needs/review Needs review priority/blocker Needs to be resolved now, because it breaks the service size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/new Issue is new and unprocessed component/machine-controller-manager kind/cleanup Something that is not needed anymore and can be cleaned up topology/seed Affects Seed clusters labels Dec 17, 2018
@prashanth26 prashanth26 requested review from ggaurav10 and a team as code owners December 17, 2018 11:35
@prashanth26 prashanth26 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 17, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 17, 2018
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 18, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 18, 2018
@prashanth26 prashanth26 mentioned this pull request Dec 18, 2018
Merged
@prashanth26 prashanth26 force-pushed the enhancement/leader-election-defaults branch from af16517 to 73ad454 Compare December 19, 2018 07:51
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2018
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) kind/task General task and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review labels Dec 19, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2018
@prashanth26 prashanth26 force-pushed the enhancement/leader-election-defaults branch from 974d2ae to dedd806 Compare December 19, 2018 10:44
@gardener-robot-ci-1 gardener-robot-ci-1 added the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 19, 2018
@prashanth26 prashanth26 force-pushed the enhancement/leader-election-defaults branch from dedd806 to bacc72c Compare December 19, 2018 12:34
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2018
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 20, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 20, 2018
@prashanth26 prashanth26 force-pushed the enhancement/leader-election-defaults branch from 2d73dba to bacc72c Compare December 20, 2018 08:12
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 20, 2018
@prashanth26
Leader election defaults updated
533c48c 
- Leader election now acquires the lock before main begins
- Updated leader election lease duration 15s --> 60s, renewal deadline 10s --> 30s and retry period 2s --> 15s while running CI tests
- Shorter default leader election renewal deadline of 10s causes failure while renewing lease in a network intense cluster for CI tests
@prashanth26 prashanth26 force-pushed the enhancement/leader-election-defaults branch from bacc72c to 533c48c Compare December 20, 2018 11:08
@prashanth26 prashanth26 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 20, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 20, 2018
@prashanth26
Copy link
Contributor Author

prashanth26 commented Dec 21, 2018 •
edited
Loading

Hi @hardikdr ,

Can you take a look at this PR, I have just updated the leader election defaults for the CI pipeline Refer these changes. I have confirmed to make these changes with @amshuman-kr 's consent.

And the change in the default leader election maps to acquiring this lock before running the controller. Most controllers including the Gardener do this. Refer here. Hence this change.

Thanks & Regards,
Prashanth

hardikdr
hardikdr approved these changes Dec 21, 2018
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

lgtm

@hardikdr hardikdr merged commit 50e5528 into gardener:master Dec 21, 2018
@prashanth26 prashanth26 deleted the enhancement/leader-election-defaults branch December 24, 2018 07:37
@ghost ghost added the component/mcm Machine Controller Manager (including Node Problem Detector, Cluster Auto Scaler, etc.) label Mar 7, 2020
@gardener-robot gardener-robot added the area/ops-productivity Operator productivity related (how to improve operations) label Jun 18, 2020
@himanshu-kun himanshu-kun mentioned this pull request Feb 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardikdr hardikdr hardikdr approved these changes

@ggaurav10 ggaurav10 Awaiting requested review from ggaurav10

Assignees
No one assigned
Labels
area/ops-productivity Operator productivity related (how to improve operations) component/mcm Machine Controller Manager (including Node Problem Detector, Cluster Auto Scaler, etc.) kind/cleanup Something that is not needed anymore and can be cleaned up kind/task General task needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) priority/blocker Needs to be resolved now, because it breaks the service size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/new Issue is new and unprocessed topology/seed Affects Seed clusters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@prashanth26 @hardikdr @gardener-robot-ci-1 @gardener-robot