merge main

Author: zeeshanlakhani

.claude/skills/add-api-version/SKILL.md

@@ -0,0 +1,35 @@
+---
+name: add-api-version
+description: Add a new version to a Dropshot HTTP API. Use when the user wants to make versioned changes to a Dropshot-managed HTTP API, such as adding, making changes to, or removing an API endpoint.
+---
+
+# Add API version
+
+Add a new version to a Dropshot API in this repository.
+
+## Instructions
+
+Follow these steps in order. Do not skip ahead.
+
+### Step 1: Fetch the guide
+
+Fetch the guide from the dropshot-api-manager repository:
+
+```bash
+curl -fsSL https://raw.githubusercontent.com/oxidecomputer/dropshot-api-manager/refs/heads/main/guides/new-version.md
+```
+
+**Do not summarize the guide.** Read and follow it exactly as written. Remember to fetch RFD 619 for context.
+
+### Step 2: Ascertain the scope of the request
+
+If not already provided, ask the user:
+
+* Which API they would like to change.
+* What changes to make.
+
+### Step 3: Make the change
+
+Follow the guide carefully and systematically to add a new version to the provided API which makes the given changes.
+
+Follow the guide step by step; do not skip any steps.

.claude/skills/api-docs-lint/SKILL.md

@@ -0,0 +1,133 @@
+# API Docstring Style Guide
+
+This guide covers public docstrings (`///`) on endpoint handlers in
+`nexus/external-api/src/lib.rs` and on structs, fields, and enums in
+`nexus/types/src/external_api/`. These docstrings become user-facing API
+documentation.
+
+## Capitalization
+
+Start all docstrings with a capital letter.
+
+```rust
+// Bad
+/// the source of an identity provider metadata descriptor
+pub idp_metadata_source: IdpMetadataSource,
+
+// Good
+/// The source of an identity provider metadata descriptor.
+pub idp_metadata_source: IdpMetadataSource,
+```
+
+## Punctuation
+
+Short, label-like descriptions do not need periods. This includes endpoint
+summary lines, struct-level descriptions, and brief field annotations. Longer
+explanatory text (especially multi-clause or multi-sentence docstrings) should
+end sentences with periods.
+
+```rust
+// Endpoint summary lines: no period
+/// Fetch silo
+/// List identity providers for silo
+/// Create IP pool
+
+// Struct-level descriptions: no period
+/// View of a Silo
+/// A collection of resource counts used to describe capacity and utilization
+
+// Short field descriptions: no period
+/// Number of virtual CPUs
+/// Common identifying metadata
+/// Size of blocks in bytes
+
+// Longer explanatory text: use periods
+/// Accounts for resources allocated to running instances or storage
+/// allocated via disks or snapshots.
+///
+/// Note that CPU and memory resources associated with stopped instances
+/// are not counted here, whereas associated disks will still be counted.
+```
+
+## Articles
+
+Omit articles ("a", "an", "the") in endpoint summary lines. Look at existing
+endpoints in `nexus/external-api/src/lib.rs` for reference. A few examples of
+the different shapes:
+
+```rust
+// Bad
+/// Fetch a silo
+/// Create an IP pool
+/// Delete the project
+/// List the silos
+
+// Good
+/// Fetch silo
+/// Create IP pool
+/// Delete project
+/// List silos
+/// Fetch resource utilization for user's current silo
+/// List identity providers for silo
+/// Add range to IP pool
+```
+
+## Paragraph Separation
+
+Separate distinct thoughts or notes with a blank `///` line. This produces
+proper paragraph breaks in rendered documentation.
+
+```rust
+// Bad - renders as one run-on paragraph
+/// A unique, immutable, system-controlled identifier for the token.
+/// Note that this ID is not the bearer token itself, which starts with
+/// "oxide-token-".
+pub id: Uuid,
+
+// Good - renders as two paragraphs
+/// A unique, immutable, system-controlled identifier for the token.
+///
+/// Note that this ID is not the bearer token itself, which starts with
+/// "oxide-token-".
+pub id: Uuid,
+```
+
+## Acronyms and Abbreviations
+
+Use standard casing for acronyms:
+  - "IdP" (Identity Provider)
+  - "SP" (Service Provider)
+  - "SAML", "ID", "IP", "VPC", "CPU", "RAM", "DER"
+
+## Doc Comments vs Regular Comments
+
+Use `///` for public API documentation that users will see. Use `//` for
+internal implementation notes that should not appear in generated docs.
+
+```rust
+// Bad - this won't appear in API docs
+// A list containing the IDs of the secret keys.
+pub secrets: Vec<WebhookSecret>,
+
+// Good - this will appear in API docs
+/// A list containing the IDs of the secret keys.
+pub secrets: Vec<WebhookSecret>,
+```
+
+## Scope
+
+These rules apply to:
+- Endpoint handler docstrings (`/// Fetch silo`)
+- Public struct docstrings (`/// View of a Silo`)
+- Public field docstrings (`/// The IP address held by this resource.`)
+- Public enum variant docstrings (`/// The sled is currently active.`)
+
+These rules do NOT apply to:
+- Private functions or structs
+- Internal comments (`//`)
+- Module-level documentation (`//!`)
+
+## General
+
+Follow standard English grammatical rules: correct articles ("a" vs "an"),
+subject-verb agreement, proper spelling, etc.

