Skip to content

Commit 2913fa4

Browse files
estebanreylestebanreyl
authored
Optimize purge for oci manifests, indexes and docker manifest list scenarios through improved concurrency (#462)
* Begin deconstructing purge and making it better * get the concurrent fetches clkeaned up and the untagger cleaned up a bit * begin accounting for unit tests * Add fixes for broken existing scenarios and update tests to account for concurrency * Try to get past 404 errors. Need to test this * Introduce new pool mechanism and add nested index support * Remove purger abstractions and move to unified pond pools throughout * add dryrun functionality back * update tests for reliability and status track * dry-run fix and more logs * add info * fix merge mishap * cleanup of todos and debug logs * Remove one unecessary manifest call per oci index referrer * update the purger to hold a larger backlog. Fixed bug with dry run not including untagged manifest sin purg eoutput * Fix dry-run logic conditional on no dry run mode and fix lint and gosec errors * Account for concurrency in tests * fix tests and update annotate count to the correct number * Address most comments * Add issues to project * Improve candidate iteration and add missing comments
1 parent 6ba8b11 commit 2913fa4

32 files changed

+3045
-790
lines changed

‎cmd/acr/annotate.go‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -192,12 +192,13 @@ func annotateTags(ctx context.Context,
192192
return -1, err
193193
}
194194
lastTag = newLastTag
195-
if manifestsToAnnotate != nil {
196-
count := len(*manifestsToAnnotate)
195+
count := len(manifestsToAnnotate)
196+
if count > 0 {
197197
if !dryRun {
198-
_, annotateErr := annotator.Annotate(ctx, manifestsToAnnotate)
198+
annotated, annotateErr := annotator.Annotate(ctx, manifestsToAnnotate)
199199
if annotateErr != nil {
200-
return -1, annotateErr
200+
annotatedTagsCount += annotated
201+
return annotatedTagsCount, annotateErr
201202
}
202203
}
203204
annotatedTagsCount += count
@@ -219,7 +220,7 @@ func getManifestsToAnnotate(ctx context.Context,
219220
loginURL string,
220221
repoName string,
221222
filter *regexp2.Regexp,
222-
lastTag string, artifactType string, dryRun bool) (*[]string, string, error) {
223+
lastTag string, artifactType string, dryRun bool) ([]string, string, error) {
223224

224225
resultTags, err := acrClient.GetAcrTags(ctx, repoName, "timedesc", lastTag)
225226
if err != nil {
@@ -264,7 +265,7 @@ func getManifestsToAnnotate(ctx context.Context,
264265
}
265266

266267
newLastTag = common.GetLastTagFromResponse(resultTags)
267-
return &manifestsToAnnotate, newLastTag, nil
268+
return manifestsToAnnotate, newLastTag, nil
268269
}
269270
return nil, "", nil
270271
}
@@ -287,7 +288,7 @@ func annotateUntaggedManifests(ctx context.Context,
287288
// Contrary to getTagsToAnnotate, getManifests gets all the manifests at once.
288289
// This was done because if there is a manifest that has no tag but is referenced by a multiarch manifest that has tags then it
289290
// should not be annotated.
290-
manifestsToAnnotate, err := common.GetUntaggedManifests(ctx, acrClient, loginURL, repoName, dryRun, false)
291+
manifestsToAnnotate, err := common.GetUntaggedManifests(ctx, poolSize, acrClient, repoName, true, nil, dryRun)
291292
if err != nil {
292293
return -1, err
293294
}
@@ -302,11 +303,12 @@ func annotateUntaggedManifests(ctx context.Context,
302303
}
303304
manifestsCount, annotateErr := annotator.Annotate(ctx, manifestsToAnnotate)
304305
if annotateErr != nil {
305-
return manifestsCount, annotateErr
306+
annotatedManifestsCount += manifestsCount
307+
return annotatedManifestsCount, annotateErr
306308
}
307309
annotatedManifestsCount += manifestsCount
308310
} else {
309-
annotatedManifestsCount = len(*manifestsToAnnotate)
311+
annotatedManifestsCount = len(manifestsToAnnotate)
310312
}
311313

312314
return annotatedManifestsCount, nil

‎cmd/acr/annotate_test.go‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ func TestGetManifeststoAnnotate(t *testing.T) {
258258
ref = fmt.Sprintf("%s/%s:%s", testLoginURL, testRepo, tagNameWithLoad)
259259
mockOrasClient.On("DiscoverLifecycleAnnotation", mock.Anything, ref, testArtifactType).Return(false, nil).Once()
260260
tagsToAnnotate, testLastTag, err := getManifestsToAnnotate(testCtx, mockClient, mockOrasClient, testLoginURL, testRepo, tagRegex, "", testArtifactType, false)
261-
assert.Equal(4, len(*tagsToAnnotate), "Number of tags to annotate should be 1")
261+
assert.Equal(4, len(tagsToAnnotate), "Number of tags to annotate should be 1")
262262
assert.Equal("", testLastTag, "Last tag should be empty")
263263
assert.Equal(nil, err, "Error should be nil")
264264
mockClient.AssertExpectations(t)
@@ -331,6 +331,8 @@ func TestAnnotateManifests(t *testing.T) {
331331
mockOrasClient := &mocks.ORASClientInterface{}
332332
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleMultiArchManifestV2WithTagsResult, nil).Once()
333333
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, errors.New("error getting manifest")).Once()
334+
// Despite the failure, the GetAcrManifests method may be called again before the failure happens
335+
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, nil).Maybe()
334336
annotatedManifests, err := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], false)
335337
assert.Equal(-1, annotatedManifests, "Number of annotated elements should be -1")
336338
assert.NotEqual(nil, err, "Error should not be nil")
@@ -345,6 +347,8 @@ func TestAnnotateManifests(t *testing.T) {
345347
mockOrasClient := &mocks.ORASClientInterface{}
346348
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleMultiArchManifestV2WithTagsResult, nil).Once()
347349
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, errors.New("error getting manifest")).Once()
350+
// Despite the failure, the GetAcrManifests method may be called again before the failure happens
351+
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, nil).Maybe()
348352
annotatedManifests, err := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], false)
349353
assert.Equal(-1, annotatedManifests, "Number of annotated elements should be -1")
350354
assert.NotEqual(nil, err, "Error should not be nil")
@@ -396,11 +400,11 @@ func TestAnnotateManifests(t *testing.T) {
396400
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:2830cc0fcddc1bc2bd4aeab0ed5ee7087dab29a49e65151c77553e46a7ed5283").Return(doubleManifestV2WithoutTagsResult, nil).Once()
397401
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:6305e31b9b0081d2532397a1e08823f843f329a7af2ac98cb1d7f0355a3e3696").Return(EmptyListManifestsResult, nil).Once()
398402
ref := fmt.Sprintf("%s/%s@sha256:63532043b5af6247377a472ad075a42bde35689918de1cf7f807714997e0e683", testLoginURL, testRepo)
399-
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Once()
403+
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Maybe() // Depending on scheduling, this may not be invoked
400404
ref = fmt.Sprintf("%s/%s@sha256:6305e31b9b0081d2532397a1e08823f843f329a7af2ac98cb1d7f0355a3e3696", testLoginURL, testRepo)
401405
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(errors.New("manifest not found")).Once()
402406
annotatedManifests, err := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], false)
403-
assert.Equal(1, annotatedManifests, "Number of annotated elements should be 1")
407+
assert.True(annotatedManifests == 1 || annotatedManifests == 0, "Number of annotated elements should be 1 or 0")
404408
assert.NotEqual(nil, err, "Error should not be nil")
405409
mockClient.AssertExpectations(t)
406410
mockOrasClient.AssertExpectations(t)
@@ -417,9 +421,9 @@ func TestAnnotateManifests(t *testing.T) {
417421
ref := fmt.Sprintf("%s/%s@sha256:63532043b5af6247377a472ad075a42bde35689918de1cf7f807714997e0e683", testLoginURL, testRepo)
418422
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(errors.New("error annotating manifest")).Once()
419423
ref = fmt.Sprintf("%s/%s@sha256:6305e31b9b0081d2532397a1e08823f843f329a7af2ac98cb1d7f0355a3e3696", testLoginURL, testRepo)
420-
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Once()
424+
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Maybe()
421425
annotatedManifests, err := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], false)
422-
assert.Equal(1, annotatedManifests, "Number of annotated elements should be 1")
426+
assert.True(annotatedManifests == 1 || annotatedManifests == 0, "Number of annotated elements should be 1 or 0")
423427
assert.NotEqual(nil, err, "Error should not be nil")
424428
mockClient.AssertExpectations(t)
425429
mockOrasClient.AssertExpectations(t)
@@ -435,11 +439,11 @@ func TestAnnotateManifests(t *testing.T) {
435439
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:2830cc0fcddc1bc2bd4aeab0ed5ee7087dab29a49e65151c77553e46a7ed5283").Return(doubleManifestV2WithoutTagsResult, nil).Once()
436440
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:6305e31b9b0081d2532397a1e08823f843f329a7af2ac98cb1d7f0355a3e3696").Return(EmptyListManifestsResult, nil).Once()
437441
ref := fmt.Sprintf("%s/%s@sha256:63532043b5af6247377a472ad075a42bde35689918de1cf7f807714997e0e683", testLoginURL, testRepo)
438-
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Once()
442+
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(nil).Maybe()
439443
ref = fmt.Sprintf("%s/%s@sha256:6305e31b9b0081d2532397a1e08823f843f329a7af2ac98cb1d7f0355a3e3696", testLoginURL, testRepo)
440444
mockOrasClient.On("Annotate", mock.Anything, ref, testArtifactType, annotationMap).Return(errors.New("error annotating manifest")).Once()
441445
annotatedManifests, err := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], false)
442-
assert.Equal(1, annotatedManifests, "Number of annotated elements should be 1")
446+
assert.True(annotatedManifests == 1 || annotatedManifests == 0, "Number of annotated elements should be 1 or 0, this is affected by the concurrent nature of the code")
443447
assert.NotEqual(nil, err, "Error should not be nil")
444448
mockClient.AssertExpectations(t)
445449
mockOrasClient.AssertExpectations(t)
@@ -610,6 +614,7 @@ func TestDryRunAnnotate(t *testing.T) {
610614
mockOrasClient := &mocks.ORASClientInterface{}
611615
mockClient.On("GetAcrTags", mock.Anything, testRepo, "timedesc", "").Return(FourTagsResult, nil).Once()
612616
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleMultiArchManifestV2WithTagsResult, nil).Once()
617+
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", multiArchDigest).Return(EmptyListManifestsResult, nil).Maybe() // This is to ensure that the GetAcrManifests method may be called again before the failure happens
613618
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, errors.New("error getting manifest")).Once()
614619
annotatedTags, err := annotateTags(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], "^lat.*", defaultRegexpMatchTimeoutSeconds, true)
615620
annotatedManifests, errManifests := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], true)
@@ -627,6 +632,7 @@ func TestDryRunAnnotate(t *testing.T) {
627632
mockOrasClient := &mocks.ORASClientInterface{}
628633
mockClient.On("GetAcrTags", mock.Anything, testRepo, "timedesc", "").Return(FourTagsResult, nil).Once()
629634
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleMultiArchManifestV2WithTagsResult, nil).Once()
635+
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", multiArchDigest).Return(nil, nil).Maybe() // This is to ensure that the GetAcrManifests method may be called again before the failure happens
630636
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return([]byte("invalid json"), nil).Once()
631637
annotatedTags, err := annotateTags(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], "^lat.*", defaultRegexpMatchTimeoutSeconds, true)
632638
annotatedManifests, errManifests := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], true)
@@ -644,7 +650,8 @@ func TestDryRunAnnotate(t *testing.T) {
644650
mockOrasClient := &mocks.ORASClientInterface{}
645651
mockClient.On("GetAcrTags", mock.Anything, testRepo, "timedesc", "").Return(FourTagsResult, nil).Once()
646652
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "").Return(singleMultiArchManifestV2WithTagsResult, nil).Once()
647-
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(multiArchManifestV2Bytes, nil).Once()
653+
mockClient.On("GetManifest", mock.Anything, testRepo, "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(multiArchManifestV2Bytes, nil).Maybe() // This may not be invoked if the
654+
// GetAcrManifests call fails first.
648655
mockClient.On("GetAcrManifests", mock.Anything, testRepo, "", "sha256:d88fb54ba4424dada7c928c6af332ed1c49065ad85eafefb6f26664695015119").Return(nil, errors.New("error fetching manifests")).Once()
649656
annotatedTags, err := annotateTags(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], "^lat.*", defaultRegexpMatchTimeoutSeconds, true)
650657
annotatedManifests, errManifests := annotateUntaggedManifests(testCtx, mockClient, mockOrasClient, defaultPoolSize, testLoginURL, testRepo, testArtifactType, testAnnotations[:], true)

0 commit comments

Comments
 (0)