Refactor Nodes class

- Renames Nodes to Node
- Simplify the #all method by making readable auxiliar methods
- Standardize the #all method result to always return an array
- Adds specs for this file

app/models/clusters/cluster.rb

@@ -228,7 +228,9 @@ module Clusters
     def calculate_reactive_cache
       return unless enabled?
 
-      { connection_status: retrieve_connection_status, nodes: retrieve_nodes }
+      gitlab_kubernetes_nodes = Gitlab::Kubernetes::Node.new(self)
+
+      { connection_status: retrieve_connection_status, nodes: gitlab_kubernetes_nodes.all.presence }
     end
 
     def persisted_applications
@@ -383,54 +385,6 @@ module Clusters
       result[:status]
     end
 
-    def retrieve_nodes
-      result = ::Gitlab::Kubernetes::KubeClient.graceful_request(id) { kubeclient.get_nodes }
-
-      return unless result[:response]
-
-      cluster_nodes = result[:response]
-
-      result = ::Gitlab::Kubernetes::KubeClient.graceful_request(id) { kubeclient.metrics_client.get_nodes }
-      nodes_metrics = result[:response].to_a
-
-      cluster_nodes.inject([]) do |memo, node|
-        sliced_node = filter_relevant_node_attributes(node)
-
-        matched_node_metric = nodes_metrics.find { |node_metric| node_metric.metadata.name == node.metadata.name }
-
-        sliced_node_metrics = matched_node_metric ? filter_relevant_node_metrics_attributes(matched_node_metric) : {}
-
-        memo << sliced_node.merge(sliced_node_metrics)
-      end
-    end
-
-    def filter_relevant_node_attributes(node)
-      {
-        'metadata' => {
-          'name' => node.metadata.name
-        },
-        'status' => {
-          'capacity' => {
-            'cpu' => node.status.capacity.cpu,
-            'memory' => node.status.capacity.memory
-          },
-          'allocatable' => {
-            'cpu' => node.status.allocatable.cpu,
-            'memory' => node.status.allocatable.memory
-          }
-        }
-      }
-    end
-
-    def filter_relevant_node_metrics_attributes(node_metrics)
-      {
-        'usage' => {
-          'cpu' => node_metrics.usage.cpu,
-          'memory' => node_metrics.usage.memory
-        }
-      }
-    end
-
     # To keep backward compatibility with AUTO_DEVOPS_DOMAIN
     # environment variable, we need to ensure KUBE_INGRESS_BASE_DOMAIN
     # is set if AUTO_DEVOPS_DOMAIN is set on any of the following options:

lib/gitlab/kubernetes/node.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Kubernetes
+    class Node
+      def initialize(cluster)
+        @cluster = cluster
+      end
+
+      def all
+        nodes.map do |node|
+          attributes = node(node)
+          attributes.merge(node_metrics(node))
+        end
+      end
+
+      private
+
+      attr_reader :cluster
+
+      def nodes_from_cluster
+        graceful_request { cluster.kubeclient.get_nodes }
+      end
+
+      def nodes_metrics_from_cluster
+        graceful_request { cluster.kubeclient.metrics_client.get_nodes }
+      end
+
+      def nodes
+        @nodes ||= nodes_from_cluster[:response].to_a
+      end
+
+      def nodes_metrics
+        @nodes_metrics ||= nodes_metrics_from_cluster[:response].to_a
+      end
+
+      def node_metrics_from_node(node)
+        nodes_metrics.find do |node_metric|
+          node_metric.metadata.name == node.metadata.name
+        end
+      end
+
+      def graceful_request(&block)
+        ::Gitlab::Kubernetes::KubeClient.graceful_request(cluster.id, &block)
+      end
+
+      def node(node)
+        {
+          'metadata' => {
+            'name' => node.metadata.name
+          },
+          'status' => {
+            'capacity' => {
+              'cpu' => node.status.capacity.cpu,
+              'memory' => node.status.capacity.memory
+            },
+            'allocatable' => {
+              'cpu' => node.status.allocatable.cpu,
+              'memory' => node.status.allocatable.memory
+            }
+          }
+        }
+      end
+
+      def node_metrics(node)
+        node_metrics = node_metrics_from_node(node)
+        return {} unless node_metrics
+
+        {
+          'usage' => {
+            'cpu' => node_metrics.usage.cpu,
+            'memory' => node_metrics.usage.memory
+          }
+        }
+      end
+    end
+  end
+end

