From b35110d978d9a2061c59bcca1310b70f6f6011e1 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 13 Dec 2017 14:20:20 +0100 Subject: Move Namespace to Gitaly using OPT-OUT --- lib/gitlab/shell.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index a22a63665be..1c48143b3b5 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -228,7 +228,8 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385 def add_namespace(storage, name) - Gitlab::GitalyClient.migrate(:add_namespace) do |enabled| + Gitlab::GitalyClient.migrate(:add_namespace, + status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |enabled| if enabled gitaly_namespace_client(storage).add(name) else @@ -250,7 +251,8 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385 def rm_namespace(storage, name) - Gitlab::GitalyClient.migrate(:remove_namespace) do |enabled| + Gitlab::GitalyClient.migrate(:remove_namespace, + status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |enabled| if enabled gitaly_namespace_client(storage).remove(name) else @@ -268,7 +270,8 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385 def mv_namespace(storage, old_name, new_name) - Gitlab::GitalyClient.migrate(:rename_namespace) do |enabled| + Gitlab::GitalyClient.migrate(:rename_namespace, + status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |enabled| if enabled gitaly_namespace_client(storage).rename(old_name, new_name) else @@ -302,7 +305,8 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/385 def exists?(storage, dir_name) - Gitlab::GitalyClient.migrate(:namespace_exists) do |enabled| + Gitlab::GitalyClient.migrate(:namespace_exists, + status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |enabled| if enabled gitaly_namespace_client(storage).exists?(dir_name) else -- cgit v1.2.3 From db2433c36da6410c803163139e41228f9ae3f26b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 20:24:12 +0100 Subject: wip --- app/models/clusters/applications/prometheus.rb | 12 +++++ app/models/environment.rb | 12 +++++ app/models/project_services/prometheus_service.rb | 63 +++++++++++++++++++--- .../queries/additional_metrics_deployment_query.rb | 2 +- lib/gitlab/prometheus/queries/deployment_query.rb | 2 +- lib/gitlab/prometheus_client.rb | 36 ++++++------- 6 files changed, 99 insertions(+), 28 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 9b0787ee6ca..72651a92e54 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -14,6 +14,18 @@ module Clusters 'stable/prometheus' end + def namespace + Gitlab::Kubernetes::Helm::NAMESPACE + end + + def service_name + 'prometheus-prometheus-server' + end + + def service_port + 80 + end + def chart_values_file "#{Rails.root}/vendor/#{name}/values.yaml" end diff --git a/app/models/environment.rb b/app/models/environment.rb index bf69b4c50f0..d895550784d 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -163,6 +163,18 @@ class Environment < ActiveRecord::Base end end + def enabled_clusters + slug = self.slug + result = project.clusters.enabled.select do |cluster| + scope = cluster.environment_scope || '*' + File.fnmatch(scope, slug) + end + + # sort results by descending order based on environment_scope being longer + # thus more closely matching environment slug + result.sort_by { |cluster| cluster.environment_scope.length }.reverse! + end + def slug super.presence || generate_slug end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index fa7b3f2bcaf..bcb19eb8909 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -54,12 +54,16 @@ class PrometheusService < MonitoringService { success: false, result: err } end + def with_reactive_cache(cl, *args) + yield calculate_reactive_cache(cl, *args) + end + def environment_metrics(environment) with_reactive_cache(Gitlab::Prometheus::Queries::EnvironmentQuery.name, environment.id, &method(:rename_data_to_metrics)) end def deployment_metrics(deployment) - metrics = with_reactive_cache(Gitlab::Prometheus::Queries::DeploymentQuery.name, deployment.id, &method(:rename_data_to_metrics)) + metrics = with_reactive_cache(Gitlab::Prometheus::Queries::DeploymentQuery.name, deployment.environment.id, deployment.id, &method(:rename_data_to_metrics)) metrics&.merge(deployment_time: deployment.created_at.to_i) || {} end @@ -68,18 +72,24 @@ class PrometheusService < MonitoringService end def additional_deployment_metrics(deployment) - with_reactive_cache(Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery.name, deployment.id, &:itself) + with_reactive_cache(Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery.name, deployment.environment.id, deployment.id, &:itself) end def matched_metrics - with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, &:itself) + with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, nil, &:itself) + end + + def manual_mode? + false end # Cache metrics for specific environment - def calculate_reactive_cache(query_class_name, *args) + def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? + client = client_for_environment(environment_id) + - data = Kernel.const_get(query_class_name).new(client).query(*args) + data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) { success: true, data: data, @@ -89,12 +99,51 @@ class PrometheusService < MonitoringService { success: false, result: err.message } end - def client - @prometheus ||= Gitlab::PrometheusClient.new(api_url: api_url) + def client(environment_id) + if manual_mode? + Gitlab::PrometheusClient.new(api_url: api_url) + else + cluster(environment_id) + end + end + + def find_cluster_with_prometheus(environment_id) + clusters = if environment_id + ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] + else + project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } + end + + clusters.detect { |cluster| cluster.application_prometheus.installed? } end private + def client_for_environment(environment_id) + cluster = find_cluster_with_prometheus(environment_id) + return unless cluster + + prometheus = cluster.application_prometheus + + client_through_kube_proxy(cluster.kubeclient, + 'service', + prometheus.service_name, + prometheus.service_port, + prometheus.namespace) if cluster.kubeclient + end + + def client_through_kube_proxy(kube_client, kind, name, port, namespace = '') + rest_client = kube_client.rest_client + base_url = rest_client.url + proxy_url = kube_client.proxy_url(kind, name, port, namespace) + + Rails.logger.warn rest_client[proxy_url.sub(base_url, '')] + Rails.logger.warn proxy_url.sub(base_url, '') + + Gitlab::PrometheusClient.new(api_url: api_url, rest_client: rest_client[proxy_url.sub(base_url, '')], headers: kube_client.headers) + end + + def rename_data_to_metrics(metrics) metrics[:metrics] = metrics.delete :data metrics diff --git a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb index 69d055c901c..294a6ae34ca 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb @@ -4,7 +4,7 @@ module Gitlab class AdditionalMetricsDeploymentQuery < BaseQuery include QueryAdditionalMetrics - def query(deployment_id) + def query(environment_id, deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| query_metrics( common_query_context( diff --git a/lib/gitlab/prometheus/queries/deployment_query.rb b/lib/gitlab/prometheus/queries/deployment_query.rb index 170f483540e..6e6da593178 100644 --- a/lib/gitlab/prometheus/queries/deployment_query.rb +++ b/lib/gitlab/prometheus/queries/deployment_query.rb @@ -2,7 +2,7 @@ module Gitlab module Prometheus module Queries class DeploymentQuery < BaseQuery - def query(deployment_id) + def query(environment_id, deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| environment_slug = deployment.environment.slug diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index aa94614bf18..8ec4515fb12 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -3,10 +3,12 @@ module Gitlab # Helper methods to interact with Prometheus network services & resources class PrometheusClient - attr_reader :api_url + attr_reader :api_url, :rest_client, :headers - def initialize(api_url:) + def initialize(api_url:, rest_client: nil, headers: nil) @api_url = api_url + @rest_client = rest_client || RestClient::Resource.new(api_url) + @headers = headers || {} end def ping @@ -40,24 +42,15 @@ module Gitlab private def json_api_get(type, args = {}) - get(join_api_url(type, args)) + path = ['api', 'v1', type].join('/') + get(path, args) rescue Errno::ECONNREFUSED raise PrometheusError, 'Connection refused' end - def join_api_url(type, args = {}) - url = URI.parse(api_url) - rescue URI::Error - raise PrometheusError, "Invalid API URL: #{api_url}" - else - url.path = [url.path.sub(%r{/+\z}, ''), 'api', 'v1', type].join('/') - url.query = args.to_query - - url.to_s - end - - def get(url) - handle_response(HTTParty.get(url)) + def get(path, args) + response = rest_client[path].get(headers.merge(params: args)) + handle_response(response) rescue SocketError raise PrometheusError, "Can't connect to #{url}" rescue OpenSSL::SSL::SSLError @@ -67,15 +60,20 @@ module Gitlab end def handle_response(response) - if response.code == 200 && response['status'] == 'success' - response['data'] || {} + json_data = json_response(response) + if response.code == 200 && json_data['status'] == 'success' + json_data['data'] || {} elsif response.code == 400 - raise PrometheusError, response['error'] || 'Bad data received' + raise PrometheusError, json_data['error'] || 'Bad data received' else raise PrometheusError, "#{response.code} - #{response.body}" end end + def json_response(response) + JSON.parse(response.body) + end + def get_result(expected_type) data = yield data['result'] if data['resultType'] == expected_type -- cgit v1.2.3 From b38b5ceb8e039283e90dd323327e59c8f608c103 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 21:42:24 +0100 Subject: Move client creation to Prometheus Application, manufacture proper rest client --- app/models/clusters/applications/prometheus.rb | 15 ++++++--- app/models/project_services/prometheus_service.rb | 37 +++++------------------ lib/gitlab/prometheus_client.rb | 14 +++------ 3 files changed, 22 insertions(+), 44 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 72651a92e54..94cac9277a5 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -14,10 +14,6 @@ module Clusters 'stable/prometheus' end - def namespace - Gitlab::Kubernetes::Helm::NAMESPACE - end - def service_name 'prometheus-prometheus-server' end @@ -33,6 +29,17 @@ module Clusters def install_command Gitlab::Kubernetes::Helm::InstallCommand.new(name, chart: chart, chart_values_file: chart_values_file) end + + def proxy_client + return unless cluster.kubeclient + + kube_client = cluster.kubeclient + proxy_url = kube_client.proxy_url('service', service_name, service_port, Gitlab::Kubernetes::Helm::NAMESPACE) + + # ensures headers containing auth data are appended to original k8s client options + options = kube_client.rest_client.options.merge(headers: kube_client.headers) + RestClient::Resource.new(proxy_url, options) + end end end end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index bcb19eb8909..5e5aa92fd1a 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -86,7 +86,7 @@ class PrometheusService < MonitoringService # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? - client = client_for_environment(environment_id) + client = client(environment_id) data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) @@ -101,12 +101,16 @@ class PrometheusService < MonitoringService def client(environment_id) if manual_mode? - Gitlab::PrometheusClient.new(api_url: api_url) + Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else - cluster(environment_id) + cluster = find_cluster_with_prometheus(environment_id) + + Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) if cluster end end + private + def find_cluster_with_prometheus(environment_id) clusters = if environment_id ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] @@ -117,33 +121,6 @@ class PrometheusService < MonitoringService clusters.detect { |cluster| cluster.application_prometheus.installed? } end - private - - def client_for_environment(environment_id) - cluster = find_cluster_with_prometheus(environment_id) - return unless cluster - - prometheus = cluster.application_prometheus - - client_through_kube_proxy(cluster.kubeclient, - 'service', - prometheus.service_name, - prometheus.service_port, - prometheus.namespace) if cluster.kubeclient - end - - def client_through_kube_proxy(kube_client, kind, name, port, namespace = '') - rest_client = kube_client.rest_client - base_url = rest_client.url - proxy_url = kube_client.proxy_url(kind, name, port, namespace) - - Rails.logger.warn rest_client[proxy_url.sub(base_url, '')] - Rails.logger.warn proxy_url.sub(base_url, '') - - Gitlab::PrometheusClient.new(api_url: api_url, rest_client: rest_client[proxy_url.sub(base_url, '')], headers: kube_client.headers) - end - - def rename_data_to_metrics(metrics) metrics[:metrics] = metrics.delete :data metrics diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index 8ec4515fb12..d1875dd1042 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -3,12 +3,10 @@ module Gitlab # Helper methods to interact with Prometheus network services & resources class PrometheusClient - attr_reader :api_url, :rest_client, :headers + attr_reader :rest_client, :headers - def initialize(api_url:, rest_client: nil, headers: nil) - @api_url = api_url + def initialize(rest_client) @rest_client = rest_client || RestClient::Resource.new(api_url) - @headers = headers || {} end def ping @@ -49,7 +47,7 @@ module Gitlab end def get(path, args) - response = rest_client[path].get(headers.merge(params: args)) + response = rest_client[path].get(params: args) handle_response(response) rescue SocketError raise PrometheusError, "Can't connect to #{url}" @@ -60,7 +58,7 @@ module Gitlab end def handle_response(response) - json_data = json_response(response) + json_data = JSON.parse(response.body) if response.code == 200 && json_data['status'] == 'success' json_data['data'] || {} elsif response.code == 400 @@ -70,10 +68,6 @@ module Gitlab end end - def json_response(response) - JSON.parse(response.body) - end - def get_result(expected_type) data = yield data['result'] if data['resultType'] == expected_type -- cgit v1.2.3 From 0c802f4fba67e4fe54f9c4442d280fc7465e6b3f Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 22:40:03 +0100 Subject: Manual Configuration instead of Activation. Prometheus Service just got a bit weirder --- app/controllers/concerns/service_params.rb | 1 + app/models/project_services/prometheus_service.rb | 36 ++++++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index 3d61458c064..c1acb50b76c 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -32,6 +32,7 @@ module ServiceParams :issues_events, :issues_url, :jira_issue_transition_id, + :manual_configuration, :merge_requests_events, :mock_service_url, :namespace, diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 5e5aa92fd1a..9644c27cae8 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -7,19 +7,27 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url + boolean_accessor :manual_configuration - with_options presence: true, if: :activated? do + with_options presence: true, if: :manual_configuration? do validates :api_url, url: true end + before_save :synchronize_service_state! + after_save :clear_reactive_cache! + def initialize_properties if properties.nil? self.properties = {} end end + def show_active_box? + false + end + def title 'Prometheus' end @@ -34,6 +42,18 @@ class PrometheusService < MonitoringService def fields [ + { type: 'fieldset', + legend: 'Manual Configuration', + fields: [ + { + type: 'checkbox', + name: 'manual_configuration', + title: s_('PrometheusService|Active'), + required: true + } + ] + }, + { type: 'text', name: 'api_url', @@ -79,16 +99,11 @@ class PrometheusService < MonitoringService with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, nil, &:itself) end - def manual_mode? - false - end - # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? client = client(environment_id) - data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) { success: true, @@ -100,12 +115,13 @@ class PrometheusService < MonitoringService end def client(environment_id) - if manual_mode? + if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else cluster = find_cluster_with_prometheus(environment_id) + raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster - Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) if cluster + Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) end end @@ -125,4 +141,8 @@ class PrometheusService < MonitoringService metrics[:metrics] = metrics.delete :data metrics end + + def synchronize_service_state! + self.active = manual_configuration + end end -- cgit v1.2.3 From 249c9a8cf63b5b36b86499720804a5180e640537 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 22:56:12 +0100 Subject: Auto enable prometheus service if Prometheus is Installed --- app/models/project_services/prometheus_service.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 9644c27cae8..abd2be2e7fe 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -8,6 +8,7 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url boolean_accessor :manual_configuration + boolean_accessor :prometheus_installed with_options presence: true, if: :manual_configuration? do validates :api_url, url: true @@ -20,7 +21,7 @@ class PrometheusService < MonitoringService def initialize_properties if properties.nil? - self.properties = {} + self.properties = { prometheus_installed: false } end end @@ -118,7 +119,7 @@ class PrometheusService < MonitoringService if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else - cluster = find_cluster_with_prometheus(environment_id) + cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) @@ -127,7 +128,7 @@ class PrometheusService < MonitoringService private - def find_cluster_with_prometheus(environment_id) + def cluster_with_prometheus(environment_id = nil) clusters = if environment_id ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] else @@ -143,6 +144,7 @@ class PrometheusService < MonitoringService end def synchronize_service_state! - self.active = manual_configuration + self.active = prometheus_installed? || manual_configuration? || cluster_with_prometheus.present? + self.prometheus_installed = !manual_configuration? && cluster_with_prometheus.present? end end -- cgit v1.2.3 From 80d4c0675f5715d724be20d47cafa372524e3ed1 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 02:45:57 +0100 Subject: Add test checking if prometheus integration is enabled after prometheus is installed --- app/models/clusters/applications/prometheus.rb | 9 ++++++++ app/models/project_services/prometheus_service.rb | 20 +++++++++++------ .../projects/services/prometheus/_help.html.haml | 10 +++++++++ lib/gitlab/prometheus/queries/environment_query.rb | 26 ++++++++++------------ .../prometheus/queries/matched_metrics_query.rb | 2 +- .../additional_metrics_deployment_query_spec.rb | 2 +- .../prometheus/queries/deployment_query_spec.rb | 2 +- .../clusters/applications/prometheus_spec.rb | 18 +++++++++++++++ 8 files changed, 65 insertions(+), 24 deletions(-) create mode 100644 app/views/projects/services/prometheus/_help.html.haml diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 94cac9277a5..ac9ea707f62 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -10,6 +10,15 @@ module Clusters default_value_for :version, VERSION + state_machine :status do + after_transition any => [:installed] do |application| + application.cluster.projects.each do |project| + # raise "exe" + project.prometheus_service&.update(active: true) + end + end + end + def chart 'stable/prometheus' end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index abd2be2e7fe..2fb94c1facb 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -1,3 +1,7 @@ +module Gitlab + PrometheusError = Class.new(StandardError) +end + class PrometheusService < MonitoringService include ReactiveService @@ -8,7 +12,6 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url boolean_accessor :manual_configuration - boolean_accessor :prometheus_installed with_options presence: true, if: :manual_configuration? do validates :api_url, url: true @@ -18,10 +21,9 @@ class PrometheusService < MonitoringService after_save :clear_reactive_cache! - def initialize_properties if properties.nil? - self.properties = { prometheus_installed: false } + self.properties = { } end end @@ -54,7 +56,6 @@ class PrometheusService < MonitoringService } ] }, - { type: 'text', name: 'api_url', @@ -126,6 +127,10 @@ class PrometheusService < MonitoringService end end + def prometheus_installed? + cluster_with_prometheus.present? + end + private def cluster_with_prometheus(environment_id = nil) @@ -135,7 +140,7 @@ class PrometheusService < MonitoringService project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } end - clusters.detect { |cluster| cluster.application_prometheus.installed? } + clusters.detect { |cluster| cluster.application_prometheus&.installed? } end def rename_data_to_metrics(metrics) @@ -144,7 +149,8 @@ class PrometheusService < MonitoringService end def synchronize_service_state! - self.active = prometheus_installed? || manual_configuration? || cluster_with_prometheus.present? - self.prometheus_installed = !manual_configuration? && cluster_with_prometheus.present? + self.active = prometheus_installed? || self.manual_configuration? + + true end end diff --git a/app/views/projects/services/prometheus/_help.html.haml b/app/views/projects/services/prometheus/_help.html.haml new file mode 100644 index 00000000000..96a194b212e --- /dev/null +++ b/app/views/projects/services/prometheus/_help.html.haml @@ -0,0 +1,10 @@ +.row.prepend-top-default.append-bottom-default + %p + - unless @service.manual_configuration? + - if @service.prometheus_installed? + = link_to 'Manage installed Prometheus', project_clusters_path(@project), class: 'btn btn-cancel' + - else + = link_to 'Install Prometheus', project_clusters_path(@project), class: 'btn btn-cancel' + - else + To automatically install prometheus disable manual configuration + diff --git a/lib/gitlab/prometheus/queries/environment_query.rb b/lib/gitlab/prometheus/queries/environment_query.rb index 1d17d3cfd56..8f1453f31bf 100644 --- a/lib/gitlab/prometheus/queries/environment_query.rb +++ b/lib/gitlab/prometheus/queries/environment_query.rb @@ -2,22 +2,20 @@ module Gitlab module Prometheus module Queries class EnvironmentQuery < BaseQuery - def query(environment_id) - ::Environment.find_by(id: environment_id).try do |environment| - environment_slug = environment.slug - timeframe_start = 8.hours.ago.to_f - timeframe_end = Time.now.to_f + def query + environment_slug = environment.slug + timeframe_start = 8.hours.ago.to_f + timeframe_end = Time.now.to_f - memory_query = raw_memory_usage_query(environment_slug) - cpu_query = raw_cpu_usage_query(environment_slug) + memory_query = raw_memory_usage_query(environment_slug) + cpu_query = raw_cpu_usage_query(environment_slug) - { - memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), - memory_current: client_query(memory_query, time: timeframe_end), - cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), - cpu_current: client_query(cpu_query, time: timeframe_end) - } - end + { + memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), + memory_current: client_query(memory_query, time: timeframe_end), + cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), + cpu_current: client_query(cpu_query, time: timeframe_end) + } end end end diff --git a/lib/gitlab/prometheus/queries/matched_metrics_query.rb b/lib/gitlab/prometheus/queries/matched_metrics_query.rb index 4c3edccc71a..d21f64a252b 100644 --- a/lib/gitlab/prometheus/queries/matched_metrics_query.rb +++ b/lib/gitlab/prometheus/queries/matched_metrics_query.rb @@ -4,7 +4,7 @@ module Gitlab class MatchedMetricsQuery < BaseQuery MAX_QUERY_ITEMS = 40.freeze - def query + def query(_ = nil) groups_data.map do |group, data| { group: group.name, diff --git a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb index c7169717fc1..0697cb2def6 100644 --- a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb @@ -7,7 +7,7 @@ describe Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery do include_examples 'additional metrics query' do let(:deployment) { create(:deployment, environment: environment) } - let(:query_params) { [deployment.id] } + let(:query_params) { [environment.id, deployment.id] } it 'queries using specific time' do expect(client).to receive(:query_range).with(anything, diff --git a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb index ffe3ad85baa..d166327b6d3 100644 --- a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb @@ -31,7 +31,7 @@ describe Gitlab::Prometheus::Queries::DeploymentQuery do expect(client).to receive(:query).with('avg(rate(container_cpu_usage_seconds_total{container_name!="POD",environment="environment-slug"}[30m])) * 100', time: stop_time) - expect(subject.query(deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, + expect(subject.query(environment.id, deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, cpu_values: nil, cpu_before: nil, cpu_after: nil) end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index 696099f7cf7..b9ce597364e 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -6,6 +6,24 @@ describe Clusters::Applications::Prometheus do include_examples 'cluster application specs', described_class + describe 'transition to installed' do + let(:project) { create(:project) } + let(:cluster) { create(:cluster, projects: [project]) } + let(:prometheus_service) { double('prometheus_service') } + + subject { create(:clusters_applications_prometheus, :installing, cluster: cluster) } + + before do + allow(project).to receive(:prometheus_service).and_return prometheus_service + end + + it 'ensures Prometheus service is activated' do + expect(prometheus_service).to receive(:update).with(active: true) + + subject.make_installed + end + end + describe "#chart_values_file" do subject { create(:clusters_applications_prometheus).chart_values_file } -- cgit v1.2.3 From 9c0b10da4147470050c1ae144fc75c2963eeb4ba Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 03:13:54 +0100 Subject: Fix prometheus client tests --- lib/gitlab/prometheus_client.rb | 17 +++++++++++++---- spec/lib/gitlab/prometheus_client_spec.rb | 14 ++++++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index d1875dd1042..79f66b42c21 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -50,10 +50,12 @@ module Gitlab response = rest_client[path].get(params: args) handle_response(response) rescue SocketError - raise PrometheusError, "Can't connect to #{url}" + raise PrometheusError, "Can't connect to #{rest_client.url}" rescue OpenSSL::SSL::SSLError - raise PrometheusError, "#{url} contains invalid SSL data" - rescue HTTParty::Error + raise PrometheusError, "#{rest_client.url} contains invalid SSL data" + rescue RestClient::ExceptionWithResponse => ex + handle_exception_response(ex.response) + rescue RestClient::Exception raise PrometheusError, "Network connection error" end @@ -61,7 +63,14 @@ module Gitlab json_data = JSON.parse(response.body) if response.code == 200 && json_data['status'] == 'success' json_data['data'] || {} - elsif response.code == 400 + else + raise PrometheusError, "#{response.code} - #{response.body}" + end + end + + def handle_exception_response(response) + if response.code == 400 + json_data = JSON.parse(response.body) raise PrometheusError, json_data['error'] || 'Bad data received' else raise PrometheusError, "#{response.code} - #{response.body}" diff --git a/spec/lib/gitlab/prometheus_client_spec.rb b/spec/lib/gitlab/prometheus_client_spec.rb index de625324092..575b7805e33 100644 --- a/spec/lib/gitlab/prometheus_client_spec.rb +++ b/spec/lib/gitlab/prometheus_client_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::PrometheusClient do include PrometheusHelpers - subject { described_class.new(api_url: 'https://prometheus.example.com') } + subject { described_class.new(RestClient::Resource.new('https://prometheus.example.com')) } describe '#ping' do it 'issues a "query" request to the API endpoint' do @@ -52,11 +52,13 @@ describe Gitlab::PrometheusClient do describe 'failure to reach a provided prometheus url' do let(:prometheus_url) {"https://prometheus.invalid.example.com"} + subject { described_class.new(RestClient::Resource.new(prometheus_url)) } + context 'exceptions are raised' do it 'raises a Gitlab::PrometheusError error when a SocketError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, SocketError) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "Can't connect to #{prometheus_url}") expect(req_stub).to have_been_requested end @@ -64,15 +66,15 @@ describe Gitlab::PrometheusClient do it 'raises a Gitlab::PrometheusError error when a SSLError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, OpenSSL::SSL::SSLError) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "#{prometheus_url} contains invalid SSL data") expect(req_stub).to have_been_requested end - it 'raises a Gitlab::PrometheusError error when a HTTParty::Error is rescued' do - req_stub = stub_prometheus_request_with_exception(prometheus_url, HTTParty::Error) + it 'raises a Gitlab::PrometheusError error when a RestClient::Exception is rescued' do + req_stub = stub_prometheus_request_with_exception(prometheus_url, RestClient::Exception) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "Network connection error") expect(req_stub).to have_been_requested end -- cgit v1.2.3 From e308bb0cd2226297093ffb488a4d97e3c02c4883 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 13:56:07 +0100 Subject: Cleanup implementation and add cluster finding tests --- app/models/clusters/cluster.rb | 3 + app/models/environment.rb | 12 -- app/models/project_services/prometheus_service.rb | 22 +++- lib/gitlab/prometheus_client.rb | 2 +- .../project_services/prometheus_service_spec.rb | 127 +++++++++++++++++++++ 5 files changed, 147 insertions(+), 19 deletions(-) diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 5ecbd4cbceb..8678f70f78c 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -49,6 +49,9 @@ module Clusters scope :enabled, -> { where(enabled: true) } scope :disabled, -> { where(enabled: false) } + scope :for_environment, -> (env) { where(environment_scope: ['*', '', env.slug]) } + scope :for_all_environments, -> { where(environment_scope: ['*', '']) } + def status_name if provider provider.status_name diff --git a/app/models/environment.rb b/app/models/environment.rb index d895550784d..bf69b4c50f0 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -163,18 +163,6 @@ class Environment < ActiveRecord::Base end end - def enabled_clusters - slug = self.slug - result = project.clusters.enabled.select do |cluster| - scope = cluster.environment_scope || '*' - File.fnmatch(scope, slug) - end - - # sort results by descending order based on environment_scope being longer - # thus more closely matching environment slug - result.sort_by { |cluster| cluster.environment_scope.length }.reverse! - end - def slug super.presence || generate_slug end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 2fb94c1facb..580e2c01aaa 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -116,31 +116,41 @@ class PrometheusService < MonitoringService { success: false, result: err.message } end - def client(environment_id) + def client(environment_id = nil) if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster + rest_client = client_from_cluster(cluster) - Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) + raise Gitlab::PrometheusError, "couldn't create proxy Prometheus client" unless rest_client + Gitlab::PrometheusClient.new(rest_client) end end def prometheus_installed? - cluster_with_prometheus.present? + project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end private def cluster_with_prometheus(environment_id = nil) clusters = if environment_id - ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] + ::Environment.find_by(id: environment_id).try do |env| + # sort results by descending order based on environment_scope being longer + # thus more closely matching environment slug + project.clusters.enabled.for_environment(env).sort_by { |c| c.environment_scope&.length }.reverse! + end else - project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } + project.clusters.enabled.for_all_environments end - clusters.detect { |cluster| cluster.application_prometheus&.installed? } + clusters&.detect { |cluster| cluster.application_prometheus&.installed? } + end + + def client_from_cluster(cluster) + cluster.application_prometheus.proxy_client end def rename_data_to_metrics(metrics) diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index 79f66b42c21..8264501b1ae 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -6,7 +6,7 @@ module Gitlab attr_reader :rest_client, :headers def initialize(rest_client) - @rest_client = rest_client || RestClient::Resource.new(api_url) + @rest_client = rest_client end def ping diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index bf39e8d7a39..3cbd0bb75f7 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -8,6 +8,8 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:service) { project.prometheus_service } let(:environment_query) { Gitlab::Prometheus::Queries::EnvironmentQuery } + subject { project.prometheus_service } + describe "Associations" do it { is_expected.to belong_to :project } end @@ -132,4 +134,129 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end end end + + describe '#client' do + context 'manual configuration is enabled' do + let(:api_url) { 'http://some_url' } + before do + subject.manual_configuration = true + subject.api_url = api_url + end + + it 'returns simple rest client from api_url' do + expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client.rest_client.url).to eq(api_url) + end + end + + context 'manual configuration is disabled' do + let!(:cluster_for_all) { create(:cluster, environment_scope: '*', projects: [project]) } + let!(:cluster_for_dev) { create(:cluster, environment_scope: 'dev', projects: [project]) } + + let!(:prometheus_for_dev) { create(:clusters_applications_prometheus, :installed, cluster: cluster_for_dev) } + let(:proxy_client) { double('proxy_client') } + + before do + subject.manual_configuration = false + end + + context 'with cluster for all environments with prometheus installed' do + let!(:prometheus_for_all) { create(:clusters_applications_prometheus, :installed, cluster: cluster_for_all) } + + context 'without environment supplied' do + it 'returns client handling all environments' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + + expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client.rest_client).to eq(proxy_client) + end + end + + context 'with dev environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'dev') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + + context 'with prod environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'prod') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + end + + context 'with cluster for all environments without prometheus installed' do + context 'without environment supplied' do + it 'raises PrometheusError because cluster was not found' do + expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + end + end + + context 'with dev environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'dev') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + + context 'with prod environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'prod') } + + it 'raises PrometheusError because cluster was not found' do + expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + end + end + end + end + end + + describe '#prometheus_installed?' do + subject { project.prometheus_service } + + context 'clusters with installed prometheus' do + let!(:cluster) { create(:cluster, projects: [project]) } + let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } + + it 'returns true' do + expect(subject.prometheus_installed?).to be(true) + end + end + + context 'clusters without prometheus installed' do + let(:cluster) { create(:cluster, projects: [project]) } + let!(:prometheus) { create(:clusters_applications_prometheus, cluster: cluster) } + + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + + context 'clusters without prometheus' do + let(:cluster) { create(:cluster, projects: [project]) } + + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + + context 'no clusters' do + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + end end -- cgit v1.2.3 From 720032733ad8fdb6124f2d0eee89807196552ad3 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 15:04:14 +0100 Subject: Cleanup PrometheusService tests --- app/models/project_services/prometheus_service.rb | 4 -- lib/gitlab/prometheus/queries/environment_query.rb | 26 ++++---- .../project_services/prometheus_service_spec.rb | 71 ++++++++++++---------- 3 files changed, 52 insertions(+), 49 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 580e2c01aaa..8bb1eb28900 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -76,10 +76,6 @@ class PrometheusService < MonitoringService { success: false, result: err } end - def with_reactive_cache(cl, *args) - yield calculate_reactive_cache(cl, *args) - end - def environment_metrics(environment) with_reactive_cache(Gitlab::Prometheus::Queries::EnvironmentQuery.name, environment.id, &method(:rename_data_to_metrics)) end diff --git a/lib/gitlab/prometheus/queries/environment_query.rb b/lib/gitlab/prometheus/queries/environment_query.rb index 8f1453f31bf..1d17d3cfd56 100644 --- a/lib/gitlab/prometheus/queries/environment_query.rb +++ b/lib/gitlab/prometheus/queries/environment_query.rb @@ -2,20 +2,22 @@ module Gitlab module Prometheus module Queries class EnvironmentQuery < BaseQuery - def query - environment_slug = environment.slug - timeframe_start = 8.hours.ago.to_f - timeframe_end = Time.now.to_f + def query(environment_id) + ::Environment.find_by(id: environment_id).try do |environment| + environment_slug = environment.slug + timeframe_start = 8.hours.ago.to_f + timeframe_end = Time.now.to_f - memory_query = raw_memory_usage_query(environment_slug) - cpu_query = raw_cpu_usage_query(environment_slug) + memory_query = raw_memory_usage_query(environment_slug) + cpu_query = raw_cpu_usage_query(environment_slug) - { - memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), - memory_current: client_query(memory_query, time: timeframe_end), - cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), - cpu_current: client_query(cpu_query, time: timeframe_end) - } + { + memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), + memory_current: client_query(memory_query, time: timeframe_end), + cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), + cpu_current: client_query(cpu_query, time: timeframe_end) + } + end end end end diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 3cbd0bb75f7..1ea162f7059 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -8,24 +8,22 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:service) { project.prometheus_service } let(:environment_query) { Gitlab::Prometheus::Queries::EnvironmentQuery } - subject { project.prometheus_service } - describe "Associations" do it { is_expected.to belong_to :project } end describe 'Validations' do - context 'when service is active' do + context 'when manual_configuration is enabled' do before do - subject.active = true + subject.manual_configuration = true end it { is_expected.to validate_presence_of(:api_url) } end - context 'when service is inactive' do + context 'when manual configuration is disabled' do before do - subject.active = false + subject.manual_configuration = false end it { is_expected.not_to validate_presence_of(:api_url) } @@ -33,12 +31,17 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end describe '#test' do + before do + service.manual_configuration = true + end + let!(:req_stub) { stub_prometheus_request(prometheus_query_url('1'), body: prometheus_value_body('vector')) } context 'success' do it 'reads the discovery endpoint' do + expect(service.test[:result]).to eq('Checked API endpoint') expect(service.test[:success]).to be_truthy - expect(req_stub).to have_been_requested + expect(req_stub).to have_been_requested.twice end end @@ -85,7 +88,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:fake_deployment_time) { 10 } before do - stub_reactive_cache(service, prometheus_data, deployment_query, deployment.id) + stub_reactive_cache(service, prometheus_data, deployment_query, deployment.environment.id, deployment.id) end it 'returns reactive data' do @@ -98,13 +101,17 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do describe '#calculate_reactive_cache' do let(:environment) { create(:environment, slug: 'env-slug') } - - around do |example| - Timecop.freeze { example.run } + before do + service.manual_configuration = true + service.active = true end subject do - service.calculate_reactive_cache(environment_query.to_s, environment.id) + service.calculate_reactive_cache(environment_query.name, environment.id) + end + + around do |example| + Timecop.freeze { example.run } end context 'when service is inactive' do @@ -157,7 +164,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:proxy_client) { double('proxy_client') } before do - subject.manual_configuration = false + service.manual_configuration = false end context 'with cluster for all environments with prometheus installed' do @@ -165,10 +172,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'without environment supplied' do it 'returns client handling all environments' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice - expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client.rest_client).to eq(proxy_client) + expect(service.client).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client.rest_client).to eq(proxy_client) end end @@ -176,10 +183,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'dev') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end @@ -187,10 +194,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end end @@ -198,7 +205,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'with cluster for all environments without prometheus installed' do context 'without environment supplied' do it 'raises PrometheusError because cluster was not found' do - expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end @@ -206,10 +213,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'dev') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end @@ -217,7 +224,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'raises PrometheusError because cluster was not found' do - expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end end @@ -225,14 +232,12 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end describe '#prometheus_installed?' do - subject { project.prometheus_service } - context 'clusters with installed prometheus' do let!(:cluster) { create(:cluster, projects: [project]) } let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } it 'returns true' do - expect(subject.prometheus_installed?).to be(true) + expect(service.prometheus_installed?).to be(true) end end @@ -241,7 +246,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:prometheus) { create(:clusters_applications_prometheus, cluster: cluster) } it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end @@ -249,13 +254,13 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:cluster) { create(:cluster, projects: [project]) } it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end context 'no clusters' do it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end end -- cgit v1.2.3 From ae9c8277d9c9fdc1ee9f190b610f756fbc999863 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 15:46:32 +0100 Subject: add tests for Manual configuration override and service activation synchronization --- .../project_services/prometheus_service_spec.rb | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 1ea162f7059..aff03e28cc4 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -264,4 +264,70 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end end end + + describe '#synchronize_service_state! before_save callback' do + context 'no clusters with prometheus are installed' do + context 'when service is inactive' do + before do + service.active = false + end + + it 'activates service when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.to change { service.active }.from(false).to(true) + end + + it 'keeps service inactive when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.not_to change { service.active }.from(false) + end + end + + context 'when service is active' do + before do + service.active = true + end + + it 'keeps the service active when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.not_to change { service.active }.from(true) + end + + it 'inactivates the service when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.to change { service.active }.from(true).to(false) + end + end + end + + context 'with prometheus installed in the cluster' do + before do + allow(service).to receive(:prometheus_installed?).and_return(true) + end + + context 'when service is inactive' do + before do + service.active = false + end + + it 'activates service when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.to change { service.active }.from(false).to(true) + end + + it 'activates service when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.to change { service.active }.from(false).to(true) + end + end + + context 'when service is active' do + before do + service.active = true + end + + it 'keeps service active when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.not_to change { service.active }.from(true) + end + + it 'keeps service active when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.not_to change { service.active }.from(true) + end + end + end + end end -- cgit v1.2.3 From 09473b192c70ada66148dace8c6196ccabfa1dd9 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:01:13 +0100 Subject: Test Prometheus proxy client generation --- app/models/clusters/applications/prometheus.rb | 9 +++- .../clusters/applications/prometheus_spec.rb | 51 ++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index ac9ea707f62..8a135237b89 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -40,15 +40,20 @@ module Clusters end def proxy_client - return unless cluster.kubeclient + return unless kube_client - kube_client = cluster.kubeclient proxy_url = kube_client.proxy_url('service', service_name, service_port, Gitlab::Kubernetes::Helm::NAMESPACE) # ensures headers containing auth data are appended to original k8s client options options = kube_client.rest_client.options.merge(headers: kube_client.headers) RestClient::Resource.new(proxy_url, options) end + + private + + def kube_client + cluster&.kubeclient + end end end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index b9ce597364e..1fdfb262618 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -31,4 +31,55 @@ describe Clusters::Applications::Prometheus do expect(subject).to eq("#{Rails.root}/vendor/prometheus/values.yaml") end end + + describe '#proxy_client' do + context 'cluster is nil' do + it 'returns nil' do + expect(subject.cluster).to be_nil + expect(subject.proxy_client).to be_nil + end + end + + context "cluster doesn't have kubeclient" do + let(:cluster) { create(:cluster) } + subject { create(:clusters_applications_prometheus, cluster: cluster) } + + it 'returns nil' do + expect(subject.proxy_client).to be_nil + end + end + + context 'cluster has kubeclient' do + let(:kubernetes_url) { 'http://example.com' } + let(:k8s_discover_response) { { + resources: [ + { + name: 'service', + kind: 'Service' + }] + } } + + let(:kube_client) { Kubeclient::Client.new(kubernetes_url) } + + let(:cluster) { create(:cluster) } + subject { create(:clusters_applications_prometheus, cluster: cluster) } + + before do + allow(kube_client.rest_client).to receive(:get).and_return(k8s_discover_response.to_json) + allow(subject.cluster).to receive(:kubeclient).and_return(kube_client) + end + + it 'creates proxy prometheus rest client' do + expect(subject.proxy_client).to be_instance_of(RestClient::Resource) + end + + it 'creates proper url' do + expect(subject.proxy_client.url).to eq('http://example.com/api/v1/proxy/namespaces/gitlab-managed-apps/service/prometheus-prometheus-server:80') + end + + it 'copies options and headers from kube client to proxy client' do + expect(subject.proxy_client.options).to eq(kube_client.rest_client.options.merge(headers: kube_client.headers)) + end + end + end end -- cgit v1.2.3 From 57c1f7cae09fe60a4be8d99b64833779476f402d Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:11:39 +0100 Subject: Fix rubocop warnings --- app/models/project_services/prometheus_service.rb | 9 ++++++--- .../gitlab/prometheus/queries/deployment_query_spec.rb | 2 +- spec/models/clusters/applications/prometheus_spec.rb | 17 ++++++++++------- spec/models/project_services/prometheus_service_spec.rb | 4 ++-- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 8bb1eb28900..3bf44c1ab2b 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -23,7 +23,7 @@ class PrometheusService < MonitoringService def initialize_properties if properties.nil? - self.properties = { } + self.properties = {} end end @@ -45,7 +45,8 @@ class PrometheusService < MonitoringService def fields [ - { type: 'fieldset', + { + type: 'fieldset', legend: 'Manual Configuration', fields: [ { @@ -100,6 +101,7 @@ class PrometheusService < MonitoringService # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? + client = client(environment_id) data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) @@ -118,9 +120,10 @@ class PrometheusService < MonitoringService else cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster - rest_client = client_from_cluster(cluster) + rest_client = client_from_cluster(cluster) raise Gitlab::PrometheusError, "couldn't create proxy Prometheus client" unless rest_client + Gitlab::PrometheusClient.new(rest_client) end end diff --git a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb index d166327b6d3..84dc31d9732 100644 --- a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb @@ -32,6 +32,6 @@ describe Gitlab::Prometheus::Queries::DeploymentQuery do time: stop_time) expect(subject.query(environment.id, deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, - cpu_values: nil, cpu_before: nil, cpu_after: nil) + cpu_values: nil, cpu_before: nil, cpu_after: nil) end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index 1fdfb262618..1102249493c 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -51,13 +51,16 @@ describe Clusters::Applications::Prometheus do context 'cluster has kubeclient' do let(:kubernetes_url) { 'http://example.com' } - let(:k8s_discover_response) { { - resources: [ - { - name: 'service', - kind: 'Service' - }] - } } + let(:k8s_discover_response) do + { + resources: [ + { + name: 'service', + kind: 'Service' + } + ] + } + end let(:kube_client) { Kubeclient::Client.new(kubernetes_url) } diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index aff03e28cc4..789846250ad 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -205,7 +205,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'with cluster for all environments without prometheus installed' do context 'without environment supplied' do it 'raises PrometheusError because cluster was not found' do - expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect { service.client }.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end @@ -224,7 +224,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'raises PrometheusError because cluster was not found' do - expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect { service.client }.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end end -- cgit v1.2.3 From 1c7a61afd20b581167c76b69f0a303fcb25cd87d Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:45:35 +0100 Subject: Add Changelog item --- .../unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml diff --git a/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml b/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml new file mode 100644 index 00000000000..b2bb173912a --- /dev/null +++ b/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml @@ -0,0 +1,6 @@ +--- +title: Implement multi server support and use kube proxy to connect to Prometheus + servers inside K8S cluster +merge_request: 16182 +author: +type: added -- cgit v1.2.3 From 3727dfcd439f6224aa359e961fd6c0920e80718b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 17 Jan 2018 12:06:34 +0100 Subject: add manual_configuration to prometheus_service factory to enable it by default --- spec/factories/services.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/factories/services.rb b/spec/factories/services.rb index 110ef33c6f7..faedcc38c8d 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -30,6 +30,7 @@ FactoryBot.define do project active true properties({ + manual_configuration: true, api_url: 'https://prometheus.example.com/' }) end -- cgit v1.2.3 From 4b1d42bbc583b0884498bd129653587acc69ab0f Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 17 Jan 2018 12:29:18 +0100 Subject: check if service is template --- app/models/project_services/prometheus_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 3bf44c1ab2b..e540a2c646a 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -129,6 +129,7 @@ class PrometheusService < MonitoringService end def prometheus_installed? + return false if template? project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end -- cgit v1.2.3 From 1c0437d95e6329035af49d13b26c4a60948b02e3 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 2 Jan 2018 09:23:39 +0100 Subject: Client implementation for WikiPageVerion Part of gitlab-org/gitaly#639 --- app/controllers/projects/wikis_controller.rb | 4 +- lib/gitlab/git/wiki.rb | 38 ++++++++++++++----- lib/gitlab/gitaly_client/wiki_service.rb | 56 +++++++++++++++++++++++++--- spec/models/wiki_page_spec.rb | 32 ++++++++++++---- 4 files changed, 105 insertions(+), 25 deletions(-) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 292e4158f8b..730f20bc086 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -76,9 +76,9 @@ class Projects::WikisController < Projects::ApplicationController @page = @project_wiki.find_page(params[:id]) if @page - @page_versions = Kaminari.paginate_array(@page.versions(page: params[:page]), + @page_versions = Kaminari.paginate_array(@page.versions(page: params[:page].to_i), total_count: @page.count_versions) - .page(params[:page]) + .page(params[:page]) else redirect_to( project_wiki_path(@project, :home), diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index d4a53d32c28..305bc9b66b0 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -93,11 +93,23 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def page_versions(page_path, options = {}) - current_page = gollum_page_by_path(page_path) + puts '-' * 80 + puts options + puts '-' * 80 + puts - commits_from_page(current_page, options).map do |gitlab_git_commit| - gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id) - Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format) + byebug + @repository.gitaly_migrate(:wiki_page_versions) do |is_enabled| + if is_enabled + gitaly_wiki_client.page_versions(page_path, pagination_params(options)) + else + current_page = gollum_page_by_path(page_path) + + commits_from_page(current_page, options).map do |gitlab_git_commit| + gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id) + Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format) + end + end end end @@ -124,15 +136,21 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def commits_from_page(gollum_page, options = {}) - unless options[:limit] - options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page - options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i - end + pagination_options = pagination_params(options) @repository.log(ref: gollum_page.last_version.id, path: gollum_page.path, - limit: options[:limit], - offset: options[:offset]) + limit: pagination_options[:limit], + offset: pagination_options[:offset]) + end + + def pagination_params(options) + return options if options[:limit] + + options = options.dup + options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page + options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i + options end def gollum_wiki diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index 5c5b170a3e0..fa7e416504d 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/lib/gitlab/gitaly_client/wiki_service.rb @@ -101,6 +101,48 @@ module Gitlab pages end + # options: + # :page - The Integer page number. + # :per_page - The number of items per page. + # :limit - Total number of items to return. + def page_versions(page_path, options) + request = Gitaly::WikiGetPageVersionsRequest.new( + repository: @gitaly_repo, + page_path: encode_binary(page_path) + ) + + min_index = options[:offset].to_i + max_index = min_index + options[:limit].to_i + byebug + + stream = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_page_versions, request) + + version_index = 0 + versions = [] + + # Allow limit and offset to be send to Gitaly: TODO + stream.each do |message| + puts '§' * 80 + puts version_index + + message.versions.each do |version| + case version_index + when min_index...max_index + versions << new_wiki_page_version(version) + when max_index..Float::INFINITY + return versions + end + + version_index += 1 + puts version_index + end + end + + # when we're requesting page 99 but the stream doesn't go that far, whatever + # is fetched thus far + versions + end + def find_file(name, revision) request = Gitaly::WikiFindFileRequest.new( repository: @gitaly_repo, @@ -129,7 +171,7 @@ module Gitlab private - # If a block is given and the yielded value is true, iteration will be + # If a block is given and the yielded value is truthy, iteration will be # stopped early at that point; else the iterator is consumed entirely. # The iterator is traversed with `next` to allow resuming the iteration. def wiki_page_from_iterator(iterator) @@ -146,10 +188,7 @@ module Gitlab else wiki_page = GitalyClient::WikiPage.new(page.to_h) - version = Gitlab::Git::WikiPageVersion.new( - Gitlab::Git::Commit.decorate(@repository, page.version.commit), - page.version.format - ) + version = new_wiki_page_version(page.version) end end @@ -158,6 +197,13 @@ module Gitlab [wiki_page, version] end + def new_wiki_page_version(version) + Gitlab::Git::WikiPageVersion.new( + Gitlab::Git::Commit.decorate(@repository, version.commit), + version.format + ) + end + def gitaly_commit_details(commit_details) Gitaly::WikiCommitDetails.new( name: encode_binary(commit_details.name), diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index ea75434e399..835ac20c5d8 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -252,18 +252,34 @@ describe WikiPage do end describe "#versions" do - before do - create_page("Update", "content") - @page = wiki.find_page("Update") + shared_examples 'wiki page versions' do + let(:page) { wiki.find_page("Update") } + + before do + create_page("Update", "content") + end + + after do + destroy_page("Update") + end + + it "returns an array of all commits for the page" do + 3.times { |i| page.update(content: "content #{i}") } + + expect(page.versions.count).to eq(4) + end + + it 'returns instances of WikiPageVersion' do + expect(page.versions).to all( be_a(Gitlab::Git::WikiPageVersion) ) + end end - after do - destroy_page("Update") + context 'when Gitaly is enabled' do + it_behaves_like 'wiki page versions' end - it "returns an array of all commits for the page" do - 3.times { |i| @page.update(content: "content #{i}") } - expect(@page.versions.count).to eq(4) + context 'when Gitaly is disabled', :disable_gitaly do + it_behaves_like 'wiki page versions' end end -- cgit v1.2.3 From bd96f2d6e351229b888bf4f41f39a082d2ac16cd Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 16 Jan 2018 10:42:00 +0100 Subject: Upgrade Gitaly versions --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 6f82890d0d1..a4c3db4bf5f 100644 --- a/Gemfile +++ b/Gemfile @@ -406,7 +406,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.73.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.74.0', require: 'gitaly' gem 'toml-rb', '~> 0.3.15', require: false -- cgit v1.2.3 From a29f0c28fd07ba14f0d0e5fb9c878a2eb117e388 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 16 Jan 2018 11:12:08 +0100 Subject: Allow pagination for WikiVersions on Gitaly request --- Gemfile.lock | 4 ++-- lib/gitlab/git/wiki.rb | 32 +++++++++++++------------------- lib/gitlab/gitaly_client/wiki_service.rb | 26 ++++---------------------- 3 files changed, 19 insertions(+), 43 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 284e1e7654d..d69c532b309 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -285,7 +285,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.73.0) + gitaly-proto (0.74.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -1056,7 +1056,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly-proto (~> 0.73.0) + gitaly-proto (~> 0.74.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.6.2) diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index 305bc9b66b0..4ed78daa443 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -93,15 +93,15 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def page_versions(page_path, options = {}) - puts '-' * 80 - puts options - puts '-' * 80 - puts - - byebug @repository.gitaly_migrate(:wiki_page_versions) do |is_enabled| if is_enabled - gitaly_wiki_client.page_versions(page_path, pagination_params(options)) + versions = gitaly_wiki_client.page_versions(page_path, options) + + # Gitaly uses gollum-lib to get the versions. Gollum defaults to 20 + # per page, but also fetches 20 if `limit` or `per_page` < 20. + # Slicing returns an array with the expected number of items. + slice_bound = options[:limit] || options[:per_page] || Gollum::Page.per_page + versions[0..slice_bound] else current_page = gollum_page_by_path(page_path) @@ -136,21 +136,15 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def commits_from_page(gollum_page, options = {}) - pagination_options = pagination_params(options) + unless options[:limit] + options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page + options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i + end @repository.log(ref: gollum_page.last_version.id, path: gollum_page.path, - limit: pagination_options[:limit], - offset: pagination_options[:offset]) - end - - def pagination_params(options) - return options if options[:limit] - - options = options.dup - options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page - options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i - options + limit: options[:limit], + offset: options[:offset]) end def gollum_wiki diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index fa7e416504d..a98c3c0b160 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/lib/gitlab/gitaly_client/wiki_service.rb @@ -108,38 +108,20 @@ module Gitlab def page_versions(page_path, options) request = Gitaly::WikiGetPageVersionsRequest.new( repository: @gitaly_repo, - page_path: encode_binary(page_path) + page_path: encode_binary(page_path), + page: options[:page] || 1, + per_page: options[:per_page] || Gollum::Page.per_page ) - min_index = options[:offset].to_i - max_index = min_index + options[:limit].to_i - byebug - stream = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_page_versions, request) - version_index = 0 versions = [] - - # Allow limit and offset to be send to Gitaly: TODO stream.each do |message| - puts '§' * 80 - puts version_index - message.versions.each do |version| - case version_index - when min_index...max_index - versions << new_wiki_page_version(version) - when max_index..Float::INFINITY - return versions - end - - version_index += 1 - puts version_index + versions << new_wiki_page_version(version) end end - # when we're requesting page 99 but the stream doesn't go that far, whatever - # is fetched thus far versions end -- cgit v1.2.3 From 28fd7b6c5df43b2f29557e813055deb438791c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:28:10 +0100 Subject: Add auto_devops_domain to ApplicationSettings model --- ...162010_add_auto_devops_domain_to_application_settings.rb | 13 +++++++++++++ db/schema.rb | 3 ++- 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb diff --git a/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb b/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb new file mode 100644 index 00000000000..7e16cb83087 --- /dev/null +++ b/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb @@ -0,0 +1,13 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddAutoDevopsDomainToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + add_column :application_settings, :auto_devops_domain, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index a0901833c3d..a683a60df45 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180113220114) do +ActiveRecord::Schema.define(version: 20180122162010) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -155,6 +155,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do t.integer "gitaly_timeout_medium", default: 30, null: false t.integer "gitaly_timeout_fast", default: 10, null: false t.boolean "authorized_keys_enabled", default: true, null: false + t.string "auto_devops_domain" end create_table "audit_events", force: :cascade do |t| -- cgit v1.2.3 From 147f0428c00e7a10e908438a99a1558c24be41f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:29:15 +0100 Subject: Add validation for auto_devops_domain --- app/models/application_setting.rb | 5 +++++ spec/models/application_setting_spec.rb | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 80bda7f22ff..0dee6df525d 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -117,6 +117,11 @@ class ApplicationSetting < ActiveRecord::Base validates :repository_storages, presence: true validate :check_repository_storages + validates :auto_devops_domain, + allow_blank: true, + hostname: { allow_numeric_hostname: true, require_valid_tld: true }, + if: :auto_devops_enabled? + validates :enabled_git_access_protocol, inclusion: { in: %w(ssh http), allow_blank: true, allow_nil: true } diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index ef480e7a80a..0d7b98229a4 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -114,6 +114,46 @@ describe ApplicationSetting do it { expect(setting.repository_storages).to eq(['default']) } end + context 'auto_devops_domain setting' do + context 'when auto_devops_enabled? is true' do + before do + setting.update(auto_devops_enabled: true) + end + + context 'with a valid value' do + before do + setting.update(auto_devops_domain: 'domain.com') + end + + it 'is valid' do + expect(setting).to be_valid + end + end + + context 'with an invalid value' do + before do + setting.update(auto_devops_domain: 'definitelynotahostname') + end + + it 'is invalid' do + expect(setting).to be_invalid + end + end + end + + context 'when auto_devops_enabled? is false' do + before do + setting.update(auto_devops_enabled: false) + end + + it 'can be blank' do + setting.update(auto_devops_domain: '') + + expect(setting).to be_valid + end + end + end + context 'circuitbreaker settings' do [:circuitbreaker_failure_count_threshold, :circuitbreaker_check_interval, -- cgit v1.2.3 From 57060295031f1ad2d5f058889561a68ede04d2f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:31:03 +0100 Subject: Expose auto_devops_domain in admin settings view --- app/helpers/application_settings_helper.rb | 1 + app/views/admin/application_settings/_form.html.haml | 7 ++++++- spec/features/admin/admin_settings_spec.rb | 10 ++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 8ef561d90e6..6f07428975d 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -148,6 +148,7 @@ module ApplicationSettingsHelper :akismet_enabled, :authorized_keys_enabled, :auto_devops_enabled, + :auto_devops_domain, :circuitbreaker_access_retries, :circuitbreaker_check_interval, :circuitbreaker_failure_count_threshold, diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index ba4ca88a8a9..d77d99180fe 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -249,7 +249,12 @@ .help-block It will automatically build, test, and deploy applications based on a predefined CI/CD configuration = link_to icon('question-circle'), help_page_path('topics/autodevops/index.md') - + .form-group + = f.label :auto_devops_domain, class: 'control-label col-sm-2' + .col-sm-10 + = f.text_field :auto_devops_domain, class: 'form-control', placeholder: 'domain.com' + .help-block + = s_("Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") .form-group .col-sm-offset-2.col-sm-10 .checkbox diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 1218ea52227..cfc6697c79a 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -47,6 +47,16 @@ feature 'Admin updates settings' do expect(page).to have_content "Application settings saved successfully" end + scenario 'Change AutoDevOps settings' do + check 'Enabled Auto DevOps (Beta) for projects by default' + fill_in 'Auto devops domain', with: 'domain.com' + click_button 'Save' + + expect(current_application_settings.auto_devops_enabled?).to be true + expect(current_application_settings.auto_devops_domain).to eq('domain.com') + expect(page).to have_content "Application settings saved successfully" + end + scenario 'Change Slack Notifications Service template settings' do first(:link, 'Service Templates').click click_link 'Slack notifications' -- cgit v1.2.3 From 8f0a14843a630fec2eec22920ce922d5548ddec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:31:35 +0100 Subject: Expose instance AutoDevOps domain in Project --- app/models/project.rb | 10 +++++++++- spec/models/project_spec.rb | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index c0f7b30ceb0..87616716c7c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1604,7 +1604,15 @@ class Project < ActiveRecord::Base def auto_devops_variables return [] unless auto_devops_enabled? - auto_devops&.variables || [] + variables = [] + + if current_application_settings.auto_devops_domain.present? + variables << { key: 'AUTO_DEVOPS_DOMAIN', + value: current_application_settings.auto_devops_domain, + public: true } + end + + auto_devops&.variables || variables end def append_or_update_attribute(name, value) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 987be8e8b46..dc0825aad73 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2976,18 +2976,40 @@ describe Project do subject { project.auto_devops_variables } - context 'when enabled in settings' do + context 'when enabled in instance settings' do before do stub_application_setting(auto_devops_enabled: true) end + context 'when domain is empty' do + before do + stub_application_setting(auto_devops_domain: nil) + end + + it 'variables does not include AUTO_DEVOPS_DOMAIN' do + is_expected.not_to include(domain_variable) + end + end + + context 'when domain is configured' do + before do + stub_application_setting(auto_devops_domain: 'example.com') + end + + it 'variables includes AUTO_DEVOPS_DOMAIN' do + is_expected.to include(domain_variable) + end + end + end + + context 'when explicitely enabled' do context 'when domain is empty' do before do create(:project_auto_devops, project: project, domain: nil) end - it 'variables are empty' do - is_expected.to be_empty + it 'variables does not include AUTO_DEVOPS_DOMAIN' do + is_expected.not_to include(domain_variable) end end @@ -2996,11 +3018,15 @@ describe Project do create(:project_auto_devops, project: project, domain: 'example.com') end - it "variables are not empty" do - is_expected.not_to be_empty + it 'variables includes AUTO_DEVOPS_DOMAIN' do + is_expected.to include(domain_variable) end end end + + def domain_variable + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true } + end end describe '#latest_successful_builds_for' do -- cgit v1.2.3 From f5591e4c901cea77f57e2b406e8e818d29919dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 20:25:22 +0100 Subject: Add CHANGELOG entry --- .../38175-add-domain-field-to-auto-devops-application-setting.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml diff --git a/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml b/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml new file mode 100644 index 00000000000..475e1dc12b5 --- /dev/null +++ b/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml @@ -0,0 +1,5 @@ +--- +title: Add Auto DevOps Domain application setting +merge_request: 16604 +author: +type: changed -- cgit v1.2.3 From 6028f9eecfad1190e99c830b3367b7ccb9fb5c81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 20:37:02 +0100 Subject: Refactor auto_devops_domain setting specs --- spec/models/application_setting_spec.rb | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 0d7b98229a4..ae2d34750a7 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -120,6 +120,12 @@ describe ApplicationSetting do setting.update(auto_devops_enabled: true) end + it 'can be blank' do + setting.update(auto_devops_domain: '') + + expect(setting).to be_valid + end + context 'with a valid value' do before do setting.update(auto_devops_domain: 'domain.com') @@ -140,18 +146,6 @@ describe ApplicationSetting do end end end - - context 'when auto_devops_enabled? is false' do - before do - setting.update(auto_devops_enabled: false) - end - - it 'can be blank' do - setting.update(auto_devops_domain: '') - - expect(setting).to be_valid - end - end end context 'circuitbreaker settings' do -- cgit v1.2.3 From 62b7c3ba4aa1a354ac5380400c707a381ef0e5e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 21:10:41 +0100 Subject: Short-circuit Project#auto_devops_variables This makes Project#auto_devops_variables more performant by evaluating the application settings only if necessary. --- app/models/project.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 87616716c7c..97f5e678021 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1604,15 +1604,13 @@ class Project < ActiveRecord::Base def auto_devops_variables return [] unless auto_devops_enabled? - variables = [] - - if current_application_settings.auto_devops_domain.present? - variables << { key: 'AUTO_DEVOPS_DOMAIN', - value: current_application_settings.auto_devops_domain, - public: true } - end - - auto_devops&.variables || variables + auto_devops&.variables || if current_application_settings.auto_devops_domain.present? + [{ key: 'AUTO_DEVOPS_DOMAIN', + value: current_application_settings.auto_devops_domain, + public: true }] + else + [] + end end def append_or_update_attribute(name, value) -- cgit v1.2.3 From 6714fbad8fc25d3cc6b295d26e3c220987c60fb0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 19 Jan 2018 14:25:30 +0100 Subject: Remove redundant pipeline stages from the database --- ...80119121225_remove_redundant_pipeline_stages.rb | 26 +++++++++++++++ db/schema.rb | 2 +- .../remove_redundant_pipeline_stages_spec.rb | 39 ++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb create mode 100644 spec/migrations/remove_redundant_pipeline_stages_spec.rb diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb new file mode 100644 index 00000000000..3868e0adae1 --- /dev/null +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -0,0 +1,26 @@ +class RemoveRedundantPipelineStages < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + redundant_stages_ids = <<~SQL + SELECT id FROM ci_stages a WHERE ( + SELECT COUNT(*) FROM ci_stages b + WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + ) > 1 + SQL + + execute <<~SQL + UPDATE ci_builds SET stage_id = NULL WHERE ci_builds.stage_id IN (#{redundant_stages_ids}) + SQL + + execute <<~SQL + DELETE FROM ci_stages WHERE ci_stages.id IN (#{redundant_stages_ids}) + SQL + end + + def down + # noop + end +end diff --git a/db/schema.rb b/db/schema.rb index a0901833c3d..9becd794553 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180113220114) do +ActiveRecord::Schema.define(version: 20180119121225) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/remove_redundant_pipeline_stages_spec.rb b/spec/migrations/remove_redundant_pipeline_stages_spec.rb new file mode 100644 index 00000000000..3bd2a9a2146 --- /dev/null +++ b/spec/migrations/remove_redundant_pipeline_stages_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180119121225_remove_redundant_pipeline_stages.rb') + +describe RemoveRedundantPipelineStages, :migration do + let(:projects) { table(:projects) } + let(:pipelines) { table(:ci_pipelines) } + let(:stages) { table(:ci_stages) } + let(:builds) { table(:ci_builds) } + + before do + projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') + pipelines.create!(id: 234, project_id: 123, ref: 'master', sha: 'adf43c3a') + + stages.create!(id: 6, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 10, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 21, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 41, project_id: 123, pipeline_id: 234, name: 'test') + stages.create!(id: 62, project_id: 123, pipeline_id: 234, name: 'test') + stages.create!(id: 102, project_id: 123, pipeline_id: 234, name: 'deploy') + + builds.create!(id: 1, commit_id: 234, project_id: 123, stage_id: 10) + builds.create!(id: 2, commit_id: 234, project_id: 123, stage_id: 21) + builds.create!(id: 3, commit_id: 234, project_id: 123, stage_id: 21) + builds.create!(id: 4, commit_id: 234, project_id: 123, stage_id: 41) + builds.create!(id: 5, commit_id: 234, project_id: 123, stage_id: 62) + builds.create!(id: 6, commit_id: 234, project_id: 123, stage_id: 102) + end + + it 'removes ambiguous stages and preserves builds' do + expect(stages.all.count).to eq 6 + expect(builds.all.count).to eq 6 + + migrate! + + expect(stages.all.count).to eq 1 + expect(builds.all.count).to eq 6 + expect(builds.all.pluck(:stage_id).compact).to eq [102] + end +end -- cgit v1.2.3 From e73333242ac42a1020319f5d491daf0647af4c54 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 12:56:11 +0100 Subject: Optimize SQL query that removes duplicated stages --- .../20180119121225_remove_redundant_pipeline_stages.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index 3868e0adae1..c1705cd8757 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -5,10 +5,10 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration def up redundant_stages_ids = <<~SQL - SELECT id FROM ci_stages a WHERE ( - SELECT COUNT(*) FROM ci_stages b - WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name - ) > 1 + SELECT id FROM ci_stages WHERE (pipeline_id, name) IN ( + SELECT pipeline_id, name FROM ci_stages + GROUP BY pipeline_id, name HAVING COUNT(*) > 1 + ) SQL execute <<~SQL -- cgit v1.2.3 From aa290573aece6408ad8bb98c7a04257202be5ff8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 14:26:03 +0100 Subject: Add a new service for creating detached CI/CD jobs --- app/models/commit_status.rb | 4 +++- app/services/ci/create_job_service.rb | 12 ++++++++++++ app/services/projects/update_pages_service.rb | 18 ++++++++++-------- lib/api/commit_statuses.rb | 10 +++++++++- spec/services/ci/create_job_service_spec.rb | 20 ++++++++++++++++++++ 5 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 app/services/ci/create_job_service.rb create mode 100644 spec/services/ci/create_job_service_spec.rb diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index c0263c0b4e2..f010b0ce9db 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -50,7 +50,9 @@ class CommitStatus < ActiveRecord::Base ## # We still create some CommitStatuses outside of CreatePipelineService. # - # These are pages deployments and external statuses. + # These are pages deployments and external statuses. We now handle these + # using CreateJobService, but we still need to ensure that a job has a + # stage assigned. TODO, In future releases we will add model validation. # before_create unless: :importing? do Ci::EnsureStageService.new(project, user).execute(self) do |stage| diff --git a/app/services/ci/create_job_service.rb b/app/services/ci/create_job_service.rb new file mode 100644 index 00000000000..683c3557cd0 --- /dev/null +++ b/app/services/ci/create_job_service.rb @@ -0,0 +1,12 @@ +module Ci + class CreateJobService < BaseService + def execute(subject = nil) + (subject || yield).tap do |subject| + Ci::EnsureStageService.new(project, current_user) + .execute(subject) + + subject.save! + end + end + end +end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index a773222bf17..63d2b6c76e6 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -58,14 +58,16 @@ module Projects end def create_status - GenericCommitStatus.new( - project: project, - pipeline: build.pipeline, - user: build.user, - ref: build.ref, - stage: 'deploy', - name: 'pages:deploy' - ) + Ci::CreateJobService.new(project, build.user).execute do + GenericCommitStatus.new( + project: project, + pipeline: build.pipeline, + user: build.user, + ref: build.ref, + stage: 'deploy', + name: 'pages:deploy' + ) + end end def extract_archive!(temp_path) diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 829eef18795..76cb9a8c808 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -69,6 +69,7 @@ module API name = params[:name] || params[:context] || 'default' pipeline = @project.pipeline_for(ref, commit.sha) + unless pipeline pipeline = @project.pipelines.create!( source: :external, @@ -90,7 +91,14 @@ module API optional_attributes = attributes_for_keys(%w[target_url description coverage]) - status.update(optional_attributes) if optional_attributes.any? + status.assign_attributes(optional_attributes) if optional_attributes.any? + + if status.new_record? + Ci::CreateJobService.new(@project, current_user).execute(status) + else + status.save! + end + render_validation_error!(status) if status.invalid? begin diff --git a/spec/services/ci/create_job_service_spec.rb b/spec/services/ci/create_job_service_spec.rb new file mode 100644 index 00000000000..a7a8032ba28 --- /dev/null +++ b/spec/services/ci/create_job_service_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Ci::CreateJobService, '#execute' do + set(:project) { create(:project, :repository) } + let(:user) { create(:admin) } + let(:status) { build(:ci_build) } + let(:service) { described_class.new(project, user) } + + it 'persists job object instantiated in the block' do + expect(service.execute { status }).to be_persisted + end + + it 'persists a job instance passed as an argument' do + expect(service.execute(status)).to be_persisted + end + + it 'ensures that a job has a stage assigned' do + expect(service.execute(status).stage_id).to be_present + end +end -- cgit v1.2.3 From e4aac7f365105cbc7547239066db075a44d69788 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 14:40:11 +0100 Subject: Do not raise an error when trying to persist a job --- app/models/commit_status.rb | 4 ++-- app/services/ci/create_job_service.rb | 2 +- lib/api/commit_statuses.rb | 2 +- spec/services/ci/create_job_service_spec.rb | 6 ++++++ 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index f010b0ce9db..47af20975ec 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -51,8 +51,8 @@ class CommitStatus < ActiveRecord::Base # We still create some CommitStatuses outside of CreatePipelineService. # # These are pages deployments and external statuses. We now handle these - # using CreateJobService, but we still need to ensure that a job has a - # stage assigned. TODO, In future releases we will add model validation. + # jobs using CreateJobService, but we still need to ensure that a job has + # a stage assigned. TODO, In future releases we will add model validation. # before_create unless: :importing? do Ci::EnsureStageService.new(project, user).execute(self) do |stage| diff --git a/app/services/ci/create_job_service.rb b/app/services/ci/create_job_service.rb index 683c3557cd0..7cb77edd690 100644 --- a/app/services/ci/create_job_service.rb +++ b/app/services/ci/create_job_service.rb @@ -5,7 +5,7 @@ module Ci Ci::EnsureStageService.new(project, current_user) .execute(subject) - subject.save! + subject.save end end end diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 76cb9a8c808..3ec17c4d9f5 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -96,7 +96,7 @@ module API if status.new_record? Ci::CreateJobService.new(@project, current_user).execute(status) else - status.save! + status.save if status.changed? end render_validation_error!(status) if status.invalid? diff --git a/spec/services/ci/create_job_service_spec.rb b/spec/services/ci/create_job_service_spec.rb index a7a8032ba28..d9dd1a40325 100644 --- a/spec/services/ci/create_job_service_spec.rb +++ b/spec/services/ci/create_job_service_spec.rb @@ -17,4 +17,10 @@ describe Ci::CreateJobService, '#execute' do it 'ensures that a job has a stage assigned' do expect(service.execute(status).stage_id).to be_present end + + it 'does not raise error if status is invalid' do + status.name = nil + + expect(service.execute(status)).not_to be_persisted + end end -- cgit v1.2.3 From ce71b1ce072b024802e7e4ff07486e253c24d964 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 15:21:20 +0100 Subject: Fix removing redundant pipeline stages on MySQL --- .../20180119121225_remove_redundant_pipeline_stages.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index c1705cd8757..597f6c3ee48 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -12,12 +12,19 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration SQL execute <<~SQL - UPDATE ci_builds SET stage_id = NULL WHERE ci_builds.stage_id IN (#{redundant_stages_ids}) + UPDATE ci_builds SET stage_id = NULL WHERE stage_id IN (#{redundant_stages_ids}) SQL - execute <<~SQL - DELETE FROM ci_stages WHERE ci_stages.id IN (#{redundant_stages_ids}) - SQL + if Gitlab::Database.postgresql? + execute <<~SQL + DELETE FROM ci_stages WHERE id IN (#{redundant_stages_ids}) + SQL + else # We can't modify a table we are selecting from on MySQL + execute <<~SQL + DELETE a FROM ci_stages AS a, ci_stages AS b + WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + SQL + end end def down -- cgit v1.2.3 From b97b4d258552cadc55f408a28569c4a0d34b38a3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 12:55:11 +0100 Subject: Add an unique index on pipeline stage name --- ...180125111139_add_unique_pipeline_stage_name_index.rb | 17 +++++++++++++++++ db/schema.rb | 5 +++-- 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb diff --git a/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb b/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb new file mode 100644 index 00000000000..4fa148b2446 --- /dev/null +++ b/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb @@ -0,0 +1,17 @@ +class AddUniquePipelineStageNameIndex < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + remove_concurrent_index :ci_stages, [:pipeline_id, :name] + add_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true + end + + def down + remove_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true + add_concurrent_index :ci_stages, [:pipeline_id, :name] + end +end diff --git a/db/schema.rb b/db/schema.rb index 9becd794553..d0802bed8f7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180119121225) do +ActiveRecord::Schema.define(version: 20180125111139) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -450,7 +450,8 @@ ActiveRecord::Schema.define(version: 20180119121225) do t.integer "lock_version" end - add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", using: :btree + add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree + add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name_unique", unique: true, using: :btree add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree -- cgit v1.2.3 From 50e9824115bbefeb59319b4f20622b162c22063f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 13:28:18 +0100 Subject: Fix migration removing duplicate stages on MySQL again --- db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index 597f6c3ee48..ce38026e7e2 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -23,6 +23,7 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration execute <<~SQL DELETE a FROM ci_stages AS a, ci_stages AS b WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + AND a.id <> b.id SQL end end -- cgit v1.2.3 From 9b73f14d07233af78512a93f9b8eadd696cd6fd1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 14:18:41 +0100 Subject: Fix indentation in migration removing duplicated stages --- db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index ce38026e7e2..89ed422ea1c 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -4,7 +4,7 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration DOWNTIME = false def up - redundant_stages_ids = <<~SQL + redundant_stages_ids = <<~SQL SELECT id FROM ci_stages WHERE (pipeline_id, name) IN ( SELECT pipeline_id, name FROM ci_stages GROUP BY pipeline_id, name HAVING COUNT(*) > 1 -- cgit v1.2.3 From 4e4ee368c9c02fcd52a1173df4e8ca882c644dba Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 14:31:27 +0100 Subject: Remove unique index not added in a migration from schema --- db/schema.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index d0802bed8f7..97cbbd111c8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -451,7 +451,6 @@ ActiveRecord::Schema.define(version: 20180125111139) do end add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree - add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name_unique", unique: true, using: :btree add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree -- cgit v1.2.3 From 48b0bc330add0fbb3398890e20a7444ba69d5f9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 15:15:51 +0100 Subject: Fix specs for retry build after making stages unique --- app/services/ci/retry_build_service.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 36 +++++++++++++++------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index c552193e66b..6128b2a8fbb 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -1,7 +1,7 @@ module Ci class RetryBuildService < ::BaseService CLONE_ACCESSORS = %i[pipeline project ref tag options commands name - allow_failure stage_id stage stage_idx trigger_request + allow_failure stage stage_id stage_idx trigger_request yaml_variables when environment coverage_regex description tag_list protected].freeze diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index a06397a0782..cea5c8a6c05 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -5,7 +5,11 @@ describe Ci::RetryBuildService do set(:project) { create(:project) } set(:pipeline) { create(:ci_pipeline, project: project) } - let(:build) { create(:ci_build, pipeline: pipeline) } + let(:stage) do + Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test') + end + + let(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } let(:service) do described_class.new(project, user) @@ -26,29 +30,27 @@ describe Ci::RetryBuildService do user_id auto_canceled_by_id retried failure_reason].freeze shared_examples 'build duplication' do - let(:stage) do - # TODO, we still do not have factory for new stages, we will need to - # switch existing factory to persist stages, instead of using LegacyStage - # - Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test') - end + let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } let(:build) do create(:ci_build, :failed, :artifacts, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :trace, :teardown_environment, - description: 'my-job', stage: 'test', pipeline: pipeline, - auto_canceled_by: create(:ci_empty_pipeline, project: project)) do |build| - ## - # TODO, workaround for FactoryBot limitation when having both - # stage (text) and stage_id (integer) columns in the table. - build.stage_id = stage.id - end + description: 'my-job', stage: 'test', stage_id: stage.id, + pipeline: pipeline, auto_canceled_by: another_pipeline) + end + + before do + # Make sure that build has both `stage_id` and `stage` because FactoryBot + # can reset one of the fields when assigning another. We plan to deprecate + # and remove legacy `stage` column in the future. + build.update_attributes(stage: 'test', stage_id: stage.id) end describe 'clone accessors' do CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do + expect(build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).to eq build.send(attribute) end @@ -121,10 +123,12 @@ describe Ci::RetryBuildService do context 'when there are subsequent builds that are skipped' do let!(:subsequent_build) do - create(:ci_build, :skipped, stage_idx: 1, pipeline: pipeline) + create(:ci_build, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') end - it 'resumes pipeline processing in subsequent stages' do + it 'resumes pipeline processing in a subsequent stage' do service.execute(build) expect(subsequent_build.reload).to be_created -- cgit v1.2.3 From 25e1345836d37a3162445720caf50808c2b3c025 Mon Sep 17 00:00:00 2001 From: Joshua Lambert Date: Fri, 26 Jan 2018 23:58:44 -0500 Subject: Fix values.yaml for Prometheus --- vendor/prometheus/values.yaml | 140 +++++++++++++++++++----------------------- 1 file changed, 64 insertions(+), 76 deletions(-) diff --git a/vendor/prometheus/values.yaml b/vendor/prometheus/values.yaml index 5249449c7f8..4c709aeac76 100644 --- a/vendor/prometheus/values.yaml +++ b/vendor/prometheus/values.yaml @@ -11,10 +11,10 @@ pushgateway: enabled: false serverFiles: - alerts: "" - rules: "" + alerts: {} + rules: {} - prometheus.yml: |- + prometheus.yml: rule_files: - /etc/config/rules - /etc/config/alerts @@ -26,92 +26,80 @@ serverFiles: - job_name: kubernetes-cadvisor scheme: https tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: node - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: node relabel_configs: - - action: labelmap - regex: __meta_kubernetes_node_label_(.+) - - target_label: __address__ - replacement: kubernetes.default.svc:443 - - source_labels: - - __meta_kubernetes_node_name - regex: "(.+)" - target_label: __metrics_path__ - replacement: "/api/v1/nodes/${1}/proxy/metrics/cadvisor" + - action: labelmap + regex: __meta_kubernetes_node_label_(.+) + - target_label: __address__ + replacement: kubernetes.default.svc:443 + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" + target_label: __metrics_path__ + replacement: "/api/v1/nodes/${1}/proxy/metrics/cadvisor" metric_relabel_configs: - - source_labels: - - pod_name - target_label: environment - regex: "(.+)-.+-.+" + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" - job_name: kubernetes-nodes scheme: https tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: node - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: node relabel_configs: - - action: labelmap - regex: __meta_kubernetes_node_label_(.+) - - target_label: __address__ - replacement: kubernetes.default.svc:443 - - source_labels: - - __meta_kubernetes_node_name - regex: "(.+)" - target_label: __metrics_path__ - replacement: "/api/v1/nodes/${1}/proxy/metrics" + - action: labelmap + regex: __meta_kubernetes_node_label_(.+) + - target_label: __address__ + replacement: kubernetes.default.svc:443 + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" + target_label: __metrics_path__ + replacement: "/api/v1/nodes/${1}/proxy/metrics" metric_relabel_configs: - - source_labels: - - pod_name - target_label: environment - regex: "(.+)-.+-.+" + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" - job_name: kubernetes-pods tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: pod - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: pod relabel_configs: - - source_labels: - - __meta_kubernetes_pod_annotation_prometheus_io_scrape - action: keep - regex: 'true' - - source_labels: - - __meta_kubernetes_pod_annotation_prometheus_io_path - action: replace - target_label: __metrics_path__ - regex: "(.+)" - - source_labels: - - __address__ - - __meta_kubernetes_pod_annotation_prometheus_io_port - action: replace - regex: "([^:]+)(?::[0-9]+)?;([0-9]+)" - replacement: "$1:$2" - target_label: __address__ - - action: labelmap - regex: __meta_kubernetes_pod_label_(.+) - - source_labels: - - __meta_kubernetes_namespace - action: replace - target_label: kubernetes_namespace - - source_labels: - - __meta_kubernetes_pod_name - action: replace - target_label: kubernetes_pod_name + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_scrape + action: keep + regex: 'true' + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_path + action: replace + target_label: __metrics_path__ + regex: "(.+)" + - source_labels: + - __address__ + - __meta_kubernetes_pod_annotation_prometheus_io_port + action: replace + regex: "([^:]+)(?::[0-9]+)?;([0-9]+)" + replacement: "$1:$2" + target_label: __address__ + - action: labelmap + regex: __meta_kubernetes_pod_label_(.+) + - source_labels: + - __meta_kubernetes_namespace + action: replace + target_label: kubernetes_namespace + - source_labels: + - __meta_kubernetes_pod_name + action: replace + target_label: kubernetes_pod_name -- cgit v1.2.3 From b1c40590e7738a782d1295d743a6a3957723c4b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 00:23:36 +0100 Subject: Extend Runner API helpers with cache info storage --- lib/api/helpers/runner.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 2cae53dba53..ad8c1c4407c 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -27,6 +27,8 @@ module API end def update_runner_info + update_runner_info_cache + return unless update_runner? current_runner.contacted_at = Time.now @@ -35,13 +37,17 @@ module API end def update_runner? - # Use a random threshold to prevent beating DB updates. - # It generates a distribution between [40m, 80m]. - # - contacted_at_max_age = UPDATE_RUNNER_EVERY + Random.rand(UPDATE_RUNNER_EVERY) + # Use a 1h threshold to prevent beating DB updates. current_runner.contacted_at.nil? || - (Time.now - current_runner.contacted_at) >= contacted_at_max_age + (Time.now - current_runner.contacted_at) >= UPDATE_RUNNER_EVERY + end + + def update_runner_info_cache + Gitlab::Redis::SharedState.with do |redis| + redis_key = "#{current_runner.runner_info_key}:contacted_at" + redis.set(redis_key, Time.now) + end end def validate_job!(job) -- cgit v1.2.3 From 0be8b3cdf677f8b20be820ad3f6fdd4b9b08fc56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 00:25:13 +0100 Subject: Check cache in Ci::Runner#online? --- app/models/ci/runner.rb | 9 +++++++- spec/models/ci/runner_spec.rb | 54 +++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index dcbb397fb78..7e616ee9144 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -88,7 +88,10 @@ module Ci end def online? - contacted_at && contacted_at > self.class.contact_time_deadline + Gitlab::Redis::SharedState.with do |redis| + last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at + last_seen && last_seen > self.class.contact_time_deadline + end end def status @@ -152,6 +155,10 @@ module Ci ensure_runner_queue_value == value if value.present? end + def runner_info_key + "runner:info:#{self.id}" + end + private def cleanup_runner_queue diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index b2b64e6ff48..fe9e5ec9436 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -95,28 +95,58 @@ describe Ci::Runner do subject { runner.online? } - context 'never contacted' do + context 'no cache value' do before do - runner.contacted_at = nil + stub_redis("#{runner.runner_info_key}:contacted_at", nil) end - it { is_expected.to be_falsey } - end + context 'never contacted' do + before do + runner.contacted_at = nil + end - context 'contacted long time ago time' do - before do - runner.contacted_at = 1.year.ago + it { is_expected.to be_falsey } + end + + context 'contacted long time ago time' do + before do + runner.contacted_at = 1.year.ago + end + + it { is_expected.to be_falsey } end - it { is_expected.to be_falsey } + context 'contacted 1s ago' do + before do + runner.contacted_at = 1.second.ago + end + + it { is_expected.to be_truthy } + end end - context 'contacted 1s ago' do - before do - runner.contacted_at = 1.second.ago + context 'with cache value' do + context 'contacted long time ago time' do + before do + stub_redis("#{runner.runner_info_key}:contacted_at", 1.year.ago.to_s) + end + + it { is_expected.to be_falsey } + end + + context 'contacted 1s ago' do + before do + stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s) + end + + it { is_expected.to be_truthy } end + end - it { is_expected.to be_truthy } + def stub_redis(key, value) + redis = double + allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) + allow(redis).to receive(:get).with(key).and_return(value) end end -- cgit v1.2.3 From 397442a06140a8cf38bebe3f4519311b1448f23d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 17:21:30 +0100 Subject: Update runner info on all authenticated requests --- lib/api/helpers/runner.rb | 2 ++ lib/api/runner.rb | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index ad8c1c4407c..5ac9181f878 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -20,6 +20,8 @@ module API def authenticate_runner! forbidden! unless current_runner + + update_runner_info end def current_runner diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 80feb629d54..50cb1df92ad 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -78,7 +78,6 @@ module API post '/request' do authenticate_runner! no_content! unless current_runner.active? - update_runner_info if current_runner.runner_queue_value_latest?(params[:last_update]) header 'X-GitLab-Last-Update', params[:last_update] -- cgit v1.2.3 From 92ac2b9ee68cad8c01a199e6475bbef272818da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 17:24:29 +0100 Subject: Add CHANGELOG entry --- ...ancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml diff --git a/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml new file mode 100644 index 00000000000..4d8e6acfcb7 --- /dev/null +++ b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml @@ -0,0 +1,5 @@ +--- +title: Update runner info on all authenticated requests +merge_request: 16756 +author: +type: changed -- cgit v1.2.3 From bdd3e39b0bd4e8fcec5d6e2d97297840211dd921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 20:56:28 +0100 Subject: Move info update implementation to Ci::Runner model --- app/models/ci/runner.rb | 26 +++++++++++++++++++--- lib/api/helpers/runner.rb | 27 +--------------------- spec/models/ci/runner_spec.rb | 52 ++++++++++++++++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 34 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7e616ee9144..44be247bcd3 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -5,6 +5,7 @@ module Ci RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour + UPDATE_DB_RUNNER_INFO_EVERY = 1.hour AVAILABLE_SCOPES = %w[specific shared active paused online].freeze FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze @@ -89,7 +90,7 @@ module Ci def online? Gitlab::Redis::SharedState.with do |redis| - last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at + last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at last_seen && last_seen > self.class.contact_time_deadline end end @@ -155,8 +156,16 @@ module Ci ensure_runner_queue_value == value if value.present? end - def runner_info_key - "runner:info:#{self.id}" + def update_runner_info(params) + update_runner_info_cache + + # Use a 1h threshold to prevent beating DB updates. + return unless self.contacted_at.nil? || + (Time.now - self.contacted_at) >= UPDATE_DB_RUNNER_INFO_EVERY + + self.contacted_at = Time.now + self.assign_attributes(params) + self.save if self.changed? end private @@ -171,6 +180,17 @@ module Ci "runner:build_queue:#{self.token}" end + def runner_info_redis_cache_key + "runner:info:#{self.id}" + end + + def update_runner_info_cache + Gitlab::Redis::SharedState.with do |redis| + redis_key = "#{runner_info_redis_cache_key}:contacted_at" + redis.set(redis_key, Time.now) + end + end + def tag_constraints unless has_tags? || run_untagged? errors.add(:tags_list, diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 5ac9181f878..8f45cae0e60 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -5,7 +5,6 @@ module API JOB_TOKEN_HEADER = 'HTTP_JOB_TOKEN'.freeze JOB_TOKEN_PARAM = :token - UPDATE_RUNNER_EVERY = 10 * 60 def runner_registration_token_valid? ActiveSupport::SecurityUtils.variable_size_secure_compare(params[:token], @@ -21,37 +20,13 @@ module API def authenticate_runner! forbidden! unless current_runner - update_runner_info + current_runner.update_runner_info(get_runner_version_from_params) end def current_runner @runner ||= ::Ci::Runner.find_by_token(params[:token].to_s) end - def update_runner_info - update_runner_info_cache - - return unless update_runner? - - current_runner.contacted_at = Time.now - current_runner.assign_attributes(get_runner_version_from_params) - current_runner.save if current_runner.changed? - end - - def update_runner? - # Use a 1h threshold to prevent beating DB updates. - - current_runner.contacted_at.nil? || - (Time.now - current_runner.contacted_at) >= UPDATE_RUNNER_EVERY - end - - def update_runner_info_cache - Gitlab::Redis::SharedState.with do |redis| - redis_key = "#{current_runner.runner_info_key}:contacted_at" - redis.set(redis_key, Time.now) - end - end - def validate_job!(job) not_found! unless job diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index fe9e5ec9436..830f7cd68c5 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -97,7 +97,7 @@ describe Ci::Runner do context 'no cache value' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", nil) + stub_redis_runner_contacted_at(nil) end context 'never contacted' do @@ -128,7 +128,7 @@ describe Ci::Runner do context 'with cache value' do context 'contacted long time ago time' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", 1.year.ago.to_s) + stub_redis_runner_contacted_at(1.year.ago.to_s) end it { is_expected.to be_falsey } @@ -136,17 +136,17 @@ describe Ci::Runner do context 'contacted 1s ago' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s) + stub_redis_runner_contacted_at(1.second.ago.to_s) end it { is_expected.to be_truthy } end end - def stub_redis(key, value) + def stub_redis_runner_contacted_at(value) redis = double allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - allow(redis).to receive(:get).with(key).and_return(value) + allow(redis).to receive(:get).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at").and_return(value) end end @@ -391,6 +391,48 @@ describe Ci::Runner do end end + describe '#update_runner_info' do + let(:runner) { create(:ci_runner) } + + subject { runner.update_runner_info(contacted_at: Time.now) } + + context 'when database was updated recently' do + before do + runner.update(contacted_at: Time.now) + end + + it 'updates cache' do + expect_redis_update + + subject + end + end + + context 'when database was not updated recently' do + before do + runner.update(contacted_at: 2.hours.ago) + end + + it 'updates database' do + expect_redis_update + + expect { subject }.to change { runner.reload.contacted_at } + end + + it 'updates cache' do + expect_redis_update + + subject + end + end + + def expect_redis_update + redis = double + expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) + expect(redis).to receive(:set).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at", anything) + end + end + describe '#destroy' do let(:runner) { create(:ci_runner) } -- cgit v1.2.3 From df2554558123fbfec007418601e2074cf251db46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 21:53:37 +0100 Subject: Generelized cached attribute usage in runner --- app/models/ci/runner.rb | 27 +++++++++++++++++++-------- spec/models/ci/runner_spec.rb | 17 +++++++++++------ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 44be247bcd3..8fe20622723 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -68,6 +68,10 @@ module Ci ONLINE_CONTACT_TIMEOUT.ago end + def cached_contacted_at + runner_info_cache(:contacted_at) || self.contacted_at + end + def set_default_values self.token = SecureRandom.hex(15) if self.token.blank? end @@ -89,10 +93,7 @@ module Ci end def online? - Gitlab::Redis::SharedState.with do |redis| - last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at - last_seen && last_seen > self.class.contact_time_deadline - end + cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline end def status @@ -157,7 +158,7 @@ module Ci end def update_runner_info(params) - update_runner_info_cache + update_runner_info_cache(params) # Use a 1h threshold to prevent beating DB updates. return unless self.contacted_at.nil? || @@ -184,10 +185,20 @@ module Ci "runner:info:#{self.id}" end - def update_runner_info_cache + def update_runner_info_cache(params) + Gitlab::Redis::SharedState.with do |redis| + redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now) + + params.each do |key, value| + redis_key = "#{runner_info_redis_cache_key}:#{key}" + redis.set(redis_key, value) + end + end + end + + def runner_info_cache(attribute) Gitlab::Redis::SharedState.with do |redis| - redis_key = "#{runner_info_redis_cache_key}:contacted_at" - redis.set(redis_key, Time.now) + redis.get("#{runner_info_redis_cache_key}:#{attribute}") end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 830f7cd68c5..14747a23c82 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -394,7 +394,7 @@ describe Ci::Runner do describe '#update_runner_info' do let(:runner) { create(:ci_runner) } - subject { runner.update_runner_info(contacted_at: Time.now) } + subject { runner.update_runner_info(name: 'testing_runner') } context 'when database was updated recently' do before do @@ -402,7 +402,7 @@ describe Ci::Runner do end it 'updates cache' do - expect_redis_update + expect_redis_update(:contacted_at, :name) subject end @@ -414,22 +414,27 @@ describe Ci::Runner do end it 'updates database' do - expect_redis_update + expect_redis_update(:contacted_at, :name) expect { subject }.to change { runner.reload.contacted_at } + .and change { runner.reload.name } end it 'updates cache' do - expect_redis_update + expect_redis_update(:contacted_at, :name) subject end end - def expect_redis_update + def expect_redis_update(*params) redis = double expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - expect(redis).to receive(:set).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at", anything) + + params.each do |param| + redis_key = "#{runner.send(:runner_info_redis_cache_key)}:#{param}" + expect(redis).to receive(:set).with(redis_key, anything) + end end end -- cgit v1.2.3 From b88103e4075678d032d7d7350caaece4a3091328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 22:38:09 +0100 Subject: Expose Ci::Runner#cached_contacted_at as Time --- app/models/ci/runner.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8fe20622723..7cf36c0bfe0 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -69,7 +69,11 @@ module Ci end def cached_contacted_at - runner_info_cache(:contacted_at) || self.contacted_at + if runner_info_cache(:contacted_at) + Time.zone.parse(runner_info_cache(:contacted_at)) + else + self.contacted_at + end end def set_default_values -- cgit v1.2.3 From bb8490b018bda7327a82e8ed38d3f1d60c9b4c39 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 19 Jan 2018 14:40:18 -0700 Subject: Start redesign of group labels page --- app/assets/stylesheets/pages/labels.scss | 22 +++++++++++++++------ app/views/shared/_label.html.haml | 34 ++++++++++++-------------------- app/views/shared/_label_row.html.haml | 12 +++++++++++ 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index e8cd8a4905c..91aa6648d41 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -64,7 +64,6 @@ .label { overflow: hidden; text-overflow: ellipsis; - vertical-align: middle; max-width: 100%; } } @@ -86,14 +85,17 @@ .label-description { display: block; margin-bottom: 10px; - margin-left: 50px; + + a { + color: $blue-600; + } @media (min-width: $screen-sm-min) { display: inline-block; - width: 30%; + max-width: 50%; margin-left: 10px; margin-bottom: 0; - vertical-align: middle; + vertical-align: top; } } @@ -116,6 +118,10 @@ } .manage-labels-list { + &.content-list li { + padding: $gl-padding 0; + } + > li:not(.empty-message):not(.is-not-draggable) { background-color: $white-light; cursor: move; @@ -133,8 +139,6 @@ } .btn-action { - color: $gl-text-color; - .fa { font-size: 18px; vertical-align: middle; @@ -155,6 +159,12 @@ float: right; } } + + @media (max-width: $screen-sm-max) { + .dropdown-menu { + min-width: 100%; + } + } } .draggable-handler { diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index 8e88cecaf9e..6a103f56706 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -8,7 +8,7 @@ %li{ id: label_css_id, data: { id: label.id } } = render "shared/label_row", label: label - .visible-xs.visible-sm-inline-block.visible-md-inline-block.dropdown + .visible-xs.visible-sm-inline-block.dropdown %button.btn.btn-default.label-options-toggle{ type: 'button', data: { toggle: "dropdown" } } Options = icon('caret-down') @@ -46,14 +46,18 @@ data: {confirm: 'Remove this label? Are you sure?'}, class: 'text-danger' - .pull-right.hidden-xs.hidden-sm.hidden-md - - if show_label_merge_requests_link - = link_to_label(label, subject: subject, type: :merge_request, css_class: 'btn btn-transparent btn-action btn-link') do - view merge requests - - if show_label_issues_link - = link_to_label(label, subject: subject, css_class: 'btn btn-transparent btn-action btn-link') do - view open issues - + .pull-right.hidden-xs.hidden-sm + - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_label, label.project.group) + = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "You are about to promote #{label.title} to a group level. This will make this milestone available to all projects inside #{label.project.group.name}. The existing project label will be merged into the group level. This action cannot be reversed.", toggle: "tooltip"}, method: :post do + %span.sr-only Promote to Group + = icon('level-up') + - if can?(current_user, :admin_label, label) + = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do + %span.sr-only Edit + = sprite_icon('pencil') + = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do + %span.sr-only Delete + = sprite_icon('remove') - if current_user .label-subscription.inline - if can_subscribe_to_label_in_different_levels?(label) @@ -75,15 +79,3 @@ %button.js-subscribe-button.label-subscribe-button.btn.btn-default{ type: 'button', data: { status: status, url: toggle_subscription_path } } %span= label_subscription_toggle_button_text(label, @project) = icon('spinner spin', class: 'label-subscribe-button-loading') - - - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_label, label.project.group) - = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "You are about to promote #{label.title} to a group level. This will make this milestone available to all projects inside #{label.project.group.name}. The existing project label will be merged into the group level. This action cannot be reversed.", toggle: "tooltip"}, method: :post do - %span.sr-only Promote to Group - = icon('level-up') - - if can?(current_user, :admin_label, label) - = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do - %span.sr-only Edit - = icon('pencil-square-o') - = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do - %span.sr-only Delete - = icon('trash-o') diff --git a/app/views/shared/_label_row.html.haml b/app/views/shared/_label_row.html.haml index 7f58298c60f..34da13ca52b 100644 --- a/app/views/shared/_label_row.html.haml +++ b/app/views/shared/_label_row.html.haml @@ -1,3 +1,7 @@ +- subject = local_assigns[:subject] +- show_label_issues_link = show_label_issuables_link?(label, :issues, project: @project) +- show_label_merge_requests_link = show_label_issuables_link?(label, :merge_requests, project: @project) + %span.label-row - if can?(current_user, :admin_label, @project) .draggable-handler @@ -16,3 +20,11 @@ - if label.description %span.label-description = markdown_field(label, :description) + .hidden-xs.hidden-sm + - if show_label_issues_link + = link_to_label(label, subject: subject) do + Issues + - if show_label_merge_requests_link + · + = link_to_label(label, subject: subject, type: :merge_request) do + Merge requests -- cgit v1.2.3 From af6aaf39f996e888ecfc536a42f25610623f6e02 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 22 Jan 2018 14:20:34 -0700 Subject: Change delete notice; update mobile styles --- app/assets/stylesheets/pages/labels.scss | 24 +++++++++++++++++++----- app/controllers/groups/labels_controller.rb | 2 +- app/views/shared/_label_row.html.haml | 22 +++++++++++----------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 91aa6648d41..9c4f51fc0d9 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -58,6 +58,7 @@ @media (min-width: $screen-sm-min) { width: 200px; + margin-left: $gl-padding * 2; margin-bottom: 0; } @@ -78,7 +79,7 @@ width: 100px; margin-left: 10px; margin-bottom: 0; - vertical-align: middle; + vertical-align: top; } } @@ -86,6 +87,10 @@ display: block; margin-bottom: 10px; + .description-text { + margin-bottom: $gl-padding; + } + a { color: $blue-600; } @@ -118,8 +123,10 @@ } .manage-labels-list { - &.content-list li { - padding: $gl-padding 0; + @media(min-width: $screen-md-min) { + &.content-list li { + padding: $gl-padding 0; + } } > li:not(.empty-message):not(.is-not-draggable) { @@ -160,7 +167,7 @@ } } - @media (max-width: $screen-sm-max) { + @media (max-width: $screen-xs-max) { .dropdown-menu { min-width: 100%; } @@ -169,6 +176,8 @@ .draggable-handler { display: inline-block; + vertical-align: top; + margin: 5px 0; opacity: 0; transition: opacity .3s; color: $gray-darkest; @@ -198,7 +207,7 @@ .toggle-priority { display: inline-block; - vertical-align: middle; + vertical-align: top; button { border-color: transparent; @@ -265,6 +274,11 @@ } .label-subscribe-button { + @media(min-width: $screen-md-min) { + min-width: 105px; + margin-left: $gl-padding; + } + .label-subscribe-button-icon { &[disabled] { opacity: 0.5; diff --git a/app/controllers/groups/labels_controller.rb b/app/controllers/groups/labels_controller.rb index dda59262483..f3a9e591c3e 100644 --- a/app/controllers/groups/labels_controller.rb +++ b/app/controllers/groups/labels_controller.rb @@ -54,7 +54,7 @@ class Groups::LabelsController < Groups::ApplicationController respond_to do |format| format.html do - redirect_to group_labels_path(@group), status: 302, notice: 'Label was removed' + redirect_to group_labels_path(@group), status: 302, notice: "#{@label.name} deleted permanently" end format.js end diff --git a/app/views/shared/_label_row.html.haml b/app/views/shared/_label_row.html.haml index 34da13ca52b..bd4f191502e 100644 --- a/app/views/shared/_label_row.html.haml +++ b/app/views/shared/_label_row.html.haml @@ -17,14 +17,14 @@ - if defined?(@project) && @project.group.present? %span.label-type = label.model_name.human.titleize - - if label.description - %span.label-description - = markdown_field(label, :description) - .hidden-xs.hidden-sm - - if show_label_issues_link - = link_to_label(label, subject: subject) do - Issues - - if show_label_merge_requests_link - · - = link_to_label(label, subject: subject, type: :merge_request) do - Merge requests + + %span.label-description + - if label.description.present? + .description-text + = markdown_field(label, :description) + .hidden-xs.hidden-sm + - if show_label_issues_link + = link_to_label(label, subject: subject) { 'Issues' } + - if show_label_merge_requests_link + · + = link_to_label(label, subject: subject, type: :merge_request) { 'Merge requests' } -- cgit v1.2.3 From 0ef1a582b625f7f6f52e377fc62dc526da34cfb6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 26 Jan 2018 15:42:42 -0700 Subject: Add modal for label deletion --- app/assets/stylesheets/framework/modal.scss | 4 ++++ app/views/shared/_delete_label_modal.html.haml | 20 ++++++++++++++++++++ app/views/shared/_label.html.haml | 9 ++++++--- 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 app/views/shared/_delete_label_modal.html.haml diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 32b9894ae04..5438a4fa207 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -4,6 +4,10 @@ .page-title { margin-top: 0; + + .label { + font-size: initial; + } } } diff --git a/app/views/shared/_delete_label_modal.html.haml b/app/views/shared/_delete_label_modal.html.haml new file mode 100644 index 00000000000..f29c8f00d52 --- /dev/null +++ b/app/views/shared/_delete_label_modal.html.haml @@ -0,0 +1,20 @@ +.modal{ id: "modal-delete-label-#{label.id}", tabindex: -1 } + .modal-dialog + .modal-content + .modal-header + %button.close{ data: {dismiss: 'modal' } } × + %h3.page-title Delete #{render_colored_label(label, tooltip: false)} ? + + .modal-body + %p + %strong #{label.name} + will be permanently deleted from #{label.group.name}. This cannot be undone. + + .modal-footer + %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel + + = link_to 'Delete label', + destroy_label_path(label), + title: 'Delete', + method: :delete, + class: 'btn btn-remove' diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index 6a103f56706..dd9b4b4db44 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -55,9 +55,10 @@ = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do %span.sr-only Edit = sprite_icon('pencil') - = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do - %span.sr-only Delete - = sprite_icon('remove') + %span{ data: { toggle: 'modal', target: "#modal-delete-label-#{label.id}" } } + = link_to "#", title: "Delete", class: 'btn btn-transparent btn-action remove-row', data: { toggle: "tooltip" } do + %span.sr-only Delete + = sprite_icon('remove') - if current_user .label-subscription.inline - if can_subscribe_to_label_in_different_levels?(label) @@ -79,3 +80,5 @@ %button.js-subscribe-button.label-subscribe-button.btn.btn-default{ type: 'button', data: { status: status, url: toggle_subscription_path } } %span= label_subscription_toggle_button_text(label, @project) = icon('spinner spin', class: 'label-subscribe-button-loading') + += render 'shared/delete_label_modal', label: label -- cgit v1.2.3 From 3be6f68a33d163922a78c51928980efc57a3efb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:00:39 +0100 Subject: Make Ci::Runner#online? slightly more performant This is a small refactor to avoid querying Redis when we know there's nothing in it. --- app/models/ci/runner.rb | 2 +- spec/models/ci/runner_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7cf36c0bfe0..f6d51fabd69 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -97,7 +97,7 @@ module Ci end def online? - cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline + contacted_at && cached_contacted_at > self.class.contact_time_deadline end def status diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 14747a23c82..99b4a82da88 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -128,6 +128,7 @@ describe Ci::Runner do context 'with cache value' do context 'contacted long time ago time' do before do + runner.contacted_at = 1.year.ago stub_redis_runner_contacted_at(1.year.ago.to_s) end @@ -136,6 +137,7 @@ describe Ci::Runner do context 'contacted 1s ago' do before do + runner.contacted_at = 50.minutes.ago stub_redis_runner_contacted_at(1.second.ago.to_s) end -- cgit v1.2.3 From 63ecb57f1b956bb7901e27f26f3bc2f3f62bcaa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:25:56 +0100 Subject: Handle updating only contacted_at runner cache --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index f6d51fabd69..fb766cb3793 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -193,7 +193,7 @@ module Ci Gitlab::Redis::SharedState.with do |redis| redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now) - params.each do |key, value| + params && params.each do |key, value| redis_key = "#{runner_info_redis_cache_key}:#{key}" redis.set(redis_key, value) end -- cgit v1.2.3 From 126b6bbc7fdeb1afd5b6d29c86051c48a09c4857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:27:03 +0100 Subject: Use faster model updates in #update_runner_info spec --- spec/models/ci/runner_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 99b4a82da88..ab931e5d43c 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -400,7 +400,7 @@ describe Ci::Runner do context 'when database was updated recently' do before do - runner.update(contacted_at: Time.now) + runner.contacted_at = Time.now end it 'updates cache' do @@ -412,7 +412,7 @@ describe Ci::Runner do context 'when database was not updated recently' do before do - runner.update(contacted_at: 2.hours.ago) + runner.contacted_at = 2.hours.ago end it 'updates database' do -- cgit v1.2.3 From 28fd49c1d23f18692b74e4f4e1f21c24c45da3e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:31:34 +0100 Subject: Fix Redis leakage in Runner API specs --- spec/requests/api/runner_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index cb66d23b77c..25d1ac73b9e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -8,6 +8,7 @@ describe API::Runner do before do stub_gitlab_calls stub_application_setting(runners_registration_token: registration_token) + allow_any_instance_of(Ci::Runner).to receive(:update_runner_info_cache) end describe '/api/v4/runners' do -- cgit v1.2.3 From 983d03313517a948ceb41cd94ffedf4c8d9d3957 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:28:20 +0100 Subject: fix service generic tests --- app/models/project_services/prometheus_service.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index e540a2c646a..676cc32b2d9 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -1,7 +1,3 @@ -module Gitlab - PrometheusError = Class.new(StandardError) -end - class PrometheusService < MonitoringService include ReactiveService @@ -47,6 +43,7 @@ class PrometheusService < MonitoringService [ { type: 'fieldset', + name: 'manual_configuration', legend: 'Manual Configuration', fields: [ { -- cgit v1.2.3 From 89eb6222738f215e81df1c54f82e44050c2b0fe4 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:32:47 +0100 Subject: enable manual configuration property for all test prometheus services --- spec/factories/projects.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index d0f3911f730..1dec4eb6c04 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -243,7 +243,8 @@ FactoryBot.define do project.create_prometheus_service( active: true, properties: { - api_url: 'https://prometheus.example.com' + api_url: 'https://prometheus.example.com', + manual_configuration: true } ) end -- cgit v1.2.3 From 529edc9e85493eb1a59b243d4a735f1ce363c13b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:35:30 +0100 Subject: Fix rubocop --- app/models/project_services/prometheus_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 676cc32b2d9..ae68fd9c3da 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -127,6 +127,7 @@ class PrometheusService < MonitoringService def prometheus_installed? return false if template? + project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end -- cgit v1.2.3 From 659e6f32d3d8e25f473c8a7413e5c6a0d19e41f6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 29 Jan 2018 08:37:56 -0700 Subject: Fix labels specs --- app/views/shared/_delete_label_modal.html.haml | 4 ++-- app/views/shared/_label.html.haml | 2 +- features/steps/project/issues/labels.rb | 5 +++-- spec/features/projects/labels/update_prioritization_spec.rb | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/views/shared/_delete_label_modal.html.haml b/app/views/shared/_delete_label_modal.html.haml index f29c8f00d52..01effefc34d 100644 --- a/app/views/shared/_delete_label_modal.html.haml +++ b/app/views/shared/_delete_label_modal.html.haml @@ -7,8 +7,8 @@ .modal-body %p - %strong #{label.name} - will be permanently deleted from #{label.group.name}. This cannot be undone. + %strong= label.name + %span will be permanently deleted from #{label.is_a?(ProjectLabel)? label.project.name : label.group.name}. This cannot be undone. .modal-footer %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index dd9b4b4db44..9e140cd3f13 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -5,7 +5,7 @@ - show_label_merge_requests_link = show_label_issuables_link?(label, :merge_requests, project: @project) - show_label_issues_link = show_label_issuables_link?(label, :issues, project: @project) -%li{ id: label_css_id, data: { id: label.id } } +%li.label-list-item{ id: label_css_id, data: { id: label.id } } = render "shared/label_row", label: label .visible-xs.visible-sm-inline-block.dropdown diff --git a/features/steps/project/issues/labels.rb b/features/steps/project/issues/labels.rb index 196e0fff63a..4df96e081f9 100644 --- a/features/steps/project/issues/labels.rb +++ b/features/steps/project/issues/labels.rb @@ -15,8 +15,9 @@ class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps step 'I delete all labels' do page.within '.labels' do - page.all('.remove-row').each do - accept_confirm { first('.remove-row').click } + page.all('.label-list-item').each do + first('.remove-row').click + first(:link, 'Delete label').click end end end diff --git a/spec/features/projects/labels/update_prioritization_spec.rb b/spec/features/projects/labels/update_prioritization_spec.rb index 85bd776932b..ae8b1364ec7 100644 --- a/spec/features/projects/labels/update_prioritization_spec.rb +++ b/spec/features/projects/labels/update_prioritization_spec.rb @@ -99,7 +99,7 @@ feature 'Prioritize labels' do expect(page).to have_content 'wontfix' # Sort labels - drag_to(selector: '.js-prioritized-labels', from_index: 1, to_index: 2) + drag_to(selector: '.label-list-item', from_index: 1, to_index: 2) page.within('.prioritized-labels') do expect(first('li')).to have_content('feature') -- cgit v1.2.3 From f610a0cc17d1f9cd607810d5aabd7bf097f645d0 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Tue, 30 Jan 2018 13:36:27 -0700 Subject: Fix padding & font size on labels --- app/assets/stylesheets/framework/modal.scss | 5 +++-- app/assets/stylesheets/pages/labels.scss | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 5438a4fa207..1b785341246 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -5,8 +5,9 @@ .page-title { margin-top: 0; - .label { - font-size: initial; + .color-label { + font-size: 14px; + padding: 6px 10px; } } } diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 9c4f51fc0d9..a72e654824e 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -105,7 +105,7 @@ } .label { - padding: 8px 9px 9px; + padding: 8px 12px; font-size: 14px; } } -- cgit v1.2.3 From 5126b1c5d3ba71a3ddad7e59142e46344b4c0eda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 22:02:44 +0000 Subject: Update auto_devops_domain help block with i18n --- app/views/admin/application_settings/_form.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index d77d99180fe..e49fbafa61d 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -254,7 +254,7 @@ .col-sm-10 = f.text_field :auto_devops_domain, class: 'form-control', placeholder: 'domain.com' .help-block - = s_("Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") + = s_("AdminSettings|Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") .form-group .col-sm-offset-2.col-sm-10 .checkbox -- cgit v1.2.3 From d2688b2855c9a039335da0ad2484aa02a44c1b2b Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Tue, 30 Jan 2018 17:13:54 -0600 Subject: add manage button to application rows in cluster integration --- .../clusters/components/application_row.vue | 18 +++++++++++++++++- .../javascripts/clusters/components/applications.vue | 15 +++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/clusters/components/application_row.vue b/app/assets/javascripts/clusters/components/application_row.vue index c13bbcee863..0aa3aead30d 100644 --- a/app/assets/javascripts/clusters/components/application_row.vue +++ b/app/assets/javascripts/clusters/components/application_row.vue @@ -32,6 +32,10 @@ type: String, required: false, }, + manageLink: { + type: String, + required: false, + }, description: { type: String, required: true, @@ -141,9 +145,21 @@