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

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

Merged
merged 1 commit into from Jun 1, 2019

Conversation

@zhan849
Copy link
Contributor

commented May 24, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

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.

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

@gnufied

This comment has been minimized.

Copy link
Member

commented May 24, 2019

/ok-to-test

Show resolved Hide resolved pkg/volume/awsebs/aws_util.go Outdated
@@ -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) {

This comment has been minimized.

Copy link
@gnufied

gnufied May 24, 2019

Member

Lets add some unit tests if we can...

This comment has been minimized.

Copy link
@zhan849

zhan849 May 24, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@gnufied

gnufied May 24, 2019

Member

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

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

@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

/retest

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

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

@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

/retest

1 similar comment
@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

/retest

@gnufied

This comment has been minimized.

Copy link
Member

commented May 26, 2019

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

Show resolved Hide resolved pkg/volume/awsebs/aws_util.go Outdated
},
},
},
expectedZones: sets.NewString(),

This comment has been minimized.

Copy link
@jsafrane

jsafrane May 27, 2019

Member

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

This comment has been minimized.

Copy link
@zhan849

zhan849 May 28, 2019

Author Contributor

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 zhan849 force-pushed the zhan849:ebs-get-zone-opt1 branch from 7a4784e to 90eb408 May 28, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 28, 2019

@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

/retest

2 similar comments
@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

/retest

@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

/retest

@gnufied

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 30, 2019

@gnufied

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/hold cancel

@gnufied

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@gnufied

This comment has been minimized.

Copy link
Member

commented May 30, 2019

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

@jsafrane

This comment has been minimized.

Copy link
Member

commented May 31, 2019

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.

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

[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:

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 merged commit f14b128 into kubernetes:master Jun 1, 2019

20 checks passed

cla/linuxfoundation zhan849 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@zhan849

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.