change default refresh delays and drain durations by rshriram · Pull Request #733 · istio/istio

rshriram · 2017-09-11T21:31:49Z

Change the default refresh interval, envoy drain duration. The current interval is suitable only for testing (1s). Increasing the default to 30s results in lesser CPU utilization (for Pilot) and better performance.

Release note:

Option to disable mixer policy checks while allowing metrics to be recorded.

kyessenov · 2017-09-11T21:34:04Z

install/kubernetes/templates/istio-pilot.yaml.tmpl

+
+    ## Set enableTracing to false to disable request tracing.
+    enableTracing: true
    zipkinAddress: zipkin.{ISTIO_NAMESPACE}:9411


These settings won't take effect. Please update to the new API definitions. The parser will ignore any undefined fields.

?
the SHA in pilot's WORKSPACE is at Sep 10th.

commit = "6838a2bfc42260cf0513552a64a457f1cf39041c", # Sep 10, 2017 remote = "https://github.com/istio/api.git", )```

kyessenov · 2017-09-11T21:34:14Z

install/kubernetes/templates/istio-pilot.yaml.tmpl

+    # its internal configuration from Istio Pilot. Lower refresh delay
+    # results in higher CPU utilization and potential performance loss in
+    # exchange for faster convergence. Tweak this value according to your setup.
+    discoveryRefreshDelay: 30s


andraxylia · 2017-09-11T21:36:05Z

/lgtm

istio-merge-robot · 2017-09-11T21:51:42Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @rshriram

andraxylia · 2017-09-11T22:06:39Z

/lgtm cancel

istio-merge-robot · 2017-09-11T22:06:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: andraxylia

Assign the PR to them by writing /assign @andraxylia in a comment when ready.

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

Details

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

kyessenov

