From 7d190fc1f19c3c475c18f2c8c13efbc746cd2cee Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:06:03 -0500 Subject: [PATCH] fix(oci): let terraform.auto.tfvars decide the shape and the node count (#1167) `validate-topology.sh` passed the topology's shape and node count as `-var`, which is the strongest source Terraform reads, so a `compute_shape` or `node_count` in `terraform.auto.tfvars` was silently discarded. The OCI guide told the reader to write both into that file and then to run the driver, so a deployment asking for `VM.Standard.E5.Flex` got `VM.Standard.E6.Flex` and said nothing about it. Three of the five shapes the guide recommends were unreachable by the command it gives, including both bare-metal ones other than the shape CI happens to use. They now go in as `TF_VAR_compute_shape` and `TF_VAR_node_count`, the weakest source, so the tfvars file wins and a deployment needs no flag to size itself. CI is unaffected: `*.auto.tfvars` is gitignored, so a runner's checkout holds no such file and a named topology still resolves to the same values every run. `instance_principal` stays a `-var`, because `--instance-principal` is a choice about the run rather than about the infrastructure's size and a stale tfvars must not quietly contradict it. Because the count can now come from the file, the driver no longer presumes it. The number of addresses in the `hosts_file` output is the authority for the single-node init path and the readiness gate, a divergence from the topology's default is logged rather than fatal, and a new `compute_shape` output lets the run report the shape it built instead of the one it asked for. In the guide: the tfvars heredoc is unquoted so `$CID` expands, with a `grep` to confirm it did, and `node_client_cidr_allow_list` is bracketed, since it is a `list(string)` and the bare CIDR was an HCL syntax error. --- docs/oci-integration/README.md | 40 +++++++++++++++---- scripts/terraform/oci-spx/README.md | 4 ++ scripts/terraform/oci-spx/outputs.tf | 4 ++ .../terraform/oci-spx/validate-topology.sh | 37 +++++++++++++---- 4 files changed, 70 insertions(+), 15 deletions(-) diff --git a/docs/oci-integration/README.md b/docs/oci-integration/README.md index a35b82874..c2ee37a6b 100644 --- a/docs/oci-integration/README.md +++ b/docs/oci-integration/README.md @@ -316,25 +316,49 @@ Choose how to deploy Spinifex on OCI, either as a single node, or at minimum a t One file, in the directory you are already in. Only `compartment_ocid` is required, because every other variable has a default that builds a working single node: +Recommended compute shapes for a VM deployment: + +- `VM.Standard.E6.Flex` - Higher performance, 5th Gen AMD EPYC processor +- `VM.Standard.E5.Flex` - Base-line performance, 4th Gen AMD EPYC processor + +Recommended bare metal shapes, preferred for large-scale production: + +- `BM.Standard.E5.192` - 192 OCPU / 2304 GB RAM. Higher performance, 4th Gen AMD EPYC processor +- `BM.Standard3.64` - 64 OCPU / 1024 GB RAM. Baseline, Intel Xeon Platinum 8358 +- `BM.Standard.E2.64` - 64 OCPU / 512 GB RAM. Legacy, 1st Gen AMD EPYC processor + ```bash -cat > terraform.auto.tfvars <<'EOF' -compartment_ocid = "ocid1.compartment.oc1..aaaa..." +cat > terraform.auto.tfvars < [!NOTE] @@ -354,11 +378,13 @@ One command builds the infrastructure, installs Spinifex, forms the cluster, con | Argument | What to pass | | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--topology` | `vm-single` for one VM, `vm-multi` for three, `bm` for one bare-metal host. Required, with no default, so a command cannot be aimed at the wrong one by omission | +| `--topology` | `vm-single` for one VM, `vm-multi` for three, `bm` for one bare-metal host. Required, with no default, so a command cannot be aimed at the wrong one by omission. It supplies a shape and a node count only where your tfvars is silent | | `--channel` | **`dev` until the next release ships**, because OCI support is not in the current stable release yet. It resolves to the newest pre-release at the moment you run it. `latest` is the default and becomes the right answer once a release carries OCI support | | `--credential-hook` | `./spx-oci-config.sh`, which installs the credential from step 2 on every node. With an instance principal, replace it with `--instance-principal` and there is no key to install | | `--keep` | **This is what makes it a deployment rather than a test.** Without it the driver destroys everything at the end, which is right for CI and wrong here | +**Your `terraform.auto.tfvars` outranks the topology**, so `--topology bm` with `compute_shape = "BM.Standard.E5.192"` in that file deploys on that shape rather than the one CI runs. The run logs the shape and node count it actually built, straight after the host addresses. + Add `--skip-workload` to stop once the cluster is verified, without launching the validation guests. Add `--dry-run` to print the plan and change nothing. > [!NOTE] diff --git a/scripts/terraform/oci-spx/README.md b/scripts/terraform/oci-spx/README.md index a56d4f9a8..44d05c2c8 100644 --- a/scripts/terraform/oci-spx/README.md +++ b/scripts/terraform/oci-spx/README.md @@ -265,6 +265,10 @@ Either way, Terraform stages the matching pool block at `/etc/spinifex/oci/exter `--topology` has no default on purpose: a command aimed at the wrong one is the easiest expensive mistake here. Each topology keeps its own state under `.validate-/`, so two can be built from one checkout without either destroying the other's instances, and every log from the run lands there. +**A topology's shape and node count are defaults, not settings.** They go to Terraform as `TF_VAR_compute_shape` and `TF_VAR_node_count`, which is the weakest source Terraform reads, so a `terraform.auto.tfvars` in the checkout outranks them and a deployment sizes itself in that file with no flag to pass. CI has no such file — `*.auto.tfvars` is gitignored — so a named topology stays the same every run. `instance_principal` is the exception and remains a `-var`, because `--instance-principal` is a choice about the run rather than about the infrastructure's size, and a stale tfvars must not quietly contradict it. + +The count that later gates read is the number of addresses the `hosts_file` output names, not the number asked for, and the run logs the shape and count it built. A tfvars that changes either is reported rather than silently diverging from the topology's name. + **The teardown decides the verdict.** A topology or workbook that cannot be destroyed is half proved, and has been a real defect before, so `destroy` runs from an `EXIT` trap even on failure and a teardown failure fails the run. `--keep` leaves everything up and records no verdict. Other flags: `--skip-workload` (form and verify, launch no guests), `--workbook NAME`, `--ssh-public-key` / `--ssh-private-key`. The workbook runs **on the node** against `127.0.0.1`, because the node certificate carries no SAN for its public address — a workbook driven from outside the VCN is still blocked. diff --git a/scripts/terraform/oci-spx/outputs.tf b/scripts/terraform/oci-spx/outputs.tf index 51c712520..a72730fc8 100644 --- a/scripts/terraform/oci-spx/outputs.tf +++ b/scripts/terraform/oci-spx/outputs.tf @@ -8,6 +8,10 @@ output "security_list_ocids" { value = { public = oci_core_security_list.public. output "compute_ocids" { value = [for instance in oci_core_instance.mulgadc : instance.id] } output "data_volume_ocids" { value = [for volume in oci_core_volume.mulgadc_data : volume.id] } +# Read back rather than assumed: the shape can come from a tfvars file, so this is +# the only place that knows what was actually built. +output "compute_shape" { value = var.compute_shape } + # What the Spinifex deploy path consumes. The primary VNIC is the host plane, so # these are the addresses update-nodes.sh and install-node.sh connect to. output "nodes" { diff --git a/scripts/terraform/oci-spx/validate-topology.sh b/scripts/terraform/oci-spx/validate-topology.sh index cfcc861a4..fa67d0c0c 100755 --- a/scripts/terraform/oci-spx/validate-topology.sh +++ b/scripts/terraform/oci-spx/validate-topology.sh @@ -34,8 +34,10 @@ CHANNEL=latest INSTALL_VERSION="" WORKBOOKS="" -# Shapes and counts per topology. Named here rather than passed in, because the -# point of a topology is that it is the same every run. +# What a topology is worth when nothing says otherwise, so a CI run of a named +# topology is the same every time. Supplied as TF_VAR_, which terraform.auto.tfvars +# outranks, so a deployment sets its shape and node count in that file and no +# flag has to carry them. declare -A TOPO_SHAPE=( [bm]="BM.Standard.E2.64" [vm-single]="VM.Standard.E6.Flex" @@ -135,8 +137,12 @@ fi # the dynamic group and policy outlive every topology. setup-identity.sh owns them. PRINCIPAL_MODE=$([ "$INSTANCE_PRINCIPAL" = 1 ] && echo adopt || echo off) -SHAPE="${TOPO_SHAPE[$TOPOLOGY]}" -NODES="${TOPO_NODES[$TOPOLOGY]}" +TOPO_DEFAULT_SHAPE="${TOPO_SHAPE[$TOPOLOGY]}" +TOPO_DEFAULT_NODES="${TOPO_NODES[$TOPOLOGY]}" +# What the apply actually built, filled in from the hosts_file output once it has. +# Until then these are what the topology asked for, which is all there is to say. +SHAPE="$TOPO_DEFAULT_SHAPE" +NODES="$TOPO_DEFAULT_NODES" # Outside the checkout on a persistent runner, because actions/checkout runs # git clean -ffdx: it would delete the Terraform state of a killed run before the # sweep could read it, leaving instances running that nothing can find. @@ -154,7 +160,12 @@ WORKBOOKS_TSV="$STATE_DIR/workbooks.tsv" # A separate state directory per topology, so two topologies can be built from one # checkout without one destroying the other's instances. tf() { - python3 "$HERE/scripts/oci_env.py" --ssh-public-key-path "$SSH_PUBLIC_KEY" \ + # The topology's shape and count go in as TF_VAR_, which is the weakest source + # Terraform reads: a terraform.auto.tfvars in the checkout outranks it, so a + # deployment sizes itself in that file and the topology only supplies the + # default a CI run wants. + TF_VAR_compute_shape="$TOPO_DEFAULT_SHAPE" TF_VAR_node_count="$TOPO_DEFAULT_NODES" \ + python3 "$HERE/scripts/oci_env.py" --ssh-public-key-path "$SSH_PUBLIC_KEY" \ ${OCI_PROFILE:+--profile "$OCI_PROFILE"} -- \ terraform -chdir="$HERE" "$@" } @@ -162,9 +173,9 @@ tf() { # terraform output takes -state but not -var, so the two sets are kept apart. They # were one set once, and the -var made output fail into the default state file. tf_state=(-state "$STATE_DIR/terraform.tfstate") +# instance_principal stays a -var, and outranks any tfvars, because --instance-principal +# is a choice about this run rather than about the infrastructure's size. tf_vars=( - -var "compute_shape=$SHAPE" - -var "node_count=$NODES" -var "instance_principal=$PRINCIPAL_MODE" "${tf_state[@]}" ) @@ -334,12 +345,22 @@ record build PASS mapfile -t HOSTS < <(tf output -raw "${tf_state[@]}" hosts_file) \ || die "could not read the hosts_file output from $STATE_DIR/terraform.tfstate" -[ "${#HOSTS[@]}" = "$NODES" ] || die "expected $NODES host(s) from the hosts_file output, got ${#HOSTS[@]}: ${HOSTS[*]}" +[ "${#HOSTS[@]}" -ge 1 ] || die "the hosts_file output named no hosts, so the apply built nothing to install on" for host in "${HOSTS[@]}"; do [[ "$host" =~ ^[0-9]+(\.[0-9]+){3}$ ]] \ || die "the hosts_file output is not an address: '$host' -- something on the wrapper's stdout is in the capture" done + +# node_count can come from a tfvars file, so the hosts the apply produced are the +# authority on how many there are. Every later count check reads this. +if [ "${#HOSTS[@]}" != "$NODES" ]; then + log "note: $NODES node(s) is the $TOPOLOGY default, and the apply built ${#HOSTS[@]}; taking the apply's" + NODES="${#HOSTS[@]}" +fi +SHAPE=$(tf output -raw "${tf_state[@]}" compute_shape) \ + || die "could not read the compute_shape output from $STATE_DIR/terraform.tfstate" log "hosts: ${HOSTS[*]}" +log "$SHAPE, $NODES node(s)" # cloud-init owns the volume, the ports and br-wan, and all three are units now, so # the question is whether they converged rather than whether a runcmd happened to