Introduce unary mixer APIs. #95

geeknoid · 2017-05-04T18:01:07Z

For now, all unary methods have a 2
suffix to allow implementation work to proceed side-by-side with
the existing API. Once work is complete in the mixer and mixer
client, then we can get rid of the streaming API definitions and
drop the 2 suffix on the unary definitions.

ZackButcher

Please add reserved <field #>, <field #2>, ... to each of the messages where you delete a field, so that the numbers are reclaimed later (causes lots of pain).

https://developers.google.com/protocol-buffers/docs/proto#reserved

ZackButcher · 2017-05-05T20:39:03Z

mixer/v1/attributes.proto

-  // the mixer. The proxy needs to intelligently manage a pool of contexts.
-  // It may be useful to reset a context when certain big events happen, such
-  // as when an HTTP2 connection into the proxy terminates.
+  // TO BE REMOVED


What blocks these being removed now? I thought we only had them to be compatible with old version of the proxy's mixer client (which would now be very old).

It's easier for me to leave all the old stuff alone and remove it all in one shots once I know the proxy is using the new API exclusively.

ZackButcher · 2017-05-05T20:39:56Z

mixer/v1/attributes.proto

-  // to each gRPC stream.
-  map<int32, string> dictionary = 1;
+  // The message-level dictionary.
+  repeated string words = 13;


Nice, this is way easier than the map<i32, str> version.

ZackButcher · 2017-05-05T20:40:59Z

mixer/v1/attributes.proto

+  // one of the word dictionaries. Positive values are indices into the
+  // global deployment-wide dictionary, negative values are indices into
+  // the message-level dictionary.
+  map<sint32, sint32> string_attributes2 = 14;


nit: we can probably just drop the _attributes postfix here and elsewhere, I think it's implied by the context of the Attributes message.

But we probably don't want to do this pre-alpha, since it breaks proxy.

I like the idea. In order to make it possible to upgrade the proxy to use these new names, I think I need to introduce the new names now and support both in the mixer for a little while. I think I can do this relatively easily without too much boilerplate in Mixer.

ZackButcher · 2017-05-05T20:43:19Z

mixer/v1/attributes.proto

-// A map of string to string. The keys in these maps are from the current
-// dictionary.
+// A map of string to string. The keys and values in this map are dictionary
+// indices (see the `Attributes` message for an explanation)


You can refer to the Attributes message by changing this to:

see the [Attributes][istio.mixer.v1.Attributes] message for ...

and doc gen will link them up correctly.

Look at you, Mr DocGen expert now... :-)

I'll change it, thanks.

mandarjog · 2017-05-07T01:55:49Z

mixer/v1/check.proto

-
-  // Indicates whether or not the preconditions succeeded
-  google.rpc.Status result = 3 [(gogoproto.nullable) = false];
+  Attributes attributes = 5 [(gogoproto.nullable) = false];


Is this the cache key being returned by the check request?
If not we need it now, correct?

How to generate a cache key must be known statically by the caller. Think of it, how do you look something up from the cache without knowing the key?

Whatever we decide is the policy for cache key generation it needs to be documented here.

mandarjog · 2017-05-07T01:58:53Z

mixer/v1/check.proto


  // The amount of time for which this result can be considered valid, given the same inputs
  google.protobuf.Duration expiration = 4 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];
+
+  // TO BE REMOVED


just request_index is deprecated? Are we continuing with the attribute update protocol?

Otherwise please update to specify what is being removed.

Here and elsewhere in this change everything below the 'TO BE REMOVED' comment is deprecated.

attribute_update is replaced with the attributes field above. It's really just a rename, but I don't want to break compat so I'm introducing a new field for it.

douglas-reid · 2017-05-08T18:24:07Z

