Skip to content

🐛 Don't try to resolve machine on delete if cluster not ready #2006

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 11, 2024
Merged

🐛 Don't try to resolve machine on delete if cluster not ready #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions controllers/openstackcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,18 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac
}
}

if instanceStatus != nil {
// If no instance was created we currently need to check for orphaned
// volumes. This requires resolving the instance spec.
// TODO: write volumes to status resources on creation so this is no longer required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we? You can still orphan a volume by crashing after a Cinder call, but before saving the ID into the status. Then on next reconcile you can discover the Machine is being deleted, so no reconcile will "pick-up" the created volume. This has super low probability, so we've hit this in Kuryr all the time. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bastion flow here is not as robust as the machine flow. However, this was the case before and I haven't tried as hard to fix it. The reason is that the bastion delete flow is extremely janky and over-complicated, and what I really want to do is re-design it.

As long as I didn't regress it here I'm ok with not fixing all the edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incidentally, notice that previously because we only ran any of this when instanceStatus != nil we would always leak volume in this case. With this change we no longer leak volumes. Updating volumes to use the same pattern as ports will make this simpler, but that's for another time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just commenting on a comment here, I don't think we'll be able to get rid of the DeleteVolumes() even after volumes are saved into the ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... kinda. I eventually want to move them into resources, and at that point they'll go into the Adopt() flow.

if instanceStatus == nil {
instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster)
if err != nil {
return err
}
if err := computeService.DeleteVolumes(instanceSpec); err != nil {
return fmt.Errorf("delete volumes: %w", err)
}
} else {
instanceNS, err := instanceStatus.NetworkStatus()
if err != nil {
return err
Expand All @@ -307,11 +318,7 @@ func deleteBastion(scope *scope.WithLogger, cluster *clusterv1.Cluster, openStac
}
}

instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster)
if err != nil {
return err
}
if err = computeService.DeleteInstance(openStackCluster, instanceStatus, instanceSpec); err != nil {
if err = computeService.DeleteInstance(openStackCluster, instanceStatus); err != nil {
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete bastion: %w", err))
return fmt.Errorf("failed to delete bastion: %w", err)
}
Expand Down
5 changes: 4 additions & 1 deletion controllers/openstackcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ var _ = Describe("OpenStackCluster controller", func() {
testCluster.Status = infrav1.OpenStackClusterStatus{
Bastion: &infrav1.BastionStatus{
ID: "bastion-uuid",
Resolved: &infrav1.ResolvedMachineSpec{
ImageID: "imageID",
},
},
}
err = k8sClient.Status().Update(ctx, testCluster)
Expand All @@ -221,8 +224,8 @@ var _ = Describe("OpenStackCluster controller", func() {
computeClientRecorder.GetServer("bastion-uuid").Return(nil, gophercloud.ErrResourceNotFound{})

err = deleteBastion(scope, capiCluster, testCluster)
Expect(testCluster.Status.Bastion).To(BeNil())
Expect(err).To(BeNil())
Expect(testCluster.Status.Bastion).To(BeNil())
})
It("should adopt an existing bastion even if its uuid is not stored in status", func() {
testCluster.SetName("adopt-existing-bastion")
Expand Down
209 changes: 141 additions & 68 deletions controllers/openstackmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,26 +169,21 @@ func resolveMachineResources(scope *scope.WithLogger, clusterResourceName string
openStackMachine.Status.Resolved = resolved
}
// Resolve and store resources
changed, err := compute.ResolveMachineSpec(scope,
return compute.ResolveMachineSpec(scope,
&openStackMachine.Spec, resolved,
clusterResourceName, openStackMachine.Name,
openStackCluster, getManagedSecurityGroup(openStackCluster, machine))
if err != nil {
return false, err
}
if changed {
// If the resolved machine spec changed we need to start the reconcile again to prevent inconsistency between reconciles.
return true, nil
}
}

func adoptMachineResources(scope *scope.WithLogger, openStackMachine *infrav1.OpenStackMachine) error {
resources := openStackMachine.Status.Resources
if resources == nil {
resources = &infrav1.MachineResources{}
openStackMachine.Status.Resources = resources
}

// Adopt any existing resources
return false, compute.AdoptMachineResources(scope, resolved, resources)
return compute.AdoptMachineResources(scope, openStackMachine.Status.Resolved, resources)
}

func patchMachine(ctx context.Context, patchHelper *patch.Helper, openStackMachine *infrav1.OpenStackMachine, machine *clusterv1.Machine, options ...patch.Option) error {
Expand Down Expand Up @@ -256,66 +251,72 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl
return ctrl.Result{}, err
}

// We may have resources to adopt if the cluster is ready
if openStackCluster.Status.Ready && openStackCluster.Status.Network != nil {
if _, err := resolveMachineResources(scope, clusterResourceName, openStackCluster, openStackMachine, machine); err != nil {
return ctrl.Result{}, err
}
// Nothing to do if the cluster is not ready because no machine resources were created.
if !openStackCluster.Status.Ready || openStackCluster.Status.Network == nil {
// The finalizer should not have been added yet in this case,
// but the following handles the upgrade case.
controllerutil.RemoveFinalizer(openStackMachine, infrav1.MachineFinalizer)
return ctrl.Result{}, nil
}

if openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() {
loadBalancerService, err := loadbalancer.NewService(scope)
if err != nil {
return ctrl.Result{}, err
}
// For machines created after v0.10, or any machine which has been
// reconciled at least once by v0.10 or later, status.Resolved always
// exists before any resources are created. We can therefore assume
// that if it does not exist, no resources were created.
//
// There is an upgrade edge case where a machine may have been marked
// deleted before upgrade but we are completing it after upgrade. For
// this use case only we make a best effort to resolve resources before
// continuing, but if we get an error we log it and continue anyway.
// This has the potential to leak resources, but only in this specific
// edge case. The alternative is to continue retrying until it succeeds,
// but that risks never deleting a machine which cannot be resolved due
// to a spec error.
//
// This code can and should be deleted in a future release when we are
// sure that all machines have been reconciled at least by a v0.10 or
// later controller.
if _, err := resolveMachineResources(scope, clusterResourceName, openStackCluster, openStackMachine, machine); err != nil {
// Return the error, but allow the resource to be removed anyway.
controllerutil.RemoveFinalizer(openStackMachine, infrav1.MachineFinalizer)
return ctrl.Result{}, err
}

err = loadBalancerService.DeleteLoadBalancerMember(openStackCluster, machine, openStackMachine, clusterResourceName)
if err != nil {
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.LoadBalancerMemberErrorReason, clusterv1.ConditionSeverityWarning, "Machine could not be removed from load balancer: %v", err)
return ctrl.Result{}, err
}
// Check for any orphaned resources
// N.B. Unlike resolveMachineResources, we must always look for orphaned resources in the delete path.
if err := adoptMachineResources(scope, openStackMachine); err != nil {
return ctrl.Result{}, fmt.Errorf("adopting machine resources: %w", err)
}

var instanceStatus *compute.InstanceStatus
if openStackMachine.Status.InstanceID != nil {
instanceStatus, err = computeService.GetInstanceStatus(*openStackMachine.Status.InstanceID)
if err != nil {
return ctrl.Result{}, err
}
} else if instanceStatus, err = computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name); err != nil {
instanceStatus, err := getInstanceStatus(openStackMachine, computeService)
if err != nil {
return ctrl.Result{}, err
}
if !openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() && util.IsControlPlaneMachine(machine) && openStackCluster.Spec.APIServerFloatingIP == nil {
if instanceStatus != nil {
instanceNS, err := instanceStatus.NetworkStatus()
if err != nil {
openStackMachine.SetFailure(
capierrors.UpdateMachineError,
fmt.Errorf("get network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err),
)
return ctrl.Result{}, nil
}

addresses := instanceNS.Addresses()
for _, address := range addresses {
if address.Type == corev1.NodeExternalIP {
if err = networkingService.DeleteFloatingIP(openStackMachine, address.Address); err != nil {
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Deleting floating IP failed: %v", err)
return ctrl.Result{}, fmt.Errorf("delete floating IP %q: %w", address.Address, err)
}
}
}
if util.IsControlPlaneMachine(machine) {
if err := removeAPIServerEndpoint(scope, openStackCluster, openStackMachine, instanceStatus, clusterResourceName); err != nil {
return ctrl.Result{}, err
}
}

instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
if err != nil {
return ctrl.Result{}, err
// If no instance was created we currently need to check for orphaned
// volumes. This requires resolving the instance spec.
// TODO: write volumes to status resources on creation so this is no longer required.
if instanceStatus == nil && openStackMachine.Status.Resolved != nil {
instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
if err != nil {
return ctrl.Result{}, err
}
if err := computeService.DeleteVolumes(instanceSpec); err != nil {
return ctrl.Result{}, fmt.Errorf("delete volumes: %w", err)
}
}

if err := computeService.DeleteInstance(openStackMachine, instanceStatus, instanceSpec); err != nil {
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)
if instanceStatus != nil {
if err := computeService.DeleteInstance(openStackMachine, instanceStatus); err != nil {
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)
}
}

trunkSupported, err := networkingService.IsTrunkExtSupported()
Expand All @@ -341,6 +342,62 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope *scope.WithLogger, cl
return ctrl.Result{}, nil
}

func getInstanceStatus(openStackMachine *infrav1.OpenStackMachine, computeService *compute.Service) (*compute.InstanceStatus, error) {
if openStackMachine.Status.InstanceID != nil {
return computeService.GetInstanceStatus(*openStackMachine.Status.InstanceID)
}
return computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name)
}

func removeAPIServerEndpoint(scope *scope.WithLogger, openStackCluster *infrav1.OpenStackCluster, openStackMachine *infrav1.OpenStackMachine, instanceStatus *compute.InstanceStatus, clusterResourceName string) error {
if openStackCluster.Spec.APIServerLoadBalancer.IsEnabled() {
loadBalancerService, err := loadbalancer.NewService(scope)
if err != nil {
return err
}

err = loadBalancerService.DeleteLoadBalancerMember(openStackCluster, openStackMachine, clusterResourceName)
if err != nil {
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.LoadBalancerMemberErrorReason, clusterv1.ConditionSeverityWarning, "Machine could not be removed from load balancer: %v", err)
return err
}
return nil
}

// XXX(mdbooth): This looks wrong to me. Surely we should only ever
// disassociate the floating IP here. I would expect the API server
// floating IP to be created and deleted with the cluster. And if the
// delete behaviour is correct, we leak it if the instance was
// previously deleted.
Comment on lines +367 to +371
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an assumption that if APIServerFloatingIP was nil, then the FIP was created by CAPO [1], but the problem is that this doesn't seem to lookup that FIP, but all the FIPs of that instance. If someone attached additional FIP to an instance, e.g. for debugging purposes it will be deleted too. This is horribly wrong to me.

Also no need to detach FIP at all. If we delete the instance and port is deleted, FIP will be detached.

[1]

cluster-api-provider-openstack/api/v1alpha5/openstackcluster_types.go

Line 82 in d33c1bc

// If not specified, a new floatingIP is allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Notice that this is just code motion, though. I'm pretty sure it's broken in exactly the same way it was broken before, it's just slightly easier to see now.

If I haven't regressed this I don't want to fix it in this PR. I added the comment because I noticed it while I was moving it.

if openStackCluster.Spec.APIServerFloatingIP == nil && instanceStatus != nil {
instanceNS, err := instanceStatus.NetworkStatus()
if err != nil {
openStackMachine.SetFailure(
capierrors.UpdateMachineError,
fmt.Errorf("get network status for OpenStack instance %s with ID %s: %v", instanceStatus.Name(), instanceStatus.ID(), err),
)
return nil
}

networkingService, err := networking.NewService(scope)
if err != nil {
return err
}

addresses := instanceNS.Addresses()
for _, address := range addresses {
if address.Type == corev1.NodeExternalIP {
if err = networkingService.DeleteFloatingIP(openStackMachine, address.Address); err != nil {
conditions.MarkFalse(openStackMachine, infrav1.APIServerIngressReadyCondition, infrav1.FloatingIPErrorReason, clusterv1.ConditionSeverityError, "Deleting floating IP failed: %v", err)
return fmt.Errorf("delete floating IP %q: %w", address.Address, err)
}
}
}
}

return nil
}

// GetPortIDs returns a list of port IDs from a list of PortStatus.
func GetPortIDs(ports []infrav1.PortStatus) []string {
portIDs := make([]string, len(ports))
Expand Down Expand Up @@ -489,34 +546,50 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
return ctrl.Result{}, nil
}

// If the OpenStackMachine doesn't have our finalizer, add it.
if controllerutil.AddFinalizer(openStackMachine, infrav1.MachineFinalizer) {
// Register the finalizer immediately to avoid orphaning OpenStack resources on delete
return ctrl.Result{}, nil
}

if !openStackCluster.Status.Ready {
scope.Logger().Info("Cluster infrastructure is not ready yet, re-queuing machine")
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.WaitingForClusterInfrastructureReason, clusterv1.ConditionSeverityInfo, "")
return ctrl.Result{RequeueAfter: waitForClusterInfrastructureReadyDuration}, nil
}

if changed, err := resolveMachineResources(scope, clusterResourceName, openStackCluster, openStackMachine, machine); changed || err != nil {
scope.Logger().V(6).Info("Machine resources updated, requeuing")
return ctrl.Result{}, err
}

// Make sure bootstrap data is available and populated.
if machine.Spec.Bootstrap.DataSecretName == nil {
scope.Logger().Info("Bootstrap data secret reference is not yet available")
conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.WaitingForBootstrapDataReason, clusterv1.ConditionSeverityInfo, "")
return ctrl.Result{}, nil
}
userData, err := r.getBootstrapData(ctx, machine, openStackMachine)

changed, err := resolveMachineResources(scope, clusterResourceName, openStackCluster, openStackMachine, machine)
if err != nil {
return ctrl.Result{}, err
}

// Also add the finalizer when writing resolved resources so we can start creating resources on the next reconcile.
if controllerutil.AddFinalizer(openStackMachine, infrav1.MachineFinalizer) {
changed = true
}

// We requeue if we either added the finalizer or resolved machine
// resources. This means that we never create any resources unless we
// have observed that the finalizer and resolved machine resources were
// successfully written in a previous transaction. This in turn means
// that in the delete path we can be sure that if there are no resolved
// resources then no resources were created.
if changed {
scope.Logger().V(6).Info("Machine resources updated, requeuing")
return ctrl.Result{}, nil
}

Comment on lines +572 to +582
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is a nice pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was always the goal of this pattern! It's only the upgrade case which make it at all complicated, and at some point we should be able to delete most of the complications.

// Check for orphaned resources previously created but not written to the status
if err := adoptMachineResources(scope, openStackMachine); err != nil {
return ctrl.Result{}, fmt.Errorf("adopting machine resources: %w", err)
}

scope.Logger().Info("Reconciling Machine")
userData, err := r.getBootstrapData(ctx, machine, openStackMachine)
if err != nil {
return ctrl.Result{}, err
}
Comment on lines +589 to +592
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails we still don't create the machine, why move it down here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still needs to live in the 'create transaction'. There's no point putting it before setting the finalizer because it won't be used. We also don't expect this to fail at this point because we've already checked the bootstrap data is set, so if it does fail it's a presumably just a low-probability ephemeral failure that could happen anywhere.

Here I've just moved it to be local to where the data is actually used.


computeService, err := compute.NewService(scope)
if err != nil {
Expand Down
Loading