.claude/skills/crdb-change/SKILL.md

@@ -0,0 +1,103 @@
+---
+name: crdb-change
+description: Generate CockroachDB SQL and migrations for schema changes. Use when creating migrations, updating the database schema, or when the user mentions migrations, schema changes, or dbinit.sql.
+---
+
+# CockroachDB database changes
+
+Generate database changes for this repository. This includes changes to:
+
+- `schema/crdb/dbinit.sql`
+- Migrations for changes
+
+## Instructions
+
+Follow these steps in order. Do not skip ahead.
+
+### Step 1: Ascertain the scope of the request
+
+If not already provided, prompt the user whether they'd like to:
+
+- **Perform both a dbinit.sql change and a migration**: If this is the case, then:
+  1. Ask the user to describe the changes they'd like to perform to `dbinit.sql`.
+  2. Make those changes, asking the user followup questions as necessary.
+  3. Go directly to step 3, with the scope of the migration being to cover those changes.
+- **Write migrations for pre-existing changes**: In this case, changes need to be fetched from version control. Go to step 2.
+
+### Step 2: Get the diff
+
+Check if `.jj` exists in the repository to determine whether to use jj or git commands.
+
+Prompt the user to ask where the schema changes are:
+
+- **Uncommitted changes**: Changes not yet committed.
+  - git: `git diff -- schema/crdb/dbinit.sql` (unstaged) or `git diff --cached -- schema/crdb/dbinit.sql` (staged)
+  - jj: `jj diff -- schema/crdb/dbinit.sql`
+
+- **This commit only** (stacked diff workflow): Changes are in the current commit only.
+  - git: `git diff HEAD^ -- schema/crdb/dbinit.sql`
+  - jj: `jj diff --from @-- -- schema/crdb/dbinit.sql`
+
+- **This branch** (feature branch workflow): Changes span the entire branch.
+  - git: `git diff $(git merge-base HEAD main) -- schema/crdb/dbinit.sql`
+  - jj: `jj diff --from 'fork_point(trunk() | @)' -- schema/crdb/dbinit.sql`
+
+If the diff doesn't show anything, ask the user which ref to diff from.
+
+### Step 3: Create migration folder
+
+Create a new folder under `schema/crdb/` using the provided name or a short descriptive name derived from the schema changes.
+
+Use existing folder names in `schema/crdb/` as examples for naming conventions.
+
+NOTE: The numbered folders, e.g. 1.0.0, are for legacy support only. No additional numbered directories should be added.
+
+### Step 4: Write migration files
+
+Based on the diff from step 1, write migration files in order:
+
+- Use `up01.sql`, `up02.sql` etc. (zero-padded) if you have more than 10 files.
+- Use `up1.sql`, `up2.sql` etc. if you have 10 or fewer files.
+- For Data Definition Language (DDL) statements, **one statement per file!**
+- For Data Modifying Language (DML) statements, multiple statements are allowed per file.
+- For `ALTER TABLE` with multiple columns, you can add them all in one statement.
+- When adding `NOT NULL` columns to existing tables, add temporary defaults, then remove them in later migration files.
+- Use `IF NOT EXISTS` for idempotency where supported.
+- Individual `up.sql` files are executed within a transaction (this always happens), and should be idempotent (this is an expectation that the migration author must uphold, with, e.g. `IF NOT EXISTS`).
+
+### Step 5: Update dbinit.sql version
+
+Bump the version number at the end of `schema/crdb/dbinit.sql`.
+
+### Step 6: Update SCHEMA_VERSION
+
+In `nexus/db-model/src/schema_versions.rs`, bump `SCHEMA_VERSION`.
+
+### Step 7: Add to KNOWN_VERSIONS
+
+In `nexus/db-model/src/schema_versions.rs`, add the new version to the `KNOWN_VERSIONS` list.
+
+### Step 8: Test the migration
+
+Run `cargo nextest run -p omicron-nexus schema` to verify that the migration is correct.
+
+## Common issues
+
+- Don't guess table names—use the diff from step 1 to find the correct table.
+- When adding constraints, always use `IF NOT EXISTS` for idempotency.
+- For columns with defaults in migration but not in final schema, add the defaults during migration then remove them.
+- Don't skip step 1—always run the diff command first to understand what needs to be migrated.
+
+## Data migration tests
+
+When creating a migration that affects existing data (like adding columns to existing tables), also add a data migration test in `nexus/tests/integration_tests/schema.rs`:
+
+- Add `before_X_0_0` function to create test data in the old format.
+- Add `after_X_0_0` function to verify the migration worked correctly.
+- Add the version to the `get_migration_checks()` map.
+
+This ensures old rows can be migrated smoothly in production.
+
+## Reference
+
+Consult `schema/crdb/README.adoc` for more information.

.config/nextest.toml

@@ -3,7 +3,7 @@
 #
 # The required version should be bumped up if we need new features, performance
 # improvements or bugfixes that are present in newer versions of nextest.
-nextest-version = { required = "0.9.117", recommended = "0.9.118" }
+nextest-version = { required = "0.9.117", recommended = "0.9.125" }
 
 experimental = ["benchmarks", "setup-scripts"]
 

.github/buildomat/build-and-test.sh

@@ -18,7 +18,7 @@ target_os=$1
 # NOTE: This version should be in sync with the recommended version in
 # .config/nextest.toml. (Maybe build an automated way to pull the recommended
 # version in the future.)
-NEXTEST_VERSION='0.9.118'
+NEXTEST_VERSION='0.9.125'
 
 cargo --version
 rustc --version
@@ -132,9 +132,38 @@ ptime -m cargo build -Z unstable-options --timings=json \
 # 2 less (negative 2) than the default.  This avoids many test flakes where
 # the test would have worked but the system was too overloaded and tests
 # take longer than their default timeouts.
+
+# Create a user config file that enables test recording.
+RECORDING_CONFIG_DIR="/tmp/nextest-recording-config"
+RECORDING_CONFIG="$RECORDING_CONFIG_DIR/config.toml"
+NEXTEST_STATE_DIR="$(mktemp -d /tmp/nextest-state.XXXXXX)"
+ARCHIVE_PATH="/tmp/nextest-run-archive.zip"
+
+mkdir -p "$RECORDING_CONFIG_DIR"
+printf '[experimental]\nrecord = true\n\n[record]\nenabled = true\n' \
+    > "$RECORDING_CONFIG"
+
+export NEXTEST_STATE_DIR
+
 banner test
