Merge branch '5769-improve-pod-logs-api' into 'master'

Improve pod logs API

See merge request gitlab-org/gitlab!17881

ee/app/controllers/ee/projects/environments_controller.rb

@@ -23,16 +23,9 @@ module EE
             if result[:status] == :processing
               head :accepted
             elsif result[:status] == :success
-              render json: {
-                pods: environment.pod_names,
-                logs: result[:logs],
-                message: result[:message]
-              }
+              render json: result
             else
-              render status: :bad_request, json: {
-                pods: environment.pod_names,
-                message: result[:message]
-              }
+              render status: :bad_request, json: result
             end
           end
         end

ee/app/models/ee/clusters/platforms/kubernetes.rb

@@ -40,11 +40,11 @@ module EE
         def calculate_reactive_cache(request, opts)
           case request
           when 'get_pod_log'
-            handle_exceptions(_('Pod not found')) do
-              container = opts['container']
-              pod_name = opts['pod_name']
-              namespace = opts['namespace']
+            container = opts['container']
+            pod_name = opts['pod_name']
+            namespace = opts['namespace']
 
+            handle_exceptions(_('Pod not found'), pod_name: pod_name, container_name: container) do
               container ||= container_names_of(pod_name, namespace).first
 
               pod_logs(pod_name, namespace, container: container)
@@ -59,13 +59,21 @@ module EE
             pod_name, namespace, container: container, tail_lines: LOGS_LIMIT
           ).body
 
-          { logs: logs, status: :success }
+          {
+            logs: logs,
+            status: :success,
+            pod_name: pod_name,
+            container_name: container
+          }
         end
 
-        def handle_exceptions(resource_not_found_error_message, &block)
+        def handle_exceptions(resource_not_found_error_message, opts, &block)
           yield
         rescue Kubeclient::ResourceNotFoundError
-          { error: resource_not_found_error_message, status: :error }
+          {
+            error: resource_not_found_error_message,
+            status: :error
+          }.merge(opts)
         rescue Kubeclient::HttpError => e
           ::Gitlab::Sentry.track_acceptable_exception(e)
 
@@ -74,7 +82,7 @@ module EE
               error_code: e.error_code
             },
             status: :error
-          }
+          }.merge(opts)
         end
 
         def container_names_of(pod_name, namespace)

ee/app/models/ee/environment.rb

@@ -64,7 +64,13 @@ module EE
     end
 
     def pod_names
-      return [] unless rollout_status
+      return [] unless rollout_status_available?
+
+      rollout_status = rollout_status_with_reactive_cache
+
+      # If cache has not been populated yet, rollout_status will be nil and the
+      # caller should try again later.
+      return unless rollout_status
 
       rollout_status.instances.map do |instance|
         instance[:pod_name]
@@ -88,13 +94,23 @@ module EE
     end
 
     def rollout_status
-      return unless has_terminals?
+      return unless rollout_status_available?
 
-      result = with_reactive_cache do |data|
-        deployment_platform.rollout_status(self, data)
-      end
+      result = rollout_status_with_reactive_cache
 
       result || ::Gitlab::Kubernetes::RolloutStatus.loading
     end
+
+    private
+
+    def rollout_status_available?
+      has_terminals?
+    end
+
+    def rollout_status_with_reactive_cache
+      with_reactive_cache do |data|
+        deployment_platform.rollout_status(self, data)
+      end
+    end
   end
 end

ee/app/services/pod_logs_service.rb

@@ -9,10 +9,11 @@ class PodLogsService < ::BaseService
 
   PARAMS = %w(pod_name container_name).freeze
 
-  SUCCESS_RETURN_KEYS = [:status, :logs].freeze
+  SUCCESS_RETURN_KEYS = [:status, :logs, :pod_name, :container_name, :pods].freeze
 
   steps :check_param_lengths,
     :check_deployment_platform,
+    :check_pod_names,
     :check_pod_name,
     :pod_logs,
     :split_logs,
@@ -52,10 +53,18 @@ class PodLogsService < ::BaseService
     success(result)
   end
 
+  def check_pod_names(result)
+    result[:pods] = environment.pod_names
+
+    return { status: :processing } unless result[:pods]
+
+    success(result)
+  end
+
   def check_pod_name(result)
     # If pod_name is not received as parameter, get the pod logs of the first
     # pod of this environment.
