Add overrides to memquota2 by mandarjog · Pull Request #1256 · istio/old_mixer_repo

mandarjog · 2017-09-10T23:41:55Z

This PR adds override support to memquota2 as back end configuration with the intention of migrating then to individual config resources in 0.3

It gains feature parity with the old memquota setup + old config model.
This lets Mixer switch off old quota dispatch after creating replacement test configurations.

Release note:

NONE

This change is

mandarjog · 2017-09-11T14:43:37Z

Added istio/istio PR

mandarjog · 2017-09-11T14:55:02Z

adapter/memquota2/config/config.proto


 message Params {
 	message Quota {
+		option (gogoproto.goproto_getters) = true;


Need getters so we can abstract over Quota and Override message.
They both implement

interface { GetValidDuration() GetMaxAmount() }

douglas-reid · 2017-09-11T15:48:34Z

adapter/memQuota/util/util.go


-	if qu.Logger.VerbosityLevel(4) {
+	if len(t) > 0 && qu.Logger.VerbosityLevel(4) {
 		qu.Logger.Infof("Running repear to reclaim %d old deduplication entries", len(t))


let's also fix: "repear" -> "reaper"

How do we keep ending up with this change being removed? This is at least the third time.

This PR can now remove the entire adapter, but I will send a separate PR for that.

douglas-reid · 2017-09-11T15:51:47Z

adapter/memquota2/memquota.go

+// matchDimensions matches configured dimensions with dimensions of the instance.
+func matchDimensions(cfg map[string]string, inst map[string]interface{}) bool {
+	for k, val := range cfg {
+		rval := inst[k]


so, this requires the dimensions to be in the same order as well, right?

This is a map lookup - it shouldn't impose ordering constraints, right?

Not really. we are checking all dimensions specified in adapterConfig. They can appear in any order.

ah, ok. i was looking too quickly while trying to participate in that meeting.

douglas-reid · 2017-09-11T15:53:27Z

adapter/memquota2/memquota.go

+func limit(cfg *config.Params_Quota, instance *quota.Instance) Limit {
+	for idx := range cfg.Overrides {
+		o := cfg.Overrides[idx]
+		if matchDimensions(o.Dimensions, instance.Dimensions) {


don't the MakeKey() methods provide a set of functionality we could use instead of matchDimensions? Is there a reason not to use the existing Key funcs?

MakeKey() creates a unique key using all dimensions.
matchDimensions needs to match on partial dimensions.

douglas-reid · 2017-09-11T15:55:24Z

pkg/api/grpcServer.go

 				}
 			}
+
+			if glog.V(2) {


do we want this section of code? Doesn't this imply dimensions that might not be on the quota descriptor? For instance, I might not choose to dimension quota with source.name.

regardless of quota dimensioning, I want accessLog to convey just some reasonable notion of source and target, how much quota was requested and how much was granted.

+1, I don't like this chunk. If we want this kind of logging, just log "allocated %d tokens along dimensions: %v". Don't hard code attributes anywhere.

I just log destination.service along with quota response now.

I also log full dimensions at log level 2.

douglas-reid · 2017-09-11T15:56:09Z

pkg/runtime/dispatcher.go

-				if inst.Name != qma.Quota {
-					continue
-				}
+				// if inst.Name != qma.Quota {


what is the harm in leaving this check enabled?

It will not work. The qma.Name == RequestCount, however instance names in config are fully qualified. So mixer client will have to send the specific fully qualified name over.

douglas-reid · 2017-09-11T15:57:20Z

testdata/config/quota.yaml

@@ -17,7 +23,7 @@ metadata:
 spec:
  dimensions:
    source: source.labels["app"] | "unknown"


with the raw vm work, i'm wondering if we should switch from labels to source.service (at least for 0.2).

how about source.labels["app"] | source.service

how about source.labels["app"] | source.service | "unknown" to cover all bases?

douglas-reid · 2017-09-11T19:00:47Z

/lgtm

istio-merge-robot · 2017-09-11T19:00:53Z

[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.

Details

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

* Add overrides to memquota2 * change access log, reduce logs * remove adapter/kubernetes/kubeconfig added my mistake * make linter happy

mandarjog requested a review from douglas-reid September 10, 2017 23:41

googlebot added the cla: yes label Sep 10, 2017

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

mandarjog mentioned this pull request Sep 11, 2017

Enable new style quota configuration in testing istio/istio#725

Merged

istio-merge-robot added release-note-none and removed release-note-label-needed labels Sep 11, 2017

mandarjog commented Sep 11, 2017

View reviewed changes

douglas-reid reviewed Sep 11, 2017

View reviewed changes

mandarjog requested a review from ZackButcher September 11, 2017 16:38

mandarjog added 2 commits September 11, 2017 11:23

Add overrides to memquota2

105956f

change access log, reduce logs

d15e4b4

mandarjog force-pushed the quota2 branch from 1af0c4d to d15e4b4 Compare September 11, 2017 18:36

mandarjog added 2 commits September 11, 2017 11:40

remove adapter/kubernetes/kubeconfig added my mistake

abb399a

make linter happy

542a0af

istio-testing assigned douglas-reid Sep 11, 2017

istio-testing added the lgtm label Sep 11, 2017

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

ZackButcher approved these changes Sep 11, 2017

View reviewed changes

mandarjog merged commit 36994fb into istio:master Sep 11, 2017

rvkubiak pushed a commit to rvkubiak/mixer that referenced this pull request Oct 10, 2017

Add overrides to memquota2 (istio#1256)

c58e702

* Add overrides to memquota2 * change access log, reduce logs * remove adapter/kubernetes/kubeconfig added my mistake * make linter happy

Conversation

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

Uh oh!

Uh oh!

mandarjog commented Sep 11, 2017

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!

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!

douglas-reid commented Sep 11, 2017

Uh oh!

istio-merge-robot commented Sep 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mandarjog commented Sep 10, 2017 •

edited

Loading