Merge branch '5350-geo-gprd-status-is-now-timing-out' into 'master'

Resolve "Geo: GPRD status is now timing out"

Closes #5350

See merge request gitlab-org/gitlab-ee!5230

doc/api/geo_nodes.md

@@ -192,10 +192,6 @@ Example response:
 GET /geo_nodes/:id/status
 ```
 
-| Attribute | Type    | Required | Description |
-| --------- | ------- | -------- | ----------- |
-| `refresh` | boolean | no       | Attempt to fetch the latest status from the Geo node directly, ignoring the cache |
-
 ```bash
 curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/geo_nodes/2/status
 ```

ee/app/assets/javascripts/geo_nodes/service/geo_nodes_service.js

@@ -13,11 +13,7 @@ export default class GeoNodesService {
 
   // eslint-disable-next-line class-methods-use-this
   getGeoNodeDetails(node) {
-    return axios.get(node.statusPath, {
-      params: {
-        refresh: true,
-      },
-    });
+    return axios.get(node.statusPath);
   }
 
   // eslint-disable-next-line class-methods-use-this

ee/app/models/geo_node_status.rb

@@ -85,6 +85,24 @@ class GeoNodeStatus < ActiveRecord::Base
     status
   end
 
+  def self.fast_current_node_status
+    # Primary's status is easy to calculate so we can calculate it on the fly
+    return current_node_status if Gitlab::Geo.primary?
+
+    spawn_worker
+
+    attrs = Rails.cache.read(cache_key) || {}
+    new(attrs)
+  end
+
+  def self.spawn_worker
+    ::Geo::MetricsUpdateWorker.perform_async
+  end
+
+  def self.cache_key
+    "geo-node:#{Gitlab::Geo.current_node.id}:status"
+  end
+
   def self.from_json(json_data)
     json_data.slice!(*allowed_params)
 
@@ -109,6 +127,10 @@ class GeoNodeStatus < ActiveRecord::Base
     self.repository_verification_enabled = Feature.enabled?('geo_repository_verification')
   end
 
+  def update_cache!
+    Rails.cache.write(self.class.cache_key, attributes)
+  end
+
   def load_data_from_current_node
     self.status_message =
       begin

ee/app/services/geo/metrics_update_service.rb

@@ -24,7 +24,6 @@ module Geo
 
     def fetch_geo_node_metrics(node)
       return unless node&.enabled?
-      return unless Gitlab::Geo.primary? || Gitlab::Metrics.prometheus_metrics_enabled?
 
       status = node_status(node)
 
@@ -34,6 +33,7 @@ module Geo
       end
 
       update_db_metrics(node, status) if Gitlab::Geo.primary?
+      status.update_cache! if node.current?
       update_prometheus_metrics(node, status) if Gitlab::Metrics.prometheus_metrics_enabled?
     end
 

ee/changelogs/unreleased/5350-geo-gprd-status-is-now-timing-out.yml

+---
+title: 'Geo: Use a pre-built node status in admin area'
+merge_request:
+author:
+type: fixed

ee/lib/api/geo.rb

@@ -34,7 +34,7 @@ module API
       get 'status' do
         authenticate_by_gitlab_geo_node_token!
 
-        status = ::GeoNodeStatus.current_node_status
+        status = ::GeoNodeStatus.fast_current_node_status
         present status, with: EE::API::Entities::GeoNodeStatus
       end
     end

ee/lib/api/geo_nodes.rb

@@ -65,11 +65,9 @@ module API
           def geo_node_status
             strong_memoize(:geo_node_status) do
               if geo_node.current?
-                GeoNodeStatus.current_node_status
-              elsif to_boolean(declared_params(include_missing: false)[:refresh])
-                ::Geo::NodeStatusFetchService.new.call(geo_node)
+                GeoNodeStatus.fast_current_node_status
               else
-                geo_node.status
+                ::Geo::NodeStatusFetchService.new.call(geo_node)
               end
             end
           end

ee/spec/models/geo_node_status_spec.rb

@@ -18,6 +18,34 @@ describe GeoNodeStatus, :geo do
     stub_current_geo_node(secondary)
   end
 
+  describe '#fast_current_node_status' do
+    it 'reads the cache and spawns the worker' do
+      expect(described_class).to receive(:spawn_worker).once
+
+      rails_cache = double
+      expect(rails_cache).to receive(:read).with(described_class.cache_key)
+      expect(Rails).to receive(:cache).and_return(rails_cache)
+
+      described_class.fast_current_node_status
+    end
+
+    it 'returns status for primary with no cache' do
+      stub_current_geo_node(primary)
+
+      expect(described_class.fast_current_node_status).to eq described_class.current_node_status
+    end
+  end
+
+  describe '#update_cache!' do
+    it 'writes a cache' do
+      rails_cache = double
+      expect(rails_cache).to receive(:write).with(described_class.cache_key, kind_of(Hash))
+      expect(Rails).to receive(:cache).and_return(rails_cache)
+
+      described_class.new.update_cache!
+    end
+  end
+
   describe '#healthy?' do
     context 'when health is blank' do
       it 'returns true' do

ee/spec/requests/api/geo_nodes_spec.rb

@@ -101,32 +101,6 @@ describe API::GeoNodes, :geo, api: true do
       expect(response).to match_response_schema('public_api/v4/geo_node_status', dir: 'ee')
     end
 
-    it 'fetches the real-time status with `refresh=true`' do
-      stub_current_geo_node(primary)
-      new_status = build(:geo_node_status, :healthy, geo_node: secondary, attachments_count: 923, lfs_objects_count: 652)
-
-      expect(GeoNode).to receive(:find).and_return(secondary)
-      expect_any_instance_of(Geo::NodeStatusFetchService).to receive(:call).and_return(new_status)
-
-      get api("/geo_nodes/#{secondary.id}/status", admin), refresh: true
-
-      expect(response).to have_gitlab_http_status(200)
-      expect(response).to match_response_schema('public_api/v4/geo_node_status', dir: 'ee')
-      expect(json_response['attachments_count']).to eq(923)
-      expect(json_response['lfs_objects_count']).to eq(652)
-    end
-
-    it 'returns 404 when no Geo Node status is not found' do
-      stub_current_geo_node(primary)
-      secondary_status.destroy!
-
-      expect(GeoNode).to receive(:find).and_return(secondary)
-
-      get api("/geo_nodes/#{secondary.id}/status", admin)
-
-      expect(response).to have_gitlab_http_status(404)
-    end
-
     it_behaves_like '404 response' do
       let(:request) { get api("/geo_nodes/#{unexisting_node_id}/status", admin) }
     end

ee/spec/requests/api/geo_spec.rb

@@ -208,6 +208,8 @@ describe API::Geo do
       end
 
       it 'responds with 200' do
+        allow(::GeoNodeStatus).to receive(:fast_current_node_status).and_return(::GeoNodeStatus.current_node_status)
+
         get api('/geo/status'), nil, request.headers
 
         expect(response).to have_gitlab_http_status(200)

ee/spec/services/geo/metrics_update_service_spec.rb

@@ -182,6 +182,16 @@ describe Geo::MetricsUpdateService, :geo do
         expect { subject.execute }.to change { metric_value(:geo_status_failed_total) }.by(1)
       end
 
+      it 'updates cache' do
+        status = GeoNodeStatus.new(success: true)
+
+        expect(status).to receive(:update_cache!)
+
+        allow(subject).to receive(:node_status).and_return(status)
+
+        subject.execute
+      end
+
       it 'does not create GeoNodeStatus entries' do
         expect { subject.execute }.to change { GeoNodeStatus.count }.by(0)
       end

spec/javascripts/geo_nodes/mock_data.js

@@ -49,7 +49,7 @@ export const mockNode = {
   nodeActionActive: false,
   basePath: 'http://127.0.0.1:3001/api/v4/geo_nodes/1',
   repairPath: 'http://127.0.0.1:3001/api/v4/geo_nodes/1/repair',
-  statusPath: 'http://127.0.0.1:3001/api/v4/geo_nodes/1/status?refresh=true',
+  statusPath: 'http://127.0.0.1:3001/api/v4/geo_nodes/1/status',
   editPath: 'http://127.0.0.1:3001/admin/geo_nodes/1/edit',
 };