Use zone from node for topology aware aws-ebs volume creation #78276

zhan849 · 2019-05-24T04:41:20Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
For topology aware (WaitForFirstConsumer) volume provisioning, node is already selected when we call cloud provider to create the volume. In aws-ebs volume provisioner, we currently still list ec2 instances for getting candidate zones, which is inefficient and redundant.

This PR keeps same behavior for volumes that need to bind immediately, but use zone info from bound node for topology aware volumes. If node for some reason does not have zone info recorded (which is unexpected), we double check with cloud provider.

With some 50pvc batch provisioning banchmark, this optimization can reduce 75% DescribeInstances calls, and reduce 50% peak QPS.

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

Special notes for your reviewer:
This issue is also being discussed in #78199 and #76976 , as reducing cloud provider calls is anyway necessary for optimizing topo aware volume provisioning code path, and whether and how to optimize finding zone for bind immediately volumes can be an orthogonal problem.

Does this PR introduce a user-facing change?:
No

Release Note:

Use zone from node for topology aware aws-ebs volume creation to reduce unnecessary cloud provider calls

/sig storage aws scalability
/priority important-soon
/cc @msau42 @gnufied @liggitt @smarterclayton @mcrute

k8s-ci-robot · 2019-05-24T04:41:28Z

Hi @zhan849. 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.

gnufied · 2019-05-24T14:02:31Z

/ok-to-test

pkg/volume/awsebs/aws_util.go

gnufied · 2019-05-24T17:34:36Z

pkg/volume/awsebs/aws_util.go

@@ -125,6 +128,39 @@ func (util *AWSDiskUtil) CreateVolume(c *awsElasticBlockStoreProvisioner, node *
 	return name, volumeOptions.CapacityGB, labels, fstype, nil
 }

+// getCandidateZones finds possible zones that a volume can be created in
+func getCandidateZones(cloud *aws.Cloud, selectedNode *v1.Node, pvcName string) (sets.String, error) {


Lets add some unit tests if we can...

I couldn't find a good way to mock aws.Cloud outside its package, so only added test cases for delayed binding test case. It's probably cloud provider's responsibility to provide reasonable mocks so I left a TODO here. please check

I think that is fine. GetCandidateZonesForDynamicVolume should already be considered "tested".

zhan849 · 2019-05-24T22:03:47Z

/retest

pkg/volume/awsebs/aws_util.go

zhan849 · 2019-05-24T23:32:25Z

/retest

zhan849 · 2019-05-25T01:17:11Z

/retest

gnufied · 2019-05-26T15:46:59Z

Looks good to me. This code change should be fairly innocuous to land in. But I would wait for @jsafrane and @leakingtapan too. Will remove the hold once it has been reviewed by either of these folks.

/lgtm
/hold

pkg/volume/awsebs/aws_util.go

jsafrane · 2019-05-27T14:03:44Z

pkg/volume/awsebs/aws_ebs_test.go

+					},
+				},
+			},
+			expectedZones: sets.NewString(),


The tests don't seem to be useful to me if they check the zones to be empty.

so the reasoning here for the function is - if we have a selected node, there should not be a "candidate zone" returned, and if not (bind immediately), we should compute candidate zone. once we have better way to mock aws cloud provider, we need to add other test cases.

Or since the logic is rather simple, I can remove the added helper and combine the logic in CreateVolume so we can remove the test. What do you think?

zhan849 · 2019-05-28T01:15:44Z

/retest

zhan849 · 2019-05-28T03:15:32Z

/retest

zhan849 · 2019-05-28T07:01:34Z

/retest

gnufied · 2019-05-30T18:36:04Z

/lgtm

gnufied · 2019-05-30T18:36:17Z

/hold cancel

gnufied · 2019-05-30T18:36:36Z

/assign @jsafrane @leakingtapan

gnufied · 2019-05-30T18:39:14Z

@jsafrane I think this should be good to go. Can you approve?

jsafrane · 2019-05-31T07:41:26Z

I think the PR is OK, just the unit test does not seem to be really useful. On the other hand, it fits well the rest of the AWS provider :-)

/approve

This is a bug fix, trying to squeeze into 1.15.

k8s-ci-robot · 2019-05-31T07:42:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, zhan849

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:

~~pkg/volume/awsebs/OWNERS~~ [jsafrane]

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

zhan849 · 2019-06-03T17:41:02Z

I think the PR is OK, just the unit test does not seem to be really useful. On the other hand, it fits well the rest of the AWS provider :-)

/approve

