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

fix: If resource synchronization retry occurs, other events of the same resource will be blocked. #760

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

gxthrj
Copy link
Contributor

@gxthrj gxthrj commented Nov 19, 2021

Please answer these questions before submitting a pull request


Bugfix

  • How to fix?

This bug is due to a workqueue shared under the same resource, and a ratelimit mechanism is added to this workqueue, but we only need to add the ratelimit when retrying fails, and when normal resource changes, we should immediately add the workqueue to be processed .

The new k8s event does not need to use the rate limit.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2021

Codecov Report

Merging #760 (2751876) into master (4a862e2) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #760   +/-   ##
=======================================
  Coverage   31.70%   31.70%           
=======================================
  Files          66       66           
  Lines        6640     6640           
=======================================
  Hits         2105     2105           
  Misses       4280     4280           
  Partials      255      255           
Impacted Files Coverage Δ
pkg/ingress/apisix_cluster_config.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_consumer.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_route.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_tls.go 0.00% <0.00%> (ø)
pkg/ingress/apisix_upstream.go 0.00% <0.00%> (ø)
pkg/ingress/endpoint.go 0.00% <0.00%> (ø)
pkg/ingress/endpointslice.go 0.00% <0.00%> (ø)
pkg/ingress/ingress.go 8.29% <0.00%> (ø)
pkg/ingress/namespace.go 0.00% <0.00%> (ø)
pkg/ingress/secret.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 774077a...2751876. Read the comment docs.

@tao12345666333 tao12345666333 added the bug Something isn't working label Nov 22, 2021
@tao12345666333 tao12345666333 added this to the 1.4.0 milestone Nov 22, 2021
@tokers
Copy link
Contributor

tokers commented Nov 22, 2021

@gxthrj Could you give some details about the fixup?

@gxthrj
Copy link
Contributor Author

gxthrj commented Nov 26, 2021

@gxthrj Could you give some details about the fixup?

This bug is due to a workqueue shared under the same resource, and a ratelimit mechanism is added to this workqueue, but we only need to add the ratelimit when retrying fails, and when normal resource changes, we should immediately add the workqueue to be processed .

The new k8s event does not need to use the rate limit.

@tao12345666333 tao12345666333 merged commit 62d7897 into apache:master Nov 26, 2021
@tao12345666333 tao12345666333 added this to In progress in v1.4 Planning via automation Nov 26, 2021
@tao12345666333 tao12345666333 moved this from In progress to Done in v1.4 Planning Nov 26, 2021
Sindweller pushed a commit to Sindweller/apisix-ingress-controller that referenced this pull request Dec 18, 2021
@tangzhenhuang
Copy link

This fix cannot solve the problem in some scenarios, for example:
Suppose there are two instances of apisix-ingress-controller. When the leader goes down for some reason and another instance becomes the leader, the new leader will block the resource event due to "client-side throttling" during the list resource phase. Because apisix-ingress-controller has not completed the list stage at this time, the control loop will definitely be blocked
image

@tao12345666333
Copy link
Member

@crazyMonkey1995 Thanks for your report. We can use #822 for tracking this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants