Port OCI failed-create deletion fallback from upstream PR #9703 to 1.33.4 tag branch

[Copilot]

This ports upstream commit eb6b7f5 into the cluster-autoscaler-1.33.4 code shape (which still has inline DeleteNodes logic), preserving fork-specific mutex/size/min-size behavior. The change ensures failed-create placeholder nodes with no InstanceID are handled by decreasing nodepool target size instead of attempting instance deletion.

• OCI DeleteNodes behavior update (oci_node_pool.go)

  • Keeps existing top-of-method lock, GetNodePoolSize, logging, and min-size guard.
  • Reorders per-node flow to parse OCI ref and check empty InstanceID before Belongs.
  • Treats fake create-error nodes (FakeNodeReasonAnnotation == FakeNodeCreateError) as non-deletable-by-instance and counts them for fallback handling.
  • Deletes only nodes with valid instance IDs; taints only those successfully targeted nodes.
  • If any failed-create nodes lacked instance IDs, calls DecreaseTargetSize(-count) after deletion path.

• Test/mocks adaptation for tag-era OCI tests (oci_node_pool_test.go)

  • Converts mockManager methods to pointer receivers so stateful assertions persist.
  • Extends mockManager with size/set-size/delete-call/cache-refresh tracking and compute-size support used by DecreaseTargetSize.
  • Implements missing no-op/mock methods required by the new path (SetNodePoolSize, GetExistingNodePoolSizeViaCompute, InvalidateAndRefreshCache, taint method).
  • Updates TestDeletePastMinSize to force min-size path with explicit mock size.
  • Adds TestDeleteCreateErrorNodeWithoutInstanceIDDecreasesTargetSize to assert:
    • no DeleteInstances call for no-instance-id create-error node,
    • cache invalidation occurs,
    • target size is decremented via SetNodePoolSize.

if ociRef.InstanceID == "" {
    if node.Annotations[cloudprovider.FakeNodeReasonAnnotation] == cloudprovider.FakeNodeCreateError {
        nodesWithoutInstanceID++
        continue
    }
    return fmt.Errorf("node %s doesn't have an instance id so it can't be deleted", node.Name)
}

...

if nodesWithoutInstanceID > 0 {
    return np.DecreaseTargetSize(-nodesWithoutInstanceID)
}

Original prompt

Goal

Port the single commit from upstream pull request kubernetes#9703 onto this fork, starting from the tag cluster-autoscaler-1.33.4, and publish the result as a brand-new branch named exactly:

cluster-autoscaler-goto-1.33.4-pr-9703

• Repository: goto-opensource/autoscaler
• Upstream commit being ported: eb6b7f5d0d1a05ee083301a21fb8ce9a962a326f — "Handle failed node creations without instance IDs by decreasing nodepool target size instead of calling delete."

⚠️ Step 0 (MOST IMPORTANT): base the new branch on the TAG, not on a branch

cluster-autoscaler-1.33.4 is a git tag, not a branch. Your session may start checked out on the default branch (master), whose OCI code has already been refactored and will NOT match the snippets below. Before doing anything else, create and switch to the target branch from the tag:

git fetch --tags --force
git checkout -b cluster-autoscaler-goto-1.33.4-pr-9703 cluster-autoscaler-1.33.4
git log -1   # should show commit c1f0b00eab535b09cb976d8d1fad1918e2be787d

All file references below are relative to the tag's tree. When you finish, push this branch (cluster-autoscaler-goto-1.33.4-pr-9703) so it points at the tag commit plus your single new commit. Do NOT open a pull request.

⚠️ This is NOT a clean git cherry-pick

Upstream master refactored the OCI nodepool delete logic into a private helper deleteNodes (lowercase) called by a public DeleteNodes wrapper, and PR kubernetes#9703 is written against that lowercase helper. The cluster-autoscaler-1.33.4 tag does not have that refactor — the delete logic is all inline inside the public DeleteNodes method (uppercase). A raw git cherry-pick eb6b7f5d will therefore fail. Port the change by hand into the existing inline DeleteNodes method, preserving the fork-specific top-of-function logic (nodePoolDeleteMutex lock, GetNodePoolSize call, klog.Infof logging, and the min-size guard). Do NOT introduce a new lowercase deleteNodes helper.

File 1 — cluster-autoscaler/cloudprovider/oci/nodepools/oci_node_pool.go

On the tag, DeleteNodes currently looks like this:

func (np *nodePool) DeleteNodes(nodes []*apiv1.Node) (err error) {
	nodePoolDeleteMutex.Lock()
	defer nodePoolDeleteMutex.Unlock()

	klog.Infof("DeleteNodes called with %d nodes", len(nodes))

	size, err := np.manager.GetNodePoolSize(np)
	if err != nil {
		return err
	}

	klog.Infof("Nodepool %s has size %d", np.id, size)
	if int(size) <= np.MinSize() {
		return fmt.Errorf("min size reached, nodes will not be deleted")
	}

	refs := make([]ocicommon.OciRef, 0, len(nodes))

	for _, node := range nodes {
		belongs, err := np.Belongs(node)
		if err != nil {
			return err
		}
		if !belongs {
			return fmt.Errorf("%s belong to a different nodepool than %s", node.Name, np.Id())
		}
		ociRef, err := ocicommon.NodeToOciRef(node)
		if err != nil {
			return err
		}

		refs = append(refs, ociRef)
	}

	if len(refs) == 0 {
		return nil
	}
	deleteInstancesErr := np.manager.DeleteInstances(np, refs)
	if deleteInstancesErr == nil {
		np.manager.TaintToPreventFurtherSchedulingOnRestart(nodes, np.kubeClient)
	} else {
		klog.Warning("Error deleting instances", deleteInstancesErr)
	}
	return deleteInstancesErr
}

Replace it with this adapted version. It folds in the upstream fix while keeping the fork's mutex/size/min-size preamble, and reorders the per-node logic so the empty-InstanceID check happens BEFORE Belongs (required — a failed-create placeholder node has no instance id, and calling Belongs/GetNodePoolForInstance on it would error):

func (np *nodePool) DeleteNodes(nodes []*apiv1.Node) (err error) {
	nodePoolDeleteMutex.Lock()
	defer nodePoolDeleteMutex.Unlock()

	klog.Infof("DeleteNodes called with %d nodes", len(nodes))

	size, err := np.manager.GetNodePoolSize(np)
	if err != nil {
		return err
	}

	klog.Infof("Nodepool %s has size %d", np.id, size)
	if int(size) <= np.MinSize() {
		return fmt.Errorf("min size reached, nodes will not be deleted")
	}

	refs := make([]ocicommon.OciRef, 0, len(nodes))
	nodesWithRefs := make([]*apiv1.Node, 0, len(nodes))
	nodesWithoutInstanceID := 0

	for _, node := range nodes {
		ociRef, err := ocicommon.NodeToOciRef(node)
		if err != nil {
			return err
		}
		if ociRef.InstanceID == "" {
			if node.Annotations[cloudprovider.FakeNodeReasonAnnotation] == cloudprovider.FakeNodeCreateError {
				nodesWithoutInstanceID++
				continue
			}
			return fmt.Errorf("node %s doesn't have an instance id so it can't be deleted", node.Name)
		}
		belongs, err := np.Belongs(node)
		if err != nil {
			return err
		}
		if !belongs {
			return fmt.Errorf("%s belong to a different nodepool than %s", node.Name, np.Id())
		}

		refs = append(refs, ociRef)
		nodesWithRefs = append(nodesWithRefs, node)
	}

	if len(refs) > 0 {
		deleteInstancesErr := np.manager.DeleteInstances(np, refs)
		if deleteInstancesErr == nil {
			// this will add...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>