Provisioning: Mark repository as unhealthy if hooks fail (#109788)

pkg/registry/apis/provisioning/controller/health.go

@@ -0,0 +1,217 @@
+package controller
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+	"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
+)
+
+// StatusPatcher defines the interface for updating repository status
+//
+//go:generate mockery --name=StatusPatcher
+type StatusPatcher interface {
+	Patch(ctx context.Context, repo *provisioning.Repository, patchOperations ...map[string]interface{}) error
+}
+
+// HealthChecker provides unified health checking for repositories
+type HealthChecker struct {
+	tester        RepositoryTester
+	statusPatcher StatusPatcher
+}
+
+// RepositoryTester defines the interface for testing repository connectivity
+//
+//go:generate mockery --name=RepositoryTester
+type RepositoryTester interface {
+	TestRepository(ctx context.Context, repo repository.Repository) (*provisioning.TestResults, error)
+}
+
+// NewHealthChecker creates a new health checker
+func NewHealthChecker(tester RepositoryTester, statusPatcher StatusPatcher) *HealthChecker {
+	return &HealthChecker{
+		tester:        tester,
+		statusPatcher: statusPatcher,
+	}
+}
+
+// ShouldCheckHealth determines if a repository health check should be performed
+func (hc *HealthChecker) ShouldCheckHealth(repo *provisioning.Repository) bool {
+	// If the repository has been updated, run the health check
+	if repo.Generation != repo.Status.ObservedGeneration {
+		return true
+	}
+
+	// If the repository has a hook error, don't run the health check
+	if repo.Status.Health.Error == provisioning.HealthFailureHook {
+		return false
+	}
+
+	// Check general timing for health checks
+	return !hc.hasRecentHealthCheck(repo.Status.Health)
+}
+
+// hasRecentHealthCheck checks if a health check was performed recently (for timing purposes)
+func (hc *HealthChecker) hasRecentHealthCheck(healthStatus provisioning.HealthStatus) bool {
+	if healthStatus.Checked == 0 {
+		return false // Never checked
+	}
+
+	age := time.Since(time.UnixMilli(healthStatus.Checked))
+	if healthStatus.Healthy {
+		return age <= time.Minute*5 // Recent if checked within 5 minutes when healthy
+	}
+	return age <= time.Minute // Recent if checked within 1 minute when unhealthy
+}
+
+// HasRecentFailure checks if there's a recent failure of a specific type
+func (hc *HealthChecker) HasRecentFailure(healthStatus provisioning.HealthStatus, failureType provisioning.HealthFailureType) bool {
+	if healthStatus.Checked == 0 || healthStatus.Healthy || healthStatus.Error != failureType {
+		return false // No failure of this type
+	}
+
+	age := time.Since(time.UnixMilli(healthStatus.Checked))
+	return age <= time.Minute // Recent if within 1 minute
+}
+
+// RecordFailureAndUpdate records a failure and updates the repository status
+func (hc *HealthChecker) RecordFailure(ctx context.Context, failureType provisioning.HealthFailureType, err error, repo *provisioning.Repository) error {
+	// Create the health status with the failure
+	healthStatus := hc.recordFailure(failureType, err)
+
+	// Create patch operation
+	patchOp := map[string]interface{}{
+		"op":    "replace",
+		"path":  "/status/health",
+		"value": healthStatus,
+	}
+
+	// Apply the patch
+	return hc.statusPatcher.Patch(ctx, repo, patchOp)
+}
+
+// recordFailure creates a health status with a specific failure
+func (hc *HealthChecker) recordFailure(failureType provisioning.HealthFailureType, err error) provisioning.HealthStatus {
+	return provisioning.HealthStatus{
+		Healthy: false,
+		Error:   failureType,
+		Checked: time.Now().UnixMilli(),
+		Message: []string{err.Error()},
+	}
+}
+
+// hasHealthStatusChanged checks if the health status has meaningfully changed
+func (hc *HealthChecker) hasHealthStatusChanged(old, new provisioning.HealthStatus) bool {
+	if old.Healthy != new.Healthy {
+		return true
+	}
+
+	if len(old.Message) != len(new.Message) {
+		return true
+	}
+
+	if old.Checked != new.Checked {
+		return true
+	}
+
+	for i, oldMsg := range old.Message {
+		if i >= len(new.Message) || oldMsg != new.Message[i] {
+			return true
+		}
+	}
+
+	return false
+}
+
+// RefreshHealth performs a health check on an existing repository,
+// updates its status if needed, and returns the test results
+func (hc *HealthChecker) RefreshHealth(ctx context.Context, repo repository.Repository) (*provisioning.TestResults, provisioning.HealthStatus, error) {
+	cfg := repo.Config()
+
+	// Use health checker to perform comprehensive health check with existing status
+	testResults, newHealthStatus, err := hc.refreshHealth(ctx, repo, cfg.Status.Health)
+	if err != nil {
+		return nil, provisioning.HealthStatus{}, fmt.Errorf("health check failed: %w", err)
+	}
+
+	// Only update if health status actually changed
+	if hc.hasHealthStatusChanged(cfg.Status.Health, newHealthStatus) {
+		patchOp := map[string]interface{}{
+			"op":    "replace",
+			"path":  "/status/health",
+			"value": newHealthStatus,
+		}
+
+		if err := hc.statusPatcher.Patch(ctx, cfg, patchOp); err != nil {
+			return testResults, newHealthStatus, fmt.Errorf("update health status: %w", err)
+		}
+	}
+
+	return testResults, newHealthStatus, nil
+}
+
+// RefreshTimestamp updates the health status timestamp without changing other fields
+func (hc *HealthChecker) RefreshTimestamp(ctx context.Context, repo *provisioning.Repository) error {
+	// Update the timestamp on the existing health status
+	healthStatus := repo.Status.Health
+	healthStatus.Checked = time.Now().UnixMilli()
+
+	// Create patch operation
+	patchOp := map[string]interface{}{
+		"op":    "replace",
+		"path":  "/status/health",
+		"value": healthStatus,
+	}
+
+	// Apply the patch
+	return hc.statusPatcher.Patch(ctx, repo, patchOp)
+}
+
+// refreshHealth performs a comprehensive health check
+// Returns test results, health status, and any error
+func (hc *HealthChecker) refreshHealth(ctx context.Context, repo repository.Repository, existingStatus provisioning.HealthStatus) (*provisioning.TestResults, provisioning.HealthStatus, error) {
+	res, err := hc.tester.TestRepository(ctx, repo)
+	if err != nil {
+		return nil, existingStatus, fmt.Errorf("failed to test repository: %w", err)
+	}
+
+	if !res.Success {
+		// Build error messages
+		var errorMsgs []string
+		for _, testErr := range res.Errors {
+			if testErr.Detail != "" {
+				errorMsgs = append(errorMsgs, testErr.Detail)
+			}
+		}
+
+		healthStatus := provisioning.HealthStatus{
+			Healthy: false,
+			Error:   provisioning.HealthFailureHealth,
+			Checked: time.Now().UnixMilli(),
+			Message: errorMsgs,
+		}
+
+		return res, healthStatus, nil
+	}
+
+	// Health check succeeded
+	now := time.Now()
+	healthStatus := provisioning.HealthStatus{
+		Healthy: true,
+		Checked: now.UnixMilli(),
+	}
+
+	// If the existing status is already healthy with no error messages and
+	// the last check was recent (within 30 seconds), preserve the existing timestamp
+	// to avoid unnecessary updates
+	if existingStatus.Healthy && existingStatus.Error == "" && len(existingStatus.Message) == 0 {
+		lastCheckedTime := time.UnixMilli(existingStatus.Checked)
+		if now.Sub(lastCheckedTime) < 30*time.Second {
+			healthStatus.Checked = existingStatus.Checked
+		}
+	}
+
+	return res, healthStatus, nil
+}

pkg/registry/apis/provisioning/controller/health_test.go

@@ -0,0 +1,592 @@
+package controller
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+	"github.com/grafana/grafana/pkg/registry/apis/provisioning/controller/mocks"
+)
+
+func TestNewHealthChecker(t *testing.T) {
+	mockTester := mocks.NewRepositoryTester(t)
+	mockPatcher := mocks.NewStatusPatcher(t)
+
+	hc := NewHealthChecker(mockTester, mockPatcher)
+
+	assert.NotNil(t, hc)
+	assert.Equal(t, mockTester, hc.tester)
+	assert.Equal(t, mockPatcher, hc.statusPatcher)
+}
+
+func TestShouldCheckHealth(t *testing.T) {
+	tests := []struct {
+		name     string
+		repo     *provisioning.Repository
+		expected bool
+	}{
+		{
+			name: "should check when generation differs",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 2},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: true,
+						Checked: time.Now().UnixMilli(),
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "should not check when hook error exists",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: false,
+						Error:   provisioning.HealthFailureHook,
+						Checked: time.Now().UnixMilli(),
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "should not check when health check is recent and healthy",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: true,
+						Checked: time.Now().Add(-time.Minute * 3).UnixMilli(), // 3 minutes ago
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "should check when health check is old and healthy",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: true,
+						Checked: time.Now().Add(-time.Minute * 6).UnixMilli(), // 6 minutes ago
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "should not check when health error is recent",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: false,
+						Error:   provisioning.HealthFailureHealth,
+						Checked: time.Now().Add(-time.Second * 30).UnixMilli(), // 30 seconds ago
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name: "should check when health error is old",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Healthy: false,
+						Error:   provisioning.HealthFailureHealth,
+						Checked: time.Now().Add(-time.Minute * 2).UnixMilli(), // 2 minutes ago
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "should check when never checked",
+			repo: &provisioning.Repository{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: provisioning.RepositoryStatus{
+					ObservedGeneration: 1,
+					Health: provisioning.HealthStatus{
+						Checked: 0, // Never checked
+					},
+				},
+			},
+			expected: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockTester := mocks.NewRepositoryTester(t)
+			mockPatcher := mocks.NewStatusPatcher(t)
+			hc := NewHealthChecker(mockTester, mockPatcher)
+
+			result := hc.ShouldCheckHealth(tt.repo)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestHasRecentFailure(t *testing.T) {
+	tests := []struct {
+		name         string
+		healthStatus provisioning.HealthStatus
+		failureType  provisioning.HealthFailureType
+		expected     bool
+	}{
+		{
+			name: "no recent failure when never checked",
+			healthStatus: provisioning.HealthStatus{
+				Checked: 0,
+			},
+			failureType: provisioning.HealthFailureHook,
+			expected:    false,
+		},
+		{
+			name: "recent hook failure",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHook,
+				Checked: time.Now().Add(-time.Second * 30).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHook,
+			expected:    true,
+		},
+		{
+			name: "old hook failure",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHook,
+				Checked: time.Now().Add(-time.Minute * 2).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHook,
+			expected:    false,
+		},
+		{
+			name: "recent health failure",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHealth,
+				Checked: time.Now().Add(-time.Second * 30).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHealth,
+			expected:    true,
+		},
+		{
+			name: "old health failure",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHealth,
+				Checked: time.Now().Add(-time.Minute * 2).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHealth,
+			expected:    false,
+		},
+		{
+			name: "wrong failure type",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHook,
+				Checked: time.Now().Add(-time.Second * 30).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHealth,
+			expected:    false,
+		},
+		{
+			name: "healthy status with wrong failure type",
+			healthStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-time.Second * 30).UnixMilli(),
+			},
+			failureType: provisioning.HealthFailureHook,
+			expected:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockTester := mocks.NewRepositoryTester(t)
+			mockPatcher := mocks.NewStatusPatcher(t)
+			hc := NewHealthChecker(mockTester, mockPatcher)
+
+			result := hc.HasRecentFailure(tt.healthStatus, tt.failureType)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestRecordFailure(t *testing.T) {
+	tests := []struct {
+		name        string
+		failureType provisioning.HealthFailureType
+		err         error
+		patchError  error
+		expectError bool
+	}{
+		{
+			name:        "successful hook failure record",
+			failureType: provisioning.HealthFailureHook,
+			err:         errors.New("hook failed"),
+			patchError:  nil,
+			expectError: false,
+		},
+		{
+			name:        "successful health failure record",
+			failureType: provisioning.HealthFailureHealth,
+			err:         errors.New("health check failed"),
+			patchError:  nil,
+			expectError: false,
+		},
+		{
+			name:        "patch failure",
+			failureType: provisioning.HealthFailureHook,
+			err:         errors.New("hook failed"),
+			patchError:  errors.New("patch failed"),
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockTester := mocks.NewRepositoryTester(t)
+			mockPatcher := mocks.NewStatusPatcher(t)
+			hc := NewHealthChecker(mockTester, mockPatcher)
+
+			repo := &provisioning.Repository{
+				Status: provisioning.RepositoryStatus{
+					Health: provisioning.HealthStatus{
+						Healthy: true,
+						Checked: time.Now().Add(-time.Hour).UnixMilli(),
+					},
+				},
+			}
+
+			if tt.patchError != nil {
+				mockPatcher.On("Patch", mock.Anything, repo, mock.AnythingOfType("map[string]interface {}")).
+					Return(tt.patchError)
+			} else {
+				mockPatcher.On("Patch", mock.Anything, repo, mock.AnythingOfType("map[string]interface {}")).
+					Return(nil).
+					Run(func(args mock.Arguments) {
+						patchOp := args[2].(map[string]interface{})
+						assert.Equal(t, "replace", patchOp["op"])
+						assert.Equal(t, "/status/health", patchOp["path"])
+
+						healthStatus := patchOp["value"].(provisioning.HealthStatus)
+						assert.False(t, healthStatus.Healthy)
+						assert.Equal(t, tt.failureType, healthStatus.Error)
+						assert.Contains(t, healthStatus.Message, tt.err.Error())
+						assert.Greater(t, healthStatus.Checked, int64(0))
+					})
+			}
+
+			err := hc.RecordFailure(context.Background(), tt.failureType, tt.err, repo)
+
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			mockPatcher.AssertExpectations(t)
+		})
+	}
+}
+
+func TestRecordFailureFunction(t *testing.T) {
+	mockTester := mocks.NewRepositoryTester(t)
+	mockPatcher := mocks.NewStatusPatcher(t)
+	hc := NewHealthChecker(mockTester, mockPatcher)
+
+	testErr := errors.New("test error")
+	result := hc.recordFailure(provisioning.HealthFailureHook, testErr)
+
+	assert.False(t, result.Healthy)
+	assert.Equal(t, provisioning.HealthFailureHook, result.Error)
+	assert.Equal(t, []string{"test error"}, result.Message)
+	assert.Greater(t, result.Checked, int64(0))
+}
+
+func TestRefreshHealth(t *testing.T) {
+	tests := []struct {
+		name           string
+		testResult     *provisioning.TestResults
+		testError      error
+		patchError     error
+		existingStatus provisioning.HealthStatus
+		expectError    bool
+		expectedHealth bool
+		expectPatch    bool
+	}{
+		{
+			name: "successful health check",
+			testResult: &provisioning.TestResults{
+				Success: true,
+				Code:    200,
+			},
+			testError: nil,
+			existingStatus: provisioning.HealthStatus{
+				Healthy: false,
+				Error:   provisioning.HealthFailureHealth,
+				Checked: time.Now().Add(-time.Hour).UnixMilli(),
+			},
+			expectError:    false,
+			expectedHealth: true,
+			expectPatch:    true,
+		},
+		{
+			name: "failed health check",
+			testResult: &provisioning.TestResults{
+				Success: false,
+				Code:    500,
+				Errors: []provisioning.ErrorDetails{
+					{Detail: "connection failed"},
+					{Detail: "timeout"},
+				},
+			},
+			testError: nil,
+			existingStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-time.Hour).UnixMilli(),
+			},
+			expectError:    false,
+			expectedHealth: false,
+			expectPatch:    true,
+		},
+		{
+			name:       "test repository error",
+			testResult: nil,
+			testError:  errors.New("repository test failed"),
+			existingStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-time.Hour).UnixMilli(),
+			},
+			expectError:    true,
+			expectedHealth: false,
+			expectPatch:    false,
+		},
+		{
+			name: "no status change - no patch needed (recent check)",
+			testResult: &provisioning.TestResults{
+				Success: true,
+				Code:    200,
+			},
+			testError: nil,
+			existingStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-15 * time.Second).UnixMilli(),
+			},
+			expectError:    false,
+			expectedHealth: true,
+			expectPatch:    false,
+		},
+		{
+			name: "status unchanged but timestamp needs update (old check)",
+			testResult: &provisioning.TestResults{
+				Success: true,
+				Code:    200,
+			},
+			testError: nil,
+			existingStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-time.Hour).UnixMilli(),
+			},
+			expectError:    false,
+			expectedHealth: true,
+			expectPatch:    true,
+		},
+		{
+			name: "patch error",
+			testResult: &provisioning.TestResults{
+				Success: false,
+				Code:    500,
+				Errors: []provisioning.ErrorDetails{
+					{Detail: "connection failed"},
+				},
+			},
+			testError:  nil,
+			patchError: errors.New("patch failed"),
+			existingStatus: provisioning.HealthStatus{
+				Healthy: true,
+				Checked: time.Now().Add(-time.Hour).UnixMilli(),
+			},
+			expectError:    true,
+			expectedHealth: false,
+			expectPatch:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockTester := mocks.NewRepositoryTester(t)
+			mockPatcher := mocks.NewStatusPatcher(t)
+			mockRepo := &mockRepository{
+				config: &provisioning.Repository{
+					Status: provisioning.RepositoryStatus{
+						Health: tt.existingStatus,
+					},
+				},
+			}
+
+			hc := NewHealthChecker(mockTester, mockPatcher)
+
+			if tt.testError != nil {
+				mockTester.On("TestRepository", mock.Anything, mockRepo).Return(tt.testResult, tt.testError)
+			} else {
+				mockTester.On("TestRepository", mock.Anything, mockRepo).Return(tt.testResult, nil)
+			}
+
+			if tt.expectPatch {
+				if tt.patchError != nil {
+					mockPatcher.On("Patch", mock.Anything, mockRepo.config, mock.AnythingOfType("map[string]interface {}")).
+						Return(tt.patchError)
+				} else {
+					mockPatcher.On("Patch", mock.Anything, mockRepo.config, mock.AnythingOfType("map[string]interface {}")).
+						Return(nil).
+						Run(func(args mock.Arguments) {
+							patchOp := args[2].(map[string]interface{})
+							assert.Equal(t, "replace", patchOp["op"])
+							assert.Equal(t, "/status/health", patchOp["path"])
+
+							healthStatus := patchOp["value"].(provisioning.HealthStatus)
+							assert.Equal(t, tt.expectedHealth, healthStatus.Healthy)
+						})
+				}
+			}
+
+			testResult, healthStatus, err := hc.RefreshHealth(context.Background(), mockRepo)
+
+			if tt.expectError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.expectedHealth, healthStatus.Healthy)
+				if tt.testResult != nil {
+					assert.Equal(t, tt.testResult, testResult)
+				}
+			}
+
+			mockTester.AssertExpectations(t)
+			if tt.expectPatch {
+				mockPatcher.AssertExpectations(t)
+			}
+		})
+	}
+}
+
+func TestHasHealthStatusChanged(t *testing.T) {
+	tests := []struct {
+		name     string
+		old      provisioning.HealthStatus
+		new      provisioning.HealthStatus
+		expected bool
+	}{
+		{
+			name: "healthy status changed",
+			old: provisioning.HealthStatus{
+				Healthy: true,
+				Message: []string{},
+			},
+			new: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error"},
+			},
+			expected: true,
+		},
+		{
+			name: "different message count",
+			old: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error1"},
+			},
+			new: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error1", "error2"},
+			},
+			expected: true,
+		},
+		{
+			name: "different messages",
+			old: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error1"},
+			},
+			new: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error2"},
+			},
+			expected: true,
+		},
+		{
+			name: "no change",
+			old: provisioning.HealthStatus{
+				Healthy: true,
+				Message: []string{},
+			},
+			new: provisioning.HealthStatus{
+				Healthy: true,
+				Message: []string{},
+			},
+			expected: false,
+		},
+		{
+			name: "same messages",
+			old: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error1", "error2"},
+			},
+			new: provisioning.HealthStatus{
+				Healthy: false,
+				Message: []string{"error1", "error2"},
+			},
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockTester := mocks.NewRepositoryTester(t)
+			mockPatcher := mocks.NewStatusPatcher(t)
+			hc := NewHealthChecker(mockTester, mockPatcher)
+
+			result := hc.hasHealthStatusChanged(tt.old, tt.new)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+// mockRepository implements repository.Repository interface for testing
+type mockRepository struct {
+	config *provisioning.Repository
+}
+
+func (m *mockRepository) Config() *provisioning.Repository {
+	return m.config
+}
+
+func (m *mockRepository) Validate() field.ErrorList {
+	return nil
+}
+
+func (m *mockRepository) Test(ctx context.Context) (*provisioning.TestResults, error) {
+	return &provisioning.TestResults{Success: true}, nil
+}

pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go

@@ -0,0 +1,62 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package mocks
+
+import (
+	context "context"
+
+	mock "github.com/stretchr/testify/mock"
+
+	repository "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
+
+	v0alpha1 "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+)
+
+// RepositoryTester is an autogenerated mock type for the RepositoryTester type
+type RepositoryTester struct {
+	mock.Mock
+}
+
+// TestRepository provides a mock function with given fields: ctx, repo
+func (_m *RepositoryTester) TestRepository(ctx context.Context, repo repository.Repository) (*v0alpha1.TestResults, error) {
+	ret := _m.Called(ctx, repo)
+
+	if len(ret) == 0 {
+		panic("no return value specified for TestRepository")
+	}
+
+	var r0 *v0alpha1.TestResults
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, repository.Repository) (*v0alpha1.TestResults, error)); ok {
+		return rf(ctx, repo)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, repository.Repository) *v0alpha1.TestResults); ok {
+		r0 = rf(ctx, repo)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*v0alpha1.TestResults)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, repository.Repository) error); ok {
+		r1 = rf(ctx, repo)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// NewRepositoryTester creates a new instance of RepositoryTester. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func NewRepositoryTester(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *RepositoryTester {
+	mock := &RepositoryTester{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/controller/mocks/StatusPatcher.go

@@ -0,0 +1,55 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package mocks
+
+import (
+	context "context"
+
+	mock "github.com/stretchr/testify/mock"
+
+	v0alpha1 "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+)
+
+// StatusPatcher is an autogenerated mock type for the StatusPatcher type
+type StatusPatcher struct {
+	mock.Mock
+}
+
+// Patch provides a mock function with given fields: ctx, repo, patchOperations
+func (_m *StatusPatcher) Patch(ctx context.Context, repo *v0alpha1.Repository, patchOperations ...map[string]interface{}) error {
+	_va := make([]interface{}, len(patchOperations))
+	for _i := range patchOperations {
+		_va[_i] = patchOperations[_i]
+	}
+	var _ca []interface{}
+	_ca = append(_ca, ctx, repo)
+	_ca = append(_ca, _va...)
+	ret := _m.Called(_ca...)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Patch")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, *v0alpha1.Repository, ...map[string]interface{}) error); ok {
+		r0 = rf(ctx, repo, patchOperations...)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// NewStatusPatcher creates a new instance of StatusPatcher. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func NewStatusPatcher(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *StatusPatcher {
+	mock := &StatusPatcher{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/controller/repository.go

@@ -2,7 +2,6 @@ package controller
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"time"
@@ -35,10 +34,6 @@ type RepoGetter interface {
 	AsRepository(ctx context.Context, cfg *provisioning.Repository) (repository.Repository, error)
 }
 
-type RepositoryTester interface {
-	TestRepository(ctx context.Context, repo repository.Repository) (*provisioning.TestResults, error)
-}
-
 const loggerName = "provisioning-repository-controller"
 
 const (
@@ -61,12 +56,13 @@ type RepositoryController struct {
 	logger         logging.Logger
 	dualwrite      dualwrite.Service
 
-	jobs      jobs.Queue
-	finalizer *finalizer
+	jobs          jobs.Queue
+	finalizer     *finalizer
+	statusPatcher StatusPatcher
 
 	// Converts config to instance
-	repoGetter RepoGetter
-	tester     RepositoryTester
+	repoGetter    RepoGetter
+	healthChecker *HealthChecker
 	// To allow injection for testing.
 	processFn         func(item *queueItem) error
 	enqueueRepository func(obj any)
@@ -86,6 +82,8 @@ func NewRepositoryController(
 	tester RepositoryTester,
 	jobs jobs.Queue,
 	dualwrite dualwrite.Service,
+	healthChecker *HealthChecker,
+	statusPatcher StatusPatcher,
 ) (*RepositoryController, error) {
 	rc := &RepositoryController{
 		client:         provisioningClient,
@@ -98,13 +96,14 @@ func NewRepositoryController(
 				Name: "provisioningRepositoryController",
 			},
 		),
-		repoGetter: repoGetter,
-		parsers:    parsers,
+		repoGetter:    repoGetter,
+		healthChecker: healthChecker,
+		statusPatcher: statusPatcher,
+		parsers:       parsers,
 		finalizer: &finalizer{
 			lister:        resourceLister,
 			clientFactory: clients,
 		},
-		tester:    tester,
 		jobs:      jobs,
 		logger:    logging.DefaultLogger.With("logger", loggerName),
 		dualwrite: dualwrite,
@@ -247,47 +246,6 @@ func (rc *RepositoryController) handleDelete(ctx context.Context, obj *provision
 	return nil
 }
 
-func (rc *RepositoryController) shouldCheckHealth(obj *provisioning.Repository) bool {
-	if obj.Status.Health.Checked == 0 || obj.Generation != obj.Status.ObservedGeneration {
-		return true
-	}
-
-	healthAge := time.Since(time.UnixMilli(obj.Status.Health.Checked))
-	if obj.Status.Health.Healthy {
-		return healthAge > time.Minute*5 // when healthy, check every 5 mins
-	}
-
-	return healthAge > time.Minute // otherwise within a minute
-}
-
-func (rc *RepositoryController) runHealthCheck(ctx context.Context, repo repository.Repository) provisioning.HealthStatus {
-	logger := logging.FromContext(ctx)
-	logger.Info("running health check")
-	res, err := rc.tester.TestRepository(ctx, repo)
-	if err != nil {
-		res = &provisioning.TestResults{
-			Success: false,
-			Errors: []provisioning.ErrorDetails{{
-				Detail: fmt.Sprintf("error running test repository: %s", err.Error()),
-			}},
-		}
-	}
-
-	healthStatus := provisioning.HealthStatus{
-		Healthy: res.Success,
-		Checked: time.Now().UnixMilli(),
-	}
-	for _, err := range res.Errors {
-		if err.Detail != "" {
-			healthStatus.Message = append(healthStatus.Message, err.Detail)
-		}
-	}
-
-	logger.Info("health check completed", "status", healthStatus)
-
-	return healthStatus
-}
-
 func (rc *RepositoryController) shouldResync(obj *provisioning.Repository) bool {
 	// don't trigger resync if a sync was never started
 	if obj.Status.Sync.Finished == 0 && obj.Status.Sync.State == "" {
@@ -309,7 +267,7 @@ func (rc *RepositoryController) shouldResync(obj *provisioning.Repository) bool
 func (rc *RepositoryController) runHooks(ctx context.Context, repo repository.Repository, obj *provisioning.Repository) ([]map[string]interface{}, error) {
 	logger := logging.FromContext(ctx)
 	hooks, _ := repo.(repository.Hooks)
-	if hooks == nil || obj.Generation == obj.Status.ObservedGeneration {
+	if hooks == nil {
 		return nil, nil
 	}
 
@@ -400,26 +358,7 @@ func (rc *RepositoryController) addSyncJob(ctx context.Context, obj *provisionin
 	return nil
 }
 
-func (rc *RepositoryController) patchStatus(ctx context.Context, obj *provisioning.Repository, patchOperations []map[string]interface{}) error {
-	if len(patchOperations) == 0 {
-		return nil
-	}
-
-	patch, err := json.Marshal(patchOperations)
-	if err != nil {
-		return fmt.Errorf("error encoding status patch: %w", err)
-	}
-
-	_, err = rc.client.Repositories(obj.GetNamespace()).
-		Patch(ctx, obj.Name, types.JSONPatchType, patch, v1.PatchOptions{}, "status")
-	if err != nil {
-		return fmt.Errorf("error applying status patch: %w", err)
-	}
-
-	return nil
-}
-
-func (rc *RepositoryController) determineSyncStatus(obj *provisioning.Repository, syncOptions *provisioning.SyncJobOptions) *provisioning.SyncStatus {
+func (rc *RepositoryController) determineSyncStatus(obj *provisioning.Repository, syncOptions *provisioning.SyncJobOptions, healthStatus provisioning.HealthStatus) *provisioning.SyncStatus {
 	const unhealthyMessage = "Repository is unhealthy"
 
 	hasUnhealthyMessage := len(obj.Status.Sync.Message) > 0 && obj.Status.Sync.Message[0] == unhealthyMessage
@@ -430,13 +369,13 @@ func (rc *RepositoryController) determineSyncStatus(obj *provisioning.Repository
 			LastRef: obj.Status.Sync.LastRef,
 			Started: time.Now().UnixMilli(),
 		}
-	case obj.Status.Health.Healthy && hasUnhealthyMessage: // if the repository is healthy and the message is set, clear it
+	case healthStatus.Healthy && hasUnhealthyMessage: // if the repository is healthy and the message is set, clear it
 		// FIXME: is this the clearest way to do this? Should we introduce another status or way of way of handling more
 		// specific errors?
 		return &provisioning.SyncStatus{
 			LastRef: obj.Status.Sync.LastRef,
 		}
-	case !obj.Status.Health.Healthy && !hasUnhealthyMessage: // if the repository is unhealthy and the message is not already set, set it
+	case !healthStatus.Healthy && !hasUnhealthyMessage: // if the repository is unhealthy and the message is not already set, set it
 		return &provisioning.SyncStatus{
 			State:   provisioning.JobStateError,
 			Message: []string{unhealthyMessage},
@@ -450,6 +389,7 @@ func (rc *RepositoryController) determineSyncStatus(obj *provisioning.Repository
 //nolint:gocyclo
 func (rc *RepositoryController) process(item *queueItem) error {
 	logger := rc.logger.With("key", item.key)
+	ctx := logging.Context(context.Background(), logger)
 
 	namespace, name, err := cache.SplitMetaNamespaceKey(item.key)
 	if err != nil {
@@ -464,7 +404,7 @@ func (rc *RepositoryController) process(item *queueItem) error {
 		return err
 	}
 
-	ctx, _, err := identity.WithProvisioningIdentity(context.Background(), namespace)
+	ctx, _, err = identity.WithProvisioningIdentity(ctx, namespace)
 	if err != nil {
 		return err
 	}
@@ -476,7 +416,7 @@ func (rc *RepositoryController) process(item *queueItem) error {
 	}
 
 	shouldResync := rc.shouldResync(obj)
-	shouldCheckHealth := rc.shouldCheckHealth(obj)
+	shouldCheckHealth := rc.healthChecker.ShouldCheckHealth(obj)
 	hasSpecChanged := obj.Generation != obj.Status.ObservedGeneration
 	patchOperations := []map[string]interface{}{}
 
@@ -503,28 +443,27 @@ func (rc *RepositoryController) process(item *queueItem) error {
 		return fmt.Errorf("unable to create repository from configuration: %w", err)
 	}
 
-	healthStatus := obj.Status.Health
-	if shouldCheckHealth {
-		healthStatus = rc.runHealthCheck(ctx, repo)
-		patchOperations = append(patchOperations, map[string]interface{}{
-			"op":    "replace",
-			"path":  "/status/health",
-			"value": healthStatus,
-		})
+	// Handle hooks - may return early if hooks fail
+	hookOps, shouldContinue, err := rc.processHooks(ctx, repo, obj)
+	if err != nil {
+		return fmt.Errorf("process hooks: %w", err)
+	}
+	if !shouldContinue {
+		return nil // Hook handling already updated status and returned early
+	}
+	if len(hookOps) > 0 {
+		patchOperations = append(patchOperations, hookOps...)
 	}
 
-	// Run hooks
-	hookOps, err := rc.runHooks(ctx, repo, obj)
-	switch {
-	case err != nil:
-		return err
-	case len(hookOps) > 0:
-		patchOperations = append(patchOperations, hookOps...)
+	// Handle health checks using the health checker
+	_, healthStatus, err := rc.healthChecker.RefreshHealth(ctx, repo)
+	if err != nil {
+		return fmt.Errorf("update health status: %w", err)
 	}
 
 	// determine the sync strategy and sync status to apply
 	syncOptions := rc.determineSyncStrategy(ctx, obj, repo, shouldResync, healthStatus)
-	if syncStatus := rc.determineSyncStatus(obj, syncOptions); syncStatus != nil {
+	if syncStatus := rc.determineSyncStatus(obj, syncOptions, healthStatus); syncStatus != nil {
 		patchOperations = append(patchOperations, map[string]interface{}{
 			"op":    "replace",
 			"path":  "/status/sync",
@@ -533,7 +472,7 @@ func (rc *RepositoryController) process(item *queueItem) error {
 	}
 
 	// Apply all patch operations
-	if err := rc.patchStatus(ctx, obj, patchOperations); err != nil {
+	if err := rc.statusPatcher.Patch(ctx, obj, patchOperations...); err != nil {
 		return err
 	}
 
@@ -546,3 +485,29 @@ func (rc *RepositoryController) process(item *queueItem) error {
 
 	return nil
 }
+
+// processHooks handles hook execution with intelligent retry logic
+// Returns hook operations, whether processing should continue, and any error
+func (rc *RepositoryController) processHooks(ctx context.Context, repo repository.Repository, obj *provisioning.Repository) ([]map[string]interface{}, bool, error) {
+	shouldRunHooks := obj.Generation != obj.Status.ObservedGeneration
+
+	// Skip hooks if status already indicates recent hook failure to avoid infinite retry
+	if shouldRunHooks && rc.healthChecker.HasRecentFailure(obj.Status.Health, provisioning.HealthFailureHook) {
+		shouldRunHooks = false
+	}
+
+	if !shouldRunHooks {
+		return nil, true, nil
+	}
+
+	hookOps, err := rc.runHooks(ctx, repo, obj)
+	if err != nil {
+		if err := rc.healthChecker.RecordFailure(ctx, provisioning.HealthFailureHook, err, obj); err != nil {
+			return nil, false, fmt.Errorf("update status after hook failure: %w", err)
+		}
+
+		return nil, false, err
+	}
+
+	return hookOps, true, nil
+}