Skip to content

Improve push squashing#6641

Merged
rshriram merged 4 commits intoistio:masterfrom
costinm:costin-cachepush
Jun 27, 2018
Merged

Improve push squashing#6641
rshriram merged 4 commits intoistio:masterfrom
costinm:costin-cachepush

Conversation

@costinm
Copy link
Copy Markdown
Contributor

@costinm costinm commented Jun 26, 2018

Fixes #5588 - the root problem was that events were squashed, but nothing triggered a push for the last events. So change #1 was pushed immediately, change #2 triggered a callback after (squash time).
Instead the callback is now after 1 second, and we evaluate if in the last second we received any event.
If not - push. Otherwise, keep waiting until the batch of changes settles down.

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 26, 2018

Codecov Report

Merging #6641 into master will increase coverage by 1%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #6641    +/-   ##
=======================================
+ Coverage      68%     68%    +1%     
=======================================
  Files         361     361            
  Lines       31515   31701   +186     
=======================================
+ Hits        21271   21442   +171     
- Misses       9382    9399    +17     
+ Partials      862     860     -2
Impacted Files Coverage Δ
pilot/pkg/proxy/envoy/discovery.go 4% <0%> (ø) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 59% <0%> (-21%) ⬇️
mixer/adapter/signalfx/cumulative.go 67% <0%> (-4%) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 65% <0%> (-3%) ⬇️
mixer/adapter/fluentd/fluentd.go 74% <0%> (-1%) ⬇️
mixer/adapter/statsd/statsd.go 96% <0%> (-1%) ⬇️
mixer/adapter/stackdriver/trace/trace.go 88% <0%> (-1%) ⬇️
mixer/adapter/servicecontrol/reportbuilder.go 89% <0%> (ø) ⬇️
pilot/pkg/kube/inject/inject.go 84% <0%> (ø) ⬇️
mixer/adapter/stdio/zap.go 100% <0%> (ø) ⬆️
... and 10 more

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 b53b35b...7a41a38. Read the comment docs.

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jun 26, 2018

@costinm: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/e2e-dashboard.sh 7a41a38 link /test e2e-dashboard
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

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

..


if time.Since(lastClearCache) > time.Duration(clearCacheTime)*time.Second {
log.Infof("Cleared discovery service cache after %v", time.Since(lastClearCache))
lastClearCache = time.Now()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This block seems unnecessary given that the previous if block would have triggered a push on the first event in a burst of config events

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's for cases where changes are made continuously - for example a larger system where an endpoint or config changes every 1/2 second for a longer time. We'll still do pushes every clearCacheTime. We can't do them every second because it would kill pilot (memory overhead is still high).

@rshriram rshriram merged commit 399cd2d into istio:master Jun 27, 2018
hklai added a commit that referenced this pull request Jun 27, 2018
hklai added a commit that referenced this pull request Jun 27, 2018
* Revert "Remove v2 transition commands since everything is now v2 (#6665)"

This reverts commit 6339eb6.

* Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (#6446)"

This reverts commit b9294f7.

* Revert "iptable just "return" by uid as the parameter u indicates (#6561)"

This reverts commit 22a0b88.

* Revert "Remove node agent service, residue from flexvolume driver. (#6651)"

This reverts commit db3da82.

* Revert "Continuously reapply galley CA bundle to prevent overwrite (#6599)"

This reverts commit f9e8fd8.

* Revert "Do not count typeConfigs if it is error. (#6527)"

This reverts commit eb1de31.

* Revert "Make racetest green - Fixed data races and flakiness (#6625)"

This reverts commit 30b8ecb.

* Revert "Improve push squashing (#6641)"

This reverts commit 399cd2d.
costinm added a commit to costinm/istio that referenced this pull request Jun 27, 2018
* Improve cache push

* Format
#
galley:
enabled: true
enabled: false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed this change due to the PR title. Could you please cc @ozevren and myself in the future for these types of changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we have this change?

+1 w/ @ayj. This should have been properly code reviewed before merging it in.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR was reverted in #6677.

FWIW, this change did expose a bug in the post-install jobs when galley was disabled. Working on a fix for now that now.

istio-testing pushed a commit that referenced this pull request Jun 28, 2018
* Improve cache push

* Format

* Improve push squashing (#6641)

* Improve cache push

* Format

* Revert experiment to verify if validation can be disabled
quanjielin pushed a commit to quanjielin/istio that referenced this pull request Jul 2, 2018
* Improve cache push

* Format
quanjielin pushed a commit to quanjielin/istio that referenced this pull request Jul 2, 2018
* Revert "Remove v2 transition commands since everything is now v2 (istio#6665)"

This reverts commit 6339eb6.

* Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (istio#6446)"

This reverts commit b9294f7.

* Revert "iptable just "return" by uid as the parameter u indicates (istio#6561)"

This reverts commit 22a0b88.

* Revert "Remove node agent service, residue from flexvolume driver. (istio#6651)"

This reverts commit db3da82.

* Revert "Continuously reapply galley CA bundle to prevent overwrite (istio#6599)"

This reverts commit f9e8fd8.

* Revert "Do not count typeConfigs if it is error. (istio#6527)"

This reverts commit eb1de31.

* Revert "Make racetest green - Fixed data races and flakiness (istio#6625)"

This reverts commit 30b8ecb.

* Revert "Improve push squashing (istio#6641)"

This reverts commit 399cd2d.
quanjielin pushed a commit to quanjielin/istio that referenced this pull request Jul 2, 2018
* Improve cache push

* Format

* Improve push squashing (istio#6641)

* Improve cache push

* Format

* Revert experiment to verify if validation can be disabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants