You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2025/08/11 08:47:44 UTC
[PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Revolyssup opened a new pull request, #12509:
URL: https://github.com/apache/apisix/pull/12509
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268579130
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
yes you're correct. I missed this, let me correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268625340
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
the healthcheck_manager module is tied only to manage the checkers on the upstream configuration. I don't know if we can use that mechanism here because here we are creating custom healthchecker using the `checks` configuration in the plugin configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268625340
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
the healthcheck_manager module is tied only to manage the checkers on the upstream configuration. I don't know if we can use that mechanism here because here we are creating custom healthchecker using the checks configuration in the plugin configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268577473
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
https://github.com/apache/apisix/pull/12426
this PR has beed merge
so the parent should be a resource path, do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2269857119
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,30 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
Review Comment:
This is not an error. `plugins` may or may not exist on the route, if they dont then nothing needs to be attached and this logic can be silently skipped
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2269645386
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,30 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
Review Comment:
return err
##########
apisix/healthcheck_manager.lua:
##########
@@ -224,7 +237,17 @@ local function timer_create_checker()
if not res_conf then
goto continue
end
- local upstream = res_conf.value.upstream or res_conf.value
+ local upstream
+ local plugin_name = get_plugin_name(resource_path)
+ if plugin_name and plugin_name ~= "" then
+ local json_path = "$." .. (resource_path:match("#(.+)$") or "")
+ local tab = jp.value(res_conf.value, json_path)
Review Comment:
need a meaning name
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,30 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local parent_info = {
+ resource_key = parent.key,
+ resource_version = tostring(parent.modifiedIndex)
+ }
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent_info
+ else
+ plugin_conf._meta = setmetatable(plugin_conf._meta,
+ { __index = {parent = parent_info} })
+ end
+ end
+ end
Review Comment:
return true
##########
apisix/healthcheck_manager.lua:
##########
@@ -60,6 +61,9 @@ end
local function fetch_latest_conf(resource_path)
+ -- if resource path contains json path, extract out the prefix
+ -- for eg: extracts /routes/1 from /routes/1#plugins.abc
+ resource_path = resource_path:match("^(.-)#") or resource_path
Review Comment:
use `local res = ngx.re.split(...)`
the `res[1]` is resource path, `res[2]` is sub resource path
we can add a new function to parse the path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2269557927
##########
apisix/healthcheck_manager.lua:
##########
@@ -206,6 +210,15 @@ function _M.upstream_version(index, nodes_ver)
end
+local function get_plugin_name(path)
+ -- Extract JSON path (after '#') or use full path
+ local json_path = path:match("#(.+)$") or path
+ -- Match plugin name in the JSON path segment
Review Comment:
yes. like `plugins["ai-proxy-multi"].`
`plugins.ai-proxy-multi.` will break
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup merged PR #12509:
URL: https://github.com/apache/apisix/pull/12509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268579130
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
yes you're correct. I missed this, let me correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
membphis commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2268656492
##########
apisix/plugin.lua:
##########
@@ -1234,6 +1236,26 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx, plugin_run
end
+function _M.set_plugins_meta_parent(plugins, parent)
+ if not plugins then
+ return
+ end
+ for _, plugin_conf in pairs(plugins) do
+ if not plugin_conf._meta then
+ plugin_conf._meta = {}
+ end
+ if not plugin_conf._meta.parent then
+ local mt_table = getmetatable(plugin_conf._meta)
+ if mt_table then
+ mt_table.parent = parent
Review Comment:
yes, i think we have to use the `healthcheck_manager` way
the old way `parent` is bad way which was deprecated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(ai-proxy-multi): add support for healthcheck [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #12509:
URL: https://github.com/apache/apisix/pull/12509#discussion_r2269555361
##########
apisix/healthcheck_manager.lua:
##########
@@ -206,6 +210,15 @@ function _M.upstream_version(index, nodes_ver)
end
+local function get_plugin_name(path)
+ -- Extract JSON path (after '#') or use full path
+ local json_path = path:match("#(.+)$") or path
+ -- Match plugin name in the JSON path segment
Review Comment:
For plugin names that contain hyphens, do we must to enclose them in quotation marks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org