Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

osm: disable ticker, space k8s resyncs to 5m #2095

Merged
merged 3 commits into from
Nov 20, 2020

Conversation

eduser25
Copy link
Contributor

@eduser25 eduser25 commented Nov 19, 2020

  • OSM does not hard-require periodic re-configs to guarantee state
    correctness in its current form. In order to help maintain this statu quo,
    we will disable ticker and increase the k8s resync period, which will
    help catch issues that break on-event updates at testing/CI time.

  • Since we still believe periodic arbitrary resyncs could be something
    that we still might want to have at later time/different circumstances, to
    ensure config correctness, we will add configurability into ticker and
    resync intervals in a later patch (Add runtime config/osm boot flags to configure ticker/resyncs #2103).

  • Control Plane [X]

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@eduser25 eduser25 added the wip Work-in-Progress label Nov 19, 2020
@eduser25 eduser25 requested a review from a team November 19, 2020 20:42
Not Final. TBD if/how do we want this.

- OSM does not require this at this time to properly function.
- Even when they don't do anything, DeepEqual over all catalog objects
does take a rather quantifiable amount of cpu cycles.

Signed-off-by: Eduard Serra <eduser25@gmail.com>
- Ticker disabled
- Kubernetes resyncs scheduled for 5mins
This is to make sure that resyncs are never the
go-to behavior at testing time.

Opened issue to make these programable
openservicemesh#2103

Signed-off-by: Eduard Serra <eduser25@gmail.com>
Signed-off-by: Eduard Serra <eduser25@gmail.com>
@codecov-io
Copy link

codecov-io commented Nov 20, 2020

Codecov Report

Merging #2095 (a4ba021) into main (e2ed301) will decrease coverage by 0.18%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2095      +/-   ##
==========================================
- Coverage   58.56%   58.38%   -0.19%     
==========================================
  Files         144      144              
  Lines        5906     5911       +5     
==========================================
- Hits         3459     3451       -8     
- Misses       2444     2457      +13     
  Partials        3        3              
Impacted Files Coverage Δ
pkg/kubernetes/types.go 0.00% <ø> (ø)
pkg/catalog/catalog.go 75.00% <11.11%> (-25.00%) ⬇️
pkg/kubernetes/event_handlers.go 69.38% <0.00%> (-16.33%) ⬇️
pkg/kubernetes/client.go 89.81% <0.00%> (-0.93%) ⬇️
pkg/envoy/lds/listener.go 26.53% <0.00%> (-0.56%) ⬇️
pkg/envoy/lds/inmesh.go 85.18% <0.00%> (ø)
pkg/envoy/xdsutil.go 66.18% <0.00%> (+1.85%) ⬆️
...certificate/providers/vault/certificate_manager.go 56.09% <0.00%> (+6.09%) ⬆️

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 e2ed301...a4ba021. Read the comment docs.

@eduser25 eduser25 removed the wip Work-in-Progress label Nov 20, 2020
@eduser25 eduser25 marked this pull request as ready for review November 20, 2020 21:08
@eduser25 eduser25 changed the title (tentative) osm: disable config replay/resync mechs by default osm: disable ticker, space k8s resyncs to 5m Nov 20, 2020
@eduser25 eduser25 requested a review from a team November 20, 2020 21:11
@@ -17,7 +17,8 @@ import (

const (
// this is catalog's tick rate for ticker, which triggers global proxy updates
updateAtLeastEvery = 30 * time.Second
// 0 disables the ticker
updateAtLeastEvery = 0 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

🤕

Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

This works.

I would have said - pull the ticker out; leave a crumb in the commit message so we can revert if we ever need this back;

@eduser25
Copy link
Contributor Author

I plan to introduce some configurability on it next week. Not going to be dead code, no worries.

@eduser25 eduser25 merged commit 120b1cf into openservicemesh:main Nov 20, 2020
@eduser25 eduser25 deleted the disable-resyncs branch November 20, 2020 21:25
draychev pushed a commit to draychev/osm that referenced this pull request Dec 14, 2020
osm: disable config replay/resync mechs by default

OSM does not hard-require periodic re-configs to guarantee state
correctness in its current form. In order to help maintain this statu quo,
we will disable ticker and increase the k8s resync period, which will
help catch issues that break on-event updates at testing/CI time.

Since we still believe periodic arbitrary resyncs could be something
that we still might want to have at later time/different circumstances, to
ensure config correctness, we will add configurability into ticker and
resync intervals in a later patch (openservicemesh#2103).

Signed-off-by: Eduard Serra <eduser25@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants