[release-0.12] Fix panic when OpenStack server is deleted by an external agent #2477
Conversation
The panic happened because we did not correctly handle that GetInstanceStatus returns a nil server if the server does not exist, rather than a 404 error. We had the same oversight in the OpenStackServer controller. It looks like this would have resulted in recreating the server.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| @@ -365,6 +365,11 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| result := r.reconcileMachineState(scope, openStackMachine, machine, machineServer) | |||
There was a problem hiding this comment.
given we set the openStackMachine.Status.Addresses at line 405, shouldn't we keep the reconcileMachineState after that?
If we don't do that I think we might end up returning the result without the addresses reconciled.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@EmilienM @MaysaMacedo How do you want to handle this? We can either merge this and then backport #2478, or create a squashed custom backport and merge them both at once. |
|
we can merge this and backport your fix |
|
/lgtm |
k8s-ci-robot
added
lgtm
This is an automated cherry-pick of #2475
/assign EmilienM