From ddd26f4d52e70028b15ad4d7295b4298121c40ad Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 05:37:01 +0530 Subject: [PATCH 1/7] Introduce Generic error and error Config Generic error is an attempt to avoid creating new error type for every new unique scenario. It can be used to configure and build custom error handling behavior, logging and event recording at present. Contextual errors, Stalling and Waiting error, have special meaning for the reconciliation results. But the Event error type can be replaced with Generic error with some specific configurations. The Event error is kept for a gradual migation to Generic error. Similarly, the Generic error can be used to easily create new error handling behaviors. The error Config can be used to configure any of the errors, including contextual errors, without altering their contextual meaning, to modify how they are handled. The error constructors configure the errors with common default configurations. These configurations can be modified to alter the behavior. Signed-off-by: Sunny --- internal/error/error.go | 118 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 114 insertions(+), 4 deletions(-) diff --git a/internal/error/error.go b/internal/error/error.go index 4333c4603..0852ba412 100644 --- a/internal/error/error.go +++ b/internal/error/error.go @@ -16,16 +16,53 @@ limitations under the License. package error -import "time" +import ( + "time" + + corev1 "k8s.io/api/core/v1" +) + +// EventTypeNone indicates no error event. It can be used to disable error +// events. +const EventTypeNone = "None" + +// Config is the error configuration. It is embedded in the errors and can be +// used to configure how the error should be handled. These configurations +// mostly define actions to be taken on the errors. Not all the configurations +// may apply to every error. +type Config struct { + // Event is the event type of an error. It is used to configure what type of + // event an error should result in. + // Valid values: + // - EventTypeNone + // - corev1.EventTypeNormal + // - corev1.EventTypeWarning + Event string + // Log is used to configure if an error should be logged. The log level is + // derived from the Event type. + // None event - info log + // Normal event - info log + // Warning event - error log + Log bool + // Notification is used to emit an error as a notification alert to a + // a notification service. + Notification bool + // Ignore is used to suppress the error for no-op reconciliations. It may + // be applicable to non-contextual errors only. + Ignore bool +} // Stalling is the reconciliation stalled state error. It contains an error -// and a reason for the stalled condition. +// and a reason for the stalled condition. It is a contextual error, used to +// express the scenario which contributed to the reconciliation result. type Stalling struct { // Reason is the stalled condition reason string. Reason string // Err is the error that caused stalling. This can be used as the message in // stalled condition. Err error + // Config is the error handler configuration. + Config } // Error implements error interface. @@ -38,8 +75,26 @@ func (se *Stalling) Unwrap() error { return se.Err } +// NewStalling constructs a new Stalling error with default configuration. +func NewStalling(err error, reason string) *Stalling { + // Stalling errors are not returned to the runtime. Log it explicitly. + // Since this failure requires user interaction, send warning notification. + return &Stalling{ + Reason: reason, + Err: err, + Config: Config{ + Event: corev1.EventTypeWarning, + Log: true, + Notification: true, + }, + } +} + // Event is an error event. It can be used to construct an event to be // recorded. +// Deprecated: use Generic error with NewGeneric() for the same behavior and +// replace the RecordContextualError with ErrorActionHandler for result +// processing. type Event struct { // Reason is the reason for the event error. Reason string @@ -58,7 +113,10 @@ func (ee *Event) Unwrap() error { } // Waiting is the reconciliation wait state error. It contains an error, wait -// duration and a reason for the wait. +// duration and a reason for the wait. It is a contextual error, used to express +// the scenario which contributed to the reconciliation result. +// It is for scenarios where a reconciliation needs to wait for something else +// to take place first. type Waiting struct { // RequeueAfter is the wait duration after which to requeue. RequeueAfter time.Duration @@ -66,9 +124,11 @@ type Waiting struct { Reason string // Err is the error that caused the wait. Err error + // Config is the error handler configuration. + Config } -// Error implement error interface. +// Error implements error interface. func (we *Waiting) Error() string { return we.Err.Error() } @@ -77,3 +137,53 @@ func (we *Waiting) Error() string { func (we *Waiting) Unwrap() error { return we.Err } + +// NewWaiting constructs a new Waiting error with default configuration. +func NewWaiting(err error, reason string) *Waiting { + // Waiting errors are not returned to the runtime. Log it explicitly. + // Since this failure results in reconciliation delay, send warning + // notification. + return &Waiting{ + Reason: reason, + Err: err, + Config: Config{ + Event: corev1.EventTypeNormal, + Log: true, + }, + } +} + +// Generic error is a generic reconcile error. It can be used in scenarios that +// don't have any special contextual meaning. +type Generic struct { + // Reason is the reason for the generic error. + Reason string + // Error is the error that caused the generic error. + Err error + // Config is the error handler configuration. + Config +} + +// Error implements error interface. +func (g *Generic) Error() string { + return g.Err.Error() +} + +// Unwrap returns the underlying error. +func (g *Generic) Unwrap() error { + return g.Err +} + +// NewGeneric constructs a new Generic error with default configuration. +func NewGeneric(err error, reason string) *Generic { + // Since it's a error, ensure to log and send failure notification. + return &Generic{ + Reason: reason, + Err: err, + Config: Config{ + Event: corev1.EventTypeWarning, + Log: true, + Notification: true, + }, + } +} From 30fe0dc6aa0527acf57586ab862eff586b102455 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 06:00:50 +0530 Subject: [PATCH 2/7] Introduce ErrorActionHandler ResultProcessor ErrorActionHandler processes the reconciliation error results based on their configurations. It performs actions like logging and event recording based on the error configuration. More actions can be accommodated in the future with more error configurations. It can be a replacement for RecordContextualError() which does the same operations but can't be configured much. Signed-off-by: Sunny --- internal/reconcile/summarize/processor.go | 59 +++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/internal/reconcile/summarize/processor.go b/internal/reconcile/summarize/processor.go index 54e135e47..b995d2db5 100644 --- a/internal/reconcile/summarize/processor.go +++ b/internal/reconcile/summarize/processor.go @@ -25,6 +25,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/fluxcd/pkg/apis/meta" + "github.com/fluxcd/pkg/runtime/events" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/object" "github.com/fluxcd/source-controller/internal/reconcile" @@ -64,3 +66,60 @@ func RecordReconcileReq(ctx context.Context, recorder kuberecorder.EventRecorder object.SetStatusLastHandledReconcileAt(obj, v) } } + +// ErrorActionHandler is a ResultProcessor that handles all the actions +// configured in the given error. Logging and event recording are the handled +// actions at present. As more configurations are added to serror.Config, more +// action handlers can be added here. +func ErrorActionHandler(ctx context.Context, recorder kuberecorder.EventRecorder, obj client.Object, _ reconcile.Result, err error) { + switch e := err.(type) { + case *serror.Generic: + if e.Log { + logError(ctx, e.Config.Event, e, e.Error()) + } + recordEvent(recorder, obj, e.Config.Event, e.Config.Notification, err, e.Reason) + case *serror.Waiting: + if e.Log { + logError(ctx, e.Config.Event, e, "reconciliation waiting", "reason", e.Err, "duration", e.RequeueAfter) + } + recordEvent(recorder, obj, e.Config.Event, e.Config.Notification, err, e.Reason) + case *serror.Stalling: + if e.Log { + logError(ctx, e.Config.Event, e, "reconciliation stalled") + } + recordEvent(recorder, obj, e.Config.Event, e.Config.Notification, err, e.Reason) + } +} + +// logError logs error based on the passed error configurations. +func logError(ctx context.Context, eventType string, err error, msg string, keysAndValues ...interface{}) { + switch eventType { + case corev1.EventTypeNormal, serror.EventTypeNone: + ctrl.LoggerFrom(ctx).Info(msg, keysAndValues...) + case corev1.EventTypeWarning: + ctrl.LoggerFrom(ctx).Error(err, msg, keysAndValues...) + } +} + +// recordEvent records events based on the passed error configurations. +func recordEvent(recorder kuberecorder.EventRecorder, obj client.Object, eventType string, notification bool, err error, reason string) { + if eventType == serror.EventTypeNone { + return + } + switch eventType { + case corev1.EventTypeNormal: + if notification { + // K8s native event and notification-controller event. + recorder.Eventf(obj, corev1.EventTypeNormal, reason, err.Error()) + } else { + // K8s native event only. + recorder.Eventf(obj, events.EventTypeTrace, reason, err.Error()) + } + case corev1.EventTypeWarning: + // TODO: Due to the current implementation of the event recorder, all + // the K8s warning events are also sent as notification controller + // notifications. Once the recorder becomes capable of separating the + // two, conditionally record events. + recorder.Eventf(obj, corev1.EventTypeWarning, reason, err.Error()) + } +} From 5d154a83dc12a274dd60290d255590c2205b7349 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 06:05:14 +0530 Subject: [PATCH 3/7] Introduce Generic error in reconcile Add Generic error in RuntimeResultBuilder and ComputeReconcileResult implementation with consideration to the error configurations. Safeguards are added in the runtime result builder to ensure default requeue after interval is set when is's set to zero or unset. Signed-off-by: Sunny --- internal/reconcile/reconcile.go | 24 ++++++++++++++++- internal/reconcile/reconcile_test.go | 40 ++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 9b4bd76af..b1e11409a 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -73,8 +73,19 @@ type AlwaysRequeueResultBuilder struct { // return values of a controller's Reconcile function. func (r AlwaysRequeueResultBuilder) BuildRuntimeResult(rr Result, err error) ctrl.Result { // Handle special errors that contribute to expressing the result. - if e, ok := err.(*serror.Waiting); ok { + switch e := err.(type) { + case *serror.Waiting: + // Safeguard: If no RequeueAfter is set, use the default success + // RequeueAfter value to ensure a requeue takes place after some time. + if e.RequeueAfter == 0 { + return ctrl.Result{RequeueAfter: r.RequeueAfter} + } return ctrl.Result{RequeueAfter: e.RequeueAfter} + case *serror.Generic: + // no-op error, reconcile at success interval. + if e.Ignore { + return ctrl.Result{RequeueAfter: r.RequeueAfter} + } } switch rr { @@ -132,6 +143,17 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb conditions.Delete(obj, meta.StalledCondition) // The reconciler needs to wait and retry. Return no error. return pOpts, result, nil + case *serror.Generic: + conditions.Delete(obj, meta.StalledCondition) + // If ignore, it's a no-op error, return no error, remove reconciling + // condition. + if t.Ignore { + // The current generation has been reconciled successfully with + // no-op result. Update status observed generation. + pOpts = append(pOpts, patch.WithStatusObservedGeneration{}) + conditions.Delete(obj, meta.ReconcilingCondition) + return pOpts, result, nil + } case nil: // The reconcile didn't result in any error, we are not in stalled // state. If a requeue is requested, the current generation has not been diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index a8edc5e4b..3d3f4fc0a 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -135,12 +135,46 @@ func TestComputeReconcileResult(t *testing.T) { name: "waiting error", result: ResultEmpty, recErr: &serror.Waiting{Err: fmt.Errorf("some error"), Reason: "some reason"}, - wantResult: ctrl.Result{}, + wantResult: ctrl.Result{RequeueAfter: testSuccessInterval}, wantErr: false, afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeFalse()) }, }, + { + name: "generic error, Stalled=True, remove Stalled", + result: ResultEmpty, + beforeFunc: func(obj conditions.Setter) { + conditions.MarkStalled(obj, "SomeReason", "some message") + }, + recErr: &serror.Generic{ + Err: fmt.Errorf("some error"), Reason: "some reason", + }, + wantResult: ctrl.Result{}, + afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { + t.Expect(conditions.IsUnknown(obj, meta.StalledCondition)).To(BeTrue()) + }, + wantErr: true, + }, + { + name: "generic ignore error, Reconciling=True, remove Reconciling", + result: ResultEmpty, + beforeFunc: func(obj conditions.Setter) { + conditions.MarkReconciling(obj, "NewRevision", "new revision") + }, + recErr: &serror.Generic{ + Err: fmt.Errorf("some error"), Reason: "some reason", + Config: serror.Config{ + Ignore: true, + }, + }, + wantResult: ctrl.Result{RequeueAfter: testSuccessInterval}, + afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { + t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue()) + t.Expect(conditions.IsUnknown(obj, meta.ReconcilingCondition)).To(BeTrue()) + }, + wantErr: false, + }, { name: "random error", result: ResultEmpty, @@ -188,7 +222,9 @@ func TestComputeReconcileResult(t *testing.T) { for _, o := range pOpts { o.ApplyToHelper(opts) } - tt.afterFunc(g, obj, opts) + if tt.afterFunc != nil { + tt.afterFunc(g, obj, opts) + } }) } } From 4882cea274c3003cb1b918f711e969bc2ddd6604 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 06:09:53 +0530 Subject: [PATCH 4/7] Replace Event error with Generic error in GitRepo For gradual migration to Generic error, update only the GitRepo reconciler to use Generic error. Replace the Waiting error for git no change scenario with a Generic error with proper no-op, early return, error configurations. This ensures that the no-op only results in log and K8s native events at normal level. Fixes a reconciliation issue when recovering from a failure state (with previous success state and artifact in the storage) and optimized git clone feature is on, which results in failure to persist as the git optimization prevented full reconciliation due to already existing artifact and removal of failure negative conditions on the object status. In order to allow failure recovery, the git clone optimizations are now only applied when the object is already in a ready state. Signed-off-by: Sunny --- controllers/gitrepository_controller.go | 167 +++++++++++++----------- 1 file changed, 88 insertions(+), 79 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 8f7dc84d9..eda11f3fd 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -183,7 +183,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques summarize.WithReconcileError(retErr), summarize.WithIgnoreNotFound(), summarize.WithProcessors( - summarize.RecordContextualError, + summarize.ErrorActionHandler, summarize.RecordReconcileReq, ), summarize.WithResultBuilder(sreconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.GetRequeueAfter()}), @@ -235,10 +235,10 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G // Create temp dir for Git clone tmpDir, err := util.TempDirForObj("", obj) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create temporary working directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create temporary working directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -380,10 +380,10 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } var secret corev1.Secret if err := r.Client.Get(ctx, name, &secret); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to get secret '%s': %w", name.String(), err), - Reason: sourcev1.AuthenticationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to get secret '%s': %w", name.String(), err), + sourcev1.AuthenticationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return error as the world as observed may change return sreconcile.ResultEmpty, e @@ -396,10 +396,10 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, authOpts, err = git.AuthOptionsWithoutSecret(obj.Spec.URL) } if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), - Reason: sourcev1.AuthenticationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), + sourcev1.AuthenticationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Return error as the contents of the secret may change return sreconcile.ResultEmpty, e @@ -415,8 +415,12 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } if oc, _ := features.Enabled(features.OptimizedGitClones); oc { - if artifact := obj.GetArtifact(); artifact != nil { - checkoutOpts.LastRevision = artifact.Revision + // Only if the object is ready, use the last revision to attempt + // short-circuiting clone operation. + if conditions.IsTrue(obj, meta.ReadyCondition) { + if artifact := obj.GetArtifact(); artifact != nil { + checkoutOpts.LastRevision = artifact.Revision + } } } @@ -466,14 +470,19 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, 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, - } + // Create generic error without notification. Since it's a no-op + // error, ignore (no runtime error), normal event. + ge := serror.NewGeneric(v, sourcev1.GitOperationSucceedReason) + ge.Notification = false + ge.Ignore = true + ge.Event = corev1.EventTypeNormal + return sreconcile.ResultEmpty, ge + } + + e := serror.NewGeneric( + fmt.Errorf("failed to checkout and determine revision: %w", err), + sourcev1.GitOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e @@ -531,36 +540,36 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Ensure target path exists and is a directory if f, err := os.Stat(dir); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to stat target artifact path: %w", err), - Reason: sourcev1.StatOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to stat target artifact path: %w", err), + sourcev1.StatOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } else if !f.IsDir() { - e := &serror.Event{ - Err: fmt.Errorf("invalid target path: '%s' is not a directory", dir), - Reason: sourcev1.InvalidPathReason, - } + e := serror.NewGeneric( + fmt.Errorf("invalid target path: '%s' is not a directory", dir), + sourcev1.InvalidPathReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } // Ensure artifact directory exists and acquire lock if err := r.Storage.MkdirAll(artifact); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to create artifact directory: %w", err), - Reason: sourcev1.DirCreationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("failed to create artifact directory: %w", err), + sourcev1.DirCreationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } unlock, err := r.Storage.Lock(artifact) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to acquire lock for artifact: %w", err), - Reason: meta.FailedReason, - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("failed to acquire lock for artifact: %w", err), + meta.FailedReason, + ) } defer unlock() @@ -568,10 +577,10 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, ignoreDomain := strings.Split(dir, string(filepath.Separator)) ps, err := sourceignore.LoadIgnorePatterns(dir, ignoreDomain) if err != nil { - return sreconcile.ResultEmpty, &serror.Event{ - Err: fmt.Errorf("failed to load source ignore patterns from repository: %w", err), - Reason: "SourceIgnoreError", - } + return sreconcile.ResultEmpty, serror.NewGeneric( + fmt.Errorf("failed to load source ignore patterns from repository: %w", err), + "SourceIgnoreError", + ) } if obj.Spec.Ignore != nil { ps = append(ps, sourceignore.ReadPatterns(strings.NewReader(*obj.Spec.Ignore), ignoreDomain)...) @@ -579,10 +588,10 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Archive directory to storage if err := r.Storage.Archive(&artifact, dir, SourceIgnoreFilter(ps, ignoreDomain)); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("unable to archive artifact to storage: %w", err), - Reason: sourcev1.ArchiveOperationFailedReason, - } + e := serror.NewGeneric( + fmt.Errorf("unable to archive artifact to storage: %w", err), + sourcev1.ArchiveOperationFailedReason, + ) conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -622,10 +631,10 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, // Do this first as it is much cheaper than copy operations toPath, err := securejoin.SecureJoin(dir, incl.GetToPath()) if err != nil { - e := &serror.Event{ - Err: fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err), - Reason: "IllegalPath", - } + e := serror.NewGeneric( + fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err), + "IllegalPath", + ) conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -633,30 +642,30 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, // Retrieve the included GitRepository dep := &sourcev1.GitRepository{} if err := r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: incl.GitRepositoryRef.Name}, dep); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("could not get resource for include '%s': %w", incl.GitRepositoryRef.Name, err), - Reason: "NotFound", - } + e := serror.NewGeneric( + fmt.Errorf("could not get resource for include '%s': %w", incl.GitRepositoryRef.Name, err), + "NotFound", + ) conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } // Confirm include has an artifact if dep.GetArtifact() == nil { - e := &serror.Event{ - Err: fmt.Errorf("no artifact available for include '%s'", incl.GitRepositoryRef.Name), - Reason: "NoArtifact", - } + e := serror.NewGeneric( + fmt.Errorf("no artifact available for include '%s'", incl.GitRepositoryRef.Name), + "NoArtifact", + ) conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } // Copy artifact (sub)contents to configured directory if err := r.Storage.CopyToPath(dep.GetArtifact(), incl.GetFromPath(), toPath); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), - Reason: "CopyFailure", - } + e := serror.NewGeneric( + fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), + "CopyFailure", + ) conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -700,10 +709,10 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj } secret := &corev1.Secret{} if err := r.Client.Get(ctx, publicKeySecret, secret); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("PGP public keys secret error: %w", err), - Reason: "VerificationError", - } + e := serror.NewGeneric( + fmt.Errorf("PGP public keys secret error: %w", err), + "VerificationError", + ) conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } @@ -714,10 +723,10 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj } // Verify commit with GPG data from secret if _, err := commit.Verify(keyRings...); err != nil { - e := &serror.Event{ - Err: fmt.Errorf("signature verification of commit '%s' failed: %w", commit.Hash.String(), err), - Reason: "InvalidCommitSignature", - } + e := serror.NewGeneric( + fmt.Errorf("signature verification of commit '%s' failed: %w", commit.Hash.String(), err), + "InvalidCommitSignature", + ) conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error()) // Return error in the hope the secret changes return sreconcile.ResultEmpty, e @@ -755,10 +764,10 @@ func (r *GitRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sour func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.GitRepository) error { if !obj.DeletionTimestamp.IsZero() { if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection for deleted resource failed: %w", err), + "GarbageCollectionFailed", + ) } else if deleted != "" { r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected artifacts for deleted resource") @@ -769,10 +778,10 @@ func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourc if obj.GetArtifact() != nil { delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5) if err != nil { - return &serror.Event{ - Err: fmt.Errorf("garbage collection of artifacts failed: %w", err), - Reason: "GarbageCollectionFailed", - } + return serror.NewGeneric( + fmt.Errorf("garbage collection of artifacts failed: %w", err), + "GarbageCollectionFailed", + ) } if len(delFiles) > 0 { r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", From 5b77f65f46e33f64cd1b4c573be97e70a0047962 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 20:41:20 +0530 Subject: [PATCH 5/7] gitrepo: Enable default feature gates in tests Introduce a new field in the GitRepositoryReconciler to set the enabled features. This makes it test friendly compared to using global flags for setting and checking flags in the tests. Enable default feature gates in all the GitRepo reconciler tests. Add test cases for reconcileSource() to test the behavior of optimized git clone when the Repo is ready and not ready. This ensures that the full reconciliation is not skipped when GitRepo is not ready. Signed-off-by: Sunny --- controllers/gitrepository_controller.go | 12 +++++- controllers/gitrepository_controller_test.go | 45 +++++++++++++++++++- controllers/suite_test.go | 2 + 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index eda11f3fd..0a7ef4384 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -115,6 +115,7 @@ type GitRepositoryReconciler struct { ControllerName string requeueDependency time.Duration + features map[string]bool } type GitRepositoryReconcilerOptions struct { @@ -134,6 +135,15 @@ func (r *GitRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error { func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts GitRepositoryReconcilerOptions) error { r.requeueDependency = opts.DependencyRequeueInterval + if r.features == nil { + r.features = map[string]bool{} + } + + // Check and enable gated features. + if oc, _ := features.Enabled(features.OptimizedGitClones); oc { + r.features[features.OptimizedGitClones] = true + } + return ctrl.NewControllerManagedBy(mgr). For(&sourcev1.GitRepository{}, builder.WithPredicates( predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{}), @@ -414,7 +424,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, checkoutOpts.SemVer = ref.SemVer } - if oc, _ := features.Enabled(features.OptimizedGitClones); oc { + if val, ok := r.features[features.OptimizedGitClones]; ok && val { // Only if the object is ready, use the last revision to attempt // short-circuiting clone operation. if conditions.IsTrue(obj, meta.ReadyCondition) { diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index 194a978d9..b88f2e014 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -57,6 +57,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/fluxcd/source-controller/internal/features" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" "github.com/fluxcd/source-controller/pkg/git" @@ -499,6 +500,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } for _, i := range testGitImplementations { @@ -545,6 +547,7 @@ 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 @@ -614,6 +617,34 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) wantRevision: "v1.0.0-alpha/", want: sreconcile.ResultSuccess, }, + { + name: "Optimized clone, Ready=True", + reference: &sourcev1.GitRepositoryRef{ + Branch: "staging", + }, + beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/" + latestRev, + }, + } + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") + }, + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + }, + { + name: "Optimized clone, Ready=False", + reference: &sourcev1.GitRepositoryRef{ + Branch: "staging", + }, + beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { + conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "not ready") + }, + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + }, } server, err := gittestserver.NewTempGitServer() @@ -641,6 +672,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } for _, tt := range tests { @@ -674,6 +706,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) @@ -682,7 +718,7 @@ 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, "", headRef.Hash().String()) g.Expect(commit.String()).To(Equal(revision)) g.Expect(conditions.IsTrue(obj, sourcev1.ArtifactOutdatedCondition)).To(BeTrue()) @@ -857,6 +893,7 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { r := &GitRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } obj := &sourcev1.GitRepository{ @@ -1042,6 +1079,7 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { EventRecorder: record.NewFakeRecorder(32), Storage: storage, requeueDependency: dependencyInterval, + features: features.FeatureGates(), } obj := &sourcev1.GitRepository{ @@ -1206,6 +1244,7 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) { r := &GitRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } obj := &sourcev1.GitRepository{ @@ -1247,6 +1286,7 @@ func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) { r := &GitRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } obj := &sourcev1.GitRepository{ @@ -1384,6 +1424,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { r := &GitRepositoryReconciler{ EventRecorder: record.NewFakeRecorder(32), Client: builder.Build(), + features: features.FeatureGates(), } obj := &sourcev1.GitRepository{ @@ -1525,6 +1566,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) { Client: builder.Build(), EventRecorder: record.NewFakeRecorder(32), Storage: testStorage, + features: features.FeatureGates(), } key := client.ObjectKeyFromObject(obj) @@ -1857,6 +1899,7 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { reconciler := &GitRepositoryReconciler{ EventRecorder: recorder, + features: features.FeatureGates(), } commit := &git.Commit{ Message: "test commit", diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 288d06010..7cef15e39 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -48,6 +48,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/cache" + "github.com/fluxcd/source-controller/internal/features" "github.com/fluxcd/source-controller/internal/helm/util" // +kubebuilder:scaffold:imports ) @@ -211,6 +212,7 @@ func TestMain(m *testing.M) { EventRecorder: record.NewFakeRecorder(32), Metrics: testMetricsH, Storage: testStorage, + features: features.FeatureGates(), }).SetupWithManager(testEnv); err != nil { panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err)) } From 749068e9c3d017b78f28cca60a77ffbfc108fc84 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 13 May 2022 16:36:46 +0530 Subject: [PATCH 6/7] pkg/git: introduce concrete and partial commit Introduce concrete and partial commits. Concrete commits have all the information from remote including the hash and commit content. Partial commits are based on locally available copy of a repo, they may only contain the commit hash and reference. IsConcreteCommit() can be used to find out if a given commit is based on local information or full remote repo information. Update go-git and libgit2 branch/tag clone optimization to return a partial commit and no error. Update and simplify the go-git and libgit2 tests for the same. Signed-off-by: Sunny --- pkg/git/git.go | 10 ++ pkg/git/git_test.go | 39 +++++ pkg/git/gogit/checkout.go | 35 ++++- pkg/git/gogit/checkout_test.go | 170 ++++++++++++---------- pkg/git/libgit2/checkout.go | 119 +++++++++------ pkg/git/libgit2/checkout_test.go | 240 ++++++++++++++++++------------- pkg/git/options.go | 3 +- 7 files changed, 391 insertions(+), 225 deletions(-) diff --git a/pkg/git/git.go b/pkg/git/git.go index cc45498d1..da0e7d225 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -118,3 +118,13 @@ type NoChangesError struct { 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. +func IsConcreteCommit(c Commit) bool { + if c.Hash != nil && c.Encoded != nil { + return true + } + return false +} diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index 9d9d94dd8..5b67b23bd 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -18,6 +18,7 @@ package git import ( "testing" + "time" . "github.com/onsi/gomega" ) @@ -263,3 +264,41 @@ of the commit`, }) } } + +func TestIsConcreteCommit(t *testing.T) { + tests := []struct { + name string + commit Commit + result bool + }{ + { + name: "concrete commit", + commit: Commit{ + Hash: Hash("foo"), + Reference: "refs/tags/main", + Author: Signature{ + Name: "user", Email: "user@example.com", When: time.Now(), + }, + Committer: Signature{ + Name: "user", Email: "user@example.com", When: time.Now(), + }, + Signature: "signature", + Encoded: []byte("commit-content"), + Message: "commit-message", + }, + result: true, + }, + { + name: "partial commit", + commit: Commit{Hash: Hash("foo")}, + result: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(IsConcreteCommit(tt.commit)).To(Equal(tt.result)) + }) + } +} diff --git a/pkg/git/gogit/checkout.go b/pkg/git/gogit/checkout.go index afa4afbf8..c3c484c61 100644 --- a/pkg/git/gogit/checkout.go +++ b/pkg/git/gogit/checkout.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "sort" + "strings" "time" "github.com/Masterminds/semver/v3" @@ -78,10 +79,21 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g } if currentRevision != "" && currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconcilation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + // Split the revision and take the last part as the hash. + // Example revision: main/43d7eb9c49cdd49b2494efd481aea1166fc22b67 + var hash git.Hash + ss := strings.Split(currentRevision, "/") + if len(ss) > 1 { + hash = git.Hash(ss[len(ss)-1]) + } else { + hash = git.Hash(ss[0]) } + c := &git.Commit{ + Hash: hash, + Reference: plumbing.NewBranchReferenceName(c.Branch).String(), + } + return c, nil } } @@ -153,10 +165,21 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. } if currentRevision != "" && currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconcilation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + // Split the revision and take the last part as the hash. + // Example revision: 6.1.4/bf09377bfd5d3bcac1e895fa8ce52dc76695c060 + var hash git.Hash + ss := strings.Split(currentRevision, "/") + if len(ss) > 1 { + hash = git.Hash(ss[len(ss)-1]) + } else { + hash = git.Hash(ss[0]) + } + c := &git.Commit{ + Hash: hash, + Reference: ref.String(), } + return c, nil } } repo, err := extgogit.PlainCloneContext(ctx, path, false, &extgogit.CloneOptions{ diff --git a/pkg/git/gogit/checkout_test.go b/pkg/git/gogit/checkout_test.go index c666308a9..61f0833c3 100644 --- a/pkg/git/gogit/checkout_test.go +++ b/pkg/git/gogit/checkout_test.go @@ -67,32 +67,36 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } tests := []struct { - name string - branch string - filesCreated map[string]string - expectedCommit string - expectedErr string - lastRevision string + name string + branch string + filesCreated map[string]string + lastRevision string + expectedCommit string + expectedConcreteCommit bool + expectedErr string }{ { - name: "Default branch", - branch: "master", - filesCreated: map[string]string{"branch": "init"}, - expectedCommit: firstCommit.String(), + name: "Default branch", + branch: "master", + filesCreated: map[string]string{"branch": "init"}, + expectedCommit: firstCommit.String(), + expectedConcreteCommit: true, }, { - name: "skip clone if LastRevision hasn't changed", - branch: "master", - filesCreated: map[string]string{"branch": "init"}, - expectedErr: fmt.Sprintf("no changes since last reconcilation: observed revision 'master/%s'", firstCommit.String()), - lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + name: "skip clone if LastRevision hasn't changed", + branch: "master", + filesCreated: map[string]string{"branch": "init"}, + lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + expectedCommit: firstCommit.String(), + expectedConcreteCommit: false, }, { - name: "Other branch - revision has changed", - branch: "test", - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + name: "Other branch - revision has changed", + branch: "test", + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("master/%s", firstCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, { name: "Non existing branch", @@ -120,58 +124,64 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit)) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit)) - for k, v := range tt.filesCreated { - g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + if tt.expectedConcreteCommit { + for k, v := range tt.filesCreated { + g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + } } }) } } func TestCheckoutTag_Checkout(t *testing.T) { + type testTag struct { + name string + annotated bool + } + tests := []struct { - name string - tag string - annotated bool - checkoutTag string - expectTag string - expectErr string - lastRev string - setLastRev bool + name string + tagsInRepo []testTag + checkoutTag string + lastRevTag string + expectConcreteCommit bool + expectErr string }{ { - name: "Tag", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", + name: "Tag", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + expectConcreteCommit: true, }, { - name: "Skip Tag if last revision hasn't changed", - tag: "tag-2", - checkoutTag: "tag-2", - setLastRev: true, - expectErr: "no changes since last reconcilation", + name: "Annotated", + tagsInRepo: []testTag{{"annotated", true}}, + checkoutTag: "annotated", + expectConcreteCommit: true, }, { - name: "Last revision changed", - tag: "tag-3", - checkoutTag: "tag-3", - expectTag: "tag-3", - lastRev: "tag-3/", + name: "Non existing tag", + // Without this go-git returns error "remote repository is empty". + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "invalid", + expectErr: "couldn't find remote ref \"refs/tags/invalid\"", }, { - name: "Annotated", - tag: "annotated", - annotated: true, - checkoutTag: "annotated", - expectTag: "annotated", + name: "Skip clone - last revision unchanged", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + lastRevTag: "tag-1", + expectConcreteCommit: false, }, { - name: "Non existing tag", - tag: "tag-1", - checkoutTag: "invalid", - expectErr: "couldn't find remote ref \"refs/tags/invalid\"", + name: "Last revision changed", + tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, + checkoutTag: "tag-2", + lastRevTag: "tag-1", + expectConcreteCommit: true, }, } for _, tt := range tests { @@ -183,32 +193,37 @@ func TestCheckoutTag_Checkout(t *testing.T) { t.Fatal(err) } - var h plumbing.Hash - var tagHash *plumbing.Reference - if tt.tag != "" { - h, err = commitFile(repo, "tag", tt.tag, time.Now()) - if err != nil { - t.Fatal(err) - } - tagHash, err = tag(repo, h, !tt.annotated, tt.tag, time.Now()) - if err != nil { - t.Fatal(err) + // Collect tags and their associated commit hash for later + // reference. + tagCommits := map[string]string{} + + // Populate the repo with commits and tags. + if tt.tagsInRepo != nil { + for _, tr := range tt.tagsInRepo { + h, err := commitFile(repo, "tag", tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + _, err = tag(repo, h, tr.annotated, tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + tagCommits[tr.name] = h.String() } } - tag := CheckoutTag{ + checkoutTag := CheckoutTag{ Tag: tt.checkoutTag, } - if tt.setLastRev { - tag.LastRevision = fmt.Sprintf("%s/%s", tt.tag, tagHash.Hash().String()) + // If last revision is provided, configure it. + if tt.lastRevTag != "" { + lc := tagCommits[tt.lastRevTag] + checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.lastRevTag, lc) } - if tt.lastRev != "" { - tag.LastRevision = tt.lastRev - } tmpDir := t.TempDir() - cc, err := tag.Checkout(context.TODO(), tmpDir, path, nil) + cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, path, nil) if tt.expectErr != "" { g.Expect(err).ToNot(BeNil()) g.Expect(err.Error()).To(ContainSubstring(tt.expectErr)) @@ -216,10 +231,17 @@ func TestCheckoutTag_Checkout(t *testing.T) { return } + // Check successful checkout results. + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit)) + targetTagHash := tagCommits[tt.checkoutTag] g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cc.String()).To(Equal(tt.expectTag + "/" + h.String())) - g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag)) + g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagHash)) + + // Check file content only when there's an actual checkout. + if tt.lastRevTag != tt.checkoutTag { + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) + } }) } } diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 9dc233fea..cc6f8e487 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -34,6 +34,8 @@ import ( "github.com/fluxcd/source-controller/pkg/git/libgit2/managed" ) +const defaultRemoteName = "origin" + // CheckoutStrategyForOptions returns the git.CheckoutStrategy for the given // git.CheckoutOptions. func CheckoutStrategyForOptions(ctx context.Context, opt git.CheckoutOptions) git.CheckoutStrategy { @@ -67,26 +69,43 @@ type CheckoutBranch struct { func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) - repo, remote, free, err := getBlankRepoAndRemote(ctx, path, url, opts) + remoteCallBacks := RemoteCallbacks(ctx, opts) + proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} + + repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) if err != nil { return nil, err } - defer free() + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, proxyOpts, nil) + if err != nil { + remote.Free() + repo.Free() + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer func() { + remote.Disconnect() + remote.Free() + repo.Free() + }() - // When the last observed revision is set, check whether it is still - // the same at the remote branch. If so, short-circuit the clone operation here. + // When the last observed revision is set, check whether it is still the + // same at the remote branch. If so, short-circuit the clone operation here. if c.LastRevision != "" { heads, err := remote.Ls(c.Branch) if err != nil { return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } if len(heads) > 0 { - currentRevision := fmt.Sprintf("%s/%s", c.Branch, heads[0].Id.String()) + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Branch, hash) if currentRevision == c.LastRevision { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/heads/" + c.Branch, } + return c, nil } } } @@ -95,7 +114,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g err = remote.Fetch([]string{c.Branch}, &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsNone, - RemoteCallbacks: RemoteCallbacks(ctx, opts), + RemoteCallbacks: remoteCallBacks, ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, }, "") @@ -151,33 +170,53 @@ type CheckoutTag struct { func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (_ *git.Commit, err error) { defer recoverPanic(&err) - repo, remote, free, err := getBlankRepoAndRemote(ctx, path, url, opts) + remoteCallBacks := RemoteCallbacks(ctx, opts) + proxyOpts := &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto} + + repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts) if err != nil { return nil, err } - defer free() + // Open remote connection. + err = remote.ConnectFetch(&remoteCallBacks, proxyOpts, nil) + if err != nil { + remote.Free() + repo.Free() + return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } + defer func() { + remote.Disconnect() + remote.Free() + repo.Free() + }() + // When the last observed revision is set, check whether it is still the + // same at the remote branch. If so, short-circuit the clone operation here. if c.LastRevision != "" { heads, err := remote.Ls(c.Tag) if err != nil { return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } if len(heads) > 0 { - currentRevision := fmt.Sprintf("%s/%s", c.Tag, heads[0].Id.String()) + hash := heads[0].Id.String() + currentRevision := fmt.Sprintf("%s/%s", c.Tag, hash) var same bool if currentRevision == c.LastRevision { same = true } else if len(heads) > 1 { - currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, heads[1].Id.String()) + hash = heads[1].Id.String() + currentAnnotatedRevision := fmt.Sprintf("%s/%s", c.Tag, hash) if currentAnnotatedRevision == c.LastRevision { same = true } } if same { - return nil, git.NoChangesError{ - Message: "no changes since last reconciliation", - ObservedRevision: currentRevision, + // Construct a partial commit with the existing information. + c := &git.Commit{ + Hash: git.Hash(hash), + Reference: "refs/tags/" + c.Tag, } + return c, nil } } } @@ -185,8 +224,8 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git. err = remote.Fetch([]string{c.Tag}, &git2go.FetchOptions{ DownloadTags: git2go.DownloadTagsAuto, - RemoteCallbacks: RemoteCallbacks(ctx, opts), - ProxyOptions: git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, + RemoteCallbacks: remoteCallBacks, + ProxyOptions: *proxyOpts, }, "") @@ -408,34 +447,34 @@ func buildSignature(s *git2go.Signature) git.Signature { } } -// getBlankRepoAndRemote returns a newly initialized repository, and a remote connected to the provided url. -// Callers must call the returning function to free all git2go objects. -func getBlankRepoAndRemote(ctx context.Context, path, url string, opts *git.AuthOptions) (*git2go.Repository, *git2go.Remote, func(), error) { +// initializeRepoWithRemote initializes or opens a repository at the given path +// and configures it with the given remote "origin" URL. If a remote already +// exists with a different URL, it returns an error. +func initializeRepoWithRemote(ctx context.Context, path, url string, opts *git.AuthOptions) (*git2go.Repository, *git2go.Remote, error) { repo, err := git2go.InitRepository(path, false) if err != nil { - return nil, nil, nil, fmt.Errorf("unable to init repository for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + return nil, nil, fmt.Errorf("unable to init repository for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) } - remote, err := repo.Remotes.Create("origin", url) + remote, err := repo.Remotes.Create(defaultRemoteName, url) if err != nil { - repo.Free() - return nil, nil, nil, fmt.Errorf("unable to create remote for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - - callBacks := RemoteCallbacks(ctx, opts) - err = remote.ConnectFetch(&callBacks, &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, nil) - if err != nil { - remote.Free() - repo.Free() - return nil, nil, nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) - } - - free := func() { - remote.Disconnect() - remote.Free() - repo.Free() + // If the remote already exists, lookup the remote. + if git2go.IsErrorCode(err, git2go.ErrorCodeExists) { + remote, err = repo.Remotes.Lookup(defaultRemoteName) + if err != nil { + repo.Free() + return nil, nil, fmt.Errorf("unable to create or lookup remote '%s'", defaultRemoteName) + } + if remote.Url() != url { + repo.Free() + return nil, nil, fmt.Errorf("remote '%s' with different address '%s' already exists", defaultRemoteName, remote.Url()) + } + } else { + repo.Free() + return nil, nil, fmt.Errorf("unable to create remote for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err)) + } } - return repo, remote, free, nil + return repo, remote, nil } func recoverPanic(err *error) { diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index 28bcbd29e..b4f6c11d1 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/pkg/git/libgit2/checkout_test.go @@ -25,6 +25,7 @@ import ( "testing" "time" + "github.com/fluxcd/source-controller/pkg/git" git2go "github.com/libgit2/git2go/v33" . "github.com/onsi/gomega" ) @@ -76,44 +77,49 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } tests := []struct { - name string - branch string - filesCreated map[string]string - expectedCommit string - expectedErr string - lastRevision string + name string + branch string + filesCreated map[string]string + lastRevision string + expectedCommit string + expectedConcreteCommit bool + expectedErr string }{ { - name: "Default branch", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), + name: "Default branch", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, { - name: "Other branch", - branch: "test", - filesCreated: map[string]string{"branch": "init"}, - expectedCommit: firstCommit.String(), + name: "Other branch", + branch: "test", + filesCreated: map[string]string{"branch": "init"}, + expectedCommit: firstCommit.String(), + expectedConcreteCommit: true, }, { - name: "Non existing branch", - branch: "invalid", - expectedErr: "reference 'refs/remotes/origin/invalid' not found", + name: "Non existing branch", + branch: "invalid", + expectedErr: "reference 'refs/remotes/origin/invalid' not found", + expectedConcreteCommit: true, }, { - name: "skip clone - lastRevision hasn't changed", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, secondCommit.String()), - expectedErr: fmt.Sprintf("no changes since last reconciliation: observed revision '%s/%s'", defaultBranch, secondCommit.String()), + name: "skip clone - lastRevision hasn't changed", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("%s/%s", defaultBranch, secondCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: false, }, { - name: "lastRevision is different", - branch: defaultBranch, - filesCreated: map[string]string{"branch": "second"}, - expectedCommit: secondCommit.String(), - lastRevision: fmt.Sprintf("%s/%s", defaultBranch, firstCommit.String()), + name: "lastRevision is different", + branch: defaultBranch, + filesCreated: map[string]string{"branch": "second"}, + lastRevision: fmt.Sprintf("%s/%s", defaultBranch, firstCommit.String()), + expectedCommit: secondCommit.String(), + expectedConcreteCommit: true, }, } @@ -136,37 +142,43 @@ func TestCheckoutBranch_Checkout(t *testing.T) { } g.Expect(err).ToNot(HaveOccurred()) g.Expect(cc.String()).To(Equal(tt.branch + "/" + tt.expectedCommit)) + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectedConcreteCommit)) - for k, v := range tt.filesCreated { - g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) - g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + if tt.expectedConcreteCommit { + for k, v := range tt.filesCreated { + g.Expect(filepath.Join(tmpDir, k)).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, k))).To(BeEquivalentTo(v)) + } } }) } } func TestCheckoutTag_Checkout(t *testing.T) { + type testTag struct { + name string + annotated bool + } + tests := []struct { - name string - tag string - annotated bool - checkoutTag string - expectTag string - expectErr string - lastRevision bool + name string + tagsInRepo []testTag + checkoutTag string + lastRevTag string + expectErr string + expectConcreteCommit bool }{ { - name: "Tag", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", + name: "Tag", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + expectConcreteCommit: true, }, { - name: "Annotated", - tag: "annotated", - annotated: true, - checkoutTag: "annotated", - expectTag: "annotated", + name: "Annotated", + tagsInRepo: []testTag{{"annotated", true}}, + checkoutTag: "annotated", + expectConcreteCommit: true, }, { name: "Non existing tag", @@ -174,19 +186,18 @@ func TestCheckoutTag_Checkout(t *testing.T) { expectErr: "unable to find 'invalid': no reference found for shorthand 'invalid'", }, { - name: "skip clone - last revision is unchanged", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-1", - lastRevision: true, - expectErr: "no changes since last reconciliation", + name: "Skip clone - last revision unchanged", + tagsInRepo: []testTag{{"tag-1", false}}, + checkoutTag: "tag-1", + lastRevTag: "tag-1", + expectConcreteCommit: false, }, { - name: "last revision changed", - tag: "tag-1", - checkoutTag: "tag-1", - expectTag: "tag-2", - lastRevision: true, + name: "Last revision changed", + tagsInRepo: []testTag{{"tag-1", false}, {"tag-2", false}}, + checkoutTag: "tag-2", + lastRevTag: "tag-1", + expectConcreteCommit: true, }, } for _, tt := range tests { @@ -199,68 +210,57 @@ func TestCheckoutTag_Checkout(t *testing.T) { } defer repo.Free() - var commit *git2go.Commit - if tt.tag != "" { - c, err := commitFile(repo, "tag", tt.tag, time.Now()) - if err != nil { - t.Fatal(err) - } - if commit, err = repo.LookupCommit(c); err != nil { - t.Fatal(err) - } - _, err = tag(repo, commit.Id(), !tt.annotated, tt.tag, time.Now()) - if err != nil { - t.Fatal(err) + // Collect tags and their associated commit for later reference. + tagCommits := map[string]*git2go.Commit{} + + // Populate the repo with commits and tags. + if tt.tagsInRepo != nil { + for _, tr := range tt.tagsInRepo { + var commit *git2go.Commit + c, err := commitFile(repo, "tag", tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + if commit, err = repo.LookupCommit(c); err != nil { + t.Fatal(err) + } + _, err = tag(repo, commit.Id(), tr.annotated, tr.name, time.Now()) + if err != nil { + t.Fatal(err) + } + tagCommits[tr.name] = commit } } checkoutTag := CheckoutTag{ Tag: tt.checkoutTag, } + // If last revision is provided, configure it. + if tt.lastRevTag != "" { + lc := tagCommits[tt.lastRevTag] + checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.lastRevTag, lc.Id().String()) + } + tmpDir := t.TempDir() cc, err := checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) - if tt.expectErr != "" { - if tt.lastRevision { - tmpDir, _ = os.MkdirTemp("", "test") - defer os.RemoveAll(tmpDir) - checkoutTag.LastRevision = cc.String() - cc, err = checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) - } g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.expectErr)) g.Expect(cc).To(BeNil()) return } - if tt.lastRevision { - checkoutTag.LastRevision = fmt.Sprintf("%s/%s", tt.tag, commit.Id().String()) - checkoutTag.Tag = tt.expectTag - if tt.tag != "" { - c, err := commitFile(repo, "tag", "changed tag", time.Now()) - if err != nil { - t.Fatal(err) - } - if commit, err = repo.LookupCommit(c); err != nil { - t.Fatal(err) - } - _, err = tag(repo, commit.Id(), !tt.annotated, tt.expectTag, time.Now()) - if err != nil { - t.Fatal(err) - } - tmpDir, _ = os.MkdirTemp("", "test") - defer os.RemoveAll(tmpDir) - cc, err = checkoutTag.Checkout(context.TODO(), tmpDir, repo.Path(), nil) - } - } + // Check successful checkout results. + g.Expect(git.IsConcreteCommit(*cc)).To(Equal(tt.expectConcreteCommit)) + targetTagCommit := tagCommits[tt.checkoutTag] g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cc.String()).To(Equal(tt.expectTag + "/" + commit.Id().String())) - g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) - if tt.lastRevision { - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo("changed tag")) - } else { - g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag)) + g.Expect(cc.String()).To(Equal(tt.checkoutTag + "/" + targetTagCommit.Id().String())) + + // Check file content only when there's an actual checkout. + if tt.lastRevTag != tt.checkoutTag { + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.checkoutTag)) } }) } @@ -510,3 +510,37 @@ func mockSignature(time time.Time) *git2go.Signature { When: time, } } + +func TestInitializeRepoWithRemote(t *testing.T) { + g := NewWithT(t) + tmp := t.TempDir() + ctx := context.TODO() + testRepoURL := "https://example.com/foo/bar" + testRepoURL2 := "https://example.com/foo/baz" + authOpts, err := git.AuthOptionsWithoutSecret(testRepoURL) + g.Expect(err).ToNot(HaveOccurred()) + authOpts2, err := git.AuthOptionsWithoutSecret(testRepoURL2) + g.Expect(err).ToNot(HaveOccurred()) + + // Fresh initialization. + repo, remote, err := initializeRepoWithRemote(ctx, tmp, testRepoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(repo.IsBare()).To(BeFalse()) + g.Expect(remote.Name()).To(Equal(defaultRemoteName)) + g.Expect(remote.Url()).To(Equal(testRepoURL)) + remote.Free() + repo.Free() + + // Reinitialize to ensure it reuses the existing origin. + repo, remote, err = initializeRepoWithRemote(ctx, tmp, testRepoURL, authOpts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(repo.IsBare()).To(BeFalse()) + g.Expect(remote.Name()).To(Equal(defaultRemoteName)) + g.Expect(remote.Url()).To(Equal(testRepoURL)) + remote.Free() + repo.Free() + + // Reinitialize with a different remote URL for existing origin. + _, _, err = initializeRepoWithRemote(ctx, tmp, testRepoURL2, authOpts2) + g.Expect(err).To(HaveOccurred()) +} diff --git a/pkg/git/options.go b/pkg/git/options.go index b5e8f2c41..ff1bccac1 100644 --- a/pkg/git/options.go +++ b/pkg/git/options.go @@ -49,8 +49,7 @@ type CheckoutOptions struct { // not supported by all Implementations. RecurseSubmodules bool - // LastRevision holds the revision observed on the last successful - // reconciliation. + // LastRevision holds the last observed revision of the local repository. // It is used to skip clone operations when no changes were detected. LastRevision string } From 581695b4d621fccb9b6a264b5dbcedf97151cec0 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 18 May 2022 19:14:46 +0530 Subject: [PATCH 7/7] gitrepo: Intro contentConfigChecksum & improvement Introduce contentConfigChecksum in the GitRepository.Status to track the configurations that affect the content of the artifact. It is used to detect a change in the configuration that requires rebuilding the whole artifact. This helps skip the reconciliation early when we find out that the remote repository has not changed. Moves fetching the included repositories in reconcileSource() to collect enough information in reconcileSource() to be able to decide if the full reconciliation can be skipped. This results in reconcileInclude() to just copy artifact to the source build directory. Introduce a gitCheckout() method to perform construction of all the git checkout options and perform the checkout operation. This helps to easily perform checkout multiple times when we need it in reconcileSource(). When we check with the remote repository if there's an update, and find out that there's no update, we check if any other configurations that affect the source content has changed, like includes, ignore rules, etc. If there's a change, we need to perform a full checkout of the remote repository in order to fetch the complete source. The git checkout no-op optimization is enabled in this method based on the presence of an artifact in the storage. The failure notification handler is modifed to handle the recovery of a no-op reconcile failure and create a notification message accordingly with the partial commit. Signed-off-by: Sunny --- api/v1beta2/gitrepository_types.go | 12 + ...rce.toolkit.fluxcd.io_gitrepositories.yaml | 9 + controllers/gitrepository_controller.go | 316 +++++++++++---- controllers/gitrepository_controller_test.go | 376 ++++++++++++++---- docs/api/source.md | 20 + docs/spec/v1beta2/gitrepositories.md | 16 +- pkg/git/git.go | 12 - 7 files changed, 585 insertions(+), 176 deletions(-) diff --git a/api/v1beta2/gitrepository_types.go b/api/v1beta2/gitrepository_types.go index 9b9948b0e..6398e2f8a 100644 --- a/api/v1beta2/gitrepository_types.go +++ b/api/v1beta2/gitrepository_types.go @@ -211,6 +211,18 @@ type GitRepositoryStatus struct { // +optional IncludedArtifacts []*Artifact `json:"includedArtifacts,omitempty"` + // ContentConfigChecksum is a checksum of all the configurations related to + // the content of the source artifact: + // - .spec.ignore + // - .spec.recurseSubmodules + // - .spec.included and the checksum of the included artifacts + // observed in .status.observedGeneration version of the object. This can + // be used to determine if the content of the included repository has + // changed. + // It has the format of `:`, for example: `sha256:`. + // +optional + ContentConfigChecksum string `json:"contentConfigChecksum,omitempty"` + meta.ReconcileRequestStatus `json:",inline"` } diff --git a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml index e4e6b97e6..2fdc9f00e 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml @@ -653,6 +653,15 @@ spec: - type type: object type: array + contentConfigChecksum: + description: 'ContentConfigChecksum is a checksum of all the configurations + related to the content of the source artifact: - .spec.ignore - + .spec.recurseSubmodules - .spec.included and the checksum of the + included artifacts observed in .status.observedGeneration version + of the object. This can be used to determine if the content of the + included repository has changed. It has the format of `:`, + for example: `sha256:`.' + type: string includedArtifacts: description: IncludedArtifacts contains a list of the last successfully included Artifacts as instructed by GitRepositorySpec.Include. diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 0a7ef4384..f3c4e5713 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -18,10 +18,12 @@ package controllers import ( "context" + "crypto/sha256" "errors" "fmt" "os" "path/filepath" + "strconv" "strings" "time" @@ -289,11 +291,11 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.G return res, resErr } -// notify emits notification related to the reconciliation. +// notify emits notification related to the result of reconciliation. func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository, commit git.Commit, res sreconcile.Result, resErr error) { - // Notify successful reconciliation for new artifact and recovery from any - // failure. - if resErr == nil && res == sreconcile.ResultSuccess && newObj.Status.Artifact != nil { + // Notify successful reconciliation for new artifact, no-op reconciliation + // and recovery from any failure. + if r.shouldNotify(oldObj, newObj, res, resErr) { annotations := map[string]string{ sourcev1.GroupVersion.Group + "/revision": newObj.Status.Artifact.Revision, sourcev1.GroupVersion.Group + "/checksum": newObj.Status.Artifact.Checksum, @@ -304,7 +306,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 '%s'", commit.String()) + } // Notify on new artifact and failure recovery. if oldChecksum != newObj.GetArtifact().Checksum { @@ -319,6 +328,25 @@ func (r *GitRepositoryReconciler) notify(oldObj, newObj *sourcev1.GitRepository, } } +// shouldNotify analyzes the result of subreconcilers and determines if a +// notification should be sent. It decides about the final informational +// notifications after the reconciliation. Failure notification and in-line +// notifications are not handled here. +func (r *GitRepositoryReconciler) shouldNotify(oldObj, newObj *sourcev1.GitRepository, res sreconcile.Result, resErr error) bool { + // Notify for successful reconciliation. + if resErr == nil && res == sreconcile.ResultSuccess && newObj.Status.Artifact != nil { + return true + } + // Notify for no-op reconciliation with ignore error. + if resErr != nil && res == sreconcile.ResultEmpty && newObj.Status.Artifact != nil { + // Convert to Generic error and check for ignore. + if ge, ok := resErr.(*serror.Generic); ok { + return ge.Ignore == true + } + } + return false +} + // reconcileStorage ensures the current state of the storage matches the // desired and previously observed state. // @@ -361,8 +389,15 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, // reconcileSource ensures the upstream Git repository and reference can be // cloned and checked out using the specified configuration, and observes its -// state. +// state. It also checks if the included repositories are available for use. // +// The included repositories are fetched and their metadata are stored. In case +// one of the included repositories isn't ready, it records +// v1beta2.IncludeUnavailableCondition=True and returns early. When all the +// included repositories are ready, it removes +// v1beta2.IncludeUnavailableCondition from the object. +// When the included artifactSet differs from the current set in the Status of +// the object, it marks the object with v1beta2.ArtifactOutdatedCondition=True. // The repository is cloned to the given dir, using the specified configuration // to check out the reference. In case of an error during this process // (including transient errors), it records v1beta2.FetchFailedCondition=True @@ -377,8 +412,13 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, // it records v1beta2.SourceVerifiedCondition=True. // When all the above is successful, the given Commit pointer is set to the // commit of the checked out Git repository. +// +// If the optimized git clone feature is enabled, it checks if the remote repo +// and the local artifact are on the same revision, and no other source content +// related configurations have changed since last reconciliation. If there's a +// change, it short-circuits the whole reconciliation with an early return. func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, - obj *sourcev1.GitRepository, commit *git.Commit, _ *artifactSet, dir string) (sreconcile.Result, error) { + obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Configure authentication strategy to access the source var authOpts *git.AuthOptions var err error @@ -415,37 +455,6 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, return sreconcile.ResultEmpty, e } - // Configure checkout strategy - checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules} - if ref := obj.Spec.Reference; ref != nil { - checkoutOpts.Branch = ref.Branch - checkoutOpts.Commit = ref.Commit - checkoutOpts.Tag = ref.Tag - checkoutOpts.SemVer = ref.SemVer - } - - if val, ok := r.features[features.OptimizedGitClones]; ok && val { - // Only if the object is ready, use the last revision to attempt - // short-circuiting clone operation. - if conditions.IsTrue(obj, meta.ReadyCondition) { - if artifact := obj.GetArtifact(); artifact != nil { - checkoutOpts.LastRevision = artifact.Revision - } - } - } - - checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(ctx, - git.Implementation(obj.Spec.GitImplementation), checkoutOpts) - if err != nil { - e := &serror.Stalling{ - Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), - Reason: sourcev1.GitOperationFailedReason, - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Do not return err as recovery without changes is impossible - return sreconcile.ResultEmpty, e - } - repositoryURL := obj.Spec.URL // managed GIT transport only affects the libgit2 implementation if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { @@ -473,32 +482,77 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, } } - // Checkout HEAD of reference in object - gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) - defer cancel() - c, err := checkoutStrategy.Checkout(gitCtx, dir, repositoryURL, authOpts) + // Fetch the included artifact metadata. + artifacts, err := r.fetchIncludes(ctx, obj) if err != nil { - var v git.NoChangesError - if errors.As(err, &v) { - // Create generic error without notification. Since it's a no-op - // error, ignore (no runtime error), normal event. - ge := serror.NewGeneric(v, sourcev1.GitOperationSucceedReason) - ge.Notification = false - ge.Ignore = true - ge.Event = corev1.EventTypeNormal - return sreconcile.ResultEmpty, ge - } + return sreconcile.ResultEmpty, err + } + // Observe if the artifacts still match the previous included ones + if artifacts.Diff(obj.Status.IncludedArtifacts) { + message := fmt.Sprintf("included artifacts differ from last observed includes") + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) + conditions.MarkReconciling(obj, "IncludeChange", message) + } + + // Persist the ArtifactSet. + *includes = *artifacts + + var optimizedClone bool + if val, ok := r.features[features.OptimizedGitClones]; ok && val { + optimizedClone = true + } + + c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, optimizedClone) + if err != nil { e := serror.NewGeneric( fmt.Errorf("failed to checkout and determine revision: %w", err), sourcev1.GitOperationFailedReason, ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // Coin flip on transient or persistent error, return error and hope for the best return sreconcile.ResultEmpty, e } // Assign the commit to the shared commit reference. *commit = *c + + // If it's a partial commit obtained from an existing artifact, check if the + // reconciliation can be skipped if other configurations have not changed. + if !git.IsConcreteCommit(*commit) { + // Calculate content configuration checksum. + if r.calculateContentConfigChecksum(obj, includes) == obj.Status.ContentConfigChecksum { + ge := serror.NewGeneric( + fmt.Errorf("no changes since last reconcilation: observed revision '%s'", + commit.String()), sourcev1.GitOperationSucceedReason, + ) + ge.Notification = false + ge.Ignore = true + ge.Event = corev1.EventTypeNormal + // Remove any stale fetch failed condition. + conditions.Delete(obj, sourcev1.FetchFailedCondition) + // IMPORTANT: This must be set to ensure that the observed + // generation of this condition is updated. In case of full + // reconciliation reconcileArtifact() ensures that it's set at the + // very end. + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, + "stored artifact for revision '%s'", commit.String()) + // TODO: Find out if such condition setting is needed when commit + // signature verification is enabled. + return sreconcile.ResultEmpty, ge + } + + // If we can't skip the reconciliation, checkout again without any + // optimization. + c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, false) + if err != nil { + e := serror.NewGeneric( + fmt.Errorf("failed to checkout and determine revision: %w", err), + sourcev1.GitOperationFailedReason, + ) + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + *commit = *c + } ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info("git repository checked out", "url", obj.Spec.URL, "revision", commit.String()) conditions.Delete(obj, sourcev1.FetchFailedCondition) @@ -521,21 +575,27 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // // The inspection of the given data to the object is differed, ensuring any // stale observations like v1beta2.ArtifactOutdatedCondition are removed. -// If the given Artifact and/or artifactSet (includes) do not differ from the -// object's current, it returns early. +// If the given Artifact and/or artifactSet (includes) and the content config +// checksum do not differ from the object's current, it returns early. // Source ignore patterns are loaded, and the given directory is archived while // taking these patterns into account. -// On a successful archive, the Artifact and Includes in the Status of the -// object are set, and the symlink in the Storage is updated to its path. +// On a successful archive, the Artifact, Includes and new content config +// checksum in the Status of the object are set, and the symlink in the Storage +// is updated to its path. func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { // Create potential new artifact with current available metadata artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String())) + // Calculate the content config checksum. + ccc := r.calculateContentConfigChecksum(obj, includes) + // Set the ArtifactInStorageCondition if there's no drift. defer func() { - if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) { + if obj.GetArtifact().HasRevision(artifact.Revision) && + !includes.Diff(obj.Status.IncludedArtifacts) && + obj.Status.ContentConfigChecksum == ccc { conditions.Delete(obj, sourcev1.ArtifactOutdatedCondition) conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "stored artifact for revision '%s'", artifact.Revision) @@ -543,7 +603,9 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, }() // The artifact is up-to-date - if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) { + if obj.GetArtifact().HasRevision(artifact.Revision) && + !includes.Diff(obj.Status.IncludedArtifacts) && + obj.Status.ContentConfigChecksum == ccc { r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.ArtifactUpToDateReason, "artifact up-to-date with remote revision: '%s'", artifact.Revision) return sreconcile.ResultSuccess, nil } @@ -609,6 +671,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, // Record it on the object obj.Status.Artifact = artifact.DeepCopy() obj.Status.IncludedArtifacts = *includes + obj.Status.ContentConfigChecksum = ccc // Update symlink on a "best effort" basis url, err := r.Storage.Symlink(artifact, "latest.tar.gz") @@ -636,7 +699,6 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, obj *sourcev1.GitRepository, _ *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { - artifacts := make(artifactSet, len(obj.Spec.Include)) for i, incl := range obj.Spec.Include { // Do this first as it is much cheaper than copy operations toPath, err := securejoin.SecureJoin(dir, incl.GetToPath()) @@ -645,56 +707,142 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err), "IllegalPath", ) - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) + return sreconcile.ResultEmpty, e + } + + // Get artifact at the same include index. The artifactSet is created + // such that the index of artifactSet matches with the index of Include. + // Hence, index is used here to pick the associated artifact from + // includes. + var artifact *sourcev1.Artifact + for j, art := range *includes { + if i == j { + artifact = art + } + } + + // Copy artifact (sub)contents to configured directory. + if err := r.Storage.CopyToPath(artifact, incl.GetFromPath(), toPath); err != nil { + e := &serror.Event{ + Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), + Reason: "CopyFailure", + } + conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } + } + conditions.Delete(obj, sourcev1.IncludeUnavailableCondition) + return sreconcile.ResultSuccess, nil +} + +// gitCheckout builds checkout options with the given configurations and +// performs a git checkout. +func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, + obj *sourcev1.GitRepository, repoURL string, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) { + // Configure checkout strategy. + checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules} + if ref := obj.Spec.Reference; ref != nil { + checkoutOpts.Branch = ref.Branch + checkoutOpts.Commit = ref.Commit + checkoutOpts.Tag = ref.Tag + checkoutOpts.SemVer = ref.SemVer + } + + // 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 optimized && conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) { + if artifact := obj.GetArtifact(); artifact != nil { + checkoutOpts.LastRevision = artifact.Revision + } + } + + checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(ctx, + git.Implementation(obj.Spec.GitImplementation), checkoutOpts) + if err != nil { + e := &serror.Stalling{ + Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err), + Reason: sourcev1.GitOperationFailedReason, + } + conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) + // Do not return err as recovery without changes is impossible. + return nil, e + } + + // Checkout HEAD of reference in object + gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) + defer cancel() + return checkoutStrategy.Checkout(gitCtx, dir, repoURL, authOpts) +} - // Retrieve the included GitRepository +// fetchIncludes fetches artifact metadata of all the included repos. +func (r *GitRepositoryReconciler) fetchIncludes(ctx context.Context, obj *sourcev1.GitRepository) (*artifactSet, error) { + artifacts := make(artifactSet, len(obj.Spec.Include)) + for i, incl := range obj.Spec.Include { + // Retrieve the included GitRepository. dep := &sourcev1.GitRepository{} if err := r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: incl.GitRepositoryRef.Name}, dep); err != nil { - e := serror.NewGeneric( + e := serror.NewWaiting( fmt.Errorf("could not get resource for include '%s': %w", incl.GitRepositoryRef.Name, err), "NotFound", ) + e.RequeueAfter = r.requeueDependency conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e + return nil, e } // Confirm include has an artifact if dep.GetArtifact() == nil { - e := serror.NewGeneric( + e := serror.NewWaiting( fmt.Errorf("no artifact available for include '%s'", incl.GitRepositoryRef.Name), "NoArtifact", ) + e.RequeueAfter = r.requeueDependency conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e + return nil, e } - // Copy artifact (sub)contents to configured directory - if err := r.Storage.CopyToPath(dep.GetArtifact(), incl.GetFromPath(), toPath); err != nil { - e := serror.NewGeneric( - fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err), - "CopyFailure", - ) - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error()) - return sreconcile.ResultEmpty, e - } artifacts[i] = dep.GetArtifact().DeepCopy() } - // We now know all includes are available + // We now know all the includes are available. conditions.Delete(obj, sourcev1.IncludeUnavailableCondition) - // Observe if the artifacts still match the previous included ones - if artifacts.Diff(obj.Status.IncludedArtifacts) { - message := fmt.Sprintf("included artifacts differ from last observed includes") - conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message) - conditions.MarkReconciling(obj, "IncludeChange", message) + return &artifacts, nil +} + +// calculateContentConfigChecksum calculates a checksum of all the +// configurations that result in a change in the source artifact. It can be used +// to decide if further reconciliation is needed when an artifact already exists +// for a set of configurations. +func (r *GitRepositoryReconciler) calculateContentConfigChecksum(obj *sourcev1.GitRepository, includes *artifactSet) string { + c := []byte{} + // Consider the ignore rules and recurse submodules. + if obj.Spec.Ignore != nil { + c = append(c, []byte(*obj.Spec.Ignore)...) } + c = append(c, []byte(strconv.FormatBool(obj.Spec.RecurseSubmodules))...) - // Persist the artifactSet. - *includes = artifacts - return sreconcile.ResultSuccess, nil + // Consider the included repository attributes. + for _, incl := range obj.Spec.Include { + c = append(c, []byte(incl.GitRepositoryRef.Name+incl.FromPath+incl.ToPath)...) + } + + // Consider the checksum and revision of all the included remote artifact. + // This ensures that if the included repos get updated, this checksum changes. + // NOTE: The content of an artifact may change at the same revision if the + // ignore rules change. Hence, consider both checksum and revision to + // capture changes in artifact checksum as well. + // TODO: Fix artifactSet.Diff() to consider checksum as well. + if includes != nil { + for _, incl := range *includes { + c = append(c, []byte(incl.Checksum)...) + c = append(c, []byte(incl.Revision)...) + } + } + + return fmt.Sprintf("sha256:%x", sha256.Sum256(c)) } // verifyCommitSignature verifies the signature of the given Git commit, if a diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index b88f2e014..fd78abcde 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -57,6 +57,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + serror "github.com/fluxcd/source-controller/internal/error" "github.com/fluxcd/source-controller/internal/features" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" @@ -141,6 +142,7 @@ Oomb3gD/TRf/nAdVED+k81GdLzciYdUGtI71/qI47G0nMBluLRE= =/4e+ -----END PGP PUBLIC KEY BLOCK----- ` + emptyContentConfigChecksum = "sha256:fcbcf165908dd18a9e49f7ff27810176db8e9f63b4352213741664245224f8aa" ) var ( @@ -551,27 +553,31 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) want sreconcile.Result wantErr bool wantRevision string + wantArtifactOutdated bool }{ { - name: "Nil reference (default branch)", - want: sreconcile.ResultSuccess, - wantRevision: "master/", + name: "Nil reference (default branch)", + want: sreconcile.ResultSuccess, + wantRevision: "master/", + wantArtifactOutdated: true, }, { name: "Branch", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: true, }, { name: "Tag", reference: &sourcev1.GitRepositoryRef{ Tag: "v0.1.0", }, - want: sreconcile.ResultSuccess, - wantRevision: "v0.1.0/", + want: sreconcile.ResultSuccess, + wantRevision: "v0.1.0/", + wantArtifactOutdated: true, }, { name: "Branch commit", @@ -580,8 +586,9 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: true, }, { name: "Branch commit", @@ -590,60 +597,81 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) Branch: "staging", Commit: "", }, - want: sreconcile.ResultSuccess, - wantRevision: "HEAD/", + want: sreconcile.ResultSuccess, + wantRevision: "HEAD/", + wantArtifactOutdated: true, }, { name: "SemVer", reference: &sourcev1.GitRepositoryRef{ SemVer: "*", }, - want: sreconcile.ResultSuccess, - wantRevision: "v2.0.0/", + want: sreconcile.ResultSuccess, + wantRevision: "v2.0.0/", + wantArtifactOutdated: true, }, { name: "SemVer range", reference: &sourcev1.GitRepositoryRef{ SemVer: "", + want: sreconcile.ResultSuccess, + wantRevision: "0.2.0/", + wantArtifactOutdated: true, }, { name: "SemVer prerelease", reference: &sourcev1.GitRepositoryRef{ SemVer: ">=1.0.0-0 <1.1.0-0", }, - wantRevision: "v1.0.0-alpha/", - want: sreconcile.ResultSuccess, + wantRevision: "v1.0.0-alpha/", + want: sreconcile.ResultSuccess, + wantArtifactOutdated: true, }, { - name: "Optimized clone, Ready=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), }, + // Checksum with all the relevant fields unset. + ContentConfigChecksum: emptyContentConfigChecksum, } - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultEmpty, - wantErr: true, - wantRevision: "staging/", + want: sreconcile.ResultEmpty, + wantErr: true, + wantRevision: "staging/", + wantArtifactOutdated: false, }, { - name: "Optimized clone, Ready=False", + name: "Optimized clone different ignore", reference: &sourcev1.GitRepositoryRef{ Branch: "staging", }, beforeFunc: func(obj *sourcev1.GitRepository, latestRev string) { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "not ready") + // Set new ignore value. + obj.Spec.Ignore = pointer.StringPtr("foo") + // Add existing artifact on the object and storage. + obj.Status = sourcev1.GitRepositoryStatus{ + Artifact: &sourcev1.Artifact{ + Revision: "staging/" + latestRev, + Path: randStringRunes(10), + }, + // Checksum with all the relevant fields unset. + ContentConfigChecksum: emptyContentConfigChecksum, + } + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") }, - want: sreconcile.ResultSuccess, - wantRevision: "staging/", + want: sreconcile.ResultSuccess, + wantRevision: "staging/", + wantArtifactOutdated: false, }, } @@ -721,7 +749,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) if tt.wantRevision != "" && !tt.wantErr { revision := strings.ReplaceAll(tt.wantRevision, "", 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)) } }) } @@ -780,7 +808,8 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) { beforeFunc: func(obj *sourcev1.GitRepository) { obj.Spec.Interval = metav1.Duration{Duration: interval} obj.Status.Artifact = &sourcev1.Artifact{Revision: "main/revision"} - obj.Status.IncludedArtifacts = []*sourcev1.Artifact{{Revision: "main/revision"}} + obj.Status.IncludedArtifacts = []*sourcev1.Artifact{{Revision: "main/revision", Checksum: "some-checksum"}} + obj.Status.ContentConfigChecksum = "sha256:f825d11a1c5987e033d2cb36449a3b0435a6abc9b2bfdbcdcc7c49bf40e9285d" }, afterFunc: func(t *WithT, obj *sourcev1.GitRepository) { t.Expect(obj.Status.URL).To(BeEmpty()) @@ -985,39 +1014,6 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { {name: "b", toPath: "b/", shouldExist: true}, }, want: sreconcile.ResultSuccess, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "IncludeChange", "included artifacts differ from last observed includes"), - *conditions.TrueCondition(meta.ReconcilingCondition, "IncludeChange", "included artifacts differ from last observed includes"), - }, - }, - { - name: "Include get failure makes IncludeUnavailable=True and returns error", - includes: []include{ - {name: "a", toPath: "a/"}, - }, - wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "NotFound", "could not get resource for include 'a': gitrepositories.source.toolkit.fluxcd.io \"a\" not found"), - }, - }, - { - name: "Include without an artifact makes IncludeUnavailable=True", - dependencies: []dependency{ - { - name: "a", - withArtifact: false, - conditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "Foo", "foo unavailable"), - }, - }, - }, - includes: []include{ - {name: "a", toPath: "a/"}, - }, - wantErr: true, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "NoArtifact", "no artifact available for include 'a'"), - }, }, { name: "Invalid FromPath makes IncludeUnavailable=True and returns error", @@ -1032,16 +1028,8 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "CopyFailure", "unpack/path: no such file or directory"), - }, - }, - { - name: "Outdated IncludeUnavailable is removed", - beforeFunc: func(obj *sourcev1.GitRepository) { - conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NoArtifact", "") + *conditions.TrueCondition(sourcev1.StorageOperationFailedCondition, "CopyFailure", "unpack/path: no such file or directory"), }, - want: sreconcile.ResultSuccess, - assertConditions: []metav1.Condition{}, }, } for _, tt := range tests { @@ -1111,6 +1099,11 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) { var commit git.Commit var includes artifactSet + // Build includes artifactSet. + artifactSet, err := r.fetchIncludes(ctx, obj) + g.Expect(err).ToNot(HaveOccurred()) + includes = *artifactSet + got, err := r.reconcileInclude(ctx, obj, &commit, &includes, tmpDir) g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) @@ -1815,12 +1808,25 @@ 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("content"), + } + partialCommit := git.Commit{ + Hash: git.Hash("some-hash"), + } + + noopErr := serror.NewGeneric(fmt.Errorf("some no-op error"), "NoOpReason") + noopErr.Ignore = true + tests := []struct { name string res sreconcile.Result resErr error oldObjBeforeFunc func(obj *sourcev1.GitRepository) newObjBeforeFunc func(obj *sourcev1.GitRepository) + commit git.Commit wantEvent string }{ { @@ -1835,7 +1841,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { newObjBeforeFunc: func(obj *sourcev1.GitRepository) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} }, - wantEvent: "Normal NewArtifact stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal NewArtifact stored artifact for commit 'test commit'", }, { name: "recovery from failure", @@ -1850,7 +1857,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "xxx", Checksum: "yyy"} conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, - wantEvent: "Normal Succeeded stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal Succeeded stored artifact for commit 'test commit'", }, { name: "recovery and new artifact", @@ -1865,7 +1873,8 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { obj.Status.Artifact = &sourcev1.Artifact{Revision: "aaa", Checksum: "bbb"} conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, - wantEvent: "Normal NewArtifact stored artifact for commit", + commit: concreteCommit, + wantEvent: "Normal NewArtifact stored artifact for commit 'test commit'", }, { name: "no updates", @@ -1880,6 +1889,22 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") }, }, + { + name: "no-op error result", + res: sreconcile.ResultEmpty, + resErr: noopErr, + 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: "xxx", Checksum: "yyy"} + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "ready") + }, + commit: partialCommit, // no-op will always result in partial commit. + wantEvent: "Normal Succeeded stored artifact for commit 'HEAD/some-hash'", + }, } for _, tt := range tests { @@ -1901,10 +1926,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: @@ -1920,3 +1942,203 @@ func TestGitRepositoryReconciler_notify(t *testing.T) { }) } } + +func TestGitRepositoryReconciler_fetchIncludes(t *testing.T) { + type dependency struct { + name string + withArtifact bool + conditions []metav1.Condition + } + + type include struct { + name string + fromPath string + toPath string + shouldExist bool + } + + tests := []struct { + name string + dependencies []dependency + includes []include + beforeFunc func(obj *sourcev1.GitRepository) + wantErr bool + wantArtifactSet artifactSet + assertConditions []metav1.Condition + }{ + { + name: "Existing includes", + dependencies: []dependency{ + { + name: "a", + withArtifact: true, + conditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "Foo", "foo ready"), + }, + }, + { + name: "b", + withArtifact: true, + conditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, "Bar", "bar ready"), + }, + }, + }, + includes: []include{ + {name: "a", toPath: "a/", shouldExist: true}, + {name: "b", toPath: "b/", shouldExist: true}, + }, + wantErr: false, + wantArtifactSet: []*sourcev1.Artifact{ + {Revision: "a"}, + {Revision: "b"}, + }, + }, + { + name: "Include get failure", + includes: []include{ + {name: "a", toPath: "a/"}, + }, + wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "NotFound", "could not get resource for include 'a': gitrepositories.source.toolkit.fluxcd.io \"a\" not found"), + }, + }, + { + name: "Include without an artifact makes IncludeUnavailable=True", + dependencies: []dependency{ + { + name: "a", + withArtifact: false, + conditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "Foo", "foo unavailable"), + }, + }, + }, + includes: []include{ + {name: "a", toPath: "a/"}, + }, + wantErr: true, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.IncludeUnavailableCondition, "NoArtifact", "no artifact available for include 'a'"), + }, + }, + { + name: "Outdated IncludeUnavailable is removed", + beforeFunc: func(obj *sourcev1.GitRepository) { + conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "NoArtifact", "") + }, + assertConditions: []metav1.Condition{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + var depObjs []client.Object + for _, d := range tt.dependencies { + obj := &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: d.name, + }, + Status: sourcev1.GitRepositoryStatus{ + Conditions: d.conditions, + }, + } + if d.withArtifact { + obj.Status.Artifact = &sourcev1.Artifact{ + Path: d.name + ".tar.gz", + Revision: d.name, + LastUpdateTime: metav1.Now(), + } + } + depObjs = append(depObjs, obj) + } + + builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()) + if len(tt.dependencies) > 0 { + builder.WithObjects(depObjs...) + } + + r := &GitRepositoryReconciler{ + Client: builder.Build(), + EventRecorder: record.NewFakeRecorder(32), + } + + obj := &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reconcile-include", + }, + Spec: sourcev1.GitRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + }, + } + + for i, incl := range tt.includes { + incl := sourcev1.GitRepositoryInclude{ + GitRepositoryRef: meta.LocalObjectReference{Name: incl.name}, + FromPath: incl.fromPath, + ToPath: incl.toPath, + } + tt.includes[i].fromPath = incl.GetFromPath() + tt.includes[i].toPath = incl.GetToPath() + obj.Spec.Include = append(obj.Spec.Include, incl) + } + + gotArtifactSet, err := r.fetchIncludes(ctx, obj) + g.Expect(err != nil).To(Equal(tt.wantErr)) + g.Expect(obj.GetConditions()).To(conditions.MatchConditions(tt.assertConditions)) + if !tt.wantErr && gotArtifactSet != nil { + g.Expect(gotArtifactSet.Diff(tt.wantArtifactSet)).To(BeFalse()) + } + }) + } +} + +func TestGitRepositoryReconciler_calculateContentConfigChecksum(t *testing.T) { + g := NewWithT(t) + obj := &sourcev1.GitRepository{} + r := &GitRepositoryReconciler{} + + emptyChecksum := r.calculateContentConfigChecksum(obj, nil) + g.Expect(emptyChecksum).To(Equal(emptyContentConfigChecksum)) + + // Ignore modified. + obj.Spec.Ignore = pointer.String("some-rule") + ignoreModChecksum := r.calculateContentConfigChecksum(obj, nil) + g.Expect(emptyChecksum).ToNot(Equal(ignoreModChecksum)) + + // Recurse submodules modified. + obj.Spec.RecurseSubmodules = true + submodModChecksum := r.calculateContentConfigChecksum(obj, nil) + g.Expect(ignoreModChecksum).ToNot(Equal(submodModChecksum)) + + // Include modified. + obj.Spec.Include = []sourcev1.GitRepositoryInclude{ + { + GitRepositoryRef: meta.LocalObjectReference{Name: "foo"}, + FromPath: "aaa", + ToPath: "bbb", + }, + } + artifacts := &artifactSet{ + &sourcev1.Artifact{Revision: "some-revision-1", Checksum: "some-checksum-1"}, + } + includeModChecksum := r.calculateContentConfigChecksum(obj, artifacts) + g.Expect(submodModChecksum).ToNot(Equal(includeModChecksum)) + + // Artifact modified revision. + artifacts = &artifactSet{ + &sourcev1.Artifact{Revision: "some-revision-2", Checksum: "some-checksum-1"}, + } + artifactModChecksum := r.calculateContentConfigChecksum(obj, artifacts) + g.Expect(includeModChecksum).ToNot(Equal(artifactModChecksum)) + + // Artifact modified checksum. + artifacts = &artifactSet{ + &sourcev1.Artifact{Revision: "some-revision-2", Checksum: "some-checksum-2"}, + } + artifactCsumModChecksum := r.calculateContentConfigChecksum(obj, artifacts) + g.Expect(artifactModChecksum).ToNot(Equal(artifactCsumModChecksum)) +} diff --git a/docs/api/source.md b/docs/api/source.md index f10fd0019..521571ead 100644 --- a/docs/api/source.md +++ b/docs/api/source.md @@ -1656,6 +1656,26 @@ Artifacts as instructed by GitRepositorySpec.Include.

