Refactor ingress IP address waiting code (#42643)

4 files changed, 109 insertions, 44 deletions

diff --git a/spec/fixtures/api/schemas/cluster_status.json b/spec/fixtures/api/schemas/cluster_status.json
index 9617157ee37..d27c12e43f2 100644
--- a/spec/fixtures/api/schemas/cluster_status.json
+++ b/spec/fixtures/api/schemas/cluster_status.json
@@ -31,7 +31,7 @@
           }
         },
         "status_reason": { "type": ["string", "null"] },
-        "external_ip": { "type": ["string", null] }
+        "external_ip": { "type": ["string", "null"] }
       },
       "required" : [ "name", "status" ]
     }
diff --git a/spec/models/clusters/applications/ingress_spec.rb b/spec/models/clusters/applications/ingress_spec.rb
index da9535f9d16..c8109bf3cf6 100644
--- a/spec/models/clusters/applications/ingress_spec.rb
+++ b/spec/models/clusters/applications/ingress_spec.rb
@@ -4,16 +4,19 @@ describe Clusters::Applications::Ingress do
   it { is_expected.to belong_to(:cluster) }
   it { is_expected.to validate_presence_of(:cluster) }
 
-  include_examples 'cluster application specs', described_class
+  before do
+    allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_in)
+  end
 
-  describe '#post_install' do
-    let(:application) { create(:clusters_applications_ingress, :installed) }
+  include_examples 'cluster application specs', described_class
 
+  describe '#make_installed!' do
     before do
-      allow(ClusterWaitForIngressIpAddressWorker).to receive(:perform_in)
-      application.post_install
+      application.make_installed!
     end
 
+    let(:application) { create(:clusters_applications_ingress, :installing) }
+
     it 'schedules a ClusterWaitForIngressIpAddressWorker' do
       expect(ClusterWaitForIngressIpAddressWorker).to have_received(:perform_in)
         .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', application.id, 3)
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 6c81acfcf84..a9915493d42 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,8 +1,13 @@
 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) }
+  let(:ingress) { [{ ip: '111.222.111.222' }] }
+  let(:exclusive_lease) { instance_double(Gitlab::ExclusiveLease, try_obtain: true) }
+
   let(:kube_service) do
     ::Kubeclient::Resource.new(
       {
@@ -14,9 +19,6 @@ describe Clusters::Applications::CheckIngressIpAddressService do
       }
     )
   end
-  let(:kubeclient) { double(::Kubeclient::Client, get_service: kube_service) }
-  let(:ingress) { [{ ip: '111.222.111.222' }] }
-  let(:exclusive_lease) { instance_double(Gitlab::ExclusiveLease, try_obtain: true) }
 
   before do
     allow(application.cluster).to receive(:kubeclient).and_return(kubeclient)
@@ -28,10 +30,10 @@ describe Clusters::Applications::CheckIngressIpAddressService do
 
   describe '#execute' do
     context 'when the ingress ip address is available' do
-      it 'updates the external_ip for the app and does not schedule another worker' do
-        expect(ClusterWaitForIngressIpAddressWorker).not_to receive(:perform_in)
+      it { is_expected.to eq(true) }
 
-        service.execute(1)
+      it 'updates the external_ip for the app' do
+        subject
 
         expect(application.external_ip).to eq('111.222.111.222')
       end
@@ -40,21 +42,7 @@ describe Clusters::Applications::CheckIngressIpAddressService do
     context 'when the ingress ip address is not available' do
       let(:ingress) { nil }
 
-      it 'it schedules another worker with 1 less retry' do
-        expect(ClusterWaitForIngressIpAddressWorker)
-          .to receive(:perform_in)
-          .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', application.id, 0)
-
-        service.execute(1)
-      end
-
-      context 'when no more retries remaining' do
-        it 'does not schedule another worker' do
-          expect(ClusterWaitForIngressIpAddressWorker).not_to receive(:perform_in)
-
-          service.execute(0)
-        end
-      end
+      it { is_expected.to eq(false) }
     end
 
     context 'when the exclusive lease cannot be obtained' do
@@ -64,22 +52,24 @@ describe Clusters::Applications::CheckIngressIpAddressService do
           .and_return(false)
       end
 
+      it { is_expected.to eq(true) }
+
       it 'does not call kubeclient' do
-        expect(kubeclient).not_to receive(:get_service)
+        subject
 
-        service.execute(1)
+        expect(kubeclient).not_to have_received(:get_service)
       end
     end
 
     context 'when there is already an external_ip' do
       let(:application) { create(:clusters_applications_ingress, :installed, external_ip: '001.111.002.111') }
 
-      it 'does nothing' do
-        expect(kubeclient).not_to receive(:get_service)
+      it { is_expected.to eq(true) }
 
-        service.execute(1)
+      it 'does not call kubeclient' do
+        subject
 
-        expect(application.external_ip).to eq('001.111.002.111')
+        expect(kubeclient).not_to have_received(:get_service)
       end
     end
 
@@ -88,12 +78,9 @@ describe Clusters::Applications::CheckIngressIpAddressService do
         allow(kubeclient).to receive(:get_service).and_raise(KubeException.new(500, 'something blew up', nil))
       end
 
-      it 'it schedules another worker with 1 less retry' do
-        expect(ClusterWaitForIngressIpAddressWorker)
-          .to receive(:perform_in)
-          .with(ClusterWaitForIngressIpAddressWorker::INTERVAL, 'ingress', application.id, 0)
-
-        service.execute(1)
+      it 'it raises Clusters::Applications::CheckIngressIpAddressServiceError' do
+        expect { subject }
+          .to raise_error(Clusters::Applications::CheckIngressIpAddressService::Error, "KubeException: something blew up")
       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 9f8bf35f604..aea924c6dd6 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
@@ -2,19 +2,94 @@ require 'spec_helper'
 
 describe ClusterWaitForIngressIpAddressWorker do
   describe '#perform' do
-    let(:service) { instance_double(Clusters::Applications::CheckIngressIpAddressService) }
+    let(:service) { instance_double(Clusters::Applications::CheckIngressIpAddressService, execute: true) }
     let(:application) { instance_double(Clusters::Applications::Ingress) }
     let(:worker) { described_class.new }
 
-    it 'finds the application and calls CheckIngressIpAddressService#execute' do
-      expect(worker).to receive(:find_application).with('ingress', 117).and_yield(application)
-      expect(Clusters::Applications::CheckIngressIpAddressService)
+    before do
+      allow(worker)
+        .to receive(:find_application)
+        .with('ingress', 117)
+        .and_yield(application)
+
+      allow(Clusters::Applications::CheckIngressIpAddressService)
         .to receive(:new)
         .with(application)
         .and_return(service)
-      expect(service).to receive(:execute).with(2)
 
+      allow(described_class)
+        .to receive(:perform_in)
+    end
+
+    it 'finds the application and calls CheckIngressIpAddressService#execute' do
       worker.perform('ingress', 117, 2)
+
+      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