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(konnect): update nodes status only when it actually changes #4324

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Jul 12, 2023

What this PR does / why we need it:

Makes sure that:

  • NodeAgent calls Konnect APIs only when the config status actually changes (enforce that on both ends of the notification channel - in NodeAgent and in KongClient). This was an actual reason for KIC updating nodes every 3s.
  • NodeAgent calls Konnect APIs only when a set of Gateway clients actually changes (enforce that on ClientsManager side)

Also improves debug logging to make it visible when the calls are made.

After this change, NodeAgent will call Konnect APIs in the following cases:

  • configuration status has changed,
  • gateways' clients have changed (Gateway deployment scaling, Pod eviction, etc.),
  • every 30s.

Which issue this PR fixes:

Fixes #4322.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@czeslavo czeslavo added area/konnect Issues and PRs related to Konnect fix backport release/2.10.x labels Jul 12, 2023
@czeslavo czeslavo added this to the KIC v2.10.0 milestone Jul 12, 2023
@czeslavo czeslavo self-assigned this Jul 12, 2023
@czeslavo czeslavo force-pushed the fix/nodes-update-only-on-change branch 2 times, most recently from a8b7117 to fa2cddd Compare July 12, 2023 13:26
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 94.8% and project coverage change: +0.1 🎉

Comparison is base (c8843de) 64.9% compared to head (0be0a59) 65.1%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4324     +/-   ##
=======================================
+ Coverage   64.9%   65.1%   +0.1%     
=======================================
  Files        154     154             
  Lines      17384   17416     +32     
=======================================
+ Hits       11297   11339     +42     
+ Misses      5367    5356     -11     
- Partials     720     721      +1     
Impacted Files Coverage Δ
internal/konnect/node_agent.go 61.0% <91.4%> (+3.5%) ⬆️
internal/clients/manager.go 94.7% <100.0%> (+0.1%) ⬆️
internal/dataplane/kong_client.go 85.1% <100.0%> (+0.3%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@czeslavo czeslavo force-pushed the fix/nodes-update-only-on-change branch from fa2cddd to 3ddedad Compare July 12, 2023 14:10
@czeslavo czeslavo marked this pull request as ready for review July 12, 2023 14:14
@czeslavo czeslavo requested a review from a team as a code owner July 12, 2023 14:14
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

👍 Some minor comments to consider

internal/konnect/node_agent_test.go Show resolved Hide resolved
internal/konnect/node_agent_test.go Outdated Show resolved Hide resolved
internal/clients/manager_test.go Outdated Show resolved Hide resolved
internal/clients/manager_test.go Outdated Show resolved Hide resolved
@czeslavo czeslavo force-pushed the fix/nodes-update-only-on-change branch from d99b42e to 0be0a59 Compare July 12, 2023 14:45
@pmalek
Copy link
Member

pmalek commented Jul 12, 2023

I believe the milestone should be set to v2.11, right? The backport will have it set to v2.10.

@czeslavo czeslavo modified the milestones: KIC v2.10.0, KIC v2.11.0 Jul 12, 2023
@czeslavo czeslavo enabled auto-merge (squash) July 12, 2023 14:59
@czeslavo czeslavo merged commit f861118 into main Jul 12, 2023
29 checks passed
@czeslavo czeslavo deleted the fix/nodes-update-only-on-change branch July 12, 2023 15:58
@github-actions
Copy link

The backport to release/2.10.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/2.10.x release/2.10.x
# Navigate to the new working tree
cd .worktrees/backport-release/2.10.x
# Create a new branch
git switch --create backport-4324-to-release/2.10.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f861118a88b9e67aa37cd42edda0162d184026e4
# Push it to GitHub
git push --set-upstream origin backport-4324-to-release/2.10.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/2.10.x

Then, create a pull request where the base branch is release/2.10.x and the compare/head branch is backport-4324-to-release/2.10.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KIC node agent reports updates nodes every 3s
2 participants