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

tests(clustering): dp status ready when use RPC Sync #14035

Merged
merged 13 commits into from
Dec 23, 2024
Merged

Conversation

lhanjian
Copy link
Contributor

@lhanjian lhanjian commented Dec 18, 2024

Summary

Checklist

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

Issue reference

Fix KAG-5994

@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Dec 18, 2024
@lhanjian lhanjian force-pushed the test/status_spec_ics branch from e425e94 to 7794529 Compare December 19, 2024 02:53
@chobits chobits self-requested a review December 19, 2024 03:04
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 19, 2024
@lhanjian lhanjian changed the title fix(test): dp status ready when use Incremental Sync fix(test): dp status ready when use RPC Sync Dec 19, 2024
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 19, 2024
@lhanjian lhanjian force-pushed the test/status_spec_ics branch from 2b813f0 to 94557b4 Compare December 19, 2024 08:17
@chronolaw chronolaw changed the title fix(test): dp status ready when use RPC Sync tests(clustering): dp status ready when use RPC Sync Dec 19, 2024
@chronolaw
Copy link
Contributor

We should remove -- XXX FIXME and skip_rpc_sync also.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 19, 2024
@@ -106,64 +103,59 @@ for _, strategy in helpers.each_strategy() do
end, 10)
end)

-- now dp receive config from cp, so dp should be ready

skip_rpc_sync("should return 200 on data plane after configuring", function()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you misunderstand my words, we still need this case for rpc_sync = off but not use skip_rpc_sync, it should be a normal it()

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It should also support {"off", "off"}

Copy link
Contributor

@chobits chobits Dec 20, 2024

Choose a reason for hiding this comment

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

note that "off" "off" has been supported by original test case, see Line 107,
I think we should write a completely new on on case to test incremental sync mode, like

if off off
-- using original cases
end

if on on then
-- write new cases in this pr
describe / it ()
....
end
end

So the new case does not need to support on

Copy link
Contributor

@chobits chobits Dec 20, 2024

Choose a reason for hiding this comment

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

because the original case (off off) will restart CP, if we implement new case in this scenario, current CP/DP incremental sync is hard to make the case stable, it is easy to get flakiness.

Why?

because restarting CP will cause DP connecting CP and introduce some delay, and currently in incremental sync feature, we could not have a good way to ensure the established connection between CP and DP. I think if we want to test CP/DP restart , we could file a new kag to track.

@lhanjian lhanjian force-pushed the test/status_spec_ics branch from 4ee5355 to a028af9 Compare December 19, 2024 09:48
@@ -106,64 +103,59 @@ for _, strategy in helpers.each_strategy() do
end, 10)
end)

-- now dp receive config from cp, so dp should be ready

skip_rpc_sync("should return 200 on data plane after configuring", function()
Copy link
Contributor

Choose a reason for hiding this comment

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

It should also support {"off", "off"}

Copy link
Contributor

@chobits chobits left a comment

Choose a reason for hiding this comment

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

wait for CI to become green, and also note fix my coding style recommendation

@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 20, 2024
@lhanjian lhanjian force-pushed the test/status_spec_ics branch from 8ac99c7 to 26dc2a4 Compare December 20, 2024 09:09
@ADD-SP ADD-SP merged commit 51cb5f1 into master Dec 23, 2024
25 checks passed
@ADD-SP ADD-SP deleted the test/status_spec_ics branch December 23, 2024 06:23
@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-14035-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-14035-to-master-to-upstream
git checkout -b cherry-pick-14035-to-master-to-upstream
ancref=$(git merge-base 14b1bf96dc37326bcca06eab15ae0bdec74cf013 b03b074daf8bc6fb6b905315ce2cddaabe4c77a2)
git cherry-pick -x $ancref..b03b074daf8bc6fb6b905315ce2cddaabe4c77a2

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants