Enable new style quota configuration in testing #725

mandarjog · 2017-09-11T00:45:34Z

Mixer2 quota requires a different type of config.
This updates the config and uses a Mixer build that supports that config.

Release note:

NONE

istio-merge-robot · 2017-09-11T02:27:30Z

@mandarjog PR needs rebase

kyessenov · 2017-09-11T03:49:35Z

samples/apps/bookinfo/rules/mixer-rule-ratings-ratelimit.yaml

+spec:
+  quotas:
+  - name: requestcount.quota.istio-config-default
+    max_amount: 5000


nit: JSON normal form is maxAmount, although proto parser is lenient.

kyessenov · 2017-09-11T03:49:51Z

samples/apps/bookinfo/rules/mixer-rule-ratings-ratelimit.yaml

+    valid_duration: 1s
+    overrides:
+    - dimensions:
+        destination: ratings


Might be worth adding a comment what this means

kyessenov · 2017-09-11T03:50:56Z

samples/apps/bookinfo/rules/mixer-rule-ratings-ratelimit.yaml

+spec:
+  dimensions:
+    source: source.labels["app"] | "unknown"
+    destination: destination.labels["apps"] | "unknown"


destination.name also works, right?

target.name: istio-ingress-2398531633-tj53n is the pod hostname so you can't write rules based on it.

Interesting, that sounds like a bug. "name" should "istio-ingress" here, the short name of the service.

kyessenov · 2017-09-11T03:51:56Z

samples/apps/bookinfo/rules/mixer-rule-standard-attributes.yaml

+        valueType: STRING
+      context.timestamp:
+        valueType: TIMESTAMP
+      context.time:


Why both time and timestamp? This also looks like a more consequential change than quota update.

copied from the API repo.

kyessenov · 2017-09-11T03:52:19Z

samples/apps/bookinfo/rules/mixer-rule-standard-attributes.yaml

@@ -84,4 +106,16 @@ spec:
        valueType: STRING
      target.serviceAccount:
        valueType: STRING
+      destination.ip:


Please add destination.port if you are at it.

needs to be done in the API repo first.

kyessenov · 2017-09-11T03:53:26Z

samples/apps/bookinfo/rules/mixer-rule-standard-attributes.yaml

+        valueType: DURATION
+      context.protocol:
+        valueType: STRING
+      context.timestamp:


the casing is all over place serviceAccount, user-agent and bytes_total?

Those I have just copied over from the API repo, let's see if we can fix it there.

Can you point me to the source of truth? I'll file an issue.

we need a pass at this anyway in istio/api.

ldemailly · 2017-09-11T04:43:52Z

what do we need to do so people type something instead of "<PLEASE PUT RELEASE-NOTE HERE, PUT NONE if NO RELEASE-NOTE>"

ldemailly

no random sha from a pr please

ldemailly · 2017-09-11T04:46:18Z

install/kubernetes/addons/grafana.yaml

@@ -26,7 +26,7 @@ spec:
    spec:
      containers:
      - name: grafana
-        image: gcr.io/istio-testing/grafana:1bc30a23190aa58635d02ff7fd31bf74de0d011e
+        image: gcr.io/istio-testing/grafana:1af0c4d990ff6bb881c0243b802ed4a8d96f1a5a


This PR needs at least the given level of Mixer. Once that PR is merged, I will remove the specific sha.
I will not merge this PR with this sha.

istio-merge-robot · 2017-09-11T18:42:39Z

@mandarjog PR needs rebase

No longer updating build SHA

mandarjog · 2017-09-13T15:32:31Z

Tests Summary
PASSED: //tests/e2e/tests/mixer:go_default_test 
PASSED: //tests/e2e/tests/bookinfo:go_default_test

This PR with the correct Mixer build passes.

mandarjog · 2017-09-13T15:59:48Z

@rshriram @ldemailly let's merge this please.
This is required for tests to pass at head of mixer.
It includes commits from #775

douglas-reid · 2017-09-13T16:29:22Z

/lgtm

istio-merge-robot · 2017-09-13T16:30:41Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-09-13T16:37:37Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @douglas-reid @mandarjog

ldemailly · 2017-09-13T16:38:13Z

/retest

ldemailly · 2017-09-13T16:59:10Z

failed with #777

ldemailly · 2017-09-13T17:06:57Z

tests/e2e/tests/mixer/mixer_test.go

 	}
 	glog.Infof("promvalue := %s", value.String())

 	got, err := vectorValue(value, map[string]string{responseCodeLabel: "429"})
 	if err != nil {
 		t.Logf("prometheus values for request_count:\n%s", promDump(promAPI, "request_count"))
-		t.Fatalf("Could not find rate limit value: %v", err)
+		fatalf(t, "Could not find rate limit value: %v", err)


this should not be fatal, should be errorf + got =0

let's not change it in this PR.

douglas-reid · 2017-09-13T17:14:46Z

private cluster testing:

****************************************************
Tests Summary
PASSED: //tests/e2e/tests/mixer:go_default_test
PASSED: //tests/e2e/tests/bookinfo:go_default_test

mandarjog · 2017-09-13T17:23:30Z

/retest

douglas-reid · 2017-09-13T18:11:55Z

/lgtm

istio-merge-robot · 2017-09-13T18:12:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid

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

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

~~OWNERS~~ [douglas-reid]

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

istio-merge-robot · 2017-09-13T18:12:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-testing · 2017-09-13T18:19:07Z

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

Test name	Commit	Details	Rerun command
prow/new-e2e-rbac_no_auth.sh	`ad30540`	link	`/test new-e2e-rbac_no_auth`

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.

istio-merge-robot · 2017-09-13T18:36:12Z

Automatic merge from submit-queue

Former-commit-id: 1acd77e02c2c1b58f3aeb465712e623514785e29

Automatic merge from submit-queue Enable new style quota configuration in testing Mixer2 quota requires a different type of config. This updates the config and uses a Mixer build that supports that config. **Release note**: ```release-note NONE ``` Former-commit-id: a5bda88

Former-commit-id: b50f4b0dabc98c7d988080971150bee5244636ec

Automatic merge from submit-queue Enable new style quota configuration in testing Mixer2 quota requires a different type of config. This updates the config and uses a Mixer build that supports that config. **Release note**: ```release-note NONE ``` Former-commit-id: a5bda88

* Revert "Strip out "spiffe://" in the identity (istio#719)" This reverts commit 99a482f. * Revert "Revert "Remove -release in filename when doing release build of proxy (istio#704)" (istio#723)" This reverts commit 13669ce. * Revert "Not to send legacy quota for v2 config. (istio#722)" This reverts commit aaf25ca.

…istio#733) This reverts commit ef74122.

* Fix args.wait not being used. * Fix import.

googlebot added the cla: yes label Sep 11, 2017

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

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 11, 2017

kyessenov reviewed Sep 11, 2017

View reviewed changes

ldemailly previously requested changes Sep 11, 2017

View reviewed changes

ldemailly reviewed Sep 11, 2017

View reviewed changes

mandarjog force-pushed the quota2 branch from 0680c2b to c020920 Compare September 11, 2017 14:22

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 11, 2017

mandarjog mentioned this pull request Sep 11, 2017

Add overrides to memquota2 istio/old_mixer_repo#1256

Merged

mandarjog requested a review from douglas-reid September 11, 2017 18:42

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 11, 2017

mandarjog mentioned this pull request Sep 12, 2017

remove memQuota adapter long with old style config files istio/old_mixer_repo#1271

Merged

Use new quota config

e39941b

mandarjog force-pushed the quota2 branch from 46c0bd8 to e39941b Compare September 13, 2017 13:29

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 13, 2017

mandarjog mentioned this pull request Sep 13, 2017

istio/istio e2e ratelimit test fails reliably if upgrading mixer to latest (seemingly because of unknown) istio/old_mixer_repo#1286

Closed

mandarjog and others added 2 commits September 13, 2017 08:15

fix config file

0fea524

Update_Dependencies

e79de57

mandarjog mentioned this pull request Sep 13, 2017

update dependencies of istio - spray and pray edition #775

Closed

istio-testing assigned douglas-reid Sep 13, 2017

istio-testing added the lgtm label Sep 13, 2017

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

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

istio deleted a comment from istio-testing Sep 13, 2017

ldemailly reviewed Sep 13, 2017

View reviewed changes

ldemailly mentioned this pull request Sep 13, 2017

Make rate limit always display the 200s RPS #776

Merged

Merge branch 'master' into quota2

ad30540

douglas-reid added this to the Istio 0.2 milestone Sep 13, 2017

istio-testing added the lgtm label Sep 13, 2017

douglas-reid approved these changes Sep 13, 2017

View reviewed changes

istio-merge-robot merged commit a5bda88 into istio:master Sep 13, 2017

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

typecast time.Duration only where necessary (istio#725)

55bea3b

Former-commit-id: 1acd77e02c2c1b58f3aeb465712e623514785e29

mandarjog deleted the quota2 branch October 30, 2017 21:07

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

typecast time.Duration only where necessary (#725)

70dbcf2

Former-commit-id: b50f4b0dabc98c7d988080971150bee5244636ec

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 pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020

Fix --wait in manifest apply not being used. (istio#725)

1a7d657

* Fix args.wait not being used. * Fix import.

0x01001011 pushed a commit to thedemodrive/istio that referenced this pull request Jul 16, 2020

target -> targets (istio#725)

b917615

Enable new style quota configuration in testing #725

Enable new style quota configuration in testing #725

Conversation

mandarjog commented Sep 11, 2017 • edited Loading

istio-merge-robot commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldemailly commented Sep 11, 2017

ldemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-merge-robot commented Sep 11, 2017

mandarjog commented Sep 13, 2017

mandarjog commented Sep 13, 2017

douglas-reid commented Sep 13, 2017

istio-merge-robot commented Sep 13, 2017

istio-merge-robot commented Sep 13, 2017

ldemailly commented Sep 13, 2017

ldemailly commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglas-reid commented Sep 13, 2017

mandarjog commented Sep 13, 2017

douglas-reid commented Sep 13, 2017

istio-merge-robot commented Sep 13, 2017

istio-merge-robot commented Sep 13, 2017

istio-testing commented Sep 13, 2017 • edited Loading

istio-merge-robot commented Sep 13, 2017

mandarjog commented Sep 11, 2017 •

edited

Loading

istio-testing commented Sep 13, 2017 •

edited

Loading