BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle #675

Danil-Grigorev · 2020-08-12T14:27:14Z

Reduce default lease retry rate on 30s which will prevent from heavy writes into etcd at idle, and constrain renew deadline on 90s.

Inspired by openshift/cloud-credential-operator#231

openshift-ci-robot · 2020-08-12T14:27:22Z

@Danil-Grigorev: This pull request references Bugzilla bug 1858400, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle

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.

openshift-ci-robot · 2020-08-12T14:32:44Z

@Danil-Grigorev: This pull request references Bugzilla bug 1858400, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle

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.

michaelgugino

Looks like we should also set RenewDeadline https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/manager.go#L163 to a sensible value

Danil-Grigorev · 2020-08-12T17:03:24Z

Set this on 60s, so it will try once per renew deadline.

Prevent machine controllers from writing in etcd at idle too often by setting 30s retry and 90s deadline on all renewals. BZ 1858403

Prevent machine controllers from writing in etcd at idle too often by setting 60s retry and delay on all renewals. BZ 1858403

Danil-Grigorev · 2020-08-12T17:18:31Z

After some realization, set it on 30s and 90s, so it would retry at least one to twice.

openshift-ci-robot · 2020-08-12T17:19:15Z

@Danil-Grigorev: This pull request references Bugzilla bug 1858400, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle

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.

michaelgugino

Let's do

LeaseDuration = 180 seconds
RenewDeadline = 120 seconds
RetryPeriod = 90 seconds

RenewDeadline needs to be less than LeaseDuration according to the examples.

elmiko · 2020-08-12T19:44:21Z

in general i think this is a good patch, but i agree with @michaelgugino that the values should be higher.

michaelgugino · 2020-08-12T19:49:46Z

Okay, after some more discussion, we determined 120/110/90 might be a better fit.

Don't want the leadership to be too long as if the pod gets moved (eg, upgrades) we don't want operations suspended for too long. 120 seconds would be adequate.

elmiko

thanks for the changes Danil, this looks good to me but i just have a quick question.

elmiko · 2020-08-13T15:00:48Z

pkg/operator/sync.go

 		"--v=3",
 		"--leader-elect=true",
-		"--leader-elect-lease-duration=90s",
+		"--leader-elect-lease-duration=120s",


this is the only confusing part to me. we set the lease duration in the controller-runtime config options, do we also need to set on the command line?

We use the default if not specified on the CLI. This is useful for development/debugging.

ahh, perfect. thanks for the explanation Mike!

michaelgugino

/lgtm

michaelgugino · 2020-08-13T15:56:46Z

pkg/operator/sync.go

 		"--v=3",
 		"--leader-elect=true",
-		"--leader-elect-lease-duration=90s",
+		"--leader-elect-lease-duration=120s",


We use the default if not specified on the CLI. This is useful for development/debugging.

elmiko

/approve

openshift-ci-robot · 2020-08-13T16:34:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [elmiko]

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

openshift-ci-robot · 2020-08-13T16:41:49Z

@Danil-Grigorev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-workers-rhel7	`fe7cbdc`	link	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-gcp-operator	`fe7cbdc`	link	`/test e2e-gcp-operator`
ci/prow/e2e-gcp	`fe7cbdc`	link	`/test e2e-gcp`
ci/prow/e2e-azure	`fe7cbdc`	link	`/test e2e-azure`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci-robot · 2020-08-13T16:46:46Z

@Danil-Grigorev: Some pull requests linked via external trackers have merged: openshift/machine-api-operator#675, openshift/machine-api-operator#649, openshift/cluster-api-provider-ovirt#56, openshift/cluster-api-provider-openstack#109. The following pull requests linked via external trackers have not merged:

openshift/cluster-api-provider-gcp#115 is open
Bugzilla bug 1858400 has been moved to the MODIFIED state.

Details

In response to this:

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle

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.

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 12, 2020

openshift-ci-robot requested review from enxebre and michaelgugino August 12, 2020 14:27

michaelgugino suggested changes Aug 12, 2020

View reviewed changes

Danil-Grigorev force-pushed the leader-election-retry-less branch from a405869 to 9ffe27a Compare August 12, 2020 17:00

Danil-Grigorev added 4 commits August 12, 2020 19:16

Slow the default lease retry and renew rate for MHC controller

3d3b575

Prevent machine controllers from writing in etcd at idle too often by setting 30s retry and 90s deadline on all renewals. BZ 1858403

Slow the default lease retry and renew rate for MachineSet controller

c939c92

Prevent machine controllers from writing in etcd at idle too often by setting 30s retry and 90s deadline on all renewals. BZ 1858403

Slow the default lease retry and renew rate for nodelink controller

5fdd6ef

Prevent machine controllers from writing in etcd at idle too often by setting 60s retry and delay on all renewals. BZ 1858403

Slow the default lease retry and renew rate for vsphere controller

48d9cce

Prevent machine controllers from writing in etcd at idle too often by setting 60s retry and delay on all renewals. BZ 1858403

Danil-Grigorev force-pushed the leader-election-retry-less branch from 9ffe27a to 48d9cce Compare August 12, 2020 17:17

Danil-Grigorev requested a review from michaelgugino August 12, 2020 17:19

michaelgugino suggested changes Aug 12, 2020

View reviewed changes

elmiko mentioned this pull request Aug 12, 2020

Bug 1864352: Add leader election mechanism to release 4.5 #664

Merged

Reconcider the default lease durations on 120/110/90

fe7cbdc

elmiko reviewed Aug 13, 2020

View reviewed changes

michaelgugino approved these changes Aug 13, 2020

View reviewed changes

openshift-ci-robot assigned michaelgugino Aug 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

elmiko approved these changes Aug 13, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2020

openshift-merge-robot merged commit fe883a2 into openshift:master Aug 13, 2020

Danil-Grigorev deleted the leader-election-retry-less branch August 13, 2020 16:52

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle #675

BUG 1858400: [Performance] Lease refresh period for machine-api-controllers is too high, causes heavy writes to etcd at idle #675

Uh oh!

Conversation

Danil-Grigorev commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 12, 2020

Uh oh!

openshift-ci-robot commented Aug 12, 2020

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danil-Grigorev commented Aug 12, 2020

Uh oh!

openshift-ci-robot commented Aug 12, 2020

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko commented Aug 12, 2020

Uh oh!

michaelgugino commented Aug 12, 2020

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

michaelgugino Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

elmiko Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

michaelgugino Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 13, 2020

Uh oh!

openshift-ci-robot commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Danil-Grigorev commented Aug 12, 2020 •

edited

Loading

Danil-Grigorev commented Aug 12, 2020 •

edited

Loading

openshift-ci-robot commented Aug 13, 2020 •

edited

Loading