Merge branch 'geo-role-keep-default-services' into 'master'

Keep default services enabled when enabling a GEO role

Closes #2818

See merge request !1963

CHANGELOG.md

@@ -7,6 +7,14 @@ omnibus-gitlab repository.
 - Remove unused Grit configuration settings
+10.0.2
+
+- Fix an issue where enabling a GitLab Geo role would also disable all default services
+
+10.0.1
+
+- No changes
+
 10.0.0
 - Use semanage instead of chcon for setting SELinux security contexts (Elliot Wright)

files/gitlab-cookbooks/package/libraries/config/gitlab.rb

@@ -31,8 +31,8 @@ module Gitlab
   role('redis_sentinel').use { RedisSentinelRole }
   role('redis_master').use { RedisMasterRole }
   role('redis_slave')
-  role('geo_primary').use { GeoPrimaryRole }
-  role('geo_secondary').use { GeoSecondaryRole }
+  role('geo_primary',   manage_services: false).use { GeoPrimaryRole }
+  role('geo_secondary', manage_services: false).use { GeoSecondaryRole }
   ## Attributes directly on the node
   attribute('registry', priority: 20).use { Registry }

files/gitlab-cookbooks/package/libraries/config/roles/default.rb

@@ -17,8 +17,8 @@
 module DefaultRole # rubocop:disable Style/MultilineIfModifier (disabled so we can use `unless defined?(DefaultRole)` at the end of the class definition)
   class << self
     def load_role
-      # Default role is only enabled if no other role is
-      return unless no_roles_enabled?
+      # Default role is only enabled if no other service role is enabled
+      return unless no_service_roles_enabled?
       service_exclusions = []
       service_exclusions << 'rails' if Gitlab['gitlab_rails']['enable'] == false
@@ -26,8 +26,8 @@ module DefaultRole # rubocop:disable Style/MultilineIfModifier (disabled so we c
       Services.enable_group(Services::DEFAULT_GROUP, except: service_exclusions)
     end
-    def no_roles_enabled?
-      Gitlab.roles.select { |key, _value| Gitlab["#{key}_role"]['enable'] }.count.zero?
+    def no_service_roles_enabled?
+      Gitlab.roles.select { |key, role| role[:manage_services] && Gitlab["#{key}_role"]['enable'] }.count.zero?
     end
   end
 end unless defined?(DefaultRole) # Prevent reloading during converge, so we can test

files/gitlab-cookbooks/package/libraries/helpers/settings_helper.rb

@@ -54,12 +54,17 @@ module SettingsHelper
   end
   # Create a new role with the given 'name' config
+  # config options are:
+  #  manage_services - Boolean to indicate whether the role enables/disables services. Defaults to enabled.
+  #                    If enabled, the default service role is disabled when using a different role that manages services
   # Roles are configured as Gitlab['<name>_role'] and are added to the node as node['roles']['<name>']
   # ex: some_specific_role['enable'] = true
   #     will result in Gitlab['some_specific_role']['enable'] = true
   #     and node['roles']['some-specific']['enable'] = true
   def role(name, **config)
-    @roles[name] = HandledHash.new.merge!(config)
+    @roles[name] = HandledHash.new.merge!(
+      { manage_services: true }
+    ).merge(config)
     send("#{name}_role", Gitlab::ConfigMash.new)
     @roles[name]
   end

spec/chef/gitlab-ee/libraries/gitlab_geo_spec.rb

@@ -8,6 +8,15 @@ describe GitlabGeo do
     allow(Gitlab).to receive(:[]).and_call_original
   end
+  shared_examples 'default services' do
+    it 'remain enabled' do
+      chef_run
+      default_services = Services.find_by_group(Services::DEFAULT_GROUP).keys
+      expect(default_services.count).to be > 0
+      default_services.each { |service| expect(Services.enabled?(service)).to be(true) }
+    end
+  end
+
   context 'when geo_primary_role enabled' do
     cached(:chef_run) do
       RSpec::Mocks.with_temporary_scope do
@@ -16,6 +25,8 @@ describe GitlabGeo do
       ChefSpec::SoloRunner.converge('gitlab-ee::default')
     end
+    it_behaves_like 'default services'
+
     context 'in geo_logcursor settings' do
       it 'is not enabled' do
         expect(node['gitlab']['geo-logcursor']['enable']).to eq(nil)
@@ -59,6 +70,8 @@ describe GitlabGeo do
       ChefSpec::SoloRunner.converge('gitlab-ee::default')
     end
+    it_behaves_like 'default services'
+
     context 'in geo_postgres settings' do
       it 'is enabled' do
         expect(node['gitlab']['geo-postgresql']['enable']).to eq(true)

spec/libraries/gitlab_spec.rb

@@ -18,9 +18,16 @@ describe Gitlab do
   end
   it 'properly defines roles' do
-    Gitlab.role('test_node')
+    role = Gitlab.role('test_node')
     expect(Gitlab['test_node_role']).not_to be_nil
     expect(Gitlab.hyphenate_config_keys['roles']).to include('test-node')
+    expect(role).to include(manage_services: true)
+  end
+
+  it 'supports overriding role default configuration' do
+    role = Gitlab.role('test_node', manage_services: false)
+    expect(Gitlab['test_node_role']).not_to be_nil
+    expect(role).to include(manage_services: false)
   end
   it 'supports overriding attribute default configuration' do

spec/libraries/roles_spec.rb

@@ -9,6 +9,7 @@ describe 'GitLabRoles' do
   describe 'DefaultRole' do
     before do
       allow(DefaultRole).to receive(:load_role).and_call_original
+      allow(GeoPrimaryRole).to receive(:load_role).and_call_original
     end
     it 'enables the default services when no other roles are active' do
@@ -17,6 +18,15 @@ describe 'GitLabRoles' do
       expect(Services).to have_received(:enable_group).with(Services::DEFAULT_GROUP, anything).once
     end
+    it 'enables the default services when no "service managed" roles are active' do
+      stub_gitlab_rb(geo_primary_role: { enable: true })
+
+      Gitlab.load_roles
+
+      expect(Services).to have_received(:enable_group).with(Services::DEFAULT_GROUP, anything).once
+      expect(GeoPrimaryRole).to have_received(:load_role)
+    end
+
     it 'leaves the default services disabled when another role is active' do
       stub_gitlab_rb(application_role: { enable: true })