feat: upgrade apisix-runtime to OpenResty 1.29.2.4#461
Closed
jarvis9443 wants to merge 10 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBump OpenResty to 1.29.2.4, add apisix-nginx-module commit pinning and script-dir support, switch OpenSSL install targets, conditionally apply an nginx-1.29.2 ngx_multi_upstream patch, and update patch.sh to detect openresty-1.29.2.*. ChangesBuild Runtime Version and Commit Management
ngx_multi_upstream_module (nginx-1.29.2) Patch
Sequence Diagram(s)sequenceDiagram
participant Client
participant ngx_http_upstream
participant ngx_connection
participant ngx_http_multi_upstream
participant UpstreamPeer
Client->>ngx_http_upstream: upstream request
ngx_http_upstream->>ngx_connection: select peer connection (u->peer.connection)
alt u->multi set
ngx_connection->>ngx_http_multi_upstream: enqueue/reuse via multi handlers
ngx_http_multi_upstream->>UpstreamPeer: manage backend multiplexing/connect
UpstreamPeer-->>ngx_http_multi_upstream: connection/data
ngx_http_multi_upstream-->>ngx_connection: signal reuse / NGX_DONE
ngx_connection-->>ngx_http_upstream: multi read handlers return
else normal path
ngx_connection->>UpstreamPeer: direct connect/send
UpstreamPeer-->>ngx_connection: response
ngx_connection-->>ngx_http_upstream: normal read handler
end
ngx_http_upstream-->>Client: finalize request/response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Upgrades the build-apisix-runtime.sh build script to target a newer OpenResty release (1.29.2.4) for building the APISIX runtime, along with aligning apisix-nginx-module to a compatible patch branch.
Changes:
- Bump OpenResty version used by the runtime build from 1.27.1.2 to 1.29.2.4.
- Update
apisix-nginx-modulereference from a release version to an OpenResty 1.29.2.4 patch branch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
patches/ngx_multi_upstream_module/nginx-1.29.2.patch (2)
198-207: 💤 Low valueConsider adjusting log level for normal finalization path.
Line 200 uses
NGX_LOG_ERRfor finalizing a fake multi-connection request. If this is a normal code path (not an actual error),NGX_LOG_INFOorNGX_LOG_DEBUGwould be more appropriate to avoid log noise in production. If this is intentionally an error-only path, the current level is fine.🤖 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 `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch` around lines 198 - 207, The log call that reports finalization of a fake multi-connection request currently uses NGX_LOG_ERR; if this is a normal/expected code path change the log level to NGX_LOG_INFO or NGX_LOG_DEBUG to reduce noise (update the ngx_log_error invocation in the block that checks u->multi and ngx_http_multi_connection_fake(r)), otherwise leave as-is for true error conditions; ensure you adjust the single call that logs "http finalize upstream fake_r %p" and keep the rest of the flow that calls ngx_http_multi_upstream_finalize_request(r->connection, rc) unchanged.
155-159: 💤 Low valueInconsistent preprocessor conditional style.
Line 155 uses
#if T_NGX_MULTI_UPSTREAMwithout parentheses, while all other occurrences in this patch use#if (T_NGX_MULTI_UPSTREAM). Consider using consistent style throughout.♻️ Suggested style fix
-#if T_NGX_MULTI_UPSTREAM +#if (T_NGX_MULTI_UPSTREAM) if (u->multi && rc == NGX_OK) { ngx_multi_clean_leak(u->peer.connection); }🤖 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 `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch` around lines 155 - 159, The preprocessor conditional around the ngx_multi_clean_leak call is inconsistent with the rest of the patch; change the directive from `#if T_NGX_MULTI_UPSTREAM` to `#if (T_NGX_MULTI_UPSTREAM)` so it matches the existing style used elsewhere. Update the conditional that guards the block checking `u->multi && rc == NGX_OK` (which calls `ngx_multi_clean_leak(u->peer.connection)`) to use the parenthesized macro form `#if (T_NGX_MULTI_UPSTREAM)` and ensure the matching `#endif` remains unchanged.
🤖 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 `@build-apisix-runtime.sh`:
- Around line 101-105: The patch file lookups in the OPENRESTY_VERSION branch
use $prev_workdir (references: ngx_multi_upstream_module_patch and prev_workdir)
which breaks local-source/module-repo runs; change those paths to be anchored to
the script's directory instead of the caller CWD by referencing the existing
script-dir variable (or compute SCRIPT_DIR via dirname "$0") and use that when
copying and when supplying the patch file to patch (targets: the cp of
"$prev_workdir/$ngx_multi_upstream_module_patch" and the patch input
"$prev_workdir/patches/ngx_multi_upstream_module/patch-sh-openresty-1.29.2.patch"
that feed ngx_multi_upstream_module-${ngx_multi_upstream_module_ver}).
In `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch`:
- Around line 90-95: The connect-error branch currently logs the error and
returns without finalizing the HTTP upstream request; update the else branch
that logs "multi: connect return %i error" (uses variables rc and c) to finalize
the request before returning by invoking ngx_http_upstream_finalize_request(r,
u, NGX_HTTP_INTERNAL_SERVER_ERROR) (or ngx_http_finalize_request(r,
NGX_HTTP_INTERNAL_SERVER_ERROR) if upstream context doesn't exist) so the
request is properly completed and resources are released.
---
Nitpick comments:
In `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch`:
- Around line 198-207: The log call that reports finalization of a fake
multi-connection request currently uses NGX_LOG_ERR; if this is a
normal/expected code path change the log level to NGX_LOG_INFO or NGX_LOG_DEBUG
to reduce noise (update the ngx_log_error invocation in the block that checks
u->multi and ngx_http_multi_connection_fake(r)), otherwise leave as-is for true
error conditions; ensure you adjust the single call that logs "http finalize
upstream fake_r %p" and keep the rest of the flow that calls
ngx_http_multi_upstream_finalize_request(r->connection, rc) unchanged.
- Around line 155-159: The preprocessor conditional around the
ngx_multi_clean_leak call is inconsistent with the rest of the patch; change the
directive from `#if T_NGX_MULTI_UPSTREAM` to `#if (T_NGX_MULTI_UPSTREAM)` so it
matches the existing style used elsewhere. Update the conditional that guards
the block checking `u->multi && rc == NGX_OK` (which calls
`ngx_multi_clean_leak(u->peer.connection)`) to use the parenthesized macro form
`#if (T_NGX_MULTI_UPSTREAM)` and ensure the matching `#endif` remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc68cc99-2a86-4681-907a-5b6d3f457d90
📒 Files selected for processing (3)
build-apisix-runtime.shpatches/ngx_multi_upstream_module/nginx-1.29.2.patchpatches/ngx_multi_upstream_module/patch-sh-openresty-1.29.2.patch
✅ Files skipped from review due to trivial changes (1)
- patches/ngx_multi_upstream_module/patch-sh-openresty-1.29.2.patch
| if [[ "$OPENRESTY_VERSION" == 1.29.2.* ]]; then | ||
| cp "$script_dir/$ngx_multi_upstream_module_patch" \ | ||
| ngx_multi_upstream_module-${ngx_multi_upstream_module_ver}/nginx-1.29.2.patch | ||
| patch -d ngx_multi_upstream_module-${ngx_multi_upstream_module_ver} -p0 \ |
| + ngx_http_multi_upstream_finalize_request(c, | ||
| + NGX_HTTP_INTERNAL_SERVER_ERROR); | ||
| + ngx_log_error(NGX_LOG_ERR, c->log, 0, | ||
| + "multi: upstream configured multi, but handler no support"); |
Comment on lines
+156
to
+161
| } else { | ||
| +#if T_NGX_MULTI_UPSTREAM | ||
| + if (u->multi && rc == NGX_OK) { | ||
| + ngx_multi_clean_leak(u->peer.connection); | ||
| + } | ||
| +#endif |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/ngx_multi_upstream_module/nginx-1.29.2.patch (1)
75-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the multi-connect result handling.
Line 75 only recognizes
NGX_AGAINand Line 81 only recognizesNGX_DONE, so an immediateNGX_OKfromngx_event_connect_peer()falls into the error path even though the connect succeeded. Also, theconnect reuse unfinishedbranch at Line 88 just logs and then returns at Line 97, which leaves that request hanging. Please handleNGX_OKas a successful connect path and finalize/retry instead of returning after the unfinished-reuse log.🤖 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 `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch` around lines 75 - 95, The rc handling must treat NGX_OK as a successful connect (like NGX_AGAIN) and must not leave requests hanging when reuse is unfinished: update the branch that checks rc so that NGX_OK goes through the same successful-connect path as NGX_AGAIN (set c->read->handler and c->write->handler to ngx_http_multi_upstream_connect_handler, call ngx_add_timer with u->conf->connect_timeout and log the connect), and in the ngx_done reuse branch where ngx_multi_connected(c) is false, after logging "multi: connect reuse unfinished %p" call ngx_http_multi_upstream_finalize_request(c, NGX_HTTP_INTERNAL_SERVER_ERROR) (or trigger the existing retry/finalize mechanism) instead of simply returning so the request is completed.
🤖 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.
Outside diff comments:
In `@patches/ngx_multi_upstream_module/nginx-1.29.2.patch`:
- Around line 75-95: The rc handling must treat NGX_OK as a successful connect
(like NGX_AGAIN) and must not leave requests hanging when reuse is unfinished:
update the branch that checks rc so that NGX_OK goes through the same
successful-connect path as NGX_AGAIN (set c->read->handler and c->write->handler
to ngx_http_multi_upstream_connect_handler, call ngx_add_timer with
u->conf->connect_timeout and log the connect), and in the ngx_done reuse branch
where ngx_multi_connected(c) is false, after logging "multi: connect reuse
unfinished %p" call ngx_http_multi_upstream_finalize_request(c,
NGX_HTTP_INTERNAL_SERVER_ERROR) (or trigger the existing retry/finalize
mechanism) instead of simply returning so the request is completed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e92cdbe-8633-4eab-93ef-6392ea3dec0b
📒 Files selected for processing (2)
build-apisix-runtime.shpatches/ngx_multi_upstream_module/nginx-1.29.2.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade apisix-runtime from OpenResty 1.27.1.2 to 1.29.2.4.
This temporarily points apisix-nginx-module to the OpenResty 1.29.2.4 patch branch from api7/apisix-nginx-module#113 and pins the checkout to a concrete commit. It should be switched to the release tag once that patch set is merged and tagged.
This also carries a temporary ngx_multi_upstream_module patch for nginx 1.29.2 because the current 1.3.2 release only detects OpenResty up to 1.27.1. That local patch should be removed after ngx_multi_upstream_module ships 1.29.2 support.
Verification:
Summary by CodeRabbit