diff --git a/app/Actions/ApplyStorageAllowance.php b/app/Actions/ApplyStorageAllowance.php index ada142e..84cbebc 100644 --- a/app/Actions/ApplyStorageAllowance.php +++ b/app/Actions/ApplyStorageAllowance.php @@ -7,6 +7,7 @@ use App\Models\Order; use App\Models\ProvisioningRun; use App\Models\Subscription; use App\Provisioning\Jobs\AdvanceRunJob; +use App\Provisioning\WorkInFlight; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; @@ -89,13 +90,15 @@ class ApplyStorageAllowance { $order = $instance->order; - // Checked against the ORDER, because that is the subject every customer - // pipeline shares. A storage run beside an unfinished build would resize - // a disk the build is still writing; one beside a plan change would - // resize it twice from two different figures. Whatever is in flight - // reads the allowance itself when it reaches the quota step, so nothing - // is lost by standing aside. - if ($this->hasRunInFlight($order)) { + // Stand aside only for a run that will deliver the whole allowance: the + // disk, the filesystem inside it and the figure Nextcloud enforces. A + // plan change does all three and there is nothing to add. A `quota` run + // does only the last of them, and letting it stand for the booking would + // enforce a quota over a filesystem that never grew — space the machine + // does not have. `address` and `restart` do none of them, and standing + // aside for one of those was how a pack could be charged monthly and + // never delivered. See App\Provisioning\WorkInFlight. + if (app(WorkInFlight::class)->covers($order, 'storage')) { return null; } @@ -125,18 +128,4 @@ class ApplyStorageAllowance return $run; } - - private function hasRunInFlight(Order $order): bool - { - return ProvisioningRun::query() - ->where('subject_type', Order::class) - ->where('subject_id', $order->id) - ->whereIn('status', [ - ProvisioningRun::STATUS_PENDING, - ProvisioningRun::STATUS_RUNNING, - ProvisioningRun::STATUS_WAITING, - ProvisioningRun::STATUS_PAUSED, - ]) - ->exists(); - } } diff --git a/app/Actions/EndInstanceService.php b/app/Actions/EndInstanceService.php index 2f687c9..2cd4893 100644 --- a/app/Actions/EndInstanceService.php +++ b/app/Actions/EndInstanceService.php @@ -120,6 +120,7 @@ class EndInstanceService // ever revived would be announced and routed nowhere. 'route_written' => false, 'routed_hostnames' => null, + 'routed_backend' => null, 'cert_ok' => false, 'domain_cert_ok' => false, ]); diff --git a/app/Actions/ReapplyInstanceAddress.php b/app/Actions/ReapplyInstanceAddress.php index 6b989b0..f6a96b5 100644 --- a/app/Actions/ReapplyInstanceAddress.php +++ b/app/Actions/ReapplyInstanceAddress.php @@ -6,6 +6,7 @@ use App\Models\Instance; use App\Models\Order; use App\Models\ProvisioningRun; use App\Provisioning\Jobs\AdvanceRunJob; +use App\Provisioning\WorkInFlight; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; @@ -66,12 +67,14 @@ class ReapplyInstanceAddress { $order = $instance->order; - // A run already under way applies whatever the domain state says when - // it gets to the step, which is this run's state or newer. Checked - // against the ORDER, because that is the subject both pipelines share: - // starting an address run beside an unfinished customer run would have - // two runs writing one router and one Nextcloud config. - if ($this->hasRunInFlight($order)) { + // A run already under way applies whatever the domain state says when it + // gets to the step — but only if it HAS the step. Asked of the two steps + // this pipeline consists of rather than of the mere existence of a run: + // `customer` and `plan-change` both carry them and there is nothing to + // add, while `restart`, `storage` and `quota` carry neither, and standing + // aside for one of those was how a domain proven during a restart stayed + // unrouted for good. See App\Provisioning\WorkInFlight. + if (app(WorkInFlight::class)->covers($order, 'address')) { return null; } @@ -115,18 +118,4 @@ class ReapplyInstanceAddress { return $instance->hasLiveMachine(); } - - private function hasRunInFlight(Order $order): bool - { - return ProvisioningRun::query() - ->where('subject_type', Order::class) - ->where('subject_id', $order->id) - ->whereIn('status', [ - ProvisioningRun::STATUS_PENDING, - ProvisioningRun::STATUS_RUNNING, - ProvisioningRun::STATUS_WAITING, - ProvisioningRun::STATUS_PAUSED, - ]) - ->exists(); - } } diff --git a/app/Actions/RestartInstance.php b/app/Actions/RestartInstance.php index 6e70a1b..7dc04ab 100644 --- a/app/Actions/RestartInstance.php +++ b/app/Actions/RestartInstance.php @@ -111,10 +111,12 @@ class RestartInstance $order = $instance->order; // Checked against the ORDER, because that is the subject every customer - // pipeline shares. A restart beside an unfinished build would stop the - // machine the build is still writing to; a restart beside a plan change - // would race the config PUT it is about to boot. Whatever is in flight - // gets to finish, and the customer can press the button again after. + // pipeline shares. ANY run, and deliberately not the narrower "does it do + // this work?" question ReapplyInstanceAddress and ApplyStorageAllowance + // ask: taking the guest away underneath a run is harmful to every one of + // them, and unlike a proven domain or a booked pack a restart is somebody + // pressing a button, so it can be pressed again once the run is done + // rather than needing to be remembered for them. if ($this->hasRunInFlight($order)) { return null; } @@ -152,12 +154,7 @@ class RestartInstance return ProvisioningRun::query() ->where('subject_type', Order::class) ->where('subject_id', $order->id) - ->whereIn('status', [ - ProvisioningRun::STATUS_PENDING, - ProvisioningRun::STATUS_RUNNING, - ProvisioningRun::STATUS_WAITING, - ProvisioningRun::STATUS_PAUSED, - ]) + ->inFlight() ->exists(); } } diff --git a/app/Console/Commands/ApplyStorageQuotas.php b/app/Console/Commands/ApplyStorageQuotas.php index db8066b..8e6c76e 100644 --- a/app/Console/Commands/ApplyStorageQuotas.php +++ b/app/Console/Commands/ApplyStorageQuotas.php @@ -157,17 +157,19 @@ class ApplyStorageQuotas extends Command AdvanceRunJob::dispatch($run->uuid); } + /** + * ANY run, deliberately — unlike the narrower "will it do this work?" question + * App\Provisioning\WorkInFlight answers for the two actions that must not lose + * a change they were asked to make. This is a repair sweep: it runs again, it + * skips instances whose quota is already enforced, and an instance passed over + * today is picked up on the next pass. Nothing is forgotten by waiting. + */ private function hasRunInFlight(Instance $instance): bool { return ProvisioningRun::query() ->where('subject_type', Order::class) ->where('subject_id', $instance->order_id) - ->whereIn('status', [ - ProvisioningRun::STATUS_PENDING, - ProvisioningRun::STATUS_RUNNING, - ProvisioningRun::STATUS_WAITING, - ProvisioningRun::STATUS_PAUSED, - ]) + ->inFlight() ->exists(); } } diff --git a/app/Livewire/Admin/Provisioning.php b/app/Livewire/Admin/Provisioning.php index 450d2a2..23556da 100644 --- a/app/Livewire/Admin/Provisioning.php +++ b/app/Livewire/Admin/Provisioning.php @@ -47,11 +47,22 @@ class Provisioning extends Component return; } - // Move the subject out of its error state so the console reflects the retry. + // Move the subject out of its error state so the console reflects the + // retry — but only when the run being retried is the one that BUILDS it. + // + // A maintenance pipeline shares the subject (see the `address`, `restart`, + // `storage`, `quota` and `plan-change` entries in config/provisioning.php) + // and only `customer` ever puts the order back to `active`, in + // CompleteProvisioning. So retrying a failed address run used to leave a + // live, paying customer's order reading `provisioning` for ever: nothing + // in the product would move it again, the customer saw nothing wrong, and + // the console's view of them became a lie. The same reasoning RunRunner's + // failRun() applies on the way down — a maintenance failure must not + // condemn what it maintains — applies on the way back up. $subject = $run->subject; if ($subject instanceof Host) { $subject->update(['status' => 'onboarding']); - } elseif ($subject instanceof Order) { + } elseif ($subject instanceof Order && $run->pipeline === $subject->provisioningPipeline()) { $subject->update(['status' => 'provisioning']); Instance::query()->where('order_id', $subject->id)->where('status', 'failed')->update(['status' => 'provisioning']); } diff --git a/app/Models/Instance.php b/app/Models/Instance.php index c9b5410..a165a53 100644 --- a/app/Models/Instance.php +++ b/app/Models/Instance.php @@ -21,7 +21,7 @@ class Instance extends Model 'customer_id', 'order_id', 'host_id', 'vmid', 'guest_ip', 'plan', 'quota_gb', 'quota_applied_gb', 'traffic_addons', 'disk_gb', 'ram_mb', 'cores', 'restart_required_since', 'subdomain', 'custom_domain', 'nc_admin_ref', 'admin_password', 'credentials_acknowledged_at', - 'route_written', 'routed_hostnames', 'cert_ok', 'status', 'cancel_requested_at', 'service_ends_at', + 'route_written', 'routed_hostnames', 'routed_backend', 'cert_ok', 'status', 'cancel_requested_at', 'service_ends_at', 'domain_token', 'domain_verified_at', 'domain_cert_ok', 'domain_checked_at', 'domain_error', 'domain_failures', ]; diff --git a/app/Models/ProvisioningRun.php b/app/Models/ProvisioningRun.php index a5f53fc..3407792 100644 --- a/app/Models/ProvisioningRun.php +++ b/app/Models/ProvisioningRun.php @@ -44,6 +44,27 @@ class ProvisioningRun extends Model return $this->morphTo(); } + /** + * Runs that have not finished one way or the other. + * + * The four statuses that mean "this will do more work" were written out at + * four call sites that ask the same question — and a fifth status added to the + * list above would have had to be remembered at every one of them. What the + * callers do with the answer differs, and App\Provisioning\WorkInFlight is + * where the difference is explained. + * + * @param \Illuminate\Database\Eloquent\Builder $query + */ + public function scopeInFlight($query): void + { + $query->whereIn('status', [ + self::STATUS_PENDING, + self::STATUS_RUNNING, + self::STATUS_WAITING, + self::STATUS_PAUSED, + ]); + } + public function events(): HasMany { return $this->hasMany(ProvisioningStepEvent::class, 'run_id'); diff --git a/app/Provisioning/Jobs/IssueInstanceAdminAccess.php b/app/Provisioning/Jobs/IssueInstanceAdminAccess.php index 89fec2f..e867e52 100644 --- a/app/Provisioning/Jobs/IssueInstanceAdminAccess.php +++ b/app/Provisioning/Jobs/IssueInstanceAdminAccess.php @@ -5,6 +5,7 @@ namespace App\Provisioning\Jobs; use App\Models\Instance; use App\Services\Proxmox\ProxmoxClient; use App\Services\Wireguard\ConfigHandoff; +use App\Support\NextcloudOcc; use App\Support\ProvisioningSettings; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -69,9 +70,10 @@ class IssueInstanceAdminAccess implements ShouldQueue $result = $proxmox->forHost($instance->host)->guestExec( $instance->host->node ?? 'pve', (int) $instance->vmid, - 'cd /opt/nextcloud && OC_PASS='.escapeshellarg($password). - ' docker compose exec -T -e OC_PASS app php occ user:resetpassword --password-from-env '. - escapeshellarg($username), + NextcloudOcc::command( + 'user:resetpassword --password-from-env '.escapeshellarg($username), + ['OC_PASS' => $password], + ), ); } catch (Throwable $e) { // An unreachable Proxmox or guest agent throws rather than diff --git a/app/Provisioning/PipelineRegistry.php b/app/Provisioning/PipelineRegistry.php index a982526..395d42a 100644 --- a/app/Provisioning/PipelineRegistry.php +++ b/app/Provisioning/PipelineRegistry.php @@ -28,6 +28,41 @@ class PipelineRegistry return count($this->stepsFor($pipeline)); } + /** + * Will a run of `$pipeline` standing at `$fromStep` still carry out every + * step of `$work`? + * + * The question a caller about to start a `$work` run has to ask about the run + * already in flight, and the reason it is asked step by step rather than by + * pipeline name: `plan-change` contains the whole of `address` and the whole + * of `storage`, `quota` contains only the last third of `storage`, and + * `restart` contains neither. A name comparison would have to be kept in step + * with config/provisioning.php by hand; this reads the same lists the runner + * executes. + * + * EVERY step, not any: the three steps of a storage change are ordered + * because a quota Nextcloud enforces over a filesystem that never grew is a + * promise of space the machine does not have. A run that would apply the new + * quota and nothing else does not cover the work — it delivers half of it, + * which is worse than delivering none. + * + * Steps at `$fromStep` itself count as still to come. The step there is + * either about to run or running, and every step reads the machine's state + * when it gets there rather than when the run was created. + */ + public function stillCovers(string $pipeline, int $fromStep, string $work): bool + { + $remaining = array_slice($this->stepsFor($pipeline), max($fromStep, 0)); + + foreach ($this->stepsFor($work) as $step) { + if (! in_array($step, $remaining, true)) { + return false; + } + } + + return true; + } + public function resolve(string $pipeline, int $index): ProvisioningStep { $steps = $this->stepsFor($pipeline); diff --git a/app/Provisioning/RunRunner.php b/app/Provisioning/RunRunner.php index 117042f..7081c51 100644 --- a/app/Provisioning/RunRunner.php +++ b/app/Provisioning/RunRunner.php @@ -66,8 +66,18 @@ class RunRunner } // started_at marks when the current step began (drives the timeout). + // + // A PENDING run is starting a step now, whatever it says: either it has + // never run, or something put it back to the beginning — see + // StartCustomerProvisioning::reviveRunStrandedWithoutAContract(), which + // resets the status, the step, the attempt and the error. `??=` here made + // the second half of this condition dead code, so a revived run kept the + // started_at of the attempt that failed hours or days earlier: its very + // first pass was ruled timed out before the step body ran, which spent one + // of five attempts and wrote 'step timed out' into the event log for a run + // that had not started yet. if ($run->started_at === null || $run->status === ProvisioningRun::STATUS_PENDING) { - $run->started_at ??= now(); + $run->started_at = now(); } $run->status = ProvisioningRun::STATUS_RUNNING; $run->save(); diff --git a/app/Provisioning/Steps/Customer/ApplyStorageQuota.php b/app/Provisioning/Steps/Customer/ApplyStorageQuota.php index 8209999..eb711e9 100644 --- a/app/Provisioning/Steps/Customer/ApplyStorageQuota.php +++ b/app/Provisioning/Steps/Customer/ApplyStorageQuota.php @@ -6,6 +6,7 @@ use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Billing\StorageAllowance; use App\Services\Proxmox\ProxmoxClient; +use App\Support\NextcloudOcc; /** * The storage a plan change actually delivers. @@ -61,11 +62,10 @@ class ApplyStorageQuota extends CustomerStep } $pve = $this->pve->forHost($instance->host); - $occ = 'cd /opt/nextcloud && docker compose exec -T app php occ '; // Idempotent, like every other occ call in this pipeline: writing the // same value again is a no-op and exits 0. - $this->guest($pve, $run, $occ.'config:app:set files default_quota --value='.escapeshellarg($quota.' GB')); + $this->guest($pve, $run, NextcloudOcc::command('config:app:set files default_quota --value='.escapeshellarg($quota.' GB'))); // Recorded only AFTER the guest accepted it — guest() throws on a // non-zero exit, so nothing below runs for a call that failed. Without diff --git a/app/Provisioning/Steps/Customer/ConfigureDnsAndTls.php b/app/Provisioning/Steps/Customer/ConfigureDnsAndTls.php index 13a5276..06b5168 100644 --- a/app/Provisioning/Steps/Customer/ConfigureDnsAndTls.php +++ b/app/Provisioning/Steps/Customer/ConfigureDnsAndTls.php @@ -75,14 +75,30 @@ class ConfigureDnsAndTls extends CustomerStep // written. `route_written` alone would short-circuit exactly the case // this step is re-run for — an address that has changed — and the // customer's domain would be announced but never routed, or stay routed - // after it was withdrawn. Comparing the hostname list keeps the retry + // after it was withdrawn. Comparing what was written keeps the retry // cheap (a second attempt at the same address writes nothing) without // making a re-apply a no-op. - if (! $instance->route_written || $instance->routed_hostnames !== $hostnames) { - $trafficHost = $host->wg_ip ?? $host->public_ip; - $backend = $instance->guest_ip ?: $host->public_ip; + // + // BOTH halves of the file are compared, because a router names a backend + // as well as a set of hostnames, and the backend is a DHCP address. The + // hostname list on its own said "nothing has changed" for a machine that + // had moved: cloud-init gives every guest `ip=dhcp`, a lease expiry or a + // cold boot can put it somewhere else, and the file kept pointing at where + // it used to be — a 502 on the customer's own cloud that nothing would + // ever rewrite. ConfigureNetwork re-reads that address on a restart; this + // is what makes the router follow it. + $trafficHost = $host->wg_ip ?? $host->public_ip; + $backend = $instance->guest_ip ?: $host->public_ip; + + if (! $instance->route_written + || $instance->routed_hostnames !== $hostnames + || $instance->routed_backend !== $backend) { $this->traefik->write($trafficHost, $instance->subdomain, $hostnames, $backend); - $instance->update(['route_written' => true, 'routed_hostnames' => $hostnames]); + $instance->update([ + 'route_written' => true, + 'routed_hostnames' => $hostnames, + 'routed_backend' => $backend, + ]); } // TLS via HTTP-01 — poll until the certificate is served. diff --git a/app/Provisioning/Steps/Customer/ConfigureNetwork.php b/app/Provisioning/Steps/Customer/ConfigureNetwork.php index 0655de5..af4a6f1 100644 --- a/app/Provisioning/Steps/Customer/ConfigureNetwork.php +++ b/app/Provisioning/Steps/Customer/ConfigureNetwork.php @@ -2,6 +2,8 @@ namespace App\Provisioning\Steps\Customer; +use App\Actions\ReapplyInstanceAddress; +use App\Models\Instance; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Proxmox\ProxmoxClient; @@ -43,6 +45,8 @@ class ConfigureNetwork extends CustomerStep } $instance->update(['guest_ip' => $guestIp]); + $this->repairAddressIfTheGuestMoved($run, $instance, $guestIp); + // Allow HTTP/HTTPS, deny everything else (management stays on the tunnel). $pve->applyFirewall($node, $vmid, [ ['type' => 'in', 'action' => 'ACCEPT', 'proto' => 'tcp', 'dport' => '80'], @@ -51,4 +55,44 @@ class ConfigureNetwork extends CustomerStep return StepResult::advance(); } + + /** + * The guest came up somewhere else — put its address back together. + * + * cloud-init hands every guest `ip=dhcp`, so the address is a lease and not a + * property of the machine: it can move on an expiry or a cold boot, and this + * step is the only thing in the product that ever reads it. The router on the + * serving host names it as the backend, so a move that nobody follows up is a + * customer's cloud answering 502 from then on, for good. + * + * The follow-up is an address run, because writing that file is the address + * pipeline's job and not this step's — one place writes the router, and this + * one only knows that it is now wrong. That run is allowed to start beside + * this one because the guard asks whether the run in flight will carry out the + * address steps rather than whether anything at all is running, and a restart + * carries neither of them (App\Provisioning\WorkInFlight). + * + * Only on a CHANGE from an address we already had. A first build has no + * previous address to have moved from — and its instance is not live yet, so + * the re-apply would decline anyway — and a retry of this step reads the + * address it just wrote, so it asks once. + */ + private function repairAddressIfTheGuestMoved(ProvisioningRun $run, Instance $instance, string $guestIp): void + { + $routed = $instance->routed_backend; + + if ($routed === null || $routed === $guestIp) { + return; + } + + $run->events()->create([ + 'step' => $this->key(), + 'attempt' => $run->attempt, + 'outcome' => 'info', + 'message' => "Die Maschine ist mit einer neuen Adresse zurückgekommen ({$routed} → {$guestIp}). " + .'Die Weiterleitung wird neu geschrieben.', + ]); + + app(ReapplyInstanceAddress::class)($instance->fresh()); + } } diff --git a/app/Provisioning/Steps/Customer/ConfigureNextcloud.php b/app/Provisioning/Steps/Customer/ConfigureNextcloud.php index 3385f50..f980bd2 100644 --- a/app/Provisioning/Steps/Customer/ConfigureNextcloud.php +++ b/app/Provisioning/Steps/Customer/ConfigureNextcloud.php @@ -5,6 +5,7 @@ namespace App\Provisioning\Steps\Customer; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Proxmox\ProxmoxClient; +use App\Support\NextcloudOcc; use App\Support\ProvisioningSettings; class ConfigureNextcloud extends CustomerStep @@ -27,10 +28,9 @@ class ConfigureNextcloud extends CustomerStep $pve = $this->pve->forHost($instance->host); $fqdn = $instance->subdomain.'.'.ProvisioningSettings::dnsZone(); - $occ = 'cd /opt/nextcloud && docker compose exec -T app php occ '; // Idempotent occ calls (setting the same values is a no-op). - $this->guest($pve, $run, $occ.'config:system:set trusted_domains 1 --value='.escapeshellarg($fqdn)); + $this->guest($pve, $run, NextcloudOcc::command('config:system:set trusted_domains 1 --value='.escapeshellarg($fqdn))); // Only a VERIFIED domain goes into trusted_domains. Nextcloud will // answer for anything listed here, so an unproven hostname added at // provisioning time would serve this customer's files to whoever @@ -45,12 +45,12 @@ class ConfigureNextcloud extends CustomerStep // exits 0 when the key is already absent, so the step stays idempotent // for the overwhelmingly common case of an instance that never had one. if ($instance->domainIsVerified()) { - $this->guest($pve, $run, $occ.'config:system:set trusted_domains 2 --value='.escapeshellarg($instance->custom_domain)); + $this->guest($pve, $run, NextcloudOcc::command('config:system:set trusted_domains 2 --value='.escapeshellarg($instance->custom_domain))); } else { - $this->guest($pve, $run, $occ.'config:system:delete trusted_domains 2'); + $this->guest($pve, $run, NextcloudOcc::command('config:system:delete trusted_domains 2')); } - $this->guest($pve, $run, $occ.'background:cron'); - $this->guest($pve, $run, $occ.'config:system:set default_phone_region --value=DE'); + $this->guest($pve, $run, NextcloudOcc::command('background:cron')); + $this->guest($pve, $run, NextcloudOcc::command('config:system:set default_phone_region --value=DE')); return StepResult::advance(); } diff --git a/app/Provisioning/Steps/Customer/CreateCustomerAdmin.php b/app/Provisioning/Steps/Customer/CreateCustomerAdmin.php index 57ce0dd..df038ff 100644 --- a/app/Provisioning/Steps/Customer/CreateCustomerAdmin.php +++ b/app/Provisioning/Steps/Customer/CreateCustomerAdmin.php @@ -5,6 +5,7 @@ namespace App\Provisioning\Steps\Customer; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Proxmox\ProxmoxClient; +use App\Support\NextcloudOcc; use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Str; @@ -36,21 +37,18 @@ class CreateCustomerAdmin extends CustomerStep $username = 'admin'; $password = Str::random(20); - $occ = 'cd /opt/nextcloud && docker compose exec -T'; // Retry-safe: if a prior crashed attempt already created the user, reset // its password instead of re-running user:add (which Nextcloud rejects). - $exists = (int) ($pve->guestExec($node, $vmid, $occ.' app php occ user:info '.escapeshellarg($username))['exitcode'] ?? 1) === 0; + $exists = (int) ($pve->guestExec($node, $vmid, NextcloudOcc::command('user:info '.escapeshellarg($username)))['exitcode'] ?? 1) === 0; $action = $exists ? 'user:resetpassword --password-from-env '.escapeshellarg($username) : 'user:add --password-from-env --group=admin '.escapeshellarg($username); - // Pass the password via env (OC_PASS) on the docker invocation itself — - // an assignment before `cd` would not survive the `&&`. - $this->guest($pve, $run, - 'cd /opt/nextcloud && OC_PASS='.escapeshellarg($password). - ' docker compose exec -T -e OC_PASS app php occ '.$action); + // The password travels as OC_PASS rather than as an argument, so it never + // shows up in the guest's process list — see NextcloudOcc::command(). + $this->guest($pve, $run, NextcloudOcc::command($action, ['OC_PASS' => $password])); // Persist only the username (encrypted ref); hand the password to step 15 // encrypted-in-context for delivery, then it is scrubbed. Never plaintext. diff --git a/app/Provisioning/Steps/Customer/DeployApplicationStack.php b/app/Provisioning/Steps/Customer/DeployApplicationStack.php index 230a0aa..b563d82 100644 --- a/app/Provisioning/Steps/Customer/DeployApplicationStack.php +++ b/app/Provisioning/Steps/Customer/DeployApplicationStack.php @@ -10,6 +10,9 @@ use Illuminate\Support\Str; class DeployApplicationStack extends CustomerStep { + /** How long the stack has to come up and report itself installed. */ + private const READY_DEADLINE = 1080; + public function __construct(private ProxmoxClient $pve) {} public function key(): string @@ -19,6 +22,7 @@ class DeployApplicationStack extends CustomerStep public function maxDuration(): int { + // Above READY_DEADLINE, so the step's own named failure fires first. return 1200; } @@ -47,11 +51,29 @@ class DeployApplicationStack extends CustomerStep $run->mergeContext(['stack_deployed' => true]); } - // Poll until Nextcloud answers (poll doesn't consume the retry budget). + // Poll until Nextcloud is INSTALLED, not merely answering (poll doesn't + // consume the retry budget). + // + // `curl -sf` alone was the whole readiness test, and status.php answers + // 200 on an instance that has never been installed — the body then reads + // {"installed":false,…}. So the run marched on into `occ config:system:set` + // and `occ user:add`, neither of which is even REGISTERED as a command on + // an uninstalled Nextcloud, and the failure surfaced three steps later as + // an unexplained non-zero exit inside the guest. The flag is the one thing + // status.php says that distinguishes a web server from a Nextcloud. $health = $pve->guestExec($node, $vmid, - 'curl -sf http://localhost/status.php >/dev/null 2>&1 && echo ok || echo wait'); + 'curl -sf http://localhost/status.php 2>/dev/null' + .' | grep -qE \'"installed"[[:space:]]*:[[:space:]]*true\' && echo ok || echo wait'); + if (trim($health['out-data'] ?? '') !== 'ok') { - return StepResult::poll(15, 'waiting for application stack'); + // Its own deadline, below maxDuration, so what an operator reads is + // what happened rather than 'step timed out'. Long: the first boot + // pulls the images, and a slow mirror is not a fault. + if ($run->started_at !== null && $run->started_at->copy()->addSeconds(self::READY_DEADLINE)->isPast()) { + return StepResult::fail('nextcloud_not_installed'); + } + + return StepResult::poll(15, 'waiting for Nextcloud to report itself installed'); } $run->forgetContext('db_password'); // scrub once the stack is up diff --git a/app/Provisioning/Steps/Customer/ReserveResources.php b/app/Provisioning/Steps/Customer/ReserveResources.php index b61d503..0580c7c 100644 --- a/app/Provisioning/Steps/Customer/ReserveResources.php +++ b/app/Provisioning/Steps/Customer/ReserveResources.php @@ -4,10 +4,10 @@ namespace App\Provisioning\Steps\Customer; use App\Models\Host; use App\Models\Instance; -use App\Services\Provisioning\HostCapacity; use App\Models\Order; use App\Models\ProvisioningRun; use App\Provisioning\StepResult; +use App\Services\Provisioning\HostCapacity; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; @@ -25,9 +25,33 @@ class ReserveResources extends CustomerStep return 'reserve_resources'; } + /** + * The whole park, not the second the body takes. + * + * RunRunner measures this from `started_at` and deliberately does NOT reset + * `started_at` on a poll — that is what lets a polling step's own deadline + * accumulate across re-entries instead of restarting every time. So for a step + * that polls, maxDuration is its TOTAL budget, and it has to sit above the + * step's own deadline. Every other polling step in the repo already does: + * ConfigureDnsAndTls waits 840 s for a certificate inside 960 s, ConfigureNetwork + * 90 s inside 150 s. + * + * At 60 s against a 120 s poll this one could not survive its own first wait. + * The run came back due, the runner saw `started_at` two minutes old, turned that + * into a timeout retry before the body ran, and five timeouts later failRun() + * marked a paid order and its instance failed and released the instance — about + * six minutes for a wait that promises fourteen days. Everything built on that + * promise (the console's capacity queue, HostCapacity::parked(), queueDemand(), + * the operator going and buying a machine) was unreachable code, and a customer + * who paid when nothing had room got an incident instead of a delivery date. + * + * One poll interval of headroom, so the step's own `no_capacity` fail below is + * always what fires. That failure names what the operator has to do; a timeout + * names nothing. + */ public function maxDuration(): int { - return 60; + return self::PARK_DAYS * 86400 + self::PARK_POLL_SECONDS; } public function execute(ProvisioningRun $run): StepResult diff --git a/app/Provisioning/Steps/Customer/RunAcceptanceChecks.php b/app/Provisioning/Steps/Customer/RunAcceptanceChecks.php index 53f2085..c47f79f 100644 --- a/app/Provisioning/Steps/Customer/RunAcceptanceChecks.php +++ b/app/Provisioning/Steps/Customer/RunAcceptanceChecks.php @@ -7,15 +7,41 @@ use App\Provisioning\StepResult; use App\Services\Monitoring\MonitoringClient; use App\Services\Proxmox\ProxmoxClient; use App\Services\Traefik\TraefikWriter; +use App\Support\NextcloudOcc; use App\Support\ProvisioningSettings; /** * Gate: nothing grants the customer access until every check here actually * passes — the cert is served, Nextcloud reports healthy, the admin account is * usable, a backup exists, and monitoring is green. + * + * A check that says no is asked again before it condemns anything. Every one of + * them is a PROBE — an HTTPS request from this VM, a command inside the guest — + * and each has its own ways of answering no without anything being wrong: + * TraefikWriter::certReachable() turns a connect timeout, a TLS handshake that + * came in late and a DNS blip into exactly the same `false` as a missing + * certificate, and a guest agent call fails outright while the guest is busy. + * This step used to return fail() on the first no, which bypasses the retry + * budget entirely — so one bad second on our side ended a fully built, certified, + * working Nextcloud as a `failed` order with the instance released, which is the + * worst outcome the pipeline can produce and the one it produced most easily. + * + * So the probes retry, like the rest of the pipeline, and the run still fails for + * good once the budget is gone: five noes in a row is not a blip. What stays + * terminal is the two facts a retry cannot change — state we wrote down + * ourselves, earlier in this same run. */ class RunAcceptanceChecks extends CustomerStep { + /** + * How long before a probe that answered no is asked again. + * + * Long enough for a warming Traefik router or a busy guest to settle, short + * enough that five attempts still finish inside the delivery the customer is + * watching. + */ + private const PROBE_RETRY_SECONDS = 30; + public function __construct( private ProxmoxClient $pve, private TraefikWriter $traefik, @@ -39,29 +65,40 @@ class RunAcceptanceChecks extends CustomerStep $vmid = (int) $run->context('vmid'); $fqdn = $instance->subdomain.'.'.ProvisioningSettings::dnsZone(); $pve = $this->pve->forHost($instance->host); - $occ = 'cd /opt/nextcloud && docker compose exec -T app php occ '; - // TLS + routing actually serving. - if (! $instance->cert_ok || ! $this->traefik->certReachable($fqdn)) { + // Terminal, and the only cert case that is: `cert_ok` is written by + // ConfigureDnsAndTls, which fails the run itself if the platform + // certificate never appears. Standing here without it means the pipeline + // reached acceptance in a state it cannot reach, and no number of + // attempts fixes that. + if (! $instance->cert_ok) { return StepResult::fail('acceptance_failed:cert'); } + // TLS + routing actually serving, asked of the outside world. + if (! $this->traefik->certReachable($fqdn)) { + return $this->askAgain('acceptance_failed:cert'); + } + // Nextcloud installed, not in maintenance, DB reachable. - $status = $pve->guestExec($node, $vmid, $occ.'status --output=json'); + $status = $pve->guestExec($node, $vmid, NextcloudOcc::command('status --output=json')); $health = json_decode($status['out-data'] ?? '', true) ?: []; if ((int) ($status['exitcode'] ?? 1) !== 0 || ($health['installed'] ?? false) !== true || ($health['maintenance'] ?? false) !== false) { - return StepResult::fail('acceptance_failed:nextcloud'); + return $this->askAgain('acceptance_failed:nextcloud'); } // The admin account exists and is queryable. - $admin = $pve->guestExec($node, $vmid, $occ.'user:info '.escapeshellarg((string) $instance->nc_admin_ref)); + $admin = $pve->guestExec($node, $vmid, NextcloudOcc::command('user:info '.escapeshellarg((string) $instance->nc_admin_ref))); if ((int) ($admin['exitcode'] ?? 1) !== 0) { - return StepResult::fail('acceptance_failed:admin'); + return $this->askAgain('acceptance_failed:admin'); } - // A backup job is registered. + // Terminal for the same reason as cert_ok above: this is a breadcrumb THIS + // run wrote at RegisterBackup, read out of our own database. If it is not + // there, no backup job was registered, and asking again five times only + // delays saying so. if (! $this->hasResource($run, 'backup_job_id')) { return StepResult::fail('acceptance_failed:backup'); } @@ -91,4 +128,19 @@ class RunAcceptanceChecks extends CustomerStep return StepResult::advance(); } + + /** + * Ask the probes again, and keep the reason for the day they run out. + * + * retry(), not poll(): a poll does not consume the budget, so a genuinely + * broken instance would sit here re-probing until the step's own maxDuration + * turned it into a nameless timeout. A retry is bounded by max_attempts and + * RunRunner hands this exact reason to failRun() when the last one is spent, + * so an operator still reads `acceptance_failed:nextcloud` rather than + * "step timed out". + */ + private function askAgain(string $reason): StepResult + { + return StepResult::retry(self::PROBE_RETRY_SECONDS, $reason); + } } diff --git a/app/Provisioning/WorkInFlight.php b/app/Provisioning/WorkInFlight.php new file mode 100644 index 0000000..ab1aba6 --- /dev/null +++ b/app/Provisioning/WorkInFlight.php @@ -0,0 +1,75 @@ +where('subject_type', Order::class) + ->where('subject_id', $order->id) + ->inFlight() + ->get(['pipeline', 'current_step']); + + foreach ($runs as $run) { + try { + if ($this->registry->stillCovers($run->pipeline, $run->current_step, $pipeline)) { + return true; + } + } catch (InvalidArgumentException) { + // A pipeline that no longer exists (renamed in a deploy while a + // run was in flight) covers nothing. The run will fail at its + // next advance for the same reason; refusing to start the work + // over it would lose the work as well. + continue; + } + } + + return false; + } +} diff --git a/app/Services/Dns/HttpHetznerDnsClient.php b/app/Services/Dns/HttpHetznerDnsClient.php index 4bbbfe7..dabdbea 100644 --- a/app/Services/Dns/HttpHetznerDnsClient.php +++ b/app/Services/Dns/HttpHetznerDnsClient.php @@ -16,6 +16,16 @@ use RuntimeException; */ class HttpHetznerDnsClient implements HetznerDnsClient { + /** Hetzner's own maximum; asking for more is answered with 100 anyway. */ + private const PAGE_SIZE = 100; + + /** + * A ceiling on the record walk — 10 000 records in one zone is far past + * anything this product will hold, and far short of a request loop that never + * ends because a page count came back wrong. + */ + private const MAX_PAGES = 100; + private function http(): PendingRequest { // Read HERE, at the point of use, rather than in a constructor or @@ -41,8 +51,7 @@ class HttpHetznerDnsClient implements HetznerDnsClient $zoneId = $this->zoneId(); $name = rtrim(str_replace('.'.ProvisioningSettings::dnsZone(), '', $fqdn), '.'); - $existing = collect($this->http()->get('/records', ['zone_id' => $zoneId])->throw()->json('records', [])) - ->first(fn ($r) => $r['name'] === $name && $r['type'] === $type); + $existing = $this->findRecord($zoneId, $name, $type); if ($existing) { $this->http()->put("/records/{$existing['id']}", [ @@ -57,6 +66,49 @@ class HttpHetznerDnsClient implements HetznerDnsClient ])->throw()->json('record.id'); } + /** + * The record with this name and type in the zone, or null. + * + * Paged, because Hetzner pages. `GET /records` answers with at most 100 + * entries and this asked only for the first page — so once the zone held more + * than a hundred records the lookup stopped finding entries that were plainly + * there, fell through to POST, and Hetzner accepted a SECOND A record for the + * same name. Two addresses for one instance, resolved round-robin, one of them + * a host the customer's machine is not on: the cloud was up about half the + * time, and every subsequent run made it worse, because the `address` and + * `plan-change` pipelines upsert on every pass. + * + * A hundred is not a distant number here. The zone carries one A record per + * customer instance plus one per host, and nothing prunes it faster than + * instances are sold. + * + * @return array|null + */ + private function findRecord(string $zoneId, string $name, string $type): ?array + { + $page = 1; + + do { + $response = $this->http() + ->get('/records', ['zone_id' => $zoneId, 'page' => $page, 'per_page' => self::PAGE_SIZE]) + ->throw(); + + foreach ($response->json('records', []) as $record) { + if (($record['name'] ?? null) === $name && ($record['type'] ?? null) === $type) { + return $record; + } + } + + // Absent or nonsensical pagination means "this was the lot": a client + // that trusted the field to be there could loop for ever against an + // API having a bad day, and MAX_PAGES is the floor under that even + // when it answers. + $lastPage = (int) $response->json('meta.pagination.last_page', $page); + } while ($page++ < min($lastPage, self::MAX_PAGES)); + + return null; + } + public function deleteRecord(string $recordId): void { $response = $this->http()->delete("/records/{$recordId}"); diff --git a/app/Support/NextcloudOcc.php b/app/Support/NextcloudOcc.php new file mode 100644 index 0000000..0c6baa5 --- /dev/null +++ b/app/Support/NextcloudOcc.php @@ -0,0 +1,63 @@ +` as that account. + * + * Environment values are passed on the docker invocation itself and handed + * through with `-e`, because an assignment placed before the `cd` would not + * survive the `&&` — this is how OC_PASS reaches `user:add + * --password-from-env` without the password ever appearing in an occ + * argument (and so in the guest's process list). + * + * @param array $env + */ + public static function command(string $arguments, array $env = []): string + { + $assignments = ''; + $forwards = ''; + + foreach ($env as $name => $value) { + $assignments .= $name.'='.escapeshellarg($value).' '; + $forwards .= '-e '.$name.' '; + } + + return 'cd '.self::DIRECTORY.' && '.$assignments + .'docker compose exec -T -u '.self::USER.' '.$forwards.'app php occ '.$arguments; + } +} diff --git a/config/provisioning.php b/config/provisioning.php index dd8fd9c..76a8350 100644 --- a/config/provisioning.php +++ b/config/provisioning.php @@ -73,9 +73,31 @@ return [ /* | Making an existing instance's address real again, without rebuilding - | anything. Exactly the two steps an address consists of: the router, - | the DNS record and the certificate, then the hostname Nextcloud - | itself will answer to. + | anything. Exactly the two steps an address consists of: the hostname + | Nextcloud itself will answer to, then the router, the DNS record and + | the certificate. + | + | In THAT order, which is the order the `customer` pipeline has always + | used, and it is not interchangeable. A half-finished address is the + | ordinary outcome of a failed run, so the question is which half is safe + | to be left holding, and the answer differs by direction: + | + | - A domain just proven. Router first meant getting the certificate and + | then failing at Nextcloud — so the customer's own domain served + | Nextcloud's "Zugriff über nicht vertrauenswürdige Domain" error page + | under a valid certificate, while `domain_cert_ok` was already true + | and the portal was calling that address live. Nextcloud first leaves + | a trusted hostname that nothing routes to, which nobody can reach and + | nothing announces. + | - A domain withdrawn. Nextcloud first stops it being ANSWERED, which is + | what withdrawal means; if the router write then fails the name still + | reaches us and is refused. Router first stops it being reached while + | Nextcloud would still have answered it — the same end state, arrived + | at with the trust left standing. + | + | So Nextcloud first is safe in both directions and router first is safe + | in only one. There is no third step to sequence and no state either step + | reads from the other. | | Same subject as `customer` (the Order), because that is what | CustomerStep::order()/instance() resolve — a run against any other @@ -85,8 +107,8 @@ return [ | carries one. */ 'address' => [ - Customer\ConfigureDnsAndTls::class, Customer\ConfigureNextcloud::class, + Customer\ConfigureDnsAndTls::class, ], /* @@ -104,7 +126,8 @@ return [ | | The two address steps are the `address` pipeline itself, reused rather | than repeated: a downgrade onto a package without an own domain has to - | stop serving that domain, and that is exactly what those two do. + | stop serving that domain, and that is exactly what those two do — in + | the same order and for the same reason, spelled out above that pipeline. */ 'plan-change' => [ Customer\ResizeVirtualMachine::class, @@ -115,8 +138,8 @@ return [ // life of the machine. Customer\GrowGuestFilesystem::class, Customer\ApplyStorageQuota::class, - Customer\ConfigureDnsAndTls::class, Customer\ConfigureNextcloud::class, + Customer\ConfigureDnsAndTls::class, Customer\SettlePlanServices::class, ], @@ -130,16 +153,27 @@ return [ | because nothing in the product could do this, which is how a paid | upgrade could stay invisible forever. | - | StartVirtualMachine and WaitForGuestAgent are the customer pipeline's - | own steps, reused rather than repeated — starting a machine and waiting - | for its agent is the same operation whichever run needs it. Started by + | StartVirtualMachine, WaitForGuestAgent and ConfigureNetwork are the + | customer pipeline's own steps, reused rather than repeated — starting a + | machine, waiting for its agent and reading the address it came up on is + | the same operation whichever run needs it. Started by | App\Actions\RestartInstance, from the customer's cloud page or the | console's instance list. + | + | ConfigureNetwork is here because a cold boot is the one event that can + | move the guest: cloud-init hands it `ip=dhcp`, and this step is the only + | writer of `instances.guest_ip` — which is the backend the router on the + | serving host points at. Without it a restarted machine could come back on + | a different address, the router would go on pointing at the old one, and + | the customer's cloud would answer 502 for the rest of its life with no + | pipeline able to repair it. The step itself asks for the address to be + | re-applied when it has actually changed; see ConfigureNetwork. */ 'restart' => [ Customer\ShutDownVirtualMachine::class, Customer\StartVirtualMachine::class, Customer\WaitForGuestAgent::class, + Customer\ConfigureNetwork::class, Customer\CompleteRestart::class, ], diff --git a/database/migrations/2026_07_30_090000_add_routed_backend_to_instances.php b/database/migrations/2026_07_30_090000_add_routed_backend_to_instances.php new file mode 100644 index 0000000..a8d6264 --- /dev/null +++ b/database/migrations/2026_07_30_090000_add_routed_backend_to_instances.php @@ -0,0 +1,39 @@ +string('routed_backend')->nullable()->after('routed_hostnames'); + }); + } + + public function down(): void + { + Schema::table('instances', function (Blueprint $table) { + $table->dropColumn('routed_backend'); + }); + } +}; diff --git a/tests/Feature/Admin/CapacityQueueTest.php b/tests/Feature/Admin/CapacityQueueTest.php index be2c516..b661629 100644 --- a/tests/Feature/Admin/CapacityQueueTest.php +++ b/tests/Feature/Admin/CapacityQueueTest.php @@ -7,9 +7,11 @@ use App\Models\Host; use App\Models\Instance; use App\Models\Order; use App\Models\ProvisioningRun; +use App\Provisioning\RunRunner; use App\Provisioning\Steps\Customer\ReserveResources; use App\Services\Provisioning\HostCapacity; use Illuminate\Support\Facades\Http; +use Illuminate\Support\Facades\Queue; use Livewire\Livewire; /** @@ -66,6 +68,43 @@ it('parks a paid order instead of failing it when nothing has room', function () ->and($result->afterSeconds)->toBe(ReserveResources::PARK_POLL_SECONDS); }); +it('keeps a park alive through the runner, not merely through the step', function () { + // Both park tests around this one call execute() directly, and that is exactly + // why this stayed invisible for the whole life of the feature. The step + // returned poll(120) correctly; the RUNNER then measured the step's own + // maxDuration from a started_at it deliberately does not reset on a poll, and + // maxDuration was 60. So every single re-entry was ruled timed out BEFORE the + // body ran, a timeout consumes an attempt, and five of them had failRun() + // marking a paid order and its instance failed and releasing the instance — + // about six minutes for a wait that promises fourteen days. The console's + // capacity queue, HostCapacity::parked(), queueDemand() and the whole "go and + // buy a server" workflow were unreachable code. + Queue::fake(); + $run = parkedOrder(); + $run->update(['status' => ProvisioningRun::STATUS_RUNNING]); + + $runner = app(RunRunner::class); + + // Four full poll intervals: further than the old sixty-second budget, and + // further than five attempts would ever have carried it. + for ($i = 0; $i < 4; $i++) { + $runner->advance($run->fresh()); + + $polled = $run->fresh(); + expect($polled->status)->toBe(ProvisioningRun::STATUS_WAITING) + ->and($polled->attempt)->toBe(0) // a poll costs nothing + ->and($polled->error)->toBeNull(); + + $this->travel(ReserveResources::PARK_POLL_SECONDS + 1)->seconds(); + } + + // Still parked, still one of the runs the console is asking somebody to buy a + // machine for, and nothing has been declared failed along the way. + expect(app(HostCapacity::class)->isParked($run->fresh()))->toBeTrue() + ->and($run->fresh()->events()->where('outcome', 'failed')->exists())->toBeFalse() + ->and($run->fresh()->events()->where('message', 'like', '%timed out%')->exists())->toBeFalse(); +}); + it('fails only once the order has waited longer than a machine can take', function () { // Long enough to buy, rack and onboard a server over a weekend; short // enough that a forgotten order surfaces instead of waiting for ever. diff --git a/tests/Feature/Admin/ProvisioningActionsTest.php b/tests/Feature/Admin/ProvisioningActionsTest.php index e9c2a00..b95b629 100644 --- a/tests/Feature/Admin/ProvisioningActionsTest.php +++ b/tests/Feature/Admin/ProvisioningActionsTest.php @@ -2,6 +2,8 @@ use App\Livewire\Admin\Provisioning; use App\Models\Host; +use App\Models\Instance; +use App\Models\Order; use App\Models\ProvisioningRun; use App\Provisioning\Jobs\AdvanceRunJob; use Illuminate\Support\Facades\Queue; @@ -27,6 +29,50 @@ it('retries a failed run from the provisioning console', function () { Queue::assertPushed(AdvanceRunJob::class); }); +it('leaves a live customer alone when the run being retried is a maintenance one', function () { + // The retry set the order to `provisioning` for ANY pipeline, and only the + // `customer` pipeline ever puts it back to `active` (CompleteProvisioning). + // So retrying a failed address, restart, storage, quota or plan-change run + // left a paying customer's order reading `provisioning` for ever: nothing in + // the product would move it again, the customer saw nothing wrong, and the + // console's view of them was simply false. Same reasoning as RunRunner's + // failRun() on the way down — a maintenance run must not condemn what it + // maintains. + Queue::fake(); + $order = Order::factory()->create(['status' => 'active']); + $instance = Instance::factory()->create([ + 'order_id' => $order->id, 'customer_id' => $order->customer_id, 'status' => 'active', + ]); + $run = ProvisioningRun::factory()->create([ + 'subject_type' => Order::class, 'subject_id' => $order->id, 'pipeline' => 'address', + 'status' => ProvisioningRun::STATUS_FAILED, 'error' => 'boom', + ]); + + Livewire::actingAs(admin(), 'operator')->test(Provisioning::class)->call('retry', $run->uuid); + + expect($run->fresh()->status)->toBe(ProvisioningRun::STATUS_RUNNING) + ->and($order->fresh()->status)->toBe('active') + ->and($instance->fresh()->status)->toBe('active'); + Queue::assertPushed(AdvanceRunJob::class); +}); + +it('does put a customer build back into provisioning when that is the run being retried', function () { + Queue::fake(); + $order = Order::factory()->create(['status' => 'failed']); + $instance = Instance::factory()->create([ + 'order_id' => $order->id, 'customer_id' => $order->customer_id, 'status' => 'failed', + ]); + $run = ProvisioningRun::factory()->create([ + 'subject_type' => Order::class, 'subject_id' => $order->id, 'pipeline' => 'customer', + 'status' => ProvisioningRun::STATUS_FAILED, 'error' => 'boom', + ]); + + Livewire::actingAs(admin(), 'operator')->test(Provisioning::class)->call('retry', $run->uuid); + + expect($order->fresh()->status)->toBe('provisioning') + ->and($instance->fresh()->status)->toBe('provisioning'); +}); + it('ignores retry on a run that is not failed', function () { Queue::fake(); $host = Host::factory()->active()->create(); diff --git a/tests/Feature/Billing/PlanChangeTest.php b/tests/Feature/Billing/PlanChangeTest.php index cf056c8..868fe0b 100644 --- a/tests/Feature/Billing/PlanChangeTest.php +++ b/tests/Feature/Billing/PlanChangeTest.php @@ -148,7 +148,13 @@ it('reads the direction from the plan rank, not from a grandfathered price', fun ]); $subscription->forceFill(['price_cents' => 9900])->saveQuietly(); // grandfathered - config()->set('provisioning.plans.team.price_cents', 17900); // today's team costs more + // The premise, asserted rather than written into config('provisioning.plans.*') + // — a key that has not existed since the catalogue moved into the database, so + // that line set nothing and this test never built the scenario it is named for. + // Today's team has to cost MORE than this customer's grandfathered business + // price, or a price-led reading of the direction would agree with the rank one + // and the test would prove nothing. + expect(Subscription::snapshotFrom('team')['price_cents'])->toBeGreaterThan(9900); $change = PlanChange::evaluate($subscription->fresh(), 'team', Illuminate\Support\Carbon::parse('2026-08-15')); @@ -184,9 +190,10 @@ it('never invoices a customer for downgrading', function () { 'current_period_start' => $start, 'current_period_end' => $start->copy()->addMonth(), ]); - // Grandfathered below today's smaller plan. + // Grandfathered below today's smaller plan — asserted, for the reason given in + // the direction test above: the config write this replaces was a no-op. $subscription->forceFill(['price_cents' => 9900])->saveQuietly(); - config()->set('provisioning.plans.team.price_cents', 17900); + expect(Subscription::snapshotFrom('team')['price_cents'])->toBeGreaterThan(9900); $credit = PlanChange::goodwillCredit($subscription->fresh(), 'team', Illuminate\Support\Carbon::parse('2026-08-15')); diff --git a/tests/Feature/CustomDomainServingTest.php b/tests/Feature/CustomDomainServingTest.php index e35ed99..3ae01af 100644 --- a/tests/Feature/CustomDomainServingTest.php +++ b/tests/Feature/CustomDomainServingTest.php @@ -7,6 +7,7 @@ use App\Models\Instance; use App\Models\Order; use App\Models\ProvisioningRun; use App\Models\User; +use App\Provisioning\RunRunner; use App\Provisioning\Steps\Customer\ConfigureDnsAndTls; use App\Provisioning\Steps\Customer\ConfigureNextcloud; use App\Services\Billing\CustomDomainAccess; @@ -44,9 +45,13 @@ function servedInstance(array $attributes = [], string $subdomain = 'berger'): a 'guest_ip' => '10.20.0.7', 'subdomain' => $subdomain, 'status' => 'active', - // The state a re-apply actually finds: built, routed, certified. + // The state a re-apply actually finds: built, routed, certified. Both + // halves of what the router file says are recorded, hostnames AND + // backend — an instance whose `routed_backend` were left null would be + // one the step has to rewrite, which is not the state being described. 'route_written' => true, 'routed_hostnames' => [platformAddress($subdomain)], + 'routed_backend' => '10.20.0.7', 'cert_ok' => true, ], $attributes)); @@ -311,6 +316,77 @@ it('does not start a second run while one is already going', function () { ->and(ProvisioningRun::query()->where('pipeline', 'address')->count())->toBe(1); }); +it('does start one beside a run that will not apply the address itself', function () { + // The guard used to stand aside for ANY run in flight, on the reasoning that + // whatever is running reads the domain state when it gets to the step. True of + // `customer` and `plan-change`, which carry both address steps. False of a + // restart, which carries neither — and nothing looks again afterwards, because + // the nightly proof check only re-applies on the FLIP from unproven to proven + // and that flip has already happened. So a domain proven while the customer's + // machine happened to be going round was announced, charged for, and never + // routed for the life of the instance. + Queue::fake(); + $fixture = servedInstance([ + 'custom_domain' => 'cloud.berger.at', 'domain_token' => 'tok', 'domain_verified_at' => now(), + ]); + + foreach (['restart', 'storage', 'quota'] as $pipeline) { + ProvisioningRun::query()->delete(); + ProvisioningRun::factory()->create([ + 'subject_type' => Order::class, + 'subject_id' => $fixture['order']->id, + 'pipeline' => $pipeline, + 'status' => ProvisioningRun::STATUS_RUNNING, + ]); + + expect(app(ReapplyInstanceAddress::class)($fixture['instance']))->not->toBeNull() + ->and(ProvisioningRun::query()->where('pipeline', 'address')->count())->toBe(1); + } + + // And still stands aside for a plan change, which reuses these two steps and + // will read the same domain state when it reaches them. + ProvisioningRun::query()->delete(); + ProvisioningRun::factory()->create([ + 'subject_type' => Order::class, + 'subject_id' => $fixture['order']->id, + 'pipeline' => 'plan-change', + 'status' => ProvisioningRun::STATUS_RUNNING, + ]); + + expect(app(ReapplyInstanceAddress::class)($fixture['instance']))->toBeNull(); +}); + +it('keeps the domain unannounced when the run fails before it has been routed', function () { + // The half-finished address, which is the ordinary outcome of a failed run. + // With the router written first, a run that got the certificate and then fell + // over at Nextcloud left the customer's own domain serving Nextcloud's + // untrusted-domain error page under a valid certificate — and `domain_cert_ok` + // was already true, so the portal was printing that address as live. + Queue::fake(); + $s = fakeServices(); + $fixture = servedInstance([ + 'custom_domain' => 'cloud.berger.at', 'domain_token' => 'tok', 'domain_verified_at' => now(), + ]); + + // Nextcloud refuses the trusted_domains write; guest() throws, which the + // runner turns into a retry, so the run never reaches the router at all. + $s['pve']->guestScript('config:system:set trusted_domains', 1); + + $run = addressRun($fixture); + $run->update(['status' => ProvisioningRun::STATUS_RUNNING, 'current_step' => 0]); + app(RunRunner::class)->advance($run); + + $instance = $fixture['instance']->fresh(); + + expect($run->fresh()->status)->toBe(ProvisioningRun::STATUS_WAITING) + ->and($run->fresh()->current_step)->toBe(0) + // Nothing announced, nothing routed: the customer is still on the platform + // address, which is the honest half to be left holding. + ->and($instance->domain_cert_ok)->toBeFalse() + ->and($instance->routed_hostnames)->toBe([platformAddress()]) + ->and($s['traefik']->serves('berger', 'cloud.berger.at'))->toBeFalse(); +}); + it('stops serving the old domain the moment the customer changes or clears it', function () { Queue::fake(); diff --git a/tests/Feature/DeploymentRunsAsTheAppUserTest.php b/tests/Feature/DeploymentRunsAsTheAppUserTest.php index e3d8e67..94836c3 100644 --- a/tests/Feature/DeploymentRunsAsTheAppUserTest.php +++ b/tests/Feature/DeploymentRunsAsTheAppUserTest.php @@ -1,9 +1,10 @@ toBe([]); }); +it('never runs occ in a customer guest as whoever docker felt like either', function () { + // The same default, the same lesson, a different subsystem — and it was learned + // here second. `docker compose exec` is root, the official Nextcloud image's + // console.php exits 1 unless the caller owns config/config.php (www-data), and + // the prefix was pasted into five separate files, so every occ call in the + // product was failing on every instance. The ones that go through + // CustomerStep::guest() turned that into a retry and then a failed run for a + // machine that was otherwise finished. + // + // So there is one builder, and this refuses a second: nothing in app/ may spell + // the invocation out by hand, which is the only way a sixth call site can be + // stopped from getting it wrong again. + $offenders = []; + + foreach (File::allFiles(app_path()) as $file) { + if ($file->getExtension() !== 'php' + || $file->getRelativePathname() === 'Support/NextcloudOcc.php') { + continue; + } + + if (str_contains(File::get($file->getRealPath()), 'docker compose exec')) { + $offenders[] = $file->getRelativePathname(); + } + } + + expect($offenders)->toBe([]) + // And the one builder names the user Nextcloud insists on. + ->and(NextcloudOcc::command('status')) + ->toContain('docker compose exec -T -u www-data '); +}); + it('repairs ownership before it needs it, not after', function () { // A server already carrying the damage has to heal on its next deployment. // Nothing else ever will: a root-owned log file stays root-owned, and the diff --git a/tests/Feature/Provisioning/CustomerProvisioningEndToEndTest.php b/tests/Feature/Provisioning/CustomerProvisioningEndToEndTest.php index 404c755..a0b8ea9 100644 --- a/tests/Feature/Provisioning/CustomerProvisioningEndToEndTest.php +++ b/tests/Feature/Provisioning/CustomerProvisioningEndToEndTest.php @@ -46,7 +46,16 @@ it('provisions a paid order all the way to active (mocked)', function () { ->and($instance->status)->toBe('active') ->and($instance->vmid)->not->toBeNull() ->and($instance->cert_ok)->toBeTrue() - ->and($instance->onboardingTasks()->count())->toBeGreaterThan(0); + ->and($instance->onboardingTasks()->count())->toBeGreaterThan(0) + // The storage allowance was DELIVERED, not merely walked past. This test + // covered all sixteen steps and asserted nothing about the quota one, so it + // proved that inserting the step did not break the pipeline rather than + // that the step does anything — and quota_applied_gb is written only after + // the guest accepted the figure, which is the whole distinction between an + // enforced allowance and a number in our database. + ->and($instance->quota_applied_gb)->toBe($instance->quota_gb) + ->and($instance->quota_applied_gb)->toBeGreaterThan(0) + ->and($s['pve']->guestRan('config:app:set files default_quota'))->toBeTrue(); // Every external resource created exactly once. foreach (['instance_id', 'vmid', 'dns_record_id', 'nc_admin', 'backup_job_id', 'monitoring_target_id'] as $kind) { diff --git a/tests/Feature/Provisioning/CustomerStepsTest.php b/tests/Feature/Provisioning/CustomerStepsTest.php index 82efeaf..1caf1c1 100644 --- a/tests/Feature/Provisioning/CustomerStepsTest.php +++ b/tests/Feature/Provisioning/CustomerStepsTest.php @@ -5,7 +5,9 @@ use App\Models\Instance; use App\Models\Order; use App\Models\ProvisioningRun; use App\Models\RunResource; +use App\Models\Subscription; use App\Notifications\CloudReady; +use App\Provisioning\RunRunner; use App\Provisioning\Steps\Customer\CloneVirtualMachine; use App\Provisioning\Steps\Customer\CompleteProvisioning; use App\Provisioning\Steps\Customer\ConfigureCloudInit; @@ -64,9 +66,19 @@ it('validates a paid order and moves it to provisioning', function () { ->and($run->subject->fresh()->status)->toBe('provisioning'); }); -it('fails validation for an unpaid order or unknown plan', function () { - expect(app(ValidateOrder::class)->execute(orderRun(['status' => 'pending']))->type)->toBe('fail'); - expect(app(ValidateOrder::class)->execute(orderRun(['status' => 'paid', 'plan' => 'ghost']))->type)->toBe('fail'); +it('fails validation for an unpaid order, and for a paid one with no contract', function () { + // Named for what the step actually checks. There is no plan check in + // ValidateOrder at all — an unknown plan fails because OpenSubscription could + // not price it and so froze no contract, which is a different sentence and the + // one that matters: nothing is built from today's catalogue. Asserting the + // reason is what keeps the two apart; on ->type alone this passed either way. + $unpaid = app(ValidateOrder::class)->execute(orderRun(['status' => 'pending'])); + expect($unpaid->type)->toBe('fail') + ->and($unpaid->reason)->toBe('order_not_paid'); + + $ghost = app(ValidateOrder::class)->execute(orderRun(['status' => 'paid', 'plan' => 'ghost'])); + expect($ghost->type)->toBe('fail') + ->and($ghost->reason)->toBe('no_subscription'); }); // 2. ReserveResources @@ -150,18 +162,39 @@ it('recovers a lost-task clone by waiting for the lock then re-cloning cleanly', ->and(RunResource::where('run_id', $run->id)->where('kind', 'vmid')->exists())->toBeFalse(); }); -it('fails clone when the plan has no template', function () { +it('fails clone when the contract carries no template', function () { + // This used to write config('provisioning.plans.start.template_vmid') — a key + // that has not existed since the catalogue moved into the database, so the + // write was a no-op and the run failed for want of a CONTRACT instead. The + // case the test is named for was never exercised. + // + // The template comes off the frozen snapshot, so the state to build is a + // contract with no blueprint on it: what a pre-catalogue subscription looks + // like before the backfill fills it in. PlanCatalogue::publish() has refused + // to put a version on sale without one ever since, and this is the failure + // that refusal exists to keep away from a paying customer. fakeServices(); $host = Host::factory()->active()->create(['node' => 'pve']); $order = Order::factory()->create(['plan' => 'start']); $instance = Instance::factory()->create(['order_id' => $order->id, 'customer_id' => $order->customer_id, 'host_id' => $host->id]); + Subscription::factory()->create([ + 'customer_id' => $order->customer_id, + 'order_id' => $order->id, + 'instance_id' => $instance->id, + 'template_vmid' => null, + ]); $run = ProvisioningRun::factory()->create([ 'subject_type' => Order::class, 'subject_id' => $order->id, 'pipeline' => 'customer', 'context' => ['instance_id' => $instance->id, 'node' => 'pve'], ]); - config()->set('provisioning.plans.start.template_vmid', 0); - expect(app(CloneVirtualMachine::class)->execute($run)->type)->toBe('fail'); + $result = app(CloneVirtualMachine::class)->execute($run); + + expect($result->type)->toBe('fail') + ->and($result->reason)->toBe('template_missing') + // It is the template that is missing and not the contract — otherwise this + // is the previous test over again under a misleading name. + ->and($order->subscription()->exists())->toBeTrue(); }); // 4. ConfigureCloudInit @@ -346,13 +379,74 @@ it('passes acceptance only when everything is genuinely green', function () { config()->set('provisioning.monitoring.required', false); $s['monitoring']->healthy = true; - // Nextcloud not installed → fail + // A probe that says no asks again rather than condemning a finished cloud: + // certReachable() returns the same false for a connect timeout as for a + // missing certificate, and this step returning fail() bypassed the retry + // budget entirely — one bad second ended a working Nextcloud as a failed + // order with the instance released. $s['pve']->guestScript('occ status', 0, '{"installed":false}'); - expect(app(RunAcceptanceChecks::class)->execute($run->fresh())->type)->toBe('fail'); + $nextcloud = app(RunAcceptanceChecks::class)->execute($run->fresh()); + expect($nextcloud->type)->toBe('retry') + ->and($nextcloud->reason)->toBe('acceptance_failed:nextcloud'); - // cert not reachable → fail $s['traefik']->certReady = false; - expect(app(RunAcceptanceChecks::class)->execute($run->fresh())->type)->toBe('fail'); + $cert = app(RunAcceptanceChecks::class)->execute($run->fresh()); + expect($cert->type)->toBe('retry') + ->and($cert->reason)->toBe('acceptance_failed:cert'); +}); + +it('still fails for good when what is wrong is a fact no retry can change', function () { + // The two checks that stay terminal read state THIS run wrote earlier, out of + // our own database. Asking again five times only delays saying so. + $s = fakeServices(); + $s['pve']->guestScript('occ status', 0, '{"installed":true,"maintenance":false}'); + ['run' => $run, 'instance' => $instance, 'host' => $host] = reservedRun([], ['cert_ok' => true]); + $instance->monitoringTargets()->create(['external_id' => 'mon-1', 'url' => 'https://x/status.php', 'status' => 'up']); + + // No backup breadcrumb: RegisterBackup never registered a job. + $missingBackup = app(RunAcceptanceChecks::class)->execute($run->fresh()); + expect($missingBackup->type)->toBe('fail') + ->and($missingBackup->reason)->toBe('acceptance_failed:backup'); + + RunResource::create(['run_id' => $run->id, 'host_id' => $host->id, 'kind' => 'backup_job_id', 'external_id' => 'b']); + + // No platform certificate — which ConfigureDnsAndTls fails the run over + // itself, so standing here without it is a state the pipeline cannot reach. + $instance->update(['cert_ok' => false]); + $missingCert = app(RunAcceptanceChecks::class)->execute($run->fresh()); + expect($missingCert->type)->toBe('fail') + ->and($missingCert->reason)->toBe('acceptance_failed:cert'); +}); + +it('spends the retry budget and then fails with the reason the probe gave', function () { + // The whole point of retrying instead of failing: five noes in a row is not a + // blip, and the run still ends failed — named for what was wrong, not + // 'step timed out'. + $s = fakeServices(); + $s['pve']->guestScript('occ status', 0, '{"installed":false}'); + ['run' => $run, 'instance' => $instance, 'host' => $host] = reservedRun([], ['cert_ok' => true]); + RunResource::create(['run_id' => $run->id, 'host_id' => $host->id, 'kind' => 'backup_job_id', 'external_id' => 'b']); + $instance->monitoringTargets()->create(['external_id' => 'mon-1', 'url' => 'https://x/status.php', 'status' => 'up']); + + $index = array_search(RunAcceptanceChecks::class, config('provisioning.pipelines.customer'), true); + $run->update(['current_step' => $index, 'status' => 'running', 'max_attempts' => 3]); + + $runner = app(RunRunner::class); + for ($i = 0; $i < 5; $i++) { + // Re-read before writing. Saving the stale model would put the attempt + // count back to what it was and the loop would never reach the budget. + $run = $run->fresh(); + + if ($run->status === 'failed') { + break; + } + + $run->forceFill(['next_attempt_at' => null, 'started_at' => now()])->save(); + $runner->advance($run); + } + + expect($run->fresh()->status)->toBe('failed') + ->and($run->fresh()->error)->toBe('acceptance_failed:nextcloud'); }); // 15. CompleteProvisioning diff --git a/tests/Feature/Provisioning/InstanceRestartTest.php b/tests/Feature/Provisioning/InstanceRestartTest.php index 656673f..06f447d 100644 --- a/tests/Feature/Provisioning/InstanceRestartTest.php +++ b/tests/Feature/Provisioning/InstanceRestartTest.php @@ -13,6 +13,7 @@ use App\Models\User; use App\Provisioning\RunRunner; use App\Provisioning\Steps\Customer\CompleteRestart; use App\Provisioning\Steps\Customer\ShutDownVirtualMachine; +use App\Support\ProvisioningSettings; use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; @@ -63,6 +64,11 @@ function restartableInstance(array $instanceAttributes = []): array $pve->startVm('pve', 101); $pve->setCloudInit('pve', 101, ['cores' => 8, 'memory' => 16384]); + // The address the guest comes back on. The restart pipeline reads it — a + // cold boot is the one event that can move a DHCP lease — and would poll for + // it until the step's deadline if the fake answered nothing. + $pve->guestScript('hostname -I', 0, '10.20.0.7'); + return compact('services', 'pve', 'host', 'order', 'instance'); } @@ -185,13 +191,18 @@ it('leaves the pending restart alone until the very last step of the run', funct $run = app(RestartInstance::class)($instance); $runner = app(RunRunner::class); - // Shutdown asked, shutdown observed, machine started, agent answered — and - // through all of it the flag stands, because none of those is the machine - // running on what was bought. - for ($i = 0; $i < 6; $i++) { + // Read off the pipeline rather than written out, so inserting a step cannot + // quietly turn "the last one" into "the one before it" and leave this test + // green while proving something weaker. + $last = count((array) config('provisioning.pipelines.restart')) - 1; + + // Shutdown asked, shutdown observed, machine started, agent answered, address + // read — and through all of it the flag stands, because none of those is the + // machine running on what was bought. + for ($i = 0; $i < 10; $i++) { $run->refresh(); - if ($run->current_step >= 3) { + if ($run->current_step >= $last) { break; } @@ -201,7 +212,7 @@ it('leaves the pending restart alone until the very last step of the run', funct expect($instance->fresh()->restartIsPending())->toBeTrue(); } - expect($run->refresh()->current_step)->toBe(3) + expect($run->refresh()->current_step)->toBe($last) ->and($instance->fresh()->restartIsPending())->toBeTrue(); driveRun($run); @@ -314,6 +325,39 @@ it('refuses the console action to an operator who does not hold the capability', expect(ProvisioningRun::query()->where('pipeline', 'restart')->count())->toBe(0); }); +it('follows the guest when a restart brings it back on another address', function () { + // cloud-init hands every guest ip=dhcp, so the address is a lease and not a + // property of the machine. The router file on the serving host names it as the + // backend — so a machine that comes back somewhere else, with nothing + // rewriting that file, is a customer's cloud answering 502 for the rest of its + // life. Nothing in the product re-read the address at all, and the route guard + // compared only the hostname list, so neither half could notice. + Queue::fake(); + ['pve' => $pve, 'instance' => $instance] = restartableInstance([ + // What the router was last told, and where the machine actually is now. + 'routed_backend' => '10.20.0.7', + 'route_written' => true, + 'routed_hostnames' => ['berger.'.ProvisioningSettings::dnsZone()], + 'subdomain' => 'berger', + 'cert_ok' => true, + ]); + $pve->guestScript('hostname -I', 0, '10.20.0.44 fe80::1'); + $this->actingAs(admin(), 'operator'); + + driveRun(app(RestartInstance::class)($instance)); + + expect($instance->fresh()->guest_ip)->toBe('10.20.0.44') + // An address run, because writing the router is that pipeline's job. It is + // allowed to start beside the restart because the guard asks whether the + // run in flight carries out the address steps, and a restart carries + // neither of them. + ->and(ProvisioningRun::query()->where('pipeline', 'address')->count())->toBe(1); + + driveRun(ProvisioningRun::query()->where('pipeline', 'address')->sole()); + + expect($instance->fresh()->routed_backend)->toBe('10.20.0.44'); +}); + it('gives the shutdown step nothing to do when the guest is already stopped', function () { Queue::fake(); ['pve' => $pve, 'instance' => $instance, 'order' => $order] = restartableInstance(); diff --git a/tests/Feature/Provisioning/PipelineRegistryTest.php b/tests/Feature/Provisioning/PipelineRegistryTest.php index ebee57d..12879aa 100644 --- a/tests/Feature/Provisioning/PipelineRegistryTest.php +++ b/tests/Feature/Provisioning/PipelineRegistryTest.php @@ -1,6 +1,11 @@ [FakeAdvanceStep::class]]))->resolve('host', 5); })->throws(InvalidArgumentException::class); + +it('answers whether a run still to come carries out another pipeline whole', function () { + // The question an action about to start a run has to ask about the run already + // in flight. Asking merely "is something running?" is what made a domain + // proven during a restart never get routed, and a storage pack booked during + // an address run get charged monthly and never delivered. + $registry = new PipelineRegistry([ + 'both' => [FakeAdvanceStep::class, FakeRetryStep::class], + 'first' => [FakeAdvanceStep::class], + 'second' => [FakeRetryStep::class], + ]); + + // Everything still ahead of it. + expect($registry->stillCovers('both', 0, 'first'))->toBeTrue() + ->and($registry->stillCovers('both', 0, 'second'))->toBeTrue() + // EVERY step, not any: half of an ordered piece of work is worse than + // none of it — a quota enforced over a filesystem that never grew. + ->and($registry->stillCovers('first', 0, 'both'))->toBeFalse() + // Already past it: a run standing at its second step will not go back and + // do its first, which is the case a name comparison cannot see at all. + ->and($registry->stillCovers('both', 1, 'first'))->toBeFalse() + ->and($registry->stillCovers('both', 1, 'second'))->toBeTrue() + // The step it is standing on counts as still to come — it is about to run, + // or running, and every step reads the state when it gets there. + ->and($registry->stillCovers('second', 0, 'second'))->toBeTrue(); +}); + +it('asks that question of the runs actually in flight against an order', function () { + $order = Order::factory()->create(); + $work = app(WorkInFlight::class); + + $running = fn (string $pipeline) => ProvisioningRun::factory()->create([ + 'subject_type' => Order::class, 'subject_id' => $order->id, + 'pipeline' => $pipeline, 'status' => ProvisioningRun::STATUS_RUNNING, + ]); + + expect($work->covers($order, 'address'))->toBeFalse() + ->and($work->covers($order, 'storage'))->toBeFalse(); + + // A quota run writes the figure Nextcloud enforces and nothing else — it does + // not grow the disk that figure is a promise about, so it does not stand for a + // storage change. This is the pair that had a booked pack charged every month + // and never delivered. + $running('quota'); + expect($work->covers($order, 'storage'))->toBeFalse() + ->and($work->covers($order, 'address'))->toBeFalse(); + + // A plan change carries both pipelines whole; there is genuinely nothing to add. + ProvisioningRun::query()->delete(); + $running('plan-change'); + expect($work->covers($order, 'storage'))->toBeTrue() + ->and($work->covers($order, 'address'))->toBeTrue(); + + // A run that has finished covers nothing, however much it contained. + ProvisioningRun::query()->update(['status' => ProvisioningRun::STATUS_COMPLETED]); + expect($work->covers($order, 'storage'))->toBeFalse() + ->and($work->covers($order, 'address'))->toBeFalse(); + + // A pipeline that no longer exists — renamed by a deploy while a run was in + // flight — covers nothing either. Refusing to start the work over it would + // lose the work as well as the run. + ProvisioningRun::query()->delete(); + $running('gone-in-a-deploy'); + expect($work->covers($order, 'address'))->toBeFalse(); +}); + +it('lays the maintenance pipelines out so a half-finished address is a safe one', function () { + // Nextcloud BEFORE the router, in the address pipeline and in the plan change + // that reuses it. Router first meant a run could get the certificate and then + // fail at Nextcloud — leaving the customer's own domain serving Nextcloud's + // untrusted-domain error under a valid certificate, with domain_cert_ok true + // so the portal called it live. The reasoning for both directions is written + // out above the `address` pipeline in config/provisioning.php. + $nextcloud = ConfigureNextcloud::class; + $dns = ConfigureDnsAndTls::class; + + foreach (['address', 'plan-change', 'customer'] as $pipeline) { + $steps = (array) config('provisioning.pipelines.'.$pipeline); + + expect(array_search($nextcloud, $steps, true)) + ->toBeLessThan(array_search($dns, $steps, true)); + } +}); diff --git a/tests/Feature/Provisioning/RunRunnerTest.php b/tests/Feature/Provisioning/RunRunnerTest.php index f364741..5dd2516 100644 --- a/tests/Feature/Provisioning/RunRunnerTest.php +++ b/tests/Feature/Provisioning/RunRunnerTest.php @@ -175,6 +175,31 @@ it('re-executes a step after a timeout retry instead of timing out again', funct expect($run->fresh()->status)->toBe('completed'); }); +it('starts the clock afresh for a run that has been put back to pending', function () { + // A revived run — see StartCustomerProvisioning::reviveRunStrandedWithout- + // AContract(), which resets the status, the step, the attempt and the error + // when the contract a payment was missing finally appears. It does not reset + // started_at, and the clause here that was written for exactly this case used + // `??=`, so it never fired: the run kept the started_at of the attempt that + // failed hours earlier, its very first pass was ruled timed out before the + // step body ran, and that spent one of five attempts and wrote 'step timed + // out' into the log of a run that had not started. + Queue::fake(); + bindPipeline(['test' => [FakeAdvanceStep::class]]); + $run = ProvisioningRun::factory()->create([ + 'pipeline' => 'test', + 'status' => ProvisioningRun::STATUS_PENDING, + 'started_at' => now()->subDays(2), + 'max_attempts' => 5, + ]); + + app(RunRunner::class)->advance($run); + + expect($run->fresh()->status)->toBe('completed') + ->and($run->fresh()->attempt)->toBe(0) + ->and($run->fresh()->events()->where('message', 'like', '%timed out%')->exists())->toBeFalse(); +}); + it('fails terminally when the pipeline step cannot be resolved', function () { bindPipeline(['test' => [FakeAdvanceStep::class]]); $run = ProvisioningRun::factory()->create(['pipeline' => 'test', 'current_step' => 9]); diff --git a/tests/Feature/Provisioning/ServicesTest.php b/tests/Feature/Provisioning/ServicesTest.php index dc18551..9890816 100644 --- a/tests/Feature/Provisioning/ServicesTest.php +++ b/tests/Feature/Provisioning/ServicesTest.php @@ -9,6 +9,7 @@ use App\Services\Ssh\CommandResult; use App\Services\Ssh\FakeRemoteShell; use App\Services\Wireguard\FakeWireguardHub; use App\Services\Wireguard\LocalWireguardHub; +use App\Support\ProvisioningSettings; use Illuminate\Support\Facades\File; use Illuminate\Support\Facades\Http; @@ -119,6 +120,52 @@ it('treats an already-deleted DNS record as done', function () { (new HttpHetznerDnsClient)->deleteRecord('rec-123'); })->throwsNoExceptions(); +it('finds an existing DNS record past the first page instead of creating a second one', function () { + // Hetzner pages `GET /records` at a hundred entries, and this asked only for + // the first page. Past a hundred records the lookup missed an entry that was + // plainly there, fell through to POST, and Hetzner accepted a SECOND A record + // for the same name: two addresses round-robined for one instance, one of them + // a host the machine is not on, so the cloud was up about half the time. The + // `address` and `plan-change` pipelines upsert on every run, so it compounded. + $filler = fn (int $from) => collect(range($from, $from + 99)) + ->map(fn (int $i) => ['id' => 'rec-'.$i, 'name' => 'other-'.$i, 'type' => 'A']) + ->all(); + + // Matched by reading the page out of the query rather than with a URL glob: + // `page=1` is a substring of `per_page=100`, so a pattern would answer page + // two with page one's records and the test would pass against the bug. + Http::fake(function ($request) use ($filler) { + parse_str((string) parse_url($request->url(), PHP_URL_QUERY), $query); + + if (str_contains($request->url(), '/zones')) { + return Http::response(['zones' => [['id' => 'zone-1']]]); + } + + if ($request->method() === 'GET') { + $page = (int) ($query['page'] ?? 1); + + return Http::response([ + 'records' => $page === 2 + ? array_merge($filler(101), [['id' => 'rec-berger', 'name' => 'berger', 'type' => 'A']]) + : $filler(1), + 'meta' => ['pagination' => ['page' => $page, 'per_page' => 100, 'last_page' => 2]], + ]); + } + + // A POST is the bug: the record exists and has to be updated. + return Http::response(['record' => ['id' => 'rec-new']], 201); + }); + + $id = (new HttpHetznerDnsClient)->upsertRecord( + 'berger.'.ProvisioningSettings::dnsZone(), 'A', '203.0.113.9' + ); + + expect($id)->toBe('rec-berger'); + Http::assertSent(fn ($request) => $request->method() === 'PUT' + && str_contains($request->url(), '/records/rec-berger')); + Http::assertNotSent(fn ($request) => $request->method() === 'POST'); +}); + it('writes and removes a real hostsdir entry on disk (FileHostDnsDirectory)', function () { $dir = sys_get_temp_dir().'/clupilot-dns-hosts-test-'.uniqid(); config()->set('provisioning.dns.hosts_dir', $dir); diff --git a/tests/Feature/Provisioning/SubscriptionSnapshotTest.php b/tests/Feature/Provisioning/SubscriptionSnapshotTest.php index 15383a6..b5127b1 100644 --- a/tests/Feature/Provisioning/SubscriptionSnapshotTest.php +++ b/tests/Feature/Provisioning/SubscriptionSnapshotTest.php @@ -4,12 +4,14 @@ use App\Actions\OpenSubscription; use App\Models\Host; use App\Models\Instance; use App\Models\Order; +use App\Models\PlanVersion; use App\Models\ProvisioningRun; use App\Models\Subscription; use App\Provisioning\Jobs\AdvanceRunJob; use App\Provisioning\Steps\Customer\CloneVirtualMachine; use App\Provisioning\Steps\Customer\ReserveResources; use App\Provisioning\Steps\Customer\ValidateOrder; +use App\Services\Billing\PlanCatalogue; use App\Services\Proxmox\ProxmoxClient; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Queue; @@ -38,6 +40,48 @@ function paidOrderWithSubscription(string $plan = 'team'): array return compact('host', 'order', 'subscription', 'run'); } +/** + * The operator changes what a plan is, after somebody has already bought it. + * + * Publishing a successor and closing the old window is the ONLY way the catalogue + * can move: a published version is immutable, precisely so that what a customer + * bought keeps describing what they are owed. So it is also the only way this + * regression can be constructed — and the tests below used to try it with + * config('provisioning.plans.*') writes, against a key that has not existed since + * the catalogue moved into plan_families/plan_versions/plan_prices. Every one of + * those writes was a no-op, so each test proved only that the contract agrees with + * itself. + * + * @param array $changes + */ +function publishSuccessor(string $plan, array $changes): PlanVersion +{ + $catalogue = app(PlanCatalogue::class); + $current = $catalogue->currentVersion($plan); + $currency = Subscription::catalogueCurrency(); + $price = $current->priceFor(Subscription::TERM_MONTHLY)->amount_cents; + + // Half-open windows: the old one ends at the instant the new one starts, so + // there is neither a gap nor two versions on sale at once. + $catalogue->schedule($current, $current->available_from, now()); + + $successor = PlanVersion::query()->create([ + ...$current->only([ + 'plan_family_id', 'quota_gb', 'traffic_gb', 'seats', 'ram_mb', + 'cores', 'disk_gb', 'performance', 'template_vmid', + ]), + ...$changes, + 'version' => $current->version + 1, + 'features' => $current->features, + 'available_from' => now(), + ]); + + $successor->prices()->create(['term' => 'monthly', 'amount_cents' => $price, 'currency' => $currency]); + $successor->prices()->create(['term' => 'yearly', 'amount_cents' => $price * 12, 'currency' => $currency]); + + return $catalogue->publish($successor, now()); +} + it('freezes the catalogue onto a subscription when the checkout is paid', function () { Queue::fake(); @@ -93,11 +137,15 @@ it('opens exactly one contract when Stripe retries the webhook', function () { it('sizes the machine from the contract, not from a catalogue edited afterwards', function () { ['order' => $order, 'run' => $run] = paidOrderWithSubscription('team'); - // The operator shrinks the team plan after this customer has paid for it. - config()->set('provisioning.plans.team.ram_mb', 4096); - config()->set('provisioning.plans.team.cores', 2); - config()->set('provisioning.plans.team.disk_gb', 120); - config()->set('provisioning.plans.team.quota_gb', 100); + // The operator cuts the team plan back after this customer has paid for it. + publishSuccessor('team', ['ram_mb' => 4096, 'cores' => 2, 'disk_gb' => 120, 'quota_gb' => 100]); + + // The premise, asserted and not assumed. This edit used to be four + // config('provisioning.plans.team.*') writes — a key that has not existed + // since the catalogue moved into the database — so the catalogue was never + // touched at all and the test could not detect the regression it exists for. + expect(Subscription::snapshotFrom('team')['ram_mb'])->toBe(4096) + ->and(Subscription::snapshotFrom('team')['disk_gb'])->toBe(120); expect(app(ReserveResources::class)->execute($run)->type)->toBe('advance'); @@ -128,8 +176,12 @@ it('clones the template the contract was sold with, not a later one', function ( ]); $run->mergeContext(['instance_id' => $instance->id, 'host_id' => $host->id, 'node' => 'pve']); - // A new blueprint is published after this customer bought. - config()->set('provisioning.plans.team.template_vmid', 9999); + // A new blueprint is published after this customer bought — the real thing, + // a successor version, because a published version's template is frozen with + // it. The line here was a config write against a key that no longer exists. + publishSuccessor('team', ['template_vmid' => 9999]); + + expect(Subscription::snapshotFrom('team')['template_vmid'])->toBe(9999); $cloned = null; $pve = Mockery::mock(ProxmoxClient::class);