Skip to content

Commit aaee5e6

Browse files
kushnaidukushnaidu
authored
Move git connectivity check into background.go (#370)
* Refactor git repo connectivity check * Add unit test to cover runOnce connectivity check fail
1 parent bdfc708 commit aaee5e6

7 files changed

Lines changed: 184 additions & 19 deletions

File tree

‎pkg/cache/crcache/cache.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,6 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
6262
c.mainLock.RUnlock()
6363
// Keep the spec updated in the cache.
6464
repo.repoSpec = repositorySpec
65-
// Check external repo connectivity
66-
if err := externalrepo.CheckRepositoryConnection(ctx, repositorySpec, c.options.ExternalRepoOptions); err != nil {
67-
klog.Warningf("Cache:OpenRepository: repo %+v connectivity check failed with error %q", key, err)
68-
return nil, err
69-
}
70-
klog.V(2).Infof("Cache::OpenRepository: verified repo connectivity %+v", key)
7165
// If there is an error from the background refresh goroutine, return it.
7266
if err := repo.getRefreshError(); err != nil {
7367
return nil, err
@@ -165,6 +159,10 @@ func (c *Cache) GetRepository(repoKey repository.RepositoryKey) repository.Repos
165159
return c.repositories[repoKey]
166160
}
167161

162+
func (c *Cache) CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error {
163+
return externalrepo.CheckRepositoryConnection(ctx, repositorySpec, c.options.ExternalRepoOptions)
164+
}
165+
168166
func (c *Cache) getOrInsertLock(key repository.RepositoryKey) *sync.Mutex {
169167
c.mainLock.RLock()
170168
if lock, exists := c.locks[key]; exists {

‎pkg/cache/dbcache/dbcache.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828
pkgerrors "github.com/pkg/errors"
2929
"go.opentelemetry.io/otel"
3030
"go.opentelemetry.io/otel/trace"
31-
"k8s.io/klog/v2"
3231

3332
_ "github.com/jackc/pgx/v5/stdlib"
3433
)
@@ -58,12 +57,6 @@ func (c *dbCache) OpenRepository(ctx context.Context, repositorySpec *configapi.
5857
c.mainLock.RUnlock()
5958
// Keep the spec updated in the cache.
6059
dbRepo.spec = repositorySpec
61-
err := externalrepo.CheckRepositoryConnection(ctx, dbRepo.spec, c.options.ExternalRepoOptions)
62-
if err != nil {
63-
klog.Warningf("dbRepository:OpenRepository: repo %+v connectivity check failed with error %q", repoKey, err)
64-
return nil, err
65-
}
66-
klog.V(2).Infof("dbCache::OpenRepository: verified repo connectivity %+v", repoKey)
6760
return dbRepo, nil
6861
}
6962
c.mainLock.RUnlock()
@@ -158,3 +151,7 @@ func (c *dbCache) GetRepository(repoKey repository.RepositoryKey) repository.Rep
158151
defer c.mainLock.RUnlock()
159152
return c.repositories[repoKey]
160153
}
154+
155+
func (c *dbCache) CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error {
156+
return externalrepo.CheckRepositoryConnection(ctx, repositorySpec, c.options.ExternalRepoOptions)
157+
}

‎pkg/cache/types/cachetypes.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ type Cache interface {
5858
GetRepositories() []*configapi.Repository
5959
GetRepository(repository.RepositoryKey) repository.Repository
6060
UpdateRepository(ctx context.Context, repositorySpec *configapi.Repository) error
61+
CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error
6162
}
6263

6364
var (

‎pkg/engine/engine_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,11 @@ func (m *mockCache) UpdateRepository(ctx context.Context, repositoryObj *configa
281281
return args.Error(0)
282282
}
283283

284+
func (m *mockCache) CheckRepositoryConnectivity(ctx context.Context, repositorySpec *configapi.Repository) error {
285+
args := m.Called(ctx, repositorySpec)
286+
return args.Error(0)
287+
}
288+
284289
func TestCreatePRWith2Tasks(t *testing.T) {
285290
pr := &porchapi.PackageRevision{
286291
Spec: porchapi.PackageRevisionSpec{

‎pkg/registry/porch/background.go‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,19 @@ func (b *background) handleRepositoryEvent(ctx context.Context, repo *configapi.
203203
case watch.Deleted:
204204
err = b.cache.CloseRepository(listCtx, repo, repoList.Items)
205205
default:
206+
// Check connectivity before caching
207+
if err = b.checkRepositoryConnectivity(listCtx, repo); err != nil {
208+
klog.Warningf("Repository connectivity check failed for %s: %v", repo.Name, err)
209+
condition := v1.Condition{
210+
Type: configapi.RepositoryReady,
211+
Status: v1.ConditionFalse,
212+
ObservedGeneration: repo.Generation,
213+
LastTransitionTime: v1.Now(),
214+
Reason: configapi.ReasonError,
215+
Message: fmt.Sprintf("Repository connectivity check failed: %v", err),
216+
}
217+
return b.updateRepositoryStatusCondition(listCtx, repo, condition)
218+
}
206219
err = b.cacheRepository(listCtx, repo)
207220
}
208221
if err == nil {
@@ -223,6 +236,23 @@ func (b *background) runOnce(ctx context.Context) error {
223236
for i := range repositories.Items {
224237
repo := &repositories.Items[i]
225238

239+
// Check repository connectivity
240+
if err := b.checkRepositoryConnectivity(ctx, repo); err != nil {
241+
klog.Warningf("Repository connectivity check failed for %s: %v", repo.Name, err)
242+
condition := v1.Condition{
243+
Type: configapi.RepositoryReady,
244+
Status: v1.ConditionFalse,
245+
ObservedGeneration: repo.Generation,
246+
LastTransitionTime: v1.Now(),
247+
Reason: configapi.ReasonError,
248+
Message: fmt.Sprintf("Repository connectivity check failed: %v", err),
249+
}
250+
if err := b.updateRepositoryStatusCondition(ctx, repo, condition); err != nil {
251+
klog.Errorf("Failed to update repository status for %s: %v", repo.Name, err)
252+
}
253+
continue // Skip cacheRepository if connectivity fails
254+
}
255+
226256
if err := b.cacheRepository(ctx, repo); err != nil {
227257
klog.Errorf("Failed to cache repository: %v", err)
228258
}
@@ -269,7 +299,14 @@ func (b *background) cacheRepository(ctx context.Context, repo *configapi.Reposi
269299
}
270300
}
271301

272-
// Update status condition with retry only on API conflict
302+
return b.updateRepositoryStatusCondition(ctx, repo, condition)
303+
}
304+
305+
func (b *background) checkRepositoryConnectivity(ctx context.Context, repo *configapi.Repository) error {
306+
return b.cache.CheckRepositoryConnectivity(ctx, repo)
307+
}
308+
309+
func (b *background) updateRepositoryStatusCondition(ctx context.Context, repo *configapi.Repository, condition v1.Condition) error {
273310
for attempt := 1; attempt <= b.repoOperationRetryAttempts; attempt++ {
274311
latestRepo := &configapi.Repository{}
275312
err := b.coreClient.Get(ctx, types.NamespacedName{
@@ -291,7 +328,6 @@ func (b *background) cacheRepository(ctx context.Context, repo *configapi.Reposi
291328
time.Sleep(100 * time.Millisecond)
292329
continue
293330
}
294-
// Return immediately for non-conflict errors
295331
return fmt.Errorf("error updating repository status: %w", err)
296332
}
297333
return fmt.Errorf("failed to update repository status after retries")

‎pkg/registry/porch/background_test.go‎

Lines changed: 75 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestBackgroundOptions(t *testing.T) {
5050
},
5151
expected: background{
5252
periodicRepoSyncFrequency: 5 * time.Second,
53-
listTimeoutPerRepo: 10 * time.Second,
53+
listTimeoutPerRepo: 10 * time.Second,
5454
},
5555
},
5656
}
@@ -105,6 +105,7 @@ func TestBackgroundHandleRepositoryEvent(t *testing.T) {
105105
setupMocks: func(mockClient *mockclient.MockWithWatch, mockResourceWriter *mockclient.MockSubResourceWriter,
106106
mockCache *mockcache.MockCache, mockRepo *mockrepo.MockRepository) {
107107
mockClient.On("List", mock.Anything, mock.Anything).Return(nil)
108+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(nil)
108109
mockCache.On("OpenRepository", mock.Anything, mock.AnythingOfType(v1alpha1Repo), mock.Anything).Return(mockRepo, nil)
109110
mockClient.On("Status").Return(mockResourceWriter)
110111
mockResourceWriter.On("Update", mock.Anything, mock.Anything).Return(nil)
@@ -131,6 +132,7 @@ func TestBackgroundHandleRepositoryEvent(t *testing.T) {
131132
setupMocks: func(mockClient *mockclient.MockWithWatch, mockResourceWriter *mockclient.MockSubResourceWriter,
132133
mockCache *mockcache.MockCache, mockRepo *mockrepo.MockRepository) {
133134
mockClient.On("List", mock.Anything, mock.Anything).Return(nil)
135+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(nil)
134136
mockCache.On("OpenRepository", mock.Anything, mock.AnythingOfType(v1alpha1Repo), mock.Anything).Return(mockRepo, nil)
135137
mockClient.On("Status").Return(mockResourceWriter)
136138
mockResourceWriter.On("Update", mock.Anything, mock.Anything).Return(nil)
@@ -179,6 +181,30 @@ func TestBackgroundHandleRepositoryEvent(t *testing.T) {
179181
},
180182
expectedError: errors.New(listErr),
181183
},
184+
{
185+
name: "Repository connectivity check failed",
186+
event: watch.Added,
187+
setupMocks: func(mockClient *mockclient.MockWithWatch, mockResourceWriter *mockclient.MockSubResourceWriter,
188+
mockCache *mockcache.MockCache, mockRepo *mockrepo.MockRepository) {
189+
mockClient.On("List", mock.Anything, mock.Anything).Return(nil)
190+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(fmt.Errorf("remote not found"))
191+
mockClient.On("Status").Return(mockResourceWriter)
192+
mockResourceWriter.On("Update", mock.Anything, mock.Anything).
193+
Run(func(args mock.Arguments) {
194+
repo := args.Get(1).(*configapi.Repository)
195+
assert.Len(t, repo.Status.Conditions, 1)
196+
assert.Equal(t, v1.ConditionFalse, repo.Status.Conditions[0].Status)
197+
assert.Equal(t, configapi.ReasonError, repo.Status.Conditions[0].Reason)
198+
assert.Contains(t, repo.Status.Conditions[0].Message, "remote not found")
199+
}).Return(nil)
200+
mockClient.On("Get", mock.Anything, mock.AnythingOfType("types.NamespacedName"), mock.AnythingOfType("*v1alpha1.Repository")).
201+
Run(func(args mock.Arguments) {
202+
repo := args.Get(2).(*configapi.Repository)
203+
repo.Status.Conditions = []v1.Condition{}
204+
}).
205+
Return(nil)
206+
},
207+
},
182208
}
183209

184210
for _, tt := range tests {
@@ -200,8 +226,10 @@ func TestBackgroundHandleRepositoryEvent(t *testing.T) {
200226
case "Invalid repository":
201227
repository = createRepo(2, 1, false)
202228
repository.Spec.Git.Directory = "invalid//directory"
229+
case "Successfully add repository event", "Successfully modified repository event":
230+
repository = createRepo(2, 1, false)
203231
default:
204-
repository = createRepo(2, 1, false) // specChanged returns true
232+
repository = createRepo(2, 1, false)
205233
}
206234
tt.setupMocks(mockClient, mockResourceWriter, mockCache, mockRepo)
207235

@@ -239,9 +267,16 @@ func TestBackgroundRunOnce(t *testing.T) {
239267
repoList := args.Get(1).(*configapi.RepositoryList)
240268
*repoList = *repositories
241269
}).Return(nil)
270+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(nil)
242271
mockCache.On("OpenRepository", mock.Anything, mock.AnythingOfType(v1alpha1Repo), mock.Anything).Return(mockRepo, nil)
243272
mockClient.On("Status").Return(mockResourceWriter)
244-
mockResourceWriter.On("Update", mock.Anything, mock.Anything).Return(nil)
273+
mockResourceWriter.On("Update", mock.Anything, mock.Anything).
274+
Run(func(args mock.Arguments) {
275+
repo := args.Get(1).(*configapi.Repository)
276+
assert.Len(t, repo.Status.Conditions, 1)
277+
assert.Equal(t, v1.ConditionTrue, repo.Status.Conditions[0].Status)
278+
assert.Equal(t, configapi.ReasonReady, repo.Status.Conditions[0].Reason)
279+
}).Return(nil)
245280
mockClient.On("Get", mock.Anything, mock.AnythingOfType("types.NamespacedName"), mock.AnythingOfType("*v1alpha1.Repository")).
246281
Run(func(args mock.Arguments) {
247282
repo := args.Get(2).(*configapi.Repository)
@@ -267,10 +302,43 @@ func TestBackgroundRunOnce(t *testing.T) {
267302
repoList := args.Get(1).(*configapi.RepositoryList)
268303
*repoList = *repositories
269304
}).Return(nil)
305+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(nil)
270306
mockCache.On("OpenRepository", mock.Anything, mock.AnythingOfType(v1alpha1Repo), mock.Anything).
271307
Return(nil, fmt.Errorf("failed to cache"))
272308
mockClient.On("Status").Return(mockResourceWriter)
273-
mockResourceWriter.On("Update", mock.Anything, mock.Anything).Return(nil)
309+
mockResourceWriter.On("Update", mock.Anything, mock.Anything).
310+
Run(func(args mock.Arguments) {
311+
repo := args.Get(1).(*configapi.Repository)
312+
assert.Len(t, repo.Status.Conditions, 1)
313+
assert.Equal(t, v1.ConditionFalse, repo.Status.Conditions[0].Status)
314+
assert.Equal(t, configapi.ReasonError, repo.Status.Conditions[0].Reason)
315+
}).Return(nil)
316+
mockClient.On("Get", mock.Anything, mock.AnythingOfType("types.NamespacedName"), mock.AnythingOfType("*v1alpha1.Repository")).
317+
Run(func(args mock.Arguments) {
318+
repo := args.Get(2).(*configapi.Repository)
319+
repo.Status.Conditions = []v1.Condition{}
320+
}).
321+
Return(nil)
322+
},
323+
},
324+
{
325+
name: "Repository connectivity check failed",
326+
setupMocks: func(mockClient *mockclient.MockWithWatch, mockResourceWriter *mockclient.MockSubResourceWriter,
327+
mockCache *mockcache.MockCache, mockRepo *mockrepo.MockRepository) {
328+
mockClient.On("List", mock.Anything, mock.AnythingOfType(v1alpha1RepoList)).Run(func(args mock.Arguments) {
329+
repoList := args.Get(1).(*configapi.RepositoryList)
330+
*repoList = *repositories
331+
}).Return(nil)
332+
mockCache.On("CheckRepositoryConnectivity", mock.Anything, mock.AnythingOfType(v1alpha1Repo)).Return(fmt.Errorf("remote not found"))
333+
mockClient.On("Status").Return(mockResourceWriter)
334+
mockResourceWriter.On("Update", mock.Anything, mock.Anything).
335+
Run(func(args mock.Arguments) {
336+
repo := args.Get(1).(*configapi.Repository)
337+
assert.Len(t, repo.Status.Conditions, 1)
338+
assert.Equal(t, v1.ConditionFalse, repo.Status.Conditions[0].Status)
339+
assert.Equal(t, configapi.ReasonError, repo.Status.Conditions[0].Reason)
340+
assert.Contains(t, repo.Status.Conditions[0].Message, "remote not found")
341+
}).Return(nil)
274342
mockClient.On("Get", mock.Anything, mock.AnythingOfType("types.NamespacedName"), mock.AnythingOfType("*v1alpha1.Repository")).
275343
Run(func(args mock.Arguments) {
276344
repo := args.Get(2).(*configapi.Repository)
@@ -556,7 +624,10 @@ func createRepo(gen int64, observedGen int64, conditionsNil bool) *configapi.Rep
556624
Generation: gen,
557625
},
558626
Spec: configapi.RepositorySpec{
627+
Type: configapi.RepositoryTypeGit,
559628
Git: &configapi.GitRepository{
629+
Repo: "https://mock-git-server.com/repo.git",
630+
Branch: "main",
560631
Directory: "/valid/path",
561632
},
562633
},

‎test/mockery/mocks/porch/pkg/cache/types/mock_Cache.go‎

Lines changed: 57 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)