diff options
author | Dylan Griffith <dyl.griffith@gmail.com> | 2018-02-23 09:08:12 +1100 |
---|---|---|
committer | Dylan Griffith <dyl.griffith@gmail.com> | 2018-02-23 09:10:14 +1100 |
commit | 3b320d675fe058311d921e26cd89b2e703310b21 (patch) | |
tree | c8135b9041f238bc2d435fea26cf3c3dd6940cd8 | |
parent | 17e85dacdd73b51a173d1f4c5efea5e20ee8c55b (diff) | |
download | gitlab-ce-3b320d675fe058311d921e26cd89b2e703310b21.tar.gz |
Simplify retrying for ClusterWaitForIngressIpAddressWorker and style changes
(#42643)
8 files changed, 31 insertions, 126 deletions
diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb index c351caeeb7b..aeaba3a0acf 100644 --- a/app/controllers/projects/clusters_controller.rb +++ b/app/controllers/projects/clusters_controller.rb @@ -4,7 +4,7 @@ class Projects::ClustersController < Projects::ApplicationController before_action :authorize_create_cluster!, only: [:new] before_action :authorize_update_cluster!, only: [:update] before_action :authorize_admin_cluster!, only: [:destroy] - before_action :sync_application_details, only: [:status] + before_action :update_applications_status, only: [:status] STATUS_POLLING_INTERVAL = 10_000 @@ -116,7 +116,7 @@ class Projects::ClustersController < Projects::ApplicationController access_denied! unless can?(current_user, :admin_cluster, cluster) end - def sync_application_details - @cluster.applications.each(&:sync_details) + def update_applications_status + @cluster.applications.each(&:schedule_status_update) end end diff --git a/app/models/clusters/applications/ingress.rb b/app/models/clusters/applications/ingress.rb index e36fe9019c2..57ced0f5c44 100644 --- a/app/models/clusters/applications/ingress.rb +++ b/app/models/clusters/applications/ingress.rb @@ -37,12 +37,12 @@ module Clusters Gitlab::Kubernetes::Helm::InstallCommand.new(name, chart: chart, chart_values_file: chart_values_file) end - def sync_details + def schedule_status_update return unless installed? return if external_ip - ClusterWaitForIngressIpAddressWorker.perform_in( - ClusterWaitForIngressIpAddressWorker::INTERVAL, name, id, IP_ADDRESS_FETCH_RETRIES) + ClusterWaitForIngressIpAddressWorker.perform_async( + name, id, IP_ADDRESS_FETCH_RETRIES) end end end diff --git a/app/models/clusters/concerns/application_core.rb b/app/models/clusters/concerns/application_core.rb index 44b0fca3d01..623b836c0ed 100644 --- a/app/models/clusters/concerns/application_core.rb +++ b/app/models/clusters/concerns/application_core.rb @@ -24,7 +24,7 @@ module Clusters self.class.application_name end - def sync_details + def schedule_status_update # Override if you need extra data synchronized # from K8s after installation end diff --git a/app/services/clusters/applications/check_ingress_ip_address_service.rb b/app/services/clusters/applications/check_ingress_ip_address_service.rb index 300b7ed522c..a98e05b25cb 100644 --- a/app/services/clusters/applications/check_ingress_ip_address_service.rb +++ b/app/services/clusters/applications/check_ingress_ip_address_service.rb @@ -1,24 +1,17 @@ module Clusters module Applications class CheckIngressIpAddressService < BaseHelmService + include Gitlab::Utils::StrongMemoize + Error = Class.new(StandardError) LEASE_TIMEOUT = 3.seconds.to_i def execute - return true if app.external_ip - return true unless try_obtain_lease - - service = get_service + return if app.external_ip + return unless try_obtain_lease - if service.status.loadBalancer.ingress - resolve_external_ip(service) - else - false - end - - rescue KubeException => e - raise Error, "#{e.class}: #{e.message}" + app.update!(external_ip: ingress_ip) if ingress_ip end private @@ -29,12 +22,14 @@ module Clusters .try_obtain end - def resolve_external_ip(service) - app.update!(external_ip: service.status.loadBalancer.ingress[0].ip) + def ingress_ip + service.status.loadBalancer.ingress&.first&.ip end - def get_service - kubeclient.get_service('ingress-nginx-ingress-controller', Gitlab::Kubernetes::Helm::NAMESPACE) + def service + strong_memoize(:ingress_service) do + kubeclient.get_service('ingress-nginx-ingress-controller', Gitlab::Kubernetes::Helm::NAMESPACE) + end end end end diff --git a/app/workers/cluster_wait_for_ingress_ip_address_worker.rb b/app/workers/cluster_wait_for_ingress_ip_address_worker.rb index 0fbb9fb2526..72b3c8b49e0 100644 --- a/app/workers/cluster_wait_for_ingress_ip_address_worker.rb +++ b/app/workers/cluster_wait_for_ingress_ip_address_worker.rb @@ -3,23 +3,11 @@ class ClusterWaitForIngressIpAddressWorker include ClusterQueue include ClusterApplications - INTERVAL = 10.seconds + INTERVAL = 30.seconds def perform(app_name, app_id, retries_remaining) find_application(app_name, app_id) do |app| - result = Clusters::Applications::CheckIngressIpAddressService.new(app).execute - retry_if_necessary(app_name, app_id, retries_remaining) unless result - end - rescue Clusters::Applications::CheckIngressIpAddressService::Error => e - retry_if_necessary(app_name, app_id, retries_remaining) - raise e - end - - private - - def retry_if_necessary(app_name, app_id, retries_remaining) - if retries_remaining > 0 - self.class.perform_in(INTERVAL, app_name, app_id, retries_remaining - 1) + Clusters::Applications::CheckIngressIpAddressService.new(app).execute end end end diff --git a/spec/models/clusters/applications/ingress_spec.rb b/spec/models/clusters/applications/ingress_spec.rb index ced5a4ee4d5..80c450c30cb 100644 --- a/spec/models/clusters/applications/ingress_spec.rb +++ b/spec/models/clusters/applications/ingress_spec.rb @@ -6,6 +6,7 @@ describe Clusters::Applications::Ingress do before do allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_in) + allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_async) end include_examples 'cluster application specs', described_class @@ -23,23 +24,23 @@ describe Clusters::Applications::Ingress do end end - describe '#sync_details' do + describe '#schedule_status_update' do let(:application) { create(:clusters_applications_ingress, :installed) } before do - application.sync_details + application.schedule_status_update end it 'schedules a ClusterWaitForIngressIpAddressWorker' do - expect(ClusterWaitForIngressIpAddressWorker).to have_received(:perform_in) - .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', application.id, 3) + expect(ClusterWaitForIngressIpAddressWorker).to have_received(:perform_async) + .with('ingress', application.id, 3) end context 'when the application is not installed' do let(:application) { create(:clusters_applications_ingress, :installing) } it 'does not schedule a ClusterWaitForIngressIpAddressWorker' do - expect(ClusterWaitForIngressIpAddressWorker).not_to have_received(:perform_in) + expect(ClusterWaitForIngressIpAddressWorker).not_to have_received(:perform_async) end end diff --git a/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb b/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb index a9915493d42..e14c82d32d4 100644 --- a/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb +++ b/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' describe Clusters::Applications::CheckIngressIpAddressService do - subject { service.execute } let(:application) { create(:clusters_applications_ingress, :installed) } let(:service) { described_class.new(application) } let(:kubeclient) { double(::Kubeclient::Client, get_service: kube_service) } @@ -20,6 +19,8 @@ describe Clusters::Applications::CheckIngressIpAddressService do ) end + subject { service.execute } + before do allow(application.cluster).to receive(:kubeclient).and_return(kubeclient) allow(Gitlab::ExclusiveLease) @@ -30,8 +31,6 @@ describe Clusters::Applications::CheckIngressIpAddressService do describe '#execute' do context 'when the ingress ip address is available' do - it { is_expected.to eq(true) } - it 'updates the external_ip for the app' do subject @@ -42,7 +41,9 @@ describe Clusters::Applications::CheckIngressIpAddressService do context 'when the ingress ip address is not available' do let(:ingress) { nil } - it { is_expected.to eq(false) } + it 'does not error' do + subject + end end context 'when the exclusive lease cannot be obtained' do @@ -52,8 +53,6 @@ describe Clusters::Applications::CheckIngressIpAddressService do .and_return(false) end - it { is_expected.to eq(true) } - it 'does not call kubeclient' do subject @@ -64,24 +63,11 @@ describe Clusters::Applications::CheckIngressIpAddressService do context 'when there is already an external_ip' do let(:application) { create(:clusters_applications_ingress, :installed, external_ip: '001.111.002.111') } - it { is_expected.to eq(true) } - it 'does not call kubeclient' do subject expect(kubeclient).not_to have_received(:get_service) end end - - context 'when a kubernetes error occurs' do - before do - allow(kubeclient).to receive(:get_service).and_raise(KubeException.new(500, 'something blew up', nil)) - end - - it 'it raises Clusters::Applications::CheckIngressIpAddressServiceError' do - expect { subject } - .to raise_error(Clusters::Applications::CheckIngressIpAddressService::Error, "KubeException: something blew up") - end - end end end diff --git a/spec/workers/cluster_wait_for_ingress_ip_address_worker_spec.rb b/spec/workers/cluster_wait_for_ingress_ip_address_worker_spec.rb index aea924c6dd6..baa295984bf 100644 --- a/spec/workers/cluster_wait_for_ingress_ip_address_worker_spec.rb +++ b/spec/workers/cluster_wait_for_ingress_ip_address_worker_spec.rb @@ -26,70 +26,5 @@ describe ClusterWaitForIngressIpAddressWorker do expect(service).to have_received(:execute) end - - context 'when the service succeeds' do - it 'does not schedule another worker' do - worker.perform('ingress', 117, 2) - - expect(described_class) - .not_to have_received(:perform_in) - end - end - - context 'when the service fails' do - before do - allow(service) - .to receive(:execute) - .and_return(false) - end - - context 'when there are retries remaining' do - it 'schedules another worker with 1 less retry' do - worker.perform('ingress', 117, 2) - - expect(described_class) - .to have_received(:perform_in) - .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', 117, 1) - end - end - - context 'when there are no retries_remaining' do - it 'does not schedule another worker' do - worker.perform('ingress', 117, 0) - - expect(described_class) - .not_to have_received(:perform_in) - end - end - end - - context 'when the update raises exception' do - before do - allow(service) - .to receive(:execute) - .and_raise(Clusters::Applications::CheckIngressIpAddressService::Error, "something went wrong") - end - - context 'when there are retries remaining' do - it 'schedules another worker with 1 less retry and re-raises the error' do - expect { worker.perform('ingress', 117, 2) } - .to raise_error(Clusters::Applications::CheckIngressIpAddressService::Error, "something went wrong") - - expect(described_class) - .to have_received(:perform_in) - .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', 117, 1) - end - end - - context 'when there are no retries_remaining' do - it 'does not schedule another worker but re-raises the error' do - expect { worker.perform('ingress', 117, 0) } - .to raise_error(Clusters::Applications::CheckIngressIpAddressService::Error, "something went wrong") - - expect(described_class) - .not_to have_received(:perform_in) - end - end - end end end |