fix: consider NodeClaim linked annotation on Delete (#28)

Co-authored-by: Rachel Gregory <[email protected]>

pkg/apis/v1alpha2/labels.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1alpha2
 
 import (
+	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	corev1beta1 "github.com/aws/karpenter-core/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-core/pkg/scheduling"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -118,4 +119,6 @@ var (
 		'c': LabelSKUConfidential,
 		'i': LabelSKUIsolatedSize,
 	}
+
+	NodeClaimLinkedAnnotationKey = v1alpha5.MachineLinkedAnnotationKey // still using the one from v1alpha5
 )

pkg/cloudprovider/cloudprovider.go

@@ -188,7 +188,14 @@ func (c *CloudProvider) GetInstanceTypes(ctx context.Context, nodePool *corev1be
 }
 
 func (c *CloudProvider) Delete(ctx context.Context, nodeClaim *corev1beta1.NodeClaim) error {
-	return c.instanceProvider.Delete(ctx, nodeClaim)
+	ctx = logging.WithLogger(ctx, logging.FromContext(ctx).With("nodeclaim", nodeClaim.Name))
+
+	providerID := lo.Ternary(nodeClaim.Status.ProviderID != "", nodeClaim.Status.ProviderID, nodeClaim.Annotations[v1alpha2.NodeClaimLinkedAnnotationKey])
+	vmName, err := utils.GetVMName(providerID)
+	if err != nil {
+		return fmt.Errorf("getting VM name, %w", err)
+	}
+	return c.instanceProvider.Delete(ctx, vmName)
 }
 
 func (c *CloudProvider) IsDrifted(ctx context.Context, nodeClaim *corev1beta1.NodeClaim) (cloudprovider.DriftReason, error) {

pkg/controllers/nodeclaim/garbagecollection/controller.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/Azure/karpenter/pkg/apis/v1alpha2"
 	"github.com/Azure/karpenter/pkg/cloudprovider"
 	"github.com/samber/lo"
 	"go.uber.org/multierr"
@@ -32,7 +33,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	corev1beta1 "github.com/aws/karpenter-core/pkg/apis/v1beta1"
 
 	link "github.com/Azure/karpenter/pkg/controllers/nodeclaim/link"
@@ -80,13 +80,13 @@ func (c *Controller) Reconcile(ctx context.Context, _ reconcile.Request) (reconc
 		return reconcile.Result{}, err
 	}
 	resolvedNodeClaims := lo.Filter(nodeClaims.Items, func(m corev1beta1.NodeClaim, _ int) bool {
-		return m.Status.ProviderID != "" || m.Annotations[v1alpha5.MachineLinkedAnnotationKey] != ""
+		return m.Status.ProviderID != "" || m.Annotations[v1alpha2.NodeClaimLinkedAnnotationKey] != ""
 	})
 	resolvedProviderIDs := sets.New[string](lo.Map(resolvedNodeClaims, func(m corev1beta1.NodeClaim, _ int) string {
 		if m.Status.ProviderID != "" {
 			return m.Status.ProviderID
 		}
-		return m.Annotations[v1alpha5.MachineLinkedAnnotationKey]
+		return m.Annotations[v1alpha2.NodeClaimLinkedAnnotationKey]
 	})...)
 	errs := make([]error, len(retrieved))
 	workqueue.ParallelizeUntil(ctx, 100, len(managedRetrieved), func(i int) {

pkg/controllers/nodeclaim/link/controller.go

@@ -35,8 +35,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
+	"github.com/Azure/karpenter/pkg/apis/v1alpha2"
 	"github.com/Azure/karpenter/pkg/cloudprovider"
-	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
 	corev1beta1 "github.com/aws/karpenter-core/pkg/apis/v1beta1"
 
 	corecloudprovider "github.com/aws/karpenter-core/pkg/cloudprovider"
@@ -46,7 +46,6 @@ import (
 )
 
 const creationReasonLabel = "linking"
-const NodeClaimLinkedAnnotationKey = v1alpha5.MachineLinkedAnnotationKey // still using the one from v1alpha5
 
 type Controller struct {
 	kubeClient    client.Client
@@ -120,7 +119,7 @@ func (c *Controller) link(ctx context.Context, retrieved *corev1beta1.NodeClaim,
 		// This annotation communicates to the nodeclaim controller that this is a nodeclaim linking scenario, not
 		// a case where we want to provision a new VM
 		nodeClaim.Annotations = lo.Assign(nodeClaim.Annotations, map[string]string{
-			NodeClaimLinkedAnnotationKey: retrieved.Status.ProviderID,
+			v1alpha2.NodeClaimLinkedAnnotationKey: retrieved.Status.ProviderID,
 		})
 		if err := c.kubeClient.Create(ctx, nodeClaim); err != nil {
 			return err
@@ -142,7 +141,7 @@ func (c *Controller) shouldCreateLinkedNodeClaim(retrieved *corev1beta1.NodeClai
 	}
 	// We have a nodeclaim registered for this, so no need to hydrate it
 	if _, ok := lo.Find(existingNodeClaims, func(m corev1beta1.NodeClaim) bool {
-		return m.Annotations[NodeClaimLinkedAnnotationKey] == retrieved.Status.ProviderID ||
+		return m.Annotations[v1alpha2.NodeClaimLinkedAnnotationKey] == retrieved.Status.ProviderID ||
 			m.Status.ProviderID == retrieved.Status.ProviderID
 	}); ok {
 		return false

pkg/controllers/nodeclaim/link/suite_test.go

@@ -32,8 +32,6 @@ import (
 	. "knative.dev/pkg/logging/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
-
 	corev1beta1 "github.com/aws/karpenter-core/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-core/pkg/events"
 	coreoptions "github.com/aws/karpenter-core/pkg/operator/options"
@@ -60,8 +58,6 @@ var azureEnv *test.Environment
 var cloudProvider *cloudprovider.CloudProvider
 var linkController *link.Controller
 
-const nodeClaimLinkedAnnotationKey = v1alpha5.MachineLinkedAnnotationKey // still using the one from v1alpha5
-
 func TestAPIs(t *testing.T) {
 	ctx = TestContextWithLogger(t)
 	RegisterFailHandler(Fail)
@@ -160,7 +156,7 @@ var _ = Describe("NodeClaimLink", func() {
 		Expect(nodeClaim.Spec.StartupTaints).To(Equal(nodePool.Spec.Template.Spec.StartupTaints))
 
 		// Expect NodeClaim has linking annotation to get NodeClaim details
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 		vm = ExpectVMExists(*vm.Name)
 		ExpectProvisionerNameTagExists(vm)
 	})
@@ -211,7 +207,7 @@ var _ = Describe("NodeClaimLink", func() {
 		))
 
 		// Expect NodeClaim has linking annotation to get NodeClaim details
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 		ExpectVMExists(*vm.Name)
 	})
 	It("should link an instance with expected kubelet from provisioner kubelet configuration", func() {
@@ -232,7 +228,7 @@ var _ = Describe("NodeClaimLink", func() {
 		Expect(lo.FromPtr(nodeClaim.Spec.Kubelet.MaxPods)).To(BeNumerically("==", 10))
 
 		// Expect NodeClaim has linking annotation to get NodeClaim details
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 		vm = ExpectVMExists(*vm.Name)
 		ExpectProvisionerNameTagExists(vm)
 	})
@@ -271,7 +267,7 @@ var _ = Describe("NodeClaimLink", func() {
 		Expect(nodeClaims.Items).To(HaveLen(100))
 
 		nodeClaimInstanceIDs := sets.New(lo.Map(nodeClaims.Items, func(m corev1beta1.NodeClaim, _ int) string {
-			return lo.Must(utils.GetVMName(m.Annotations[nodeClaimLinkedAnnotationKey]))
+			return lo.Must(utils.GetVMName(m.Annotations[v1alpha2.NodeClaimLinkedAnnotationKey]))
 		})...)
 
 		Expect(nodeClaimInstanceIDs).To(HaveLen(len(vmNames)))
@@ -292,7 +288,7 @@ var _ = Describe("NodeClaimLink", func() {
 		nodeClaim := nodeClaims.Items[0]
 
 		// Expect NodeClaim has linking annotation to get NodeClaim details
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 		vm = ExpectVMExists(*vm.Name)
 		ExpectProvisionerNameTagExists(vm)
 	})
@@ -319,7 +315,7 @@ var _ = Describe("NodeClaimLink", func() {
 		Expect(env.Client.List(ctx, nodeClaims)).To(Succeed())
 		Expect(nodeClaims.Items).To(HaveLen(1))
 		nodeClaim := nodeClaims.Items[0]
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 	})
 	It("should not link an instance without a provisioner tag", func() {
 		v := ExpectVMExists(*vm.Name)
@@ -378,7 +374,7 @@ var _ = Describe("NodeClaimLink", func() {
 		nodeClaim := nodeClaims.Items[0]
 
 		// Expect NodeClaim has linking annotation to get NodeClaim details
-		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(nodeClaimLinkedAnnotationKey, providerID))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1alpha2.NodeClaimLinkedAnnotationKey, providerID))
 		vm = ExpectVMExists(*vm.Name)
 		ExpectProvisionerNameTagExists(vm)
 		Expect(*v.Tags["testKey"]).To(Equal("testVal"))

pkg/providers/instance/instance.go

@@ -37,7 +37,6 @@ import (
 	"github.com/Azure/karpenter/pkg/providers/instancetype"
 	"github.com/Azure/karpenter/pkg/providers/launchtemplate"
 	"github.com/Azure/karpenter/pkg/providers/loadbalancer"
-	"github.com/Azure/karpenter/pkg/utils"
 
 	corecloudprovider "github.com/aws/karpenter-core/pkg/cloudprovider"
 	"github.com/aws/karpenter-core/pkg/scheduling"
@@ -189,12 +188,7 @@ func (p *Provider) List(ctx context.Context) ([]*armcompute.VirtualMachine, erro
 	return vmList, nil
 }
 
-func (p *Provider) Delete(ctx context.Context, nodeClaim *corev1beta1.NodeClaim) error {
-	vmName, err := utils.GetVMName(nodeClaim.Status.ProviderID)
-	if err != nil {
-		return fmt.Errorf("getting vm name, %w", err)
-	}
-
+func (p *Provider) Delete(ctx context.Context, vmName string) error {
 	logging.FromContext(ctx).Debugf("Deleting virtual machine %s", vmName)
 	return deleteVirtualMachine(ctx, p.azClient.virtualMachinesClient, p.resourceGroup, vmName)
 }