[RUN-4802] Add opt-in parallel VM querying to Azure node resource model - #95
Conversation
Mirrors the AWS EC2 fix (RUN-3100 / rundeck-ec2-nodes-plugin#176). Adds a queryNodeInstancesInParallel plugin property (default false) that, when enabled, maps VMs to nodes concurrently on a fixed thread pool instead of sequentially, cutting refresh time for accounts with many VMs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in plugin configuration (queryNodeInstancesInParallel, default false) to allow Azure VM→node mapping to run concurrently during resource model refresh, intended to reduce refresh time for subscriptions with many VMs while keeping default behavior unchanged.
Changes:
- Adds a new resource model source property
queryNodeInstancesInParalleland wires it throughAzureResourceModelSource→AzureManagerBuilder. - Updates
AzureManager.listVms()to optionally map VMs toAzureNodeconcurrently using anExecutorService.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureResourceModelSourceFactory.groovy | Adds the new opt-in configuration property definition. |
| src/main/groovy/com/rundeck/plugins/azure/plugin/AzureResourceModelSource.groovy | Reads the new property from configuration and passes it into AzureManagerBuilder. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureManagerBuilder.groovy | Adds builder support for queryNodeInstancesInParallel and sets it on AzureManager. |
| src/main/groovy/com/rundeck/plugins/azure/azure/AzureManager.groovy | Implements parallel VM→node mapping when the opt-in flag is enabled. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jtobard
left a comment
There was a problem hiding this comment.
Hey @carlosrfranco — reviewed this against RUN-4802. The approach looks right
and lines up well with the per-account latency described in the ticket. A
couple of things to address before merging, both raised by Copilot and
confirmed valid:
- Bound the thread pool and add a timeout.
Executors.newFixedThreadPool(list.size()) in AzureManager.listVms() sizes
the pool to VM count with no cap, and future.get() has no timeout. Unlike
the AWS precedent (PR #176), which bounds the pool by endpoint/region count
(small and stable), this one scales with VM count — which is exactly the
axis this flag is meant to help on large subscriptions. Worth capping it
(e.g. Math.min(list.size(), N) with a small fixed max) and adding a timeout
to invokeAll/future.get() so a hung SDK call can't block the refresh
indefinitely. - Add test coverage for the new concurrency path. Right now there's no test
exercising queryNodeInstancesInParallel end to end — not the config →
builder wiring, and not the parallel branch in listVms() itself (executor
creation, exception translation, shutdown/timeout handling). Since this is
new concurrency logic, it'd be good to have at least one spec covering the
parallel path before it ships.
Everything else checks out — pulled the branch and ran the tests locally,
build is green (./gradlew test passes), consistent with what you noted in the
PR description.
On the failing Snyk check: that's safe to ignore for this PR — it's a
pre-existing high-severity finding in spring-security-crypto pulled in
transitively via rundeck-core@6.1.0, unrelated to this change (would fail on
main today too, and there's no upstream fix yet).
Bound the thread pool used for parallel VM querying to a fixed max (10) instead of one thread per VM, and use a timed invokeAll that fails fast with a clear error if a query hangs past the timeout, instead of blocking indefinitely. Extract the configuration-to-AzureManagerBuilder wiring into a createManager() method and add focused tests asserting that queryNodeInstancesInParallel is read from Properties and passed through to the built AzureManager. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jtobard
left a comment
There was a problem hiding this comment.
Re-reviewed after b9d4500. Both blockers from the previous pass are resolved:
- Thread pool is now bounded (
min(list.size(), 10)) with a 120s timeout oninvokeAll, and a timed-out future now raises a clearResourceModelSourceExceptioninstead of hanging or leaking a bareCancellationException. queryNodeInstancesInParallelconfig wiring now has dedicated test coverage via the extractedcreateManager()method (true/false + default-unset cases).
Snyk is still red, but it's now flagging license-policy issues (EPL/CDDL on junit/spock/azure-sdk transitive deps) with no build.gradle changes in this PR — looks like a policy/DB drift between scans, unrelated to this change. Safe to ignore for merge purposes.
Approving.
Jira Ticket
RUN-4802
Summary
Adds an opt-in
queryNodeInstancesInParallelplugin property (defaultfalse) to the Azure Resource Model source, mirroring the AWS EC2 fix (RUN-3100 / rundeck-ec2-nodes-plugin#176). When enabled,AzureManager.listVms()maps VMs to nodes concurrently on a fixed thread pool instead of sequentially, cutting refresh time for accounts with many VMs. Sequential behavior is unchanged by default.Note: implemented on top of the current Track 1 Azure SDK (
main). PR #84 (RUN-4692) is an in-flight Track 2 SDK migration that rewritesAzureManager/AzureManagerBuilder/AzureNode; once that merges, this logic will need to be re-applied on the new API shapes../gradlew compileGroovyand./gradlew testboth pass.