git clone optimization when artifact in storage

Update the GitRepositoryReconciler to attempt git clone optimization
only when an artifact is already present in storage and do not skip
reconciliation. When a partial commit is received, due to no-op clone,
copy the existing artifact to the source build directory and populate
all the associated fields in reconcileSource(). This ensure that the
reconciliation is not skipped and allows other subreconcilers to operate
on other parts of the GitRepo object like include, ignore, etc, when
attributes related to them change but the remote repository has not
changed.

Remove git.NoChangeError as no-op clone now returns a partial commit and
no error.

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

api/v1beta2/condition_types.go

@@ -100,4 +100,7 @@ const (
 
 	// CacheOperationFailedReason signals a failure in cache operation.
 	CacheOperationFailedReason string = "CacheOperationFailed"
+
+	// CopyOperationFailedReason signals a failure in copying files.
+	CopyOperationFailedReason string = "CopyOperationFailed"
 )

controllers/gitrepository_controller.go

@@ -304,7 +304,14 @@ func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository,
 			oldChecksum = oldObj.GetArtifact().Checksum
 		}
 
-		message := fmt.Sprintf("stored artifact for commit '%s'", commit.ShortMessage())
+		// A partial commit due to no-op clone doesn't contain the commit
+		// message information. Have separate message for it.
+		var message string
+		if git.IsConcreteCommit(commit) {
+			message = fmt.Sprintf("stored artifact for commit '%s'", commit.ShortMessage())
+		} else {
+			message = fmt.Sprintf("stored artifact for commit hash '%s'", commit.Hash)
+		}
 
 		// Notify on new artifact and failure recovery.
 		if oldChecksum != newObj.GetArtifact().Checksum {
@@ -425,8 +432,13 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 	}
 
 	if val, ok := r.features[features.OptimizedGitClones]; ok && val {
-		if artifact := obj.GetArtifact(); artifact != nil {
-			checkoutOpts.LastRevision = artifact.Revision
+		// Only if the object has an existing artifact in storage, attempt to
+		// short-circuit clone operation. reconcileStorage has already verified
+		// that the artifact exists.
+		if conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) {
+			if artifact := obj.GetArtifact(); artifact != nil {
+				checkoutOpts.LastRevision = artifact.Revision
+			}
 		}
 	}
 
@@ -474,12 +486,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 	defer cancel()
 	c, err := checkoutStrategy.Checkout(gitCtx, dir, repositoryURL, authOpts)
 	if err != nil {
-		var v git.NoChangesError
-		if errors.As(err, &v) {
-			return sreconcile.ResultSuccess,
-				&serror.Waiting{Err: v, Reason: v.Message, RequeueAfter: obj.GetRequeueAfter()}
-		}
-
 		e := &serror.Event{
 			Err:    fmt.Errorf("failed to checkout and determine revision: %w", err),
 			Reason: sourcev1.GitOperationFailedReason,
@@ -490,8 +496,40 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 	}
 	// Assign the commit to the shared commit reference.
 	*commit = *c
+
+	// If it's a partial commit obtained from an existing artifact, copy the
+	// artifact content into new source directory.
+	if !git.IsConcreteCommit(*commit) {
+		ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info(fmt.Sprintf(
+			"no changes since last reconciliation, observed revision '%s'", commit.String()))
+
+		// Remove the target directory, as CopyToPath() renames another
+		// directory to which the artifact is unpacked into the target
+		// directory. At this point, the target directory is empty, safe to
+		// remove.
+		if err := os.RemoveAll(dir); err != nil {
+			ctrl.LoggerFrom(ctx).Error(err, "failed to remove source build directory")
+		}
+		if err := r.Storage.CopyToPath(obj.GetArtifact(), "/", dir); err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("failed to copy existing artifact to source dir: %w", err),
+				Reason: sourcev1.CopyOperationFailedReason,
+			}
+			conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
+			return sreconcile.ResultEmpty, e
+		}
+		conditions.Delete(obj, sourcev1.FetchFailedCondition)
+		conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
+
+		return sreconcile.ResultSuccess, nil
+	}
+
 	ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info("git repository checked out", "url", obj.Spec.URL, "revision", commit.String())
 	conditions.Delete(obj, sourcev1.FetchFailedCondition)
+	// In case no-op clone resulted in a failure and in the subsequent
+	// reconciliation a new remote revision was observed, delete any stale
+	// StorageOperationFailedCondition.
+	conditions.Delete(obj, sourcev1.StorageOperationFailedCondition)
 
 	// Verify commit signature
 	if result, err := r.verifyCommitSignature(ctx, obj, *commit); err != nil || result == sreconcile.ResultEmpty {

controllers/gitrepository_controller_test.go

@@ -547,30 +547,35 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 		name                  string
 		skipForImplementation string
 		reference             *sourcev1.GitRepositoryRef
+		beforeFunc            func(obj *sourcev1.GitRepository, latestRev string)
 		want                  sreconcile.Result
 		wantErr               bool
 		wantRevision          string
+		wantArtifactOutdated  bool
 	}{
 		{
-			name:         "Nil reference (default branch)",
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "master/<commit>",
+			name:                 "Nil reference (default branch)",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "master/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name: "Branch",
 			reference: &sourcev1.GitRepositoryRef{
 				Branch: "staging",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "staging/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "staging/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name: "Tag",
 			reference: &sourcev1.GitRepositoryRef{
 				Tag: "v0.1.0",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "v0.1.0/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "v0.1.0/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name:                  "Branch commit",
@@ -579,8 +584,9 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 				Branch: "staging",
 				Commit: "<commit>",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "staging/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "staging/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name:                  "Branch commit",
@@ -589,32 +595,56 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 				Branch: "staging",
 				Commit: "<commit>",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "HEAD/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "HEAD/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name: "SemVer",
 			reference: &sourcev1.GitRepositoryRef{
 				SemVer: "*",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "v2.0.0/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "v2.0.0/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name: "SemVer range",
 			reference: &sourcev1.GitRepositoryRef{
 				SemVer: "<v0.2.1",
 			},
-			want:         sreconcile.ResultSuccess,
-			wantRevision: "0.2.0/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "0.2.0/<commit>",
+			wantArtifactOutdated: true,
 		},
 		{
 			name: "SemVer prerelease",
 			reference: &sourcev1.GitRepositoryRef{
 				SemVer: ">=1.0.0-0 <1.1.0-0",
 			},
-			wantRevision: "v1.0.0-alpha/<commit>",
-			want:         sreconcile.ResultSuccess,
+			wantRevision:         "v1.0.0-alpha/<commit>",
+			want:                 sreconcile.ResultSuccess,
+			wantArtifactOutdated: true,
+		},
+		{
+			name: "Optimized clone",
+			reference: &sourcev1.GitRepositoryRef{
+				Branch: "staging",
+			},
+			beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) {
+				// Add existing artifact on the object and storage.
+				obj.Status = sourcev1.GitRepositoryStatus{
+					Artifact: &sourcev1.Artifact{
+						Revision: "staging/" + latestRev,
+						Path:     randStringRunes(10),
+					},
+				}
+				testStorage.Archive(obj.GetArtifact(), "testdata/git/repository", nil)
+				conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo")
+			},
+			want:                 sreconcile.ResultSuccess,
+			wantRevision:         "staging/<commit>",
+			wantArtifactOutdated: false,
 		},
 	}
 
@@ -677,6 +707,10 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 					obj := obj.DeepCopy()
 					obj.Spec.GitImplementation = i
 
+					if tt.beforeFunc != nil {
+						tt.beforeFunc(obj, headRef.Hash().String())
+					}
+
 					var commit git.Commit
 					var includes artifactSet
 					got, err := r.reconcileSource(ctx, obj, &commit, &includes, tmpDir)
@@ -685,10 +719,10 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
 					}
 					g.Expect(err != nil).To(Equal(tt.wantErr))
 					g.Expect(got).To(Equal(tt.want))
-					if tt.wantRevision != "" {
+					if tt.wantRevision != "" && !tt.wantErr {
 						revision := strings.ReplaceAll(tt.wantRevision, "<commit>", headRef.Hash().String())
 						g.Expect(commit.String()).To(Equal(revision))
-						g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(BeTrue())
+						g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(Equal(tt.wantArtifactOutdated))
 					}
 				})
 			}
