diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 07afd3983..56c141716 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -141,12 +141,7 @@ func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, o 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 + r.features = features.FeatureGates() } return ctrl.NewControllerManagedBy(mgr). @@ -427,8 +422,13 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, // change, it short-circuits the whole reconciliation with an early return. func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, obj *sourcev1.GitRepository, commit *git.Commit, includes *artifactSet, dir string) (sreconcile.Result, error) { + gitImplementation := obj.Spec.GitImplementation + if goGitOnly, _ := r.features[features.ForceGoGitImplementation]; goGitOnly { + gitImplementation = sourcev1.GoGitImplementation + } + // Exit early, if we need to use libgit2 AND managed transport hasn't been intialized. - if !r.Libgit2TransportInitialized() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation { + if !r.Libgit2TransportInitialized() && gitImplementation == sourcev1.LibGit2Implementation { return sreconcile.ResultEmpty, serror.NewStalling( errors.New("libgit2 managed transport not initialized"), "Libgit2TransportNotEnabled", ) @@ -504,7 +504,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, optimizedClone = true } - c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone, gitImplementation) if err != nil { return sreconcile.ResultEmpty, err } @@ -538,7 +538,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, // If we can't skip the reconciliation, checkout again without any // optimization. - c, err := r.gitCheckout(ctx, obj, authOpts, dir, false) + c, err := r.gitCheckout(ctx, obj, authOpts, dir, false, gitImplementation) if err != nil { return sreconcile.ResultEmpty, err } @@ -730,7 +730,8 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, // gitCheckout builds checkout options with the given configurations and // performs a git checkout. func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, - obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) { + obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, + optimized bool, gitImplementation string) (*git.Commit, error) { // Configure checkout strategy. cloneOpts := git.CloneOptions{ RecurseSubmodules: obj.Spec.RecurseSubmodules, @@ -758,18 +759,17 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, var gitReader git.RepositoryReader var err error - switch obj.Spec.GitImplementation { + switch gitImplementation { case sourcev1.LibGit2Implementation: gitReader, err = libgit2.NewClient(dir, authOpts) case sourcev1.GoGitImplementation: gitReader, err = gogit.NewClient(dir, authOpts) default: - err = fmt.Errorf("invalid Git implementation: %s", obj.Spec.GitImplementation) + err = fmt.Errorf("invalid Git implementation: %s", gitImplementation) } if err != nil { - // Do not return err as recovery without changes is impossible. - e := serror.NewStalling( - fmt.Errorf("failed to create Git client for implementation '%s': %w", obj.Spec.GitImplementation, err), + e := serror.NewGeneric( + fmt.Errorf("failed to create Git client for implementation '%s': %w", gitImplementation, err), sourcev1.GitOperationFailedReason, ) conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c25b6f6df..b92a98367 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -498,10 +498,14 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { } r := &GitRepositoryReconciler{ - Client: builder.Build(), - EventRecorder: record.NewFakeRecorder(32), - Storage: testStorage, - features: features.FeatureGates(), + Client: builder.Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + features: map[string]bool{ + features.OptimizedGitClones: true, + // Ensure that both implementations are tested. + features.ForceGoGitImplementation: false, + }, Libgit2TransportInitialized: transport.Enabled, } @@ -543,10 +547,12 @@ func TestGitRepositoryReconciler_reconcileSource_libgit2TransportUninitialized(t g := NewWithT(t) r := &GitRepositoryReconciler{ - Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), - EventRecorder: record.NewFakeRecorder(32), - Storage: testStorage, - features: features.FeatureGates(), + Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + features: map[string]bool{ + features.ForceGoGitImplementation: false, + }, Libgit2TransportInitialized: mockTransportNotInitialized, } @@ -727,10 +733,14 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) } r := &GitRepositoryReconciler{ - Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), - EventRecorder: record.NewFakeRecorder(32), - Storage: testStorage, - features: features.FeatureGates(), + Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + features: map[string]bool{ + features.OptimizedGitClones: true, + // Ensure that both implementations are tested. + features.ForceGoGitImplementation: false, + }, Libgit2TransportInitialized: transport.Enabled, } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index b8d8c5af1..40113cc1b 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -242,11 +242,15 @@ func TestMain(m *testing.M) { } if err := (&GitRepositoryReconciler{ - Client: testEnv, - EventRecorder: record.NewFakeRecorder(32), - Metrics: testMetricsH, - Storage: testStorage, - features: features.FeatureGates(), + Client: testEnv, + EventRecorder: record.NewFakeRecorder(32), + Metrics: testMetricsH, + Storage: testStorage, + features: map[string]bool{ + features.OptimizedGitClones: true, + // Ensure that both implementations are used during tests. + features.ForceGoGitImplementation: false, + }, Libgit2TransportInitialized: transport.Enabled, }).SetupWithManager(testEnv); err != nil { panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err)) diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 7cfbfd18b..173554459 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -385,6 +385,13 @@ resume. ### Git implementation +> **_NOTE:_** `libgit2` is being deprecated. When it is used the controllers +are known to panic over long periods of time, or when under high GC pressure. +A new opt-out feature gate `ForceGoGitImplementation` was introduced, which will +use `go-git` regardless of the value defined at `.spec.gitImplementation`. +This can be disabled by starting the controller with the additional flag below: +`--feature-gates=ForceGoGitImplementation=false`. + `.spec.gitImplementation` is an optional field to change the client library implementation used for Git operations (e.g. clone, checkout). The default value is `go-git`. @@ -396,14 +403,8 @@ drawbacks. For example, not being able to make use of shallow clones forces the controller to fetch the whole Git history tree instead of a specific one, resulting in an increase of disk space and traffic usage. -| Git Implementation | Shallow Clones | Git Submodules | V2 Protocol Support | -|--------------------|----------------|----------------|---------------------| -| `go-git` | true | true | false | -| `libgit2` | false | false | true | - -Some Git providers like Azure DevOps _require_ the `libgit2` implementation, as -their Git servers provide only support for the -[v2 protocol](https://git-scm.com/docs/protocol-v2). +**Note:** The `libgit2` implementation does not support shallow clones or +Git submodules. #### Optimized Git clones diff --git a/internal/features/features.go b/internal/features/features.go index 0449cf41a..cfc887611 100644 --- a/internal/features/features.go +++ b/internal/features/features.go @@ -30,12 +30,29 @@ const ( // the last revision is still the same at the target repository, // and if that is so, skips the reconciliation. OptimizedGitClones = "OptimizedGitClones" + // ForceGoGitImplementation ignores the value set for gitImplementation + // and ensures that go-git is used for all GitRepository objects. + // + // Libgit2 is built in C and we use the Go bindings provided by git2go + // to cross the C-GO chasm. Unfortunately, when libgit2 is being used the + // controllers are known to panic over long periods of time, or when + // under high GC pressure. + // + // This feature gate enables the gradual deprecation of libgit2 in favour + // of go-git, which so far is the most stable of the pair. + // + // When enabled, libgit2 won't be initialized, nor will any git2go CGO + // code be called. + ForceGoGitImplementation = "ForceGoGitImplementation" ) var features = map[string]bool{ // OptimizedGitClones // opt-out from v0.25 OptimizedGitClones: true, + // ForceGoGitImplementation + // opt-out from v0.32 + ForceGoGitImplementation: true, } // DefaultFeatureGates contains a list of all supported feature gates and diff --git a/main.go b/main.go index 88d0d5136..9aec36b20 100644 --- a/main.go +++ b/main.go @@ -204,9 +204,11 @@ func main() { } storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, setupLog) - if err = transport.InitManagedTransport(); err != nil { - // Log the error, but don't exit so as to not block reconcilers that are healthy. - setupLog.Error(err, "unable to initialize libgit2 managed transport") + if gogitOnly, _ := features.Enabled(features.ForceGoGitImplementation); !gogitOnly { + if err = transport.InitManagedTransport(); err != nil { + // Log the error, but don't exit so as to not block reconcilers that are healthy. + setupLog.Error(err, "unable to initialize libgit2 managed transport") + } } if err = (&controllers.GitRepositoryReconciler{