spec/lib/gitlab/kubernetes/node_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Kubernetes::Node do
+  include KubernetesHelpers
+
+  describe '#all' do
+    let(:cluster) { create(:cluster, :provided_by_user, :group) }
+    let(:expected_nodes) { [] }
+
+    before do
+      stub_kubeclient_nodes_and_nodes_metrics(cluster.platform.api_url)
+    end
+
+    subject { described_class.new(cluster).all }
+
+    context 'when connection to the cluster is successful' do
+      let(:expected_nodes) { [kube_node.merge(kube_node_metrics)] }
+
+      it { is_expected.to eq(expected_nodes) }
+    end
+
+    context 'when cluster cannot be reached' do
+      before do
+        allow(cluster.kubeclient.core_client).to receive(:discover)
+          .and_raise(SocketError)
+      end
+
+      it { is_expected.to eq(expected_nodes) }
+    end
+
+    context 'when cluster cannot be authenticated to' do
+      before do
+        allow(cluster.kubeclient.core_client).to receive(:discover)
+          .and_raise(OpenSSL::X509::CertificateError.new('Certificate error'))
+      end
+
+      it { is_expected.to eq(expected_nodes) }
+    end
+
+    context 'when Kubeclient::HttpError is raised' do
+      before do
+        allow(cluster.kubeclient.core_client).to receive(:discover)
+          .and_raise(Kubeclient::HttpError.new(403, 'Forbidden', nil))
+      end
+
+      it { is_expected.to eq(expected_nodes) }
+    end
+
+    context 'when an uncategorised error is raised' do
+      before do
+        allow(cluster.kubeclient.core_client).to receive(:discover)
+          .and_raise(StandardError)
+      end
+
+      it { is_expected.to eq(expected_nodes) }
+
+      it 'notifies Sentry' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception)
+          .with(instance_of(StandardError), hash_including(cluster_id: cluster.id))
+          .once
+
+        subject
+      end
+    end
+  end
+end

spec/models/clusters/cluster_spec.rb

@@ -1111,13 +1111,23 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
 
     context 'cluster is enabled' do
       let(:cluster) { create(:cluster, :provided_by_user, :group) }
+      let(:gl_k8s_node_double) { double(Gitlab::Kubernetes::Node) }
+      let(:expected_nodes) { nil }
 
       before do
-        stub_kubeclient_nodes_and_nodes_metrics(cluster.platform.api_url)
+        stub_kubeclient_discover(cluster.platform.api_url)
+        allow(Gitlab::Kubernetes::Node).to receive(:new).with(cluster).and_return(gl_k8s_node_double)
+        allow(gl_k8s_node_double).to receive(:all).and_return([])
       end
 
       context 'connection to the cluster is successful' do
-        it { is_expected.to eq(connection_status: :connected, nodes: [kube_node.merge(kube_node_metrics)]) }
+        before do
+          allow(gl_k8s_node_double).to receive(:all).and_return(expected_nodes)
+        end
+
+        let(:expected_nodes) { [kube_node.merge(kube_node_metrics)] }
+
+        it { is_expected.to eq(connection_status: :connected, nodes: expected_nodes) }
       end
 
       context 'cluster cannot be reached' do
@@ -1126,7 +1136,7 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
             .and_raise(SocketError)
         end
 
-        it { is_expected.to eq(connection_status: :unreachable, nodes: nil) }
+        it { is_expected.to eq(connection_status: :unreachable, nodes: expected_nodes) }
       end
 
       context 'cluster cannot be authenticated to' do
@@ -1135,7 +1145,7 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
             .and_raise(OpenSSL::X509::CertificateError.new("Certificate error"))
         end
 
-        it { is_expected.to eq(connection_status: :authentication_failure, nodes: nil) }
+        it { is_expected.to eq(connection_status: :authentication_failure, nodes: expected_nodes) }
       end
 
       describe 'Kubeclient::HttpError' do
@@ -1147,18 +1157,18 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
             .and_raise(Kubeclient::HttpError.new(error_code, error_message, nil))
         end
 
-        it { is_expected.to eq(connection_status: :authentication_failure, nodes: nil) }
+        it { is_expected.to eq(connection_status: :authentication_failure, nodes: expected_nodes) }
 
         context 'generic timeout' do
           let(:error_message) { 'Timed out connecting to server'}
 
-          it { is_expected.to eq(connection_status: :unreachable, nodes: nil) }
+          it { is_expected.to eq(connection_status: :unreachable, nodes: expected_nodes) }
         end
 
         context 'gateway timeout' do
           let(:error_message) { '504 Gateway Timeout for GET https://kubernetes.example.com/api/v1'}
 
-          it { is_expected.to eq(connection_status: :unreachable, nodes: nil) }
+          it { is_expected.to eq(connection_status: :unreachable, nodes: expected_nodes) }
         end
       end
 
@@ -1168,12 +1178,12 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
             .and_raise(StandardError)
         end
 
-        it { is_expected.to eq(connection_status: :unknown_failure, nodes: nil) }
+        it { is_expected.to eq(connection_status: :unknown_failure, nodes: expected_nodes) }
 
         it 'notifies Sentry' do
           expect(Gitlab::ErrorTracking).to receive(:track_exception)
             .with(instance_of(StandardError), hash_including(cluster_id: cluster.id))
-            .twice
+            .once
 
           subject
         end