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.

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

Bugfix/azure sdk updates #261

Merged
merged 6 commits into from
Jun 13, 2019
Merged

Bugfix/azure sdk updates #261

merged 6 commits into from
Jun 13, 2019

Conversation

prashanth26
Copy link
Contributor

@prashanth26 prashanth26 commented May 7, 2019 •
edited
Loading

What this PR does / why we need it:

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

Special notes for your reviewer:

Release note:

Update Azure SDK from 12.5-beta to 26.1

Ensure 'x-ms-request-id' header is logged in case of Azure API failures.

Orphan VMs/disks/NICs handler has been fixed

Change credits: @chgeuer

@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) do-not-merge/work-in-progress labels May 7, 2019
@prashanth26 prashanth26 requested review from ggaurav10 and a team as code owners May 7, 2019 06:14
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 7, 2019
@PadmaB PadmaB added this to the 1905a milestone May 13, 2019
chgeuer and others added 4 commits May 14, 2019 15:37
@chgeuer @prashanth26
Adapted Azure SDK calls to 26.1
292ae63 
- Concurrent resource deletion (NIC / disk) where possible
- Ensure 'x-ms-request-id' header is logged in case of API failures.
@chgeuer @prashanth26
Updated Azure SDK from 12.5-beta to 26.1
b768a1b
@prashanth26
Azure detaches data disk before VM deletion
acb7357 
- This is a merge conflict resolution
- Poweroff is no longer done for Azure VMs before deletion
- Improved log messages
@prashanth26
Removed unwanted code checks and method
8e5e256
@prashanth26 prashanth26 force-pushed the bugfix/azure-sdk-updates branch from 8c97a3b to 3254a35 Compare May 14, 2019 10:11
@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 14, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 14, 2019
@prashanth26 prashanth26 force-pushed the bugfix/azure-sdk-updates branch from 3254a35 to c5dd7ea Compare May 16, 2019 08:37
@prashanth26 prashanth26 mentioned this pull request May 16, 2019
Closed
@prashanth26 prashanth26 force-pushed the bugfix/azure-sdk-updates branch from c2221d6 to 69989f4 Compare May 16, 2019 09:56
@prashanth26
Copy link
Contributor Author

Hi @chgeuer ,

I ran into a bug while testing where the following line ran into a NPE -

machine-controller-manager/pkg/driver/driver_azure.go

Line 755 in c5dd7ea

glog.Errorf("Azure ARM API call with x-ms-request-id=%s failed. %s: %s\n", requestID, message, *detailedErr)
.

From what I understand, the response could be nil in case of errors like - context has been cancelled etc. Refer -

machine-controller-manager/vendor/github.com/Azure/go-autorest/autorest/azure/async.go

Line 351 in c5dd7ea

// returns the cached HTTP response after a call to pollForStatus(), can be nil
,

I have made these changes to fix them, please provide your feedback on them as well.
69989f4

Regards,
Prashanth

@prashanth26 prashanth26 added area/operations area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/bug Bug kind/enhancement Enhancement, improvement, extension needs/review Needs review and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 16, 2019
@prashanth26 prashanth26 added priority/critical Needs to be resolved soon, because it impacts users negatively reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/in-delivery Issue is in delivery/deployment topology/seed Affects Seed clusters labels May 16, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 16, 2019
@chgeuer
Copy link
Contributor

chgeuer commented May 20, 2019

Sorry, was OOF last week, and pretty unresponsive… Looks good to me. Is there anything I need to do?

@prashanth26
Copy link
Contributor Author

prashanth26 commented May 21, 2019 •
edited
Loading

Not an issue.

No just wanted to make sure that you have a look at it :) I shall do one last round of tests, and merge it soon.

Nothing else is needed from your side. Thank for you the PR.

Regards,
Prashanth

hardikdr
hardikdr approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I ran the integration test locally for azure and seems to be passing.

amshuman-kr
amshuman-kr approved these changes Jun 12, 2019
View reviewed changes
Copy link

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

LGTM

@hardikdr hardikdr merged commit 2c75673 into gardener:master Jun 13, 2019
@prashanth26 prashanth26 deleted the bugfix/azure-sdk-updates branch July 17, 2019 08:00
@ghost ghost added component/mcm Machine Controller Manager (including Node Problem Detector, Cluster Auto Scaler, etc.) platform/azure Microsoft Azure platform/infrastructure labels Mar 7, 2020
@gardener-robot gardener-robot added the area/ops-productivity Operator productivity related (how to improve operations) label Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amshuman-kr amshuman-kr amshuman-kr approved these changes

@hardikdr hardikdr hardikdr approved these changes

@ggaurav10 ggaurav10 Awaiting requested review from ggaurav10

Assignees
No one assigned
Labels
area/ops-productivity Operator productivity related (how to improve operations) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related component/mcm Machine Controller Manager (including Node Problem Detector, Cluster Auto Scaler, etc.) do-not-merge/work-in-progress kind/bug Bug kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review platform/azure Microsoft Azure platform/infrastructure priority/critical Needs to be resolved soon, because it impacts users negatively size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/in-delivery Issue is in delivery/deployment topology/seed Affects Seed clusters
Projects
None yet
Milestone
1905a
Development

Successfully merging this pull request may close these issues.

Orphan system disks were found on Azure
7 participants
@prashanth26 @chgeuer @hardikdr @amshuman-kr @PadmaB @gardener-robot-ci-1 @gardener-robot