Automatically detect Gitaly feature flags

For Gitaly developers, Rails was a required codebase to change when
working with feature flags. That was great for our throughput numbers,
but not a great change to make. With this change, each Gitaly feature
flag will be detected and send to the Gitaly server by default.

Given this change relies on a full set of the persisted feature flags,
the potentially stale cache is used. This reduces load on the database
server, and given on GitLab.com has many nodes this would still give the
illusion of starting to use the feature flag right away. For our
customers the impact is larger, as they might need to wait upto a
minute, though on average 30s, for a feature flag to take effect.

doc/development/gitaly.md

@@ -298,27 +298,12 @@ Here are the steps to gate a new feature in Gitaly behind a feature flag.
 
 ### GitLab Rails
 
-1. In GitLab Rails:
-
-   1. Add the feature flag to `SERVER_FEATURE_FLAGS` in `lib/feature/gitaly.rb`:
-
-      ```ruby
-      SERVER_FEATURE_FLAGS = %w[go-find-all-tags].freeze
-      ```
-
-   1. Search for `["gitaly"]["features"]` (currently in `spec/requests/api/internal/base_spec.rb`)
-      and fix the expected results for the tests by adding the new feature flag into it:
-
-      ```ruby
-      expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-get-all-lfs-pointers-go' => 'true', 'gitaly-feature-go-find-all-tags' => 'true')
-      ```
-
 1. Test in a Rails console by setting the feature flag:
 
    NOTE: **Note:**
    Pay attention to the name of the flag and the one used in the Rails console.
    There is a difference between them (dashes replaced by underscores and name
-   prefix is changed).
+   prefix is changed). Make sure to prefix all flags with `gitaly_`.
 
    ```ruby
    Feature.enable('gitaly_go_find_all_tags')

lib/feature.rb

@@ -32,6 +32,8 @@ class Feature
     end
 
     def persisted_names
+      return [] unless Gitlab::Database.exists?
+
       Gitlab::SafeRequestStore[:flipper_persisted_names] ||=
         begin
           # We saw on GitLab.com, this database request was called 2300

lib/feature/gitaly.rb

 # frozen_string_literal: true
 
-require 'set'
-
 class Feature
   class Gitaly
-    # Server feature flags should use '_' to separate words.
-    SERVER_FEATURE_FLAGS =
-      %w[
-        cache_invalidator
-        inforef_uploadpack_cache
-        commit_without_batch_check
-        use_core_delta_islands
-        use_git_protocol_v2
-      ].freeze
-
-    DEFAULT_ON_FLAGS = Set.new([]).freeze
+    PREFIX = "gitaly_"
 
     class << self
       def enabled?(feature_flag)
         return false unless Feature::FlipperFeature.table_exists?
 
-        default_on = DEFAULT_ON_FLAGS.include?(feature_flag)
-        Feature.enabled?("gitaly_#{feature_flag}", default_enabled: default_on)
+        Feature.enabled?("#{PREFIX}#{feature_flag}")
       rescue ActiveRecord::NoDatabaseError, PG::ConnectionBad
         false
       end
 
       def server_feature_flags
-        SERVER_FEATURE_FLAGS.map do |f|
-          ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s]
+        Feature.persisted_names
+          .select { |f| f.start_with?(PREFIX) }
+          .map do |f|
+          flag = f.delete_prefix(PREFIX)
+
+          ["gitaly-feature-#{flag.tr('_', '-')}", enabled?(flag).to_s]
         end.to_h
       end
     end

spec/lib/feature/gitaly_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 describe Feature::Gitaly do
   let(:feature_flag) { "mep_mep" }
 
-  before do
-    stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag])
-  end
-
   describe ".enabled?" do
     context 'when the gate is closed' do
       before do
@@ -28,15 +24,13 @@ describe Feature::Gitaly do
   end
 
   describe ".server_feature_flags" do
-    context 'when one flag is disabled' do
     before do
-        stub_feature_flags(gitaly_mep_mep: false)
+      allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep foo])
     end
 
     subject { described_class.server_feature_flags }
 
     it { is_expected.to be_a(Hash) }
-      it { is_expected.to eq("gitaly-feature-mep-mep" => "false") }
-    end
+    it { is_expected.to eq("gitaly-feature-mep-mep" => "true") }
   end
 end

spec/requests/api/internal/base_spec.rb

@@ -313,6 +313,10 @@ describe API::Internal::Base do
       end
 
       context "git pull" do
+        before do
+          allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep])
+        end
+
         it "has the correct payload" do
           pull(key, project)
 
@@ -326,7 +330,7 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
+          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'true')
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
@@ -346,7 +350,6 @@ describe API::Internal::Base do
             expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
             expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
             expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
             expect(user.reload.last_activity_on).to be_nil
           end
         end
@@ -594,7 +597,6 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
         end
       end