-    result[:pod_name] ||= environment.pod_names&.first
+    result[:pod_name] ||= result[:pods].first
 
     unless result[:pod_name]
       return error(_('No pods available'))
@@ -73,18 +82,18 @@ class PodLogsService < ::BaseService
 
     return { status: :processing } unless response
 
-    result[:logs] = response[:logs]
+    result.merge!(response.slice(:pod_name, :container_name, :logs))
 
     if response[:status] == :error
-      error(response[:error])
+      error(response[:error]).reverse_merge(result)
     else
       success(result)
     end
   end
 
   def split_logs(result)
-    logs = split_by_newline(result[:logs])
-    success(logs: logs)
+    result[:logs] = split_by_newline(result[:logs])
+    success(result)
   end
 
   def filter_return_keys(result)

ee/spec/controllers/projects/environments_controller_spec.rb

@@ -91,14 +91,14 @@ describe Projects::EnvironmentsController do
     end
 
     shared_examples 'resource not found' do |message|
-      let(:service_result) { { status: :error, message: message } }
-
       it 'returns 400' do
         get :logs, params: environment_params(pod_name: pod_name, format: :json)
 
         expect(response).to have_gitlab_http_status(:bad_request)
         expect(json_response['message']).to eq(message)
         expect(json_response['pods']).to match_array([pod_name])
+        expect(json_response['pod_name']).to eq(pod_name)
+        expect(json_response['container_name']).to eq(container)
       end
     end
 
@@ -124,11 +124,16 @@ describe Projects::EnvironmentsController do
     end
 
     context 'when using JSON format' do
+      let(:container) { 'container-1' }
+
       let(:service_result) do
         {
           status: :success,
           logs: ['Log 1', 'Log 2', 'Log 3'],
-          message: 'message'
+          message: 'message',
+          pods: [pod_name],
+          pod_name: pod_name,
+          container_name: container
         }
       end
 
@@ -143,6 +148,8 @@ describe Projects::EnvironmentsController do
         expect(json_response["logs"]).to match_array(["Log 1", "Log 2", "Log 3"])
         expect(json_response["pods"]).to match_array([pod_name])
         expect(json_response['message']).to eq(service_result[:message])
+        expect(json_response['pod_name']).to eq(pod_name)
+        expect(json_response['container_name']).to eq(container)
       end
 
       it 'registers a usage of the endpoint' do
@@ -152,7 +159,15 @@ describe Projects::EnvironmentsController do
       end
 
       context 'when kubernetes API returns error' do
-        let(:service_result) { { status: :error, message: 'Kubernetes API returned status code: 400' } }
+        let(:service_result) do
+          {
+            status: :error,
+            message: 'Kubernetes API returned status code: 400',
+            pods: [pod_name],
+            pod_name: pod_name,
+            container_name: container
+          }
+        end
 
         it 'returns bad request' do
           get :logs, params: environment_params(pod_name: pod_name, format: :json)
@@ -161,15 +176,42 @@ describe Projects::EnvironmentsController do
           expect(json_response["logs"]).to eq(nil)
           expect(json_response["pods"]).to match_array([pod_name])
           expect(json_response["message"]).to eq('Kubernetes API returned status code: 400')
+          expect(json_response['pod_name']).to eq(pod_name)
+          expect(json_response['container_name']).to eq(container)
         end
       end
 
       context 'when pod does not exist' do
-        let(:service_result) { { status: :error, message: 'Pod not found' } }
+        let(:service_result) do
+          {
+            status: :error,
+            message: 'Pod not found',
+            pods: [pod_name],
+            pod_name: pod_name,
+            container_name: container
+          }
+        end
 
         it_behaves_like 'resource not found', 'Pod not found'
       end
 
+      context 'when service returns error without pods, pod_name, container_name' do
+        let(:service_result) do
+          {
+            status: :error,
+            message: 'No deployment platform'
+          }
+        end
+
+        it 'returns the error without pods, pod_name and container_name' do
+          get :logs, params: environment_params(pod_name: pod_name, format: :json)
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']).to eq('No deployment platform')
+          expect(json_response.keys).to contain_exactly('message', 'status')
+        end
+      end
+
       context 'when service returns status processing' do
         let(:service_result) { { status: :processing } }
 

ee/spec/features/projects/environments_pod_logs_spec.rb

@@ -22,7 +22,14 @@ describe 'Environment > Pod Logs', :js do
     stub_kubeclient_pod_details(pod_name, environment.deployment_namespace)
     stub_kubeclient_logs(pod_name, environment.deployment_namespace, container: 'container-0')
 
-    allow_any_instance_of(EE::Environment).to receive(:pod_names).and_return(pod_names)
+    # rollout_status_instances = [{ pod_name: foo }, {pod_name: bar}]
+    rollout_status_instances = pod_names.collect { |name| { pod_name: name } }
+    rollout_status = instance_double(
+      ::Gitlab::Kubernetes::RolloutStatus, instances: rollout_status_instances
+    )
+
+    allow_any_instance_of(EE::Environment).to receive(:rollout_status_with_reactive_cache)
+      .and_return(rollout_status)
 
     sign_in(project.owner)
   end

ee/spec/models/ee/clusters/platforms/kubernetes_spec.rb

@@ -147,6 +147,15 @@ describe Clusters::Platforms::Kubernetes do
       it do
         expect(subject[:logs]).to eq("\"Log 1\\nLog 2\\nLog 3\"")
         expect(subject[:status]).to eq(:success)
+        expect(subject[:pod_name]).to eq(pod_name)
+        expect(subject[:container_name]).to eq(container)
+      end
+    end
+
+    shared_examples 'returns pod_name and container_name' do
+      it do
+        expect(subject[:pod_name]).to eq(pod_name)
+        expect(subject[:container_name]).to eq(container)
       end
     end
 
@@ -185,6 +194,8 @@ describe Clusters::Platforms::Kubernetes do
         end
 
         it_behaves_like 'kubernetes API error', 500
+
+        it_behaves_like 'returns pod_name and container_name'
       end
 
       context 'when container does not exist' do
@@ -196,6 +207,8 @@ describe Clusters::Platforms::Kubernetes do
         end
 
         it_behaves_like 'kubernetes API error', 400
+
+        it_behaves_like 'returns pod_name and container_name'
       end
 
       context 'when kubernetes responds with 404s' do
@@ -204,14 +217,18 @@ describe Clusters::Platforms::Kubernetes do
         end
 
         it_behaves_like 'resource not found error', 'Pod not found'
+
+        it_behaves_like 'returns pod_name and container_name'
       end
 
       context 'when container name is not specified' do
+        let(:container) { 'container-0' }
+
         subject { service.read_pod_logs(pod_name, namespace) }
 
         before do
           stub_kubeclient_pod_details(pod_name, namespace)
-          stub_kubeclient_logs(pod_name, namespace, container: 'container-0')
+          stub_kubeclient_logs(pod_name, namespace, container: container)
         end
 
         include_examples 'successful log request'

ee/spec/models/environment_spec.rb

@@ -74,7 +74,8 @@ describe Environment, :use_clean_rails_memory_store_caching do
       end
 
       it 'returns the pod_names' do
-        allow(environment).to receive(:rollout_status).and_return(rollout_status)
+        allow(environment).to receive(:rollout_status_with_reactive_cache)
+          .and_return(rollout_status)
 
         expect(environment.pod_names).to eq([pod_name])
       end

ee/spec/services/pod_logs_service_spec.rb

@@ -10,6 +10,8 @@ describe PodLogsService do
     let(:environment) { create(:environment, name: 'production') }
     let(:project) { environment.project }
     let(:pod_name) { 'pod-1' }
+    let(:response_pod_name) { pod_name }
+    let(:pods) { [pod_name] }
     let(:container_name) { 'container-1' }
     let(:logs) { ['Log 1', 'Log 2', 'Log 3'] }
     let(:result) { subject.execute }
@@ -29,6 +31,9 @@ describe PodLogsService do
       it do
         expect(result[:status]).to eq(:success)
         expect(result[:logs]).to eq(logs)
+        expect(result[:pods]).to eq(pods)
+        expect(result[:pod_name]).to eq(response_pod_name)
+        expect(result[:container_name]).to eq(container_name)
       end
     end
 
@@ -39,30 +44,36 @@ describe PodLogsService do
       end
     end
 
-    shared_context 'return error' do |message|
-      before do
-        allow_any_instance_of(EE::Clusters::Platforms::Kubernetes).to receive(:read_pod_logs)
-          .with(pod_name, environment.deployment_namespace, container: container_name)
-          .and_return({ status: :error, error: message })
+    shared_examples 'returns pod_name and container_name' do
+      it do
+        expect(result[:pod_name]).to eq(response_pod_name)
+        expect(result[:container_name]).to eq(container_name)
       end
     end
 
-    shared_context 'return success' do
+    shared_context 'return error' do |message|
       before do
         allow_any_instance_of(EE::Clusters::Platforms::Kubernetes).to receive(:read_pod_logs)
           .with(pod_name, environment.deployment_namespace, container: container_name)
-          .and_return({ status: :success, logs: "Log 1\nLog 2\nLog 3" })
+          .and_return({
+            status: :error,
+            error: message,
+            pod_name: response_pod_name,
+            container_name: container_name
+          })
       end
     end
 
-    shared_context 'deployment platform' do
+    shared_context 'return success' do
       before do
-        create(:cluster, :provided_by_gcp,
-          environment_scope: '*', projects: [project])
-
         allow_any_instance_of(EE::Clusters::Platforms::Kubernetes).to receive(:read_pod_logs)
-          .with(pod_name, environment.deployment_namespace, container: container_name)
-          .and_return(kube_logs_body)
+          .with(response_pod_name, environment.deployment_namespace, container: container_name)
+          .and_return({
+            status: :success,
+            logs: "Log 1\nLog 2\nLog 3",
+            pod_name: response_pod_name,
+            container_name: container_name
+          })
       end
     end
 
@@ -83,12 +94,25 @@ describe PodLogsService do
     end
 
     context 'with deployment platform' do
-      include_context 'deployment platform'
+      let(:rollout_status) do
+        instance_double(::Gitlab::Kubernetes::RolloutStatus, instances: [{ pod_name: response_pod_name }])
+      end
+
+      before do
+        create(:cluster, :provided_by_gcp,
+          environment_scope: '*', projects: [project])
+
+        create(:deployment, :success, environment: environment)
+        allow(environment).to receive(:rollout_status_with_reactive_cache)
+          .and_return(rollout_status)
+      end
 
       context 'when pod does not exist' do
         include_context 'return error', 'Pod not found'
 
         it_behaves_like 'error', 'Pod not found'
+
+        it_behaves_like 'returns pod_name and container_name'
       end
 
       context 'when container_name is specified' do
@@ -118,16 +142,10 @@ describe PodLogsService do
         let(:pod_name) { '' }
         let(:container_name) { nil }
         let(:first_pod_name) { 'some-pod' }
+        let(:pods) { [first_pod_name] }
+        let(:response_pod_name) { first_pod_name }
 
-        before do
-          create(:deployment, :success, environment: environment)
-          allow_any_instance_of(Gitlab::Kubernetes::RolloutStatus).to receive(:instances)
-            .and_return([{ pod_name: first_pod_name }])
-
-          allow_any_instance_of(EE::Clusters::Platforms::Kubernetes).to receive(:read_pod_logs)
-            .with(first_pod_name, environment.deployment_namespace, container: nil)
-            .and_return({ status: :success, logs: "Log 1\nLog 2\nLog 3" })
-        end
+        include_context 'return success'
 
         it_behaves_like 'success'
 
@@ -139,22 +157,32 @@ describe PodLogsService do
         end
 
         context 'when there are no pods' do
-          before do
-            allow_any_instance_of(Gitlab::Kubernetes::RolloutStatus).to receive(:instances)
-              .and_return([])
-          end
+          let(:rollout_status) { instance_double(::Gitlab::Kubernetes::RolloutStatus, instances: []) }
 
           it 'returns error' do
             expect(result[:status]).to eq(:error)
             expect(result[:message]).to eq('No pods available')
           end
         end
+
+        context 'when rollout_status cache is empty' do
+          before do
+            allow(environment).to receive(:rollout_status_with_reactive_cache)
+              .and_return(nil)
+          end
+
+          it 'returns nil' do
+            expect(subject.execute).to eq(status: :processing, last_step: :check_pod_names)
+          end
+        end
       end
 
       context 'when error is returned' do
         include_context 'return error', 'Kubernetes API returned status code: 400'
 
         it_behaves_like 'error', 'Kubernetes API returned status code: 400'
+
+        it_behaves_like 'returns pod_name and container_name'
       end
 
       context 'when nil is returned' do