add a disk deletion func to prevent spruious requeues (#55)

- Handle disk deletion via a separate func (similar to instance
deletion) that flows through the disk state machine
 - Reduce spurious re-queues and remove (some) duplicate logging 
 - Pass build info from goreleaser into main (version and commit hash)
 - Use the build info to construct a useragent string for the oxide sdk

Author: bwagner5

.goreleaser.yaml

@@ -36,6 +36,11 @@ builds:
       - CGO_ENABLED=0
     flags:
       - -trimpath
+    ldflags:
+      - -s -w 
+      - -X main.version={{.Version}}
+      - -X main.commit={{.Commit}}
+    mod_timestamp: '{{ .CommitTimestamp }}' 
     goos:
       - linux
     goarch:

cmd/main.go

@@ -19,6 +19,7 @@ package main
 import (
 	"crypto/tls"
 	"flag"
+	"fmt"
 	"os"
 
 	// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
@@ -44,6 +45,8 @@ import (
 var (
 	scheme   = runtime.NewScheme()
 	setupLog = ctrl.Log.WithName("setup")
+	version  = "dev"
+	commit   = "none"
 )
 
 func init() {
@@ -100,7 +103,7 @@ func main() {
 		"The name of the metrics server key file.",
 	)
 	flag.BoolVar(&enableHTTP2, "enable-http2", false,
-		"If set, HTTP/2 will be enabled for the metrics and webhook servers")
+		"If set, HTTP/2 will be enabled for the metrics server")
 	opts := zap.Options{
 		Development: true,
 	}
@@ -193,18 +196,20 @@ func main() {
 		os.Exit(1)
 	}
 
+	userAgent := fmt.Sprintf("cluster-api-provider-oxide/%s-%s", version, commit)
+
 	if err := (&controller.OxideMachineReconciler{
 		Client:             mgr.GetClient(),
 		Scheme:             mgr.GetScheme(),
-		OxideClientFactory: cloud.NewOxideClient,
+		OxideClientFactory: cloud.NewOxideClientFactory(userAgent),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "Failed to create controller", "controller", "OxideMachine")
 		os.Exit(1)
 	}
 	if err := (&controller.OxideClusterReconciler{
 		Client:             mgr.GetClient(),
 		Scheme:             mgr.GetScheme(),
-		OxideClientFactory: cloud.NewOxideClient,
+		OxideClientFactory: cloud.NewOxideClientFactory(userAgent),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "Failed to create controller", "controller", "OxideCluster")
 		os.Exit(1)

internal/cloud/mock/mock_client.go

@@ -3,6 +3,7 @@ package cloud
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -37,6 +38,7 @@ type OxideClient interface {
 		oxide.InstanceExternalIpListParams,
 	) (*oxide.ExternalIpResultsPage, error)
 
+	DiskView(context.Context, oxide.DiskViewParams) (*oxide.Disk, error)
 	DiskDelete(context.Context, oxide.DiskDeleteParams) error
 }
 
@@ -45,26 +47,31 @@ const (
 	SecretDataTokenKey = "oxide-token"
 )
 
-// NewOxideClient constructs an oxide.Client using the secret reference from the provided
+// NewOxideClientFactory constructs oxide.Client instances using the secret reference from the
+// provided
 // OxideCluster.
-func NewOxideClient(
-	ctx context.Context,
-	k8sClient client.Client,
-	oxideCluster *infrav1.OxideCluster,
-) (OxideClient, error) {
-	secret := &corev1.Secret{}
-	if err := k8sClient.Get(ctx, client.ObjectKey{
-		Namespace: oxideCluster.Spec.CredentialsRef.Namespace,
-		Name:      oxideCluster.Spec.CredentialsRef.Name,
-	}, secret); err != nil {
-		return nil, fmt.Errorf("loading oxide credentials: %w", err)
+func NewOxideClientFactory(userAgent string) OxideClientFactory {
+	return func(
+		ctx context.Context,
+		k8sClient client.Client,
+		oxideCluster *infrav1.OxideCluster,
+	) (OxideClient, error) {
+		secret := &corev1.Secret{}
+		if err := k8sClient.Get(ctx, client.ObjectKey{
+			Namespace: oxideCluster.Spec.CredentialsRef.Namespace,
+			Name:      oxideCluster.Spec.CredentialsRef.Name,
+		}, secret); err != nil {
+			return nil, fmt.Errorf("loading oxide credentials: %w", err)
+		}
+		oxideClient, err := oxide.NewClient(
+			oxide.WithHost(string(secret.Data[SecretDataHostKey])),
+			oxide.WithToken(string(secret.Data[SecretDataTokenKey])),
+			oxide.WithTimeout(30*time.Second),
+			oxide.WithUserAgent(userAgent),
+		)
+		if err != nil {
+			return nil, fmt.Errorf("constructing oxide client: %w", err)
+		}
+		return oxideClient, nil
 	}
-	oxideClient, err := oxide.NewClient(
-		oxide.WithHost(string(secret.Data[SecretDataHostKey])),
-		oxide.WithToken(string(secret.Data[SecretDataTokenKey])),
-	)
-	if err != nil {
-		return nil, fmt.Errorf("constructing oxide client: %w", err)
-	}
-	return oxideClient, nil
 }

internal/cloud/oxide.go

@@ -118,6 +118,13 @@ func (r *OxideClusterReconciler) Reconcile(
 		}
 		controllerutil.RemoveFinalizer(oxideCluster, infrav1.ClusterFinalizer)
 		return ctrl.Result{}, retErr
+	} else if !cluster.DeletionTimestamp.IsZero() {
+		log.Info(
+			"cluster is being deleted, aborting OxideCluster reconcile",
+			"cluster",
+			cluster.Name,
+		)
+		return ctrl.Result{}, nil
 	}
 
 	controllerutil.AddFinalizer(oxideCluster, infrav1.ClusterFinalizer)
@@ -203,17 +210,35 @@ func (r *OxideClusterReconciler) Reconcile(
 		// Attach the floating IP to the 0th provisioned instance.
 		for _, machine := range machines.Items {
 			if machine.Spec.ProviderID == "" {
+				log.Info(
+					"skipping floating IP attachment to machine due to missing providerID",
+					"machine",
+					machine.Name,
+				)
 				continue
 			}
 			provisioned := machine.Status.Initialization.Provisioned
 			if provisioned == nil || !*provisioned {
+				log.Info(
+					"skipping floating IP attachment to machine since it is not provisioned",
+					"machine",
+					machine.Name,
+				)
+				continue
+			}
+			if !machine.DeletionTimestamp.IsZero() {
+				log.Info(
+					"skipping floating IP attachment to machine since it is being deleted",
+					"machine",
+					machine.Name,
+				)
 				continue
 			}
 			instanceID, err := cloud.InstanceIDFromProviderID(machine.Spec.ProviderID)
 			if err != nil {
 				return ctrl.Result{}, fmt.Errorf("parsing provider id: %w", err)
 			}
-			log.Info("attaching floating ip", "ip", ip.Ip, "instance", instanceID)
+			log.Info("attaching floating IP", "ip", ip.Ip, "instance", instanceID)
 			ip, err = oxideClient.FloatingIpAttach(ctx, oxide.FloatingIpAttachParams{
 				FloatingIp: oxide.NameOrId(ip.Id),
 				Body: &oxide.FloatingIpAttach{

internal/controller/oxidecluster_controller.go

@@ -247,6 +247,10 @@ func (r *OxideMachineReconciler) Reconcile(
 		Status: metav1.ConditionTrue,
 		Reason: getReadyReason(instance),
 	})
+	if oxideMachine.Status.Initialization.Provisioned == nil ||
+		!*oxideMachine.Status.Initialization.Provisioned {
+		log.Info("instance is running; marking as provisioned", "instance", instance.Name)
+	}
 	oxideMachine.Status.Initialization.Provisioned = new(true)
 
 	// Look up instance addresses if not already known. As of this writing, the controller isn't
@@ -311,7 +315,6 @@ func (r *OxideMachineReconciler) ensureInstanceRunning(
 		log.Info("waiting for instance to start", "state", instance.RunState)
 		return false, instance, nil
 	case oxide.InstanceStateRunning:
-		log.Info("instance is running; marking as provisioned", "instance", instance.Name)
 		return true, instance, nil
 	default:
 		log.Info("waiting for instance", "instance", instance.Id, "state", instance.RunState)
@@ -329,8 +332,6 @@ func (r *OxideMachineReconciler) handleDelete(
 	instanceName string,
 	diskName string,
 ) (ctrl.Result, error) {
-	log := logf.FromContext(ctx)
-
 	instanceDeleted, instance, err := r.ensureInstanceDeleted(
 		ctx,
 		oxideClient,
@@ -353,14 +354,17 @@ func (r *OxideMachineReconciler) handleDelete(
 	// assume the disk isn't attached. If the disk was attached to another instance out of band, or
 	// is otherwise in an unexpected state, the reconciler isn't responsible for detaching it, and
 	// returns an error.
-	log.Info("deleting disk", "disk", diskName)
-	if err := oxideClient.DiskDelete(ctx, oxide.DiskDeleteParams{
-		Project: oxide.NameOrId(projectName),
-		Disk:    oxide.NameOrId(diskName),
-	}); err != nil {
-		if !errors.Is(err, oxide.ErrObjectNotFound) {
-			return ctrl.Result{}, fmt.Errorf("deleting disk: %w", err)
-		}
+	diskDeleted, err := r.ensureDiskDeleted(
+		ctx,
+		oxideClient,
+		projectName,
+		diskName,
+	)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("ensuring disk deleted: %w", err)
+	}
+	if !diskDeleted {
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 	}
 
 	controllerutil.RemoveFinalizer(oxideMachine, infrav1.MachineFinalizer)
@@ -399,6 +403,9 @@ func (r *OxideMachineReconciler) ensureInstanceDeleted(
 			Instance: oxide.NameOrId(instanceName),
 		})
 		if err != nil {
+			if errors.Is(err, oxide.ErrObjectNotFound) {
+				return true, nil, nil
+			}
 			return false, instance, fmt.Errorf("stopping instance: %w", err)
 		}
 		return false, instance, nil
@@ -408,8 +415,13 @@ func (r *OxideMachineReconciler) ensureInstanceDeleted(
 			Project:  oxide.NameOrId(projectName),
 			Instance: oxide.NameOrId(instanceName),
 		}); err != nil {
+			if errors.Is(err, oxide.ErrObjectNotFound) {
+				return true, nil, nil
+			}
 			return false, instance, fmt.Errorf("deleting instance: %w", err)
 		}
+		// return false here since we need to wait for the instance to be deleted from the control
+		// plane or reach a terminal state
 		return false, nil, nil
 	default:
 		log.Info(
@@ -423,6 +435,56 @@ func (r *OxideMachineReconciler) ensureInstanceDeleted(
 	}
 }
 
+func (r *OxideMachineReconciler) ensureDiskDeleted(
+	ctx context.Context,
+	oxideClient cloud.OxideClient,
+	projectName string,
+	diskName string,
+) (bool, error) {
+	log := logf.FromContext(ctx)
+
+	// View the disk. If it doesn't exist, we're done.
+	disk, err := oxideClient.DiskView(ctx, oxide.DiskViewParams{
+		Project: oxide.NameOrId(projectName),
+		Disk:    oxide.NameOrId(diskName),
+	})
+	if err != nil {
+		if errors.Is(err, oxide.ErrObjectNotFound) {
+			return true, nil
+		}
+		return false, fmt.Errorf("viewing disk: %w", err)
+	}
+
+	// Disk deletion state machine:
+	// * If detached, delete
+	// * Else log and requeue.
+	switch disk.State.State() {
+	case oxide.DiskStateStateDetached, oxide.DiskStateStateFaulted:
+		log.Info("destroying disk", "disk", disk.Id)
+		err := oxideClient.DiskDelete(ctx, oxide.DiskDeleteParams{
+			Project: oxide.NameOrId(projectName),
+			Disk:    oxide.NameOrId(diskName),
+		})
+		if err != nil {
+			if errors.Is(err, oxide.ErrObjectNotFound) {
+				return true, nil
+			}
+			return false, fmt.Errorf("destroying disk: %w", err)
+		}
+		log.Info("destroyed disk", "disk", disk.Id)
+		return true, nil
+	default:
+		log.Info(
+			"waiting for disk; requeueing",
+			"disk",
+			disk.Id,
+			"state",
+			disk.State.State(),
+		)
+		return false, nil
+	}
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *OxideMachineReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).