@@ -1782,10 +1816,20 @@ func TestGitRepositoryReconciler_statusConditions(t *testing.T) {
 }
 
 func TestGitRepositoryReconciler_notify(t *testing.T) {
+	concreteCommit := git.Commit{
+		Hash:    git.Hash("some-hash"),
+		Message: "test commit",
+		Encoded: []byte("commit content"),
+	}
+	partialCommit := git.Commit{
+		Hash: git.Hash("some-hash"),
+	}
+
 	tests := []struct {
 		name             string
 		res              sreconcile.Result
 		resErr           error
+		commit           git.Commit
 		oldObjBeforeFunc func(obj *sourcev1.GitRepository)
 		newObjBeforeFunc func(obj *sourcev1.GitRepository)
 		wantEvent        string
@@ -1799,6 +1843,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 			name:   "new artifact",
 			res:    sreconcile.ResultSuccess,
 			resErr: nil,
+			commit: concreteCommit,
 			newObjBeforeFunc: func(obj *sourcev1.GitRepository) {
 				obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"}
 			},
@@ -1808,6 +1853,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 			name:   "recovery from failure",
 			res:    sreconcile.ResultSuccess,
 			resErr: nil,
+			commit: concreteCommit,
 			oldObjBeforeFunc: func(obj *sourcev1.GitRepository) {
 				obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"}
 				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "fail")
@@ -1823,6 +1869,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 			name:   "recovery and new artifact",
 			res:    sreconcile.ResultSuccess,
 			resErr: nil,
+			commit: concreteCommit,
 			oldObjBeforeFunc: func(obj *sourcev1.GitRepository) {
 				obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"}
 				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "fail")
@@ -1838,6 +1885,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 			name:   "no updates",
 			res:    sreconcile.ResultSuccess,
 			resErr: nil,
+			commit: concreteCommit,
 			oldObjBeforeFunc: func(obj *sourcev1.GitRepository) {
 				obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"}
 				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready")
@@ -1847,6 +1895,22 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready")
 			},
 		},
+		{
+			name:   "recovery with no-op clone commit",
+			res:    sreconcile.ResultSuccess,
+			resErr: nil,
+			commit: partialCommit,
+			oldObjBeforeFunc: func(obj *sourcev1.GitRepository) {
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"}
+				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.GitOperationFailedReason, "fail")
+				conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "foo")
+			},
+			newObjBeforeFunc: func(obj *sourcev1.GitRepository) {
+				obj.Status.Artifact = &sourcev1.Artifact{Revision: "aaa", Checksum: "bbb"}
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready")
+			},
+			wantEvent: "Normal NewArtifact stored artifact for commit hash",
+		},
 	}
 
 	for _, tt := range tests {
@@ -1868,10 +1932,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
 				EventRecorder: recorder,
 				features:      features.FeatureGates(),
 			}
-			commit := &git.Commit{
-				Message: "test commit",
-			}
-			reconciler.notify(oldObj, newObj, *commit, tt.res, tt.resErr)
+			reconciler.notify(oldObj, newObj, tt.commit, tt.res, tt.resErr)
 
 			select {
 			case x, ok := <-recorder.Events:

docs/spec/v1beta2/gitrepositories.md

@@ -406,8 +406,8 @@ reconciliations. It supports both `go-git` and `libgit2` implementations
 when cloning repositories using branches or tags.
 
 When enabled, avoids full clone operations by first checking whether
-the last revision is still the same at the target repository,
-and if that is so, skips the reconciliation.
+the revision of the last stored artifact is still the head of the remote
+repository, and if that is so, the local artifact is reused.
 
 This feature is enabled by default. It can be disabled by starting the
 controller with the argument `--feature-gates=OptimizedGitClones=false`.

pkg/git/git.go

@@ -107,18 +107,6 @@ type CheckoutStrategy interface {
 	Checkout(ctx context.Context, path, url string, config *AuthOptions) (*Commit, error)
 }
 
-// NoChangesError represents the case in which a Git clone operation
-// is attempted, but cancelled as the revision is still the same as
-// the one observed on the last successful reconciliation.
-type NoChangesError struct {
-	Message          string
-	ObservedRevision string
-}
-
-func (e NoChangesError) Error() string {
-	return fmt.Sprintf("%s: observed revision '%s'", e.Message, e.ObservedRevision)
-}
-
 // IsConcreteCommit returns if a given commit is a concrete commit. Concrete
 // commits have most of commit metadata and commit content. In contrast, a
 // partial commit may only have some metadata and no commit content.