mixer/v1/service.proto

 service Mixer {
  // Checks preconditions before performing an operation.
  // The preconditions enforced depend on the set of supplied attributes and
  // the active configuration.
-  rpc Check(stream CheckRequest) returns (stream CheckResponse) {}
+  rpc Check2(CheckRequest) returns (CheckResponse) {}


i believe we've discussed this before, but why not create a separate service for these new methods and messages?

That seems like a cleaner way to transition and would avoid having all of these ugly *2 names.

It shouldn't be all that ugly in mixer to add handling for multiple entry points into the same dispatch code and should allow clean parallel development.

It's actually more involved. If I have two services with methods called Check but with different signatures, that means I'll need to have two different types implementing the interface. So a bunch of code in grpcServer.go would need to be split/replicated. It's just not worth the effort.

Cut'n'paste is not the worst evil and it would make reviewing this in isolation easier.

geeknoid · 2017-05-08T18:40:45Z

I think it is simply not worth any effort here. This is transient and will all get cleaned up in a couple weeks.

…

On Mon, May 8, 2017 at 11:24 AM, Douglas Reid ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mixer/v1/service.proto <#95 (comment)>: > service Mixer { // Checks preconditions before performing an operation. // The preconditions enforced depend on the set of supplied attributes and // the active configuration. - rpc Check(stream CheckRequest) returns (stream CheckResponse) {} + rpc Check2(CheckRequest) returns (CheckResponse) {} i believe we've discussed this before, but why not create a separate service for these new methods and messages? That seems like a cleaner way to transition and would avoid having all of these ugly *2 names. It shouldn't be all that ugly in mixer to add handling for multiple entry points into the same dispatch code and should allow clean parallel development. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#95 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHSweb5u9G5hl1kasJDA-Y3k254oaks5r313IgaJpZM4NRBM8> .

geeknoid · 2017-05-08T19:50:56Z

PTAL

Renamed fields per Zack's suggestion.
Clarified the TO BE REMOVED comments per Mandar's comment.
Will consider add reserved annotations when the old stuff is removed. It's not clear whether this is useful at all, since removing the old stuff is a fully breaking change anyway, so we may as well just renumber all the fields anew and start with a clean slate. We'll see when we get there...
Look at what it would take to avoid having a temporary 2 suffix on method names. It turns out to be a pain, and I stil end up having the service name itself with a 2 suffix (Mixer2). So it's not worth the hassle, I'm leaving this alone.

mandarjog · 2017-05-10T01:28:12Z

From the performance work that @costinm did, we have come across a distributed dead lock between the mixer client and the mixer server.

Both client and server are blocked on stream.Write, no one is reading and the stream is full.
There are several bugs that caused this problem that are being addressed, however having a clearer mixer api would have also helped.

Since Check and Quota are so closely related they should be one call.

PreRequest {
 CheckRequest
 ConditionalQuotaRequest   --> only executed if check succeeds 
}

This is not only more performant but it is also correctly expresses semantics correctly which are not apparent when Check and Quota are separate unrelated calls.
By encapsulating we are protecting a call with side effects by a conditional.
It also makes for a cleaner caching story.
@louiscryan
This is orthogonal to batching unrelated requests.

Details of the deadlock.
https://github.com/istio/proxy/issues/300#issuecomment-300338197

costinm · 2017-05-10T02:51:07Z

AFAIK there are 2 kinds of quota (at least): one applied per user or other attributes resulting from CheckRequest, but the other is to rate limit access to the resource and protect the CheckRequest itself. Also quota has pretty different perf characteristics - you can use batching, sampling, buckets, etc. It may be worth keeping it separate - in terms of cost it's not that much if it's properly optimized. Having rate limit/quota in front of authentication can really save the day...

…

On May 9, 2017 6:28 PM, "mandarjog" ***@***.***> wrote: From the performance work that @costinm <https://github.com/costinm> did, we have come across a distributed dead lock between the mixer client and the mixer server. Both client and server are blocked on stream.Write, no one is reading and the stream is full. There are several bugs that caused this problem that are being addressed, however having a clearer mixer api would have also helped. Since Check and Quota are so closely related they should be one call. PreRequest { CheckRequest ConditionalQuotaRequest --> only executed if check succeeds } This is not only more performant but it is also correctly expresses semantics correctly which are not apparent when Check and Quota are separate unrelated calls. By encapsulating we are protecting a call with side effects by a conditional. It also makes for a cleaner caching story. @louiscryan <https://github.com/louiscryan> This is orthogonal to batching unrelated requests. Details of the deadlock. istio/proxy#300 (comment) <https://github.com/istio/proxy/issues/300> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6sAl8-waFlZjahK3nYwQ_-hTTSshks5r4RKsgaJpZM4NRBM8> .

geeknoid · 2017-05-10T04:13:46Z

Mandar, can you explain why combining the two calls into one would avoid the deadlock? I have a hard time believing that this is so. On Tue, May 9, 2017 at 7:51 PM, Costin Manolache <[email protected]> wrote:

…

AFAIK there are 2 kinds of quota (at least): one applied per user or other attributes resulting from CheckRequest, but the other is to rate limit access to the resource and protect the CheckRequest itself. Also quota has pretty different perf characteristics - you can use batching, sampling, buckets, etc. It may be worth keeping it separate - in terms of cost it's not that much if it's properly optimized. Having rate limit/quota in front of authentication can really save the day... On May 9, 2017 6:28 PM, "mandarjog" ***@***.***> wrote: > From the performance work that @costinm <https://github.com/costinm> did, > we have come across a distributed dead lock between the mixer client and > the mixer server. > > Both client and server are blocked on stream.Write, no one is reading and > the stream is full. > There are several bugs that caused this problem that are being addressed, > however having a clearer mixer api would have also helped. > > Since Check and Quota are so closely related they should be one call. > > PreRequest { > CheckRequest > ConditionalQuotaRequest --> only executed if check succeeds > } > > This is not only more performant but it is also correctly expresses > semantics correctly which are not apparent when Check and Quota are > separate unrelated calls. > By encapsulating we are protecting a call with side effects by a > conditional. > It also makes for a cleaner caching story. > @louiscryan <https://github.com/louiscryan> > This is orthogonal to batching unrelated requests. > > Details of the deadlock. > istio/proxy#300 (comment) <https://github.com/istio/proxy/issues/300> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#95 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe- auth/AAFI6sAl8-waFlZjahK3nYwQ_-hTTSshks5r4RKsgaJpZM4NRBM8> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHRl-cihulMRuIM1NP7Z-0x4ZY7vlks5r4SYcgaJpZM4NRBM8> .

mandarjog · 2017-05-10T04:35:49Z

ref dead lock table.

The API design was not the cause of the deadlock, but on the mixer client side it introduced complexity of having 2 successive async calls to the mixer. It was this complexity that lead to the bug.

If the common case is to use those 2 calls together, then that should be part of the API, making it simple to write the mixer client.

kyessenov · 2017-05-10T04:38:00Z

hint hint: we need futures in GRPC.
i think the proxy change and deadlines in mixer are probably enough to fix deadlock.

louiscryan · 2017-05-10T16:35:49Z

mixer/v1/attributes.proto

+  map<sint32, google.protobuf.Timestamp> timestamps = 18 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
+  map<sint32, google.protobuf.Duration> durations = 19 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];
+  map<sint32, bytes> bytes = 20;
+  map<sint32, StringMap> stringMaps = 21 [(gogoproto.nullable) = false];


