Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(*): port several PDK and shared llm module from ai-proxy-advanced feature #13347

Merged
merged 5 commits into from
Aug 2, 2024

Conversation

fffonion
Copy link
Contributor

@fffonion fffonion commented Jul 8, 2024

Summary

This PR adds several PDK methods to allow ai-proxy-advanced plugin to do customized load balancing and failover.
It also moves the ai-proxy handler to a shared module so that both ai-proxy{,-advanced} plugins can reuse same codebase.

https://github.com/Kong/kong-ee/pull/9562

Checklist

  • The Pull Request has tests
  • [na] A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • [na] There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

AG-12

@fffonion fffonion added skip-changelog and removed cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Jul 8, 2024
@AndyZhang0707 AndyZhang0707 requested review from samugi, liverpool8056 and jschmid1 and removed request for samugi July 8, 2024 09:19
@@ -359,6 +359,16 @@ local function execute(balancer_data, ctx)
balancer_data.balancer_handle = handle

else
-- Note: balancer_data.retry_callback is only set by PDK once in access phase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the retry_callback can be invoked only when balancer == nil, is it expected to work like this?

In addition, should we wrap retry_callback with pcall?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the retry_callback can be invoked only when balancer == nil, is it expected to work like this?

Yes, if we skip the balancer lookup by doing kong.service.set_target, i.e. currently request doesn't have a upstream, then balancer, upstream = balancers.get_balancer(balancer_data) will return no balancer. Actually, this else branch is just made to support kong.service.set_target.
This matches our expectation, retry_callback will not be called if Kong's balancer is not skipped.

In addition, should we wrap retry_callback with pcall?

yes, adding that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address all comments in EE side of PR and port here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@fffonion
Copy link
Contributor Author

Reviewers: please review https://github.com/Kong/kong-ee/pull/9562 first for a whole picture, I will then address comments there and port here.

@fffonion fffonion marked this pull request as draft July 11, 2024 11:22
@fffonion fffonion marked this pull request as ready for review July 25, 2024 07:05
@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jul 25, 2024
@fffonion fffonion merged commit 83fce6b into master Aug 2, 2024
25 checks passed
@fffonion fffonion deleted the aip-ce-port branch August 2, 2024 05:47
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13347-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13347-to-master-to-upstream
git checkout -b cherry-pick-13347-to-master-to-upstream
ancref=$(git merge-base 982143769aa5bd271307fa70840051da1ca32a08 360b4f0e7d8a7d2a7e1ca373fd4f7d43a76b37e1)
git cherry-pick -x $ancref..360b4f0e7d8a7d2a7e1ca373fd4f7d43a76b37e1

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 2, 2024
@fffonion
Copy link
Contributor Author

fffonion commented Aug 2, 2024

ce-ee cherrypick not needed, it's part of the ai-proxy-advanced PR and is merged

@AndyZhang0707 AndyZhang0707 removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants