Adding priority levels to Envoy endpoints (not yet used for LB) #2088

alyssawilk · 2017-11-21T16:54:12Z

Signed-off-by: Alyssa Wilk alyssar@chromium.org

Description:
Reworking the organization of endpoints from HostSets-with-localities to a tree with PrioritySets at the top and HostSets as nodes.
This allows the failover characteristics requested in #1929 as well as satisfying the defunct #1354
This will be followed up by a PR (or set of PRs) converting the load balancers over to use the new priority logic.

API Changes:
envoyproxy/data-plane-api#259

Risk Level: High
It's a pretty big refactor and there were some subtle gotchas (like stats initially updating before the backends)

Testing:
Current tests passing.
health checker unit tests verify all hosts are health checked across priority groups
outlier detection unit tests do the same for outlier detection.
upstream_impl_test.cc has unit testing basic priority set operations
load stats integration test extended for priorities. Verifies stats reporting insofar as there's now way to direct load yet, but the localities show up as expected.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2017-11-27T16:05:02Z

@htuch @mattklein123 can you take a look? If the basic approach is good I'll fix up tests and ping back

mattklein123 · 2017-11-27T16:06:03Z

Yes I will look this morning. Sorry for the delay.

mattklein123

@alyssawilk I didn't review in detail but did a quick skim. At an interface level this makes sense to me so I would say let's complete this PR. I think it's good to split the LB changes into another PR if that is possible since this is going to be complicated to do a full review on.

mattklein123 · 2017-11-27T17:09:11Z

source/common/upstream/eds.cc

