Look up instance by providerID and node name consistently.

Currently, some instance operations look up instances by providerID and then
fall back to node name, and other operations just use providerID. This causes
problems using the cluster api, since nodes aren't provisioned with a
providerID initially. This patch looks up instances by both providerID and node
name consistently, and lightly refactors lookups to avoid repeat queries.

Author: jmcarp

internal/provider/instances_v2.go

@@ -6,8 +6,8 @@ package provider
 
 import (
 	"context"
+	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/oxidecomputer/oxide.go/oxide"
@@ -21,6 +21,8 @@ var _ cloudprovider.InstancesV2 = (*InstancesV2)(nil)
 // gibibyte is the number of bytes in a gibibyte.
 const gibibyte = 1024 * 1024 * 1024
 
+var errNotFound = errors.New("instance not found")
+
 // InstancesV2 implements [cloudprovider.InstancesV2] to provide Oxide specific
 // instance functionality.
 type InstancesV2 struct {
@@ -36,19 +38,12 @@ func (i *InstancesV2) InstanceExists(ctx context.Context, node *v1.Node) (bool,
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	instanceID, err := InstanceIDFromProviderID(node.Spec.ProviderID)
+	_, err := i.getInstance(ctx, node)
 	if err != nil {
-		return false, fmt.Errorf("failed retrieving instance id from provider id: %w", err)
-	}
-
-	if _, err := i.client.InstanceView(ctx, oxide.InstanceViewParams{
-		Instance: oxide.NameOrId(instanceID),
-	}); err != nil {
-		if strings.Contains(err.Error(), "NotFound") {
+		if errors.Is(err, errNotFound) {
 			return false, nil
 		}
-
-		return false, fmt.Errorf("failed viewing oxide instance %s: %v", instanceID, err)
+		return false, fmt.Errorf("failed retrieving instance: %w", err)
 	}
 
 	return true, nil
@@ -63,32 +58,23 @@ func (i *InstancesV2) InstanceMetadata(
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	// Get the instance ID, either from the provider ID or by looking up by name.
-	instanceID, err := i.getInstanceID(ctx, node)
+	instance, err := i.getInstance(ctx, node)
 	if err != nil {
-		return nil, err
-	}
-
-	// Retrieve the instance details.
-	instance, err := i.client.InstanceView(ctx, oxide.InstanceViewParams{
-		Instance: oxide.NameOrId(instanceID),
-	})
-	if err != nil {
-		return nil, fmt.Errorf("failed viewing oxide instance: %v", err)
+		return nil, fmt.Errorf("failed retrieving instance: %w", err)
 	}
 
 	nics, err := i.client.InstanceNetworkInterfaceList(
 		ctx,
 		oxide.InstanceNetworkInterfaceListParams{
-			Instance: oxide.NameOrId(instanceID),
+			Instance: oxide.NameOrId(instance.Id),
 		},
 	)
 	if err != nil {
 		return nil, fmt.Errorf("failed listing instance network interfaces: %v", err)
 	}
 
 	externalIPs, err := i.client.InstanceExternalIpList(ctx, oxide.InstanceExternalIpListParams{
-		Instance: oxide.NameOrId(instanceID),
+		Instance: oxide.NameOrId(instance.Id),
 	})
 	if err != nil {
 		return nil, fmt.Errorf("failed listing instance external ips: %v", err)
@@ -148,46 +134,49 @@ func (i *InstancesV2) InstanceMetadata(
 	}
 
 	return &cloudprovider.InstanceMetadata{
-		ProviderID:    NewProviderID(instanceID),
+		ProviderID:    NewProviderID(instance.Id),
 		InstanceType:  fmt.Sprintf("%d-%d", instance.Ncpus, instance.Memory/gibibyte),
 		NodeAddresses: nodeAddresses,
 	}, nil
 }
 
-// getInstanceID retrieves the instance ID either from the node's provider ID
+// getInstance retrieves the instance, either from the node's provider ID
 // or by looking up the instance by name.
-func (i *InstancesV2) getInstanceID(ctx context.Context, node *v1.Node) (string, error) {
+func (i *InstancesV2) getInstance(ctx context.Context, node *v1.Node) (*oxide.Instance, error) {
+	var params oxide.InstanceViewParams
+
 	if node.Spec.ProviderID != "" {
-		return InstanceIDFromProviderID(node.Spec.ProviderID)
+		instanceID, err := InstanceIDFromProviderID(node.Spec.ProviderID)
+		if err != nil {
+			return nil, fmt.Errorf("failed parsing provider id: %w", err)
+		}
+		params.Instance = oxide.NameOrId(instanceID)
+	} else {
+		params.Project = oxide.NameOrId(i.project)
+		params.Instance = oxide.NameOrId(node.GetName())
 	}
 
-	// If no provider ID is set, look up the instance by name.
-	instance, err := i.client.InstanceView(ctx, oxide.InstanceViewParams{
-		Project:  oxide.NameOrId(i.project),
-		Instance: oxide.NameOrId(node.GetName()),
-	})
+	instance, err := i.client.InstanceView(ctx, params)
 	if err != nil {
-		return "", fmt.Errorf("failed viewing oxide instance by name: %v", err)
+		var httpErr *oxide.HTTPError
+		if errors.As(err, &httpErr) && httpErr.ErrorResponse.ErrorCode == "ObjectNotFound" {
+			return nil, errNotFound
+		}
+
+		return nil, fmt.Errorf("failed looking up oxide instance: %w", err)
 	}
 
-	return instance.Id, nil
+	return instance, nil
 }
 
 // InstanceShutdown checks whether the provided node is shut down in Oxide.
 func (i *InstancesV2) InstanceShutdown(ctx context.Context, node *v1.Node) (bool, error) {
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	instanceID, err := InstanceIDFromProviderID(node.Spec.ProviderID)
-	if err != nil {
-		return false, fmt.Errorf("failed retrieving instance id from provider id: %w", err)
-	}
-
-	instance, err := i.client.InstanceView(ctx, oxide.InstanceViewParams{
-		Instance: oxide.NameOrId(instanceID),
-	})
+	instance, err := i.getInstance(ctx, node)
 	if err != nil {
-		return false, fmt.Errorf("failed viewing oxide instance %s: %v", instanceID, err)
+		return false, fmt.Errorf("failed retrieving instance: %w", err)
 	}
 
 	return instance.RunState == oxide.InstanceStateStopped, nil