Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
1
Merge Requests
1
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
nexedi
gitlab-ce
Commits
5aca511a
Commit
5aca511a
authored
3 years ago
by
David Fernandez
Committed by
Bob Van Landuyt
3 years ago
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Remove the container expiration loopless feature flag [RUN ALL RSPEC] [RUN AS-IF-FOSS]
parent
5f7bbca3
No related merge requests found
Changes
7
Show whitespace changes
Inline
Side-by-side
Showing
7 changed files
with
367 additions
and
645 deletions
+367
-645
app/services/container_expiration_policies/cleanup_service.rb
...services/container_expiration_policies/cleanup_service.rb
+0
-1
app/workers/container_expiration_policies/cleanup_container_repository_worker.rb
...xpiration_policies/cleanup_container_repository_worker.rb
+12
-38
app/workers/container_expiration_policy_worker.rb
app/workers/container_expiration_policy_worker.rb
+0
-16
config/feature_flags/development/container_registry_expiration_policies_loopless.yml
...pment/container_registry_expiration_policies_loopless.yml
+0
-8
spec/services/container_expiration_policies/cleanup_service_spec.rb
...ces/container_expiration_policies/cleanup_service_spec.rb
+136
-150
spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb
...tion_policies/cleanup_container_repository_worker_spec.rb
+215
-345
spec/workers/container_expiration_policy_worker_spec.rb
spec/workers/container_expiration_policy_worker_spec.rb
+4
-87
No files found.
app/services/container_expiration_policies/cleanup_service.rb
View file @
5aca511a
...
...
@@ -49,7 +49,6 @@ module ContainerExpirationPolicies
private
def
schedule_next_run_if_needed
return
unless
Feature
.
enabled?
(
:container_registry_expiration_policies_loopless
)
return
if
policy
.
next_run_at
.
future?
repos_before_next_run
=
::
ContainerRepository
.
for_project_id
(
policy
.
project_id
)
...
...
This diff is collapsed.
Click to expand it.
app/workers/container_expiration_policies/cleanup_container_repository_worker.rb
View file @
5aca511a
...
...
@@ -65,19 +65,9 @@ module ContainerExpirationPolicies
def
container_repository
strong_memoize
(
:container_repository
)
do
ContainerRepository
.
transaction
do
# rubocop: disable CodeReuse/ActiveRecord
# We need a lock to prevent two workers from picking up the same row
container_repository
=
if
loopless_enabled?
next_container_repository
else
ContainerRepository
.
waiting_for_cleanup
.
order
(
:expiration_policy_cleanup_status
,
:expiration_policy_started_at
)
.
limit
(
1
)
.
lock
(
'FOR UPDATE SKIP LOCKED'
)
.
first
end
container_repository
=
next_container_repository
# rubocop: enable CodeReuse/ActiveRecord
container_repository
&
.
tap
(
&
:cleanup_ongoing!
)
end
end
...
...
@@ -102,28 +92,20 @@ module ContainerExpirationPolicies
def
cleanup_scheduled_count
strong_memoize
(
:cleanup_scheduled_count
)
do
if
loopless_enabled?
limit
=
max_running_jobs
+
1
ContainerExpirationPolicy
.
with_container_repositories
.
runnable_schedules
.
limit
(
limit
)
.
count
else
ContainerRepository
.
cleanup_scheduled
.
count
end
end
end
def
cleanup_unfinished_count
strong_memoize
(
:cleanup_unfinished_count
)
do
if
loopless_enabled?
limit
=
max_running_jobs
+
1
ContainerRepository
.
with_unfinished_cleanup
.
limit
(
limit
)
.
count
else
ContainerRepository
.
cleanup_unfinished
.
count
end
end
end
...
...
@@ -132,21 +114,13 @@ module ContainerExpirationPolicies
now
=
Time
.
zone
.
now
if
loopless_enabled?
policy
.
next_run_at
<
now
||
(
now
+
max_cleanup_execution_time
.
seconds
<
policy
.
next_run_at
)
else
now
+
max_cleanup_execution_time
.
seconds
<
policy
.
next_run_at
end
end
def
throttling_enabled?
Feature
.
enabled?
(
:container_registry_expiration_policies_throttling
)
end
def
loopless_enabled?
Feature
.
enabled?
(
:container_registry_expiration_policies_loopless
)
end
def
max_cleanup_execution_time
::
Gitlab
::
CurrentSettings
.
container_registry_delete_tags_service_timeout
end
...
...
This diff is collapsed.
Click to expand it.
app/workers/container_expiration_policy_worker.rb
View file @
5aca511a
...
...
@@ -38,18 +38,6 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
def
perform_throttled
try_obtain_lease
do
unless
loopless_enabled?
with_runnable_policy
do
|
policy
|
ContainerExpirationPolicy
.
transaction
do
policy
.
schedule_next_run!
ContainerRepository
.
for_project_id
(
policy
.
id
)
.
each_batch
do
|
relation
|
relation
.
update_all
(
expiration_policy_cleanup_status: :cleanup_scheduled
)
end
end
end
end
ContainerExpirationPolicies
::
CleanupContainerRepositoryWorker
.
perform_with_capacity
end
end
...
...
@@ -86,10 +74,6 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
Feature
.
enabled?
(
:container_registry_expiration_policies_throttling
)
end
def
loopless_enabled?
Feature
.
enabled?
(
:container_registry_expiration_policies_loopless
)
end
def
lease_timeout
5
.
hours
end
...
...
This diff is collapsed.
Click to expand it.
config/feature_flags/development/container_registry_expiration_policies_loopless.yml
deleted
100644 → 0
View file @
5f7bbca3
---
name
:
container_registry_expiration_policies_loopless
introduced_by_url
:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56962
rollout_issue_url
:
https://gitlab.com/gitlab-org/gitlab/-/issues/325273
milestone
:
'
13.11'
type
:
development
group
:
group::package
default_enabled
:
false
This diff is collapsed.
Click to expand it.
spec/services/container_expiration_policies/cleanup_service_spec.rb
View file @
5aca511a
...
...
@@ -9,9 +9,15 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
let
(
:service
)
{
described_class
.
new
(
repository
)
}
describe
'#execute'
do
let
(
:policy
)
{
repository
.
project
.
container_expiration_policy
}
subject
{
service
.
execute
}
shared_examples
'cleaning up a container repository'
do
before
do
policy
.
update!
(
enabled:
true
)
policy
.
update_column
(
:next_run_at
,
5
.
minutes
.
ago
)
end
context
'with a successful cleanup tags service execution'
do
let
(
:cleanup_tags_service_params
)
{
project
.
container_expiration_policy
.
policy_params
.
merge
(
'container_expiration_policy'
=>
true
)
}
let
(
:cleanup_tags_service
)
{
instance_double
(
Projects
::
ContainerRepository
::
CleanupTagsService
)
}
...
...
@@ -132,17 +138,6 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
expect
(
repository
.
expiration_policy_completed_at
).
to
eq
(
nil
)
end
end
end
context
'with loopless enabled'
do
let
(
:policy
)
{
repository
.
project
.
container_expiration_policy
}
before
do
policy
.
update!
(
enabled:
true
)
policy
.
update_column
(
:next_run_at
,
5
.
minutes
.
ago
)
end
it_behaves_like
'cleaning up a container repository'
context
'next run scheduling'
do
let_it_be_with_reload
(
:repository2
)
{
create
(
:container_repository
,
project:
project
)
}
...
...
@@ -213,13 +208,4 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
end
end
end
context
'with loopless disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
false
)
end
it_behaves_like
'cleaning up a container repository'
end
end
end
This diff is collapsed.
Click to expand it.
spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb
View file @
5aca511a
...
...
@@ -85,7 +85,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
context
'with policy running shortly'
do
before
do
repository
.
cleanup_unfinished!
if
loopless_enabled?
repository
.
cleanup_unfinished!
policy
.
update_column
(
:next_run_at
,
1
.
minute
.
from_now
)
end
...
...
@@ -108,22 +108,12 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
it
'skips the repository'
do
expect
(
ContainerExpirationPolicies
::
CleanupService
).
not_to
receive
(
:new
)
if
loopless_enabled?
expect
{
subject
}
.
to
not_change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}
.
and
not_change
{
repository
.
reload
.
expiration_policy_cleanup_status
}
else
expect
{
subject
}.
to
change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}.
from
(
1
).
to
(
0
)
expect
(
repository
.
reload
.
cleanup_unscheduled?
).
to
be_truthy
end
end
end
end
context
'with loopless enabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
true
)
end
context
'with repository in cleanup unscheduled state'
do
before
do
...
...
@@ -375,106 +365,6 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
subject
end
end
end
context
'with loopless disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
false
)
end
context
'with repository in cleanup scheduled state'
do
it_behaves_like
'handling all repository conditions'
end
context
'with repository in cleanup unfinished state'
do
before
do
repository
.
cleanup_unfinished!
end
it_behaves_like
'handling all repository conditions'
end
context
'with another repository in cleanup unfinished state'
do
let_it_be
(
:another_repository
)
{
create
(
:container_repository
,
:cleanup_unfinished
)
}
it
'process the cleanup scheduled repository first'
do
service_response
=
cleanup_service_response
(
repository:
repository
)
expect
(
ContainerExpirationPolicies
::
CleanupService
)
.
to
receive
(
:new
).
with
(
repository
).
and_return
(
double
(
execute:
service_response
))
expect_log_extra_metadata
(
service_response:
service_response
)
subject
end
end
context
'with multiple repositories in cleanup unfinished state'
do
let_it_be
(
:repository2
)
{
create
(
:container_repository
,
:cleanup_unfinished
,
expiration_policy_started_at:
20
.
minutes
.
ago
)
}
let_it_be
(
:repository3
)
{
create
(
:container_repository
,
:cleanup_unfinished
,
expiration_policy_started_at:
10
.
minutes
.
ago
)
}
before
do
repository
.
update!
(
expiration_policy_cleanup_status: :cleanup_unfinished
,
expiration_policy_started_at:
30
.
minutes
.
ago
)
end
it
'process the repository with the oldest expiration_policy_started_at'
do
service_response
=
cleanup_service_response
(
repository:
repository
)
expect
(
ContainerExpirationPolicies
::
CleanupService
)
.
to
receive
(
:new
).
with
(
repository
).
and_return
(
double
(
execute:
service_response
))
expect_log_extra_metadata
(
service_response:
service_response
)
subject
end
end
context
'with repository in cleanup ongoing state'
do
before
do
repository
.
cleanup_ongoing!
end
it
'does not process it'
do
expect
(
Projects
::
ContainerRepository
::
CleanupTagsService
).
not_to
receive
(
:new
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}
expect
(
repository
.
cleanup_ongoing?
).
to
be_truthy
end
end
context
'with no repository in any cleanup state'
do
before
do
repository
.
cleanup_unscheduled!
end
it
'does not process it'
do
expect
(
Projects
::
ContainerRepository
::
CleanupTagsService
).
not_to
receive
(
:new
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}
expect
(
repository
.
cleanup_unscheduled?
).
to
be_truthy
end
end
context
'with no container repository waiting'
do
before
do
repository
.
destroy!
end
it
'does not execute the cleanup tags service'
do
expect
(
Projects
::
ContainerRepository
::
CleanupTagsService
).
not_to
receive
(
:new
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}
end
end
context
'with feature flag disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_throttling:
false
)
end
it
'is a no-op'
do
expect
(
Projects
::
ContainerRepository
::
CleanupTagsService
).
not_to
receive
(
:new
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
waiting_for_cleanup
.
count
}
end
end
end
def
cleanup_service_response
(
status: :finished
,
repository
:,
cleanup_tags_service_original_size:
100
,
cleanup_tags_service_before_truncate_size:
80
,
cleanup_tags_service_after_truncate_size:
80
,
cleanup_tags_service_before_delete_size:
50
,
cleanup_tags_service_deleted_size:
50
)
ServiceResponse
.
success
(
...
...
@@ -509,9 +399,20 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
end
describe
'#remaining_work_count'
do
let_it_be
(
:disabled_repository
)
{
create
(
:container_repository
,
:cleanup_scheduled
)
}
let
(
:capacity
)
{
10
}
subject
{
worker
.
remaining_work_count
}
shared_examples
'handling all conditions'
do
before
do
stub_application_setting
(
container_registry_expiration_policies_worker_capacity:
capacity
)
ContainerExpirationPolicy
.
update_all
(
enabled:
true
)
repository
.
project
.
container_expiration_policy
.
update_column
(
:next_run_at
,
5
.
minutes
.
ago
)
disabled_repository
.
project
.
container_expiration_policy
.
update_column
(
:enabled
,
false
)
end
context
'with container repositories waiting for cleanup'
do
let_it_be
(
:unfinished_repositories
)
{
create_list
(
:container_repository
,
2
,
:cleanup_unfinished
)
}
...
...
@@ -548,33 +449,6 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
end
end
context
'with loopless enabled'
do
let_it_be
(
:disabled_repository
)
{
create
(
:container_repository
,
:cleanup_scheduled
)
}
let
(
:capacity
)
{
10
}
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
true
)
stub_application_setting
(
container_registry_expiration_policies_worker_capacity:
capacity
)
# loopless mode is more accurate that non loopless: policies need to be enabled
ContainerExpirationPolicy
.
update_all
(
enabled:
true
)
repository
.
project
.
container_expiration_policy
.
update_column
(
:next_run_at
,
5
.
minutes
.
ago
)
disabled_repository
.
project
.
container_expiration_policy
.
update_column
(
:enabled
,
false
)
end
it_behaves_like
'handling all conditions'
end
context
'with loopless disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
false
)
end
it_behaves_like
'handling all conditions'
end
end
describe
'#max_running_jobs'
do
let
(
:capacity
)
{
50
}
...
...
@@ -599,8 +473,4 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
expect
(
worker
.
logger
)
.
to
receive
(
:info
).
with
(
worker
.
structured_payload
(
structure
))
end
def
loopless_enabled?
Feature
.
enabled?
(
:container_registry_expiration_policies_loopless
)
end
end
This diff is collapsed.
Click to expand it.
spec/workers/container_expiration_policy_worker_spec.rb
View file @
5aca511a
...
...
@@ -34,93 +34,11 @@ RSpec.describe ContainerExpirationPolicyWorker do
end
end
context
'With no container expiration policies'
do
context
'with loopless disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
false
)
end
it
'does not execute any policies'
do
expect
(
ContainerRepository
).
not_to
receive
(
:for_project_id
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
cleanup_scheduled
.
count
}
end
end
end
context
'with throttling enabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_throttling:
true
)
end
context
'with loopless disabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
false
)
end
context
'with container expiration policies'
do
let_it_be
(
:container_expiration_policy
)
{
create
(
:container_expiration_policy
,
:runnable
)
}
let_it_be
(
:container_repository
)
{
create
(
:container_repository
,
project:
container_expiration_policy
.
project
)
}
before
do
expect
(
worker
).
to
receive
(
:with_runnable_policy
).
and_call_original
end
context
'with a valid container expiration policy'
do
it
'schedules the next run'
do
expect
{
subject
}.
to
change
{
container_expiration_policy
.
reload
.
next_run_at
}
end
it
'marks the container repository as scheduled for cleanup'
do
expect
{
subject
}.
to
change
{
container_repository
.
reload
.
cleanup_scheduled?
}.
from
(
false
).
to
(
true
)
expect
(
ContainerRepository
.
cleanup_scheduled
.
count
).
to
eq
(
1
)
end
it
'calls the limited capacity worker'
do
expect
(
ContainerExpirationPolicies
::
CleanupContainerRepositoryWorker
).
to
receive
(
:perform_with_capacity
)
subject
end
end
context
'with a disabled container expiration policy'
do
before
do
container_expiration_policy
.
disable!
end
it
'does not run the policy'
do
expect
(
ContainerRepository
).
not_to
receive
(
:for_project_id
)
expect
{
subject
}.
not_to
change
{
ContainerRepository
.
cleanup_scheduled
.
count
}
end
end
context
'with an invalid container expiration policy'
do
let
(
:user
)
{
container_expiration_policy
.
project
.
owner
}
before
do
container_expiration_policy
.
update_column
(
:name_regex
,
'*production'
)
end
it
'disables the policy and tracks an error'
do
expect
(
ContainerRepository
).
not_to
receive
(
:for_project_id
)
expect
(
Gitlab
::
ErrorTracking
).
to
receive
(
:log_exception
).
with
(
instance_of
(
described_class
::
InvalidPolicyError
),
container_expiration_policy_id:
container_expiration_policy
.
id
)
expect
{
subject
}.
to
change
{
container_expiration_policy
.
reload
.
enabled
}.
from
(
true
).
to
(
false
)
expect
(
ContainerRepository
.
cleanup_scheduled
).
to
be_empty
end
end
end
it_behaves_like
'handling a taken exclusive lease'
end
context
'with loopless enabled'
do
before
do
stub_feature_flags
(
container_registry_expiration_policies_loopless:
true
)
expect
(
worker
).
not_to
receive
(
:with_runnable_policy
)
end
it
'calls the limited capacity worker'
do
expect
(
ContainerExpirationPolicies
::
CleanupContainerRepositoryWorker
).
to
receive
(
:perform_with_capacity
)
...
...
@@ -129,7 +47,6 @@ RSpec.describe ContainerExpirationPolicyWorker do
it_behaves_like
'handling a taken exclusive lease'
end
end
context
'with throttling disabled'
do
before
do
...
...
This diff is collapsed.
Click to expand it.
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment