ci: delete failed EKS node groups before retrying terraform destroy

[bobbyiliev]

An EKS node group whose creation fails keeps its cluster undeletable (ResourceInUseException: Cluster has nodegroups attached), and Terraform does not retry the node group once the cluster delete fails, so all three terraform destroy attempts hit the same wall and the run leaks its cluster, VPC, KMS key and log groups. Because each AWS test root uses a fixed name prefix and every run starts from empty state, that leak then fails every later nightly on "already exists" within minutes, which is how one bad apply in Nightly 18263 turned into an open-ended outage of both AWS Terraform nightlies. State.destroy now calls an unblock_destroy hook between attempts, which AWS overrides to delete any attached node groups and wait for them to go away before the next attempt.

Test plan

The failure path needs a real EKS cluster with a CREATE_FAILED node group, so it is exercised by a Nightly on this branch (the branch name matches the two AWS jobs' *terraform* filter). The healthy path is unchanged: the hook only runs after a terraform destroy attempt has already failed, and the base implementation is a no-op, so GCP and Azure keep their current behavior.

🤖 Generated with Claude Code

[bosconi]

Looks good. Thanks for this!

[def-]

QA LLM Review (Post Merge)

@bobbyiliev — an automated review of commit fdc535d651 found the following potential MEDIUM+ issue(s) after this PR was merged.

1. HIGH -- unblock_destroy never finds the cluster, so no node group is ever deleted

test/terraform/mzcompose.py:732

A failed terraform destroy has already removed the root outputs from state by the time the hook runs, so terraform output -raw eks_cluster_name yields no cluster name and the node group is never deleted. The leak this change targets still happens, and it happens silently: the hook prints no "Deleting EKS node group" line and swallows the one AWS error it does produce.

Details

Terraform destroys root outputs at the start of the destroy apply, before the resources they reference, so the state written after a failed destroy has "outputs": {} while the resources are still there. Verified with the CI-pinned Terraform (1.13.5, ci/builder/Dockerfile:364) on a config whose destroy fails at a resource behind a module output:

state outputs: {}
state resources: [('module.eks', 'terraform_data', {'value': 'aws-test-dev-eks', ...})]
$ terraform output -raw eks_cluster_name   # after the failed destroy
rc 0, stderr '', stdout = 540-char "Warning: No outputs found" banner

Two consequences, both making the hook inert:

• terraform output -raw exits 0 and writes the warning to stdout, not stderr. So no CalledProcessError fires, .strip() returns the banner text, and if not cluster at line 737 is false: the "Nothing left in state to clean up after" guard does not catch this.
• _list_node_groups is then called with the banner as --cluster-name. The AWS CLI rejects it, subprocess.CalledProcessError is caught, [] is returned, and the for loop plus the wait loop both no-op. The hook returns having done nothing.

The state's resources do survive a failed destroy, so read the name from there instead (and keep it validated, so a future non-name stdout cannot reach the AWS CLI):

    def _eks_cluster_name(self) -> str:
        """EKS cluster name from state, "" if no cluster is left in it.

        Not `terraform output`: root outputs are destroyed at the start of a
        `terraform destroy` apply, so by the time a destroy has failed the
        state has no outputs left, while the resources are still in it.
        """
        try:
            state = json.loads(
                spawn.capture(["terraform", "state", "pull"], cwd=self.path)
            )
        except (subprocess.CalledProcessError, json.JSONDecodeError):
            return ""
        for resource in state.get("resources", []):
            if resource.get("type") != "aws_eks_cluster":
                continue
            for instance in resource.get("instances", []):
                name = instance.get("attributes", {}).get("name", "")
                if re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9_-]*", name):
                    return name
        return ""

Deriving it from the prefix works too, since setup already builds the same name for update-kubeconfig (f"{prefix}-dev-eks", test/terraform/mzcompose.py:786); that needs setup to stash the prefix on self. Either way, worth logging when no cluster name is found, so the next occurrence is distinguishable from a hook that ran and had nothing to do.

[bobbyiliev]

Confirmed and fixed in #38731, thanks. Reproduced against the CI-pinned Terraform 1.13.5: with resources in state and "outputs": {}, terraform output -raw eks_cluster_name exits 0 and writes 540 bytes of warning banner to stdout with nothing on stderr, so neither the except nor the if not cluster guard fires and the banner reached --cluster-name. The hook was inert and silent about it.

#38731 reads the aws_eks_cluster resource from terraform state pull instead, validates the name against [A-Za-z0-9][A-Za-z0-9_-]* so no unexpected stdout can reach the AWS CLI again, and logs when no cluster is found. Verified against the real leaked state from Nightly 18272: returns the cluster name both as uploaded and with outputs stripped, empty for a fully destroyed state, and rejects the banner.