Looks better (but it'd be better to double check test logs that each settings is being applied)

rshriram · 2017-09-11T22:43:21Z

It is not.
envoy-rev0.json --restart-epoch 0 --drain-time-s 2 --parent-shutdown-time-s 3 --service-cluster istio-proxy --service-node sidecar~~10.32.11.251~~productpage-v1-3512922263-qgfpn.mixer-test-

I thought we sourced the yamls in the tests

andraxylia · 2017-09-11T23:02:36Z

This configMap has changed in pilot to use a different structure "default".

rshriram · 2017-09-11T23:03:46Z

Yes and I updated the config map here accordingly.

On Mon, Sep 11, 2017 at 7:02 PM Andra Cismaru ***@***.***> wrote: This configMap has changed in pilot to use a different structure "default". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd81L8x5R9X-0AfIgGSFwjZaIWpTjks5shbwPgaJpZM4PTzIU> .

-- ~shriram

andraxylia · 2017-09-11T23:09:44Z

You need to upgrade the pilot SHA to latest master in the same commit, otherwise istioctl kube-inject will fail the validate.

andraxylia

You need to run updateVersion.sh for any change in template. Sorry for not noticing earlier.

andraxylia · 2017-09-12T01:28:32Z

Actually, I will approve since I do not want to block the PR, so you can merge it, but please run updateVersions on it.

rshriram · 2017-09-12T02:10:52Z

Pilot sha is now pointing to master. Ran updateVersion.sh as @andraxylia suggested. Lets see if this passes.

ldemailly · 2017-09-12T20:56:23Z

/retest

andraxylia · 2017-09-13T01:32:23Z

install/updateVersion.sh

 TEMP_DIR="/tmp"
 GIT_COMMIT=false

+## Install constants for Pilot. Do not change


The changes in this file are not needed, but we can remove them later if the tests pass.
It is enough to have the test infra replace the default values.

Yep, I realized that.. removed it

rshriram · 2017-09-13T01:58:52Z

defaults are not being applied as per the regex in go files.. @yutongz any inputs?

yutongz · 2017-09-13T02:21:08Z

Which one is not applied? @rshriram

rshriram · 2017-09-13T02:30:01Z

these: https://github.com/istio/istio/pull/733/files#diff-2a1d43c23839deded26ef25da29c0506R249

yutongz · 2017-09-13T03:01:30Z

Right, so there are two things you need to do:

Your change is in generateRbac() whatever inside, only replace stuff in rbac-beta.yaml, you may want to put all your "replace work" in https://github.com/istio/istio/blob/master/tests/e2e/framework/kubernetes.go#L284
You need to update istio.yaml with the change in pilot. e2e framework directly use istio.yaml or istio-auth.yaml

ldemailly · 2017-09-13T05:41:05Z

will close, you can redo from fresh/green without SHA changes (or resolve conflicts on this PR if you want but carefully) - reopen if you want to reuse

istio-testing · 2017-09-13T14:23:38Z

@rshriram: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/new-e2e-rbac_no_auth.sh	`35534cd`	link	`/test new-e2e-rbac_no_auth`
prow/e2e-suite-rbac-no_auth.sh	`35534cd`	link	`/test e2e-suite-rbac-no_auth`
prow/e2e-suite-rbac-auth.sh	`35534cd`	link	`/test e2e-suite-rbac-auth`
prow/istio-presubmit.sh	`bb7ffb1`	link	`/test istio-presubmit`

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.

Former-commit-id: 4a2140d8bbade09a6bd85a4112d83f520a41844f

Former-commit-id: 5ad7a1bc16d534f0383445f6ce6603db5726bf90

…istio#733) This reverts commit ef74122.

* Sync installer I was going to wait for the repo merge, but I wanted to fix any failing tests that may pop up to make that merge smoother * fix e2e

change default refresh delays and drain durations

1886eba

rshriram added this to the Istio 0.2 milestone Sep 11, 2017

googlebot added the cla: yes label Sep 11, 2017

rshriram requested a review from andraxylia September 11, 2017 21:31

istio-merge-robot added the release-note label Sep 11, 2017

kyessenov requested changes Sep 11, 2017

View reviewed changes

andraxylia approved these changes Sep 11, 2017

View reviewed changes

istio-testing assigned andraxylia Sep 11, 2017

istio-testing added the lgtm label Sep 11, 2017

istio-merge-robot added the approved label Sep 11, 2017

updating config

503517d

istio-merge-robot removed the lgtm label Sep 11, 2017

Shriram Rajagopalan added 2 commits September 11, 2017 17:52

Merge branch 'master' into change_defaults

c87dcc5

fixes

a5f70a8

andraxylia added the do-not-merge Block automatic merging of a PR. label Sep 11, 2017

istio-merge-robot removed the approved label Sep 11, 2017

kyessenov approved these changes Sep 11, 2017

View reviewed changes

andraxylia suggested changes Sep 12, 2017

View reviewed changes

andraxylia approved these changes Sep 12, 2017

View reviewed changes

updateVersion.sh

8aab7b8

Merge branch 'master' into change_defaults

68a2635

ldemailly mentioned this pull request Sep 12, 2017

pilot e2e smoketest didn't detect istio/istio breakage with newer pilot istio/old_pilot_repo#1255

Closed

sebastienvas mentioned this pull request Sep 12, 2017

pilot e2e smoketest didn't detect istio/istio breakage with newer pilot #757

Closed

Shriram Rajagopalan added 5 commits September 12, 2017 20:53

Merge branch 'master' into change_defaults

3a355bf

templatize delays

fc8353a

undo

9d7b18e

replace via templates

9b83c88

undo templating

35534cd

andraxylia reviewed Sep 13, 2017

View reviewed changes

andraxylia approved these changes Sep 13, 2017

View reviewed changes

ldemailly closed this Sep 13, 2017

Shriram Rajagopalan added 3 commits September 13, 2017 10:17

Merge branch 'master' into change_defaults

64b83d9

fix

946dc99

update yamls

bb7ffb1

rshriram reopened this Sep 13, 2017

rshriram closed this Sep 13, 2017

rshriram mentioned this pull request Sep 13, 2017

Change default intervals for pilot #779

Closed

ldemailly deleted the change_defaults branch September 15, 2017 21:29

mandarjog pushed a commit to mandarjog/istio that referenced this pull request Oct 30, 2017

Another license fix. (istio#733)

6d6ffaa

Former-commit-id: 4a2140d8bbade09a6bd85a4112d83f520a41844f

mandarjog pushed a commit that referenced this pull request Oct 31, 2017

Another license fix. (#733)

90cfb2c

Former-commit-id: 5ad7a1bc16d534f0383445f6ce6603db5726bf90

kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018

Revert "Revert "Not to send legacy quota for v2 config." (istio#725)" (…

a7e4ff4

…istio#733) This reverts commit ef74122.

howardjohn added a commit to howardjohn/istio that referenced this pull request Jan 12, 2020

Sync installer (istio#733)

2cf0bae

* Sync installer I was going to wait for the repo merge, but I wanted to fix any failing tests that may pop up to make that merge smoother * fix e2e

Conversation

rshriram commented Sep 11, 2017

Uh oh!

kyessenov Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

rshriram Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

kyessenov Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 11, 2017

Uh oh!

istio-merge-robot commented Sep 11, 2017

Uh oh!

andraxylia commented Sep 11, 2017

Uh oh!

istio-merge-robot commented Sep 11, 2017

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

rshriram commented Sep 11, 2017

Uh oh!

andraxylia commented Sep 11, 2017

Uh oh!

rshriram commented Sep 11, 2017 via email

Uh oh!

andraxylia commented Sep 11, 2017

Uh oh!

andraxylia left a comment

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 12, 2017

Uh oh!

rshriram commented Sep 12, 2017

Uh oh!

ldemailly commented Sep 12, 2017

Uh oh!

andraxylia Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

rshriram commented Sep 13, 2017

Uh oh!

yutongz commented Sep 13, 2017

Uh oh!

rshriram commented Sep 13, 2017

Uh oh!

yutongz commented Sep 13, 2017

Uh oh!

ldemailly commented Sep 13, 2017

Uh oh!

istio-testing commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andraxylia Sep 13, 2017 •

edited

Loading