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

Sendconfig update improvements #1397

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Sendconfig update improvements #1397

merged 8 commits into from
Jun 4, 2021

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Jun 4, 2021

In CI there have been several problems that appear to be due to the high rate at which KIC 2.0 has been sending updates to the Kong Admin API at a regular interval. This PR includes some improvements that seemed to function better with minimal/no end-user impact, while making the defaults less noisy in testing/CI. This also adds an update to sendconfig.SendUpdate() which will stifle repetitive nil update warnings (you'll get one once for each update that would make no change now, rather than them repeating every few seconds).

This helps to resolve #1390

shaneutt added 4 commits June 4, 2021 11:19
in my testing, DBLESS and Postgres mode have both shown problems with
repetitive sub-second updates. This is something we're tracking as a
potential bug elsewhere, but for now increasing the interval seems to
make the most sense for stability, as testing has shown it reduces/removes
the timing problems which we've encountered with subsecond interval updates.
Recent updates have made our controller faster and having such a long
Ingress timeout doesn't make sense any longer: it just increasing the
time it takes to reach a timeout condition on certain kinds of failures.
@shaneutt shaneutt added bug Something isn't working priority/high size/small labels Jun 4, 2021
@shaneutt shaneutt self-assigned this Jun 4, 2021
@shaneutt shaneutt requested a review from a team as a code owner June 4, 2021 15:30
@shaneutt shaneutt linked an issue Jun 4, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #1397 (033906a) into next (cbb22ba) will decrease coverage by 0.07%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1397      +/-   ##
==========================================
- Coverage   55.20%   55.12%   -0.08%     
==========================================
  Files          42       42              
  Lines        3639     3646       +7     
==========================================
+ Hits         2009     2010       +1     
- Misses       1479     1485       +6     
  Partials      151      151              
Impacted Files Coverage Δ
pkg/sendconfig/common_workflows.go 0.00% <ø> (ø)
pkg/sendconfig/sendconfig.go 26.08% <75.00%> (+6.08%) ⬆️
...n/internal/proxy/clientgo_cached_proxy_resolver.go 57.84% <0.00%> (-5.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbb22ba...033906a. Read the comment docs.

@shaneutt shaneutt force-pushed the calmer-sendconfig branch from c60a87e to 81e76be Compare June 4, 2021 16:34
@shaneutt shaneutt requested a review from ccfishk June 4, 2021 16:35
@shaneutt shaneutt requested a review from ccfishk June 4, 2021 16:45
@shaneutt
Copy link
Contributor Author

shaneutt commented Jun 4, 2021

The follow up issue for this PR (wherein we will further investigate after this mitigation) is:

#1398

Copy link
Contributor

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

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

LGTM.

@shaneutt shaneutt merged commit 2b7bfcb into next Jun 4, 2021
@shaneutt shaneutt deleted the calmer-sendconfig branch June 4, 2021 18:53
@shaneutt shaneutt mentioned this pull request Jul 7, 2021
1 task
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.

bug: test flake with tcpingress proxy with dbless
2 participants