From 4226aca420920c1844e8eade4798a2dff188a6fc Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 17 Oct 2019 09:07:07 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .gitlab/ci/docs.gitlab-ci.yml | 1 + ...n-abilities-when-issue-moved-or-locked.yml | 5 + .../geo/replication/object_storage.md | 2 +- doc/user/project/index.md | 20 ++ doc/user/project/issues/design_management.md | 3 + lib/gitlab/sidekiq_daemon/memory_killer.rb | 58 +++++- scripts/review_apps/base-config.yaml | 12 +- .../mutations/resolves_project_spec.rb | 2 + .../mutations/merge_requests/set_wip_spec.rb | 2 + .../concerns/resolves_pipelines_spec.rb | 2 + .../graphql/resolvers/issues_resolver_spec.rb | 2 + .../merge_request_pipelines_resolver_spec.rb | 2 + .../resolvers/merge_requests_resolver_spec.rb | 2 + .../resolvers/metadata_resolver_spec.rb | 2 + .../project_pipelines_resolver_spec.rb | 2 + .../resolvers/project_resolver_spec.rb | 2 + spec/graphql/resolvers/tree_resolver_spec.rb | 2 + .../types/ci/detailed_status_type_spec.rb | 2 + spec/graphql/types/ci/pipeline_type_spec.rb | 2 + .../graphql/types/extended_issue_type_spec.rb | 2 + spec/graphql/types/issue_type_spec.rb | 2 + spec/graphql/types/merge_request_type_spec.rb | 2 + spec/graphql/types/metadata_type_spec.rb | 2 + .../base_permission_type_spec.rb | 2 + .../types/permission_types/issue_spec.rb | 2 + .../permission_types/merge_request_spec.rb | 2 + .../merge_request_type_spec.rb | 2 + .../types/permission_types/note_spec.rb | 2 + .../types/permission_types/project_spec.rb | 2 + spec/graphql/types/project_type_spec.rb | 2 + spec/graphql/types/query_type_spec.rb | 2 + spec/graphql/types/repository_type_spec.rb | 2 + spec/graphql/types/time_type_spec.rb | 2 + .../sidekiq_daemon/memory_killer_spec.rb | 173 ++++++++++++++---- 34 files changed, 273 insertions(+), 53 deletions(-) create mode 100644 changelogs/unreleased/13426-disable-design-mutation-abilities-when-issue-moved-or-locked.yml diff --git a/.gitlab/ci/docs.gitlab-ci.yml b/.gitlab/ci/docs.gitlab-ci.yml index cc0999b50c6..14eeebb9db9 100644 --- a/.gitlab/ci/docs.gitlab-ci.yml +++ b/.gitlab/ci/docs.gitlab-ci.yml @@ -70,6 +70,7 @@ docs lint: graphql-docs-verify: extends: + - .only-ee - .default-tags - .default-retry - .default-cache diff --git a/changelogs/unreleased/13426-disable-design-mutation-abilities-when-issue-moved-or-locked.yml b/changelogs/unreleased/13426-disable-design-mutation-abilities-when-issue-moved-or-locked.yml new file mode 100644 index 00000000000..21c2dbff7e9 --- /dev/null +++ b/changelogs/unreleased/13426-disable-design-mutation-abilities-when-issue-moved-or-locked.yml @@ -0,0 +1,5 @@ +--- +title: Make designs read-only if the issue has been moved, or if its discussion has been locked +merge_request: 18551 +author: +type: changed diff --git a/doc/administration/geo/replication/object_storage.md b/doc/administration/geo/replication/object_storage.md index ad95255f2a9..a9087abcbd9 100644 --- a/doc/administration/geo/replication/object_storage.md +++ b/doc/administration/geo/replication/object_storage.md @@ -14,7 +14,7 @@ To have: ## Enabling GitLab managed object storage replication -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/10586) in GitLab 12.4. +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/10586) in GitLab 12.4. CAUTION: **Caution:** This is a [**beta** feature](https://about.gitlab.com/handbook/product/#beta) and is not ready yet for production use at any scale. diff --git a/doc/user/project/index.md b/doc/user/project/index.md index 2e17c89a1f6..7ae288996da 100644 --- a/doc/user/project/index.md +++ b/doc/user/project/index.md @@ -155,6 +155,26 @@ when a project is part of a group (under a If you choose to leave a project you will no longer be a project member, therefore, unable to contribute. +## Project's landing page + +The project's landing page shows different information depending on +the project's visibility settings and user permissions. + +For public projects, and to members of internal and private projects +with [permissions to view the project's code](../permissions.md#project-members-permissions): + +- The content of a + [`README` or an index file](repository/#repository-readme-and-index-files) + is displayed (if any), followed by the list of directories within the + project's repository. +- If the project doesn't contain either of these files, the + visitor will see the list of files and directories of the repository. + +For users without permissions to view the project's code: + +- The wiki homepage is displayed, if any. +- The list of issues within the project is displayed. + ## Redirects when changing repository paths When a repository path changes, it is essential to smoothly transition from the diff --git a/doc/user/project/issues/design_management.md b/doc/user/project/issues/design_management.md index 0e1f827d781..169da7049a6 100644 --- a/doc/user/project/issues/design_management.md +++ b/doc/user/project/issues/design_management.md @@ -63,6 +63,9 @@ To upload design images, click the **Upload Designs** button and select images t Designs with the same filename as an existing uploaded design will create a new version of the design, and will replace the previous version. +Designs cannot be added if the issue has been moved, or its +[discussion is locked](../../discussions/#lock-discussions). + ## Viewing designs Images on the Design Management page can be enlarged by clicking on them. diff --git a/lib/gitlab/sidekiq_daemon/memory_killer.rb b/lib/gitlab/sidekiq_daemon/memory_killer.rb index 804170169e8..9d0d67a488f 100644 --- a/lib/gitlab/sidekiq_daemon/memory_killer.rb +++ b/lib/gitlab/sidekiq_daemon/memory_killer.rb @@ -21,14 +21,46 @@ module Gitlab # In case not set, default to 300M. This is for extra-safe. DEFAULT_MAX_MEMORY_GROWTH_KB = 300_000 + # Phases of memory killer + PHASE = { + running: 1, + above_soft_limit: 2, + stop_fetching_new_jobs: 3, + shutting_down: 4, + killing_sidekiq: 5 + }.freeze + def initialize super @enabled = true + @metrics = init_metrics end private + def init_metrics + { + sidekiq_current_rss: ::Gitlab::Metrics.gauge(:sidekiq_current_rss, 'Current RSS of Sidekiq Worker'), + sidekiq_memory_killer_soft_limit_rss: ::Gitlab::Metrics.gauge(:sidekiq_memory_killer_soft_limit_rss, 'Current soft_limit_rss of Sidekiq Worker'), + sidekiq_memory_killer_hard_limit_rss: ::Gitlab::Metrics.gauge(:sidekiq_memory_killer_hard_limit_rss, 'Current hard_limit_rss of Sidekiq Worker'), + sidekiq_memory_killer_phase: ::Gitlab::Metrics.gauge(:sidekiq_memory_killer_phase, 'Current phase of Sidekiq Worker') + } + end + + def refresh_state(phase) + @phase = PHASE.fetch(phase) + @current_rss = get_rss + @soft_limit_rss = get_soft_limit_rss + @hard_limit_rss = get_hard_limit_rss + + # track the current state as prometheus gauges + @metrics[:sidekiq_memory_killer_phase].set({}, @phase) + @metrics[:sidekiq_current_rss].set({}, @current_rss) + @metrics[:sidekiq_memory_killer_soft_limit_rss].set({}, @soft_limit_rss) + @metrics[:sidekiq_memory_killer_hard_limit_rss].set({}, @hard_limit_rss) + end + def run_thread Sidekiq.logger.info( class: self.class.to_s, @@ -77,41 +109,51 @@ module Gitlab # Tell Sidekiq to stop fetching new jobs # We first SIGNAL and then wait given time # We also monitor a number of running jobs and allow to restart early + refresh_state(:stop_fetching_new_jobs) signal_and_wait(SHUTDOWN_TIMEOUT_SECONDS, 'SIGTSTP', 'stop fetching new jobs') return unless enabled? # Tell sidekiq to restart itself # Keep extra safe to wait `Sidekiq.options[:timeout] + 2` seconds before SIGKILL + refresh_state(:shutting_down) signal_and_wait(Sidekiq.options[:timeout] + 2, 'SIGTERM', 'gracefully shut down') return unless enabled? # Ideally we should never reach this condition # Wait for Sidekiq to shutdown gracefully, and kill it if it didn't # Kill the whole pgroup, so we can be sure no children are left behind + refresh_state(:killing_sidekiq) signal_pgroup('SIGKILL', 'die') end def rss_within_range? - current_rss = nil + refresh_state(:running) + deadline = Gitlab::Metrics::System.monotonic_time + GRACE_BALLOON_SECONDS.seconds loop do return true unless enabled? - current_rss = get_rss - # RSS go above hard limit should trigger forcible shutdown right away - break if current_rss > hard_limit_rss + break if @current_rss > @hard_limit_rss # RSS go below the soft limit - return true if current_rss < soft_limit_rss + return true if @current_rss < @soft_limit_rss # RSS did not go below the soft limit within deadline, restart break if Gitlab::Metrics::System.monotonic_time > deadline sleep(CHECK_INTERVAL_SECONDS) + + refresh_state(:above_soft_limit) end - log_rss_out_of_range(current_rss, hard_limit_rss, soft_limit_rss) + # There are two chances to break from loop: + # - above hard limit, or + # - above soft limit after deadline + # When `above hard limit`, it immediately go to `stop_fetching_new_jobs` + # So ignore `above hard limit` and always set `above_soft_limit` here + refresh_state(:above_soft_limit) + log_rss_out_of_range(@current_rss, @hard_limit_rss, @soft_limit_rss) false end @@ -143,11 +185,11 @@ module Gitlab output.to_i end - def soft_limit_rss + def get_soft_limit_rss SOFT_LIMIT_RSS_KB + rss_increase_by_jobs end - def hard_limit_rss + def get_hard_limit_rss HARD_LIMIT_RSS_KB end diff --git a/scripts/review_apps/base-config.yaml b/scripts/review_apps/base-config.yaml index 1dc79e6d45e..3ccf2d62ae4 100644 --- a/scripts/review_apps/base-config.yaml +++ b/scripts/review_apps/base-config.yaml @@ -35,10 +35,10 @@ gitlab: gitlab-shell: resources: requests: - cpu: 70m + cpu: 125m memory: 20M limits: - cpu: 140m + cpu: 250m memory: 40M maxReplicas: 3 hpa: @@ -75,10 +75,10 @@ gitlab: workhorse: resources: requests: - cpu: 100m + cpu: 175m memory: 100M limits: - cpu: 200m + cpu: 350m memory: 200M readinessProbe: initialDelaySeconds: 5 # Default is 0 @@ -87,10 +87,10 @@ gitlab: gitlab-runner: resources: requests: - cpu: 300m + cpu: 355m memory: 300M limits: - cpu: 600m + cpu: 710m memory: 600M minio: resources: diff --git a/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb b/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb index aa0f5c55902..09d1f66a2c7 100644 --- a/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb +++ b/spec/graphql/mutations/concerns/mutations/resolves_project_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Mutations::ResolvesProject do diff --git a/spec/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/graphql/mutations/merge_requests/set_wip_spec.rb index e600abf3941..c4accab9e46 100644 --- a/spec/graphql/mutations/merge_requests/set_wip_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_wip_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Mutations::MergeRequests::SetWip do diff --git a/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb b/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb index 3140af27af5..fa031af4013 100644 --- a/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb +++ b/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe ResolvesPipelines do diff --git a/spec/graphql/resolvers/issues_resolver_spec.rb b/spec/graphql/resolvers/issues_resolver_spec.rb index d122c081069..2232c9b7d7b 100644 --- a/spec/graphql/resolvers/issues_resolver_spec.rb +++ b/spec/graphql/resolvers/issues_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::IssuesResolver do diff --git a/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb b/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb index 09b17bf6fc9..b8bdfc36ba7 100644 --- a/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::MergeRequestPipelinesResolver do diff --git a/spec/graphql/resolvers/merge_requests_resolver_spec.rb b/spec/graphql/resolvers/merge_requests_resolver_spec.rb index 97b8e5ed41c..fe167a6ae3e 100644 --- a/spec/graphql/resolvers/merge_requests_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_requests_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::MergeRequestsResolver do diff --git a/spec/graphql/resolvers/metadata_resolver_spec.rb b/spec/graphql/resolvers/metadata_resolver_spec.rb index e662ed127a5..afff9eabb97 100644 --- a/spec/graphql/resolvers/metadata_resolver_spec.rb +++ b/spec/graphql/resolvers/metadata_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::MetadataResolver do diff --git a/spec/graphql/resolvers/project_pipelines_resolver_spec.rb b/spec/graphql/resolvers/project_pipelines_resolver_spec.rb index 6862ae8a5ed..f312a118c96 100644 --- a/spec/graphql/resolvers/project_pipelines_resolver_spec.rb +++ b/spec/graphql/resolvers/project_pipelines_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::ProjectPipelinesResolver do diff --git a/spec/graphql/resolvers/project_resolver_spec.rb b/spec/graphql/resolvers/project_resolver_spec.rb index d0fc2957909..860f8b4abb8 100644 --- a/spec/graphql/resolvers/project_resolver_spec.rb +++ b/spec/graphql/resolvers/project_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::ProjectResolver do diff --git a/spec/graphql/resolvers/tree_resolver_spec.rb b/spec/graphql/resolvers/tree_resolver_spec.rb index 9f95b740ab1..0ea4e6eeaad 100644 --- a/spec/graphql/resolvers/tree_resolver_spec.rb +++ b/spec/graphql/resolvers/tree_resolver_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Resolvers::TreeResolver do diff --git a/spec/graphql/types/ci/detailed_status_type_spec.rb b/spec/graphql/types/ci/detailed_status_type_spec.rb index a21162adb42..169a03c770b 100644 --- a/spec/graphql/types/ci/detailed_status_type_spec.rb +++ b/spec/graphql/types/ci/detailed_status_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::Ci::DetailedStatusType do diff --git a/spec/graphql/types/ci/pipeline_type_spec.rb b/spec/graphql/types/ci/pipeline_type_spec.rb index ec1c689a4be..2fafc1bc13f 100644 --- a/spec/graphql/types/ci/pipeline_type_spec.rb +++ b/spec/graphql/types/ci/pipeline_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::Ci::PipelineType do diff --git a/spec/graphql/types/extended_issue_type_spec.rb b/spec/graphql/types/extended_issue_type_spec.rb index 047f4a90d54..72ce53ae1be 100644 --- a/spec/graphql/types/extended_issue_type_spec.rb +++ b/spec/graphql/types/extended_issue_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['ExtendedIssue'] do diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb index 59c42aa446d..8aa2385ddaa 100644 --- a/spec/graphql/types/issue_type_spec.rb +++ b/spec/graphql/types/issue_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Issue'] do diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb index 59bd0123d88..6904c2dcd07 100644 --- a/spec/graphql/types/merge_request_type_spec.rb +++ b/spec/graphql/types/merge_request_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['MergeRequest'] do diff --git a/spec/graphql/types/metadata_type_spec.rb b/spec/graphql/types/metadata_type_spec.rb index 5236380e477..2988f3c6ba5 100644 --- a/spec/graphql/types/metadata_type_spec.rb +++ b/spec/graphql/types/metadata_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Metadata'] do diff --git a/spec/graphql/types/permission_types/base_permission_type_spec.rb b/spec/graphql/types/permission_types/base_permission_type_spec.rb index 0ee8b883d51..a45102e5b50 100644 --- a/spec/graphql/types/permission_types/base_permission_type_spec.rb +++ b/spec/graphql/types/permission_types/base_permission_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::PermissionTypes::BasePermissionType do diff --git a/spec/graphql/types/permission_types/issue_spec.rb b/spec/graphql/types/permission_types/issue_spec.rb index f0fbeda202f..a94bc6b780e 100644 --- a/spec/graphql/types/permission_types/issue_spec.rb +++ b/spec/graphql/types/permission_types/issue_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::PermissionTypes::Issue do diff --git a/spec/graphql/types/permission_types/merge_request_spec.rb b/spec/graphql/types/permission_types/merge_request_spec.rb index e1026b01a74..e0f8bdd4712 100644 --- a/spec/graphql/types/permission_types/merge_request_spec.rb +++ b/spec/graphql/types/permission_types/merge_request_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::PermissionTypes::MergeRequest do diff --git a/spec/graphql/types/permission_types/merge_request_type_spec.rb b/spec/graphql/types/permission_types/merge_request_type_spec.rb index 6e57122867a..572b4ac42d0 100644 --- a/spec/graphql/types/permission_types/merge_request_type_spec.rb +++ b/spec/graphql/types/permission_types/merge_request_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::MergeRequestType do diff --git a/spec/graphql/types/permission_types/note_spec.rb b/spec/graphql/types/permission_types/note_spec.rb index 32d56eb1f7a..a7811fc20e5 100644 --- a/spec/graphql/types/permission_types/note_spec.rb +++ b/spec/graphql/types/permission_types/note_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['NotePermissions'] do diff --git a/spec/graphql/types/permission_types/project_spec.rb b/spec/graphql/types/permission_types/project_spec.rb index 4974995b587..6d5a905c128 100644 --- a/spec/graphql/types/permission_types/project_spec.rb +++ b/spec/graphql/types/permission_types/project_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Types::PermissionTypes::Project do diff --git a/spec/graphql/types/project_type_spec.rb b/spec/graphql/types/project_type_spec.rb index 739e48044d1..f632b904e78 100644 --- a/spec/graphql/types/project_type_spec.rb +++ b/spec/graphql/types/project_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Project'] do diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index bc3b8a42392..784a4f4b4c9 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Query'] do diff --git a/spec/graphql/types/repository_type_spec.rb b/spec/graphql/types/repository_type_spec.rb index 8a8238f2a2a..236f9bb9459 100644 --- a/spec/graphql/types/repository_type_spec.rb +++ b/spec/graphql/types/repository_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Repository'] do diff --git a/spec/graphql/types/time_type_spec.rb b/spec/graphql/types/time_type_spec.rb index 4196d9d27d4..88a535ed3bb 100644 --- a/spec/graphql/types/time_type_spec.rb +++ b/spec/graphql/types/time_type_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema.types['Time'] do diff --git a/spec/lib/gitlab/sidekiq_daemon/memory_killer_spec.rb b/spec/lib/gitlab/sidekiq_daemon/memory_killer_spec.rb index adea2ba30b3..45bcc71dfcb 100644 --- a/spec/lib/gitlab/sidekiq_daemon/memory_killer_spec.rb +++ b/spec/lib/gitlab/sidekiq_daemon/memory_killer_spec.rb @@ -7,9 +7,12 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do let(:pid) { 12345 } before do - allow(memory_killer).to receive(:pid).and_return(pid) allow(Sidekiq.logger).to receive(:info) allow(Sidekiq.logger).to receive(:warn) + allow(memory_killer).to receive(:pid).and_return(pid) + + # make sleep no-op + allow(memory_killer).to receive(:sleep) {} end describe '#run_thread' do @@ -39,8 +42,9 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do pid: pid, message: "Exception from run_thread: My Exception") - expect(memory_killer).to receive(:rss_within_range?).twice.and_raise(StandardError, 'My Exception') - expect(memory_killer).to receive(:sleep).twice.with(Gitlab::SidekiqDaemon::MemoryKiller::CHECK_INTERVAL_SECONDS) + expect(memory_killer).to receive(:rss_within_range?) + .twice + .and_raise(StandardError, 'My Exception') expect { subject }.not_to raise_exception end @@ -52,7 +56,9 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do pid: pid, message: "Exception from run_thread: My Exception") - expect(memory_killer).to receive(:rss_within_range?).once.and_raise(Exception, 'My Exception') + expect(memory_killer).to receive(:rss_within_range?) + .once + .and_raise(Exception, 'My Exception') expect(memory_killer).to receive(:sleep).with(Gitlab::SidekiqDaemon::MemoryKiller::CHECK_INTERVAL_SECONDS) expect(Sidekiq.logger).to receive(:warn).once @@ -78,7 +84,9 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do end it 'not invoke restart_sidekiq when rss in range' do - expect(memory_killer).to receive(:rss_within_range?).twice.and_return(true) + expect(memory_killer).to receive(:rss_within_range?) + .twice + .and_return(true) expect(memory_killer).not_to receive(:restart_sidekiq) @@ -86,9 +94,12 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do end it 'invoke restart_sidekiq when rss not in range' do - expect(memory_killer).to receive(:rss_within_range?).at_least(:once).and_return(false) + expect(memory_killer).to receive(:rss_within_range?) + .at_least(:once) + .and_return(false) - expect(memory_killer).to receive(:restart_sidekiq).at_least(:once) + expect(memory_killer).to receive(:restart_sidekiq) + .at_least(:once) subject end @@ -97,10 +108,9 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do describe '#stop_working' do subject { memory_killer.send(:stop_working)} - it 'changed enable? to false' do - expect(memory_killer.send(:enabled?)).to be true - subject - expect(memory_killer.send(:enabled?)).to be false + it 'changes enable? to false' do + expect { subject }.to change { memory_killer.send(:enabled?) } + .from(true).to(false) end end @@ -121,8 +131,12 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do it 'return true when everything is within limit' do expect(memory_killer).to receive(:get_rss).and_return(100) - expect(memory_killer).to receive(:soft_limit_rss).and_return(200) - expect(memory_killer).to receive(:hard_limit_rss).and_return(300) + expect(memory_killer).to receive(:get_soft_limit_rss).and_return(200) + expect(memory_killer).to receive(:get_hard_limit_rss).and_return(300) + + expect(memory_killer).to receive(:refresh_state) + .with(:running) + .and_call_original expect(Gitlab::Metrics::System).to receive(:monotonic_time).and_call_original expect(memory_killer).not_to receive(:log_rss_out_of_range) @@ -131,9 +145,17 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do end it 'return false when rss exceeds hard_limit_rss' do - expect(memory_killer).to receive(:get_rss).and_return(400) - expect(memory_killer).to receive(:soft_limit_rss).at_least(:once).and_return(200) - expect(memory_killer).to receive(:hard_limit_rss).at_least(:once).and_return(300) + expect(memory_killer).to receive(:get_rss).at_least(:once).and_return(400) + expect(memory_killer).to receive(:get_soft_limit_rss).at_least(:once).and_return(200) + expect(memory_killer).to receive(:get_hard_limit_rss).at_least(:once).and_return(300) + + expect(memory_killer).to receive(:refresh_state) + .with(:running) + .and_call_original + + expect(memory_killer).to receive(:refresh_state) + .with(:above_soft_limit) + .and_call_original expect(Gitlab::Metrics::System).to receive(:monotonic_time).and_call_original @@ -143,13 +165,21 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do end it 'return false when rss exceed hard_limit_rss after a while' do - expect(memory_killer).to receive(:get_rss).and_return(250, 400) - expect(memory_killer).to receive(:soft_limit_rss).at_least(:once).and_return(200) - expect(memory_killer).to receive(:hard_limit_rss).at_least(:once).and_return(300) + expect(memory_killer).to receive(:get_rss).and_return(250, 400, 400) + expect(memory_killer).to receive(:get_soft_limit_rss).at_least(:once).and_return(200) + expect(memory_killer).to receive(:get_hard_limit_rss).at_least(:once).and_return(300) + + expect(memory_killer).to receive(:refresh_state) + .with(:running) + .and_call_original + + expect(memory_killer).to receive(:refresh_state) + .at_least(:once) + .with(:above_soft_limit) + .and_call_original expect(Gitlab::Metrics::System).to receive(:monotonic_time).twice.and_call_original expect(memory_killer).to receive(:sleep).with(check_interval_seconds) - expect(memory_killer).to receive(:log_rss_out_of_range).with(400, 300, 200) expect(subject).to be false @@ -157,8 +187,16 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do it 'return true when rss below soft_limit_rss after a while within GRACE_BALLOON_SECONDS' do expect(memory_killer).to receive(:get_rss).and_return(250, 100) - expect(memory_killer).to receive(:soft_limit_rss).and_return(200, 200) - expect(memory_killer).to receive(:hard_limit_rss).and_return(300, 300) + expect(memory_killer).to receive(:get_soft_limit_rss).and_return(200, 200) + expect(memory_killer).to receive(:get_hard_limit_rss).and_return(300, 300) + + expect(memory_killer).to receive(:refresh_state) + .with(:running) + .and_call_original + + expect(memory_killer).to receive(:refresh_state) + .with(:above_soft_limit) + .and_call_original expect(Gitlab::Metrics::System).to receive(:monotonic_time).twice.and_call_original expect(memory_killer).to receive(:sleep).with(check_interval_seconds) @@ -168,17 +206,27 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do expect(subject).to be true end - it 'return false when rss exceed soft_limit_rss longer than GRACE_BALLOON_SECONDS' do - expect(memory_killer).to receive(:get_rss).exactly(4).times.and_return(250) - expect(memory_killer).to receive(:soft_limit_rss).exactly(5).times.and_return(200) - expect(memory_killer).to receive(:hard_limit_rss).exactly(5).times.and_return(300) + context 'when exceeding GRACE_BALLOON_SECONDS' do + let(:grace_balloon_seconds) { 0 } - expect(Gitlab::Metrics::System).to receive(:monotonic_time).exactly(5).times.and_call_original - expect(memory_killer).to receive(:sleep).exactly(3).times.with(check_interval_seconds).and_call_original + it 'return false when rss exceed soft_limit_rss' do + allow(memory_killer).to receive(:get_rss).and_return(250) + allow(memory_killer).to receive(:get_soft_limit_rss).and_return(200) + allow(memory_killer).to receive(:get_hard_limit_rss).and_return(300) - expect(memory_killer).to receive(:log_rss_out_of_range).with(250, 300, 200) + expect(memory_killer).to receive(:refresh_state) + .with(:running) + .and_call_original - expect(subject).to be false + expect(memory_killer).to receive(:refresh_state) + .with(:above_soft_limit) + .and_call_original + + expect(memory_killer).to receive(:log_rss_out_of_range) + .with(250, 300, 200) + + expect(subject).to be false + end end end @@ -190,19 +238,42 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do before do stub_const("#{described_class}::SHUTDOWN_TIMEOUT_SECONDS", shutdown_timeout_seconds) allow(Sidekiq).to receive(:options).and_return(timeout: 9) + allow(memory_killer).to receive(:get_rss).and_return(100) + allow(memory_killer).to receive(:get_soft_limit_rss).and_return(200) + allow(memory_killer).to receive(:get_hard_limit_rss).and_return(300) end it 'send signal' do - expect(memory_killer).to receive(:signal_and_wait).with(shutdown_timeout_seconds, 'SIGTSTP', 'stop fetching new jobs').ordered - expect(memory_killer).to receive(:signal_and_wait).with(11, 'SIGTERM', 'gracefully shut down').ordered - expect(memory_killer).to receive(:signal_pgroup).with('SIGKILL', 'die').ordered + expect(memory_killer).to receive(:refresh_state) + .with(:stop_fetching_new_jobs) + .ordered + .and_call_original + expect(memory_killer).to receive(:signal_and_wait) + .with(shutdown_timeout_seconds, 'SIGTSTP', 'stop fetching new jobs') + .ordered + + expect(memory_killer).to receive(:refresh_state) + .with(:shutting_down) + .ordered + .and_call_original + expect(memory_killer).to receive(:signal_and_wait) + .with(11, 'SIGTERM', 'gracefully shut down') + .ordered + + expect(memory_killer).to receive(:refresh_state) + .with(:killing_sidekiq) + .ordered + .and_call_original + expect(memory_killer).to receive(:signal_pgroup) + .with('SIGKILL', 'die') + .ordered subject end end describe '#signal_and_wait' do - let(:time) { 7 } + let(:time) { 0 } let(:signal) { 'my-signal' } let(:explanation) { 'my-explanation' } let(:check_interval_seconds) { 2 } @@ -226,14 +297,17 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do end it 'send signal and wait till deadline if any job not finished' do - expect(Process).to receive(:kill).with(signal, pid).ordered - expect(Gitlab::Metrics::System).to receive(:monotonic_time).and_call_original.at_least(:once) + expect(Process).to receive(:kill) + .with(signal, pid) + .ordered + + expect(Gitlab::Metrics::System).to receive(:monotonic_time) + .and_call_original + .at_least(:once) expect(memory_killer).to receive(:enabled?).and_return(true).at_least(:once) expect(memory_killer).to receive(:any_jobs?).and_return(true).at_least(:once) - expect(memory_killer).to receive(:sleep).and_call_original.exactly(4).times - subject end end @@ -401,4 +475,27 @@ describe Gitlab::SidekiqDaemon::MemoryKiller do expect(subject).to eq(10) end end + + describe '#refresh_state' do + let(:metrics) { memory_killer.instance_variable_get(:@metrics) } + + subject { memory_killer.send(:refresh_state, :shutting_down) } + + it 'calls gitlab metrics gauge set methods' do + expect(memory_killer).to receive(:get_rss) { 1010 } + expect(memory_killer).to receive(:get_soft_limit_rss) { 1020 } + expect(memory_killer).to receive(:get_hard_limit_rss) { 1040 } + + expect(metrics[:sidekiq_memory_killer_phase]).to receive(:set) + .with({}, described_class::PHASE[:shutting_down]) + expect(metrics[:sidekiq_current_rss]).to receive(:set) + .with({}, 1010) + expect(metrics[:sidekiq_memory_killer_soft_limit_rss]).to receive(:set) + .with({}, 1020) + expect(metrics[:sidekiq_memory_killer_hard_limit_rss]).to receive(:set) + .with({}, 1040) + + subject + end + end end -- GitLab