proxy: Rewrite mock controller to accept a stream of dst updates #808
Conversation
Currently, the mock controller, which is used in tests, takes all of its updates a priori, which makes it hard to control when an update occurs within a test. Now, the controller exposes a `DstSender`, which wraps an unbounded channel of destination updates. This allows tests to trigger updates at a specific point in the test. In order to accomplish this, the controller's hand-rolled gRPC server implementation has been discarded in favor of a real gRPC destination service. This requires that the `controller-grpc` project now builds both clients and servers for the destination service. Additionally, we now build a tap client as well (assuming that we'll want to write tests against our tap server).
|
Um, yes please! :praisethesun: |
There was a problem hiding this comment.
This is excellent --- I had wanted to see a change like this for a while, thanks for actually implementing it!
I believe that these changes will fix the flakiness of the outbound_dst_labels tests, so we can remove the ignore attribute from them and (provided that passes CI) close #751.
| @@ -519,19 +514,17 @@ mod outbound_dst_labels { | |||
| fn controller_updates_addr_labels() { | |||
There was a problem hiding this comment.
I believe the changes on this branch should make this test no longer flaky, so we can remove the ignore attribute.
There was a problem hiding this comment.
I don't think that's the case.
This test does the following:
- publishes a dst
- routes a request
- publishes an updated dst
- routes a request
at (2), we must get a dst to complete the routing. at (4), however, i don't think that we can be guaranteed that the router has observed the update, since it can just route on the old data. Typically, it will; but no guarantees about cross-thread visibility/ordering.
There was a problem hiding this comment.
Ah, I think you're right, nevermind. Oh well.
| @@ -576,11 +577,15 @@ mod outbound_dst_labels { | |||
| fn controller_updates_set_labels() { | |||
There was a problem hiding this comment.
Once again, this test is probably no longer flaky.
proxy/tests/telemetry.rs
Outdated
| @@ -488,15 +476,22 @@ mod outbound_dst_labels { | |||
| // map ordering. | |||
| assert_contains!(metrics.get("/metrics"), "dst_set_label1=\"foo\""); | |||
| assert_contains!(metrics.get("/metrics"), "dst_set_label2=\"bar\""); | |||
| drop(dst_tx) | |||
There was a problem hiding this comment.
Will this not be dropped implicitly when exiting the test function?
There was a problem hiding this comment.
oh, yeah, this was for debugging
proxy/tests/support/controller.rs
Outdated
| use conduit_proxy_controller_grpc as pb; | ||
| use self::bytes::BufMut; | ||
| use self::prost::Message; | ||
| //use conduit_proxy_controller_grpc as pb; |
|
Looks like 1fdcb49 broke the build but it's somewhat unclear to me how that change caused an unused import in the generated code... |
…kerd#808) Currently, the mock controller, which is used in tests, takes all of its updates a priori, which makes it hard to control when an update occurs within a test. Now, the controller exposes a `DstSender`, which wraps an unbounded channel of destination updates. This allows tests to trigger updates at a specific point in the test. In order to accomplish this, the controller's hand-rolled gRPC server implementation has been discarded in favor of a real gRPC destination service. This requires that the `controller-grpc` project now builds both clients and servers for the destination service. Additionally, we now build a tap client as well (assuming that we'll want to write tests against our tap server).
Currently, the mock controller, which is used in tests, takes all of its
updates a priori, which makes it hard to control when an update occurs within a
test.
Now, the controller exposes a
DstSender, which wraps an unbounded channel ofdestination updates. This allows tests to trigger updates at a specific point
in the test.
In order to accomplish this, the controller's hand-rolled gRPC server
implementation has been discarded in favor of a real gRPC destination service.
This requires that the
controller-grpcproject now builds both clientsand servers for the destination service. Additionally, we now build a tap
client as well (assuming that we'll want to write tests against our tap
server).
This change is necessary in order to test an upcoming change to telemetry labeling.