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.

Sign up for GitHub

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

Jump to bottom

fix: update_count is reset once updated, cause cache key conflict #9811

Merged
merged 2 commits into from Jul 14, 2023
Merged

fix: update_count is reset once updated, cause cache key conflict #9811

merged 2 commits into from Jul 14, 2023

Conversation

kingluo
Copy link
Contributor

@kingluo kingluo commented Jul 11, 2023

Description

update_count is reset once updated, which will cause lru cache key conflict, in turn reusing the old route conf.

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 first)

@monkeyDluffy6017
Copy link
Contributor

No test cases?

moonming
moonming reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

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

  1. The title of PR needs to be able to see what problem is fixed
  2. Did not understand the relevant modification of the test case

@kingluo kingluo changed the title fix: route_conf.modifiedIndex fix: update_count is reset once updated, cause cache key conflict Jul 11, 2023
@kingluo
Copy link
Contributor Author

kingluo commented Jul 11, 2023 •
edited

  1. The title of PR needs to be able to see what problem is fixed
  2. Did not understand the relevant modification of the test case

The update_count is volatile, instead, we need to use monotonically increasing etcd modified revision.
So that's why the test cases need to change the conf_version format to adapt this bugfix.

As a matter of fact, this bug cannot be triggered in the master branch, so that's why there is no dedicated test case.
After #9456, there is no watch for each admin resource, which is the precondition of this bug (no compacted event from etcd for individual resource anymore). That is, although the fix is correct (etcd revision is always better than a counter in memory), we will not have a problem in the master branch. @leslie-tsang

@moonming
Copy link
Member

// As a matter of fact, this bug cannot be triggered in the master branch, so that's why there is no dedicated test case.

So we don't need this PR?

@kingluo
Copy link
Contributor Author

kingluo commented Jul 11, 2023

// As a matter of fact, this bug cannot be triggered in the master branch, so that's why there is no dedicated test case.

So we don't need this PR?

Yes, and no.

I think it's necessary, As I said before, the original code should not use a volatile counter instead of the etcd revision, which is definitely a bad practice.
But on the other hand, regarding the lack of test cases, yes, this bug is also useless for the master branch.

@moonming
Copy link
Member

The master branch does not need this fix, and is not sufficiently tested.

@moonming moonming closed this Jul 12, 2023
@moonming moonming reopened this Jul 12, 2023
@monkeyDluffy6017
Copy link
Contributor

@moonming Any more comments?

@soulbird
Copy link
Contributor

fixed #9822

@monkeyDluffy6017 monkeyDluffy6017 merged commit 9758a3e into apache:master Jul 14, 2023
80 checks passed
@lingsamuel lingsamuel mentioned this pull request Jul 14, 2023
Closed
AlinsRan pushed a commit to AlinsRan/apisix that referenced this pull request Jul 18, 2023
@kingluo @AlinsRan
fix: update_count is reset once updated, cause cache key conflict (ap…
07ff6dd 
…ache#9811)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monkeyDluffy6017 monkeyDluffy6017 monkeyDluffy6017 approved these changes

@moonming moonming moonming approved these changes

@leslie-tsang leslie-tsang leslie-tsang approved these changes

Assignees
No one assigned
Labels
approved need backport
Projects
Apache APISIX backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kingluo @monkeyDluffy6017 @moonming @soulbird @leslie-tsang