Merge branch 'mk/remove-image-scaler-defaults' into 'master'

Set default max image scaler procs to num_cpu / 2

See merge request gitlab-org/gitlab-workhorse!635

changelogs/unreleased/mk-remove-image-scaler-defaults.yml

+---
+title: Default MaxScalerProcs to num_cores / 2
+merge_request: 635
+author:
+type: other

config.toml.example

@@ -13,5 +13,5 @@ URL = "unix:/home/git/gitlab/redis/redis.socket"
   azure_storage_access_key = "YOUR ACCOUNT KEY"
 
 [image_resizer]
-  max_scaler_procs = 100
+  max_scaler_procs = 4 # Recommendation: CPUs / 2
   max_filesize = 250000

internal/config/config.go

 package config
 
 import (
+	"math"
 	"net/url"
+	"runtime"
 	"strings"
 	"time"
 
@@ -105,14 +107,9 @@ type Config struct {
 	ImageResizerConfig       *ImageResizerConfig       `toml:"image_resizer"`
 }
 
-const (
-	DefaultImageResizerMaxScalerProcs = 100
-	DefaultImageResizerMaxFilesize    = 250 * 1000 // 250kB
-)
-
 var DefaultImageResizerConfig = &ImageResizerConfig{
-	MaxScalerProcs: DefaultImageResizerMaxScalerProcs,
-	MaxFilesize:    DefaultImageResizerMaxFilesize,
+	MaxScalerProcs: uint32(math.Max(2, float64(runtime.NumCPU())/2)),
+	MaxFilesize:    250 * 1000, // 250kB,
 }
 
 func LoadConfig(data string) (*Config, error) {

internal/config/config_test.go

@@ -21,14 +21,8 @@ func TestLoadEmptyConfig(t *testing.T) {
 	cfg, err := LoadConfig(config)
 	require.NoError(t, err)
 
-	expected := Config{
-		ImageResizerConfig: &ImageResizerConfig{
-			MaxScalerProcs: DefaultImageResizerMaxScalerProcs,
-			MaxFilesize:    DefaultImageResizerMaxFilesize,
-		},
-	}
-
-	require.Equal(t, expected, *cfg)
+	require.Equal(t, cfg.ImageResizerConfig.MaxFilesize, uint64(250000))
+	require.GreaterOrEqual(t, cfg.ImageResizerConfig.MaxScalerProcs, uint32(2))
 
 	require.Nil(t, cfg.ObjectStorageCredentials)
 	require.NoError(t, cfg.RegisterGoCloudURLOpeners())

internal/imageresizer/image_resizer.go

@@ -30,6 +30,7 @@ import (
 type Resizer struct {
 	config.Config
 	senddata.Prefix
+	numScalerProcs processCounter
 }
 
 type resizeParams struct {
@@ -42,8 +43,6 @@ type processCounter struct {
 	n int32
 }
 
-var numScalerProcs processCounter
-
 var envInjector = tracing.NewEnvInjector()
 
 // Images might be located remotely in object storage, in which case we need to stream
@@ -122,7 +121,7 @@ func init() {
 }
 
 func NewResizer(cfg config.Config) *Resizer {
-	return &Resizer{cfg, "send-scaled-img:"}
+	return &Resizer{Config: cfg, Prefix: "send-scaled-img:"}
 }
 
 // This Injecter forks into a dedicated scaler process to resize an image identified by path or URL
@@ -158,7 +157,7 @@ func (r *Resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 
 	// We first attempt to rescale the image; if this should fail for any reason, imageReader
 	// will point to the original image, i.e. we render it unchanged.
-	imageReader, resizeCmd, err := tryResizeImage(req, sourceImageReader, logger.Writer(), params, fileSize, r.Config.ImageResizerConfig)
+	imageReader, resizeCmd, err := r.tryResizeImage(req, sourceImageReader, logger.Writer(), params, fileSize, r.Config.ImageResizerConfig)
 	if err != nil {
 		// something failed, but we can still write out the original image, do don't return early
 		helper.LogErrorWithFields(req, err, *logFields(0))
@@ -228,24 +227,24 @@ func (r *Resizer) unpackParameters(paramsData string) (*resizeParams, error) {
 }
 
 // Attempts to rescale the given image data, or in case of errors, falls back to the original image.
-func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, params *resizeParams, fileSize int64, cfg *config.ImageResizerConfig) (io.Reader, *exec.Cmd, error) {
+func (r *Resizer) tryResizeImage(req *http.Request, reader io.Reader, errorWriter io.Writer, params *resizeParams, fileSize int64, cfg *config.ImageResizerConfig) (io.Reader, *exec.Cmd, error) {
 	if fileSize > int64(cfg.MaxFilesize) {
-		return r, nil, fmt.Errorf("ImageResizer: %db exceeds maximum file size of %db", fileSize, cfg.MaxFilesize)
+		return reader, nil, fmt.Errorf("ImageResizer: %db exceeds maximum file size of %db", fileSize, cfg.MaxFilesize)
 	}
 
-	if !numScalerProcs.tryIncrement(int32(cfg.MaxScalerProcs)) {
-		return r, nil, fmt.Errorf("ImageResizer: too many running scaler processes")
+	if !r.numScalerProcs.tryIncrement(int32(cfg.MaxScalerProcs)) {
+		return reader, nil, fmt.Errorf("ImageResizer: too many running scaler processes (%d / %d)", r.numScalerProcs.n, cfg.MaxScalerProcs)
 	}
 
 	ctx := req.Context()
 	go func() {
 		<-ctx.Done()
-		numScalerProcs.decrement()
+		r.numScalerProcs.decrement()
 	}()
 
-	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, r, errorWriter, params)
+	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, reader, errorWriter, params)
 	if err != nil {
-		return r, nil, fmt.Errorf("ImageResizer: failed forking into scaler process: %w", err)
+		return reader, nil, fmt.Errorf("ImageResizer: failed forking into scaler process: %w", err)
 	}
 	return resizedImageReader, resizeCmd, nil
 }

internal/imageresizer/image_resizer_test.go

@@ -17,8 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
 )
 
-var r = Resizer{}
-
 func TestMain(m *testing.M) {
 	if err := testhelper.BuildExecutables(); err != nil {
 		log.WithError(err).Fatal()
@@ -28,6 +26,7 @@ func TestMain(m *testing.M) {
 }
 
 func TestUnpackParametersReturnsParamsInstanceForValidInput(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 
 	outParams, err := r.unpackParameters(encodeParams(t, &inParams))
@@ -37,6 +36,7 @@ func TestUnpackParametersReturnsParamsInstanceForValidInput(t *testing.T) {
 }
 
 func TestUnpackParametersReturnsErrorWhenLocationBlank(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "", Width: 64, ContentType: "image/jpg"}
 
 	_, err := r.unpackParameters(encodeParams(t, &inParams))
@@ -45,6 +45,7 @@ func TestUnpackParametersReturnsErrorWhenLocationBlank(t *testing.T) {
 }
 
 func TestUnpackParametersReturnsErrorWhenContentTypeBlank(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: ""}
 
 	_, err := r.unpackParameters(encodeParams(t, &inParams))
@@ -53,12 +54,13 @@ func TestUnpackParametersReturnsErrorWhenContentTypeBlank(t *testing.T) {
 }
 
 func TestTryResizeImageSuccess(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 	inFile := testImage(t)
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(
+	reader, cmd, err := r.tryResizeImage(
 		req,
 		inFile,
 		os.Stderr,
@@ -74,12 +76,13 @@ func TestTryResizeImageSuccess(t *testing.T) {
 }
 
 func TestTryResizeImageSkipsResizeWhenSourceImageTooLarge(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 	inFile := testImage(t)
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(
+	reader, cmd, err := r.tryResizeImage(
 		req,
 		inFile,
 		os.Stderr,
@@ -94,12 +97,13 @@ func TestTryResizeImageSkipsResizeWhenSourceImageTooLarge(t *testing.T) {
 }
 
 func TestTryResizeImageFailsWhenContentTypeNotMatchingFileContents(t *testing.T) {
+	r := Resizer{}
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/jpeg"}
 	inFile := testImage(t) // this is a PNG file; the image scaler should fail fast in this case
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	_, cmd, err := tryResizeImage(
+	_, cmd, err := r.tryResizeImage(
 		req,
 		inFile,
 		os.Stderr,