As discussed offline we can also do

message Value { sint32 name = 1; oneof value_entry { sint32 word_ref = 2; int64 int_value = 3; double double_value = 4; ... } } repeated Value values = 1;

If we wanted to avoid the object materialization costs associated with this we create a custom protobuf deserializer which is not particularly hard. The only concern that would remain is how quickly/easily clients can produce this structure.

Since I have this current API implemented, I was going to check that in first, and then evaluate whether this new approach is usable and performs well.

Fair enough, we can keep iterating once there are some benchmarks

louiscryan · 2017-05-10T16:37:03Z

mixer/v1/attributes.proto

 message StringMap {
+  map<sint32, sint32> entries = 2;


This should be a definite improvement, nice.

louiscryan · 2017-05-10T16:38:51Z

mixer/v1/quota.proto

-  // Indicates whether the quota request was successfully processed.
-  google.rpc.Status result = 3 [(gogoproto.nullable) = false];
-
+message QuotaResponse {
  // The amount of time the returned quota can be considered valid, this is 0 for non-expiring quotas.


Document what null implies here

Expanded the comment.

louiscryan · 2017-05-10T16:43:34Z

mixer/v1/report.proto

-  // Index within the stream for this request, used to match to responses
+  // The attributes to use for this request.
+  //
+  // These attributes are delta-encoded. In other words, each individual


Do we really want sequential delta encoding like this, it seems like a lot of work for the client.

I think clients would rather just build up a single dictionary over the batch here and then emit independent Atrtibutes using that dictionary as the base. Sequential delta encoding will be much harder to implement for the client and the server and is unlikely to yield much benefit over a large shared dictionary if any.

The model I defined here supports a shared dictionary.

If a client doesn't want to support delta-encoding, it can just send a full complement of attributes with every message. I added a comment to that effect.

I think this can be a big win. If we batch 1000 metrics where only a handful of attributes change from message to message, the payload will be substantially smaller using delta encoding.

Really? I'd like to see some quantification of that. If every string has become an int reference into a common dictionary I suspect that delta encoding is (a) at best a very marginal improvement and (b) slower because of the increased complexity involved in handle add/removes

THIS IS A BREAKING CHANGE. OLDER MIXER CLIENTS WILL STOP WORKING.

* Control plane updates (istio#49) * Control plane updates Signed-off-by: Liam White <[email protected]> * fix merge mistakes Signed-off-by: Liam White <[email protected]> * fix telem store comments Signed-off-by: Liam White <[email protected]> * docs: Fix single-tick (istio#50) Signed-off-by: Dhi Aurrahman <[email protected]> * add protocol to elastic settings (istio#51) * add protocol to elastic settings Signed-off-by: Liam White <[email protected]> * gogo Signed-off-by: Liam White <[email protected]> * generate fixes Signed-off-by: Liam White <[email protected]> * Add job annotations for components with jobs (istio#52) * Add container port and service types + refactor (istio#56) * Add container port and service types + refactor Signed-off-by: Liam White <[email protected]> * makefile and comment touch-ups Signed-off-by: Liam White <[email protected]> * Make Istio install atomic and add more currently used fields (istio#61) * Fix TLS capitalization (istio#62) * Add proto for some more postgres configuration (istio#53) * Add proto for some more postgres configuration * Address comments * More comments * Add Escape Hatch for users with weird edge cases (istio#65) Also need to move back to golang protobuf in order to re-use the Istio patch framework. * Use older Istio API to avoid breaking dp operator (istio#66) * Postgres SSL settings (istio#67) * Postgres SSL settings * Update install/managementplane/v1alpha1/settings.proto Co-authored-by: Liam White <[email protected]> * Update install/managementplane/v1alpha1/settings.proto Co-authored-by: Liam White <[email protected]> * Add remaining SSL modes Co-authored-by: Liam White <[email protected]> * Configure CNI (istio#68) * Configure CNI * review Signed-off-by: Liam White <[email protected]> * Add CNI config file name, used when chained is false Co-authored-by: Liam White <[email protected]> * Revert "Postgres SSL settings (istio#67)" (istio#70) * Revert "Postgres SSL settings (istio#67)" This reverts commit 391889fdaf5bfa3996c85719975df1c44ce94102. * Add back all ssl modes * Doc Fixes for Install API (istio#71) * ci: Add check (istio#73) This patch brings the check script to release-0.8.0. Seems like this is the new "master". Signed-off-by: Dhi Aurrahman <[email protected]> * docs: Add types frontmatter (istio#74) This adds types frontmatter data to types.proto. Until we have an automated way to populate title, we are encouraged to add a title when it is desired. Signed-off-by: Dhi Aurrahman <[email protected]> * Postgres SSL mode default to require (istio#75) * Autogen JSON shims (istio#80) * nits * Update install readme (istio#85) * Add ES version to settings (istio#88) * Add ES version to settings * ES version int with validation Co-authored-by: Liam White <[email protected]> Co-authored-by: Dhi Aurrahman <[email protected]> Co-authored-by: Vikas Choudhary (vikasc) <[email protected]> Co-authored-by: Marc Cirauqui <[email protected]> This PR merges the release_0.8.0 (as of commit - d3a19fdd8d25cdd8f386c4a4a2a821daa9865df0) branch into master. operator protos needs go_plugin instead of gogofast_plugin for the protoc. It is as is merge otherwise.

geeknoid added the kind/enhancement label May 4, 2017

geeknoid added this to the api beta milestone May 4, 2017

geeknoid requested review from mandarjog, ZackButcher and douglas-reid May 4, 2017 18:01

ZackButcher reviewed May 5, 2017

View reviewed changes

mandarjog reviewed May 7, 2017

View reviewed changes

douglas-reid reviewed May 8, 2017

View reviewed changes

louiscryan reviewed May 10, 2017

View reviewed changes

Replace Mixer's streaming API with a unary API.

3205a08

THIS IS A BREAKING CHANGE. OLDER MIXER CLIENTS WILL STOP WORKING.

geeknoid closed this May 13, 2017

Introduce unary mixer APIs. #95

Introduce unary mixer APIs. #95

Uh oh!

Conversation

geeknoid commented May 4, 2017

Uh oh!

ZackButcher left a comment

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!

geeknoid commented May 8, 2017 via email

Uh oh!

geeknoid commented May 8, 2017

Uh oh!

mandarjog commented May 10, 2017

Uh oh!

costinm commented May 10, 2017 via email

Uh oh!

geeknoid commented May 10, 2017 via email

Uh oh!

mandarjog commented May 10, 2017

Uh oh!

kyessenov commented May 10, 2017

Uh oh!

louiscryan May 10, 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!

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!

Uh oh!

louiscryan May 10, 2017 •

edited

Loading