Merge pull request #945 from pjbgf/feature-nolibgit2

gogit: Add new ForceGoGitImplementation FeatureGate

Author: Paulo Gomes

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())

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,
 	}
 

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))

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
 

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

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{