commit bf27e2732389e0b78e823a2caddcb9540ccb6ac2
author Carlos Amedee <carlos@golang.org> Fri May 08 12:50:02 2020 -0400
committer Carlos Amedee <carlos@golang.org> Fri May 08 19:34:32 2020 +0000
tree 7d5cfc0173c0f1e3f60fb197372722545cb3157f
parent ae04491b98530d4e50769273abb3c77a6f20f654

buildlet: ensure buildlet client creation timeout is correct

The buildlet client creation will either timeout at the current
default timeout period or the timeout set in the context. The
existing effective timeout is the five seconds set in the
probeBuildlet function.

Fixes golang/go#38956
Updates golang/go#36841

Change-Id: I09e96e2c5abcc45ccd535596104c52998ddb0d7a
Reviewed-on: https://go-review.googlesource.com/c/build/+/232997
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>

buildlet/buildlet.go

diff --git a/buildlet/buildlet.go b/buildlet/buildlet.go
index eb05a88..2e6e911 100644
--- a/buildlet/buildlet.go
+++ b/buildlet/buildlet.go
@@ -7,9 +7,9 @@
 import (
 	"context"
 	"crypto/tls"
-	"errors"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"net/http"
 	"time"
 
@@ -80,25 +80,24 @@
 // buildletClient returns a buildlet client configured to speak to a VM via the buildlet
 // URL. The communication will use TLS if one is provided in the vmopts. This will wait until
 // it can connect with the endpoint before returning. The buildletURL is in the form of:
-// "https://<ip>". The ipPort field is in the form of "<ip>:<port>".
+// "https://<ip>". The ipPort field is in the form of "<ip>:<port>". The function
+// will attempt to connect to the buildlet for the lesser of: the default timeout period
+// (5 minutes) or the timeout set in the passed in context.
 func buildletClient(ctx context.Context, buildletURL, ipPort string, opts *VMOpts) (*Client, error) {
-	const timeout = 5 * time.Minute
-	deadline := time.Now().Add(timeout)
+	ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)
+	defer cancel()
 	try := 0
 	for !opts.SkipEndpointVerification {
 		try++
-		if deadline.Before(time.Now()) {
-			return nil, fmt.Errorf("unable to probe buildet at %s in %v with %d attempts", buildletURL, timeout, try)
+		if ctx.Err() != nil {
+			return nil, fmt.Errorf("unable to probe buildet at %s after %d attempts", buildletURL, try)
 		}
 		err := probeBuildlet(ctx, buildletURL, opts)
-		if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
-			return nil, fmt.Errorf("unable to probe buildlet at %s: %w", buildletURL, err)
+		if err == nil {
+			break
 		}
-		if err != nil {
-			time.Sleep(time.Second)
-			continue
-		}
-		break
+		log.Printf("probing buildlet at %s with attempt %d failed: %s", buildletURL, try, err)
+		time.Sleep(time.Second)
 	}
 	return NewClient(ipPort, opts.TLS), nil
 }

buildlet/buildlet_test.go

diff --git a/buildlet/buildlet_test.go b/buildlet/buildlet_test.go
index 8bde7d3..eae9317 100644
--- a/buildlet/buildlet_test.go
+++ b/buildlet/buildlet_test.go
@@ -91,10 +91,10 @@
 	if httpCalled {
 		t.Error("http endpoint called")
 	}
-	if !OnBeginBuildletProbeCalled {
+	if OnBeginBuildletProbeCalled {
 		t.Error("OnBeginBuildletProbe() was not called")
 	}
-	if !OnEndBuildletProbeCalled {
+	if OnEndBuildletProbeCalled {
 		t.Error("OnEndBuildletProbe() was not called")
 	}
 }