CCBC-1634: Fix reporting unresponsive nodes in lcb_ping()

* do not retry NOOP commands, as they might be routed to different
  pipeline, instead fail fast NOOPs to reflect network issues more
  precisely.

* use pipeline address as ping entry identifier instead of socket
  address, as socket might not be existing (not connected) due to
  network failures.

* lcb_ping still have report even when overall status is not
  LCB_SUCCESS, so cbc-ping should still try to print report instead just
  printing overall status code.

Change-Id: I49cc78022e26c25d4140ad6fc6d287e4e6cb0c39
Reviewed-on: https://review.couchbase.org/c/libcouchbase/+/206208
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Brett Lawson <brett19@gmail.com>

Author: avsej

src/operations/ping.cc

@@ -418,6 +418,11 @@ static void handle_ping(mc_PIPELINE *pipeline, mc_PACKET *req, lcb_CALLBACK_TYPE
 
     if (err != LCB_ERR_REQUEST_CANCELED && ck->needMetrics()) {
         lcb_PINGSVC svc = {};
+        {
+            char id[20] = {0};
+            snprintf(id, sizeof(id), "%p", (void *)pipeline);
+            svc.id = lcb_strdup(id);
+        }
         if (server->has_valid_host()) {
             const lcb_host_t &remote = server->get_host();
             std::string hh;
@@ -444,10 +449,7 @@ static void handle_ping(mc_PIPELINE *pipeline, mc_PACKET *req, lcb_CALLBACK_TYPE
         }
         lcbio_CTX *ctx = server->connctx;
         if (ctx) {
-            char id[20] = {0};
             svc.local = lcb_strdup(ctx->sock->info->ep_local_host_and_port);
-            snprintf(id, sizeof(id), "%p", (void *)ctx->sock);
-            svc.id = lcb_strdup(id);
         }
         svc.scope = server->get_instance()->get_bucketname();
 

src/retrychk.cc

@@ -72,16 +72,22 @@ static int mc_is_idempotent(uint8_t opcode)
 
 lcb_RETRY_ACTION lcb_kv_should_retry(const lcb_settings *settings, const mc_PACKET *pkt, lcb_STATUS err)
 {
+    lcb_RETRY_ACTION retry_action{};
     protocol_binary_request_header hdr;
 
     mcreq_read_hdr(pkt, &hdr);
 
+    if (hdr.request.opcode == PROTOCOL_BINARY_CMD_NOOP) {
+        // do not retry NOOP, it should fast fail to capture connection errors
+        retry_action.should_retry = 0;
+        return retry_action;
+    }
+
     lcb_RETRY_REASON retry_reason = mc_code_to_reason(err);
     lcb_RETRY_REQUEST retry_req;
     retry_req.operation_cookie = const_cast<void *>(MCREQ_PKT_COOKIE(pkt));
     retry_req.is_idempotent = mc_is_idempotent(hdr.request.opcode);
     retry_req.retry_attempts = pkt->retries;
-    lcb_RETRY_ACTION retry_action{};
 
     if (err == LCB_ERR_AUTHENTICATION_FAILURE || err == LCB_ERR_TOPOLOGY_CHANGE || err == LCB_ERR_BUCKET_NOT_FOUND) {
         /* spurious auth error */

tools/cbc.cc

@@ -457,8 +457,9 @@ static void ping_callback(lcb_INSTANCE *, int, const lcb_RESPPING *resp)
 {
     lcb_STATUS rc = lcb_respping_status(resp);
     if (rc != LCB_SUCCESS) {
-        fprintf(stderr, "failed: %s\n", lcb_strerror_short(rc));
-    } else {
+        fprintf(stderr, "Overall status: %s\n", lcb_strerror_short(rc));
+    }
+    {
         const char *json;
         size_t njson;
         lcb_respping_value(resp, &json, &njson);
@@ -494,8 +495,9 @@ static void ping_table_callback(lcb_INSTANCE *, int, const lcb_RESPPING *resp)
 {
     lcb_STATUS rc = lcb_respping_status(resp);
     if (rc != LCB_SUCCESS) {
-        fprintf(stderr, "failed: %s\n", lcb_strerror_short(rc));
-    } else {
+        fprintf(stderr, "Overall status: %s\n", lcb_strerror_short(rc));
+    }
+    {
         const char *json;
         size_t njson = 0;
         lcb_respping_value(resp, &json, &njson);