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

Azure driver: Fix issue #3976 #4249

Merged
merged 1 commit into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@m4r10k
Contributor

m4r10k commented Sep 8, 2017

This PR will fix #3976

Why

The current driver behavior does not delete the empty storage accounts. This will mess up your Microsoft Azure cloud resource group, like I wrote it as comment on the referenced issue. The current behavior will produce the following result if you, like we do, use for example Gitlab autoscaling runners.
screenshot

What I've done

I added a functionality which checks, if there are storage blobs left over in the associated storage account of the virtual machine, after the virtual machine is deleted. If there are no container items left in the storage account, this PR will delete the corresponding storage account.

How it is done

I modified the removeOSDiskBlob method and used the already existing client methods to retrieve a list of the containers associated with the storage account. If the retrieved list is empty, after the deletion of the virtual machine, the PR deletes the storage account too with the appropriate methods.

Result

No more dangling storage accounts.

@m4r10k m4r10k changed the title from Fix #3976 to Azure driver: Fix issue #3976 Sep 12, 2017

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 17, 2017

Contributor

@shin @StefanScherer @nathanleclaire any comments on this from the committers? Is something missing or unclear? Can I do something to get this commited? Thank you!

Contributor

m4r10k commented Sep 17, 2017

@shin @StefanScherer @nathanleclaire any comments on this from the committers? Is something missing or unclear? Can I do something to get this commited? Thank you!

@nathanleclaire

This comment has been minimized.

Show comment
Hide comment
@nathanleclaire

nathanleclaire Sep 18, 2017

Contributor

Thanks @kleinsasserm -- I am not maintainer anymore, but you want to actually ping @shin-

Contributor

nathanleclaire commented Sep 18, 2017

Thanks @kleinsasserm -- I am not maintainer anymore, but you want to actually ping @shin-

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 18, 2017

Contributor

Thanks to you @nathanleclaire for your response too!

Contributor

m4r10k commented Sep 18, 2017

Thanks to you @nathanleclaire for your response too!

@shin-

Thank you for your submission. Looks good overall but I suggested a few changes to make the code more robust and improve logging.

Show outdated Hide outdated drivers/azure/azureutil/azureutil.go
Show outdated Hide outdated drivers/azure/azureutil/azureutil.go

@shin- shin- added this to the 0.13.0 milestone Oct 2, 2017

Fix #3976
Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>

Change storage account deletion logic to fix #3976

Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>
@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Oct 9, 2017

Contributor

@shin- Hi Shin! I've updated the PR with your suggestions. Hopefully I've done the update of the PR with the correct git rebase and push commands.

Contributor

m4r10k commented Oct 9, 2017

@shin- Hi Shin! I've updated the PR with your suggestions. Hopefully I've done the update of the PR with the correct git rebase and push commands.

@shin-

shin- approved these changes Oct 11, 2017

@shin-

This comment has been minimized.

Show comment
Hide comment
@shin-

shin- Oct 11, 2017

Member

LGTM, thank you! :)

Member

shin- commented Oct 11, 2017

LGTM, thank you! :)

@shin- shin- merged commit f3af489 into docker:master Oct 11, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dco-signed All commits are signed
@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Oct 11, 2017

Contributor

Thank you too! :)

Contributor

m4r10k commented Oct 11, 2017

Thank you too! :)

@m4r10k m4r10k deleted the m4r10k:3976-fix-azure-rm-storage-accounts branch Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment