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

Add VMSS and VMSSVM clients with backoff retry #86740

Merged
merged 6 commits into from Jan 7, 2020

Conversation

@feiskyer
Copy link
Member

feiskyer commented Dec 31, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

/kind feature
/sig cloud-provider
/area provider/azure
/priority important-soon

What this PR does / why we need it:

Continue of kubernetes-sigs/cloud-provider-azure#247. This PR is based on #86719, and added a set of new Azure clients:

  • Added ARM client with backoff retries
  • Added VMSS/VMSSVM clients based on ARM client, they also
    • support client-side rate limiting
    • suppress requests on throttling
    • avoid retrying on some specific error messages
  • Switch to those clients in cloud provider Azure

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The first 3 commits are coming from #86719.

Does this PR introduce a user-facing change?:

Azure VMSS/VMSSVM clients now suppress requests on throttling

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Dec 31, 2019

/test pull-kubernetes-e2e-aks-engine-azure
/test pull-kubernetes-e2e-azure-disk
/test pull-kubernetes-e2e-azure-disk-vmss

@feiskyer feiskyer force-pushed the feiskyer:arm-vmss-clients branch from 2013b4b to 9356448 Dec 31, 2019
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Dec 31, 2019

Rebased since #86719 has been merged.

/test pull-kubernetes-e2e-aks-engine-azure
/test pull-kubernetes-e2e-azure-disk
/test pull-kubernetes-e2e-azure-disk-vmss

Copy link
Member

cblecker left a comment

/approve
for vendor/

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 2, 2020

/assign @andyzhangx (for Azure changes)
/assign @dims (for go modules changes)

}

// Send sends a http request to ARM service with possible retry to regional ARM endpoint.
func (c *Client) Send(ctx context.Context, request *http.Request) (*http.Response, *retry.Error) {

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Jan 2, 2020

Member

it's a lot of code changes, did you copy from somewhere else? where is the source? is it possible to import that lib?

This comment has been minimized.

Copy link
@feiskyer

feiskyer Jan 2, 2020

Author Member

I'm referring Azure Go SDK for most interfaces (e.g. the VMSS clients here). we're unable to import them directly because we want to control the internal loops ourselves.

See kubernetes-sigs/cloud-provider-azure#247 for backgrounds why we need to do so.

@@ -897,16 +897,12 @@ func (ss *scaleSet) EnsureHostInPool(service *v1.Service, nodeName types.NodeNam
defer cancel()
klog.V(2).Infof("EnsureHostInPool begins to update vmssVM(%s) with new backendPoolID %s", vmName, backendPoolID)
rerr := ss.VirtualMachineScaleSetVMsClient.Update(ctx, nodeResourceGroup, ssName, instanceID, newVM, "network_update")
if rerr != nil && rerr.Retriable && ss.CloudProviderBackoff {

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Jan 2, 2020

Member

where is the retry logic? I don't see armclient is used some where in this PR?

This comment has been minimized.

Copy link
@feiskyer

feiskyer Jan 2, 2020

Author Member

See https://github.com/kubernetes/kubernetes/pull/86740/files#diff-9c5eab3304e3867d8a50d9b7ff7bb8e6R101-R112 for the retries. retry.DoExponentialBackoffRetry() is used as a decorator for backoff retries.

@feiskyer feiskyer force-pushed the feiskyer:arm-vmss-clients branch from 9356448 to c813e25 Jan 2, 2020
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 2, 2020

@andyzhangx Addressed comments and metrics also added back. PTAL

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 2, 2020

/test pull-kubernetes-e2e-aks-engine-azure
/test pull-kubernetes-e2e-azure-disk
/test pull-kubernetes-e2e-azure-disk-vmss

@nilo19

This comment has been minimized.

Copy link
Member

nilo19 commented Jan 2, 2020

Maybe the cluster autoscaler should quote the new version of the VMSS client as well.

/lgtm

Copy link
Member

andyzhangx left a comment

/lgtm

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 2, 2020

@feiskyer why are we adding new features in the legacy in-tree code?

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 3, 2020

@feiskyer why are we adding new features in the legacy in-tree code?

@dims this is because out of tree cloud provider is not production ready yet, e.g. CSI drivers are not GA and out of tree cloud provider doesn't support in-tree PV drivers. So before those dependencies GA, we still need to make changes in in-tree codes.

@kubernetes kubernetes deleted a comment from nilo19 Jan 3, 2020
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 6, 2020

@dims @liggitt could you help to approve the changes?

/assign @dims @liggitt

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 6, 2020

/approve
/lgtm

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

/approve
/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 6, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dims, feiskyer, liggitt

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

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 6, 2020

@dims @liggitt Thanks a lot

/retest

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Jan 6, 2020

/test pull-kubernetes-e2e-gce

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 6, 2020

/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.

@k8s-ci-robot k8s-ci-robot merged commit b046dbc into kubernetes:master Jan 7, 2020
18 checks passed
18 checks passed
cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-aks-engine-azure Job succeeded.
Details
pull-kubernetes-e2e-azure-disk Job succeeded.
Details
pull-kubernetes-e2e-azure-disk-vmss Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 7, 2020
@feiskyer feiskyer deleted the feiskyer:arm-vmss-clients branch Jan 7, 2020
@feiskyer feiskyer mentioned this pull request Jan 8, 2020
11 of 20 tasks complete
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.