Simplify retrying for ClusterWaitForIngressIpAddressWorker and style changes

(#42643)

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

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

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

app/services/clusters/applications/check_ingress_ip_address_service.rb

 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,13 +22,15 @@ 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
+      def service
+        strong_memoize(:ingress_service) do
           kubeclient.get_service('ingress-nginx-ingress-controller', Gitlab::Kubernetes::Helm::NAMESPACE)
         end
       end
     end
+  end
 end

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

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
 

spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb

 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

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