admin: improve infrastructure #3172

trabetti · 2018-04-23T21:18:45Z

Signed-off-by: trabetti talis@il.ibm.com

title: admin: add infrastructure, which will later be used for hystrix dashboard support

Description: split from PR #2736

Pass AdminFilter& to admin handlers, so that all its parameters can be used by the handlers, without the need to pass them as arguments. Also enables passing more data to future handlers without adding interface arguments.
Handlers can add callbacks to be executed when connection is destroyed
Support for streaming response

Risk Level: Medium

Testing:
Will need to add coverage tests

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz

Thanks for splitting this out. Looks great. No action called for on my one comment: just pointing that out in general :)

jmarantz · 2018-04-23T21:52:31Z

source/server/http/admin.cc

@@ -150,6 +150,16 @@ Http::FilterTrailersStatus AdminFilter::decodeTrailers(Http::HeaderMap&) {
  return Http::FilterTrailersStatus::StopIteration;
 }

+void AdminFilter::onDestroy() {
+  for (const auto& callback : on_destroy_callbacks_) {


for the record, generally we shouldn't use 'auto' as the type of a range variable when looping over a simple (non-map) container. See

https://google.github.io/styleguide/cppguide.html#auto

However in this case it's OK because the usage makes it pretty obvious what you've got.

mrice32

Thanks for splitting this out! Could we also add the Server::Instance interface change that was in the Hystrix PR:

virtual std::chrono::milliseconds statsFlushInterval() PURE;

mattklein123

Generally LGTM, small interface comment/nit.

mattklein123 · 2018-04-23T22:03:32Z

include/envoy/server/admin.h

@@ -15,6 +15,7 @@
 namespace Envoy {
 namespace Server {

+class AdminFilter;


nit: I'm not in favor of exposing out the AdminFilter object in this manner into the abstract includes. Can we add an AdminFilter or AdminStream interface here and then fill it in with relevant callbacks? Then the existing admin filter can implement this interface as needed?

sgtm. @trabetti I'm glad we're iterating on this in this small PR. Can you make Matt's suggested changes, and I'll take another look.

@mattklein123 in a past life I might have called this new interface the RequestContext, except for the callback-array, which feels more specialized to the admin domain, but I'm not sure. But AdminStream is probably fine too.

Either is fine with me.

@mattklein123 @jmarantz - I pushed a fix. Not sure if what I did is what you meant here. Please review.

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz

looks like the tests are broken with a compile error.

jmarantz · 2018-04-24T16:45:23Z

include/envoy/server/admin.h

+  virtual void setEndStreamOnComplete(const bool& end_stream) PURE;
+  virtual void addOnDestroyCallback(std::function<void()> cb) PURE;
+  virtual Http::StreamDecoderFilterCallbacks* getDecoderFilterCallbacks() PURE;
+  virtual const Http::HeaderMap* getRequestHeaders() PURE;


virtual const Http::HeaderMap& getRequestHeaders() const PURE;

jmarantz · 2018-04-24T16:46:15Z

include/envoy/server/admin.h

+  virtual ~AdminStream() {}
+  virtual void setEndStreamOnComplete(const bool& end_stream) PURE;
+  virtual void addOnDestroyCallback(std::function<void()> cb) PURE;
+  virtual Http::StreamDecoderFilterCallbacks* getDecoderFilterCallbacks() PURE;


virtual const Http::StreamDecoderFilterCallbacks& getDecoderFilterCallbacks() const PURE;

return callbacks by const ref, and make the method const.

mattklein123

Yes in general this is great. Some small comments and please check the CI errors, but otherwise is on the right track. Thank you!

mattklein123 · 2018-04-24T16:46:07Z

include/envoy/server/admin.h

+class AdminStream {
+public:
+  virtual ~AdminStream() {}
+  virtual void setEndStreamOnComplete(const bool& end_stream) PURE;


Can you doc comment all these methods?

mattklein123 · 2018-04-24T16:46:20Z

include/envoy/server/admin.h

-         Buffer::Instance& data) -> Http::Code {                                                   \
-    return X(path_and_query, response_headers, data);                                              \
+         Buffer::Instance& data, AdminStream& admin_filter) -> Http::Code {                        \
+    return X(path_and_query, response_headers, data, admin_filter);                                \


nit: s/admin_filter/admin_stream here and elsewhere

mattklein123 · 2018-04-24T16:46:50Z

source/server/http/admin.h

@@ -228,12 +235,14 @@ class AdminImpl : public Admin,
 /**
 * A terminal HTTP filter that implements server admin functionality.
 */
-class AdminFilter : public Http::StreamDecoderFilter, Logger::Loggable<Logger::Id::admin> {
+class AdminFilter : public Http::StreamDecoderFilter,
+                    AdminStream,


public AdminStream

mrice32

LGTM after addressing @mattklein123 and @jmarantz's comments.

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz · 2018-04-24T20:50:50Z

include/envoy/server/admin.h

+
+  /**
+   * @param end_stream set to false for streaming response. Default is true, which will close
+   * connection on socket


End sentences with period, here & below.

jmarantz · 2018-04-24T21:00:10Z

include/envoy/server/admin.h

+   * @return Http::StreamDecoderFilterCallbacks* to be used by the handler to get socket for data
+   * streaming
+   */
+  virtual const Http::StreamDecoderFilterCallbacks* getDecoderFilterCallbacks() const PURE;


Can you either make a guarantee that the return-value is non-null by returning a ref, or documenting that nullptr might be an actual return-value?

I think that in the actual usage model, there's no way for null to be returned in the context of an admin callback to which the stream object is passed. However, I can see that AdminFilter does have these null on construction.

So what's the correct way to use this, and getRequestHeaders()?

One idea is you can make this type-safe by having an StreamImpl object which contains a pointer to the AdminFilter as a delegate, and implements these methods with guarantees implied by returning const ref for the callbacks and requestHeaders, know that at the time is is called, those are guaranteed non-null.

In that case you wouldn't multiply inherit from AdminFilter to AdminStream.

This is a slightly more complex implementation, but a much simpler-to-describe behavior. With what you have, I think you can't make the guarantee without explaining when it's safe to assume non-null, or forcing the caller to check for nullptr, creating hard-to-test paths through the code.

WDYT?

@jmarantz I do not feel I am familiar enough with the behavior to decide whether we can guarantee a non-null. Maybe someone else can comment on this. Can you be more explicit about the suggested solution?

@jmarantz @mrice32 Another question - there is coverage missing here, but most of it will be covered by the Hystrix test. Can we skip the coverage testing now?

RE guarantees: look at the call-site for runCallback in AdminFilter::onComplete, and you'll see that there has to be non-null request-headers there (there's a RELEASE_ASSERT). At that point you could create the AdminStream object on the stack with the non-null request-headers ref and pass it to runCallback, rather than 'this' (having cut the multiple-inheritance relationship).

It looks to me also that callbacks_ has to be non-null at that point as well, or the call to encodeHeaders will segv.

re code-coverage, ideally each level of abstraction should have its own covering unit tests, and not rely on some other complex system to test it. Is there some reason why this would be hard to test in this case?

Thanks for the idea of changing the pointer to const ref.
Just to be sure: the variables callbacks_ and request_headers_ are not initialized in the constructor (like parent_ variable). Therefore, they were defined as pointers and couldn’t be defined as const ref.
Do you suggest holding them in the class as pointers, and return them as const ref, after a null checking, in the getter functions (getDecoderFilterCallbacks and getRequestHeaders)?
Thanks in advance!

No, I suggest:

have AdminStream be a pure base class, not part of a multi-inheritance mesh.

have the callbacks & req-headers be const refs member vars of AdminStreamImpl

construct AdminStreamImpl in AdminFilters::onComplete where it's clear you have those
available non-null.

Re code-coverage: it will be hard to test the onDestroy() code with added callbacks. I could use addHandler() to add a new handler that adds a callback. The two relevant tests are admin_test and integration_admin_test. In admin_test I have an admin object but no requests, so onDestroy() is not called. In integration_admin_test onDestroy() is called but I don't have an admin object.
I may be missing a simple solution.. it looks to me that it is complicated to build such test, but hystrix_test should cover it, that's why I asked.

I understand there are not already existing tests, but I'm still not clear why it's difficult to make one.

I definitely think it's better to cover each level of abstraction on its own and not wait for its application with a higher level integration to be able to test it.

jmarantz

Sorry for the delay -- I wanted to give you an exact answer and needed to poke around on a larger screen to point you to the call-site.

jmarantz · 2018-04-26T12:42:22Z

include/envoy/server/admin.h

+   * @return Http::StreamDecoderFilterCallbacks* to be used by the handler to get socket for data
+   * streaming
+   */
+  virtual const Http::StreamDecoderFilterCallbacks* getDecoderFilterCallbacks() const PURE;


RE guarantees: look at the call-site for runCallback in AdminFilter::onComplete, and you'll see that there has to be non-null request-headers there (there's a RELEASE_ASSERT). At that point you could create the AdminStream object on the stack with the non-null request-headers ref and pass it to runCallback, rather than 'this' (having cut the multiple-inheritance relationship).

It looks to me also that callbacks_ has to be non-null at that point as well, or the call to encodeHeaders will segv.

jmarantz · 2018-04-26T12:51:46Z

include/envoy/server/admin.h

+   * @return Http::StreamDecoderFilterCallbacks* to be used by the handler to get socket for data
+   * streaming
+   */
+  virtual const Http::StreamDecoderFilterCallbacks* getDecoderFilterCallbacks() const PURE;


re code-coverage, ideally each level of abstraction should have its own covering unit tests, and not rely on some other complex system to test it. Is there some reason why this would be hard to test in this case?

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz · 2018-04-30T21:06:42Z

source/server/http/admin.h

+                  std::list<std::function<void()>>& on_destroy_callbacks)
+      : callbacks_(callbacks), request_headers_(request_headers),
+        on_destroy_callbacks_(on_destroy_callbacks){};
+  void setEndStreamOnComplete(const bool& end_stream) override {


no need for a const reference here, just take the bool by value.

jmarantz · 2018-04-30T21:08:56Z

test/integration/integration_admin_test.cc

+  auto callback = [](absl::string_view, Http::HeaderMap&, Buffer::Instance&,
+                     Server::AdminStream& admin_stream) -> Http::Code {
+    auto on_destroy_callback = []() {
+      // TODO (@trabetti) : any good idea what to do in this callback that can be checked?


set a bool declared in the test-method.

jmarantz · 2018-04-30T21:12:30Z

test/server/http/admin_test.cc

    admin_filter_.decodeHeaders(request_headers_, false);
-    return admin_.runCallback(path_and_query, response_headers, response, admin_filter_);
+    AdminStreamImpl admin_stream(callbacks, request_headers_, on_destroy_callbacks);


Hmmm...I'm wondering, when you integrate Hystrix, if this will be the right model. Will your callback to initiate the hystrix callback never return until the client closes the connection, so that the AdminStreamImpl has the right lifetime?

jmarantz · 2018-05-01T00:57:47Z

include/envoy/server/instance.h

+  /**
+   * @return the flush interval of stats sinks.
+   */
+  virtual std::chrono::milliseconds statsFlushInterval() PURE;


const method, here & the override

The implementation calls statsFlushInterval() of config, which is not a const ref, I should change there too, would that be ok?

envoy/include/envoy/server/configuration.h

Line 76 in ed468c9

virtual std::chrono::milliseconds statsFlushInterval() PURE;

Surely an oversight, looking at the only impl in source/server/configuration_impl.h and the similar functions declared above & below. Let's fix there and in the interface, rather than propagating the issue.

jmarantz · 2018-05-01T01:00:42Z

test/integration/integration_admin_test.cc

+//  EXPECT_STREQ("200", response->headers().Status()->value().c_str());
+//  EXPECT_EQ(spdlog::level::trace, Logger::Registry::getLog(Logger::Id::assert).level());
+//  // TODO (@trabetti) : what can be checked on response headers?
+//}


What's the story here?

tried to see if there's a way to test end_stream = false. but then the response never returns.

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz · 2018-05-02T12:16:58Z

include/envoy/server/admin.h

@@ -36,14 +70,18 @@ class Admin {

  /**
   * Callback for admin URL handlers.
-   * @param url supplies the URL prefix to install the handler for.
+   * @param path_and_query supplies the URL prefix to install the handler for.


I realize this was the existing condition, but I think this param description is not right. Something like "the path and query of the request URL"

jmarantz · 2018-05-02T12:22:07Z

include/envoy/server/instance.h

+  /**
+   * @return the flush interval of stats sinks.
+   */
+  virtual std::chrono::milliseconds statsFlushInterval() PURE;


Surely an oversight, looking at the only impl in source/server/configuration_impl.h and the similar functions declared above & below. Let's fix there and in the interface, rather than propagating the issue.

jmarantz

Nice job getting the coverage. Almost there. Just some nits and one optional suggestion around clarifying ownership.

jmarantz · 2018-05-02T12:22:58Z

include/envoy/server/configuration.h

@@ -73,7 +73,7 @@ class Main {
   * @return std::chrono::milliseconds the time interval between flushing to configured stat sinks.
   *         The server latches counters.
   */
-  virtual std::chrono::milliseconds statsFlushInterval() PURE;
+  virtual const std::chrono::milliseconds& statsFlushInterval() PURE;


why return a const ref here? Most of the other time-returning things I see in Envoy are returning by value.

jmarantz · 2018-05-02T12:23:29Z

source/server/config_validation/server.h

@@ -89,6 +89,10 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
  ThreadLocal::Instance& threadLocal() override { return thread_local_; }
  const LocalInfo::LocalInfo& localInfo() override { return *local_info_; }

+  const std::chrono::milliseconds& statsFlushInterval() override {


const method per discussion in outdated conversation. You will have to fix the underlying interface and impl.

not const ref return though.

Sure, I should have paid attention. will correct all these.

jmarantz · 2018-05-02T12:24:01Z

include/envoy/server/instance.h

+  /**
+   * @return the flush interval of stats sinks.
+   */
+  virtual const std::chrono::milliseconds& statsFlushInterval() PURE;


const method; not const ref return.

jmarantz · 2018-05-02T12:24:11Z

source/server/configuration_impl.h

@@ -127,7 +127,7 @@ class MainImpl : Logger::Loggable<Logger::Id::config>, public Main {
  Tracing::HttpTracer& httpTracer() override { return *http_tracer_; }
  RateLimit::ClientFactory& rateLimitClientFactory() override { return *ratelimit_client_factory_; }
  std::list<Stats::SinkPtr>& statsSinks() override { return stats_sinks_; }
-  std::chrono::milliseconds statsFlushInterval() override { return stats_flush_interval_; }
+  const std::chrono::milliseconds& statsFlushInterval() override { return stats_flush_interval_; }


const ref, not const ref return.

jmarantz · 2018-05-02T12:25:19Z

source/server/http/admin.h

+ */
+class AdminStreamImpl : public AdminStream {
+public:
+  AdminStreamImpl(Http::StreamDecoderFilterCallbacks& callbacks, Http::HeaderMap& request_headers,


It's worth commenting here that the ctor args are held by reference and must be held in a data structure that outlasts the stream. Another thought I had was to have the AdminFilter own the AdminStreamImpl as a std::unique_ptr, which would be the only location for these objects. That might be easier to reason about in terms of correctness. It would populate the AdminStreamImpl in the same place it does now.

ok, will look into it, I just need to check with the Hystrix implementation that it doesn't break anything

I'm not super opinionated about who needs to own the admin stream since either way it will be possible to be left with a dangling reference without explicitly destroying something IIUC (in once case it will be the stream, in the other it will be the callbacks). However, as @jmarantz suggests, in either case, please document how one would know that the reference is invalidated (using onDestryCallabacks()).

jmarantz · 2018-05-02T12:37:24Z

test/server/http/admin_test.cc

  };
+
+  // Add non removable handler.


it would be nicer to make separate testcases for these. You could generate actually make the callback be a static method of AdminInstanceTest to share it between the methods.

I thought it is better to cover all these setters/getters in one test.

Best practice is to make each individual test just test one thing. The tests will each be small if you factor out the callback generator into a static helper method.

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz

Just some nits & personal prefs. But I'll leave it up to you.

jmarantz · 2018-05-02T20:00:10Z

test/server/http/admin_test.cc

  };
+
+  // Add non removable handler.


Best practice is to make each individual test just test one thing. The tests will each be small if you factor out the callback generator into a static helper method.

jmarantz · 2018-05-02T20:01:16Z

test/server/http/admin_test.cc

+}
+
+TEST_P(AdminInstanceTest, RejectHandlerWithXss) {
+  auto callback = [](absl::string_view, Http::HeaderMap&, Buffer::Instance&,


You could factor this helper callback generator into a simple static test method, since it is used 3x (previously 2x)

mattklein123

Generally LGTM but some comments on comments.

mattklein123 · 2018-05-03T07:38:03Z

source/server/http/admin.h

@@ -252,6 +283,8 @@ class AdminFilter : public Http::StreamDecoderFilter, Logger::Loggable<Logger::I
  AdminImpl& parent_;
  Http::StreamDecoderFilterCallbacks* callbacks_{};
  Http::HeaderMap* request_headers_{};
+  std::unique_ptr<std::list<std::function<void()>>> on_destroy_callbacks_ =


nit: I don't think it's really worth optimizing this to create the list when needed, as this is admin code. I would just remove the unique_ptr here and always have the list.

Sure? Not sure it will have any use beside in the Hystrix handler.

I don't feel very strongly about it, but this makes the code harder to read and I don't think the slightly smaller CPU/memory use matters here.

mattklein123 · 2018-05-03T07:39:24Z

include/envoy/server/admin.h

+  virtual ~AdminStream() {}
+
+  /**
+   * @param end_stream set to false for streaming response. Default is true, which will close


"which will close connection on socket" is misleading and not necessarily true if the admin connection were kept alive, which is possible with the existing code. I would rephrase to make it clear that this is really about ending the response when the initial handler completes or something like that.

mattklein123 · 2018-05-03T07:39:50Z

include/envoy/server/admin.h

+
+  /**
+   * @param cb callback to be added to the list of callbacks invoked by onDestroy() when connection
+   * is dropped.


Same comment. Connection is not necessarily dropped. When the stream is closed is more accurate.

mattklein123 · 2018-05-03T07:40:16Z

include/envoy/server/admin.h

+  virtual void addOnDestroyCallback(std::function<void()> cb) PURE;
+
+  /**
+   * @return Http::StreamDecoderFilterCallbacks* to be used by the handler to get socket for data


We are not really getting socket data here. We are getting HTTP request data, etc. I would rephrase.

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz · 2018-05-06T21:10:25Z

include/envoy/server/admin.h

+   * @param cb callback to be added to the list of callbacks invoked by onDestroy() when connection
+   * is closed.
+   */
+  virtual void addOnDestroyCallback(std::function<void()> cb) PURE;


nit/taste: I feel like the 'On' here and the 'Callback' both add the same info, and it would be slightly nicer to call this 'addDestroyCallback', but I'm fine as it is too.

thanks for the comment. I prefer to keep the name since it references the onDestroy() method, where the callbacks are being used.

jmarantz · 2018-05-06T21:12:54Z

source/server/http/admin.h

+  bool endStreamOnComplete() const override { return end_stream_on_complete_; }
+
+private:
+  const Http::StreamDecoderFilterCallbacks& callbacks_;


comment here too about who owns this, if you prefer not to try to have AdminStream be the sole owner (with AdminFilter owning the AdminStream as a std::unique_ptr) as I suggested above.

Verified this against the intended hystrix implementation. working. pushing a fix.

Signed-off-by: trabetti <talis@il.ibm.com>

trabetti · 2018-05-10T05:21:48Z

@jmarantz @mrice32 What do you think, is it ready for merge?

jmarantz · 2018-05-10T18:46:43Z

source/server/http/admin.cc

-  Http::Code code = parent_.runCallback(path, *header_map, response, admin_stream);
+  std::unique_ptr<AdminStreamImpl> admin_stream =
+      std::make_unique<AdminStreamImpl>(*callbacks_, *request_headers_, on_destroy_callbacks_);
+  Http::Code code = parent_.runCallback(path, *header_map, response, *admin_stream);


I think my suggestion was not clear. I was suggesting you make this unique_ptr be a member variable of AdminFilter, and should own the headers, callbacks, and destroy-callbacks exclusively.

The existing of this as an independent class referencing member variables of AdminStream seemed perilous and I think that change makes the ownership semantics much clearer and easy to reason about, without affecting the actual flow of the logic.

WDYT?

I don't mind this change. But need to check if callbacks can be on a unique pointer, since it points to outside admin.

jmarantz · 2018-05-10T21:49:25Z

In general I feel that holding onto refs to member vars of other classes is hard to reason about. But it we were already doing that and your PR doesn't make it worse, then for callbacks it makes sense to leave then as a ref, but still doing as I suggested for the objects created in runCallback.

mrice32

Everything LGTM except I think the reference invalidation semantics need to be documented a bit more.

mrice32 · 2018-05-11T15:07:55Z

source/server/http/admin.h

+ */
+class AdminStreamImpl : public AdminStream {
+public:
+  AdminStreamImpl(Http::StreamDecoderFilterCallbacks& callbacks, Http::HeaderMap& request_headers,


I'm not super opinionated about who needs to own the admin stream since either way it will be possible to be left with a dangling reference without explicitly destroying something IIUC (in once case it will be the stream, in the other it will be the callbacks). However, as @jmarantz suggests, in either case, please document how one would know that the reference is invalidated (using onDestryCallabacks()).

trabetti · 2018-05-13T07:16:41Z

@jmarantz : I still think of the AdminStream as a compact way to pass parameters to and from the handlers. Making it a class member and moving members used by AdminFilter to be owned by the AdminStream seems to me more confusing.
@mrice32 : in a comment I would write that the callbacks are invalidated after onDestory() is called, so anyone using them, should add a callback function to onDestroyCallbacks(), to invalidate whatever their handler is doing. This is what we are doing in the hystrix code. Is this what you mean, or is there anything we can do in onDestroy() to handle this in the general case?

jmarantz · 2018-05-13T12:54:38Z

Any time you have a data structure that contains references or pointers to member variables owned by other data structures, it casts suspicion and doubt and forces the reader to look very carefully.

That is why I believe clean ownership semantics are better, and less confusing.

I also suggested in this PR to call this 'RequestContext' rather than 'AdminStream', and it might grow into a useful abstraction beyond this over time. In many other server architectures I've used in the past, a request-context is where this sort of data would live. But I don't have the breadth of knowledge of Envoy's structures yet to know how that would fit in more broadly.

However as I said initially when I made this suggestion, I'm also OK with commenting the lifetime of what each member variable is referring to is, particularly because as @mrice32 points out, at least one of the member variables would need lifetime beyond the AdminStream.

jmarantz · 2018-05-13T12:59:03Z

source/server/http/admin.cc

@@ -653,7 +663,11 @@ void AdminFilter::onComplete() {
  Buffer::OwnedImpl response;
  Http::HeaderMapPtr header_map{new Http::HeaderMapImpl};
  RELEASE_ASSERT(request_headers_);
-  Http::Code code = parent_.runCallback(path, *request_headers_, *header_map, response);
+
+  std::unique_ptr<AdminStreamImpl> admin_stream =


My suggestion to make this a unique_ptr rather than instantiate it on-stack only makes sense if you are going to hold the unique_ptr in the AdminStream, moving the ownership of header_map and response along with it.

If you are going to stay with the structure as is, you might as well just instantiate the AdminStream on the stack here. You are not releasing it or std::move()ing it either, so unique_ptr locally here doesn't have much impact.

Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123

Looks like on the right track, thanks for the detailed review. A couple of comments.

mattklein123 · 2018-05-13T21:10:56Z

include/envoy/server/admin.h

+  virtual void setEndStreamOnComplete(bool end_stream) PURE;
+
+  /**
+   * @param cb callback to be added to the list of callbacks invoked by onDestroy() when connection


I can't remember if I commented on this before, but this is not getting called with the connection is closed, it's getting called when the stream is closed. Can you update the comment?

mattklein123 · 2018-05-13T21:13:41Z

source/server/http/admin.cc

@@ -653,7 +663,10 @@ void AdminFilter::onComplete() {
  Buffer::OwnedImpl response;
  Http::HeaderMapPtr header_map{new Http::HeaderMapImpl};
  RELEASE_ASSERT(request_headers_);
-  Http::Code code = parent_.runCallback(path, *request_headers_, *header_map, response);
+
+  AdminStreamImpl admin_stream(*callbacks_, *request_headers_, on_destroy_callbacks_);


I'm sorry, I haven't been tracking this lengthy conversation, but why doesn't AdminFilter implement AdminStream and then you just pass *this? That is the correct ownership model anyway IMO and avoids all of the other concerns? cc @jmarantz

We started with that model and then changed it in response to feedback. If it makes more sense we can go back to that. @mattklein123 @jmarantz

I had one issue with that model which is that there are some lazy-initialized member vars, including request_headers_. AdminStreamImpl (in the current code) is only live when request_headers_ is non-null, but AdminFilter would satisfy an AdminStream interface all the time, even though some of the data it provides access to is sometimes null.

So I think ownership is clear with that design, but not lifetime.

I think the clearest model for ownership and lifetime is to have AdminStreamImpl own the request headers (and maybe call it "RequestContext" instead of "AdminStream"), and it can be instantiated in AdminFilter::onComplete and stored in a unique_ptr in the AdminFilter.

However, I think I'm done bikeshedding this now :) I'm also OK with having the AdminFilter inherit from AdminStream and comment in AdminStream return objects whose lifetime is bounded more tightly than the AdminStream object, or having AdminStreamImpl capture references to member variables in other structures.

Alright, I don't mean to rehash a huge conversation. Personally, I think AdminFilter implementing AdminStream is the clearest model, as the filter/stream is alive for the lifetime of the admin request, so that is what I would do, but if we want to start with this we can see how it goes. Though I don't see how this can be the final solution as being stack allocated, as for handlers that continue processing they will do so outside the context of onComplete, so something will need to change?

I don't know if it answers your question, but - In the Hystrix example, only the StreamDecoderFilterCallbacks is needed after onComplete is done with. It references to outside the AdminFilter. In Hystrix we keep pointer to it in the sink, so the pointer stay valid after the lifetime of onComplete(). The idea we agreed here is to handle this by adding methods to on_destroy_callbacks_ that will stop using the StreamDecoderFilterCallbacks pointer when onDestroy() is called. Adding this function should be done in the handler.

IMO this is all needless complexity. Each filter lives for the live of the stream, along with associated request headers, stream decoder filter callbacks, etc. The filter is exactly the lifetime of what you want, and it should implement the interface.

The StreamDecoderFilterCallbacks points to an object on ConnectionManagerImpl, and it is removed when the connection is closed (e.g. the hystrix dashboard is closed). Since for Hystrix we are doing the streaming using the sink timer, and we hold another pointer to the StreamDecoderFilterCallbacks , we must invalidate that pointer on the sink when onDestory() is called in the filter.

I've already commented on this several times, but there is nothing happening here at the granularity of the connection. Only streams, and the filter object has the lifetime of the stream. Trust me, you want the filter to implement the interface.

+1. Having the filter implement the interface should work perfectly for the Hystrix case. It will make the lifetime easier to understand, and it will incur the smallest code footprint.

I talked about this offline with @jmarantz. I think we are all on the same page to move to the inheritance model. Sorry for the runaround @trabetti but I think important learning/exploration for all involved.

jmarantz · 2018-05-13T21:15:17Z

source/server/http/admin.h

@@ -49,6 +49,9 @@ class AdminStreamImpl : public AdminStream {
  bool endStreamOnComplete() const override { return end_stream_on_complete_; }

 private:
+  // the callbacks member is a reference to a member of a ConnectionManagerImpl object. It is
+  // destroyed when connection is terminated. handlers using it should invalidate the reference by
+  // adding a method to be run when onDestroy() is invoked, using addOnDestroyCallback().
  const Http::StreamDecoderFilterCallbacks& callbacks_;
  const Http::HeaderMap& request_headers_;


similar comments are also needed for request_headers_ and on_destroy_callbacks_. Basically any time you have a reference or pointer as a member variable you need to comment on ownership & lifetime.

jmarantz

lgtm assuming you add comments about lifetime on member reference vars.

mrice32

Looks great! A few minor nits on the reference comment.

mrice32 · 2018-05-14T14:15:12Z

source/server/http/admin.h

+  bool endStreamOnComplete() const override { return end_stream_on_complete_; }
+
+private:
+  // the callbacks member is a reference to a member of a ConnectionManagerImpl object. It is


nit: The callbacks. Please capitalize the first letter of each sentence here and later in this comment.

mrice32 · 2018-05-14T14:19:07Z

source/server/http/admin.h

+
+private:
+  // the callbacks member is a reference to a member of a ConnectionManagerImpl object. It is
+  // destroyed when connection is terminated. handlers using it should invalidate the reference by


nit: maybe reword the second sentence to to:

Handlers relying on the reference should use addOnDestroyCallback() to add a callback that will notify them when the reference is no longer valid.

Signed-off-by: trabetti <talis@il.ibm.com>

trabetti · 2018-05-14T20:19:29Z

Just pushed a version with AdminFilter implementing AdminStream, @mattklein123 is this what you meant?

mattklein123 · 2018-05-14T20:26:13Z

@trabetti yes. Can you merge master and look at the remaining comments? Then I think we can get this merged.

trabetti · 2018-05-14T20:33:29Z

that would be great

jmarantz · 2018-05-14T22:38:57Z

Confirmed; I'm fine with going back to the multiple-inheritance model; but returning references to the objects that must not be nullptr, using the pattern

ASSERT(request_headers_.get() != nullptr);
return *request_headers_;

Signed-off-by: trabetti <talis@il.ibm.com>

…ure_merge_master Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123

LGTM, just 2 small comments nits. @trabetti sadly I'm having a hard time opening this PR now as it unicorns 50% of the time or so so with more comments we might have to close and open a new one, but let's see how it goes. @mrice32 @jmarantz any final comments?

mattklein123 · 2018-05-16T15:30:11Z

include/envoy/server/admin.h

+  virtual void addOnDestroyCallback(std::function<void()> cb) PURE;
+
+  /**
+   * @return Http::StreamDecoderFilterCallbacks* to be used by the handler to get HTTP request data


nit: Http::StreamDecoderFilterCallbacks&

mattklein123 · 2018-05-16T15:30:23Z

include/envoy/server/admin.h

+  virtual const Http::StreamDecoderFilterCallbacks& getDecoderFilterCallbacks() const PURE;
+
+  /**
+   * @return Http::HeaderMap* to be used by handler to parse header information sent with the


nit: Http::HeaderMap&

mrice32

LGTM!

trabetti · 2018-05-16T19:40:33Z

any idea what is this tsan failure?

Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123 · 2018-05-16T19:50:52Z

@trabetti unrelated. Fixed in master.

improve admin infrastructure

c5c37ab

Signed-off-by: trabetti <talis@il.ibm.com>

trabetti mentioned this pull request Apr 23, 2018

support for hystrix event stream output #2736

Closed

jmarantz approved these changes Apr 23, 2018

View reviewed changes

mrice32 reviewed Apr 23, 2018

View reviewed changes

mattklein123 reviewed Apr 23, 2018

View reviewed changes

mattklein123 assigned mrice32 and jmarantz Apr 23, 2018

review comments

b9c2f93

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed Apr 24, 2018

View reviewed changes

mattklein123 reviewed Apr 24, 2018

View reviewed changes

mrice32 approved these changes Apr 24, 2018

View reviewed changes

review comments #2

86b7700

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed Apr 24, 2018

View reviewed changes

jmarantz reviewed Apr 26, 2018

View reviewed changes

trabetti added 2 commits April 29, 2018 22:02

adding AdminStreamImpl

e40d95b

Signed-off-by: trabetti <talis@il.ibm.com>

testing

9d57013

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed May 1, 2018

View reviewed changes

trabetti added 2 commits May 1, 2018 17:05

improved testing + review comments

dd29bdb

Signed-off-by: trabetti <talis@il.ibm.com>

added AdminStream to admin_test. should be full coverage now.

20dc916

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed May 2, 2018

View reviewed changes

statsFlushInterval() to const method and not return const ref

8504569

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed May 2, 2018

View reviewed changes

jmarantz approved these changes May 2, 2018

View reviewed changes

mattklein123 reviewed May 3, 2018

View reviewed changes

eliranroffe approved these changes May 3, 2018

View reviewed changes

fix comments and removed pointer from on_destroy_callbacks_

98ed189

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed May 6, 2018

View reviewed changes

made admin_stream an std::unique_ptr

45099ff

Signed-off-by: trabetti <talis@il.ibm.com>

jmarantz reviewed May 10, 2018

View reviewed changes

mrice32 reviewed May 11, 2018

View reviewed changes

jmarantz reviewed May 13, 2018

View reviewed changes

undo AdminStream as unique pointer, add ownership comment to callbacks

3fe4c93

Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123 reviewed May 13, 2018

View reviewed changes

jmarantz reviewed May 13, 2018

View reviewed changes

jmarantz approved these changes May 13, 2018

View reviewed changes

mrice32 reviewed May 14, 2018

View reviewed changes

AdminFilter to implement AdminStream

f994804

Signed-off-by: trabetti <talis@il.ibm.com>

trabetti added 4 commits May 15, 2018 10:21

nullptr checks

52d7529

Signed-off-by: trabetti <talis@il.ibm.com>

update to master

a9df738

Signed-off-by: trabetti <talis@il.ibm.com>

Merge remote-tracking branch 'upstream/master' into admin_infrastruct…

e4c47d2

…ure_merge_master Signed-off-by: trabetti <talis@il.ibm.com>

Merge remote-tracking branch 'upstream/master' into admin_infrastruct…

34a9a33

…ure_merge_master Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123 reviewed May 16, 2018

View reviewed changes

mrice32 approved these changes May 16, 2018

View reviewed changes

fix comment

911c108

Signed-off-by: trabetti <talis@il.ibm.com>

mattklein123 approved these changes May 16, 2018

View reviewed changes

mattklein123 merged commit a54f6a8 into envoyproxy:master May 16, 2018

trabetti deleted the admin_infrastructure branch May 17, 2018 18:51

admin: improve infrastructure #3172

admin: improve infrastructure #3172

Uh oh!

Conversation

trabetti commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 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!

jmarantz 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!

mattklein123 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!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Apr 24, 2018 • 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!

trabetti Apr 25, 2018 • 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!

trabetti Apr 26, 2018 • 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!

eliranroffe Apr 26, 2018 • 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!

jmarantz 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!

trabetti commented Apr 23, 2018 •

edited

Loading

jmarantz Apr 24, 2018 •

edited

Loading

trabetti Apr 25, 2018 •

edited

Loading

trabetti Apr 26, 2018 •

edited

Loading

eliranroffe Apr 26, 2018 •

edited

Loading

trabetti May 1, 2018 •

edited

Loading