From a4cbbe10c0d36a89334707f4f3f8e4526391ade6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Thu, 20 Nov 2025 14:55:45 +0100 Subject: [PATCH] Provisioning: Add retry logic for nanogit client operations (#114216) * chore(deps): update nanogit to v0.3.0 in go.mod and go.sum files * Add retry logic for nanogit client operations - Configure retry logic in withGitContext to ensure all Git operations have retry support - Use nanogit's ExponentialBackoffRetrier with 8 attempts (~10s retry window) - Retry transient network errors and HTTP-specific server errors (5xx for GET/DELETE, 429 for all) - Rename logger function to withGitContext to better reflect its responsibilities * fix: resolve staticcheck S1008 linting issue in retry_client.go Simplify return statement to use errors.As directly instead of if-return pattern * Revert "fix: resolve staticcheck S1008 linting issue in retry_client.go" This reverts commit bd367b5629bd49d9d15db708352b9ed1805376ab. --- apps/provisioning/go.mod | 2 +- apps/provisioning/go.sum | 4 +- .../pkg/repository/git/repository.go | 80 ++++++++++++++++--- .../pkg/repository/git/repository_test.go | 10 +-- go.mod | 2 +- go.sum | 4 +- go.work.sum | 4 +- 7 files changed, 81 insertions(+), 25 deletions(-) diff --git a/apps/provisioning/go.mod b/apps/provisioning/go.mod index 9bb12d5ae4f..a72aa77fe68 100644 --- a/apps/provisioning/go.mod +++ b/apps/provisioning/go.mod @@ -10,7 +10,7 @@ require ( github.com/grafana/grafana-app-sdk/logging v0.48.1 github.com/grafana/grafana/apps/secret v0.0.0-20250902093454-b56b7add012f github.com/grafana/grafana/pkg/apimachinery v0.0.0-20250804150913-990f1c69ecc2 - github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b + github.com/grafana/nanogit v0.3.0 github.com/migueleliasweb/go-github-mock v1.1.0 github.com/stretchr/testify v1.11.1 golang.org/x/oauth2 v0.33.0 diff --git a/apps/provisioning/go.sum b/apps/provisioning/go.sum index 78f06781b15..92a4d53c282 100644 --- a/apps/provisioning/go.sum +++ b/apps/provisioning/go.sum @@ -70,8 +70,8 @@ github.com/grafana/grafana/apps/secret v0.0.0-20250902093454-b56b7add012f h1:f+Z github.com/grafana/grafana/apps/secret v0.0.0-20250902093454-b56b7add012f/go.mod h1:RA8mP8KVIwKXBx3Ssqa/uEBABib5LvUWYPVMxrNvnP0= github.com/grafana/grafana/pkg/apimachinery v0.0.0-20250804150913-990f1c69ecc2 h1:X0cnaFdR+iz+sDSuoZmkryFSjOirchHe2MdKSRwBWgM= github.com/grafana/grafana/pkg/apimachinery v0.0.0-20250804150913-990f1c69ecc2/go.mod h1:RRvSjHH12/PnQaXraMO65jUhVu8n59mzvhfIMBETnV4= -github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b h1:rFjoqJFb2KxJ29K9ltuWRSsdA46SbN0GCxoQc36h5kg= -github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= +github.com/grafana/nanogit v0.3.0 h1:XNEef+4Vi+465ZITJs/g/xgnDRJbWhhJ7iQrAnWZ0oQ= +github.com/grafana/nanogit v0.3.0/go.mod h1:6s6CCTpyMOHPpcUZaLGI+rgBEKdmxVbhqSGgCK13j7Y= github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY= github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= diff --git a/apps/provisioning/pkg/repository/git/repository.go b/apps/provisioning/pkg/repository/git/repository.go index 4d002e4f831..82d5ca6bd1d 100644 --- a/apps/provisioning/pkg/repository/git/repository.go +++ b/apps/provisioning/pkg/repository/git/repository.go @@ -25,6 +25,7 @@ import ( "github.com/grafana/nanogit/options" "github.com/grafana/nanogit/protocol" "github.com/grafana/nanogit/protocol/hash" + "github.com/grafana/nanogit/retry" ) type RepositoryConfig struct { @@ -144,7 +145,7 @@ func isValidGitURL(gitURL string) bool { // Test implements provisioning.Repository. func (r *gitRepository) Test(ctx context.Context) (*provisioning.TestResults, error) { - ctx, _ = r.logger(ctx, "") + ctx, _ = r.withGitContext(ctx, "") t := string(r.config.Spec.Type) @@ -219,7 +220,7 @@ func (r *gitRepository) Test(ctx context.Context) (*provisioning.TestResults, er // Read implements provisioning.Repository. func (r *gitRepository) Read(ctx context.Context, filePath, ref string) (*repository.FileInfo, error) { - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) finalPath := safepath.Join(r.gitConfig.Path, filePath) // Resolve ref to commit hash @@ -271,7 +272,7 @@ func (r *gitRepository) Read(ctx context.Context, filePath, ref string) (*reposi } func (r *gitRepository) ReadTree(ctx context.Context, ref string) ([]repository.FileTreeEntry, error) { - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) // Resolve ref to commit hash refHash, err := r.resolveRefToHash(ctx, ref) @@ -319,7 +320,7 @@ func (r *gitRepository) Create(ctx context.Context, path, ref string, data []byt if ref == "" { ref = r.gitConfig.Branch } - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) branchRef, err := r.ensureBranchExists(ctx, ref) if err != nil { return err @@ -364,7 +365,7 @@ func (r *gitRepository) Update(ctx context.Context, path, ref string, data []byt if ref == "" { ref = r.gitConfig.Branch } - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) // Check if trying to update a directory if safepath.IsDir(path) { @@ -411,7 +412,7 @@ func (r *gitRepository) Write(ctx context.Context, path string, ref string, data ref = r.gitConfig.Branch } - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) info, err := r.Read(ctx, path, ref) if err != nil && !(errors.Is(err, repository.ErrFileNotFound)) { return fmt.Errorf("check if file exists before writing: %w", err) @@ -431,7 +432,7 @@ func (r *gitRepository) Delete(ctx context.Context, path, ref, comment string) e if ref == "" { ref = r.gitConfig.Branch } - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) branchRef, err := r.ensureBranchExists(ctx, ref) if err != nil { @@ -454,7 +455,7 @@ func (r *gitRepository) Move(ctx context.Context, oldPath, newPath, ref, comment if ref == "" { ref = r.gitConfig.Branch } - ctx, _ = r.logger(ctx, ref) + ctx, _ = r.withGitContext(ctx, ref) branchRef, err := r.ensureBranchExists(ctx, ref) if err != nil { @@ -545,6 +546,7 @@ func (r *gitRepository) History(_ context.Context, _ string, _ string) ([]provis } func (r *gitRepository) ListRefs(ctx context.Context) ([]provisioning.RefItem, error) { + ctx, _ = r.withGitContext(ctx, "") refs, err := r.client.ListRefs(ctx) if err != nil { return nil, fmt.Errorf("list refs: %w", err) @@ -566,7 +568,7 @@ func (r *gitRepository) ListRefs(ctx context.Context) ([]provisioning.RefItem, e } func (r *gitRepository) LatestRef(ctx context.Context) (string, error) { - ctx, _ = r.logger(ctx, "") + ctx, _ = r.withGitContext(ctx, "") branchRef, err := r.client.GetRef(ctx, fmt.Sprintf("refs/heads/%s", r.gitConfig.Branch)) if err != nil { return "", fmt.Errorf("get branch ref: %w", err) @@ -583,7 +585,7 @@ func (r *gitRepository) CompareFiles(ctx context.Context, base, ref string) ([]r return nil, fmt.Errorf("ref cannot be empty") } - ctx, logger := r.logger(ctx, ref) + ctx, logger := r.withGitContext(ctx, ref) // Resolve base ref to hash var baseHash hash.Hash @@ -671,11 +673,15 @@ func (r *gitRepository) CompareFiles(ctx context.Context, base, ref string) ([]r } func (r *gitRepository) Stage(ctx context.Context, opts repository.StageOptions) (repository.StagedRepository, error) { + ctx = ensureRetryContext(ctx) + ctx, _ = r.withGitContext(ctx, "") return NewStagedGitRepository(ctx, r, opts) } // resolveRefToHash resolves a ref (branch name or commit hash) to a commit hash func (r *gitRepository) resolveRefToHash(ctx context.Context, ref string) (hash.Hash, error) { + ctx, _ = r.withGitContext(ctx, ref) + // Use default branch if ref is empty if ref == "" { ref = r.gitConfig.Branch @@ -706,6 +712,8 @@ func (r *gitRepository) resolveRefToHash(ctx context.Context, ref string) (hash. // ensureBranchExists checks if a branch exists and creates it if it doesn't, // returning the branch reference to avoid duplicate GetRef calls func (r *gitRepository) ensureBranchExists(ctx context.Context, branchName string) (nanogit.Ref, error) { + ctx, _ = r.withGitContext(ctx, branchName) + if !IsValidGitBranchName(branchName) { return nanogit.Ref{}, &apierrors.StatusError{ ErrStatus: metav1.Status{ @@ -802,7 +810,57 @@ func (r *gitRepository) commitAndPush(ctx context.Context, writer nanogit.Staged return nil } -func (r *gitRepository) logger(ctx context.Context, ref string) (context.Context, logging.Logger) { +// defaultGitRetrier returns a default retrier configuration for Git operations. +// +// Retry attempts will happen when: +// - Network errors occur: connection timeouts, temporary network failures, or connection errors +// - HTTP 5xx server errors: For GET and DELETE operations (idempotent) +// - HTTP 429 Too Many Requests: For all operations (rate limiting is temporary) +// +// The retry behavior: +// - Total attempts: 8 (1 initial attempt + 7 retries) +// - Initial delay: 100ms before the first retry +// - Exponential backoff: delay doubles after each failed attempt (100ms → 200ms → 400ms → 800ms → 1.6s → 3.2s → 5s) +// - Maximum delay: capped at 5 seconds +// - Jitter: enabled to prevent thundering herd problems +// - Total retry window: approximately 10 seconds from first attempt to last retry +// +// All attempts will fail when: +// - The Git server is completely unavailable or unreachable +// - Network connectivity issues persist beyond the retry window (~10 seconds) +// - The server returns transient errors consistently for the entire retry duration +// - Context cancellation occurs before retries complete +// +// Non-transient errors (e.g., 4xx client errors except 429, authentication failures) are not retried and returned immediately. +func defaultGitRetrier() *retry.ExponentialBackoffRetrier { + return retry.NewExponentialBackoffRetrier(). + WithMaxAttempts(8). // 1 initial + 7 retries = 8 total attempts (~10s total retry window) + WithInitialDelay(100 * time.Millisecond). + WithMaxDelay(5 * time.Second). + WithMultiplier(2.0). + WithJitter() +} + +// ensureRetryContext ensures that retry logic is configured in the context. +// This function should be called at the beginning of all methods that make client calls +// to guarantee retry logic is always present, regardless of context state. +func ensureRetryContext(ctx context.Context) context.Context { + // Only add retrier if one doesn't already exist in the context + if retry.FromContext(ctx).MaxAttempts() <= 1 { + ctx = retry.ToContext(ctx, defaultGitRetrier()) + } + return ctx +} + +// withGitContext sets up the context with logging, git repository metadata, and retry logic. +// This function should be called at the beginning of all public methods to ensure: +// - Proper logging context with git repository details +// - Retry logic is configured for all Git operations +// - Context is properly prepared for nanogit client calls +func (r *gitRepository) withGitContext(ctx context.Context, ref string) (context.Context, logging.Logger) { + // Ensure retry logic is configured first, before any early returns + ctx = ensureRetryContext(ctx) + logger := logging.FromContext(ctx) type containsGit int diff --git a/apps/provisioning/pkg/repository/git/repository_test.go b/apps/provisioning/pkg/repository/git/repository_test.go index d3946b7b6d4..4958ab372ab 100644 --- a/apps/provisioning/pkg/repository/git/repository_test.go +++ b/apps/provisioning/pkg/repository/git/repository_test.go @@ -2163,7 +2163,7 @@ func TestGitRepository_commitAndPush(t *testing.T) { } } -func TestGitRepository_logger(t *testing.T) { +func TestGitRepository_withGitContext(t *testing.T) { gitRepo := &gitRepository{ config: &provisioning.Repository{ Spec: provisioning.RepositorySpec{ @@ -2179,7 +2179,7 @@ func TestGitRepository_logger(t *testing.T) { t.Run("creates new logger context", func(t *testing.T) { ctx := context.Background() - newCtx, logger := gitRepo.logger(ctx, "feature-branch") + newCtx, logger := gitRepo.withGitContext(ctx, "feature-branch") require.NotNil(t, newCtx) require.NotNil(t, logger) @@ -2188,7 +2188,7 @@ func TestGitRepository_logger(t *testing.T) { t.Run("uses default branch when ref is empty", func(t *testing.T) { ctx := context.Background() - newCtx, logger := gitRepo.logger(ctx, "") + newCtx, logger := gitRepo.withGitContext(ctx, "") require.NotNil(t, newCtx) require.NotNil(t, logger) @@ -2198,10 +2198,10 @@ func TestGitRepository_logger(t *testing.T) { ctx := context.Background() // First call creates the logger context - ctx1, logger1 := gitRepo.logger(ctx, "branch1") + ctx1, logger1 := gitRepo.withGitContext(ctx, "branch1") // Second call should return the existing logger context - ctx2, logger2 := gitRepo.logger(ctx1, "branch2") + ctx2, logger2 := gitRepo.withGitContext(ctx1, "branch2") // When logger context already exists, it should return the same context require.Equal(t, ctx1, ctx2) diff --git a/go.mod b/go.mod index 23756c59ae0..d8df0cd6bd7 100644 --- a/go.mod +++ b/go.mod @@ -106,7 +106,7 @@ require ( github.com/grafana/grafana-plugin-sdk-go v0.283.0 // @grafana/plugins-platform-backend github.com/grafana/loki/pkg/push v0.0.0-20250823105456-332df2b20000 // @grafana/alerting-backend github.com/grafana/loki/v3 v3.2.1 // @grafana/observability-logs - github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b // indirect; @grafana/grafana-git-ui-sync-team + github.com/grafana/nanogit v0.3.0 // indirect; @grafana/grafana-git-ui-sync-team github.com/grafana/otel-profiling-go v0.5.1 // @grafana/grafana-backend-group github.com/grafana/pyroscope-go/godeltaprof v0.1.9 // @grafana/observability-traces-and-profiling github.com/grafana/pyroscope/api v1.2.1-0.20250415190842-3ff7247547ae // @grafana/observability-traces-and-profiling diff --git a/go.sum b/go.sum index 61068fdef0b..1c294bc1fe2 100644 --- a/go.sum +++ b/go.sum @@ -1661,8 +1661,8 @@ github.com/grafana/loki/pkg/push v0.0.0-20250823105456-332df2b20000 h1:/5LKSYgLm github.com/grafana/loki/pkg/push v0.0.0-20250823105456-332df2b20000/go.mod h1:/ZklAgE1i4f3Z8uriXwESmCr1VLF8lBGaJspuaGuf78= github.com/grafana/loki/v3 v3.2.1 h1:VB7u+KHfvL5aHAxgoVBvz5wVhsdGuqKC7uuOFOOe7jw= github.com/grafana/loki/v3 v3.2.1/go.mod h1:WvdLl6wOS+yahaeQY+xhD2m2XzkHDfKr5FZaX7D/X2Y= -github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b h1:rFjoqJFb2KxJ29K9ltuWRSsdA46SbN0GCxoQc36h5kg= -github.com/grafana/nanogit v0.0.0-20251106115617-c622d3e0fc4b/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= +github.com/grafana/nanogit v0.3.0 h1:XNEef+4Vi+465ZITJs/g/xgnDRJbWhhJ7iQrAnWZ0oQ= +github.com/grafana/nanogit v0.3.0/go.mod h1:6s6CCTpyMOHPpcUZaLGI+rgBEKdmxVbhqSGgCK13j7Y= github.com/grafana/otel-profiling-go v0.5.1 h1:stVPKAFZSa7eGiqbYuG25VcqYksR6iWvF3YH66t4qL8= github.com/grafana/otel-profiling-go v0.5.1/go.mod h1:ftN/t5A/4gQI19/8MoWurBEtC6gFw8Dns1sJZ9W4Tls= github.com/grafana/prometheus-alertmanager v0.25.1-0.20250911094103-5456b6e45604 h1:aXfUhVN/Ewfpbko2CCtL65cIiGgwStOo4lWH2b6gw2U= diff --git a/go.work.sum b/go.work.sum index 9dc15683534..bbfe799f72c 100644 --- a/go.work.sum +++ b/go.work.sum @@ -937,7 +937,6 @@ github.com/grpc-ecosystem/go-grpc-middleware v1.3.0/go.mod h1:z0ButlSOZa5vEBq9m2 github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus v1.0.1/go.mod h1:lXGCsh6c22WGtjr+qGHj1otzZpV/1kwTMAqkwZsnWRU= github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.1.0/go.mod h1:XKMd7iuf/RGPSMJ/U4HP0zS2Z9Fh8Ps9a+6X26m/tmI= github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.0/go.mod h1:qOchhhIlmRcqk/O9uCo/puJlyo07YINaIqdZfZG3Jkc= -github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.3 h1:B+8ClL/kCQkRiU82d9xajRPKYMrB7E0MbtzWVi1K4ns= github.com/grpc-ecosystem/grpc-gateway v1.16.0 h1:gmcG1KaJ57LophUzW0Hy8NmPhnMZb4M0+kPpLofRdBo= github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0/go.mod h1:YN5jB8ie0yfIUg6VvR9Kz84aCaG7AsGZnLjhHbUqwPg= github.com/grpc-ecosystem/grpc-gateway/v2 v2.26.3/go.mod h1:ndYquD05frm2vACXE1nsccT4oJzjhw2arTS2cpUD1PI= @@ -1323,7 +1322,6 @@ github.com/prometheus/common v0.62.0/go.mod h1:vyBcEuLSvWos9B1+CyL7JZ2up+uFzXhkq github.com/prometheus/common v0.64.0/go.mod h1:0gZns+BLRQ3V6NdaerOhMbwwRbNh9hkGINtQAsP5GS8= github.com/prometheus/common v0.65.0/go.mod h1:0gZns+BLRQ3V6NdaerOhMbwwRbNh9hkGINtQAsP5GS8= github.com/prometheus/common v0.66.1/go.mod h1:gcaUsgf3KfRSwHY4dIMXLPV0K/Wg1oZ8+SbZk/HH/dA= -github.com/prometheus/common v0.67.2 h1:PcBAckGFTIHt2+L3I33uNRTlKTplNzFctXcWhPyAEN8= github.com/prometheus/common/assets v0.2.0 h1:0P5OrzoHrYBOSM1OigWL3mY8ZvV2N4zIE/5AahrSrfM= github.com/prometheus/exporter-toolkit v0.10.1-0.20230714054209-2f4150c63f97/go.mod h1:LoBCZeRh+5hX+fSULNyFnagYlQG/gBsyA/deNzROkq8= github.com/prometheus/statsd_exporter v0.21.0/go.mod h1:rbT83sZq2V+p73lHhPZfMc3MLCHmSHelCh9hSGYNLTQ= @@ -1374,6 +1372,7 @@ github.com/schollz/closestmatch v2.1.0+incompatible h1:Uel2GXEpJqOWBrlyI+oY9LTiy github.com/schollz/closestmatch v2.1.0+incompatible/go.mod h1:RtP1ddjLong6gTkbtmuhtR2uUrrJOpYzYRvbcPAid+g= github.com/schollz/progressbar/v3 v3.14.6 h1:GyjwcWBAf+GFDMLziwerKvpuS7ZF+mNTAXIB2aspiZs= github.com/schollz/progressbar/v3 v3.14.6/go.mod h1:Nrzpuw3Nl0srLY0VlTvC4V6RL50pcEymjy6qyJAaLa0= +github.com/sclevine/spec v1.4.0/go.mod h1:LvpgJaFyvQzRvc1kaDs0bulYwzC70PbiYjC4QnFHkOM= github.com/segmentio/fasthash v1.0.3 h1:EI9+KE1EwvMLBWwjpRDc+fEM+prwxDYbslddQGtrmhM= github.com/segmentio/fasthash v1.0.3/go.mod h1:waKX8l2N8yckOgmSsXJi7x1ZfdKZ4x7KRMzBtS3oedY= github.com/segmentio/parquet-go v0.0.0-20220811205829-7efc157d28af/go.mod h1:PxYdAI6cGd+s1j4hZDQbz3VFgobF5fDA0weLeNWKTE4= @@ -1920,7 +1919,6 @@ golang.org/x/oauth2 v0.25.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbht golang.org/x/oauth2 v0.28.0/go.mod h1:onh5ek6nERTohokkhCD/y2cV4Do3fxFHFuAejCkRWT8= golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKlU= golang.org/x/oauth2 v0.31.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= -golang.org/x/oauth2 v0.33.0 h1:4Q+qn+E5z8gPRJfmRy7C2gGG3T4jIprK6aSYgTXGRpo= golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= golang.org/x/sync v0.12.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sync v0.13.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=