@@ -42,7 +42,7 @@ EdsClusterImpl::EdsClusterImpl(const envoy::api::v2::Cluster& cluster, Runtime::
 void EdsClusterImpl::startPreInit() { subscription_->start({cluster_name_}, *this); }

 void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources) {
-  std::vector<HostSharedPtr> new_hosts;
+  std::vector<std::vector<HostSharedPtr>> new_hosts(4);  // Large enough to avoid most resizes.


Good use of https://github.com/abseil/abseil-cpp/blob/master/absl/container/inlined_vector.h ?

Fixed by changing around the memory model (which I needed to do in case there were priority levels with no updates)

mattklein123 · 2017-11-27T17:11:40Z

source/common/upstream/upstream_impl.cc

+  priority_set_.addMemberUpdateCb([this](uint32_t priority,
+                                         const std::vector<HostSharedPtr>& hosts_added,
+                                         const std::vector<HostSharedPtr>& hosts_removed) {
+    // FIXME(alyssawilk) talk to Matt about how to change stats in prod.


IMO it's probably better to just keep the current stats, and also have per-priority stats? (Perhaps only per-priority if there are > 1 priorities).

SG. I'll leave these as-is and we can add per-priority when/if folks want 'em

We may be able to get away with just having priority in LB reports, and I've added a TODO for that

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

Some early feedback.

htuch · 2017-11-28T21:10:16Z

include/envoy/upstream/upstream.h

@@ -119,19 +119,22 @@ class Host : virtual public HostDescription {
 typedef std::shared_ptr<const Host> HostConstSharedPtr;

 /**
- * Base host set interface. This is used both for clusters, as well as per thread/worker host sets
- * used during routing/forwarding.
+ * Base host set interface.  This contains all of the endpoints for a given LocalityLbEndpoints


Nit: the war on two spaces continues..

htuch · 2017-11-28T21:11:39Z

include/envoy/upstream/upstream.h

+  virtual void updateHosts(
+      std::shared_ptr<const std::vector<HostSharedPtr>> hosts,
+      std::shared_ptr<const std::vector<HostSharedPtr>> healthy_hosts,
+      std::shared_ptr<const std::vector<std::vector<HostSharedPtr>>> hosts_per_locality,


I think (?) some typedefs might makes this more readable.

htuch · 2017-11-28T21:13:12Z

include/envoy/upstream/upstream.h

+   *
+   * @param hosts supplies the (usually new) list of hosts in the host set.
+   * @param healthy hosts supplies the subset of hosts which are healthy.
+   * @param hosts_per_locality supplies the hosts subdivided by locality.


At a high level, without diving into implementation, it seems that it's redundant to track and supply both (healthy) hosts and (healthy) hosts per locality, one of these (presumably the latter) should be sufficient. I know this is an extant thing, just wanted to start a thread to discuss this with you and @mattklein123 .

Agree, but as it turns out we track locality inconsistently (I think the rule is only if there are multiple localities and one is local and it's configured?) so it's quite non-trivial to combine the two.

I wonder if the better move might be to always just have the "locality style" group and if we don't care to pick by locality it's just in one big group. I know the healthy host is used for some things (mainly size for determining if it's a global panic, but size could be latched) but I can't remember if the full host list is ever used when splitting up by locality.

Previously this was hidden inside the implementation and is split out like this for perf reasons (basically compute once and then use everywhere). @alyssawilk do we need to expose this at the interface layer? I agree with @htuch at the interface level it's not great.

It's that, or doing a bunch of casts, since the generic priority set impl is handing out containers of HostSets not HostSet Impls. And because they're complex types (it's super handy to iterate over the vector) I believe we can't just override.

If we prefer casts in upstream_impl.cc I'm happy to do that.

I haven't thought through all the details but if you think this is the way to go it's fine with me. I can take a more complete look through this tomorrow if that would help. Sorry for the delay.

htuch · 2017-11-28T21:14:32Z

include/envoy/upstream/upstream.h

+  /**
+   * @return the host sets for this priority.  If no such host set exists, it will be created.
+   */
+  virtual HostSet& getHostSet(uint32_t priority) PURE;


Do we ever remove hosts sets at a given priority?

Nope. I thought that was more of an implementation detail though so that's commented upstream_impl.h

htuch · 2017-11-28T21:14:57Z

include/envoy/upstream/upstream.h

+   * Install a callback that will be invoked when any of the HostSets in the PrioritySet changes.
+   * This includes when a new HostSet is created.
+   *
+   * @param callback supplies the callback to invoke.


Nit: document @return

htuch · 2017-11-28T21:15:10Z

include/envoy/upstream/upstream.h

+   * Returns the host sets for this priority set, ordered by priority.
+   * The first element in the vector is the host set for priority 0, and so on.
+   *
+   * @return the host sets for this priority set.


Nit: include return type in @return.

Done.
We're fairly inconsistent on this point and I think it makes it a lot uglier but shrug

htuch · 2017-11-28T21:22:03Z

source/common/upstream/eds.cc

@@ -59,20 +60,42 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources) {
                                     cluster_load_assignment.cluster_name()));
  }
  for (const auto& locality_lb_endpoint : cluster_load_assignment.endpoints()) {
+    uint32_t priority = locality_lb_endpoint.priority().value();


Nit: const

htuch · 2017-11-28T21:24:46Z

source/common/upstream/eds.cc

@@ -59,20 +60,42 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources) {
                                     cluster_load_assignment.cluster_name()));
  }
  for (const auto& locality_lb_endpoint : cluster_load_assignment.endpoints()) {
+    uint32_t priority = locality_lb_endpoint.priority().value();
+    if (new_hosts.size() <= priority) {
+      new_hosts.resize(priority + 1);


Would it make sense to make this structure std::map, i.e. ordered, to support sparse priority use? I'm thinking not, since probably folks will use dense priorities in practice, but throwing it out there.

Yeah as commented in the eds proto you can't skip. We don't have config validation so I just make sure the code works regardless but it doesn't make sense to do so reorganizing the data structures to optimize for it seems silly.

htuch · 2017-11-28T21:27:06Z

source/common/upstream/load_stats_reporter.cc

-        rq_success += host->stats().rq_success_.latch();
-        rq_error += host->stats().rq_error_.latch();
-        rq_active += host->stats().rq_active_.value();
+    for (auto& host_set : cluster.prioritySet().hostSetsPerPriority()) {


I think we should be merging across all priorities for a given locality here.

Really? I thought we should report stats per-priority (and I have a TODO for that somewhere).
If you prefer merging comment and I'll remove the TODO and merge.

As discussed IRL, it would provide greatest flexibility to do it per-priority as you plan. We should do something in the interim to avoid surprises here, since the code as implemented is just going to provide multiple reports when priorities are used for the same locality, with no way to distinguish them (other than implied ordering). I'd suggest merging for now and adding priorities later.

We could do that. Alternately if you don't object I can just only report load for priority group 0, and TODO to add the stats reporting before I fix the load balancers.

Without the load balancers priority-aware it's impossible to get load at other priority clusters, and that's a faster workaround until we do the API change and real load reporting (which is trivial)

Yep, priority 0 only and a TODO is fine for this PR, thanks.

htuch · 2017-11-28T22:11:20Z

source/common/upstream/subset_lb.h

@@ -71,7 +71,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable<Logger::Id::ups
    LoadBalancerPtr lb_;
  };

-  // Implements HostSet::MemberUpdateCb
+  // called by HostSet::MemberUpdateCb


Nit: s/called/Called/

htuch · 2017-11-28T22:12:21Z

source/common/upstream/subset_lb.h

@@ -94,7 +94,9 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable<Logger::Id::ups

  SubsetMetadata extractSubsetMetadata(const std::set<std::string>& subset_keys, const Host& host);

-  const HostSetImpl& emptyHostSet() { CONSTRUCT_ON_FIRST_USE(HostSetImpl); };
+  const HostSetImpl& emptyHostSet() {
+    CONSTRUCT_ON_FIRST_USE(HostSetImpl, original_host_set_.priority());


Maybe add a comment stating that the host set priority will never change, hence why this is valid to do.

Turns out this function isn't used

htuch · 2017-11-28T22:14:51Z

source/common/upstream/upstream_impl.cc

+  if (host_sets_.size() < priority + 1) {
+    for (size_t i = host_sets_.size(); i <= priority; ++i) {
+      host_sets_.push_back(HostSetPtr{new HostSetImpl(i)});
+      host_sets_[i]->addMemberUpdateCb([this](uint32_t priority,


This is another place it seems that std::map implementation might be cleaner, but not sure.

I think map will be uglier when we start selecting priority levels, because you're not guaranteed in-order priorities or existence. Vector with empty sets makes more sense to me.

Also one is not supposed to skip priorities, we just dont have config sanitization yet.

FWIW, I don't think it's broken to skip priorities, in the same sense that it's not broken to assign processes to different priority levels in a scheduler where there is no contiguity. If it simplifies things I guess it's fine.

proto validation will never do this type of checking for us. I would be fine having a check that all priorities need to be sequential and do this check in the EDS response. If we did this, we could also pre-create the host sets. I actually wonder if we really need the getHostSet() function which auto creates. It seems less error prone to just operate on the returned vector? Thoughts?

I believe we can get rid of getHostSet from the API pretty easily, so that's done.

At this point we could replace getHostSet with
if (getHostSetsPerPriority.size() < priority) { createHostSet(priority)} but that seems suboptimal :-P
I'll rename getOrCreateHostSet to make the create explicit. SG?

htuch · 2017-11-28T22:17:00Z

This looks rad overall.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

LGTM, the only non-trivial thing here on my end is to do the load report merge (or some other mitigation to avoid getting multiple reports for the same locality), plus @mattklein123 comment on interface.

htuch · 2017-11-29T22:48:34Z

source/common/upstream/load_stats_reporter.cc

-        rq_success += host->stats().rq_success_.latch();
-        rq_error += host->stats().rq_error_.latch();
-        rq_active += host->stats().rq_active_.value();
+    for (auto& host_set : cluster.prioritySet().hostSetsPerPriority()) {


As discussed IRL, it would provide greatest flexibility to do it per-priority as you plan. We should do something in the interim to avoid surprises here, since the code as implemented is just going to provide multiple reports when priorities are used for the same locality, with no way to distinguish them (other than implied ordering). I'd suggest merging for now and adding priorities later.

htuch · 2017-11-29T22:50:24Z

source/common/upstream/upstream_impl.cc

+  if (host_sets_.size() < priority + 1) {
+    for (size_t i = host_sets_.size(); i <= priority; ++i) {
+      host_sets_.push_back(HostSetPtr{new HostSetImpl(i)});
+      host_sets_[i]->addMemberUpdateCb([this](uint32_t priority,


FWIW, I don't think it's broken to skip priorities, in the same sense that it's not broken to assign processes to different priority levels in a scheduler where there is no contiguity. If it simplifies things I guess it's fine.

htuch · 2017-11-29T22:52:26Z

test/common/upstream/eds_test.cc

+
+  ASSERT_EQ(2, cluster_->prioritySet().hostSetsPerPriority().size());
+  ASSERT_EQ(2, cluster_->prioritySet().getHostSet(0).hosts().size());
+  ASSERT_EQ(1, cluster_->prioritySet().getHostSet(1).hosts().size());


EXPECT_EQ vs. ASSERT_EQ here?

htuch · 2017-11-29T22:52:44Z

test/common/upstream/eds_test.cc

+  ASSERT_EQ(4, cluster_->prioritySet().getHostSet(3).hosts().size());
+}
+
+TEST_F(EdsTest, PriorityAndLocality) {


Please describe what this test does in a one liner here.

htuch · 2017-11-29T22:53:18Z

test/common/upstream/eds_test.cc

+    EXPECT_EQ(2, second_hosts_per_locality[1].size());
+  }
+
+  add_hosts_to_locality_and_priority("oceania", "koala", "eucalyptus", 0, 3);


Please add comments as in the above TEST in the body of this one.

htuch · 2017-11-29T22:55:43Z

test/common/upstream/health_checker_impl_test.cc

+  respond(0, "200", false, true);
+  respond(1, "200", false, true);
+  EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthy());
+  EXPECT_TRUE(cluster_->prioritySet().getMockHostSet(1)->hosts_[0]->healthy());


These two tests are very similar, could refactor but up to you (if there was a 3rd copy I'd be more inclined to do it). Also, please add a comment line above each test (although it's fairly obvious here from the name).

htuch · 2017-11-29T22:57:11Z

test/common/upstream/upstream_impl_test.cc

@@ -638,6 +647,54 @@ TEST(StaticClusterImplTest, SourceAddressPriority) {
  }
 }

+TEST(PrioritySet, Extend) {


htuch · 2017-11-29T22:57:25Z

test/common/upstream/upstream_impl_test.cc

+  uint32_t changes = 0;
+  uint32_t last_priority = 0;
+  priority_set.addMemberUpdateCb([&](uint32_t priority, const std::vector<HostSharedPtr>&,
+                                     const std::vector<HostSharedPtr>&) -> void {


Nit: -> void is redundant.

mattklein123

I just looked through in more detail. In general this looks great. Beyond @htuch comments, my main question is whether we can simplify slightly and get rid of getHostSet(i) function?

mattklein123 · 2017-11-30T03:32:19Z

source/common/upstream/cluster_manager_impl.cc

@@ -264,11 +264,13 @@ ClusterManagerStats ClusterManagerImpl::generateStats(Stats::Scope& scope) {
 }

 void ClusterManagerImpl::postInitializeCluster(Cluster& cluster) {
-  if (cluster.hosts().empty()) {
-    return;
+  for (size_t i = 0; i < cluster.prioritySet().hostSetsPerPriority().size(); ++i) {


nit: Any reason to not just iterate directly through the returned vector here? Why do we need to get the size and then call get each time? (Same question if this happens elsewhere)

Oh yeah, now that host set latches its own priority I can rewrite these.

mattklein123 · 2017-11-30T03:36:59Z

source/common/upstream/upstream_impl.cc

+  if (host_sets_.size() < priority + 1) {
+    for (size_t i = host_sets_.size(); i <= priority; ++i) {
+      host_sets_.push_back(HostSetPtr{new HostSetImpl(i)});
+      host_sets_[i]->addMemberUpdateCb([this](uint32_t priority,


proto validation will never do this type of checking for us. I would be fine having a check that all priorities need to be sequential and do this check in the EDS response. If we did this, we could also pre-create the host sets. I actually wonder if we really need the getHostSet() function which auto creates. It seems less error prone to just operate on the returned vector? Thoughts?

mattklein123 · 2017-11-30T03:39:34Z

source/common/upstream/subset_lb.h

@@ -73,7 +73,7 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable<Logger::Id::ups
    LoadBalancerPtr lb_;
  };

-  // Implements HostSet::MemberUpdateCb
+  // Called by HostSet::MemberUpdateCb


nit: del "Called by" to match rest of code

That's the standard when we're implementing an interface, but is it when we're explaining the caller of a function?

sorry nevermind!

mattklein123 · 2017-11-30T03:42:05Z

source/common/upstream/upstream_impl.cc

+      info_->stats().membership_change_.inc();
+    }
+
+    uint32_t healthy_hosts = 0;


maybe add a TODO here for per-priority stats?

Sanity check: do you believe anyone wants them yet?
on more thought, I think we can get away with remotely reported load stats and I'd rather not add them unless someone wants them.

I can't imagine that anyone that actually uses priority wouldn't want broken out stats, but given that we won't be using this at Lyft right now we can wait until someone requests. :)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

LGTM. Thanks for the interface changes. I didn't review the tests, so will defer to @htuch or someone else for final approval.

mattklein123 · 2017-11-30T16:33:54Z

source/common/upstream/cluster_manager_impl.cc

@@ -602,43 +610,50 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::ClusterEntry(
                         parent.parent_.local_info_, parent.parent_, parent.parent_.runtime_,
                         parent.parent_.random_,
                         Router::ShadowWriterPtr{new Router::ShadowWriterImpl(parent.parent_)}) {
+
+  // TODO(alyssawilk) make lb priority-set aware in a follow-up patch


nit: period end of sentence.

mattklein123 · 2017-11-30T16:34:35Z

source/common/upstream/eds.cc

-  HostVectorSharedPtr current_hosts_copy(new std::vector<HostSharedPtr>(hosts()));
+  for (size_t i = 0; i < new_hosts.size(); ++i) {
+    if (new_hosts[i] != nullptr) {
+      UpdateHostsPerLocality(priority_set_.getOrCreateHostSet(i), *new_hosts[i]);


nit: updateHostsPerLocality

mattklein123 · 2017-11-30T16:36:40Z

source/common/upstream/upstream_impl.h

@@ -312,6 +347,9 @@ class ClusterImplBase : public Cluster,
                                 const LocalInfo::LocalInfo& local_info,
                                 Outlier::EventLoggerSharedPtr outlier_event_logger,
                                 bool added_via_api);
+  // From Cluster


nit: // Upstream::Cluster

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch · 2017-11-30T23:37:47Z

test/common/upstream/eds_test.cc

+  EXPECT_EQ(4, cluster_->prioritySet().hostSetsPerPriority()[3]->hosts().size());
+}
+
+// Set up an EDS config with multiple priorities and localitie and make sure


Nit: localities (not worth blocking merge).

alyssawilk · 2017-12-04T15:47:07Z

Thanks for the merge. Fixed over in in #2154

…

On Thu, Nov 30, 2017 at 3:44 PM, htuch ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In test/common/upstream/eds_test.cc <#2088 (comment)>: > + EXPECT_EQ(0, cluster_->prioritySet().hostSetsPerPriority()[2]->hosts().size()); + EXPECT_EQ(5, cluster_->prioritySet().hostSetsPerPriority()[3]->hosts().size()); + + // Update the number of hosts in priority #4. Make sure no other priority + // levels are affected. + cluster_load_assignment->clear_endpoints(); + add_hosts_to_priority(3, 4); + EXPECT_NO_THROW(cluster_->onConfigUpdate(resources)); + ASSERT_EQ(4, cluster_->prioritySet().hostSetsPerPriority().size()); + EXPECT_EQ(4, cluster_->prioritySet().hostSetsPerPriority()[0]->hosts().size()); + EXPECT_EQ(1, cluster_->prioritySet().hostSetsPerPriority()[1]->hosts().size()); + EXPECT_EQ(0, cluster_->prioritySet().hostSetsPerPriority()[2]->hosts().size()); + EXPECT_EQ(4, cluster_->prioritySet().hostSetsPerPriority()[3]->hosts().size()); +} + +// Set up an EDS config with multiple priorities and localitie and make sure Nit: localities (not worth blocking merge). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2088 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvdHvUanKYnuPO6tJGGeIiUq-jrQCks5s7z3DgaJpZM4QmKaQ> .

…roxy#2088) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Description: Adds public API and support for providing a list of domains to which H2 connections will be established *without* protocol negotiation (i.e. ALPN). See Envoy docs for the domain name patterns supported: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-route Risk Level: Low Testing: Unit Co-authored-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

alyssawilk added 2 commits November 21, 2017 11:48

Adding priority levels to most-of-Envoy (not yet used for LB)

c22f593

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

using priorities in EDS

db0b016

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 reviewed Nov 27, 2017

View reviewed changes

alyssawilk added 5 commits November 27, 2017 12:38

Merge branch 'master' into priorities

187884d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

test/... passing

661d296

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

fixing FIXMEs

aa4c85e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

EDS unit tests and cleanup

371c279

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

halfway done with unit tests

731fa47

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk changed the title ~~WIP: Adding priority levels to Envoy endpoints (not yet used for LB)~~ Adding priority levels to Envoy endpoints (not yet used for LB) Nov 28, 2017

integration tests

8ae3131

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the priorities branch from d121445 to 8ae3131 Compare November 28, 2017 19:18

Merge branch 'master' into priorities

ef1df21

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch reviewed Nov 28, 2017

View reviewed changes

alyssawilk added 2 commits November 29, 2017 09:17

reviewer comments

5b92074

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'refs/heads/master' into priorities

757c2ae

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch reviewed Nov 29, 2017

View reviewed changes

mattklein123 reviewed Nov 30, 2017

View reviewed changes

reviewer comments

3fbbb3b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 reviewed Nov 30, 2017

View reviewed changes

alyssawilk added 3 commits November 30, 2017 11:41

GoogleCamelCase strikes again!

afc3baa

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'refs/heads/master' into priorities

def4217

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

priority -> uint32, LB stats sent with priority.

a744afb

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch approved these changes Nov 30, 2017

View reviewed changes

htuch merged commit d7f07f1 into envoyproxy:master Nov 30, 2017

alyssawilk deleted the priorities branch December 13, 2017 20:54

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Run buildifier on the build files and add it to "make check". (envoyp…

54d5807

…roxy#2088) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Adding priority levels to Envoy endpoints (not yet used for LB) #2088

Adding priority levels to Envoy endpoints (not yet used for LB) #2088

Conversation

alyssawilk commented Nov 21, 2017 • edited Loading

alyssawilk commented Nov 27, 2017

mattklein123 commented Nov 27, 2017

mattklein123 left a 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

htuch left a 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

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

htuch commented Nov 28, 2017

htuch left a 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

mattklein123 left a 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

mattklein123 left a 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

alyssawilk commented Dec 4, 2017 via email

alyssawilk commented Nov 21, 2017 •

edited

Loading