helm: mixer cluster tuning #5568

kyessenov · 2018-05-11T21:06:58Z

Copied 10k max requests setting (increase from 1024) for mixer clusters.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-05-11T21:25:51Z

install/kubernetes/helm/istio/charts/pilot/templates/config.yaml

+  name: istio-policy
+  namespace: {{ .Release.Namespace }}
+spec:
+  name: istio-policy.{{ .Release.Namespace }}.svc.cluster.local


@rshriram is this how it is supposed to work? how do we verify this takes effect without stressing the system?

Set the maxRequests and maxReq/conn values to absurdly low numbers so that even moderate stress will cause the same issue.

Since this is simply restoring the config that has always existed, I don't think we need to do the above test.

Ok, once @rshriram or @costinm confirm the semantics of this API, we should be OK to merge.

As mandar says. Also you can dump proxy config (/debug/adsz) to see it it shows up.

You should also set mTLS here if using control plane auth.

Ping. Does CDS respect the override relative to data plane auth flag?

yes it does

kyessenov · 2018-05-12T02:07:25Z

Per Costin there is no control plane auth flag anymore. It's equivalent with the regular data plane auth flag.

…

On Fri, May 11, 2018, 6:58 PM Shriram Rajagopalan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In install/kubernetes/helm/istio/charts/pilot/templates/config.yaml <#5568 (comment)>: > @@ -0,0 +1,28 @@ +{{ define "config.yaml.tpl" }} +apiVersion: networking.istio.io/v1alpha3 +kind: DestinationRule +metadata: + name: istio-policy + namespace: {{ .Release.Namespace }} +spec: + name: istio-policy.{{ .Release.Namespace }}.svc.cluster.local You should also set mTLS here if using control plane auth. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJGIxn5-xqJrm4zg_Ak6Lv2ByKq5Ymqpks5txkHYgaJpZM4T8EG7> .

costinm · 2018-05-14T20:12:11Z

install/kubernetes/helm/istio/charts/pilot/templates/config.yaml

@@ -0,0 +1,28 @@
+{{ define "config.yaml.tpl" }}


