Skip to content

fix get azure accounts timeout issue when there is no out-bound IP #74191

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 2 commits into from
Feb 22, 2019
Merged

fix get azure accounts timeout issue when there is no out-bound IP #74191

k8s-ci-robot merged 2 commits into from
Feb 22, 2019

Conversation

@andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Feb 18, 2019 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
make getAllStorageAccounts func happen only when there is create unmanaged disk request, for managed disk cluster, it's not necessary to make getAllStorageAccounts func run.

Which issue(s) this PR fixes:

Fixes #74190

Original issue: kubernetes-sigs/cloud-provider-azure#86

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix get azure accounts timeout issue when there is no out-bound IP

/kind bug
/assign @feiskyer
/priority important-soon
/sig azure

cc @djsly

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 18, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/azure cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 18, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2019
@andyzhangx andyzhangx mentioned this pull request Feb 18, 2019
Closed
feiskyer
feiskyer reviewed Feb 18, 2019
View reviewed changes
@djsly
Copy link

djsly commented Feb 18, 2019

Question, does the call to getAllStorageAccounts needs to be done at instantiation time ?

@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 19, 2019 •
edited
Loading

Question, does the call to getAllStorageAccounts needs to be done at instantiation time ?

yes, and it's for unmanaged disk which is corner scenario now, while for compatibility, I still keep it in the code. So is setting timeout as 1 min ok in your scenario? @djsly

@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 19, 2019 •
edited
Loading

I have fixed @feiskyer 's comments, and will wait for @djsly 's reply, to make sure whether this PR is acceptable in the no outbound IP initiallly scenario.

@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 19, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2019
@djsly
Copy link

djsly commented Feb 19, 2019

@andyzhangx if you are saying that there is no choice but to query the list of StorageAccount at initiation time, then I guess it's good.

@djsly
Copy link

djsly commented Feb 19, 2019

I was looking at the code, and I see that the accounts property of the BlobDiskController isn't used at all in the azure.go. The bloc controller gets created blobController, err := newBlobDiskController(common) but not used until a // create/attach/detach/delete blob based (unmanaged disks) tasks is performed.

type BlobDiskController struct {
	common   *controllerCommon
	accounts map[string]*storageAccountState
}

Couldnt the call to accounts, err := c.getAllStorageAccounts() be done on the first call of create/attach/detach/delete blob, like those public methods could be enhanced with a "isInitialized" and this would allow them to call accounts, err := c.getAllStorageAccounts() if it hasn't been initialized. Preventing the API call all together during the initialization of the Azure Cloud Provider object ?

Food for though...

@andyzhangx andyzhangx changed the title set timeout for get azure account operation fix get azure accounts timeout issue when there is no out-bound IP Feb 19, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 19, 2019
@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 19, 2019

@djsly thanks for your code analysis, you are right:
getAllStorageAccounts func should only happen when there is create unmanaged disk request, for managed disk cluster, it's not necessary to make getAllStorageAccounts func run.

I have changed the code according to your request. PTAL, thanks.

djsly
djsly reviewed Feb 19, 2019
View reviewed changes
andyzhangx
andyzhangx commented Feb 20, 2019
View reviewed changes
Copy link
Member Author

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for @djsly 's suggestion, this is an old issue which should be fixed in earlier version. I will cherry pick to earlier version when this PR get merged.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2019
@djsly
Copy link

djsly commented Feb 20, 2019

@andyzhangx , perfect! thanks for the cherry picks.

@djsly
Copy link

djsly commented Feb 20, 2019

hello, I tested 1.13.3 + patch on a VM that doesn't have an outbound IP. We have a Standard ILB in k8s with a single frontend from the vnet's subnet.

This time, there isn't any timeouts

