Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ concurrency:

jobs:
test:
runs-on: ubuntu-latest
runs-on: self-hosted
strategy:
fail-fast: false
matrix:
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/maven-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jobs:
fail-fast: false
matrix:
java: [ 8.0.442, 17.0.13 ]
runs-on: ubuntu-latest
runs-on: self-hosted
timeout-minutes: 45
steps:
- name: Free Disk Space
Expand Down Expand Up @@ -68,7 +68,7 @@ jobs:
presto-coordinator-image:
name: "presto-coordinator-image"
needs: "maven-checks"
runs-on: "ubuntu-22.04"
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify self-hosted runner has Docker daemon and registry credentials.

The presto-coordinator-image job builds and pushes Docker images to ghcr.io. Ensure self-hosted runners have:

  1. Docker daemon running and accessible
  2. Sufficient disk space for image builds and temporary layers
  3. Network connectivity to ghcr.io

No Docker-specific runner labels are specified, which could lead to job failures if assigned to runners without Docker support.

Add explicit Docker support labels to the runner specification:

  presto-coordinator-image:
    name: "presto-coordinator-image"
    needs: "maven-checks"
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, docker, x64]

steps:
- uses: "actions/checkout@v4"
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/prestissimo-worker-images-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ on:
jobs:
prestissimo-worker-images-build:
name: "prestissimo-worker-images-build"
runs-on: "ubuntu-22.04"
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify self-hosted runner OS version and architecture matching.

The workflow previously specified ubuntu-22.04 but now uses generic self-hosted. This could cause build inconsistencies if:

  1. Runners are running different Ubuntu versions (20.04 vs 22.04 vs 24.04)
  2. Build dependencies vary across OS versions
  3. Toolchain versions differ (CMAKE, GCC, LLVM, etc.)

To ensure reproducibility and catch OS-related regressions early, either:

  1. Explicitly label runners with their OS version:
    -    runs-on: self-hosted
    +    runs-on: [self-hosted, linux, ubuntu-22.04, x64, docker]
  2. Or add a step to verify the runtime OS and fail if unexpected:
    - name: Verify OS version
      run: |
        expected_version="22.04"
        actual_version=$(lsb_release -rs)
        if [ "$actual_version" != "$expected_version" ]; then
          echo "ERROR: Expected Ubuntu $expected_version, got $actual_version"
          exit 1
        fi
🤖 Prompt for AI Agents
In .github/workflows/prestissimo-worker-images-build.yml around line 10 the
runner is changed from an explicit ubuntu-22.04 to a generic self-hosted which
can introduce OS/architecture variability; update the workflow to enforce a
consistent environment by either (A) adding explicit runner labels that include
OS version and architecture (for example add labels like "self-hosted",
"ubuntu-22.04", "x86_64" to the runs-on selector) or (B) add an early
verification step that checks the runtime OS version and architecture and fails
the job if they do not match the expected values (e.g., check lsb_release -rs
and uname -m and exit non-zero on mismatch); implement one of these approaches
and ensure the expected OS/version/arch values are documented in the workflow
file.

steps:
- uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683"
with:
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/prestocpp-linux-build-and-unit-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ concurrency:

jobs:
prestocpp-linux-build-for-test:
runs-on: ubuntu-22.04
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify all self-hosted runners have Docker support and pre-configured containers.

All four jobs in this workflow use a specific container image: prestodb/presto-native-dependency:0.293-20250522140509-484b00e. Ensure that:

  1. All self-hosted runners have Docker daemon running and accessible
  2. The container image is available (or will be pulled on first use)
  3. Container startup overhead is acceptable for your use case

Add explicit Docker runner labels to prevent job assignment to incompatible runners:

  prestocpp-linux-build-for-test:
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, docker, x64]

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
.github/workflows/prestocpp-linux-build-and-unit-test.yml around line 23: the
workflow currently uses "runs-on: self-hosted" but does not guarantee Docker
support or image availability; update the runs-on to a more specific label (for
example "runs-on: [self-hosted, docker]" or your org's Docker-capable runner
label) to prevent scheduling on incompatible hosts, document in the
workflow/comments that runners must have the Docker daemon accessible and that
the container image
prestodb/presto-native-dependency:0.293-20250522140509-484b00e should be either
pre-pulled or allowed to be pulled at runtime, and if startup latency is a
concern add a note to pre-cache the image on runners or use a warm-up job; also
ensure job-level timeout/visibility and any required permissions for using
Docker are declared.

container:
image: prestodb/presto-native-dependency:0.293-20250522140509-484b00e
env:
Expand Down Expand Up @@ -97,7 +97,7 @@ jobs:

prestocpp-linux-presto-e2e-tests:
needs: prestocpp-linux-build-for-test
runs-on: ubuntu-22.04
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify e2e test job has adequate runner resources.

The prestocpp-linux-presto-e2e-tests job runs presto server with test suite, requiring:

  • Docker support (container job)
  • Adequate memory (Presto server + Maven tests)
  • Network access for remote function tests (commented TODO at line 159-161)

Ensure runners are labeled with resource specifications to prevent out-of-memory kills or job timeouts.

Add runner labels:

  prestocpp-linux-presto-e2e-tests:
    needs: prestocpp-linux-build-for-test
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, docker, x64, presto-e2e-runner]

Then configure runners with the presto-e2e-runner label to have adequate resources (recommended: ≥8GB RAM, ≥4 CPU cores).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runs-on: self-hosted
runs-on: [self-hosted, linux, docker, x64, presto-e2e-runner]
🤖 Prompt for AI Agents
.github/workflows/prestocpp-linux-build-and-unit-test.yml around line 100: the
prestocpp-linux-presto-e2e-tests job uses a generic self-hosted runner which may
not provide Docker, sufficient memory, CPU or network access; update the job to
use specific runner labels (e.g., add "presto-e2e-runner" and an OS label)
instead of only "self-hosted", and ensure your self-hosted runners are
provisioned/configured with Docker and adequate resources (recommended ≥8GB RAM
and ≥4 CPU cores) and required network access for remote function tests.

container:
image: prestodb/presto-native-dependency:0.293-20250522140509-484b00e
env:
Expand Down Expand Up @@ -172,7 +172,7 @@ jobs:

prestocpp-linux-presto-native-tests:
needs: prestocpp-linux-build-for-test
runs-on: ubuntu-22.04
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Ensure runner capacity for matrix strategy with storage format variations.

The prestocpp-linux-presto-native-tests job uses a matrix strategy with storage-format: ["PARQUET", "DWRF"] (lines 178-179), spawning at least 2 concurrent jobs per Java version per available runner. Without dedicated runner labels, this could cause job queuing or starvation.

Add runner labels to ensure capacity:

  prestocpp-linux-presto-native-tests:
    needs: prestocpp-linux-build-for-test
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, docker, x64, presto-native-tests-runner]

Then ensure you have enough runners with the presto-native-tests-runner label to accommodate the matrix combinations.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runs-on: self-hosted
runs-on: [self-hosted, linux, docker, x64, presto-native-tests-runner]
🤖 Prompt for AI Agents
.github/workflows/prestocpp-linux-build-and-unit-test.yml around line 175: the
job uses runs-on: self-hosted and a matrix for storage-format which will spawn
multiple concurrent jobs; update the job's runs-on to include a dedicated runner
label (for example "self-hosted,presto-native-tests-runner") so matrix jobs land
only on runners with that label, then verify you have enough runners registered
with the presto-native-tests-runner label to handle the maximum concurrent
matrix combinations.

strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -248,7 +248,7 @@ jobs:

prestocpp-linux-presto-sidecar-tests:
needs: prestocpp-linux-build-for-test
runs-on: ubuntu-22.04
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add runner labels to prevent job misassignment for sidecar tests.

The prestocpp-linux-presto-sidecar-tests job requires Docker container support and adequate resources (line 251). No runner labels are specified, which could lead to assignment to incompatible or under-provisioned runners.