This should go in the mixer chart ( it's not specific to pilot )

No, mixer does not define these CRDs so it is inappropriate to submit pilot CRs in mixer helm chart. How would isolated mixer install work?

Use 'pilot.enabled' to exclude - if mixer can run without pilot.

This is not about enablement, but the fact that CRDs are defined in pilot.

@costinm if pilot.enabled=false the CRD job would not be rendered.

If pilot.enabled is false istio won't work.

We can move the CRDs to the top - but that's not really a priority, in 0.8 we don't support istio without pilot.

costinm · 2018-05-14T20:12:58Z

install/kubernetes/helm/istio/charts/pilot/templates/configmap.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1


What is this ? Why in pilot ?

This is the standard way to install CRs. Helm applies YAMLs in an arbitrary order, so you have to do round-about way to force ordering with hooks.

Did you test with helm template as well ( i.e. no helm ) ?

No, I don't have a way to run locally. CI/CD passed the tests.

costinm

Move to mixer - I think it already has some script to create resources. Pilot template is not intended as holder for all component 's configs

kyessenov · 2018-05-14T21:17:35Z

I repeat that we cannot move pilot config to mixer. If mixer is installed prior to pilot by helm, then it will fail.

costinm · 2018-05-14T21:19:40Z

Order is not guaranteed - moving all crds in pilot template doesn't guarantee they'll be run before pilot is ready.

costinm · 2018-05-14T21:20:48Z

And those are not 'pilot configs' - but istio configs. Pilot may not run at all, or some other implementation may be used - but configs should still be installed

kyessenov · 2018-05-14T21:22:41Z

I'm not sure I understand what you're saying. Please take over this PR and submit at your will.

costinm · 2018-05-14T22:29:03Z

Just move the configs to mixer.

sdake

@kyessenov when we originally did this work, we put all of the pilot CRDs in mixer. I'm not really sure why they ended up there rather than pilot (which is what creates the CRDs which is what I think your arguing related to configmaps). The technique of just adding the configmaps to mixer should be fine for now, as the hacky workaround we have for this problem until Helm solves it properly.

sdake · 2018-05-15T01:23:07Z

install/kubernetes/helm/istio/charts/pilot/templates/config.yaml

@@ -0,0 +1,28 @@
+{{ define "config.yaml.tpl" }}


@costinm if pilot.enabled=false the CRD job would not be rendered.

sdake · 2018-05-15T02:15:03Z

Also the filename is install/kubernetes/helm/istio/charts/pilot/templates/create-custom-resources-job.yaml but its more like create-configmaps-job.yaml.

ignore.

codecov · 2018-05-15T17:40:19Z

Codecov Report

Merging #5568 into release-0.8 will increase coverage by 1%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-0.8   #5568    +/-   ##
============================================
+ Coverage           73%     74%    +1%     
============================================
  Files              322     324     +2     
  Lines            27609   27479   -130     
============================================
- Hits             20140   20074    -66     
+ Misses            6681    6609    -72     
- Partials           788     796     +8

Impacted Files	Coverage Δ
mixer/adapter/rbac/controller.go	`39% <0%> (-15%)`	⬇️
pilot/pkg/proxy/envoy/v2/cds.go	`71% <0%> (-14%)`	⬇️
pilot/pkg/proxy/envoy/v2/discovery.go	`68% <0%> (-8%)`	⬇️
pilot/pkg/serviceregistry/kube/queue.go	`86% <0%> (-3%)`	⬇️
pilot/pkg/proxy/envoy/v2/ads.go	`73% <0%> (-3%)`	⬇️
mixer/adapter/redisquota/redisquota.go	`88% <0%> (-2%)`	⬇️
pilot/pkg/proxy/envoy/v2/debug.go	`29% <0%> (-2%)`	⬇️
mixer/adapter/servicecontrol/testhelper.go	`72% <0%> (-2%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`69% <0%> (-1%)`	⬇️
broker/pkg/platform/kube/crd/client.go	`71% <0%> (-1%)`	⬇️
... and 34 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 8ade289...00befba. Read the comment docs.

kyessenov · 2018-05-15T17:47:40Z

@sdake Are you sure that pilot's CRDs are defined in mixer helm chart? https://github.com/istio/istio/blob/master/install/kubernetes/helm/istio/charts/pilot/templates/crds.yaml#L52

kyessenov · 2018-05-15T17:49:23Z

@sdake Regarding the naming: I have followed what mixer does about the same problem (submitting CRs after CRDs), so the name is like this for consistency mostly.

mandarjog · 2018-05-15T17:53:26Z

@sdake Mixer job only defines Mixer CRs. It is best to keep CRs with the component that defines CRDs for it. Since DestinationRule is a Pilot CRD, the related CR should be defined with Pilot.
I think we should get this in quickly.

sdake

/lgtm
/approve

istio-testing · 2018-05-15T18:35:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdake

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~install/OWNERS~~ [sdake]

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

costinm

Again: please move them to mixer template.

Pilot is not the place to include configs for other components - for example it is possible for users to disable pilot and use a different implementation.

kyessenov · 2018-05-15T20:59:26Z

This is not an issue for an alternative implementation of pilot. Using mixer standalone with direct API does not need any pilot configs.

istio-testing · 2018-05-15T21:47:10Z

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

Test name	Commit	Details	Rerun command
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh	`00befba`	link	`/test istio-pilot-e2e-envoyv2-v1alpha3`
prow/istio-pilot-e2e.sh	`00befba`	link	`/test istio-pilot-e2e`
prow/e2e-bookInfoTests-v1alpha3.sh	`00befba`	link	`/test e2e-bookInfo-envoyv2-v1alpha3`

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.

sdake · 2018-05-16T13:50:12Z

Fixing #5635 - I noticed you may need a patch verb addition.

kyessenov · 2018-05-17T03:54:26Z

@sdake Yeah, "patch" would be useful. Don't think it's going to break anything now, since this is the first config that comes with pilot by default.

sdake · 2018-06-01T23:33:49Z

@kyessenov re patch, your right not needed for this particular patch, however, it appears with Kubernetes 1.10+ a security vulnerability was repaired which permitted patching a CRD without proper RBAC rules. This will need to go into a follow-on patch if you don't mind so upgrade from 0.8 to 1.0 works properly.

Cheers
-steve

kyessenov · 2018-06-01T23:36:09Z

I abandoned this PR.

googlebot added the cla: yes label May 11, 2018

kyessenov added 2 commits May 11, 2018 14:08

add config for mixerfilter

2b3e78b

Signed-off-by: Kuat Yessenov <kuat@google.com>

add job

c92248d

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov force-pushed the mixer_cluster_tuning branch from 4597a8b to c92248d Compare May 11, 2018 21:09

kyessenov requested review from mandarjog and hklai May 11, 2018 21:10

kyessenov commented May 11, 2018

View reviewed changes

kyessenov requested a review from rshriram May 11, 2018 22:06

hklai requested review from costinm and diemtvu May 14, 2018 20:09

costinm reviewed May 14, 2018

View reviewed changes

sdake reviewed May 15, 2018

View reviewed changes

Merge branch 'release-0.8' into mixer_cluster_tuning

a847afd

Merge branch 'release-0.8' into mixer_cluster_tuning

cb5ae8c

sdake approved these changes May 15, 2018

View reviewed changes

istio-testing assigned sdake May 15, 2018

istio-testing added the lgtm label May 15, 2018

istio-testing added the approved label May 15, 2018

costinm reviewed May 15, 2018

View reviewed changes

sdake removed approved labels May 15, 2018

Merge branch 'release-0.8' into mixer_cluster_tuning

00befba

istio-testing added the approved label May 15, 2018

mandarjog mentioned this pull request May 17, 2018

Add mixer cluster configuration to mixer CR jobs #5669

Merged

kyessenov closed this May 17, 2018

hklai mentioned this pull request May 17, 2018

Add mixer destination policy to the default install #5546

Closed

helm: mixer cluster tuning #5568

helm: mixer cluster tuning #5568

Uh oh!

Conversation

kyessenov commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented May 12, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented May 14, 2018

Uh oh!

costinm commented May 14, 2018

Uh oh!

costinm commented May 14, 2018

Uh oh!

kyessenov commented May 14, 2018

Uh oh!

costinm commented May 14, 2018

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdake commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kyessenov commented May 15, 2018

Uh oh!

kyessenov commented May 15, 2018

Uh oh!

mandarjog commented May 15, 2018

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented May 15, 2018

Uh oh!

costinm left a comment

kyessenov commented May 11, 2018 •

edited

Loading

sdake commented May 15, 2018 •

edited

Loading

codecov bot commented May 15, 2018 •

edited

Loading

kyessenov commented May 15, 2018 •

edited

Loading

istio-testing commented May 15, 2018 •

edited

Loading