From 71c5e75eca0afaf1478e8b545e5265596a5917e8 Mon Sep 17 00:00:00 2001 From: Roberto Jimenez Sanchez Date: Thu, 20 Nov 2025 09:47:26 +0100 Subject: [PATCH] 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 --- .../pkg/repository/git/repository.go | 80 ++++++++++++++++--- .../pkg/repository/git/repository_test.go | 10 +-- 2 files changed, 74 insertions(+), 16 deletions(-) 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)