+
+# Export an archive even on test failure.
+NEXTEST_EXIT=0
 ptime -m timeout 2h cargo nextest run --profile ci --locked \
-    --test-threads -2
+    --test-threads -2 \
+    --user-config-file "$RECORDING_CONFIG" \
+    || NEXTEST_EXIT=$?
+
+if ! ptime -m cargo nextest store export latest \
+    --user-config-file "$RECORDING_CONFIG" \
+    --archive-file "$ARCHIVE_PATH"; then
+    echo "warning: failed to export recording archive" >&2
+fi
+
+if [[ "$NEXTEST_EXIT" -ne 0 ]]; then
+    echo "error: cargo nextest run failed with exit code $NEXTEST_EXIT" >&2
+    exit "$NEXTEST_EXIT"
+fi
 
 #
 # https://github.com/nextest-rs/nextest/issues/16

.github/buildomat/jobs/build-and-test-helios.sh

@@ -8,6 +8,7 @@
 #:	"%/work/*",
 #:	"%/work/oxidecomputer/omicron/target/nextest/ci/junit.xml",
 #:	"%/work/oxidecomputer/omicron/target/live-tests-archive.tgz",
+#:	"=/tmp/nextest-run-archive.zip",
 #:	"%/var/tmp/omicron_tmp/**/*",
 #:	"!/var/tmp/omicron_tmp/crdb-base*",
 #:	"!/var/tmp/omicron_tmp/rustc*",
@@ -35,5 +36,10 @@
 #: series = "live-tests"
 #: name = "live-tests-archive.tgz"
 #: from_output = "/work/oxidecomputer/omicron/target/live-tests-archive.tgz"
+#:
+#: [[publish]]
+#: series = "nextest-recording-helios"
+#: name = "nextest-run-archive.zip"
+#: from_output = "/tmp/nextest-run-archive.zip"
 
 exec .github/buildomat/build-and-test.sh illumos

.github/buildomat/jobs/build-and-test-linux.sh

@@ -7,6 +7,7 @@
 #: output_rules = [
 #:	"%/work/*",
 #:	"%/work/oxidecomputer/omicron/target/nextest/ci/junit.xml",
+#:	"=/tmp/nextest-run-archive.zip",
 #:	"%/var/tmp/omicron_tmp/**/*",
 #:	"!/var/tmp/omicron_tmp/crdb-base*",
 #:	"!/var/tmp/omicron_tmp/rustc*",
@@ -29,6 +30,11 @@
 #: series = "build-info-linux"
 #: name = "crate-build-timings.json"
 #: from_output = "/work/crate-build-timings.json"
+#:
+#: [[publish]]
+#: series = "nextest-recording-linux"
+#: name = "nextest-run-archive.zip"
+#: from_output = "/tmp/nextest-run-archive.zip"
 
 
 sudo apt-get install -y jq

.github/buildomat/jobs/deploy.sh

@@ -2,7 +2,7 @@
 #:
 #: name = "helios / deploy"
 #: variety = "basic"
-#: target = "lab-2.0-opte-0.37"
+#: target = "lab-2.0-opte-0.39"
 #: output_rules = [
 #:  "%/var/svc/log/oxide-*.log*",
 #:  "%/zone/oxz_*/root/var/svc/log/oxide-*.log*",
@@ -359,10 +359,10 @@ OMICRON_NO_UNINSTALL=1 \
 
 # Wait for switch zone to come up
 retry=0
-until curl --head --silent -o /dev/null "http://[fd00:1122:3344:101::2]:12224/"
+until curl --max-time 1 --head --silent --show-error -o /dev/null "http://[fd00:1122:3344:101::2]:12224/"
 do
 	if [[ $retry -gt 30 ]]; then
-		echo "Failed to reach switch zone after 30 seconds"
+		echo "Failed to reach switch zone after 30 attempts"
 		exit 1
 	fi
 	sleep 1