-
Notifications
You must be signed in to change notification settings - Fork 62
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
Issues/delete service instances #3484
Issues/delete service instances #3484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I left some minor remarks.
Also, could you squash the commits in order not to have the wip one and amend the commit description with the issue number, e.g.
fixes #3292
controllers/controllers/services/instances/managed/controller.go
Outdated
Show resolved
Hide resolved
b622a10
to
9428bae
Compare
9428bae
to
b5c5781
Compare
|
@@ -129,6 +117,18 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, serviceInstance *kor | |||
return ctrl.Result{}, fmt.Errorf("failed to create client for broker %q: %w", serviceBroker.Name, err) | |||
} | |||
|
|||
if !serviceInstance.GetDeletionTimestamp().IsZero() { | |||
return r.finalizeCFServiceInstance(ctx, osbapiClient, serviceInstance, serviceOffering, servicePlan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we still need to move the finalization bit before getting getting plan/offering/etc
Our CI clusters could not be deleted as there are a couple of CFServiceInstances with finalizers that cannot be deleted. They have their deletion timestamp set but finalization fails with
message: 'failed to get service plan "1ac1edee-804e-51bd-a4c0-149e8271f275": CFServicePlan.korifi.cloudfoundry.org "1ac1edee-804e-51bd-a4c0-149e8271f275" not found'
Indeed, there are no service plans on the cluster.
I do not know how the cluster reached this situation, but in any case, fainalizing resources should be "best effort" - they should try to delete whatever possible, but if an error occurs, still allow deletion. Having said that, I think we should move finalization at the top of the Reconcile
method, just after setting the observed generation and have plan and offering being looked up in the finalize method. If an error occurrs while fetching the plan/offering occurs, it should be only logged AND the finalizer should be removed
Head branch was pushed to by a user without write access
log := logr.FromContextOrDiscard(ctx).WithName("finalizeCFServiceInstance") | ||
|
||
if !controllerutil.ContainsFinalizer(cfServiceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { | ||
if !controllerutil.ContainsFinalizer(serviceInstance, korifiv1alpha1.CFManagedServiceInstanceFinalizerName) { | ||
return ctrl.Result{}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !containsFinalizer() {
return ctrl.Result{}, nil
}
if deprovisionRequested() {
if removeFinalizer(){
log.finalizerRemoved()
}
return ctrl.Result{}, nil
}
err := deprovisionInstance() // only returns error if osbapiClient.Deprovision fails; also, do NOT wait for the deprovision operation to complete
iferr {
return ctrl.Result{}, err
}
return ctrl.Result{Requeue: true}, nil
73b8cca
to
d4b51d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left some minor comments, looks all right otherwise
controllers/controllers/services/instances/managed/controller.go
Outdated
Show resolved
Hide resolved
controllers/controllers/services/instances/managed/controller_test.go
Outdated
Show resolved
Hide resolved
controllers/controllers/services/instances/managed/controller_test.go
Outdated
Show resolved
Hide resolved
}).Should(Succeed()) | ||
}) | ||
|
||
It("does not delete the service instance", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test work at all? If the broker accepts the deprovision request, then the service instance should be deleted (see the It
above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test failed only once when I ran the tests intil they fail, which is strange for me too..
359f9e7
to
6005840
Compare
/easycla |
6005840
to
62d7501
Compare
/easycla |
Is there a related GitHub Issue?
#3292
What is this change about?
Implenting functionality about managed service instacnes
Does this PR introduce a breaking change?
No