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.
2026-10-15 09:01:31 +00:00 · 2026-10-05 17:06:03 -05:00
parent 4fbde4a665
commit 7d190fc1f1
4 changed files with 70 additions and 15 deletions
@@ -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 <<EOF
+compartment_ocid        = "$CID"
 deployment_name         = "spinifex"

-# Shape and size. Bare metal is preferred; a flex VM is what the quota allows.
-compute_shape           = "VM.Standard.E6.Flex"
-compute_ocpus           = 8          # OCI counts an OCPU as a full core
+# Shape and size.
+# Bare metal is preferred for large scale production usage.
+compute_shape           = "VM.Standard.E5.Flex"
+compute_ocpus           = 8          # OCI counts an OCPU as a full core, 1 OCPU = 2vCPU
 compute_memory_in_gbs   = 32

-# 1 for a single node, 3 for a cluster. This is the only line that chooses.
+# 1 for a single node, 3 required for a cluster.
 node_count              = 1

 # The block volume behind /var/lib/spinifex, attached over iSCSI.
 data_volume_size_in_gbs = 256
 data_volume_vpus_per_gb = 120        # 120 = Ultra High Performance
+
+# Firewall. A list, so it can hold several ranges. Read the note below before
+# narrowing it.
+node_client_cidr_allow_list = ["0.0.0.0/0"]
 EOF
 ```

+The heredoc delimiter is unquoted, which is what lets `$CID` expand from the variable you exported in step 2. Check the file rather than assume, because a literal `$CID` in there fails at plan time with an unhelpful message about an invalid OCID:
+
+```bash
+grep compartment_ocid terraform.auto.tfvars
+```
+
+**This file is what decides the deployment.** `compute_ocpus` and `compute_memory_in_gbs` size a VM flex shape and are ignored on a fixed bare-metal one, which comes at one fixed size with all available OCPU/memory.
+
 [Every Terraform variable](../oci-architecture/README.md#every-terraform-variable) has the full list.

 > [!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]
@@ -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-<topology>/`, 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.
@@ -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" {
@@ -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