t[78376]: I0220 17:10:09.032750   78376 azure_auth.go:58] azure: using managed identity extension to retrieve access token
t[78376]: I0220 17:10:09.032768   78376 azure_auth.go:69] azure: using System Assigned MSI to retrieve access token
t[78376]: I0220 17:10:09.032805   78376 azure.go:258] Azure cloudprovider (read ops) using rate limit config: QPS=25, bucket=200
t[78376]: I0220 17:10:09.032821   78376 azure.go:262] Azure cloudprovider (write ops) using rate limit config: QPS=10, bucket=100
t[78376]: I0220 17:10:09.032933   78376 azure.go:324] Azure cloudprovider using try backoff: retries=6, exponent=1.500000, duration=6, jitter=1.000000
t[78376]: I0220 17:10:09.032983   78376 server.go:525] Successfully initialized cloud provider: "azure" from the config file: "/etc/kubernetes/azure.json"
t[78376]: I0220 17:10:09.033003   78376 server.go:791] cloud provider determined current node name to be kn-default-2

I still have a problem where all my nodes are showing up as NotReady now after they updated with the dirty kubelet. I'll pursue the investigation

NAME           STATUS                     ROLES    AGE    VERSION
km0            Ready,SchedulingDisabled   master   43h    v1.13.3-dirty
km1            Ready,SchedulingDisabled   master   43h    v1.13.3-dirty
km2            Ready,SchedulingDisabled   master   43h    v1.13.3-dirty
kn-default-0   NotReady                   node     43h    v1.13.3-dirty
kn-default-1   NotReady                   node     26h    v1.13.3-dirty
kn-default-2   NotReady                   <none>   146m   v1.13.3-dirty
kn-infra-0     NotReady                   infra    43h    v1.13.3-dirty
kn-infra-1     NotReady                   infra    43h    v1.13.3-dirty
kn-infra-2     NotReady                   infra    43h    v1.13.3-dirty

@djsly
Copy link

djsly commented Feb 20, 2019 •
edited
Loading

ok the new call now that timesout after 10mins is:

 78376 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-2, location: eastus2

t[78376]: E0220 17:19:40.370516   78376 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-2, location: eastus2
t[78376]: I0220 17:19:40.370570   78376 kubelet_node_status.go:446] Recording NodeHasSufficientMemory event message for node kn-default-2
t[78376]: E0220 17:19:40.370581   78376 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-2, location: eastus2
t[78376]: I0220 17:19:40.370595   78376 kubelet_node_status.go:446] Recording NodeHasNoDiskPressure event message for node kn-default-2
t[78376]: I0220 17:19:40.370606   78376 setters.go:736] Error getting volume limit for plugin kubernetes.io/aws-ebs
t[78376]: I0220 17:19:40.370612   78376 kubelet_node_status.go:446] Recording NodeHasSufficientPID event message for node kn-default-2
t[78376]: I0220 17:19:40.370617   78376 setters.go:736] Error getting volume limit for plugin kubernetes.io/gce-pd
t[78376]: I0220 17:19:40.370631   78376 kubelet_node_status.go:446] Recording NodeHasSufficientMemory event message for node kn-default-2
t[78376]: I0220 17:19:40.370650   78376 kubelet_node_status.go:446] Recording NodeHasNoDiskPressure event message for node kn-default-2
t[78376]: I0220 17:19:40.370658   78376 cpu_manager.go:155] [cpumanager] starting with none policy
t[78376]: I0220 17:19:40.370670   78376 cpu_manager.go:156] [cpumanager] reconciling every 10s
t[78376]: I0220 17:19:40.370685   78376 policy_none.go:42] [cpumanager] none policy: Start
t[78376]: I0220 17:19:40.370955   78376 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-2", UID:"kn-default-2", APIVers
t[78376]: I0220 17:19:40.370982   78376 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-2", UID:"kn-default-2", APIVers
t[78376]: I0220 17:19:40.371000   78376 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-2", UID:"kn-default-2", APIVers
t[78376]: I0220 17:19:40.371015   78376 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-2", UID:"kn-default-2", APIVers
t[78376]: I0220 17:19:40.371035   78376 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-2", UID:"kn-default-2", APIVers
t[78376]: I0220 17:19:40.371259   78376 container_manager_linux.go:376] Updating kernel flag: vm/overcommit_memory, expected value: 1, actual value: 0
t[78376]: I0220 17:19:40.371614   78376 container_manager_linux.go:376] Updating kernel flag: kernel/panic, expected value: 10, actual value: 0
t[78376]: I0220 17:19:40.371920   78376 cgroup_manager_linux.go:274] The Cgroup [kubepods] has some missing paths: [/sys/fs/cgroup/cpu,cpuacct/kubepods /sy
t[78376]: I0220 17:19:40.370665   78376 kubelet_node_status.go:446] Recording NodeHasSufficientPID event message for node kn-default-2
t[78376]: I0220 17:19:40.376631   78376 kubelet_node_status.go:72] Attempting to register node kn-default-2

@djsly
Copy link

djsly commented Feb 20, 2019 

result, err := az.VirtualMachineSizesClient.List(context.TODO(), az.Location)
                if err != nil || result.Value == nil {
                        klog.Errorf("failed to list vm sizes in GetVolumeLimits, plugin.host: %s, location: %s", plugin.host.GetHostName(), az.Location)
                        return volumeLimits, nil
                }

Problem here is that the source of the call is in k8s, not azure.go

./pkg/kubelet/nodestatus/setters.go:                    attachLimits, err := volumePlugin.GetVolumeLimits()

Can't we use instanceMetadata for this ?

I'm not sure yet, why the full list of VmSizes in a SubID is required.

// List lists virtual-machine-sizes available in a location for a subscription.
//
// location is the location upon which virtual-machine-sizes is queried.

@djsly
Copy link

djsly commented Feb 20, 2019

so I now understand better how this works, Since the instance metadata doesn't provide the MaxDataDiskCount you get the list of all VM instance type possible in the SubID/Location and match the InstanceType of the current VM with the list to extract the MaxDataDiskCount ...

I would try your timeout logic you initially wrote but I can't find the commit anymore, must have been squashed. Do we know if the instance metadata team could augment the metadata with this information ?

@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 21, 2019

@djsly use this code for set timeout as 1 min:

ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)

can we merge this PR first? @djsly

@djsly
Copy link

djsly commented Feb 21, 2019

Sure this is good to go. I guess we will need to open another one for the issue with the

result, err := az.VirtualMachineSizesClient.List(context.TODO(), az.Location)

I will leave that to your discretion to see if you prefer to manage two PRs with multiple cherry picks.

Or if it would be best tackle all those issues within the same PR, since in the end all this work is related to Kubelet not starting in a timely fashion when there isn't any outbound IP

@djsly
Copy link

djsly commented Feb 22, 2019

OK I tested the context.WithTimeout() on the VirtualMachineSizesClient API call and this seem to be the last long blocking call . You can see bellow that the node isn't registered for 15sec, until the first API call timesout.

I0221 23:14:50.774027   22942 kubelet_node_status.go:278] Setting node annotation to enable volume controller attach/detach
belet[22942]: I0221 23:14:50.774066   22942 kubelet_node_status.go:326] Adding node label from cloud provider: beta.kubernetes.io/instance-type=Sta
belet[22942]: I0221 23:14:50.774077   22942 azure_zones.go:69] Availability zone is not enabled for the node, falling back to fault domain
belet[22942]: I0221 23:14:50.774085   22942 kubelet_node_status.go:337] Adding node label from cloud provider: failure-domain.beta.kubernetes.io/zo
belet[22942]: I0221 23:14:50.774093   22942 kubelet_node_status.go:341] Adding node label from cloud provider: failure-domain.beta.kubernetes.io/re
belet[22942]: W0221 23:14:50.774139   22942 setters.go:144] replacing cloudprovider-reported hostname of kn-default-3 with overridden hostname of k
...
...
belet[22942]: E0221 23:14:50.827380   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:14:50.927536   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:14:51.027770   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:14:51.127979   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:14:51.228213   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:14:51.328371   22942 kubelet.go:2266] node "kn-default-3" not found
...
...
...
belet[22942]: E0221 23:15:04.780600   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:15:04.880789   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:15:04.980982   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: E0221 23:15:05.066718   22942 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-3, location: eastu
belet[22942]: I0221 23:15:05.066768   22942 setters.go:736] Error getting volume limit for plugin kubernetes.io/aws-ebs
belet[22942]: I0221 23:15:05.066779   22942 setters.go:736] Error getting volume limit for plugin kubernetes.io/gce-pd
belet[22942]: I0221 23:15:05.066800   22942 kubelet_node_status.go:446] Recording NodeHasSufficientMemory event message for node kn-default-3
belet[22942]: I0221 23:15:05.066824   22942 kubelet_node_status.go:446] Recording NodeHasNoDiskPressure event message for node kn-default-3
belet[22942]: I0221 23:15:05.066838   22942 kubelet_node_status.go:446] Recording NodeHasSufficientPID event message for node kn-default-3
belet[22942]: I0221 23:15:05.066866   22942 kubelet_node_status.go:72] Attempting to register node kn-default-3
belet[22942]: I0221 23:15:05.067321   22942 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-3", UID:"kn-default
belet[22942]: I0221 23:15:05.067343   22942 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-3", UID:"kn-default
belet[22942]: I0221 23:15:05.067354   22942 server.go:459] Event(v1.ObjectReference{Kind:"Node", Namespace:"", Name:"kn-default-3", UID:"kn-default
belet[22942]: E0221 23:15:05.081204   22942 kubelet.go:2266] node "kn-default-3" not found
belet[22942]: I0221 23:15:05.082542   22942 kubelet_node_status.go:75] Successfully registered node kn-default-3

@djsly
Copy link

djsly commented Feb 22, 2019

also, before my patch, we can see that the calls were failing every 10mins (I'm guessing kubelet will keep retrying) and after I patched, on existing Nodes, the error messages is every ~15sec

Feb 21 19:28:23  kubelet[42192]: E0221 19:28:23.656890   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 19:38:53  kubelet[42192]: E0221 19:38:53.679577   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 19:49:23  kubelet[42192]: E0221 19:49:23.703875   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 19:59:53  kubelet[42192]: E0221 19:59:53.725452   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 20:10:23  kubelet[42192]: E0221 20:10:23.754637   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 20:20:53  kubelet[42192]: E0221 20:20:53.788917   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 20:31:23  kubelet[42192]: E0221 20:31:23.818574   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 20:41:53  kubelet[42192]: E0221 20:41:53.840404   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 20:52:23  kubelet[42192]: E0221 20:52:23.891879   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:02:53  kubelet[42192]: E0221 21:02:53.917138   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:13:23  kubelet[42192]: E0221 21:13:23.937846   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:23:53  kubelet[42192]: E0221 21:23:53.957359   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:34:23  kubelet[42192]: E0221 21:34:23.981998   42192 azure_dd.go:165] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2

Feb 21 21:38:07  kubelet[88761]: E0221 21:38:07.171911   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:38:22  kubelet[88761]: E0221 21:38:22.191122   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:39:22  kubelet[88761]: E0221 21:39:22.191373   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:40:37  kubelet[88761]: E0221 21:40:37.212780   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:41:52  kubelet[88761]: E0221 21:41:52.234522   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:43:07  kubelet[88761]: E0221 21:43:07.254709   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:44:22  kubelet[88761]: E0221 21:44:22.276533   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:45:37  kubelet[88761]: E0221 21:45:37.300389   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:46:52  kubelet[88761]: E0221 21:46:52.319842   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:48:07  kubelet[88761]: E0221 21:48:07.348183   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:49:22  kubelet[88761]: E0221 21:49:22.367277   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:50:37  kubelet[88761]: E0221 21:50:37.392020   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:51:52  kubelet[88761]: E0221 21:51:52.410877   88761 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:52:29  kubelet[97239]: E0221 21:52:29.759218   97239 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:52:44  kubelet[97239]: E0221 21:52:44.785222   97239 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:52:59  kubelet[97239]: E0221 21:52:59.802677   97239 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2
Feb 21 21:53:59  kubelet[97239]: E0221 21:53:59.802780   97239 azure_dd.go:168] failed to list vm sizes in GetVolumeLimits, plugin.host: kn-default-1, location: eastus2

@feiskyer
Copy link
Member

feiskyer commented Feb 22, 2019

@djsly Thanks for validating this.

@andyzhangx Could you fix the issue within same PR? They are actually same issue, but just different APIs. And we need to cherry pick the changes to stable releases.

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Feb 22, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

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:

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

@andyzhangx
Copy link
Member Author

andyzhangx commented Feb 22, 2019

@feiskyer @djsly pls review commit: 28a9aa8

feiskyer
feiskyer reviewed Feb 22, 2019
View reviewed changes
pkg/cloudprovider/providers/azure/azure_blobDiskController.go Outdated
Copy link
Member

@feiskyer feiskyer Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep c.accouts as nil here? or else, it won't try again on errors.

Copy link
Member Author

@andyzhangx andyzhangx Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feiskyer yes, won't try again on error, it would make new map and create new storage account(don't use existing accounts)

feiskyer
feiskyer reviewed Feb 22, 2019
View reviewed changes
pkg/volume/azure_dd/azure_dd.go Outdated
Copy link
Member

@feiskyer feiskyer Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add same timeout to getAllStorageAccounts?

Copy link
Member Author

@andyzhangx andyzhangx Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feiskyer fixed.

feiskyer
feiskyer reviewed Feb 22, 2019
View reviewed changes
pkg/volume/azure_dd/azure_dd.go Outdated
Copy link
Member

@feiskyer feiskyer Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: define a consts for timeout, e.g. listAPITimeout = 60*time.Second

@feiskyer
Copy link
Member

feiskyer commented Feb 22, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Feb 22, 2019
@andyzhangx
add timeout in GetVolumeLimits operation
8cd09bb 
add timeout for getAllStorageAccounts
feiskyer
feiskyer reviewed Feb 22, 2019
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit 8d6f20e into kubernetes:master Feb 22, 2019
@djsly
Copy link

djsly commented Feb 22, 2019

Thanks guys!

This was referenced Feb 25, 2019
Merged
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Feb 27, 2019
@k8s-ci-robot
Merge pull request #74486 from andyzhangx/automated-cherry-pick-of-#7…
c27b913 
…4191-upstream-release-1.13

Automated cherry pick of #74191: remove get azure accounts in the init process set timeout
k8s-ci-robot added a commit that referenced this pull request Mar 5, 2019
@k8s-ci-robot
Merge pull request #74490 from andyzhangx/automated-cherry-pick-of-#7…
b2539d5 
…4191-upstream-release-1.11

Automated cherry pick of #74191: remove get azure accounts in the init process set timeout
k8s-ci-robot added a commit that referenced this pull request Mar 7, 2019
@k8s-ci-robot
Merge pull request #74489 from andyzhangx/automated-cherry-pick-of-#7…
b5fae76 
…4191-upstream-release-1.12

Automated cherry pick of #74191: remove get azure accounts in the init process set timeout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brendandburns brendandburns Awaiting requested review from brendandburns

@khenidak khenidak Awaiting requested review from khenidak

2 more reviewers

@feiskyer feiskyer feiskyer left review comments

@djsly djsly djsly left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@feiskyer feiskyer

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v1.14

Development

Successfully merging this pull request may close these issues.

A storageAccount API call blocks kubelet from becoming Ready when there isn't any outbound IP

4 participants

@andyzhangx @djsly @feiskyer @k8s-ci-robot