Start of channelz resolution support

6 files changed, 63 insertions, 14 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 23da11363b..0324706170 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -518,6 +518,14 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
   }
   // Data used to set the channel's connectivity state.
   bool set_connectivity_state = true;
+  // We only want to trace the address resolution in the follow cases:
+  // (a) Address resolution resulted in service config change.
+  // (b) Address resolution that causes number of backends to go from
+  //     zero to non-zero.
+  // (c) Address resolution that causes number of backends to go from
+  //     non-zero to zero.
+  // (d) Address resolution that causes a new LB policy to be created.
+  bool trace_this_address_resolution = false;
   grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE;
   grpc_error* connectivity_error =
       GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy");
@@ -545,23 +553,56 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
         gpr_log(GPR_INFO, "chand=%p: updating existing LB policy \"%s\" (%p)",
                 chand, lb_policy_name.get(), chand->lb_policy.get());
       }
-      chand->lb_policy->UpdateLocked(*chand->resolver_result);
+      // case (b) or (c)
+      trace_this_address_resolution =
+          chand->lb_policy->UpdateLocked(*chand->resolver_result);
       // No need to set the channel's connectivity state; the existing
       // watch on the LB policy will take care of that.
       set_connectivity_state = false;
     } else {
+      trace_this_address_resolution = true;  // case (d)
       // Instantiate new LB policy.
       create_new_lb_policy_locked(chand, lb_policy_name.get(),
                                   &connectivity_state, &connectivity_error);
+      // we also log the name of the new LB policy in addition to logging this
+      // resolution event.
+      if (chand->channelz_channel != nullptr) {
+        char* str;
+        gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get());
+        chand->channelz_channel->AddTraceEvent(
+            grpc_core::channelz::ChannelTrace::Severity::Info,
+            grpc_slice_from_copied_string(str));
+        gpr_free(str);
+      }
     }
     // Find service config.
     grpc_core::UniquePtr<char> service_config_json =
         get_service_config_from_resolver_result_locked(chand);
+    if ((service_config_json == nullptr &&
+         chand->info_service_config_json != nullptr) ||
+        (service_config_json != nullptr &&
+         chand->info_service_config_json == nullptr) ||
+        (service_config_json != nullptr &&
+         chand->info_service_config_json != nullptr &&
+         gpr_stricmp(service_config_json.get(),
+                     chand->info_service_config_json.get()) != 0)) {
+      trace_this_address_resolution = true;  // case (a)
+      if (chand->channelz_channel != nullptr) {
+        chand->channelz_channel->AddTraceEvent(
+            grpc_core::channelz::ChannelTrace::Severity::Info,
+            grpc_slice_from_static_string("Service config reloaded"));
+      }
+    }
     // Swap out the data used by cc_get_channel_info().
     gpr_mu_lock(&chand->info_mu);
     chand->info_lb_policy_name = std::move(lb_policy_name);
     chand->info_service_config_json = std::move(service_config_json);
     gpr_mu_unlock(&chand->info_mu);
+    if (trace_this_address_resolution && chand->channelz_channel != nullptr) {
+      chand->channelz_channel->AddTraceEvent(
+          grpc_core::channelz::ChannelTrace::Severity::Info,
+          grpc_slice_from_static_string("New addresses resolved"));
+    }
     // Clean up.
     grpc_channel_args_destroy(chand->resolver_result);
     chand->resolver_result = nullptr;
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index 21f80b7b94..3940f138e6 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -95,7 +95,9 @@ class LoadBalancingPolicy
   /// Updates the policy with a new set of \a args from the resolver.
   /// Note that the LB policy gets the set of addresses from the
   /// GRPC_ARG_LB_ADDRESSES channel arg.
-  virtual void UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT;
+  /// Returns true if the update caused the number of backends to go from
+  ///  zero to non-zero, or non-zero to zero.
+  virtual bool UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT;
 
   /// Finds an appropriate subchannel for a call, based on data in \a pick.
   /// \a pick must remain alive until the pick is complete.
diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
index 5511df7a27..4b49d10d9e 100644
--- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
@@ -123,7 +123,7 @@ class GrpcLb : public LoadBalancingPolicy {
  public:
   GrpcLb(const grpc_lb_addresses* addresses, const Args& args);
 
-  void UpdateLocked(const grpc_channel_args& args) override;
+  bool UpdateLocked(const grpc_channel_args& args) override;
   bool PickLocked(PickState* pick, grpc_error** error) override;
   void CancelPickLocked(PickState* pick, grpc_error* error) override;
   void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask,
@@ -1331,7 +1331,7 @@ void GrpcLb::ProcessChannelArgsLocked(const grpc_channel_args& args) {
   grpc_channel_args_destroy(lb_channel_args);
 }
 
-void GrpcLb::UpdateLocked(const grpc_channel_args& args) {
+bool GrpcLb::UpdateLocked(const grpc_channel_args& args) {
   ProcessChannelArgsLocked(args);
   // If fallback is configured and the RR policy already exists, update
   // it with the new fallback addresses.
@@ -1358,6 +1358,7 @@ void GrpcLb::UpdateLocked(const grpc_channel_args& args) {
         &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_,
         nullptr);
   }
+  return false;  // TODO
 }
 
 //
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index f4dca146f7..e6f237c768 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -46,7 +46,7 @@ class PickFirst : public LoadBalancingPolicy {
  public:
   explicit PickFirst(const Args& args);
 
-  void UpdateLocked(const grpc_channel_args& args) override;
+  bool UpdateLocked(const grpc_channel_args& args) override;
   bool PickLocked(PickState* pick, grpc_error** error) override;
   void CancelPickLocked(PickState* pick, grpc_error* error) override;
   void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask,
@@ -333,7 +333,7 @@ void PickFirst::UpdateChildRefsLocked() {
   child_subchannels_ = std::move(cs);
 }
 
-void PickFirst::UpdateLocked(const grpc_channel_args& args) {
+bool PickFirst::UpdateLocked(const grpc_channel_args& args) {
   AutoChildRefsUpdater guard(this);
   const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
   if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
@@ -350,10 +350,12 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
               "ignoring.",
               this);
     }
-    return;
+    return false;
   }
   const grpc_lb_addresses* addresses =
       static_cast<const grpc_lb_addresses*>(arg->value.pointer.p);
+  size_t old_size =
+      subchannel_list_ == nullptr ? 0 : subchannel_list_->num_subchannels();
   if (grpc_lb_pick_first_trace.enabled()) {
     gpr_log(GPR_INFO,
             "Pick First %p received update with %" PRIuPTR " addresses", this,
@@ -371,7 +373,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
         "pf_update_empty");
     subchannel_list_ = std::move(subchannel_list);  // Empty list.
     selected_ = nullptr;
-    return;
+    return old_size != 0;
   }
   if (selected_ == nullptr) {
     // We don't yet have a selected subchannel, so replace the current
@@ -411,7 +413,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
           // not have contained the currently selected subchannel), drop
           // it, so that it doesn't override what we've done here.
           latest_pending_subchannel_list_.reset();
-          return;
+          return false;
         }
         GRPC_ERROR_UNREF(error);
       }
@@ -437,6 +439,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
           ->CheckConnectivityStateAndStartWatchingLocked();
     }
   }
+  return old_size == 0;
 }
 
 void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked(
diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
index e9ed85cf66..76d1d8b123 100644
--- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
@@ -57,7 +57,7 @@ class RoundRobin : public LoadBalancingPolicy {
  public:
   explicit RoundRobin(const Args& args);
 
-  void UpdateLocked(const grpc_channel_args& args) override;
+  bool UpdateLocked(const grpc_channel_args& args) override;
   bool PickLocked(PickState* pick, grpc_error** error) override;
   void CancelPickLocked(PickState* pick, grpc_error* error) override;
   void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask,
@@ -664,7 +664,7 @@ void RoundRobin::NotifyOnStateChangeLocked(grpc_connectivity_state* current,
                                                  notify);
 }
 
-void RoundRobin::UpdateLocked(const grpc_channel_args& args) {
+bool RoundRobin::UpdateLocked(const grpc_channel_args& args) {
   const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
   AutoChildRefsUpdater guard(this);
   if (GPR_UNLIKELY(arg == nullptr || arg->type != GRPC_ARG_POINTER)) {
@@ -677,7 +677,7 @@ void RoundRobin::UpdateLocked(const grpc_channel_args& args) {
           GRPC_ERROR_CREATE_FROM_STATIC_STRING("Missing update in args"),
           "rr_update_missing");
     }
-    return;
+    return false;  // TODO
   }
   grpc_lb_addresses* addresses =
       static_cast<grpc_lb_addresses*>(arg->value.pointer.p);
@@ -711,6 +711,7 @@ void RoundRobin::UpdateLocked(const grpc_channel_args& args) {
     // If we've started picking, start watching the new list.
     latest_pending_subchannel_list_->StartWatchingLocked();
   }
+  return false;  // todo
 }
 
 //
diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
index e6c94167f6..aee33b085c 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
@@ -123,7 +123,7 @@ class XdsLb : public LoadBalancingPolicy {
  public:
   XdsLb(const grpc_lb_addresses* addresses, const Args& args);
 
-  void UpdateLocked(const grpc_channel_args& args) override;
+  bool UpdateLocked(const grpc_channel_args& args) override;
   bool PickLocked(PickState* pick, grpc_error** error) override;
   void CancelPickLocked(PickState* pick, grpc_error* error) override;
   void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask,
@@ -1325,7 +1325,7 @@ void XdsLb::ProcessChannelArgsLocked(const grpc_channel_args& args) {
   grpc_channel_args_destroy(lb_channel_args);
 }
 
-void XdsLb::UpdateLocked(const grpc_channel_args& args) {
+bool XdsLb::UpdateLocked(const grpc_channel_args& args) {
   ProcessChannelArgsLocked(args);
   // If fallback is configured and the RR policy already exists, update
   // it with the new fallback addresses.
@@ -1352,6 +1352,7 @@ void XdsLb::UpdateLocked(const grpc_channel_args& args) {
         &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_,
         nullptr);
   }
+  return false;  // TODO
 }
 
 //