Skip to content

Commit

Permalink
fix(controller): Move v4 client instantiation to reconcileDelete
Browse files Browse the repository at this point in the history 
In order to reduce the number of calls made to /prism_central to
establish whether a v4 client can be created, we move the v4 client
creation to reconcileDelete function as currently that's the only
place where we need to make v4 VG detach calls.
  • Loading branch information
@thunderboltsid
thunderboltsid committed Jul 30, 2024
1 parent 043bcb8 commit dae5e54
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 100 deletions.
16 changes: 0 additions & 16 deletions controllers/nutanixcluster_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,22 +201,6 @@ func (r *NutanixClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque
NutanixCluster: cluster,
NutanixClient: v3Client,
}

createV4Client, err := isPrismCentralV4Compatible(ctx, v3Client)
if err != nil {
log.Error(err, "error occurred while checking environment compatibility for Prism Central v4 APIs")
}

if createV4Client {
v4Client, err := getPrismCentralV4ClientForCluster(ctx, cluster, r.SecretInformer, r.ConfigMapInformer)
if err != nil {
log.Error(err, "error occurred while fetching Prism Central v4 client")
return reconcile.Result{}, err
}

rctx.NutanixClientV4 = v4Client
}

// Check for request action
if !cluster.DeletionTimestamp.IsZero() {
// NutanixCluster is being deleted
Expand Down
166 changes: 89 additions & 77 deletions controllers/nutanixmachine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,22 +275,6 @@ func (r *NutanixMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reque
NutanixClient: v3Client,
}

createV4Client, err := isPrismCentralV4Compatible(ctx, v3Client)
if err != nil {
log.Error(err, "error occurred while checking compatibility for Prism Central v4 APIs")
}

if createV4Client {
log.Info("Creating Prism Central v4 client for cluster", "cluster", ntxCluster.Name)
v4Client, err := getPrismCentralV4ClientForCluster(ctx, ntxCluster, r.SecretInformer, r.ConfigMapInformer)
if err != nil {
log.Error(err, "error occurred while fetching Prism Central v4 client")
return reconcile.Result{}, err
}

rctx.NutanixClientV4 = v4Client
}

defer func() {
if err == nil {
// Always attempt to Patch the NutanixMachine object and its status after each reconciliation.
Expand Down Expand Up @@ -327,81 +311,109 @@ func (r *NutanixMachineReconciler) reconcileDelete(rctx *nctx.MachineContext) (r
log.Error(errorMsg, "failed to delete VM")
return reconcile.Result{}, errorMsg
}

Check warning on line 314 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L314 

Added line #L314 was not covered by tests
// Check if VMUUID is absent
if vmUUID == "" {
log.Info(fmt.Sprintf("VMUUID was not found in spec for VM %s. Skipping delete", vmName))
} else {
// Search for VM by UUID
vm, err := FindVMByUUID(ctx, v3Client, vmUUID)
// Error while finding VM
log.Info(fmt.Sprintf("Removing finalizers for VM %s during delete reconciliation", vmName))
ctrlutil.RemoveFinalizer(rctx.NutanixMachine, infrav1.NutanixMachineFinalizer)
return reconcile.Result{}, nil
}

// Search for VM by UUID
vm, err := FindVMByUUID(ctx, v3Client, vmUUID)
// Error while finding VM
if err != nil {
errorMsg := fmt.Errorf("error finding vm %s with uuid %s: %v", vmName, vmUUID, err)
log.Error(errorMsg, "error finding vm")

Check warning on line 328 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L318-L328 

Added lines #L318 - L328 were not covered by tests
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
return reconcile.Result{}, errorMsg
}

Check warning on line 332 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L331-L332 

Added lines #L331 - L332 were not covered by tests
// Vm not found
if vm == nil {
log.Info(fmt.Sprintf("no vm found with UUID %s ... Already deleted? Skipping delete", vmUUID))

Check warning on line 335 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L335 

Added line #L335 was not covered by tests
log.Info(fmt.Sprintf("Removing finalizers for VM %s during delete reconciliation", vmName))
ctrlutil.RemoveFinalizer(rctx.NutanixMachine, infrav1.NutanixMachineFinalizer)
return reconcile.Result{}, nil
}

// Check if the VM name matches the Machine name or the NutanixMachine name.

Check warning on line 341 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L337-L341 

Added lines #L337 - L341 were not covered by tests
// Earlier, we were creating VMs with the same name as the NutanixMachine name.
// Now, we create VMs with the same name as the Machine name in line with other CAPI providers.
// This check is to ensure that we are deleting the correct VM for both cases as older CAPX VMs
// will have the NutanixMachine name as the VM name.

Check warning on line 345 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L344-L345 

Added lines #L344 - L345 were not covered by tests
if *vm.Spec.Name != vmName && *vm.Spec.Name != rctx.NutanixMachine.Name {
return reconcile.Result{}, fmt.Errorf("found VM with UUID %s but name %s did not match Machine name %s or NutanixMachineName %s", vmUUID, *vm.Spec.Name, vmName, rctx.NutanixMachine.Name)
}

log.V(1).Info(fmt.Sprintf("VM %s with UUID %s was found.", *vm.Spec.Name, vmUUID))
lastTaskUUID, err := GetTaskUUIDFromVM(vm)
if err != nil {
errorMsg := fmt.Errorf("error occurred fetching task UUID from vm: %v", err)

Check warning on line 353 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L352-L353 

Added lines #L352 - L353 were not covered by tests
log.Error(errorMsg, "error fetching task UUID")
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
return reconcile.Result{}, errorMsg
}

if lastTaskUUID != "" {
log.Info(fmt.Sprintf("checking if VM %s with UUID %s has in progress tasks", vmName, vmUUID))
taskInProgress, err := HasTaskInProgress(ctx, rctx.NutanixClient, lastTaskUUID)

Check warning on line 361 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L355-L361 

Added lines #L355 - L361 were not covered by tests
if err != nil {
errorMsg := fmt.Errorf("error finding vm %s with uuid %s: %v", vmName, vmUUID, err)
log.Error(errorMsg, "error finding vm")
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
return reconcile.Result{}, errorMsg
log.Error(err, fmt.Sprintf("error occurred while checking task %s for VM %s. Trying to delete VM", lastTaskUUID, vmName))

Check warning on line 363 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L363 

Added line #L363 was not covered by tests
}
// Vm not found
if vm == nil {
log.V(1).Info(fmt.Sprintf("no vm found with UUID %s ... Already deleted? Skipping delete", vmUUID))
} else {
// Check if the VM name matches the Machine name or the NutanixMachine name.
// Earlier, we were creating VMs with the same name as the NutanixMachine name.
// Now, we create VMs with the same name as the Machine name in line with other CAPI providers.
// This check is to ensure that we are deleting the correct VM for both cases as older CAPX VMs
// will have the NutanixMachine name as the VM name.
if *vm.Spec.Name != vmName && *vm.Spec.Name != rctx.NutanixMachine.Name {
return reconcile.Result{}, fmt.Errorf("found VM with UUID %s but name %s did not match Machine name %s or NutanixMachineName %s", vmUUID, *vm.Spec.Name, vmName, rctx.NutanixMachine.Name)
}
log.V(1).Info(fmt.Sprintf("VM %s with UUID %s was found.", *vm.Spec.Name, vmUUID))
lastTaskUUID, err := GetTaskUUIDFromVM(vm)
if err != nil {
errorMsg := fmt.Errorf("error occurred fetching task UUID from vm: %v", err)
log.Error(errorMsg, "error fetching task UUID")
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
return reconcile.Result{}, errorMsg
}
if lastTaskUUID != "" {
log.Info(fmt.Sprintf("checking if VM %s with UUID %s has in progress tasks", vmName, vmUUID))
taskInProgress, err := HasTaskInProgress(ctx, rctx.NutanixClient, lastTaskUUID)
if err != nil {
log.Error(err, fmt.Sprintf("error occurred while checking task %s for VM %s. Trying to delete VM", lastTaskUUID, vmName))
}
if taskInProgress {
log.Info(fmt.Sprintf("VM %s task with UUID %s still in progress. Requeuing", vmName, vmUUID))
return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
}
log.V(1).Info(fmt.Sprintf("No running tasks anymore... Initiating delete for vm %s with UUID %s", vmName, vmUUID))
} else {
log.V(1).Info(fmt.Sprintf("no task UUID found on VM %s. Starting delete.", *vm.Spec.Name))
}
if taskInProgress {
log.Info(fmt.Sprintf("VM %s task with UUID %s still in progress. Requeuing", vmName, vmUUID))
return reconcile.Result{RequeueAfter: 5 * time.Second}, nil

Check warning on line 367 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L365-L367 

Added lines #L365 - L367 were not covered by tests
}
log.V(1).Info(fmt.Sprintf("No running tasks anymore... Initiating delete for vm %s with UUID %s", vmName, vmUUID))
} else {
log.V(1).Info(fmt.Sprintf("no task UUID found on VM %s. Starting delete.", *vm.Spec.Name))

Check warning on line 371 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L369-L371 

Added lines #L369 - L371 were not covered by tests
}

if rctx.NutanixClientV4 != nil {
if err := detachVolumeGroupsFromVM(ctx, rctx.NutanixClientV4, vmUUID); err != nil {
errorMsg := fmt.Errorf("failed to detach volume groups from VM %s with UUID %s: %v", vmName, vmUUID, err)
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.VolumeGroupDetachFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
log.Error(errorMsg, "failed to detach volume groups", "cluster", rctx.NutanixCluster.Name)
return reconcile.Result{}, err
}
var vgDetachNeeded bool
if vm.Spec.Resources != nil && vm.Spec.Resources.DiskList != nil {

Check warning on line 375 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L375 

Added line #L375 was not covered by tests
for _, disk := range vm.Spec.Resources.DiskList {
if disk.VolumeGroupReference != nil {
vgDetachNeeded = true
break

Check warning on line 379 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L378-L379 

Added lines #L378 - L379 were not covered by tests
}
}
}

Check warning on line 382 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L381-L382 

Added lines #L381 - L382 were not covered by tests

// Delete the VM since the VM was found (err was nil)
deleteTaskUUID, err := DeleteVM(ctx, v3Client, vmName, vmUUID)
if vgDetachNeeded {
createV4Client, err := isPrismCentralV4Compatible(ctx, v3Client)
if err != nil {
log.Info("error occurred while checking compatibility for Prism Central v4 APIs", "error", err.Error())
}

if createV4Client {
log.Info("Creating Prism Central v4 client for cluster", "cluster", rctx.NutanixCluster.Name)
v4Client, err := getPrismCentralV4ClientForCluster(ctx, rctx.NutanixCluster, r.SecretInformer, r.ConfigMapInformer)

Check warning on line 392 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L388-L392 

Added lines #L388 - L392 were not covered by tests
if err != nil {
errorMsg := fmt.Errorf("failed to delete VM %s with UUID %s: %v", vmName, vmUUID, err)
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
log.Error(errorMsg, "failed to delete VM")
log.Error(err, "error occurred while fetching Prism Central v4 client")
return reconcile.Result{}, err
}

Check warning on line 396 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L395-L396 

Added lines #L395 - L396 were not covered by tests

if err := detachVolumeGroupsFromVM(ctx, v4Client, vmUUID); err != nil {
errorMsg := fmt.Errorf("failed to detach volume groups from VM %s with UUID %s: %v", vmName, vmUUID, err)
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.VolumeGroupDetachFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())
log.Error(errorMsg, "failed to detach volume groups", "cluster", rctx.NutanixCluster.Name)

Check warning on line 401 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L401 

Added line #L401 was not covered by tests
return reconcile.Result{}, err
}
log.Info(fmt.Sprintf("Deletion task with UUID %s received for vm %s with UUID %s. Requeueing", deleteTaskUUID, vmName, vmUUID))
return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
}
}

// Remove the finalizer from the NutanixMachine object
log.Info(fmt.Sprintf("Removing finalizers for VM %s during delete reconciliation", vmName))
ctrlutil.RemoveFinalizer(rctx.NutanixMachine, infrav1.NutanixMachineFinalizer)

return reconcile.Result{}, nil
// Delete the VM since the VM was found (err was nil)
deleteTaskUUID, err := DeleteVM(ctx, v3Client, vmName, vmUUID)
if err != nil {
errorMsg := fmt.Errorf("failed to delete VM %s with UUID %s: %v", vmName, vmUUID, err)
conditions.MarkFalse(rctx.NutanixMachine, infrav1.VMProvisionedCondition, infrav1.DeletionFailed, capiv1.ConditionSeverityWarning, errorMsg.Error())

Check warning on line 411 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L408-L411 

Added lines #L408 - L411 were not covered by tests
log.Error(errorMsg, "failed to delete VM")
return reconcile.Result{}, err
}

Check warning on line 414 in controllers/nutanixmachine_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nutanixmachine_controller.go#L413-L414 

Added lines #L413 - L414 were not covered by tests
log.Info(fmt.Sprintf("Deletion task with UUID %s received for vm %s with UUID %s. Requeueing", deleteTaskUUID, vmName, vmUUID))
return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
}

func (r *NutanixMachineReconciler) reconcileNormal(rctx *nctx.MachineContext) (reconcile.Result, error) {
Expand Down
11 changes: 4 additions & 7 deletions pkg/context/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (

"github.com/nutanix-cloud-native/prism-go-client/utils"
prismclientv3 "github.com/nutanix-cloud-native/prism-go-client/v3"
prismclientv4 "github.com/nutanix-cloud-native/prism-go-client/v4"
capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/controllers/remote"
capierrors "sigs.k8s.io/cluster-api/errors"
Expand All @@ -40,19 +39,17 @@ var (

// ClusterContext is a context used with a NutanixCluster reconciler
type ClusterContext struct {
Context context.Context
NutanixClient *prismclientv3.Client
NutanixClientV4 *prismclientv4.Client
Context context.Context
NutanixClient *prismclientv3.Client

Cluster *capiv1.Cluster
NutanixCluster *infrav1.NutanixCluster
}

// MachineContext is a context used with a NutanixMachine reconciler
type MachineContext struct {
Context context.Context
NutanixClient *prismclientv3.Client
NutanixClientV4 *prismclientv4.Client
Context context.Context
NutanixClient *prismclientv3.Client

Cluster *capiv1.Cluster
Machine *capiv1.Machine
Expand Down

0 comments on commit dae5e54

Please sign in to comment.