Add explicit runner labels:

  prestocpp-linux-presto-sidecar-tests:
    needs: prestocpp-linux-build-for-test
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, docker, x64, presto-sidecar-tests-runner]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runs-on: self-hosted
prestocpp-linux-presto-sidecar-tests:
needs: prestocpp-linux-build-for-test
runs-on: [self-hosted, linux, docker, x64, presto-sidecar-tests-runner]
🤖 Prompt for AI Agents
In .github/workflows/prestocpp-linux-build-and-unit-test.yml around line 251,
the prestocpp-linux-presto-sidecar-tests job uses runs-on: self-hosted which can
be picked up by incompatible or under-provisioned runners; update runs-on to
specify an explicit set of runner labels (e.g., include self-hosted plus linux,
docker, and a resource-sizing label such as xlarge or high-memory) so the job
will only be scheduled on hosts that provide Docker and sufficient resources.

container:
image: prestodb/presto-native-dependency:0.293-20250522140509-484b00e
env:
Expand Down
30 changes: 30 additions & 0 deletions .github/workflows/test-npm.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: Test NPM Availability

on:
workflow_dispatch:

jobs:
test-npm:
runs-on: self-hosted
steps:
- name: Check system info
run: |
echo "=== Runner Information ==="
echo "Hostname: $(hostname)"
echo "OS: $(uname -a)"
echo "User: $(whoami)"
- name: Check npm and node versions
run: |
echo "=== NPM and Node Versions ==="
which npm || echo "npm not found in PATH"
npm --version || echo "npm command failed"
which node || echo "node not found in PATH"
node --version || echo "node command failed"
- name: List installed packages
run: |
echo "=== Checking for npm in common locations ==="
ls -la /usr/bin/npm 2>/dev/null || echo "/usr/bin/npm not found"
ls -la /usr/local/bin/npm 2>/dev/null || echo "/usr/local/bin/npm not found"
which -a npm 2>/dev/null || echo "npm not in PATH"
Comment on lines +1 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Clarify purpose and long-term intent of NPM availability diagnostic workflow.

The test-npm.yml workflow provides useful diagnostics for checking npm/node availability and paths on self-hosted runners. However, before merging, clarify:

  1. Is this workflow intended to remain in the main branch as a permanent troubleshooting tool?
  2. Should it be run only on specific branches or moved to a separate documentation/testing repository?
  3. How will results be used? (Should they auto-fail if npm is not found?)
  4. Are there automated actions triggered by workflow failures?

If this is a temporary diagnostic workflow for self-hosted runner setup validation, add documentation:

# TODO: Review this diagnostic workflow after self-hosted runner setup is complete
# Created: 2025-11-11, Purpose: Verify npm/node availability on self-hosted runners
name: Test NPM Availability  # Diagnostic workflow - may be removed after setup validation

Additionally, consider making the workflow fail if npm is not available, to surface configuration issues:

      - name: Check npm and node versions
        run: |
          echo "=== NPM and Node Versions ==="
+         set -e  # Exit on first error
          which npm || (echo "ERROR: npm not found in PATH" && exit 1)
          npm --version
          which node || (echo "ERROR: node not found in PATH" && exit 1)
          node --version
🤖 Prompt for AI Agents
.github/workflows/test-npm.yml lines 1-30: clarify intent and make diagnostics
actionable by (1) adding a top-of-file TODO header comment with creation date,
purpose and note that it may be removed after self-hosted runner validation, (2)
deciding and encoding scope/retention — either restrict triggers to specific
branches or move the workflow to a docs/tests repo and update the name to
indicate “diagnostic”, and (3) make the job fail when npm/node are missing by
replacing the permissive checks with explicit exit codes (e.g., run commands
that exit non-zero when npm/node are not found) or add an explicit conditional
step that checks presence and uses "exit 1" if absent so the workflow surfaces
configuration issues.

18 changes: 18 additions & 0 deletions .github/workflows/test-runner-pickup.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
name: Test Runner Pickup

on:
workflow_dispatch:
push:
branches:
- ci-test

jobs:
simple-test:
runs-on: self-hosted
steps:
- name: Echo test
run: |
echo "Runner picked up the job!"
echo "Hostname: $(hostname)"
echo "Date: $(date)"
echo "User: $(whoami)"
Comment on lines +1 to +18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Clarify purpose and long-term intent of diagnostic workflow.

