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

Fix Azure machine deletion if resource group is gone #566

Merged

Conversation

dkistner
Copy link
Member

What this PR does / why we need it:
Allow MCM to delete Azure machines even if the underlying resource groups are already deleted. E. g. user delete the resource group manually.

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

Special notes for your reviewer:

Release note:

MCM will delete Azure machines even if the underlying resource group is already deleted.

/invite @kon-angelo @ialidzhikov

@dkistner dkistner requested review from ggaurav10 and a team as code owners November 23, 2020 10:05
@gardener-robot gardener-robot added the needs/review Needs review label Nov 23, 2020
@gardener-robot gardener-robot added the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Nov 23, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 23, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 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 Nov 23, 2020
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Nov 23, 2020
@prashanth26
Copy link
Contributor

cc: @AxiomSamarth
TODO: incorporate in OOT branch.

Copy link
Contributor

@prashanth26 prashanth26 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 this fix @dkistner .
/lgtm

Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

Can you add an unit test about it to make sure that we don't regress it in the future?

@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed reviewed/lgtm Has approval for merging labels Nov 23, 2020
@dkistner
Copy link
Member Author

TODO: incorporate in OOT branch.

gardener/machine-controller-manager-provider-azure#6 (currently private)

Can you add an unit test about it to make sure that we don't regress it in the future?

Hmm. We don't have much unit tests yet for the Azure driver (primarily for the data disks feature) and implementing this would mean to mock a lot of Azure api calls. Don't get me wrong I wanted to have that tests, but I guess it would make more sense (from an effort point of view) to do it in the out-of-tree, where we anyways then need to increase the test coverage.

@prashanth26
Copy link
Contributor

Hmm. We don't have much unit tests yet for the Azure driver (primarily for the data disks feature) and implementing this would mean to mock a lot of Azure api calls. Don't get me wrong I wanted to have that tests, but I guess it would make more sense (from an effort point of view) to do it in the out-of-tree, where we anyways then need to increase the test coverage.

I would second this. With the OOT @AxiomSamarth is already building a framework for mocking and writing tests for azure MCM OOT provider. Should be easier to get it done there.

@prashanth26
Copy link
Contributor

Can we merge this change for now? @ialidzhikov

@ialidzhikov
Copy link
Member

Can we merge this change for now? @ialidzhikov

Sure, please go on. It was just a remark from outside that we miss unit tests for such small things ;).

@prashanth26
Copy link
Contributor

prashanth26 commented Nov 25, 2020

Currently, the core MCM logic has a good framework for unit tests and we submit PRs with tests only. However, with drivers, we haven't invested here as we are moving all this logic into OOT.

@ialidzhikov
Copy link
Member

Sure, thanks. I got it.

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging needs/changes Needs (more) changes and removed needs/changes Needs (more) changes labels Nov 25, 2020
@kon-angelo
Copy link
Contributor

Partially the problem lies with client libraries and what they expose to make unit tests. On AWS you get an interface that you can mock, but for others (azure, openstack) you mostly get structs and functions exposed and that means that there is quite a bit of manual labor involved to have proper unit tests. Not saying we shouldn't do it though.

@prashanth26 prashanth26 merged commit b3673ea into gardener:master Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

machine-controller-manager cannot delete Machine when the resource group is already deleted
8 participants