+contentConfigChecksum
+ +string + + + +(Optional) +

ContentConfigChecksum is a checksum of all the configurations related to +the content of the source artifact: +- .spec.ignore +- .spec.recurseSubmodules +- .spec.included and the checksum of the included artifacts +observed in .status.observedGeneration version of the object. This can +be used to determine if the content of the included repository has +changed. +It has the format of <algo>:<checksum>, for example: sha256:<checksum>.

+ + + + ReconcileRequestStatus
diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 2d95db474..3275c32ca 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -405,9 +405,12 @@ Optimized Git clones decreases resource utilization for GitRepository 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. +When enabled, it avoids full Git clone operations by first checking whether +the revision of the last stored artifact is still the head of the remote +repository and none of the other factors that contribute to a change in the +artifact, like ignore rules and included repositories, have changed. If that is +so, the reconciliation is skipped. Else, a full reconciliation is performed as +usual. This feature is enabled by default. It can be disabled by starting the controller with the argument `--feature-gates=OptimizedGitClones=false`. @@ -838,6 +841,13 @@ Note that a GitRepository can be [reconciling](#reconciling-gitrepository) while failing at the same time, for example due to a newly introduced configuration issue in the GitRepository spec. +### Content Configuration Checksum + +The source-controller calculates the SHA256 checksum of the various +configurations of the GitRepository that indicate a change in source and +records it in `.status.contentConfigChecksum`. This field is used to determine +if the source artifact needs to be rebuilt. + ### Observed Generation The source-controller reports an [observed generation][typical-status-properties] diff --git a/pkg/git/git.go b/pkg/git/git.go index da0e7d225..5ce6fb09a 100644 --- a/pkg/git/git.go +++ b/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.