Commit 48f36bdd authored by Kamil Trzciński's avatar Kamil Trzciński

Fix `hot-reloading` of feature flag definitions

It appears that during `hot-reload` there is very
often the race condition: class gets loaded,
but we cannot properly trigger `load_all!`
before code might be interested in checking feature flag
state

There's also one missing `type: :ops` for supporting `sidekiq`
parent 0d076e5e
---
name: gitlab_sidekiq_enable_semi_reliable_fetcher
introduced_by_url:
rollout_issue_url:
group:
type: ops
default_enabled: true
---
name: gitlab_sidekiq_reliable_fetcher
introduced_by_url:
rollout_issue_url:
group:
type: ops
default_enabled: true
......@@ -3,13 +3,13 @@
def enable_reliable_fetch?
return true unless Feature::FlipperFeature.table_exists?
Feature.enabled?(:gitlab_sidekiq_reliable_fetcher, default_enabled: true)
Feature.enabled?(:gitlab_sidekiq_reliable_fetcher, type: :ops, default_enabled: true)
end
def enable_semi_reliable_fetch_mode?
return true unless Feature::FlipperFeature.table_exists?
Feature.enabled?(:gitlab_sidekiq_enable_semi_reliable_fetcher, default_enabled: true)
Feature.enabled?(:gitlab_sidekiq_enable_semi_reliable_fetcher, type: :ops, default_enabled: true)
end
# Custom Queues configuration
......
......@@ -138,7 +138,7 @@ class Feature
def register_definitions
return unless check_feature_flags_definition?
Feature::Definition.load_all!
Feature::Definition.reload!
end
def register_hot_reloader
......
......@@ -84,17 +84,14 @@ class Feature
end
def definitions
@definitions ||= {}
# We lazily load all definitions
# The hot reloading might request a feature flag
# before we can properly call `load_all!`
@definitions ||= load_all!
end
def load_all!
definitions.clear
paths.each do |glob_path|
load_all_from_path!(glob_path)
end
definitions
def reload!
@definitions = load_all!
end
def valid_usage!(key, type:, default_enabled:)
......@@ -110,9 +107,7 @@ class Feature
def register_hot_reloader!
# Reload feature flags on change of this file or any `.yml`
file_watcher = Rails.configuration.file_watcher.new(reload_files, reload_directories) do
# We use `Feature::Definition` as on Ruby code-reload
# a new class definition is created
Feature::Definition.load_all!
Feature::Definition.reload!
end
Rails.application.reloaders << file_watcher
......@@ -123,6 +118,16 @@ class Feature
private
def load_all!
# We currently do not load feature flag definitions
# in production environments
return [] unless Gitlab.dev_or_test_env?
paths.each_with_object({}) do |glob_path, definitions|
load_all_from_path!(definitions, glob_path)
end
end
def load_from_file(path)
definition = File.read(path)
definition = YAML.safe_load(definition)
......@@ -133,7 +138,7 @@ class Feature
raise Feature::InvalidFeatureFlagError, "Invalid definition for `#{path}`: #{e.message}"
end
def load_all_from_path!(glob_path)
def load_all_from_path!(definitions, glob_path)
Dir.glob(glob_path).each do |path|
definition = load_from_file(path)
......@@ -146,7 +151,7 @@ class Feature
end
def reload_files
[File.expand_path(__FILE__)]
[]
end
def reload_directories
......
......@@ -114,34 +114,32 @@ RSpec.describe Feature::Definition do
File.join(store2, '**', '*.yml')
]
)
# We stub `definitions` to ensure that they
# are not overwritten by `.load_all!`
allow(described_class).to receive(:definitions).and_return(definitions)
end
subject { described_class.send(:load_all!) }
it "when there's no feature flags a list of definitions is empty" do
expect(described_class.load_all!).to be_empty
is_expected.to be_empty
end
it "when there's a single feature flag it properly loads them" do
write_feature_flag(store1, path, yaml_content)
expect(described_class.load_all!).to be_one
is_expected.to be_one
end
it "when the same feature flag is stored multiple times raises exception" do
write_feature_flag(store1, path, yaml_content)
write_feature_flag(store2, path, yaml_content)
expect { described_class.load_all! }
expect { subject }
.to raise_error(/Feature flag 'feature_flag' is already defined/)
end
it "when one of the YAMLs is invalid it does raise exception" do
write_feature_flag(store1, path, '{}')
expect { described_class.load_all! }
expect { subject }
.to raise_error(/Feature flag is missing name/)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment