[RORDEV-1228] Move CI from Azure Pipelines to GitHub Actions (Ubicloud Linux + GitHub-hosted Windows) - #1292
Conversation
Move CI to Ubicloud (Linux, ubicloud-standard-4 = 16GB) and Blacksmith (Windows, blacksmith-4vcpu-windows-2025), reusing ci/run-pipeline.sh unchanged. Adds .github/workflows/ci.yml mirroring all 14 Azure stages with full ES-version parity (34 Linux IT + 33 Windows IT legs), plus a pre-analysis of every secret to port (ci/github/SECRETS.md) and a placeholder registration script (set-secrets.sh). Validated with actionlint. Azure azure-pipelines.yml is left in place for parallel-run cutover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hosted; disable Azure
Re-audit of the port against azure-pipelines.yml found and fixed:
- BLOCKER: manual release_without_testing path could never run (skipped
needs propagate; release jobs now use !cancelled() + explicit
needs.<job>.result two-arm conditions mirroring Azure)
- BLOCKER: cancel-in-progress applied to develop/master pushes and could
kill an in-flight release; now PR-only (Azure batch semantics)
- Secret leak: docker-hub-auth.sh echoed the Azure ##vso isSecret line,
which on GH Actions PRINTS base64(user:token); now CI-aware (::add-mask::)
- Missing contents:write for release tag push (resolves the deploy-key
question: GITHUB_TOKEN suffices, GH_DEPLOY_KEY_B64 dropped from docs)
- it_windows now gates build_ror and the release path (Azure
succeeded('TEST') parity); es_s3_up now ordered before all check/test
jobs (newes/* artifact race)
- Added missing Windows unit-tests job (manual run_all_tests_on_windows)
- Dynamic IT matrices from setup (no VM boots for skipped legs), monthly
CVE cache key, **/epic/** + *newes/* pattern fixes, TRAVIS_BUILD_NUMBER
mapped to run_number, job-scope GRADLE_USER_HOME clear on Windows
Per review decisions: Windows runs on free GH-hosted windows-2025
(Blacksmith dropped); Azure triggers disabled (trigger:none, pr:none,
schedules removed) leaving Azure manually runnable as fallback.
PORT.md now carries a per-trigger scenario table as the regression spec.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Pin the weekly toolchains rebuild to develop (GH schedules fire on the default branch; ref guard protects against a default-branch change) - Add an image-tag drift guard to build_toolchains_image: all ror-ci-toolchains tags in ci.yml must match TOOLCHAINS_IMAGE - Gate it_windows on toolchains_verify (Azure TEST-stage parity: a broken image fails fast instead of running doomed Windows legs) - Unset empty CVE credentials on fork PRs: GitHub passes empty (not absent) secrets, and a set-but-empty NVD/OSS-Index key makes dependency-check attempt authenticated calls (401) instead of anonymous mode — Azure only exported these when not a fork - PORT.md: document the deliberately-broader md paths-ignore and that probe-host-disk.sh stays for the Azure manual fallback Remaining review findings were already fixed (toolchains gating of the manual release path) or confirmed non-issues by the reviewer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sm, sh gave exit 127) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…PshardIndex The parent shardedTest -PshardCount=K invocation also configures the test task; on a fresh CI workspace the shard filter's class-tree walk ran at configuration time before testClasses compiled anything, matched 0 suites, and the empty-shard guard killed the parent build. Children always receive -PshardIndex (ShardedGradlewTest), so gating on it skips the parent cleanly. Never seen before because CI always ran IT_PARALLELISM=1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-shard log artifacts k=2 on standard-4 (16GB): ~1.4x faster when stable, but 6/10 legs lost a shard to memory death (truncated TEST xmls, no assertion failures) — 8GB per shard is under the floor. k=1 remains the stable point on 16GB. Also re-adds the per-shard log upload so shard deaths are diagnosable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…output dirs
All K shards ran integration-tests:test in the same workspace sharing
build/test-results/test — concurrent shards corrupted/cleaned each
other's result files ('Could not write XML test results', truncated
TEST-*.xml), killing otherwise-green shards. Root cause of both the
k=2 (6/10) and k=3 (10/10) leg failures; not memory pressure.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ran it) The child's prebuildEsImage dependency is not a cache hit: it launches a nested Tooling-API build (Assembling ROR) and K concurrent nested builds in one workspace race each other — shards died in 31s-1m15s. The parent shardedTest task's dependsOn already prebuilds the image once before any shard spawns, so children exclude the task with -x. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…16GB runner Third shard race: suites creating non-singleton ES containers call RorPluginGradleProject.assemble at TEST RUNTIME — a nested gradle build. K shard processes' nested builds race Gradle's project locks and die with BuildException (initializationError across random suites). Fix at the choke point: cross-process file lock in runTask, so every nested build (prebuild or runtime) serializes; up-to-date builds hold it briefly. Sweep: it_linux back on ubicloud-standard-4 (16GB, 5.3GB/shard) at k=3 — the k>1 failures were all software races, not RAM, so the cheap box may hold 3 shards. k=3@32GB measured ~2.1x vs k=1 (5 green legs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ine) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sweep: k=1 51.4min avg/leg, k=3 ~24min, k=4 ~22min (best), k=6 kills the VM (host OOM, runner shutdown signal, exit 137). Serial floor ~10min makes k=5 pointless next to the cliff. 2.3x faster IT legs vs k=1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…6 (VM death) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
xpack.ml.enabled: false moves to the universal base config (baseEsConfigBuilder): no suite exercises ML, and the ML native processes cost ~200-400MB RSS per container plus startup time. The key is supported across the whole matrix (6.3->9.x) unlike xpack.monitoring.enabled (removed in 8.0), so no version guard. XpackSecurityPlugin's explicit 'true' is removed (a re-add would duplicate the elasticsearch.yml key, which is fatal). k=5 probe verdict: host OOM on 5/6 heavier modules (same exit-137 VM death as k=6); only the lightest module survived. The RAM cliff is at k=5, so IT_PARALLELISM returns to the k=4 optimum (one notch of margin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-mode matrix Optimization iteration 1 (measured baseline: es94x leg 24.7 min at k=4): - Shard packing: replace duration-blind hashCode-mod with LPT bin-packing weighted by committed suite-timings.json (measured wall seconds per suite INCLUDING container boots, from run 29167332908 sharded-logs; junit-XML time= excludes boots and would mis-balance). Measured hash split was 20.1/11.6/17.8/13.4 min; planned LPT split is 17.0 x4. Unknown suites default to 60s; packing is deterministic across shard JVMs. TEST-xml upload flipped to always() so green runs refresh data. - Child shard daemons get -Xmx1024m (were inheriting gradle.properties' -Xmx6144m: ~6GB x K reserved on a 16GB box — the actual ceiling behind the k=5/k=6 host-OOM deaths, more than Elasticsearch). - DEV MODE (temporary, restore before merge): it_linux matrix = es94x only, it_windows skipped — one cheap leg per optimization iteration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d sleeps in admin suite
- it_linux -> ubicloud-premium-4 (same 4vCPU/16GB shape, ~2x CPU at 1.6x
price). Iteration-1 rerun showed LPT packing works (all shards level at
~17min planned / 19-22 actual) but one suite died on a 100s HTTP read
timeout: level shards = sustained full utilization, so container-heavy
inits lost the CPU slack that lopsided packing accidentally provided.
More per-core speed is the structural fix; revert if gain < 20%.
- BaseAdminApiSuite: the two fixed 15s "wait for 2nd node reload" sleeps
become eventually{} polls on the second instance's expected state
(patienceConfig already bounds them at the old sleep duration).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… 19.3-22 min) at 1.6x price Tests are wait-bound (ES boots, network), not CPU-bound, so faster cores barely help. Balanced shards + all fixes stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…starts Balanced shards keep all 4 workers at full load the whole run; the 16GB standard box died 3/3 times since (2 host-OOM, 1 timeout) while premium passed. Two memory-pressure cuts to keep the cheap box: - workers reuse the ROR plugin zip the parent prebuild already built (ROR_REUSE_ASSEMBLED=1, opt-in via ParallelProcessRunner only, so a plain local test run still rebuilds fresh) — removes one nested gradle build per worker at the busiest moment - 30s stagger between worker starts de-synchronizes the heaviest multi-container suite boots (costs ~1.5min wall worst-case) If this still dies on standard-4: keep hash sharding on standard, or pay for premium. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-PbalancedShards Balanced shards host-OOMed the 16GB standard runner 4/4 times (stagger and zip-reuse did not save it) while the same box was stable for two full runs with hash packing: the hash split's idle gaps double as memory slack. Balanced packing stays available for 32GB+ machines. The 30s start stagger is removed with it. Kept: worker zip reuse, 1GB worker daemons, poll-instead-of-sleep — none implicated, all pure wins. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
toolchains_verify, required_checks, cve_check, unit_tests move to ubuntu-latest (free for public repos, same 4cpu/16GB shape). Heavy IT legs and builds stay on Ubicloud. Temporary it_linux_ghfree probe runs the same leg on the free runner to measure the speed gap head-to-head. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p answered free-Linux probe Free-vs-paid Linux verdict: the free GitHub 16GB box host-OOMs under the 4-worker IT load (exit 137) while Ubicloud passes in 21.3 min — IT legs stay paid, light jobs stay free. Probe job removed. Windows: ShardedGradlewTest is now Windows-capable (cmd.exe wrapper for gradlew.bat; ProcessHandle reaping is already cross-platform). The dev-mode Windows leg (es94x) runs shardedTest with 3 workers on the free GH box (baseline ~70-78 min serial), plus the same leg on blacksmith-4vcpu-windows-2025 as a paid probe (continue-on-error). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… shard isolation - HeavySuiteGate: machine-wide file-lock permits (default off; CI sets 2) cap how many multi-cluster suites boot containers concurrently across shard JVMs. Converts the balanced-packing OOM deaths into short waits; self-healing (a crashed worker's lock dies with it). One permit per SUITE, taken before any of its clusters start — no hold-and-wait, no deadlock. Wraps run() via SuiteMixin (containers start inside run()). - Balanced packing re-enabled via ROR_BALANCED_SHARDS env (Linux+Windows IT jobs), with ROR_HEAVY_SUITE_PERMITS=2. - Windows sharding round 2: launch worked (3 shards ran 10-32min) but everything name-keyed collided — ES ports (fixed per node name) and install dirs (es_<cluster>_<node>). Both now offset per shard via the ror.shard.index system property (1000-port windows, per-shard dirs). - Blacksmith probe removed: Blacksmith requires a GitHub organization; personal-account jobs queue forever. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wnload cache The HeavySuiteGate decouples worker count from peak memory, unlocking the k=5 experiment that host-OOMed before the gate existed: 5 workers, max 3 concurrent heavy suites (below the ~4 that used to kill the box). Windows keeps 2 permits; its ES zip download cache is shared across shards again (the per-shard split made every shard re-download the distribution). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Windows (leg was 52.8 min; ~16.5 min setup + 36 min shards):
- ror-tools:test removed from per-leg runs (10 min/leg; every node
install already exercises the patcher; suite coverage moves to the
manual unit_tests_windows job)
- Windows Defender exclusions for workspace/temp/java.exe — real-time
scanning taxes every file write and our tests write hundreds of
thousands of files
- ES distribution unzipped ONCE per version into a lock-guarded shared
template, then robocopy /MT per node (was: single-threaded Java unzip
of ~600MB per node, through the AV filter)
Linux k=5 verdict so far: memory SOLVED by the gate (machine alive,
4/5 shards green 15.7-17.6 min, permit waits <=102s); the one failure
was a pre-existing race in IndexLifecycleManagementApiSuite — ILM
assigns the initial step asynchronously and the fixed-order move call
intermittently gets 400 under load. Wrapped the three step-moves in
eventually{}. k=5 stays for one more measured run.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All optimizations banked: k=5 sharding + HeavySuiteGate(3) + balanced packing on Linux (es94x 20.6 min, was 24.7); Windows sharded k=3 with Defender exclusions, template-copy ES installs, self-test moved to unit_tests_windows (es94x 40.8 min, was ~70-78). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
Full-matrix run 29198717346 showed k=5 host-OOMs 4/10 Linux modules (es80x/810x/818x/90x, exit 137) — they run suites es94x excludes, so their memory peaks exceed the es94x-tuned envelope. Back to k=4 with the HeavySuiteGate kept. Windows es818x exposed a fixed-port race: WireMock binds 8080/8081 on the shared host, so two shards starting wiremock-backed suites together collided. Same per-shard +k*1000 window as WindowsEsPortProvider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
es90x still host-OOM'd at k=4/permits=3: three 3-node heavies + a single-node suite + four gradle JVMs graze 16GB, and balanced packing front-loads the heaviest suites so the peak hits at minute one. Two permits cap the worst case ~12GB. Also disable machinery no suite exercises, version-guarded per key: watcher (6.3->9.x), built-in stack templates + ILM policies (8.0+), apm-data templates (8.15+), deprecation indexing (7.16+), SLM history (7.5+). Cuts per-node boot work and cluster-state bloat, biggest on the 8.x/9.x legs that were OOMing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
…arena CI packs 4-5 ES nodes on a 4-vCPU box; each node sizing its ~20 thread pools for 4 cores wastes ~1MB stack per surplus thread. node.processors (or 'processors' pre-7.4) tells each node it has 2. Test nodes never fill the default 240m JIT code cache either, and one Netty direct-memory arena covers test traffic. Applied to both docker and native-Windows paths; Windows shares esConfigFile so the yaml keys flow automatically. Deliberately NOT swapping the GC: ES pins G1/CMS per version+JDK in its own jvm.options and conflicting or removed GC flags are boot-fatal on several bundled JDKs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request ports CI orchestration from Azure Pipelines to GitHub Actions, including matrix-based Linux and Windows testing, validation, release, and publishing jobs. Integration tests gain deterministic hash or timing-based sharding, shard-specific outputs, cross-process locking, heavy-suite gating, and Windows resource isolation. Test infrastructure adds Elasticsearch configuration defaults and startup retry handling, while several asynchronous tests replace fixed waits with polling. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant setup
participant required_checks
participant it_linux
participant it_windows
participant determine_ci_type
participant release_ror
GitHubActions->>setup: compute execution flags and test matrices
setup->>required_checks: run repository validation
setup->>it_linux: start Linux integration shards
setup->>it_windows: start Windows integration shards
required_checks->>determine_ci_type: provide validation results
it_linux->>determine_ci_type: provide Linux test result
it_windows->>determine_ci_type: provide Windows test result
determine_ci_type->>release_ror: select release or test flow
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
integration-tests/src/test/scala/tech/beshu/ror/integration/suites/IndexLifecycleManagementApiSuite.scala (1)
117-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared ILM step JSON payloads to reduce triplication.
The
currentStepandnextStepJSON bodies are byte-for-byte identical across all threeeventuallyblocks (and also appear in the unchanged "be forbidden" test at lines 206-223). Defining them once as class-level vals would eliminate ~30 lines of duplication and make step-format changes a single-point edit.♻️ Proposed extraction
private lazy val adminIndexManager = new IndexManager(adminClient, esVersionUsed) private lazy val adminIndexLifecycleManager = new IndexLifecycleManager(adminClient, esVersionUsed) private lazy val dev1IndexLifecycleManager = new IndexLifecycleManager(basicAuthClient("dev1", "test"), esVersionUsed) private lazy val dev3IndexLifecycleManager = new IndexLifecycleManager(basicAuthClient("dev3", "test"), esVersionUsed) + private val newCompleteStep = ujson.read( + s""" + |{ + | "phase": "new", + | "action": "complete", + | "name": "complete" + |} + """.stripMargin + ) + private val warmForcemergeStep = ujson.read( + s""" + |{ + | "phase": "warm", + | "action": "forcemerge", + | "name": "forcemerge" + |} + """.stripMargin + ) + override implicit val patienceConfig: PatienceConfig = PatienceConfig(timeout = scaled(Span(30, Seconds)), interval = scaled(Span(200, Millis)))Then each
eventuallyblock simplifies to:eventually { val result = dev3IndexLifecycleManager.moveToLifecycleStep( index, - currentStep = ujson.read(s""" - |{ - | "phase": "new", - | "action": "complete", - | "name": "complete" - |} - """.stripMargin), - nextStep = ujson.read( - s""" - |{ - | "phase": "warm", - | "action": "forcemerge", - | "name": "forcemerge" - |} - """.stripMargin - ) + currentStep = newCompleteStep, + nextStep = warmForcemergeStep ) result should have statusCode 200 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/src/test/scala/tech/beshu/ror/integration/suites/IndexLifecycleManagementApiSuite.scala` around lines 117 - 201, Extract the identical ILM step JSON payloads used by the three `eventually` blocks and the unchanged “be forbidden” test into class-level vals. Update each `moveToLifecycleStep` call, including `currentStep` and `nextStep`, to reuse those shared values while preserving the existing payload contents and test behavior..github/workflows/ci.yml (1)
305-320: 🔒 Security & Privacy | 🔵 TrivialFork PRs execute on Ubicloud (self-hosted-style) runners.
it_linux(andbuild_ror) run onubicloud-standard-4for everypull_requestevent, including from public forks, since theirifconditions don't exclude fork-origin PRs. GitHub secrets are safely withheld from forks, but arbitrary PR-supplied code still executes on these non-GitHub-hosted VMs. The comments elsewhere in this file claim Ubicloud VMs are "ephemeral" (Line 71-72), which would mitigate persistence/lateral-movement risk similarly to GitHub-hosted runners — worth confirming that guarantee holds, and that fork-PR-run approval gating (Require approval for first-time contributorsor similar) is enabled at the repo level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 305 - 320, Prevent untrusted fork pull requests from executing on Ubicloud runners: update the it_linux and build_ror job conditions or runner selection so fork-origin PRs use GitHub-hosted runners or are skipped. Apply the same protection consistently to both jobs, and confirm repository approval gating is enabled for first-time fork contributors.build-base/src/main/java/tech/beshu/ror/buildbase/ParallelProcessRunner.java (1)
93-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGeneric runner hardcodes a project-specific env var.
ParallelProcessRunneris a generic parallel-process utility, but it unconditionally injects a Gradle/ROR-specific env var (ROR_REUSE_ASSEMBLED) into every command it runs. Harmless today sinceShardedGradlewTestis the only caller, but it couples orchestration policy into a reusable primitive. Consider accepting this as a per-command (or per-runner) option instead of hardcoding it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-base/src/main/java/tech/beshu/ror/buildbase/ParallelProcessRunner.java` around lines 93 - 95, Remove the hardcoded ROR_REUSE_ASSEMBLED environment injection from ParallelProcessRunner and expose environment customization as a per-command or per-runner option. Update ShardedGradlewTest or its caller to supply this variable explicitly, while keeping ParallelProcessRunner generic for other processes.tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala (1)
169-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the template lock/marker approach itself.
The
ensureTemplatedouble-checked file-lock pattern is correct and well-commented. One small, optional improvement: therobocopyresult is discarded after reading.exitCode— on failure (rc >= 8) the exception only reports the code, not robocopy's own diagnostic output, which was piped and thrown away. Capturingstdout/stderrin the exception message would speed up triage of Windows CI robocopy failures.🩹 Optional: surface robocopy output on failure
- val rc = os + val result = os .proc("robocopy", template.toString, targetDir.toString, "/E", "/MT:16", "/NFL", "/NDL", "/NJH", "/NJS", "/NP") .call(check = false, stdout = os.Pipe, stderr = os.Pipe) - .exitCode - if (rc >= 8) throw new IllegalStateException(s"robocopy of ES template failed with exit code $rc") + if (result.exitCode >= 8) + throw new IllegalStateException( + s"robocopy of ES template failed with exit code ${result.exitCode}: ${result.out.text()} ${result.err.text()}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala` around lines 169 - 238, Update unzipEs’s robocopy invocation to retain the process result, including its piped stdout and stderr, instead of only extracting exitCode. When rc >= 8, include the captured diagnostic output in the IllegalStateException while preserving the existing success handling for exit codes 0–7.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 138-139: Update every listed actions/checkout@v4 step in the
workflow, including the checkout near line 138 and the additional referenced
steps, to set persist-credentials: false in their with configuration. Preserve
the existing release_ror checkout setting of persist-credentials: true because
that job pushes a tag.
In `@ci/github/set-secrets.sh`:
- Around line 10-11: Update set_secret and set_var to distinguish blank inputs
from failed gh commands: explicitly check whether the value is empty and print
the existing skip message only in that case; otherwise invoke gh secret set or
gh variable set and preserve its nonzero failure status instead of routing
failures to the skip branch.
In
`@tests-utils/src/main/scala/tech/beshu/ror/utils/containers/HeavySuiteGate.scala`:
- Around line 69-81: Update the slot acquisition logic in HeavySuiteGate so
FileChannel is always closed when tryLock() throws, while preserving the
existing behavior for successful acquisition and null-lock cases. Use a
try/finally or equivalent guarded cleanup around channel.tryLock(), keeping the
channel open only when a lock is successfully stored in acquired.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 305-320: Prevent untrusted fork pull requests from executing on
Ubicloud runners: update the it_linux and build_ror job conditions or runner
selection so fork-origin PRs use GitHub-hosted runners or are skipped. Apply the
same protection consistently to both jobs, and confirm repository approval
gating is enabled for first-time fork contributors.
In
`@build-base/src/main/java/tech/beshu/ror/buildbase/ParallelProcessRunner.java`:
- Around line 93-95: Remove the hardcoded ROR_REUSE_ASSEMBLED environment
injection from ParallelProcessRunner and expose environment customization as a
per-command or per-runner option. Update ShardedGradlewTest or its caller to
supply this variable explicitly, while keeping ParallelProcessRunner generic for
other processes.
In
`@integration-tests/src/test/scala/tech/beshu/ror/integration/suites/IndexLifecycleManagementApiSuite.scala`:
- Around line 117-201: Extract the identical ILM step JSON payloads used by the
three `eventually` blocks and the unchanged “be forbidden” test into class-level
vals. Update each `moveToLifecycleStep` call, including `currentStep` and
`nextStep`, to reuse those shared values while preserving the existing payload
contents and test behavior.
In
`@tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala`:
- Around line 169-238: Update unzipEs’s robocopy invocation to retain the
process result, including its piped stdout and stderr, instead of only
extracting exitCode. When rc >= 8, include the captured diagnostic output in the
IllegalStateException while preserving the existing success handling for exit
codes 0–7.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82ffa427-8a22-42eb-b586-5ecbd6a23d5b
📒 Files selected for processing (23)
.github/actionlint.yaml.github/workflows/ci.ymlazure-pipelines.ymlbuild-base/src/main/java/tech/beshu/ror/buildbase/ParallelProcessRunner.javabuild-base/src/main/java/tech/beshu/ror/buildbase/ShardedGradlewTest.javaci/ci-lib.shci/docker-hub-auth.shci/github/PORT.mdci/github/SECRETS.mdci/github/set-secrets.shintegration-tests/build.gradleintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/IndexLifecycleManagementApiSuite.scalaintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/BaseAdminApiSuite.scalaintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/BaseAuditingToolsSuite.scalaintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scalaintegration-tests/suite-timings.jsontests-utils/src/main/scala/tech/beshu/ror/utils/containers/HeavySuiteGate.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/Wiremock.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsPortProvider.scalatests-utils/src/main/scala/tech/beshu/ror/utils/gradle/RorPluginGradleProject.scala
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala (1)
157-174: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRetry logic is well-structured; one consideration on
Thread.sleepinterrupt handling.The one-time delayed retry for the Docker classic-builder race is clean and well-documented.
Thread.sleep(5000)can throwInterruptedException, which would propagate as an unchecked exception and bypass the error logging at line 168. Sincestart()already throws on failure and the caller wraps this inTask.delay(...).runSyncUnsafe(...), this is low-risk in practice. However, catchingInterruptedExceptionexplicitly and either restoring the interrupt flag or falling through to the retry would make the behavior more robust.Optional: handle InterruptedException during sleep
case ex: Throwable if isDockerParentImageRace(ex) => logger.warn("Docker parent-image build race detected - retrying container start once", ex) - Thread.sleep(5000) + try Thread.sleep(5000) + catch { case _: InterruptedException => Thread.currentThread().interrupt() } try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala` around lines 157 - 174, Update the retry branch in the container start flow around isDockerParentImageRace and Thread.sleep(5000) to handle InterruptedException explicitly. Restore the thread’s interrupt status and route the interruption through the existing retry-failure logging/error path, preserving the one-time retry behavior and avoiding an unlogged escape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala`:
- Around line 157-174: Update the retry branch in the container start flow
around isDockerParentImageRace and Thread.sleep(5000) to handle
InterruptedException explicitly. Restore the thread’s interrupt status and route
the interruption through the existing retry-failure logging/error path,
preserving the one-time retry behavior and avoiding an unlogged escape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f1ec164-b9f0-4c8a-8153-3214de3a7ebf
📒 Files selected for processing (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala
After auditNode1.enableNetwork() the audit sink client can still hold the node in dead-host backoff and drop events (same as the all-nodes-out case the test itself documents). Querying immediately raced that recovery and flaked in 2 of 8 full-matrix runs under sharded CPU load. Now: probe until one audited request lands (recovery proven), truncate the sinks (existing pattern from earlier in the test), then assert on a clean slate. traceIds1/2 persistence was already asserted earlier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
…lidation Review points from PR #1292 (coutoPL + coderabbit): - Shard packing moved out of build.gradle into build-base SuiteSharder (pure function, 10 unit tests: disjoint cover, determinism, graceful degradation on missing/stale timings); build.gradle keeps only I/O - HeavySuiteGate replaced by FileLockSemaphore (tests-utils/misc): FP-style, permits+slotDir as constructor args, suiteName -> label; the env wiring lives at the call site (HeavySuiteGated in support.scala). Also closes the channel when tryLock throws (coderabbit) - RorShard util encapsulates ror.shard.index (port offset, sharded dir names); Wiremock/WindowsEsPortProvider/WindowsEsDirectoryManager use it - EsContainer docker-race retry extracted to a named private method - suite-timings.json embeds a _readme: what it is, advisory semantics, how and when to regenerate, ownership - SECRETS.md and set-secrets.sh deleted; the durable secrets/variables table folded into PORT.md (one CI doc instead of three files) - PR-comment-ish code comments removed/rewritten as timeless - persist-credentials: false on all 13 non-pushing checkouts (coderabbit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/misc/FileLockSemaphore.scala (1)
41-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a configurable timeout to
acquire.The current implementation blocks indefinitely. While the Javadoc notes self-healing on process death, a holder that hangs without crashing (deadlock, infinite loop) will never release its slot, causing all waiting shards to block until the CI job timeout — a silent hang with no diagnostic output. A configurable max-wait with a clear error message would fail fast and speed up diagnosis.
♻️ Optional timeout parameter
- /** Blocks until a slot is free. `label` is only a log tag. */ - def acquire(label: String): Slot = { + /** Blocks until a slot is free, or `timeoutMillis` elapses. `label` is only a log tag. */ + def acquire(label: String, timeoutMillis: Long = Long.MaxValue): Slot = { val start = System.currentTimeMillis() `@tailrec` def loop(attempt: Int): Slot = { + if (System.currentTimeMillis() - start > timeoutMillis) { + throw new TimeoutException(s"[$label] timed out after ${elapsedSeconds(start)}s waiting for a free slot") + } tryAcquireAnySlot() match { case Some(slot) => logger.info(s"[$label] slot acquired after ${elapsedSeconds(start)}s") slot case scala.None => if (attempt % 20 == 0) { logger.info(s"[$label] still waiting for a free slot (${elapsedSeconds(start)}s)") } Thread.sleep(3000) loop(attempt + 1) } } loop(attempt = 1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-utils/src/main/scala/tech/beshu/ror/utils/misc/FileLockSemaphore.scala` around lines 41 - 60, Update FileLockSemaphore.acquire to support a configurable maximum wait duration while retrying in its nested loop. Track elapsed time and, once the limit is reached, fail with a clear diagnostic message identifying the label and timeout instead of sleeping and recursing indefinitely; preserve the existing slot-acquisition and periodic waiting behavior before the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@tests-utils/src/main/scala/tech/beshu/ror/utils/misc/FileLockSemaphore.scala`:
- Around line 41-60: Update FileLockSemaphore.acquire to support a configurable
maximum wait duration while retrying in its nested loop. Track elapsed time and,
once the limit is reached, fail with a clear diagnostic message identifying the
label and timeout instead of sleeping and recursing indefinitely; preserve the
existing slot-acquisition and periodic waiting behavior before the timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 848e87d9-2e52-48ae-8992-ff508b243b1a
📒 Files selected for processing (15)
.github/workflows/ci.ymlbuild-base/src/main/java/tech/beshu/ror/buildbase/SuiteSharder.javabuild-base/src/test/java/tech/beshu/ror/buildbase/SuiteSharderTest.javaci/github/PORT.mdintegration-tests/build.gradleintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/BaseAuditingToolsSuite.scalaintegration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scalaintegration-tests/suite-timings.jsontests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/Wiremock.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scalatests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsPortProvider.scalatests-utils/src/main/scala/tech/beshu/ror/utils/misc/FileLockSemaphore.scalatests-utils/src/main/scala/tech/beshu/ror/utils/misc/RorShard.scala
💤 Files with no reviewable changes (1)
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scala
🚧 Files skipped from review as they are similar to previous changes (10)
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsPortProvider.scala
- integration-tests/suite-timings.json
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/Wiremock.scala
- integration-tests/build.gradle
- integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scala
- ci/github/PORT.md
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala
- integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/BaseAuditingToolsSuite.scala
- .github/workflows/ci.yml
…p retries Run #10 exposed both: - The single post-recovery audit probe could itself be DROPPED (sent while the sink client still holds all nodes in dead-host backoff) and then no amount of waiting helps. waitUntilAuditSinkRecovers() now re-sends fresh probes (10s poll each, 180s overall) until one lands, then the test truncates and asserts as before. - osixia/openldap occasionally exits 1 on first boot under CI load; withStartupAttempts(2) re-creates it once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
… sides The guard diffs the base-version jar against the target-version jar and expects byte identity. With org.gradle.caching=true a child compile can resurrect one side FROM-CACHE (written by an earlier invocation in the same job) while the other compiles fresh — and jars from different compile sessions are never byte-identical, so the guard false-alarms. Seen on build_ror in run 29253349548 (7 FROM-CACHE hits, all 4 modules red) while the same commit passes locally and the same comparison was green in prior runs with 0 cache hits. Both child compiles now run with --no-build-cache, making the comparison hermetic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
…n, naming - ci/github/PORT.md replaced by ci/README.md: describes the pipeline as it IS (jobs, parallelism, secrets, tuning limits) — no transition history; the Azure mapping moves to the PR description where it belongs - suite-timings drift is now auto-detected: integration-tests: regenerateSuiteTimings rebuilds the file from the run's junit XMLs, warns (::warning:: annotations) on drift, and the es94x leg uploads the regenerated file as an artifact — updating = download + commit - suite-timings.json parsing moved into SuiteSharder (timingsFrom), covered by a unit test; build.gradle keeps only the file handle - ci.yml usability: matrix jobs named by their distinguishing value (integration_es94x / win_es94x / audit_build_check...) instead of the truncated composite; shard logs printed as collapsible groups in the job console (browsable, still uploaded as artifacts); cve_check warns explicitly when NVD_API_KEY is missing; run_disk_probe manual action ported (the last one) - all 5 Azure manual actions now present - RemoteClusterAuditingToolsSuite: probeLanded -> entryVisibleInAllSinks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7ZwM6WNAkeHKqSjxnqgPa
H2 SHUTDOWN DEFRAG closes the database during defrag. When dependency-check reopens the DB in read-only mode for analysis, a relative path fails the isFile() check because the CWD context is lost after H2 closes the connection. Resolving to an absolute path before invoking Gradle fixes this.
…0x timings leg, coverage table - cve_check moves into an optional_checks matrix job mirroring Azure's Optional Checks (continue-on-error = warn-but-pass); name = matrix value - unit_tests renamed unit_tests_linux (symmetry with unit_tests_windows) - suite-timings drift leg pinned to es90x — the first ES 9 module, a stable reference that never moves when a newer es9Xx module lands - ci.yml top design-notes comment dropped (ci/README.md owns the architecture) - ci/README.md: full Azure-stage -> GH-job coverage table (nothing was dropped in the port; azure-pipelines.yml stays as a triggerless manual fallback for one release cycle, then gets deleted) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSyUyjGmV6wXtn3TPbxUbh
Conflict resolutions (3 files): - verify-repackage-bytecode.gradle: develop's RORDEV-2074 rewrite taken wholesale — it supersedes this branch's --no-build-cache fix (adds clean + no-build-cache + jar stashing + per-group verification) - build-base/build.gradle: develop's reformatted deps + this branch's localGroovy() (groovy-json for SuiteSharder) - RorPluginGradleProject.scala: semantic merge — develop's memoized assemble + verifyRepackageBytecode-before-build kept; this branch's ROR_REUSE_ASSEMBLED shard-worker reuse path and the cross-process file lock around nested gradle builds woven in. Shard workers reuse the parent-prebuilt (and now parent-VERIFIED) zip; local runs still verify+build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSyUyjGmV6wXtn3TPbxUbh
…t logic, checked port windows - FileLocks.withExclusiveLock: the ONE cross-process mutex helper; replaces the two hand-rolled FileChannel blocks (RorPluginGradleProject.runTask, WindowsEsDirectoryManager.ensureTemplate) - SuiteTimings (build-base): drift detection extracted pure with named thresholds (DRIFT_ABS_SECONDS=60, DRIFT_REL=0.5) + 7 unit tests; the regenerateSuiteTimings task keeps only the XML/JSON I/O (same split as SuiteSharder) - HeavySuiteGated: fails loudly when ROR_HEAVY_SUITE_PERMITS is set but -Dproject.dir is missing — a tmpdir fallback silently disabled the machine-wide OOM cap (per-JVM slot dirs) - RorShard: PortWindowSize named + shardedBasePort(base, rangeWidth) with a window-overflow require; WindowsEsPortProvider/Wiremock derive from it - GradlewCommand (build-base): single owner of "how to launch gradlew from a child process on this OS" (cmd.exe /c + gradlew.bat on Windows), used by ShardedGradlewTest and verify-repackage-bytecode.gradle Verified: build-base:test green (SuiteSharder + SuiteTimings), tests-utils + integration-tests test classes compile, formatCodeCheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSyUyjGmV6wXtn3TPbxUbh
…hy the mutex is not a 1-permit semaphore) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GSyUyjGmV6wXtn3TPbxUbh
This PR moves our CI from Azure Pipelines to GitHub Actions. Linux jobs run on Ubicloud runners (4 CPU / 16 GB). Windows jobs run on GitHub's own
windows-2025machines (free for public repos). Azure triggers are turned off in this same PR, but the Azure pipeline can still be run by hand as a fallback until we delete it.The build scripts themselves are untouched — every job still calls
ci/run-pipeline.shwith aROR_TASK, exactly like on Azure. Only the orchestration around them was rewritten.The headline numbers
A full PR run now takes ~55 minutes. On Azure it took 4h48. Cost is about $0.50 per PR run (~$14/month).
What's in the PR
.github/workflows/ci.yml— all 14 Azure stages as GitHub jobs. Test matrices are computed per trigger: full 34 Linux / 33 Windows versions on develop/master, a 10 / 3 subset on PRs. Skipped versions never start a VM.integration-tests/suite-timings.json).HeavySuiteGate) limits how many multi-node-cluster suites can boot at the same time. Without it, 16 GB machines run out of memory — we measured this, repeatedly.ci/docker-hub-auth.sh, a WireMock port clash between parallel Windows JVMs, an ILM test race, and a too-short wait in the audit tests.ci/github/PORT.md(how the port maps to Azure, and how the parallelism works),ci/github/SECRETS.md(all 17 secrets + 8 variables to set, with a helper script).The current settings (4 JVMs, gate of 2, balanced packing) are the measured sweet spot for this machine size. We tried going further — 5 JVMs, gate of 3, smaller heaps, deleting unused ES modules from the image — each on an isolated one-leg test run. Every one of them hit a wall (out of memory, boot timeouts, or an ES-internal module dependency), so this is where the knobs stay.
Validation
The full matrix (10 Linux + 3 Windows legs) ran green end to end, including the release-path job conditions (checked against a written scenario table for every trigger type). Two independent review passes over the whole diff found no correctness issues.
Azure → GitHub Actions mapping (reviewer reference — the current-state architecture lives in
ci/README.md)concurrency(cancel-in-progress for PRs only; branch pushes queue, so a release run is never cancelled)disk_proberun_disk_probees_s3_upnewes/*branches; ordered before all check/test jobsbuild_toolchains_imagetoolchains_verifycve_checkrequired_checksunit_testsit_linuxit_windowsunit_tests_windowsbuild_ror!cancelled()+ explicitneeds.<job>.result(GH skips dependents of skipped jobs — this keepsrelease_without_testingworking)All 5 Azure manual actions are ported:
run_all_tests_on_linux,run_all_tests_on_windows,build_toolchains_image,release_without_testing,run_disk_probe.Azure-feature translations:
$(System.AccessToken)→GITHUB_TOKEN;##vso[task.setvariable]→$GITHUB_OUTPUT; secure filesecret.pgp→ base64 secret decoded in-step;Cache@2monthly CVE key →actions/cache+date +%Y%m; tag push viapermissions: contents: write(no SSH deploy key).Deliberately different from Azure: markdown-only changes anywhere skip CI (Azure's globs couldn't express that); the docker pre-clean/reap steps are dropped (runners are ephemeral VMs).
Before merging
ci/github/set-secrets.sh, values map 1:1 from the Azure variable group).cve_checkgreen (it's the only red job).🤖 Generated with Claude Code
Summary by CodeRabbit