Improve handling of old/slow-to-start remote renderer (#40492)

* Assume the remote renderer is old if it returns 404 to the version endpoint

* Retry fetch of remote image renderer version on failure

Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>

pkg/services/rendering/http_mode.go

@@ -28,6 +28,11 @@ var netClient = &http.Client{
 	Transport: netTransport,
 }
 
+var (
+	remoteVersionFetchInterval time.Duration = time.Second * 15
+	remoteVersionFetchRetries  uint          = 4
+)
+
 func (rs *RenderingService) renderViaHTTP(ctx context.Context, renderKey string, opts Opts) (*RenderResult, error) {
 	filePath, err := rs.getNewFilePath(RenderPNG)
 	if err != nil {
@@ -194,6 +199,24 @@ func (rs *RenderingService) readFileResponse(ctx context.Context, resp *http.Res
 	return nil
 }
 
+func (rs *RenderingService) getRemotePluginVersionWithRetry(callback func(string, error)) {
+	go func() {
+		var err error
+		for try := uint(0); try < remoteVersionFetchRetries; try++ {
+			version, err := rs.getRemotePluginVersion()
+			if err == nil {
+				callback(version, err)
+				return
+			}
+			rs.log.Info("Couldn't get remote renderer version, retrying", "err", err, "try", try)
+
+			time.Sleep(remoteVersionFetchInterval)
+		}
+
+		callback("", err)
+	}()
+}
+
 func (rs *RenderingService) getRemotePluginVersion() (string, error) {
 	rendererURL, err := url.Parse(rs.Cfg.RendererUrl + "/version")
 	if err != nil {
@@ -212,7 +235,10 @@ func (rs *RenderingService) getRemotePluginVersion() (string, error) {
 		}
 	}()
 
-	if resp.StatusCode != http.StatusOK {
+	if resp.StatusCode == http.StatusNotFound {
+		// Old versions of the renderer lacked the version endpoint
+		return "1.0.0", nil
+	} else if resp.StatusCode != http.StatusOK {
 		return "", fmt.Errorf("remote rendering request to get version failed, status code: %d, status: %s", resp.StatusCode,
 			resp.Status)
 	}

pkg/services/rendering/rendering.go

@@ -10,6 +10,7 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"time"
 
@@ -43,6 +44,7 @@ type RenderingService struct {
 	domain          string
 	inProgressCount int32
 	version         string
+	versionMutex    sync.RWMutex
 
 	Cfg                   *setting.Cfg
 	RemoteCacheService    *remotecache.RemoteCache
@@ -93,13 +95,18 @@ func (rs *RenderingService) Run(ctx context.Context) error {
 	if rs.remoteAvailable() {
 		rs.log = rs.log.New("renderer", "http")
 
-		version, err := rs.getRemotePluginVersion()
-		if err != nil {
-			rs.log.Info("Couldn't get remote renderer version", "err", err)
-		}
+		rs.getRemotePluginVersionWithRetry(func(version string, err error) {
+			if err != nil {
+				rs.log.Info("Couldn't get remote renderer version", "err", err)
+			}
 
-		rs.log.Info("Backend rendering via external http server", "version", version)
-		rs.version = version
+			rs.log.Info("Backend rendering via external http server", "version", version)
+
+			rs.versionMutex.Lock()
+			defer rs.versionMutex.Unlock()
+
+			rs.version = version
+		})
 		rs.renderAction = rs.renderViaHTTP
 		rs.renderCSVAction = rs.renderCSVViaHTTP
 		<-ctx.Done()
@@ -153,6 +160,9 @@ func (rs *RenderingService) IsAvailable() bool {
 }
 
 func (rs *RenderingService) Version() string {
+	rs.versionMutex.RLock()
+	defer rs.versionMutex.RUnlock()
+
 	return rs.version
 }
 

pkg/services/rendering/rendering_test.go

@@ -3,9 +3,13 @@ package rendering
 import (
 	"context"
 	"errors"
+	"net/http"
+	"net/http/httptest"
 	"path/filepath"
 	"testing"
+	"time"
 
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -139,3 +143,68 @@ func TestRenderLimitImage(t *testing.T) {
 		})
 	}
 }
+
+func TestRenderingServiceGetRemotePluginVersion(t *testing.T) {
+	cfg := setting.NewCfg()
+	rs := &RenderingService{
+		Cfg: cfg,
+		log: log.New("rendering-test"),
+	}
+
+	t.Run("When renderer responds with correct version should return that version", func(t *testing.T) {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			_, err := w.Write([]byte("{\"version\":\"2.7.1828\"}"))
+			require.NoError(t, err)
+		}))
+		defer server.Close()
+
+		rs.Cfg.RendererUrl = server.URL + "/render"
+		version, err := rs.getRemotePluginVersion()
+
+		require.NoError(t, err)
+		require.Equal(t, "2.7.1828", version)
+	})
+
+	t.Run("When renderer responds with 404 should assume a valid but old version", func(t *testing.T) {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusNotFound)
+		}))
+		defer server.Close()
+
+		rs.Cfg.RendererUrl = server.URL + "/render"
+		version, err := rs.getRemotePluginVersion()
+
+		require.NoError(t, err)
+		require.Equal(t, version, "1.0.0")
+	})
+
+	t.Run("When renderer responds with 500 should retry until success", func(t *testing.T) {
+		tries := uint(0)
+		ctx, cancel := context.WithCancel(context.Background())
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			tries++
+
+			if tries < remoteVersionFetchRetries {
+				w.WriteHeader(http.StatusInternalServerError)
+			} else {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				_, err := w.Write([]byte("{\"version\":\"3.1.4159\"}"))
+				require.NoError(t, err)
+				cancel()
+			}
+		}))
+		defer server.Close()
+
+		rs.Cfg.RendererUrl = server.URL + "/render"
+		remoteVersionFetchInterval = time.Millisecond
+		remoteVersionFetchRetries = 5
+		go func() {
+			require.NoError(t, rs.Run(ctx))
+		}()
+
+		require.Eventually(t, func() bool { return rs.Version() == "3.1.4159" }, time.Second, time.Millisecond)
+	})
+}