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(manager): set InitCacheSyncDuration to 5s by default and allow it… #5238

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

backjo
Copy link
Contributor

@backjo backjo commented Nov 27, 2023

… to be configured via cli

What this PR does / why we need it:
This sets the default value for InitCacheSyncDuration to 5 seconds - as it was in #2249 - to avoid 'partial' writes to the proxy configuration before the Kubernetes objects have a had a chance to sync. This default value appears to have been inadvertently changed as part of #4101 . Additionally, this PR exposes the value for configuration by end-users.

Which issue this PR fixes:

Closes #5175

Special notes for your reviewer:

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

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2023

CLA assistant check
All committers have signed the CLA.

@backjo backjo marked this pull request as ready for review November 27, 2023 16:03
@backjo backjo requested a review from a team as a code owner November 27, 2023 16:03
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Does this actually need to be configurable, or is fixing the regression sufficient? We'd left it inaccessible originally because we weren't sure if cases where the resource count was large enough where population did exceed 5s were likely in practice.

It's unfortunately not an intuitive setting to understand if you aren't familiar with the implementation internals, so it felt like it'd likely be an obscure knob where nobody knows when/if they should tweak it.

That's not a huge reason to avoid adding it, but if you have encountered a sufficiently large config set to need it and have logs or other indicators that'd show when users need to increase it, that'd be useful for docs.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/cli-arguments.md Outdated Show resolved Hide resolved
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
@backjo
Copy link
Contributor Author

backjo commented Nov 28, 2023

Does this actually need to be configurable, or is fixing the regression sufficient? We'd left it inaccessible originally because we weren't sure if cases where the resource count was large enough where population did exceed 5s were likely in practice.

It's unfortunately not an intuitive setting to understand if you aren't familiar with the implementation internals, so it felt like it'd likely be an obscure knob where nobody knows when/if they should tweak it.

That's not a huge reason to avoid adding it, but if you have encountered a sufficiently large config set to need it and have logs or other indicators that'd show when users need to increase it, that'd be useful for docs.

Yeah, I decided to add it because from the logs we put in, we felt a little too close for comfort (~4ish seconds) on the time it took for the initial configuration to be loaded - especially given that a partial configuration results in 4XX errors all over the place for us. I totally agree it's a non-intuitive setting if one is unfamiliar with the implementation details - and ideally a fix per #2315 would get rid of the need for it altogether - but given that it's the current lever used to solve the cache sync problem and the latency may vary depending on the size / performance of the cluster it's being run on, I think it makes sense to expose for operators to configure.

@czeslavo
Copy link
Contributor

Added backport release/2.12.x to have it ported to the LTS.

czeslavo
czeslavo previously approved these changes Nov 28, 2023
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Looks good to me, apart from a minor nit.

internal/manager/config.go Outdated Show resolved Hide resolved
mlavacca
mlavacca previously approved these changes Nov 28, 2023
czeslavo
czeslavo previously approved these changes Nov 28, 2023
@rainest
Copy link
Contributor

rainest commented Nov 28, 2023

Blah, forgot that you can't quite suggest doc changes there from the PR due to the need for make generate to copy those elsewhere.

@backjo backjo dismissed stale reviews from czeslavo and mlavacca via b0f92b6 November 29, 2023 07:28
@rainest rainest merged commit 1737253 into Kong:main Nov 30, 2023
19 checks passed
@team-k8s-bot
Copy link
Collaborator

The backport to release/2.12.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.12.x release/2.12.x
# Navigate to the new working tree
cd .worktrees/backport-release/2.12.x
# Create a new branch
git switch --create backport-5238-to-release/2.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17372534e2caa29742d1a9a723f9a3c6d1906069
# Push it to GitHub
git push --set-upstream origin backport-5238-to-release/2.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/2.12.x

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

rainest added a commit that referenced this pull request Nov 30, 2023
Fix regression that removed initial cache sync delay.

Add --init-cache-sync-duration flag to override the delay period.

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
rainest added a commit that referenced this pull request Dec 1, 2023
Fix regression that removed initial cache sync delay.

Add --init-cache-sync-duration flag to override the delay period.

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
@mflendrich mflendrich mentioned this pull request Dec 4, 2023
27 tasks
@piotrwielgolaski-tomtom
Copy link

when can we hope to get this in 3.x line? getting partial configuration is very dangerous

@team-k8s-bot
Copy link
Collaborator

The backport to release/3.0.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/3.0.x release/3.0.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.0.x
# Create a new branch
git switch --create backport-5238-to-release/3.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17372534e2caa29742d1a9a723f9a3c6d1906069
# Push it to GitHub
git push --set-upstream origin backport-5238-to-release/3.0.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.0.x

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

tao12345666333 pushed a commit that referenced this pull request Jan 10, 2024
Fix regression that removed initial cache sync delay.

Add --init-cache-sync-duration flag to override the delay period.

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
(cherry picked from commit 1737253)
tao12345666333 pushed a commit that referenced this pull request Jan 10, 2024
Fix regression that removed initial cache sync delay.

Add --init-cache-sync-duration flag to override the delay period.

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
(cherry picked from commit 1737253)
randmonkey added a commit that referenced this pull request Jan 10, 2024
…5414)

* fix(manager) restore InitCacheSyncDuration (#5238)

Fix regression that removed initial cache sync delay.

Add --init-cache-sync-duration flag to override the delay period.

---------

Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
(cherry picked from commit 1737253)

* chore(gha): preemptively add 1p creds

* chore: remove "password" input in kong/license step of workflows (#5316)

---------

Co-authored-by: Jonah Back <jonah@jonahback.com>
Co-authored-by: Isa Farnik <isa@konghq.com>
Co-authored-by: Tao Yi <tao.yi@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition between secret reconciler and object reference index
8 participants