Welcome @patrickdillon!

It looks like this is your first PR to kubernetes-sigs/cloud-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cloud-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @patrickdillon. Thanks for your PR.

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

/ok-to-test

/lgtm

Thanks for the fix. Can you help add the unit tests?

/cherrypick release-1.32

@MartinForReal: once the present PR merges, I will cherry-pick it on top of release-1.32 in a new PR and assign it to you.

/cherrypick release-1.31

@MartinForReal: once the present PR merges, I will cherry-pick it on top of release-1.31 in a new PR and assign it to you.

/cherrypick release-1.30

@MartinForReal: once the present PR merges, I will cherry-pick it on top of release-1.30 in a new PR and assign it to you.

/cherrypick release-1.29

@MartinForReal: once the present PR merges, I will cherry-pick it on top of release-1.29 in a new PR and assign it to you.

Yes I will add unit tests, and we may need additional changes as the CSI driver is also failing in azure stack.

@MartinForReal @nilo19 do you understand the problem here, particularly why would InstanceView be nil?

PlatformFaultDomain in vm properties states that it requires a minimal API version incompatible with Azure Stack, but I would not necessarily expect InstanceView to be nil, so I was not certain this is the right fix:

I find understanding the supported API versions with Azure Stack to be very challenging

I find understanding the supported API versions with Azure Stack to be very challenging

@patrickdillon are you able to build an image with this change and validate on your Azure Stack cluster?

@patrickdillon are you able to build an image with this change and validate on your Azure Stack cluster?

Oh yes, I was able to build and test the fix in an openshift install. This simple change does resolve the panic issue. That said, it's still unclear to me whether we would expect InstanceView to be nil or if there are more implications due to that.

I am fine moving forward, just wondering. I will look into adding unit tests today.

Added unit test in 7974cf4 and also confirmed that the unit test panics on master without the code fix.

/label tide/merge-method-squash

Which apiversion is supported in azure stack?

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

the fix is already verified here: kubernetes-sigs/azuredisk-csi-driver#3003, let's merge this fix

thanks, I will take care of the proceeding fix when the PR is merged in cloud-provider-azure

Which apiversion is supported in azure stack?

I was hoping someone here could help clarify that, because I find the topic confusing. According to these docs the profiles released in September and March have the greatest azurestack compatibility.

Looking at the most recent profile, 2020-09-01, that profile is using the V2 SDK at 2020-09-01 (as opposed to calling out to the v1 services packages as other APIs may do):

https://github.com/Azure/azure-sdk-for-go/blob/2187aca40bbf6931ffac666977e5d29128d82585/profiles/2020-09-01/compute/mgmt/compute/computeapi/interfaces.go#L11

So my understanding is that 2020-09-01 is the latest support API version for compute, although we've had better results using 2020-06-01.

Neither would meet the stated API version requirement of 2020-12-01.

/retest

@MartinForReal: new pull request created: #8866

@MartinForReal: #8755 failed to apply on top of branch "release-1.31":

Applying: Fix panic for GetZoneByNodeName on Azure Stack
Using index info to reconstruct a base tree...
M	pkg/provider/azure_standard.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/provider/azure_standard.go
CONFLICT (content): Merge conflict in pkg/provider/azure_standard.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix panic for GetZoneByNodeName on Azure Stack

@MartinForReal: #8755 failed to apply on top of branch "release-1.30":

Applying: Fix panic for GetZoneByNodeName on Azure Stack
Using index info to reconstruct a base tree...
M	pkg/provider/azure_standard.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/provider/azure_standard.go
CONFLICT (content): Merge conflict in pkg/provider/azure_standard.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix panic for GetZoneByNodeName on Azure Stack

@MartinForReal: #8755 failed to apply on top of branch "release-1.29":

Applying: Fix panic for GetZoneByNodeName on Azure Stack
Using index info to reconstruct a base tree...
M	pkg/provider/azure_standard.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/provider/azure_standard.go
CONFLICT (content): Merge conflict in pkg/provider/azure_standard.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix panic for GetZoneByNodeName on Azure Stack