From 39e999d6179f15755e5c14f9f3ef53547e85d5a8 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Wed, 2 Nov 2022 13:18:12 +0000 Subject: [PATCH 1/3] git: Load default feature gates Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller.go | 7 +------ controllers/gitrepository_controller_test.go | 10 ++++++---- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 07afd3983..6e0afebc3 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). diff --git a/controllers/gitrepository_controller_test.go b/controllers/gitrepository_controller_test.go index c25b6f6df..5cb0eed72 100644 --- a/controllers/gitrepository_controller_test.go +++ b/controllers/gitrepository_controller_test.go @@ -727,10 +727,12 @@ 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, + }, Libgit2TransportInitialized: transport.Enabled, } From 331fd649526dff7e0f2ed49934abc64aa6a7aa87 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Nov 2022 14:24:09 +0000 Subject: [PATCH 2/3] gogit: Add new ForceGoGitImplementation FeatureGate ForceGoGitImplementation ignores the value set for gitImplementation and ensures that go-git is used for all GitRepository objects. This can be used to confirm that Flux instances won't break if/when the libgit2 implementation was to be deprecated. When enabled, libgit2 won't be initialized, nor will any git2go cgo code be called. Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller.go | 20 ++++++++++------ controllers/gitrepository_controller_test.go | 24 +++++++++++++------- controllers/suite_test.go | 14 ++++++++---- docs/spec/v1beta2/gitrepositories.md | 17 +++++++------- internal/features/features.go | 17 ++++++++++++++ main.go | 8 ++++--- 6 files changed, 69 insertions(+), 31 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 6e0afebc3..7b189097b 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -422,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", ) @@ -499,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 } @@ -533,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 } @@ -725,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, @@ -753,18 +759,18 @@ 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), + 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 5cb0eed72..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, } @@ -732,6 +738,8 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T) 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{ From bdcf708ef8daa65bf0244d3179192598de10e421 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Fri, 11 Nov 2022 10:10:50 +0000 Subject: [PATCH 3/3] git: Replace Stalling error for git implementation Signed-off-by: Paulo Gomes --- controllers/gitrepository_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 7b189097b..56c141716 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -768,8 +768,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, err = fmt.Errorf("invalid Git implementation: %s", gitImplementation) } if err != nil { - // Do not return err as recovery without changes is impossible. - e := serror.NewStalling( + e := serror.NewGeneric( fmt.Errorf("failed to create Git client for implementation '%s': %w", gitImplementation, err), sourcev1.GitOperationFailedReason, )