fix(nats): add self-managed regional placement tag - #1370
Conversation
Expose the logical NCP region to JetStream so regional request queues can be placed on the bundled NATS cluster. Closes #1365 Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe NATS chart and self-managed stack now include ChangesNATS placement tags
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds the required regional placement tag for self-managed NATS without changing pods, storage, or resource requests. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
The apparent historical success is explained by two different components being able to create the same request-queue stream.
That mismatch was usually masked while at least one worker existed because the API created the stream first. Invocation found the existing stream and never exercised its own sequenceDiagram
participant Client
participant Invocation
participant API as NVCF API
participant Worker
participant NATS
Note over NATS: Before this PR: server_tags = dc:ncp
rect rgb(235, 255, 235)
Note over API,Worker: Existing worker or minimum instances greater than zero
Worker->>API: Establish request consumer
API->>NATS: Create rq_ncp_VERSION with placement dc:ncp
NATS-->>API: Stream created
Client->>Invocation: Invoke function
Invocation->>NATS: Publish rq.ncp.VERSION.REQUEST
NATS-->>Invocation: Acknowledged by existing stream
Note over Invocation,NATS: Invocation's aws-region:ncp create path is not reached
end
rect rgb(255, 240, 235)
Note over API,Worker: Autoscaler allows scale to zero, so no worker creates the stream
Client->>Invocation: First invocation from zero
Invocation->>NATS: Publish rq.ncp.VERSION.REQUEST
NATS-->>Invocation: StreamNotFound
Invocation->>NATS: Create rq_ncp_VERSION with placement aws-region:ncp
NATS-->>Invocation: No eligible server has that tag
end
The autoscaler does not introduce or consume this placement tag. It exposes the latent mismatch by making the no-worker, no-precreated-stream path normal. Vanity gateway is not involved in the JetStream lifecycle; it only provides the alternate ingress path that the smoke coverage also exercises. This PR stays focused on making the bundled NATS server eligible for both existing creators by advertising |
1 participant
TL;DR
Add the logical NCP region tag required for JetStream to place region-scoped request queues on the bundled self-managed NATS cluster.
Additional Details
The stack supplied
dc:ncpbut notaws-region:ncp. Regional request-queue streams select the latter tag, so their placement could fail even though NATS was healthy.The NATS chart now includes both tags by default. The self-managed stack passes the tag list to the currently released chart and still permits environment overrides. Focused tests cover the chart render, default stack values, and custom stack tags.
Customer Release Notes
Self-managed NATS now accepts region-scoped function request queues for the default NCP region.
Plan Summary
The NATS server configuration gains one bounded placement tag. No pods, storage, or resource requests change.
Usage
No operator action is required. Existing environments can replace the default tag list through
nats.config.merge.server_tags.For the Reviewer
Please focus on the Helmfile value merge and the environment override case in
nats-placement-tags.sh.For QA
QA needed: no additional manual QA beyond the final BDD integration run.
Tests run:
make testindeploy/helm/natsdeploy/stacks/self-managed/tests/nats-placement-tags.shgit diff --checkThe first chart test attempt identified a missing local Helm repository. Adding the repository declared in
Chart.yamlresolved the prerequisite, and the suite passed.Notes
The autoscaler smoke in #1363 exercises the regional request-queue path after this PR is available on
main.Issues
Closes #1365
Related Pull Requests
Dependencies
No new or updated third-party dependencies. NOTICE is unchanged.
Checklist
Summary by CodeRabbit
New Features
Tests