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(clustering): report node version in sync #13844

Merged
merged 16 commits into from
Nov 27, 2024
Merged

fix(clustering): report node version in sync #13844

merged 16 commits into from
Nov 27, 2024
+50 −31

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Nov 7, 2024 •
edited
Loading

Summary

May change after KAG-5369 and #13887
KAG-5772

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 #[issue number]

@github-actions github-actions bot added core/clustering cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Nov 7, 2024
@chronolaw chronolaw changed the title feat(clustering): report node version in sync fix(clustering): report node version in sync Nov 7, 2024
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Nov 7, 2024
@chronolaw chronolaw force-pushed the feat/add_kong_version_for_sync branch 2 times, most recently from 9546acb to af1f6b8 Compare November 7, 2024 22:38
@chronolaw chronolaw marked this pull request as ready for review November 14, 2024 02:02
dndx
dndx requested changes Nov 14, 2024
View reviewed changes
Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

This does not seems to be the right approach, it should ideally be done as part of "RPC handshake method change" work, as node version will not change between calls.

@chronolaw chronolaw marked this pull request as draft November 14, 2024 12:09
@chronolaw chronolaw force-pushed the feat/add_kong_version_for_sync branch from d11e6f5 to 0a9f335 Compare November 16, 2024 00:56
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 16, 2024
@chronolaw chronolaw marked this pull request as ready for review November 16, 2024 01:04
@chronolaw
Copy link
Contributor Author

@dndx I have changed the implementation, could you take a look again? thanks.

@StarlightIbuki
Copy link
Contributor

This does not seems to be the right approach, it should ideally be done as part of "RPC handshake method change" work, as node version will not change between calls.

+1. Please consider redesign the solution

StarlightIbuki
StarlightIbuki requested changes Nov 18, 2024
View reviewed changes
kong/clustering/rpc/manager.lua Outdated Show resolved Hide resolved
@chronolaw chronolaw marked this pull request as draft November 18, 2024 09:05
@StarlightIbuki
Copy link
Contributor

This seems to be going to be covered by a new change. Are we going to close this?

@chronolaw chronolaw added this to the 3.9.0 milestone Nov 20, 2024
@chronolaw chronolaw force-pushed the feat/add_kong_version_for_sync branch from 8630f2c to edc253a Compare November 20, 2024 22:39
@chronolaw chronolaw marked this pull request as ready for review November 20, 2024 22:53
@chronolaw
Copy link
Contributor Author

This seems to be going to be covered by a new change. Are we going to close this?

This PR is re-implemented to work with meta rpc call.

@chronolaw
Copy link
Contributor Author

@dndx , could you take a look and approve it? thanks.

chobits
chobits approved these changes Nov 25, 2024
View reviewed changes
kong/clustering/rpc/manager.lua Show resolved Hide resolved
dndx
dndx requested changes Nov 25, 2024
View reviewed changes
kong/clustering/rpc/manager.lua
Comment on lines +206 to +214
self.client_info[node_id] = {
ip = ngx_var.remote_addr,
version = info.kong_version,
}
Copy link
Member

Choose a reason for hiding this comment

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

Should _handle_meta_call also assert(self.concentrator)? In case self.client_info is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@chronolaw chronolaw force-pushed the feat/add_kong_version_for_sync branch from a0ca6be to 6f3890f Compare November 27, 2024 08:40
@dndx dndx merged commit c98f385 into master Nov 27, 2024
26 checks passed
@dndx dndx deleted the feat/add_kong_version_for_sync branch November 27, 2024 09:30
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StarlightIbuki StarlightIbuki StarlightIbuki approved these changes

@dndx dndx dndx approved these changes

@chobits chobits chobits approved these changes

Assignees

@chronolaw chronolaw

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/clustering size/M skip-changelog
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chronolaw @StarlightIbuki @team-gateway-bot @chobits @dndx