The test-runner-pickup.yml workflow is a simple diagnostic tool for testing self-hosted runner availability and responsiveness. Before merging, clarify:

  1. Is this workflow intended to remain in the main branch?
  2. Should it be run only on specific branches or moved to a separate test repository?
  3. Should it be added to .github/workflows/ or a separate experimental directory?
  4. Who will monitor these results, and what action triggers on failure?

If this is a temporary diagnostic workflow for validating the CI migration, consider adding a comment documenting its purpose and planned removal date:

# TODO: Remove this diagnostic workflow after validating self-hosted runner setup
# Created: 2025-11-11, Target removal: 2025-12-11
name: Test Runner Pickup  # Diagnostic: Self-hosted runner availability test
🤖 Prompt for AI Agents
.github/workflows/test-runner-pickup.yml lines 1-18: this diagnostic workflow
needs explicit documentation and decision guidance before merging — add a
top-of-file comment that states its purpose (self-hosted runner availability
test), intended lifespan (creation date and planned removal date or retained
indefinitely), recommended branch policy (e.g., only run on a specific branch or
move to a separate experimental repo/dir), and who owns/monitors the workflow
and what actions to take on failures; optionally restrict triggers to a specific
branch or remove from main workflows directory if meant temporary.

4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ concurrency:

jobs:
changes:
runs-on: ubuntu-latest
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add runner labels to prevent job starvation with matrix strategy.

The changes job and subsequent test job (line 44) use a matrix strategy that can spawn up to 20 concurrent jobs (2 Java versions × 10 test modules). Without explicit runner labels or a dedicated runner group, this could cause significant job queuing if runners are shared across other workflows.

Consider adding specific runner labels to ensure adequate capacity:

  changes:
-    runs-on: self-hosted
+    runs-on: [self-hosted, linux, x64, test-runner-pool]

Then configure a runner group or add a custom label test-runner-pool to the appropriate subset of self-hosted runners to ensure tests have dedicated resources.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runs-on: self-hosted
runs-on: [self-hosted, linux, x64, test-runner-pool]
🤖 Prompt for AI Agents
.github/workflows/tests.yml around line 25 (and the test job at line 44): the
workflow currently uses runs-on: self-hosted which can lead to job starvation
with the 20-job matrix; update the jobs that run the matrix (the changes job and
the test job at line 44) to use a specific runner label such as runs-on:
[self-hosted, test-runner-pool] (or another dedicated label name you choose),
and then ensure your self-hosted runners are grouped/configured to include that
custom label (or create a runner group) so the matrix jobs target only those
dedicated runners and avoid contention with other workflows.

# Required permissions
permissions:
pull-requests: read
Expand All @@ -41,7 +41,7 @@ jobs:
- '!presto-docs/**'

test:
runs-on: ubuntu-latest
runs-on: self-hosted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Verify self-hosted runner disk space for Maven cache accumulation.

The test job runs Maven builds with a 80-minute timeout and matrix strategy spawning multiple jobs. Over time, Maven caches and build artifacts may accumulate on persistent self-hosted runners, potentially exhausting disk space.

Implement a cache cleanup strategy, such as:

  • Configure Maven cache limits or implement periodic cleanup jobs
  • Use ephemeral runners with automatic cleanup between jobs (recommended per GitHub documentation)
  • Monitor disk usage on self-hosted runners and alert if thresholds are exceeded
🤖 Prompt for AI Agents
.github/workflows/tests.yml around line 44: the workflow uses self-hosted
runners ("runs-on: self-hosted") which can accumulate Maven caches/artifacts
across matrix jobs and eventually exhaust disk space; update CI to either switch
to ephemeral hosted runners or add cleanup and monitoring: prefer using
GitHub-hosted runners if possible; otherwise add steps to the job (or a periodic
maintenance workflow) to prune Maven local repository (~/.m2/repository) and
GitHub Actions runner work directories after builds, enforce maven cache size
limits or use mvn dependency:purge-local-repository where appropriate, schedule
a weekly/cron workflow to run disk-cleanup on self-hosted runners, and add
runner-side disk-usage monitoring/alerts so administrators are notified when
free space drops below a threshold.

needs: changes
strategy:
fail-fast: false
Expand Down
Loading