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: allow configuring timeout and retries for upstream with ingress #1876

Merged
merged 35 commits into from
Dec 12, 2023

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Jun 21, 2023

Type of change:

Fixes #1783

Documentation to be updated.

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

… annotations

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup Revolyssup changed the title feat: allow configuring timeout and retries for upstream with ingress… feat: allow configuring timeout and retries for upstream with ingress Jun 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (7a9a0fa) 37.12% compared to head (aec4d5a) 37.13%.
Report is 1 commits behind head on master.

❗ Current head aec4d5a differs from pull request most recent head af956c7. Consider uploading reports for the commit af956c7 to get more accurate results

Files Patch % Lines
pkg/providers/ingress/translation/translator.go 0.00% 32 Missing ⚠️
...gress/translation/annotations/upstream/upstream.go 83.78% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1876   +/-   ##
=======================================
  Coverage   37.12%   37.13%           
=======================================
  Files          93       93           
  Lines        7886     7938   +52     
=======================================
+ Hits         2928     2948   +20     
- Misses       4568     4598   +30     
- Partials      390      392    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lingsamuel
Copy link
Member

make update-gofmt (or make update-all) is required

@tao12345666333 tao12345666333 added this to the 1.8.0 milestone Jun 26, 2023
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@tao12345666333 tao12345666333 added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jun 29, 2023
@lingsamuel
Copy link
Member

maybe e2e-test is needed @tao12345666333 wdyt

@tao12345666333
Copy link
Member

yes, we need an e2e test case to cover this one.

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup
Copy link
Contributor Author

Revolyssup commented Aug 4, 2023

@tao12345666333 How do you suggest I simulate backend for testing retries and timeouts? For retries, I configured an http server on port 8080 that responds with 200 on 3rd attempt(400 previously) and the I set the retry to 3, but tests seem to fail. Do you have a better approach? Or is there some issue with my approach?

@tao12345666333
Copy link
Member

Using the serverless plugin of APISIX, it will be easier to simulate with Lua.

@Revolyssup Revolyssup self-assigned this Nov 17, 2023
@AlinsRan
Copy link
Contributor

AlinsRan commented Dec 8, 2023

Please provide documentation for this feature.
https://apisix.apache.org/docs/ingress-controller/concepts/annotations

metadata:
annotations:
k8s.apisix.apache.org/retry: "1"
k8s.apisix.apache.org/timeout.read: "20s"
Copy link
Contributor

Choose a reason for hiding this comment

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

What timeout? The timeout here has many layers of meaning, should it be a client or a service?
You should clarify the target or object of the timeout.

Retrying is also the same.

Copy link
Contributor Author

@Revolyssup Revolyssup Dec 8, 2023

Choose a reason for hiding this comment

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

How about, upstream.retries and upstream.timeout.read? @AlinsRan

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using - as the separator for annotations name, which is the format we are currently using. Please maintain consistency.

upstream-retries
upstream-read-timeout.

Copy link
Member

@kayx23 kayx23 left a comment

Choose a reason for hiding this comment

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

some suggestions for docs

docs/en/latest/concepts/annotations.md Outdated Show resolved Hide resolved
docs/en/latest/concepts/annotations.md Outdated Show resolved Hide resolved
docs/en/latest/concepts/annotations.md Show resolved Hide resolved
docs/en/latest/concepts/annotations.md Outdated Show resolved Hide resolved
@Revolyssup Revolyssup merged commit 8fbf558 into apache:master Dec 12, 2023
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

request help: native Kubernetes ingress yaml add timeout config
8 participants