This is a bug fix, trying to squeeze into 1.15.

Thanks @jsafrane for milestone 1.15, do I need to send out PR for cherry-pick or there will be an automatic process for this?

jsafrane · 2019-06-04T09:26:42Z

It has been merged to master, AFAIK it should get promoted to 1.15 branch automatically (if it is not there yet).

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 24, 2019

k8s-ci-robot requested review from gnufied, liggitt, mcrute and msau42 May 24, 2019 04:41

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 24, 2019

k8s-ci-robot requested a review from smarterclayton May 24, 2019 04:41

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2019

zhan849 mentioned this pull request May 24, 2019

Discuss the feasibility of aws-ebs provisioner to get node zone info from k8s instead of cloud provider #78199

Closed

zhan849 changed the title ~~Use zone from node for topology aware volume creation~~ Use zone from node for topology aware aws-ebs volume creation May 24, 2019

zhan849 mentioned this pull request May 24, 2019

AWS EBS provisioner should get node zone info from k8s #76976

Merged

gnufied reviewed May 24, 2019

View reviewed changes

pkg/volume/awsebs/aws_util.go Outdated Show resolved Hide resolved

gnufied reviewed May 24, 2019

View reviewed changes

zhan849 force-pushed the ebs-get-zone-opt1 branch 2 times, most recently from 110f089 to 5a70ddf Compare May 24, 2019 21:25

gnufied reviewed May 24, 2019

View reviewed changes

pkg/volume/awsebs/aws_util.go Outdated Show resolved Hide resolved

zhan849 force-pushed the ebs-get-zone-opt1 branch from 5a70ddf to 7a4784e Compare May 24, 2019 22:47

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2019

k8s-ci-robot assigned gnufied May 26, 2019

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

jsafrane reviewed May 27, 2019

View reviewed changes

Use zone from node for topology aware volume creation

90eb408

zhan849 force-pushed the ebs-get-zone-opt1 branch from 7a4784e to 90eb408 Compare May 28, 2019 00:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2019

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2019

k8s-ci-robot assigned jsafrane and leakingtapan May 30, 2019

jsafrane added this to the v1.15 milestone May 31, 2019

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

k8s-ci-robot merged commit f14b128 into kubernetes:master Jun 1, 2019

huffmanca mentioned this pull request Jul 18, 2019

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

Merged

wongma7 mentioned this pull request Oct 9, 2019

Introduce node cache to aws library #78409

Closed

b0b0haha mentioned this pull request Dec 15, 2021

Use zone from node for topology aware volume creation #107045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use zone from node for topology aware aws-ebs volume creation #78276

Use zone from node for topology aware aws-ebs volume creation #78276

zhan849 commented May 24, 2019 •

edited

Loading

k8s-ci-robot commented May 24, 2019

gnufied commented May 24, 2019

gnufied May 24, 2019

zhan849 May 24, 2019

gnufied May 24, 2019

zhan849 commented May 24, 2019

zhan849 commented May 24, 2019

zhan849 commented May 25, 2019

gnufied commented May 26, 2019

jsafrane May 27, 2019

zhan849 May 28, 2019

zhan849 commented May 28, 2019

zhan849 commented May 28, 2019

zhan849 commented May 28, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

jsafrane commented May 31, 2019

k8s-ci-robot commented May 31, 2019

zhan849 commented Jun 3, 2019

jsafrane commented Jun 4, 2019

Use zone from node for topology aware aws-ebs volume creation #78276

Use zone from node for topology aware aws-ebs volume creation #78276

Conversation

zhan849 commented May 24, 2019 • edited Loading

k8s-ci-robot commented May 24, 2019

gnufied commented May 24, 2019

gnufied May 24, 2019

Choose a reason for hiding this comment

zhan849 May 24, 2019

Choose a reason for hiding this comment

gnufied May 24, 2019

Choose a reason for hiding this comment

zhan849 commented May 24, 2019

zhan849 commented May 24, 2019

zhan849 commented May 25, 2019

gnufied commented May 26, 2019

jsafrane May 27, 2019

Choose a reason for hiding this comment

zhan849 May 28, 2019

Choose a reason for hiding this comment

zhan849 commented May 28, 2019

zhan849 commented May 28, 2019

zhan849 commented May 28, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

gnufied commented May 30, 2019

jsafrane commented May 31, 2019

k8s-ci-robot commented May 31, 2019

zhan849 commented Jun 3, 2019

jsafrane commented Jun 4, 2019

zhan849 commented May 24, 2019 •

edited

Loading