diff --git a/.github/actions/refresh-graph-containers/action.yml b/.github/actions/refresh-graph-containers/action.yml index 41eed0f3e..9fc77f73c 100644 --- a/.github/actions/refresh-graph-containers/action.yml +++ b/.github/actions/refresh-graph-containers/action.yml @@ -1,5 +1,10 @@ name: Refresh Graph Container -description: Refreshes a single graph API container in-place without replacing the instance +description: >- + Refreshes a single graph API container in-place without replacing the instance, + by invoking the instance-side refresh script over SSM. All per-instance logic โ€” + the busy-counter wait, the pull, the digest check, the swap, the health check โ€” + lives in /usr/local/bin/refresh-graph-container.sh on the instance, not here. + This action is only the trigger and the result reporter. inputs: environment: @@ -18,16 +23,9 @@ inputs: description: 'AWS region' required: false default: 'us-east-1' - aws-account-id: - description: 'AWS account ID for ECR URI' - required: true - health-check-timeout: - description: 'Seconds to wait for health check after update' - required: false - default: '30' max-wait-minutes: description: >- - Maximum minutes to wait for in-flight destructive operations + Maximum minutes the instance waits for in-flight destructive operations (materialization, SEC staging, extensions mat, bulk table creates) to complete before cycling the container. Polls every 60 seconds. required: false @@ -44,7 +42,10 @@ inputs: outputs: status: description: 'Update status (success/failed)' - value: ${{ steps.update.outputs.status }} + value: ${{ steps.refresh.outputs.status }} + result: + description: 'Whether the container was restarted (updated) or already current (no-op)' + value: ${{ steps.refresh.outputs.result }} instance-id: description: 'Instance ID that was updated' value: ${{ inputs.instance-id }} @@ -76,248 +77,160 @@ runs: exit 1 fi - - name: Wait for In-Flight Destructive Ops on ${{ inputs.instance-id }} - id: wait_for_idle + - name: Refresh Container on ${{ inputs.instance-id }} + id: refresh shell: bash run: | - set -e + set -uo pipefail - REGISTRY_TABLE="robosystems-graph-${{ inputs.environment }}-instance-registry" INSTANCE_ID="${{ inputs.instance-id }}" REGION="${{ inputs.aws-region }}" - MAX_ATTEMPTS=${{ inputs.max-wait-minutes }} - FORCE_IGNORE="${{ inputs.force-ignore-busy }}" - # Stale detection: counter > 0 but no heartbeat for 6h โ†’ treat as crashed. - # 6h comfortably covers even full SEC historical backfills (30-120min). - # If a single op actually runs longer than this, it's almost certainly hung. - STALE_WINDOW_SECONDS=21600 + MAX_WAIT_MINUTES="${{ inputs.max-wait-minutes }}" + FORCE_IGNORE_BUSY="${{ inputs.force-ignore-busy }}" + + # The instance may spend up to MAX_WAIT_MINUTES waiting for an in-flight + # destructive op, then needs time to pull, restart, and pass its health + # check. Both budgets are derived from that ceiling rather than fixed, so + # raising max-wait-minutes can never silently truncate the refresh. + BUDGET_SECONDS=$(( MAX_WAIT_MINUTES * 60 + 900 )) + + echo "๐Ÿ”„ Refreshing graph container on ${INSTANCE_ID} (${{ inputs.node-type }})" + echo " busy-wait ceiling: ${MAX_WAIT_MINUTES}m, total budget: ${BUDGET_SECONDS}s" + + # Two commands, run as sequential lines of one script. The first is a + # presence guard with its own exit code, so "the instance has not picked up + # this script yet" is reported as itself rather than inferred from bash's + # generic 127 โ€” see the exit-code handling below. + # + # executionTimeout is always explicit. The AWS-RunShellScript default is + # 3600s, which happens to accommodate a 30-minute busy-wait today, but + # leaving it implicit means a later raise of max-wait-minutes truncates + # the command mid-refresh. + COMMAND_ID=$(aws ssm send-command \ + --instance-ids "${INSTANCE_ID}" \ + --document-name "AWS-RunShellScript" \ + --comment "graph container refresh (${{ inputs.environment }})" \ + --parameters "commands=[\"[ -x /usr/local/bin/refresh-graph-container.sh ] || { echo REFRESH_RESULT=skipped-no-script; exit 4; }\",\"MAX_WAIT_MINUTES=${MAX_WAIT_MINUTES} FORCE_IGNORE_BUSY=${FORCE_IGNORE_BUSY} /usr/local/bin/refresh-graph-container.sh\"],executionTimeout=[\"${BUDGET_SECONDS}\"]" \ + --query "Command.CommandId" \ + --output text \ + --region "${REGION}") - if [ "$FORCE_IGNORE" = "true" ]; then - echo "โš ๏ธ force-ignore-busy=true โ€” bypassing busy-counter check for ${INSTANCE_ID}" - echo "status=bypassed" >> $GITHUB_OUTPUT - exit 0 + if [ -z "${COMMAND_ID}" ] || [ "${COMMAND_ID}" == "None" ]; then + echo "โŒ Failed to dispatch SSM command to ${INSTANCE_ID}" + echo "status=failed" >> $GITHUB_OUTPUT + exit 1 fi - echo "๐Ÿ” Checking destructive-op counter on ${INSTANCE_ID} (table: ${REGISTRY_TABLE})" - - ATTEMPT=0 - COUNT=0 - while [ $ATTEMPT -lt $MAX_ATTEMPTS ]; do - ATTEMPT=$((ATTEMPT + 1)) - - ITEM=$(aws dynamodb get-item \ - --table-name "${REGISTRY_TABLE}" \ - --key "{\"instance_id\":{\"S\":\"${INSTANCE_ID}\"}}" \ - --query "Item" \ - --output json \ - --region "${REGION}" 2>/dev/null || echo "null") - - if [ "$ITEM" = "null" ] || [ -z "$ITEM" ]; then - echo "โ„น๏ธ No registry entry for ${INSTANCE_ID} (not yet registered or unmanaged) โ€” proceeding" - COUNT=0 - break - fi - - COUNT=$(echo "$ITEM" | jq -r '.active_destructive_ops.N // "0"') - LAST_AT=$(echo "$ITEM" | jq -r '.last_destructive_op_at.S // ""') - KIND=$(echo "$ITEM" | jq -r '.last_destructive_op_kind.S // "unknown"') - - # Integer comparison โ€” a counter that went negative (from a - # swallowed increment-failure on the writer side) should be - # treated as idle, not "still waiting." - if [ "${COUNT:-0}" -le 0 ] 2>/dev/null; then - if [ "${COUNT:-0}" -lt 0 ] 2>/dev/null; then - echo "::warning::Negative busy counter on ${INSTANCE_ID} (count=${COUNT}); treating as idle. May indicate a swallowed increment failure." - fi - if [ $ATTEMPT -eq 1 ]; then - echo "โœ… Instance ${INSTANCE_ID} is idle โ€” proceeding with refresh" - else - echo "โœ… Instance ${INSTANCE_ID} became idle after ${ATTEMPT} attempt(s)" - fi - COUNT=0 - break - fi + echo "โณ Command ${COMMAND_ID} dispatched; polling for completion..." + + # A bounded manual poll, deliberately NOT `aws ssm wait command-executed`. + # That waiter is delay:5 ร— maxAttempts:20 = 100 seconds, far shorter than + # this command can legitimately take; when it expired it reported failure + # for a refresh that was still running and about to succeed, and the + # natural response โ€” re-running the deploy โ€” cycled the container twice. + DEADLINE=$(( $(date +%s) + BUDGET_SECONDS )) + STATUS="Pending" + while true; do + STATUS=$(aws ssm get-command-invocation \ + --command-id "${COMMAND_ID}" \ + --instance-id "${INSTANCE_ID}" \ + --query "Status" \ + --output text \ + --region "${REGION}" 2>/dev/null) || STATUS="Pending" - # Stale detection: if the counter is stuck but heartbeat is old, - # assume the writer crashed and proceed anyway (GHA log warning). - if [ -n "$LAST_AT" ]; then - LAST_EPOCH=$(date -u -d "${LAST_AT}" +%s 2>/dev/null || echo 0) - NOW_EPOCH=$(date -u +%s) - AGE=$((NOW_EPOCH - LAST_EPOCH)) - if [ $LAST_EPOCH -gt 0 ] && [ $AGE -gt $STALE_WINDOW_SECONDS ]; then - echo "::warning::Stale busy counter on ${INSTANCE_ID}: count=${COUNT}, kind=${KIND}, last_update=${LAST_AT} (${AGE}s ago > ${STALE_WINDOW_SECONDS}s). Treating as crashed and proceeding." - COUNT=0 + case "${STATUS}" in + Success) break - fi - fi + ;; + Failed|Cancelled|TimedOut|Undeliverable|Terminated) + break + ;; + esac - echo "โณ Attempt ${ATTEMPT}/${MAX_ATTEMPTS}: instance ${INSTANCE_ID} busy (count=${COUNT}, kind=${KIND}, last=${LAST_AT}). Waiting 60s..." - sleep 60 + if [ "$(date +%s)" -ge "${DEADLINE}" ]; then + echo "::error::Refresh on ${INSTANCE_ID} exceeded its ${BUDGET_SECONDS}s budget (last status: ${STATUS}). The command may still be running on the instance." + echo "status=failed" >> $GITHUB_OUTPUT + exit 1 + fi + sleep 15 done - if [ "${COUNT:-0}" -gt 0 ] 2>/dev/null; then - echo "::error::Timed out after ${MAX_ATTEMPTS} minute(s) waiting for ${INSTANCE_ID} to become idle (count=${COUNT}, kind=${KIND}). Use force-ignore-busy=true to override." - echo "status=timeout" >> $GITHUB_OUTPUT - exit 1 - fi - - echo "status=idle" >> $GITHUB_OUTPUT - - - name: Update Container on ${{ inputs.instance-id }} - id: update - shell: bash - run: | - echo "๐Ÿ”„ Updating graph container on instance ${{ inputs.instance-id }} - ${{ inputs.node-type }}" - - # Get ECR URI - ECR_URI="${{ inputs.aws-account-id }}.dkr.ecr.${{ inputs.aws-region }}.amazonaws.com/robosystems" - - # Container name represents the service role, not the storage engine - if [ "${{ inputs.node-type }}" == "shared-writer" ] || [ "${{ inputs.node-type }}" == "shared-replica" ]; then - CONTAINER_NAME="graph-api-shared" - else - CONTAINER_NAME="graph-api" - fi - - # Graph API health endpoint - HEALTH_PORT="8001" - - # Use universal startup script - STARTUP_SCRIPT="/usr/local/bin/run-graph-container.sh" - - echo "๐Ÿ“ฆ Pulling and updating container: $CONTAINER_NAME (runtime: ${{ inputs.backend }}, health port: $HEALTH_PORT)" - - # Send update command - COMMAND_ID=$(aws ssm send-command \ - --instance-ids "${{ inputs.instance-id }}" \ - --document-name "AWS-RunShellScript" \ - --parameters "commands=[ - \"#!/bin/bash\", - \"set -e\", - \"echo \\\"๐Ÿ”„ Starting container refresh for $CONTAINER_NAME\\\"\", - \"echo \\\"Loading environment variables...\\\"\", - \"if [ -f /etc/environment ]; then set -a; source /etc/environment; set +a; fi\", - \"echo \\\"Pulling latest image...\\\"\", - \"aws ecr get-login-password --region ${{ inputs.aws-region }} | docker login --username AWS --password-stdin ${ECR_URI%/*}\", - \"docker pull ${ECR_URI}:${{ inputs.environment }}\", - \"export ECR_IMAGE=\\\"${ECR_URI}:${{ inputs.environment }}\\\"\", - \"echo \\\"Stopping existing container...\\\"\", - \"docker stop $CONTAINER_NAME || true\", - \"docker rm $CONTAINER_NAME || true\", - \"echo \\\"Starting new container...\\\"\", - \"$STARTUP_SCRIPT\", - \"echo \\\"โœ… Container updated successfully\\\"\" - ]" \ - --query "Command.CommandId" \ - --output text \ - --region "${{ inputs.aws-region }}") - - echo "โณ Waiting for update to complete (Command: ${COMMAND_ID})..." - - # Wait for command execution - if aws ssm wait command-executed \ + OUTPUT=$(aws ssm get-command-invocation \ --command-id "${COMMAND_ID}" \ - --instance-id "${{ inputs.instance-id }}" \ - --region "${{ inputs.aws-region }}"; then - - echo "โœ… Container update completed successfully" + --instance-id "${INSTANCE_ID}" \ + --query "StandardOutputContent" \ + --output text \ + --region "${REGION}" 2>/dev/null || echo "") - # Get the output for logging - OUTPUT=$(aws ssm get-command-invocation \ + if [ "${STATUS}" != "Success" ]; then + RESPONSE_CODE=$(aws ssm get-command-invocation \ --command-id "${COMMAND_ID}" \ - --instance-id "${{ inputs.instance-id }}" \ - --query "StandardOutputContent" \ + --instance-id "${INSTANCE_ID}" \ + --query "ResponseCode" \ --output text \ - --region "${{ inputs.aws-region }}" 2>/dev/null || echo "") - - if [ -n "$OUTPUT" ]; then - echo "๐Ÿ“ Update output:" - echo "$OUTPUT" | head -20 + --region "${REGION}" 2>/dev/null || echo "-1") + + # Two exit codes mean "this instance has not picked up the new + # deployment yet," which is a transitional state to be waited out (ASG + # churn) or backfilled โ€” not a broken refresh โ€” so neither may fail the + # deploy. Both are reported loudly and distinctly, so a fleet that never + # cycles cannot hide behind a green run. + # + # 4 โ€” refresh-graph-container.sh is not on the instance at all. + # Common scripts are downloaded from S3 in userdata, i.e. only at + # boot, so uploading a new one does not put it on a running + # instance. Every instance predating this change reports 4 until + # it cycles. The guard that emits it is an explicit -x test in + # the dispatched command, deliberately NOT an inference from + # bash's 127: a missing `docker` inside the script would also + # exit 127, and that is a real failure, not a stale instance. + # 3 โ€” the script is present but /etc/environment predates the + # complete-environment contract, so it declined to start a + # container missing mounts the boot configured. + if [ "${RESPONSE_CODE}" == "4" ]; then + echo "::warning::${INSTANCE_ID} skipped: refresh-graph-container.sh is not on this instance yet (it is installed at boot). The container was left running its current image. Cycle the instance to pick it up." + echo "status=success" >> $GITHUB_OUTPUT + echo "result=skipped-no-script" >> $GITHUB_OUTPUT + exit 0 fi - else - echo "โŒ Container update failed" + if [ "${RESPONSE_CODE}" == "3" ]; then + echo "::warning::${INSTANCE_ID} skipped: /etc/environment predates the complete-environment contract. The container was left running its current image. Cycle the instance or backfill /etc/environment." + echo "${OUTPUT}" | tail -20 + echo "status=success" >> $GITHUB_OUTPUT + echo "result=skipped-stale-env" >> $GITHUB_OUTPUT + exit 0 + fi - # Get error details + echo "โŒ Refresh failed on ${INSTANCE_ID} (status: ${STATUS}, exit: ${RESPONSE_CODE})" ERROR_OUTPUT=$(aws ssm get-command-invocation \ --command-id "${COMMAND_ID}" \ - --instance-id "${{ inputs.instance-id }}" \ + --instance-id "${INSTANCE_ID}" \ --query "StandardErrorContent" \ --output text \ - --region "${{ inputs.aws-region }}" 2>/dev/null || echo "No error details available") - + --region "${REGION}" 2>/dev/null || echo "No error details available") echo "Error details:" - echo "$ERROR_OUTPUT" - + echo "${ERROR_OUTPUT}" + echo "Output:" + echo "${OUTPUT}" | tail -30 echo "status=failed" >> $GITHUB_OUTPUT exit 1 fi - # Health check - echo "๐Ÿฅ Waiting ${{ inputs.health-check-timeout }} seconds for container to stabilize..." - sleep ${{ inputs.health-check-timeout }} + # The script reports whether it actually restarted anything. Surfacing it + # matters: a refresh that no-ops on nearly every instance must be + # distinguishable from one that pulled nothing and quietly did nothing. + RESULT=$(echo "${OUTPUT}" | grep -o 'REFRESH_RESULT=[a-z-]*' | tail -1 | cut -d= -f2) + RESULT="${RESULT:-unknown}" - echo "๐Ÿฅ Performing health check..." - HEALTH_COMMAND=$(aws ssm send-command \ - --instance-ids "${{ inputs.instance-id }}" \ - --document-name "AWS-RunShellScript" \ - --parameters "commands=[\"curl -f -s http://localhost:${HEALTH_PORT}/health || echo UNHEALTHY\"]" \ - --query "Command.CommandId" \ - --output text \ - --region "${{ inputs.aws-region }}") + echo "๐Ÿ“ Refresh output:" + echo "${OUTPUT}" | tail -30 - sleep 5 - - HEALTH_STATUS=$(aws ssm get-command-invocation \ - --command-id "${HEALTH_COMMAND}" \ - --instance-id "${{ inputs.instance-id }}" \ - --query "StandardOutputContent" \ - --output text \ - --region "${{ inputs.aws-region }}" 2>/dev/null || echo "UNHEALTHY") - - if [[ "$HEALTH_STATUS" == *"UNHEALTHY"* ]]; then - echo "โŒ Health check failed for instance ${{ inputs.instance-id }}" - echo "status=failed" >> $GITHUB_OUTPUT - exit 1 + if [ "${RESULT}" == "no-op" ]; then + echo "โœ… ${INSTANCE_ID} already on the current image โ€” no restart" else - echo "โœ… Instance ${{ inputs.instance-id }} is healthy" - echo "Health response: ${HEALTH_STATUS:0:100}..." - echo "status=success" >> $GITHUB_OUTPUT + echo "โœ… ${INSTANCE_ID} refreshed and healthy" fi - - name: Cleanup Old Docker Images - if: success() - shell: bash - run: | - echo "๐Ÿงน Cleaning up old Docker images on ${{ inputs.instance-id }}..." - - CLEANUP_COMMAND=$(aws ssm send-command \ - --instance-ids "${{ inputs.instance-id }}" \ - --document-name "AWS-RunShellScript" \ - --parameters 'commands=["docker image prune -af --filter \"until=1h\""]' \ - --query "Command.CommandId" \ - --output text \ - --region "${{ inputs.aws-region }}") - - # Wait briefly for cleanup (non-blocking - don't fail deployment if cleanup has issues) - sleep 10 - - CLEANUP_STATUS=$(aws ssm get-command-invocation \ - --command-id "${CLEANUP_COMMAND}" \ - --instance-id "${{ inputs.instance-id }}" \ - --query "Status" \ - --output text \ - --region "${{ inputs.aws-region }}" 2>/dev/null || echo "Unknown") - - if [ "$CLEANUP_STATUS" == "Success" ]; then - CLEANUP_OUTPUT=$(aws ssm get-command-invocation \ - --command-id "${CLEANUP_COMMAND}" \ - --instance-id "${{ inputs.instance-id }}" \ - --query "StandardOutputContent" \ - --output text \ - --region "${{ inputs.aws-region }}" 2>/dev/null || echo "") - echo "โœ… Docker cleanup completed" - echo "$CLEANUP_OUTPUT" | tail -5 - else - echo "โš ๏ธ Docker cleanup status: $CLEANUP_STATUS (non-fatal)" - fi + echo "status=success" >> $GITHUB_OUTPUT + echo "result=${RESULT}" >> $GITHUB_OUTPUT diff --git a/.github/workflows/graph-maintenance.yml b/.github/workflows/graph-maintenance.yml index 731d3f06b..1beb8a729 100644 --- a/.github/workflows/graph-maintenance.yml +++ b/.github/workflows/graph-maintenance.yml @@ -274,7 +274,11 @@ jobs: for ENV in $ENVS; do echo "๐Ÿ” Discovering instances for $ENV..." - # Find LadybugDB writer instances by tag + # Find LadybugDB writer instances by tag. This covers every writer + # tier including shared: the launch template tags all of them + # LadybugRole=writer and distinguishes the tier with WriterTier. + # There is no LadybugRole=shared-writer โ€” a second query for that + # value matched nothing and has been removed. WRITERS=$(aws ec2 describe-instances \ --filters \ "Name=tag:Environment,Values=$ENV" \ @@ -283,16 +287,7 @@ jobs: --query "Reservations[].Instances[].InstanceId" \ --output text 2>/dev/null || echo "") - # Find shared-writer instances - SHARED_WRITERS=$(aws ec2 describe-instances \ - --filters \ - "Name=tag:Environment,Values=$ENV" \ - "Name=tag:LadybugRole,Values=shared-writer" \ - "Name=instance-state-name,Values=running" \ - --query "Reservations[].Instances[].InstanceId" \ - --output text 2>/dev/null || echo "") - - for ID in $WRITERS $SHARED_WRITERS; do + for ID in $WRITERS; do if [ -n "$ID" ]; then INSTANCE_IDS="$INSTANCE_IDS $ID" echo " Found: $ID" diff --git a/.github/workflows/prod.yml b/.github/workflows/prod.yml index 2d2204235..c30d9ddc8 100644 --- a/.github/workflows/prod.yml +++ b/.github/workflows/prod.yml @@ -705,7 +705,6 @@ jobs: # Graph refresh (EC2 containers via SSM) graph_refresh_enabled: ${{ inputs.graph_container_refresh != false }} graph_node_types: "writer" - graph_health_check_timeout: "30" # Pre-refresh busy-counter wait (protects in-flight materialization # on the shared-tier SEC master from mid-op cycling) max_wait_minutes: ${{ inputs.graph_refresh_max_wait_minutes || '30' }} diff --git a/.github/workflows/service-refresh.yml b/.github/workflows/service-refresh.yml index 377c20ab2..71a7382f3 100644 --- a/.github/workflows/service-refresh.yml +++ b/.github/workflows/service-refresh.yml @@ -77,11 +77,6 @@ on: required: false type: string default: "writer" - graph_health_check_timeout: - description: "Seconds to wait for graph health check" - required: false - type: string - default: "30" max_wait_minutes: description: "Minutes to wait for in-flight destructive ops on the target instance" required: false @@ -130,7 +125,29 @@ jobs: outputs: matrix: ${{ steps.collect.outputs.matrix }} has_instances: ${{ steps.collect.outputs.has_instances }} + job_timeout_minutes: ${{ steps.budget.outputs.job_timeout_minutes }} steps: + - name: Compute refresh job timeout + id: budget + run: | + # The refresh job must outlive the instance-side busy-wait, so this is + # derived from max_wait_minutes rather than hardcoded โ€” a static 15 was + # shorter than the 30-minute default wait, so a genuinely busy instance + # could never be waited out. + # + # Computed in shell, NOT in a `${{ }}` expression: GitHub Actions + # expressions have no arithmetic operators (the operator set is + # grouping, index, dereference, !, comparisons, && and ||), so a `+` + # there is a workflow parse error, not a value. + MAX_WAIT="${{ inputs.max_wait_minutes || '30' }}" + if ! [[ "$MAX_WAIT" =~ ^[0-9]+$ ]]; then + echo "::error::max_wait_minutes must be a positive integer, got '$MAX_WAIT'" + exit 1 + fi + # +15 covers the pull, restart and health check on top of the wait. + echo "job_timeout_minutes=$((MAX_WAIT + 15))" >> $GITHUB_OUTPUT + echo "Refresh job timeout: $((MAX_WAIT + 15))m (busy-wait ceiling ${MAX_WAIT}m)" + - name: Configure AWS credentials uses: aws-actions/configure-aws-credentials@v6 with: @@ -149,25 +166,24 @@ jobs: if [ "$NODE_TYPES" == "all" ] || [ "$NODE_TYPES" == "writer" ] || [ "$NODE_TYPES" == "shared" ]; then echo "๐Ÿ“ฆ Collecting writer instances..." - # Get all graph writers using the LadybugRole tag + # Get all graph writers using the LadybugRole tag. WriterTier comes + # out of this same response โ€” reading it with a per-instance + # describe-tags call inside the loop below is O(n) API calls from a + # single runner and throttles long before a large fleet finishes. LBUG_WRITERS=$(aws ec2 describe-instances \ --filters \ "Name=tag:Environment,Values=${{ inputs.environment }}" \ "Name=tag:LadybugRole,Values=writer" \ "Name=instance-state-name,Values=running" \ - --query "Reservations[].Instances[].InstanceId" \ + --query "Reservations[].Instances[].[InstanceId, Tags[?Key=='WriterTier']|[0].Value]" \ --output text \ --region "${{ github.event_name == 'workflow_dispatch' && (vars.AWS_REGION || 'us-east-1') || inputs.aws_region }}" 2>/dev/null || echo "") - # Process graph writers - for INSTANCE in $LBUG_WRITERS; do - TIER=$(aws ec2 describe-tags \ - --filters \ - "Name=resource-id,Values=$INSTANCE" \ - "Name=key,Values=WriterTier" \ - --query "Tags[0].Value" \ - --output text \ - --region "${{ github.event_name == 'workflow_dispatch' && (vars.AWS_REGION || 'us-east-1') || inputs.aws_region }}" 2>/dev/null || echo "") + # Process graph writers (tab-separated " " rows) + while read -r INSTANCE TIER; do + [ -n "$INSTANCE" ] || continue + # An untagged instance renders as the literal "None" in text output + [ "$TIER" == "None" ] && TIER="" # Filter instances based on node_types parameter if [ "$NODE_TYPES" == "shared" ]; then @@ -193,7 +209,7 @@ jobs: else INSTANCES_JSON="${INSTANCES_JSON%]},${INSTANCE_OBJ}]" fi - done + done <<< "$LBUG_WRITERS" fi # Collect shared replicas (only if all or shared-replicas is selected) @@ -241,7 +257,11 @@ jobs: needs: [collect-graph-instances] if: needs.collect-graph-instances.outputs.has_instances == 'true' runs-on: ${{ github.event_name == 'workflow_dispatch' && 'ubuntu-latest' || fromJSON(inputs.runner_config) }} - timeout-minutes: 15 + # Derived from the busy-wait ceiling by the collect job (see its "Compute + # refresh job timeout" step) rather than hardcoded here, so the two cannot + # drift. The `|| 45` is a floor for the impossible case of an empty output, + # since an empty timeout-minutes is not a valid value. + timeout-minutes: ${{ needs.collect-graph-instances.outputs.job_timeout_minutes || 45 }} permissions: id-token: write contents: read @@ -263,13 +283,6 @@ jobs: role-to-assume: ${{ vars.AWS_ROLE_ARN }} aws-region: ${{ github.event_name == 'workflow_dispatch' && (vars.AWS_REGION || 'us-east-1') || inputs.aws_region }} - - name: Get AWS Account ID - id: aws-account - run: | - # Fetch from STS - validates OIDC connection is working - ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text) - echo "account_id=${ACCOUNT_ID}" >> $GITHUB_OUTPUT - - name: Update Graph Container uses: ./.github/actions/refresh-graph-containers with: @@ -278,8 +291,6 @@ jobs: node-type: ${{ matrix.node_type }} backend: ${{ matrix.backend }} aws-region: ${{ github.event_name == 'workflow_dispatch' && (vars.AWS_REGION || 'us-east-1') || inputs.aws_region }} - aws-account-id: ${{ steps.aws-account.outputs.account_id }} - health-check-timeout: ${{ inputs.graph_health_check_timeout || '30' }} max-wait-minutes: ${{ inputs.max_wait_minutes || '30' }} force-ignore-busy: ${{ inputs.force_ignore_busy || 'false' }} diff --git a/.github/workflows/staging.yml b/.github/workflows/staging.yml index 6986a822f..656cd1496 100644 --- a/.github/workflows/staging.yml +++ b/.github/workflows/staging.yml @@ -722,7 +722,6 @@ jobs: # Graph refresh (EC2 containers via SSM) graph_refresh_enabled: ${{ inputs.graph_container_refresh != false }} graph_node_types: "writer" - graph_health_check_timeout: "30" # Pre-refresh busy-counter wait (protects in-flight materialization # on the shared-tier SEC master from mid-op cycling) max_wait_minutes: ${{ inputs.graph_refresh_max_wait_minutes || '30' }} diff --git a/bin/lambda/graph_volume_monitor.py b/bin/lambda/graph_volume_monitor.py index c855d4de7..943722764 100644 --- a/bin/lambda/graph_volume_monitor.py +++ b/bin/lambda/graph_volume_monitor.py @@ -227,7 +227,21 @@ def monitor_all_instances(expand_immediately: bool = False) -> dict[str, Any]: def discover_lbug_instances() -> list[dict]: - """Discover all running Graph instances""" + """Discover all running Graph writer instances. + + `writer` is the only value the writer launch template ever puts on + `LadybugRole`, across every tier โ€” the tier is carried separately on + `WriterTier`. This filter previously also listed `shared_master` and + `shared_replica`, which are `NODE_TYPE` values and never appear on + `LadybugRole`, so they matched nothing. + + Do not widen this to match any `LadybugRole` value. Replicas carry no + `LadybugRole` today and are slated to get `replica` (deliberately not + `shared_replica`) so that a tag expression can select the whole graph fleet. + Either way they must stay out of this function: its callers drive volume + expansion and replicas carry no data volume to manage. See + `discover_replica_instance_ids`, which enumerates them separately. + """ instances = [] @@ -236,10 +250,7 @@ def discover_lbug_instances() -> list[dict]: response = ec2.describe_instances( Filters=[ {"Name": "tag:Service", "Values": ["RoboSystems"]}, - { - "Name": "tag:LadybugRole", - "Values": ["writer", "shared_master", "shared_replica"], - }, + {"Name": "tag:LadybugRole", "Values": ["writer"]}, {"Name": "instance-state-name", "Values": ["running"]}, {"Name": "tag:Environment", "Values": [ENVIRONMENT]}, ] diff --git a/bin/userdata/common/graph-health-check.sh b/bin/userdata/common/graph-health-check.sh index f3c89c47a..bf49b26dc 100644 --- a/bin/userdata/common/graph-health-check.sh +++ b/bin/userdata/common/graph-health-check.sh @@ -23,17 +23,21 @@ set -e TOKEN=$(curl -s -X PUT "http://169.254.169.254/latest/api/token" -H "X-aws-ec2-metadata-token-ttl-seconds: 21600") INSTANCE_ID=$(curl -s -H "X-aws-ec2-metadata-token: $TOKEN" http://169.254.169.254/latest/meta-data/instance-id) -# Validate the runtime type and derive the container name from the instance role +# Validate the runtime type if [ "${DATABASE_TYPE}" != "ladybug" ]; then echo "ERROR: Unsupported DATABASE_TYPE: ${DATABASE_TYPE}" exit 1 fi -if [ "${NODE_TYPE}" = "shared_master" ] || [ "${NODE_TYPE}" = "shared_replica" ]; then - CONTAINER_NAME="graph-api-shared" -else - CONTAINER_NAME="graph-api" -fi +# Ask run-graph-container.sh for the container name rather than re-deriving the +# NODE_TYPE mapping a third time. Both scripts are downloaded from S3 in the same +# userdata block, so they are always the same vintage. If it is missing, this +# check cannot identify the container OR restart it (see below), so failing here +# is more honest than guessing a name and reporting health for it. +CONTAINER_NAME=$(/usr/local/bin/run-graph-container.sh --print-container-name) || { + echo "[$(date)] ERROR: could not determine container name from run-graph-container.sh" + exit 1 +} # Check container status if docker ps | grep -q $CONTAINER_NAME; then diff --git a/bin/userdata/common/graph-lifecycle.sh b/bin/userdata/common/graph-lifecycle.sh index ec5216ca1..8b8b18301 100755 --- a/bin/userdata/common/graph-lifecycle.sh +++ b/bin/userdata/common/graph-lifecycle.sh @@ -20,11 +20,12 @@ INSTANCE_REGISTRY_TABLE="${INSTANCE_REGISTRY_TABLE:-robosystems-graph-${ENVIRONM # ================================================================================== # DATABASE-SPECIFIC CONFIGURATION # ================================================================================== -if [ "${NODE_TYPE}" = "shared_master" ] || [ "${NODE_TYPE}" = "shared_replica" ]; then - CONTAINER_NAME="graph-api-shared" -else - CONTAINER_NAME="graph-api" -fi +# Ask run-graph-container.sh for the container name rather than re-deriving the +# NODE_TYPE mapping. It owns that mapping; every other copy of it drifted. +CONTAINER_NAME=$(/usr/local/bin/run-graph-container.sh --print-container-name) || { + echo "ERROR: could not determine container name from run-graph-container.sh" >&2 + exit 1 +} GRAPH_API_PORT="8001" DRAIN_ENDPOINT="http://localhost:${GRAPH_API_PORT}/admin/drain" CONNECTIONS_ENDPOINT="http://localhost:${GRAPH_API_PORT}/admin/connections" diff --git a/bin/userdata/common/refresh-graph-container.sh b/bin/userdata/common/refresh-graph-container.sh new file mode 100644 index 000000000..0dcc33515 --- /dev/null +++ b/bin/userdata/common/refresh-graph-container.sh @@ -0,0 +1,252 @@ +#!/bin/bash +# Graph Container Refresh +# +# The instance-side entry point for a container refresh: "bring yourself to the +# current image, safely." Everything it needs is already in /etc/environment, so +# the caller passes nothing but the two optional overrides below. That makes a +# refresh debuggable by hand: +# +# aws ssm send-command --instance-ids i-xxx \ +# --document-name AWS-RunShellScript \ +# --parameters 'commands=["/usr/local/bin/refresh-graph-container.sh"],executionTimeout=["3600"]' +# +# Optional environment overrides the caller may set on the command: +# MAX_WAIT_MINUTES minutes to wait for in-flight destructive ops (default 30) +# FORCE_IGNORE_BUSY "true" bypasses the busy-counter wait entirely. Emergency +# escape hatch โ€” an interrupted materialization may require a +# full graph rebuild. +# +# Exits 0 when the container was refreshed OR was already current, non-zero on +# failure. The non-zero exit is load-bearing: SSM records it as `Failed`, which is +# what lets a fleet-wide --max-errors budget halt a bad rollout instead of +# marching a broken image across every customer's database. + +set -o pipefail + +MAX_WAIT_MINUTES="${MAX_WAIT_MINUTES:-30}" +FORCE_IGNORE_BUSY="${FORCE_IGNORE_BUSY:-false}" + +# Busy counter > 0 but no heartbeat for 6h โ†’ treat as crashed. 6h comfortably +# covers even full SEC historical backfills (30-120min); anything longer is +# almost certainly hung. +STALE_WINDOW_SECONDS=21600 + +# Bumped whenever /etc/environment gains a variable a refresh depends on. +# See require_complete_environment. +REQUIRED_ENV_SCHEMA=2 + +# Distinct exit code for "this instance predates the environment contract." The +# caller maps it to a loud skip rather than a failure: the instance has not +# cycled since the contract landed, which is a transitional state to be waited +# out or backfilled, not a broken refresh. Any other non-zero exit is a real +# failure and must stay one. +EXIT_STALE_ENV=3 + +log() { echo "[refresh] $*"; } +die() { + echo "[refresh] ERROR: $*" >&2 + exit 1 +} + +# ================================================================================== +# ENVIRONMENT +# ================================================================================== +[ -f /etc/environment ] || die "/etc/environment is missing โ€” cannot reconstitute the container environment" + +set -a +# shellcheck disable=SC1091 +source /etc/environment +set +a + +# A refresh sources only /etc/environment. Any variable the boot exported but did +# not persist there silently falls back to run-graph-container.sh's defaults, and +# the refreshed container then differs from the one this instance booted with โ€” +# drift that surfaces as a missing volume mount rather than an error. Rather than +# guess at per-node-type completeness, trust the schema marker the userdata writes +# once it has persisted the full set. +require_complete_environment() { + local schema="${GRAPH_ENV_SCHEMA:-0}" + if ! [ "${schema}" -ge "${REQUIRED_ENV_SCHEMA}" ] 2>/dev/null; then + log "SKIPPED: /etc/environment predates the complete-environment contract" + log " found GRAPH_ENV_SCHEMA=${schema}, need >= ${REQUIRED_ENV_SCHEMA}" + log " Refreshing would start a container missing mounts this boot configured," + log " so the running container is being left alone rather than degraded." + log " Replace the instance (ASG cycle) so it re-runs its userdata, or backfill" + log " the missing variables into /etc/environment by hand." + echo "REFRESH_RESULT=skipped-stale-env" + exit "${EXIT_STALE_ENV}" + fi + + local missing="" + for var in DATABASE_TYPE NODE_TYPE CONTAINER_PORT ECR_URI ECR_IMAGE_TAG \ + ENVIRONMENT INSTANCE_ID PRIVATE_IP AVAILABILITY_ZONE INSTANCE_TYPE \ + AWS_REGION CLUSTER_TIER; do + [ -n "${!var:-}" ] || missing="${missing} ${var}" + done + [ -z "${missing}" ] || die "/etc/environment is missing required variables:${missing}" +} + +require_complete_environment + +# The image to converge on comes from /etc/environment, NOT from the environment +# name. They are usually the same moving tag, but the shared replicas can be +# pinned to a specific build tag during a storage-format-breaking engine upgrade +# (SHARED_REPLICA_IMAGE_TAG_{PROD,STAGING} โ†’ ECRImageTag), precisely so that a +# boot cannot pull a new engine before the new-format sec.lbug is published. +# Pulling ":${ENVIRONMENT}" here would defeat that pin. +export ECR_IMAGE="${ECR_URI}:${ECR_IMAGE_TAG}" +ECR_REGISTRY="${ECR_URI%/*}" + +# run-graph-container.sh owns the NODE_TYPE โ†’ container-name mapping. Ask it +# rather than re-deriving, so there is exactly one spelling of that fact. +CONTAINER_NAME=$(/usr/local/bin/run-graph-container.sh --print-container-name) || + die "could not determine container name from run-graph-container.sh" +[ -n "${CONTAINER_NAME}" ] || die "run-graph-container.sh returned an empty container name" + +log "instance=${INSTANCE_ID} node_type=${NODE_TYPE} container=${CONTAINER_NAME}" +log "target image=${ECR_IMAGE}" + +# ================================================================================== +# WAIT FOR IN-FLIGHT DESTRUCTIVE OPS +# ================================================================================== +# This is a coordination signal, NOT a guard. `instance_busy`'s own module logs +# write failures and never raises them, on the principle that a broken counter +# must not block the actual work โ€” see ref/data-plane.md ยง65. Every escape hatch +# below is therefore deliberate and must stay: a negative counter is idle, a +# counter stuck with a stale heartbeat is a crashed writer, and a missing registry +# row proceeds. Do not tighten these into a resource control. +wait_until_idle() { + if [ "${FORCE_IGNORE_BUSY}" = "true" ]; then + log "WARNING: FORCE_IGNORE_BUSY=true โ€” bypassing the busy-counter check" + return 0 + fi + + local table="robosystems-graph-${ENVIRONMENT}-instance-registry" + log "checking destructive-op counter (table: ${table})" + + local attempt=0 count=0 last_at kind row + while [ "${attempt}" -lt "${MAX_WAIT_MINUTES}" ]; do + attempt=$((attempt + 1)) + + # One call per poll, and no jq dependency: --query flattens the three fields + # we need into a single tab-separated row. + row=$(aws dynamodb get-item \ + --table-name "${table}" \ + --key "{\"instance_id\":{\"S\":\"${INSTANCE_ID}\"}}" \ + --query "Item.[active_destructive_ops.N, last_destructive_op_at.S, last_destructive_op_kind.S]" \ + --output text \ + --region "${AWS_REGION}" 2>/dev/null) || row="" + + # Fail open on a missing row or an unreadable registry: not yet registered, + # unmanaged, throttled, or a permissions gap โ€” none of which should block a + # refresh. + if [ -z "${row}" ] || [ "${row}" = "None" ]; then + log "no registry entry for ${INSTANCE_ID} โ€” proceeding" + return 0 + fi + + IFS=$'\t' read -r count last_at kind <<<"${row}" + [ "${count}" = "None" ] && count=0 + [ "${last_at}" = "None" ] && last_at="" + [ "${kind}" = "None" ] && kind="unknown" + + if [ "${count:-0}" -le 0 ] 2>/dev/null; then + if [ "${count:-0}" -lt 0 ] 2>/dev/null; then + log "WARNING: negative busy counter (count=${count}); treating as idle. May indicate a swallowed increment failure." + fi + if [ "${attempt}" -eq 1 ]; then + log "instance is idle โ€” proceeding" + else + log "instance became idle after ${attempt} attempt(s)" + fi + return 0 + fi + + if [ -n "${last_at}" ]; then + local last_epoch now_epoch age + last_epoch=$(date -u -d "${last_at}" +%s 2>/dev/null || echo 0) + now_epoch=$(date -u +%s) + age=$((now_epoch - last_epoch)) + if [ "${last_epoch}" -gt 0 ] && [ "${age}" -gt "${STALE_WINDOW_SECONDS}" ]; then + log "WARNING: stale busy counter (count=${count}, kind=${kind}, last=${last_at}, ${age}s ago > ${STALE_WINDOW_SECONDS}s). Treating as crashed and proceeding." + return 0 + fi + # An unparseable heartbeat disables the crashed-writer escape hatch, so + # this run can only end in idle or timeout. Say so once rather than let + # the operator watch 30 minutes of "busy" with no idea why the stale + # check never fired. `date -d` needs GNU coreutils (present on AL2023). + if [ "${last_epoch}" -eq 0 ] && [ "${attempt}" -eq 1 ]; then + log "WARNING: could not parse heartbeat '${last_at}' โ€” stale-counter detection is inactive for this run" + fi + fi + + log "attempt ${attempt}/${MAX_WAIT_MINUTES}: busy (count=${count}, kind=${kind}, last=${last_at}). Waiting 60s..." + sleep 60 + done + + die "timed out after ${MAX_WAIT_MINUTES} minute(s) waiting for the instance to become idle (count=${count}, kind=${kind}). Set FORCE_IGNORE_BUSY=true to override." +} + +wait_until_idle + +# ================================================================================== +# PULL +# ================================================================================== +# The pull always precedes the stop, so customer-visible downtime is the restart +# and not the download. Any future edit that reorders these is a regression. +log "logging in to ${ECR_REGISTRY}" +aws ecr get-login-password --region "${AWS_REGION}" | + docker login --username AWS --password-stdin "${ECR_REGISTRY}" >/dev/null || + die "ECR login failed" + +log "pulling ${ECR_IMAGE}" +docker pull "${ECR_IMAGE}" || die "docker pull failed for ${ECR_IMAGE}" + +# ================================================================================== +# DIGEST SKIP +# ================================================================================== +# The graph API image changes far less often than the API/worker images, so most +# fleet refreshes have nothing to do. Skipping the restart when the pulled image +# already matches the running one turns those into no-op pulls instead of a +# customer-visible bounce per instance. +# +# REFRESH_RESULT is printed either way: a run that no-ops on nearly every instance +# has to say so, or "success" becomes indistinguishable from "pulled nothing and +# quietly did nothing." +PULLED_IMAGE_ID=$(docker image inspect --format '{{.Id}}' "${ECR_IMAGE}" 2>/dev/null) || PULLED_IMAGE_ID="" +RUNNING_IMAGE_ID=$(docker inspect --format '{{.Image}}' "${CONTAINER_NAME}" 2>/dev/null) || RUNNING_IMAGE_ID="" +CONTAINER_RUNNING=$(docker inspect --format '{{.State.Running}}' "${CONTAINER_NAME}" 2>/dev/null) || CONTAINER_RUNNING="false" + +if [ -n "${PULLED_IMAGE_ID}" ] && + [ "${PULLED_IMAGE_ID}" = "${RUNNING_IMAGE_ID}" ] && + [ "${CONTAINER_RUNNING}" = "true" ]; then + log "already on ${ECR_IMAGE} (${PULLED_IMAGE_ID:0:19}) and running โ€” no restart needed" + echo "REFRESH_RESULT=no-op" + exit 0 +fi + +if [ "${CONTAINER_RUNNING}" != "true" ]; then + log "container is not running โ€” refreshing regardless of image match" +fi + +# ================================================================================== +# SWAP +# ================================================================================== +# run-graph-container.sh stops and removes the old container, starts the new one, +# and blocks on its own bounded health check, exiting non-zero if the container +# never becomes healthy. That health check is not repeated here โ€” it lives in one +# place, the same place that knows how the container is started. +log "swapping container via run-graph-container.sh" +/usr/local/bin/run-graph-container.sh || die "run-graph-container.sh failed โ€” container did not come up healthy" + +# ================================================================================== +# CLEANUP +# ================================================================================== +# Best-effort: a full disk is worth warning about, but a prune failure must not +# turn a healthy refresh into a failed invocation that eats the error budget. +docker image prune -af --filter until=1h >/dev/null 2>&1 || + log "WARNING: docker image prune failed (non-fatal)" + +log "refresh complete on ${INSTANCE_ID}" +echo "REFRESH_RESULT=updated" +exit 0 diff --git a/bin/userdata/common/run-graph-container.sh b/bin/userdata/common/run-graph-container.sh index 0318c7beb..57d9b6dd0 100644 --- a/bin/userdata/common/run-graph-container.sh +++ b/bin/userdata/common/run-graph-container.sh @@ -4,6 +4,27 @@ set -e +# Determine container name based on node type. The container name represents the +# service role, not the storage engine. This is the ONLY place that derives it โ€” +# refresh-graph-container.sh asks for it via --print-container-name rather than +# re-deriving, so there is exactly one spelling of the mapping. +determine_container_name() { + if [ "${NODE_TYPE}" = "shared_master" ] || [ "${NODE_TYPE}" = "shared_replica" ]; then + echo "graph-api-shared" + else + echo "graph-api" + fi +} + +# Name query: answered before the full validation below, because the mapping needs +# nothing but NODE_TYPE and callers should not have to construct a whole runnable +# environment just to ask what the container is called. +if [ "${1:-}" = "--print-container-name" ]; then + : ${NODE_TYPE:?"NODE_TYPE must be set"} + determine_container_name + exit 0 +fi + # Validate required environment variables : ${DATABASE_TYPE:?"DATABASE_TYPE must be set (ladybug)"} : ${NODE_TYPE:?"NODE_TYPE must be set"} @@ -26,15 +47,6 @@ STAGING_MOUNT_SOURCE="${STAGING_MOUNT_SOURCE:-}" STAGING_MOUNT_TARGET="${STAGING_MOUNT_TARGET:-}" DOCKER_PROFILE="${DOCKER_PROFILE:-${DATABASE_TYPE}-writer}" -# Determine container name based on node type -determine_container_name() { - if [ "${NODE_TYPE}" = "shared_master" ] || [ "${NODE_TYPE}" = "shared_replica" ]; then - echo "graph-api-shared" - else - echo "graph-api" - fi -} - CONTAINER_NAME=$(determine_container_name) echo "=== Starting ${DATABASE_TYPE} Container ===" diff --git a/bin/userdata/ladybug-replica.sh b/bin/userdata/ladybug-replica.sh index b075a8b87..cb998b633 100644 --- a/bin/userdata/ladybug-replica.sh +++ b/bin/userdata/ladybug-replica.sh @@ -160,6 +160,12 @@ aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/run-graph-container.sh \ exit 1 } +aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/refresh-graph-container.sh \ + /usr/local/bin/refresh-graph-container.sh || { + echo "ERROR: Could not download container refresh script from S3" + exit 1 +} + aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/graph-health-check.sh \ /usr/local/bin/graph-health-check.sh || { echo "ERROR: Could not download health check script from S3" @@ -169,6 +175,7 @@ aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/graph-health-check.sh \ # Make scripts executable chmod +x /usr/local/bin/register-graph-instance.sh chmod +x /usr/local/bin/run-graph-container.sh +chmod +x /usr/local/bin/refresh-graph-container.sh chmod +x /usr/local/bin/graph-health-check.sh # ================================================================================== @@ -224,7 +231,15 @@ export DOCKER_PROFILE="ladybug-shared-writer" export REPOSITORY_TYPE="${REPOSITORY_TYPE}" export SHARED_REPOSITORIES="${SHARED_REPOSITORIES}" -# Persist variables to /etc/environment for health checks and restarts +# Persist variables to /etc/environment for health checks and restarts. +# +# This must stay a superset of everything run-graph-container.sh reads, because +# a container refresh sources only this file. Anything exported above but not +# written here silently falls back to that script's defaults, and the refreshed +# container then differs from the one this boot started โ€” which is exactly how +# the mount paths and DOCKER_PROFILE below went missing: a refreshed replica +# came back with no Lance mount (while LANCE_INDEX_PATH still pointed into it) +# and no staging mount. echo "DATABASE_TYPE=ladybug" >> /etc/environment echo "NODE_TYPE=${LBUG_NODE_TYPE}" >> /etc/environment echo "CONTAINER_PORT=${LBUG_PORT}" >> /etc/environment @@ -239,10 +254,24 @@ echo "AWS_REGION=${AWS_REGION}" >> /etc/environment echo "CLUSTER_TIER=${CLUSTER_TIER}" >> /etc/environment echo "REPOSITORY_TYPE=${REPOSITORY_TYPE}" >> /etc/environment echo "SHARED_REPOSITORIES=${SHARED_REPOSITORIES}" >> /etc/environment -echo "LANCE_INDEX_PATH=/app/data/lance" >> /etc/environment +echo "DATA_MOUNT_SOURCE=${DATA_MOUNT_SOURCE}" >> /etc/environment +echo "DATA_MOUNT_TARGET=${DATA_MOUNT_TARGET}" >> /etc/environment +echo "LOGS_MOUNT_SOURCE=${LOGS_MOUNT_SOURCE}" >> /etc/environment +echo "LOGS_MOUNT_TARGET=${LOGS_MOUNT_TARGET}" >> /etc/environment +echo "STAGING_MOUNT_SOURCE=${STAGING_MOUNT_SOURCE}" >> /etc/environment +echo "STAGING_MOUNT_TARGET=${STAGING_MOUNT_TARGET}" >> /etc/environment +echo "LANCE_MOUNT_SOURCE=${LANCE_MOUNT_SOURCE}" >> /etc/environment +echo "LANCE_MOUNT_TARGET=${LANCE_MOUNT_TARGET}" >> /etc/environment +echo "LANCE_INDEX_PATH=${LANCE_MOUNT_TARGET}" >> /etc/environment +echo "DOCKER_PROFILE=${DOCKER_PROFILE}" >> /etc/environment echo "DATABASE_ENDPOINT=${DATABASE_ENDPOINT:-}" >> /etc/environment echo "DATABASE_PORT=${DATABASE_PORT:-5432}" >> /etc/environment echo "VALKEY_URL=${VALKEY_URL:-}" >> /etc/environment +# Asserts that everything above was written. refresh-graph-container.sh refuses to +# refresh below this version rather than start a container missing mounts this boot +# configured. Bump it here and in that script together whenever a variable a +# refresh depends on is added above. +echo "GRAPH_ENV_SCHEMA=2" >> /etc/environment # Run shared container runner /usr/local/bin/run-graph-container.sh diff --git a/bin/userdata/ladybug-writer.sh b/bin/userdata/ladybug-writer.sh index c8aa40560..a4c10b905 100644 --- a/bin/userdata/ladybug-writer.sh +++ b/bin/userdata/ladybug-writer.sh @@ -268,6 +268,12 @@ aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/run-graph-container.sh \ exit 1 } +aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/refresh-graph-container.sh \ + /usr/local/bin/refresh-graph-container.sh || { + echo "ERROR: Could not download container refresh script from S3" + exit 1 +} + aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/graph-health-check.sh \ /usr/local/bin/graph-health-check.sh || { echo "ERROR: Could not download health check script from S3" @@ -284,6 +290,7 @@ aws s3 cp s3://${DEPLOYMENT_BUCKET}/userdata/common/graph-lifecycle.sh \ chmod +x /usr/local/bin/setup-cloudwatch-graph.sh chmod +x /usr/local/bin/register-graph-instance.sh chmod +x /usr/local/bin/run-graph-container.sh +chmod +x /usr/local/bin/refresh-graph-container.sh chmod +x /usr/local/bin/graph-health-check.sh chmod +x /usr/local/bin/graph-lifecycle.sh @@ -420,6 +427,11 @@ echo "LANCE_MOUNT_SOURCE=${LANCE_MOUNT_SOURCE}" >> /etc/environment echo "LANCE_MOUNT_TARGET=${LANCE_MOUNT_TARGET}" >> /etc/environment echo "LANCE_INDEX_PATH=${LANCE_MOUNT_TARGET}" >> /etc/environment echo "DOCKER_PROFILE=${DOCKER_PROFILE}" >> /etc/environment +# Asserts that everything above was written. refresh-graph-container.sh refuses to +# refresh below this version rather than start a container missing mounts this boot +# configured. Bump it here and in that script together whenever a variable a +# refresh depends on is added above. +echo "GRAPH_ENV_SCHEMA=2" >> /etc/environment # Run shared container runner /usr/local/bin/run-graph-container.sh diff --git a/tests/infrastructure/test_userdata_refresh.py b/tests/infrastructure/test_userdata_refresh.py new file mode 100644 index 000000000..21ca483f6 --- /dev/null +++ b/tests/infrastructure/test_userdata_refresh.py @@ -0,0 +1,348 @@ +"""Behavioral tests for bin/userdata/common/refresh-graph-container.sh. + +That script decides whether to bounce a customer's graph container, so its +fail-open branches, its refusal branches, and its no-op branch all need pinning +down. `instance_busy` is a coordination signal rather than a guard +(ref/data-plane.md ยง65), which means every one of the "proceed anyway" paths is +deliberate โ€” and a well-meaning future edit that tightens one into a real guard +would look like a bug fix. These tests are what make that edit fail. + +The script is driven as a real bash process rather than reimplemented: it is +copied into a sandbox with its two absolute paths (`/etc/environment` and +`/usr/local/bin/`) rewritten, and PATH is pointed at stub `aws`/`docker` +binaries. Nothing is added to the production script to make it testable. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "bin" / "userdata" / "common" / "refresh-graph-container.sh" +ACTION = REPO_ROOT / ".github" / "actions" / "refresh-graph-containers" / "action.yml" + +# Exit codes the script uses to mean "this instance has not picked up the new +# deployment yet." The composite action maps these to a warning-and-skip instead +# of a failed deploy, so they are a contract between two files. +EXIT_STALE_ENV = 3 + +REQUIRED_ENV = { + "DATABASE_TYPE": "ladybug", + "NODE_TYPE": "shared_replica", + "CONTAINER_PORT": "8001", + "ECR_URI": "123.dkr.ecr.us-east-1.amazonaws.com/robosystems", + "ECR_IMAGE_TAG": "prod", + "ENVIRONMENT": "prod", + "INSTANCE_ID": "i-test", + "PRIVATE_IP": "10.0.0.1", + "AVAILABILITY_ZONE": "us-east-1a", + "INSTANCE_TYPE": "r7g.large", + "AWS_REGION": "us-east-1", + "CLUSTER_TIER": "shared", +} + +RUN_CONTAINER_STUB = """#!/bin/bash +if [ "${1:-}" = "--print-container-name" ]; then + if [ "${NODE_TYPE}" = "shared_master" ] || [ "${NODE_TYPE}" = "shared_replica" ]; then + echo "graph-api-shared" + else + echo "graph-api" + fi + exit 0 +fi +echo "$ECR_IMAGE" > "$STUB_STARTED_MARKER" +exit ${STUB_RUN_EXIT:-0} +""" + +AWS_STUB = """#!/bin/bash +case "$1 $2" in + "dynamodb get-item") + [ -n "$STUB_DDB_ROW" ] && printf '%b\\n' "$STUB_DDB_ROW" + exit ${STUB_DDB_EXIT:-0} ;; + "ecr get-login-password") echo "fake-token"; exit 0 ;; +esac +exit 0 +""" + +DOCKER_STUB = """#!/bin/bash +case "$1" in + login) exit 0 ;; + pull) echo "$2" >> "$STUB_PULL_LOG"; exit ${STUB_PULL_EXIT:-0} ;; + image) + case "$2" in + inspect) echo "${STUB_PULLED_ID}"; exit 0 ;; + prune) exit 0 ;; + esac ;; + inspect) + case "$3" in + '{{.Image}}') echo "${STUB_RUNNING_ID}"; exit 0 ;; + '{{.State.Running}}') echo "${STUB_CONTAINER_RUNNING:-true}"; exit 0 ;; + esac ;; +esac +exit 0 +""" + +# The target instances run AL2023 with GNU coreutils. macOS `date` has no -d, +# which would silently disable the stale-heartbeat branch under test, so the +# stub emulates GNU parsing on every platform for determinism. +DATE_STUB = """#!/bin/bash +if [ "$1" = "-u" ] && [ "$2" = "-d" ]; then + python3 -c " +import datetime,sys +try: + print(int(datetime.datetime.strptime(sys.argv[1],'%Y-%m-%dT%H:%M:%SZ').replace(tzinfo=datetime.timezone.utc).timestamp())) +except Exception: + sys.exit(1) +" "$3" + exit $? +fi +exec /bin/date "$@" +""" + + +class Result: + def __init__(self, proc, pull_log: Path, started: Path): + self.exit_code = proc.returncode + self.output = proc.stdout + proc.stderr + self.pulls = pull_log.read_text().split() if pull_log.exists() else [] + self.restarted = started.exists() + + +@pytest.fixture +def refresh(tmp_path): + """Return a callable that runs the real script against stubs.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + env_file = tmp_path / "environment" + + source = SCRIPT.read_text() + sandboxed = source.replace("/etc/environment", str(env_file)).replace( + "/usr/local/bin/", f"{bin_dir}/" + ) + script = bin_dir / "refresh.sh" + script.write_text(sandboxed) + + for name, body in [ + ("run-graph-container.sh", RUN_CONTAINER_STUB), + ("aws", AWS_STUB), + ("docker", DOCKER_STUB), + ("date", DATE_STUB), + ("sleep", "#!/bin/bash\nexit 0\n"), + ]: + path = bin_dir / name + path.write_text(body) + path.chmod(0o755) + script.chmod(0o755) + + pull_log = tmp_path / "pulls.log" + started = tmp_path / "started" + + def run(env_overrides=None, schema=2, missing=(), **stubs) -> Result: + lines = [f"{k}={v}" for k, v in REQUIRED_ENV.items() if k not in missing] + if env_overrides: + lines = [line for line in lines if line.split("=")[0] not in env_overrides] + [ + f"{k}={v}" for k, v in env_overrides.items() + ] + if schema is not None: + lines.append(f"GRAPH_ENV_SCHEMA={schema}") + env_file.write_text("\n".join(lines) + "\n") + + pull_log.unlink(missing_ok=True) + started.unlink(missing_ok=True) + pull_log.touch() + + env = { + **os.environ, + "PATH": f"{bin_dir}:{os.environ['PATH']}", + "STUB_PULL_LOG": str(pull_log), + "STUB_STARTED_MARKER": str(started), + "STUB_PULLED_ID": "sha256:aaa", + "STUB_RUNNING_ID": "sha256:aaa", + "STUB_CONTAINER_RUNNING": "true", + "STUB_DDB_ROW": "None", + } + env.update({k: str(v) for k, v in stubs.items()}) + + proc = subprocess.run( + ["bash", str(script)], capture_output=True, text=True, env=env, timeout=120 + ) + return Result(proc, pull_log, started) + + return run + + +class TestEnvironmentContract: + """An incomplete /etc/environment must be refused, never silently defaulted. + + A refresh sources only that file, so a variable the boot exported but did not + persist falls back to run-graph-container.sh's defaults and the refreshed + container differs from the running one โ€” drift that surfaces as a missing + volume mount rather than an error. + """ + + @pytest.mark.parametrize("schema", [None, 0, 1]) + def test_refuses_below_required_schema(self, refresh, schema): + result = refresh(schema=schema) + assert result.exit_code == EXIT_STALE_ENV + assert "REFRESH_RESULT=skipped-stale-env" in result.output + assert not result.restarted + + def test_refuses_when_a_required_variable_is_absent(self, refresh): + result = refresh(missing=("CLUSTER_TIER",)) + assert result.exit_code == 1 + assert "CLUSTER_TIER" in result.output + assert not result.restarted + + def test_proceeds_when_complete(self, refresh): + assert refresh().exit_code == 0 + + +class TestBusyCounterFailsOpen: + """Every one of these paths is deliberate; see ref/data-plane.md ยง65. + + The counter reports, it does not decide. A broken or absent counter must never + block a refresh, and tightening any of these into a real guard is the mistake + that doc records someone making. + """ + + def test_missing_registry_row_proceeds(self, refresh): + result = refresh(STUB_DDB_ROW="None") + assert result.exit_code == 0 + assert "no registry entry" in result.output + + def test_row_without_counter_is_idle(self, refresh): + result = refresh(STUB_DDB_ROW="None\\tNone\\tNone") + assert result.exit_code == 0 + assert "is idle" in result.output + + def test_negative_counter_is_idle_and_warns(self, refresh): + result = refresh(STUB_DDB_ROW="-1\\tNone\\tunknown") + assert result.exit_code == 0 + assert "negative busy counter" in result.output + + def test_unreadable_registry_proceeds(self, refresh): + result = refresh(STUB_DDB_ROW="", STUB_DDB_EXIT=255) + assert result.exit_code == 0 + assert "no registry entry" in result.output + + def test_stale_heartbeat_is_treated_as_crashed(self, refresh): + result = refresh(STUB_DDB_ROW="2\\t2000-01-01T00:00:00Z\\tmaterialize") + assert result.exit_code == 0 + assert "stale busy counter" in result.output + + def test_busy_with_fresh_heartbeat_times_out(self, refresh): + now = subprocess.run( + ["date", "-u", "+%Y-%m-%dT%H:%M:%SZ"], capture_output=True, text=True + ).stdout.strip() + result = refresh(STUB_DDB_ROW=f"2\\t{now}\\tmaterialize", MAX_WAIT_MINUTES=2) + assert result.exit_code == 1 + assert "timed out after 2" in result.output + assert not result.restarted + + def test_force_ignore_busy_bypasses_the_wait(self, refresh): + now = subprocess.run( + ["date", "-u", "+%Y-%m-%dT%H:%M:%SZ"], capture_output=True, text=True + ).stdout.strip() + result = refresh(STUB_DDB_ROW=f"2\\t{now}\\tmaterialize", FORCE_IGNORE_BUSY="true") + assert result.exit_code == 0 + assert "bypassing the busy-counter" in result.output + + +class TestDigestSkip: + """An unchanged image must be a no-op pull, not a customer-visible restart.""" + + def test_same_digest_and_running_is_a_noop(self, refresh): + result = refresh(STUB_PULLED_ID="sha256:aaa", STUB_RUNNING_ID="sha256:aaa") + assert result.exit_code == 0 + assert "REFRESH_RESULT=no-op" in result.output + assert not result.restarted + + def test_new_digest_restarts(self, refresh): + result = refresh(STUB_PULLED_ID="sha256:bbb", STUB_RUNNING_ID="sha256:aaa") + assert result.exit_code == 0 + assert "REFRESH_RESULT=updated" in result.output + assert result.restarted + + def test_stopped_container_restarts_even_on_a_digest_match(self, refresh): + """A no-op on a dead container would leave it dead.""" + result = refresh( + STUB_PULLED_ID="sha256:aaa", + STUB_RUNNING_ID="sha256:aaa", + STUB_CONTAINER_RUNNING="false", + ) + assert result.exit_code == 0 + assert "REFRESH_RESULT=updated" in result.output + assert result.restarted + + def test_absent_container_restarts(self, refresh): + result = refresh( + STUB_PULLED_ID="sha256:bbb", + STUB_RUNNING_ID="", + STUB_CONTAINER_RUNNING="false", + ) + assert result.exit_code == 0 + assert result.restarted + + +class TestFailurePropagation: + """Non-zero exits are what let a fleet-wide --max-errors budget halt a rollout.""" + + def test_pull_failure_exits_nonzero(self, refresh): + result = refresh( + STUB_PULLED_ID="sha256:bbb", STUB_RUNNING_ID="sha256:aaa", STUB_PULL_EXIT=1 + ) + assert result.exit_code == 1 + assert "docker pull failed" in result.output + assert not result.restarted + + def test_unhealthy_container_exits_nonzero(self, refresh): + result = refresh( + STUB_PULLED_ID="sha256:bbb", STUB_RUNNING_ID="sha256:aaa", STUB_RUN_EXIT=1 + ) + assert result.exit_code == 1 + assert "did not come up healthy" in result.output + + +class TestImageTag: + def test_pull_honors_the_recorded_tag_not_the_environment(self, refresh): + """The shared replicas can be pinned to a build tag during a + storage-format-breaking engine upgrade, precisely so a boot cannot pull a new + engine before the new-format sec.lbug is published. A refresh that hardcoded + ":$ENVIRONMENT" would defeat that pin during the upgrade it protects. + """ + result = refresh( + env_overrides={"ECR_IMAGE_TAG": "v1.7.9-build"}, + STUB_PULLED_ID="sha256:bbb", + STUB_RUNNING_ID="sha256:aaa", + ) + assert result.pulls == [ + "123.dkr.ecr.us-east-1.amazonaws.com/robosystems:v1.7.9-build" + ] + assert not any(pull.endswith(":prod") for pull in result.pulls) + + +class TestExitCodeContract: + """The skip exit codes are a contract between the script and the action. + + If the script's code changes without the action's mapping changing, a stale + instance goes from a warning to a red deploy โ€” the exact failure this pairing + exists to prevent, and one nothing else would catch. + """ + + def test_action_maps_the_scripts_stale_env_code(self): + script = SCRIPT.read_text() + action = ACTION.read_text() + assert f"EXIT_STALE_ENV={EXIT_STALE_ENV}" in script + assert f'RESPONSE_CODE}}" == "{EXIT_STALE_ENV}"' in action + + def test_action_guards_for_a_missing_script(self): + """Instances install common scripts at boot, so an un-cycled instance has no + script to run. That must be a distinct exit code rather than an inference + from bash's 127, which a missing `docker` would also produce. + """ + action = ACTION.read_text() + assert "-x /usr/local/bin/refresh-graph-container.sh" in action + assert "exit 4" in action + assert 'RESPONSE_CODE}" == "4"' in action diff --git a/tests/infrastructure/test_workflow_expressions.py b/tests/infrastructure/test_workflow_expressions.py new file mode 100644 index 000000000..5d6592f96 --- /dev/null +++ b/tests/infrastructure/test_workflow_expressions.py @@ -0,0 +1,81 @@ +"""GitHub Actions expression validation. + +There is no actionlint in CI, so a malformed `${{ }}` expression is not caught +until the workflow is triggered โ€” and for a reusable workflow like +`service-refresh.yml`, which `prod.yml` and `staging.yml` both call, that means +the first thing it breaks is a deploy. + +This guards the one class of malformed expression that is easy to write and looks +correct: arithmetic. GitHub Actions expressions have no arithmetic operators. The +documented operator set is grouping, index, property dereference, `!`, the four +comparisons, `==`, `!=`, `&&`, and `||` โ€” nothing else. A `+` inside `${{ }}` is +a parse error, not a sum. + +https://docs.github.com/en/actions/reference/workflows-and-actions/expressions + +Do the arithmetic in a `run:` step and pass it through a step or job output. +`service-refresh.yml`'s "Compute refresh job timeout" step is the worked example. +""" + +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +GITHUB_DIR = REPO_ROOT / ".github" + +_EXPRESSION_RE = re.compile(r"\$\{\{(.*?)\}\}", re.DOTALL) +# String literals are stripped before the scan: single-quoted values legitimately +# contain the characters we look for ('us-east-1', 'refs/heads/main', '*/5 * * *'). +_STRING_LITERAL_RE = re.compile(r"'[^']*'") +# Object filters are also stripped. `needs.*.result` and `steps.*.outputs` use `*` +# as a dereference wildcard, which is valid and unrelated to multiplication โ€” a +# wildcard is always attached to a dot or brackets, never spaced as an operand. +_OBJECT_FILTER_RE = re.compile(r"\.\*|\[\*\]") + +# `+` is unambiguous โ€” it appears in no valid expression. `*`, `-` and `/` all +# have legitimate unquoted uses (object filters above; hyphenated job ids such as +# needs.deploy-graph-infra.outputs.x; paths), so they are flagged only when spaced +# like arithmetic, which none of those forms are. +_ARITHMETIC_PATTERNS = [ + (re.compile(r"\+"), "+"), + (re.compile(r"\s\*\s"), "*"), + (re.compile(r"\s-\s"), "-"), + (re.compile(r"\s/\s"), "/"), +] + + +def _workflow_files() -> list[Path]: + return sorted( + [*GITHUB_DIR.glob("workflows/*.yml"), *GITHUB_DIR.glob("actions/*/action.yml")] + ) + + +def test_workflow_files_are_discovered(): + """A silent glob miss would make every assertion below vacuously pass.""" + files = _workflow_files() + assert len(files) > 10, f"expected the .github tree, found {len(files)} files" + assert any(f.name == "service-refresh.yml" for f in files) + + +@pytest.mark.parametrize("path", _workflow_files(), ids=lambda p: p.name) +def test_no_arithmetic_in_expressions(path: Path): + violations: list[str] = [] + + for match in _EXPRESSION_RE.finditer(path.read_text()): + expression = match.group(1) + scannable = _OBJECT_FILTER_RE.sub("", _STRING_LITERAL_RE.sub("''", expression)) + for pattern, operator in _ARITHMETIC_PATTERNS: + if pattern.search(scannable): + line = path.read_text()[: match.start()].count("\n") + 1 + violations.append( + f"{path.relative_to(REPO_ROOT)}:{line}: '{operator}' in ${{{{{expression.strip()}}}}}" + ) + break + + assert not violations, ( + "GitHub Actions expressions have no arithmetic operators; a '+' inside " + "${{ }} is a parse error that breaks the whole workflow. Compute the value " + "in a run: step and pass it via an output instead.\n " + "\n ".join(violations) + )