Always ensure to have machine-delete-finalizer

[phiphi282]

What this PR does / why we need it:
After creating a new provider instance we always want to have the
machine-delete-finalizer. Possible provider specific finalizers can't be
deleted without it.

We have noted this especially for azure clusters, where the machine-controller doesn't try to run deleteCloudProvider instance because the finalizer machine-delete-finalizer is missing.

Which issue(s) this PR fixes (optional, in fixes #<issue number> format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Optional Release Note:

NONE

[kubermatic-bot]

Hi @phiphi282. Thanks for your PR.

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[phiphi282]

/assign @xrstf

[xrstf]

/ok-to-test
/test all

[phiphi282]

Is there any progress on this? :)

[irozzo-1A]

Hey @phiphi282. Let me see if I got your problem right. With some providers (at least Azure) the instance is created requiring clean-up and an error returned at the same time by Create method. Is it correct?

[phiphi282]

The problem is with the deletion of the machine-delete-finalizer.

If the provider instance is created but the function returns an error, the machine-delete-finalizer will be deleted.

The cleanupCloudProviderInstance function is only trying to delete the cloud-provider instance when having this finalizer though. At least for azure this keeps the instances from getting deleted because there are azure specific finalizers that don't get cleaned up as well.

[irozzo-1A]

@phiphi282 Ok, I think I got your point right. Having an error returned by provider.Create() method does not necessarily mean that the instance clean-up logic should not be run. I will dig a bit to understand when and why this check was added.

Do you have an example at hand of error that is returned with Azure provider.Create() where instance clean-up is needed?

[phiphi282]

I kind of forgot what we did exactly unfortunately, but we created and deleted some instances relatively fast. I think this caused the machine to be created on the azure side but without completely finishing the creation.

The machine already got all azure related finalizers attached to it already which kept the machine from being deleted.
cc @eduardostalinho Maybe you can describe a bit better what to do to create the error.

[eduardostalinho]

I'm afraid that I don't have the logs here anymore.
But I remember we tried to deete a cluster that wasn't created properly (or in time) because of prov.Create and the deletion hanged because of this finalizer missing.

[irozzo-1A]

I think that this PR risks to have side effects. The fact that cluster deletion hanged as mentioned bu @eduardostalinho is probably because the instance was created despite the error returned by Create method, and this was preventing the deletion of subnet or some other resources.

The problem I see is that when Create fails and no instance is actually created, the Cleanup method could fail:

machine-controller/pkg/controller/machine/machine.go

Line 557 in aec77a5

completelyGone, err := prov.Cleanup(machine, r.providerData)

causing the machine deletion to hang.

[irozzo-1A]

@PhillipAmend @eduardostalinho What I propose is to keep this on hold at the moment, wait for a re-occurrence of the issues to have a better understanding of what happened. Is it ok for you?

[phiphi282]

@irozzo-1A From what I could see in the provider code all Delete function check the existence of the Instance on the provider side first.

e.g aws:

instance, err := p.get(machine)
if err != nil {
	if err == cloudprovidererrors.ErrInstanceNotFound {
		return true, nil
	}
	return false, err
}

With this the machine deletion should not be hanging because of a failed delete. If we don't try to delete the machine at all however (because of the missing finalizer) the machine deletion will definitely hang when there are still other finalizers on the machine.

I would be okay for me to wait a bit but we probably won't have re-occurrence of the issue from our side, since we are already using this fix (and from what we can tell it works fine on openstack, aws and azure at least).

[irozzo-1A]

Hi @phiphi282, I agree that if all Cleaup methods for all providers can run safely even when Create did not complete successfully this PR should not have side effects. Knowing that this is already tested on AWS, Openstack and Azure is also reassuring.

I will take another look beginning of next week.

[irozzo-1A]

/lgtm

[kubermatic-bot]

LGTM label has been added.

Git tree hash: 16b55425d25d9741145de0adea18c534c243119e

[irozzo-1A]

/approve

[kubermatic-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irozzo-1A, phiphi282

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [irozzo-1A]

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

[irozzo-1A]

Thx for this contribution @phiphi282. I double-checked and I think you were right, this should not have side-effects. If we will observer regressions for some providers won't be critical.