change aws encryptedCheck to exponential backoff #78601

rusik69 · 2019-05-31T21:07:00Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
changing poll to exponential backoff for waiting to create encrypted aws volumes

Which issue(s) this PR fixes:
Fixes #77741

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-05-31T21:07:08Z

Hi @Loqutus. Thanks for your PR.

I'm waiting for a kubernetes 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.

rusik69 · 2019-05-31T21:07:51Z

/assign @gnufied

gnufied · 2019-05-31T22:16:10Z

/ok-to-test

jsafrane · 2019-06-03T08:00:45Z

See #78596 - what numbers are better? Did anyone run some benchmark?

jsafrane · 2019-06-03T15:19:01Z

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

-	encryptedCheckInterval = 1 * time.Second
-	encryptedCheckTimeout  = 30 * time.Second
+	// average, 8s max.
+	encryptedCheckInitialDelay = 1 * time.Second


Can you please rename the variables? They're used for generic volume creation check now.

gnufied · 2019-06-03T16:23:29Z

I did some benchmarking btw. It looks like on average it takes on average between 5-6 seconds for volume to become available after creation. So, defaults proposed in this PR is not great.

I propose we change, default to:

	backoff := wait.Backoff{
		Duration: 5 * time.Second,
		Factor:   1.2,
		Steps:    10,
	}

The idea is - exponential backoff fires first call without wait and subsequent calls are more in line with expectation. Also this will cause the whole operation to wait for 129 seconds.

gnufied · 2019-06-03T16:26:47Z

It might even be better to not poll volume immediately after creation at all because volume is never available immediately after creation. But I see that ExponentialBackoff does not offer a way to do that.

gnufied · 2019-06-07T18:10:39Z

@Loqutus are you planning to fix this PR? It is something that I would prefer that we fix sooner than later.

rusik69 · 2019-06-11T20:54:20Z

/retest

bertinatto

Could you squash the commits?

bertinatto · 2019-06-12T14:10:18Z

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

@@ -225,12 +225,13 @@ const (
 	createTagFactor       = 2.0
 	createTagSteps        = 9

-	// encryptedCheck* is configuration of poll for created volume to check
+	// volumeCreate* is configuration of exponential backoff for created volume to check
 	// it has not been silently removed by AWS.
 	// On a random AWS account (shared among several developers) it took 4s on


it has not been silently removed by AWS

Seems like this is no longer accurate

bertinatto · 2019-06-12T14:40:37Z

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

+		Factor:   volumeCreateBackoffFactor,
+		Steps:    volumeCreateBackoffSteps,
+	}
+	err = wait.ExponentialBackoff(backoff, func() (done bool, err error) {


It might even be better to not poll volume immediately after creation at all because volume is never available immediately after creation. But I see that ExponentialBackoff does not offer a way to do that.

Yeah, that could save us a DescribeVolume request, which might be helpful for clusters hitting the AWS quota limit.

What if we use PollUntil instead? PollUntil waits the interval before firing the function.

I am fine with adding time.Sleep(5 * time.Second) before proceeding with exponential backoff. It has same effect as anything else and works with exponential backoff..

bertinatto · 2019-06-12T14:41:37Z

CC @leakingtapan

gnufied · 2019-06-24T18:11:44Z

/lgtm

rusik69 · 2019-07-08T20:18:06Z

/retest

gnufied · 2019-07-08T20:25:11Z

Do you mind backporting this fix to 1.15 and 1.14 versions?

/approve

k8s-ci-robot · 2019-07-08T20:25:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, loqutus

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:

~~staging/src/k8s.io/legacy-cloud-providers/aws/OWNERS~~ [gnufied]

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

fejta-bot · 2019-07-09T01:04:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-07-09T04:55:01Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-07-09T09:49:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-07-09T12:58:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

…8601-upstream-release-1.14 Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff

…8601-upstream-release-1.15 Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff

change aws encryptedCheck to exponential backoff

e691f7d

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2019

k8s-ci-robot assigned gnufied May 31, 2019

k8s-ci-robot added area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 31, 2019

k8s-ci-robot requested review from micahhausler and zmerlynn May 31, 2019 21:09

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 May 31, 2019

jsafrane mentioned this pull request Jun 3, 2019

Fix volume available wait #78596

Closed

jsafrane reviewed Jun 3, 2019

View reviewed changes

Ruslan Gustomiasov added 2 commits June 8, 2019 20:10

rename and change aws backoff vars

98dd133

fix volumecreate comments

ee5d623

bertinatto reviewed Jun 12, 2019

View reviewed changes

add sleep 5 before exponential backoff in waitUntilVolumeAvailable

2eeb863

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2019

k8s-ci-robot merged commit c88bda6 into kubernetes:master Jul 9, 2019

huffmanca mentioned this pull request Jul 18, 2019

Bug 1704356: Backport AWS cloud-provider improvements openshift/origin#23424

Merged

bertinatto mentioned this pull request Oct 23, 2019

Lower AWS DescribeVolume frequency #84181

Merged

gnufied mentioned this pull request Nov 6, 2019

Automated cherry pick of #75054: Remove the condition for only wait for KMS key is used #84659

Merged

This was referenced Nov 8, 2019

Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff #84953

Merged

Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff #84954

Merged

k8s-ci-robot added a commit that referenced this pull request Nov 8, 2019

Merge pull request #84953 from melnikalex/automated-cherry-pick-of-#7…

5423298

…8601-upstream-release-1.14 Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff

k8s-ci-robot added a commit that referenced this pull request Nov 9, 2019

Merge pull request #84954 from melnikalex/automated-cherry-pick-of-#7…

bd88250

…8601-upstream-release-1.15 Automated cherry pick of #78601: change aws encryptedCheck to exponential backoff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change aws encryptedCheck to exponential backoff #78601

change aws encryptedCheck to exponential backoff #78601

rusik69 commented May 31, 2019

k8s-ci-robot commented May 31, 2019

rusik69 commented May 31, 2019

gnufied commented May 31, 2019

jsafrane commented Jun 3, 2019

jsafrane Jun 3, 2019

gnufied commented Jun 3, 2019 •

edited

gnufied commented Jun 3, 2019

gnufied commented Jun 7, 2019

rusik69 commented Jun 11, 2019

bertinatto left a comment

bertinatto Jun 12, 2019

bertinatto Jun 12, 2019

gnufied Jun 14, 2019

bertinatto commented Jun 12, 2019

gnufied commented Jun 24, 2019

rusik69 commented Jul 8, 2019

gnufied commented Jul 8, 2019

k8s-ci-robot commented Jul 8, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

change aws encryptedCheck to exponential backoff #78601

change aws encryptedCheck to exponential backoff #78601

Conversation

rusik69 commented May 31, 2019

k8s-ci-robot commented May 31, 2019

rusik69 commented May 31, 2019

gnufied commented May 31, 2019

jsafrane commented Jun 3, 2019

jsafrane Jun 3, 2019

Choose a reason for hiding this comment

gnufied commented Jun 3, 2019 • edited

gnufied commented Jun 3, 2019

gnufied commented Jun 7, 2019

rusik69 commented Jun 11, 2019

bertinatto left a comment

Choose a reason for hiding this comment

bertinatto Jun 12, 2019

Choose a reason for hiding this comment

bertinatto Jun 12, 2019

Choose a reason for hiding this comment

gnufied Jun 14, 2019

Choose a reason for hiding this comment

bertinatto commented Jun 12, 2019

gnufied commented Jun 24, 2019

rusik69 commented Jul 8, 2019

gnufied commented Jul 8, 2019

k8s-ci-robot commented Jul 8, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

fejta-bot commented Jul 9, 2019

gnufied commented Jun 3, 2019 •

edited