Add async recursive delete for apps and service instances - #5299
Conversation
1539023 to
a87ea6c
Compare
Behind `temporary_enable_async_recursive_delete flag` (default off). Recursive delete jobs re-enqueue instead of failing when sub-resources (service bindings) are deleted asynchronously, waiting for them to settle and surfacing the original broker error on failure. Adds root/sub job tracking via `root_job_guid` on the jobs table. Foundation for future recursive deletes (org, space).
a87ea6c to
05ca1aa
Compare
|
|
||
| result | ||
| rescue SubResourceError => e | ||
| raise if !@fail_if_in_progress && e.any_in_progress? # re-raise SubResourceError so that root job continues to run |
There was a problem hiding this comment.
At this point @fail_if_in_progress will always be false.
| resource_model: AppModel, | ||
| resource_guid: app.guid, | ||
| operation: 'app.delete' | ||
| ) { VCAP::CloudController::V3::RecursiveDeleteAppJob.new(app.guid, user_audit_info) } |
There was a problem hiding this comment.
Use do |locked_instance| for symmetry.
| end | ||
|
|
||
| def raise_if_sub_jobs_failed | ||
| return if sub_job_errors.empty? |
There was a problem hiding this comment.
Constructs sub_job_errors only to check if it is empty or not. Should do something like sub_jobs.any? { |s| s.state == PollableJobModel::FAILED_STATE } instead.
| def sub_jobs_in_flight? | ||
| return false if active_sub_jobs.empty? | ||
|
|
||
| add_in_progress_warning(root_job) |
There was a problem hiding this comment.
Somewhat unexpected that this predicate method has a side-effect (the warning).
| AppStop.stop(app: app, user_audit_info: @user_audit_info, delete_triggered: true) if app.desired_state != ProcessModel::STOPPED | ||
| AppDelete.new(@user_audit_info).delete([app]) | ||
| finish | ||
| end |
There was a problem hiding this comment.
Instead of log_failed_bindings (and log_failed_children in RecursiveDeleteServiceInstanceJob):
rescue CloudController::Errors::CompoundError => e
// log errors from the CompoundError
raise
Maybe this could even go into the RootJobMixin.
| end | ||
|
|
||
| def any_in_progress? | ||
| in_progress_operations.any? |
There was a problem hiding this comment.
@errors.any? { |e| e.is_a?(AsyncOperationInProgress) } is slightly more efficient.
| end | ||
|
|
||
| def with_root_job_guid_set | ||
| return yield if @root_job_guid.nil? |
There was a problem hiding this comment.
Does this make sense? This would mean that there could be logs with an old root_job_guid, or?
| drop_column :jobs, :root_job_guid, if_exists: true | ||
| end | ||
|
|
||
| if database_type == :mysql |
| update_last_operation_with_failure(e.message) unless service_instance.operation_in_progress? | ||
| raise e | ||
| rescue StandardError => e | ||
| update_last_operation_with_failure(e.message) unless service_instance.operation_in_progress? | ||
| raise e |
There was a problem hiding this comment.
Most parts of the rescue blocks are identical, so maybe merge them:
rescue StandardError => e
# In-progress sub-resource deletions aren't failures: re-raise so the root job re-enqueues and waits.
raise if e.is_a?(SubResourceError) && e.any_in_progress?
update_last_operation_with_failure(e.message) unless service_instance.operation_in_progress?
raise e
end
| def initialize(app_guid, user_audit_info) | ||
| super() | ||
| @app_guid = app_guid |
There was a problem hiding this comment.
I would favor the usage of resource_guid here as well (consistent with RecursiveDeleteServiceInstanceJob and the RootJobMixin).
2 participants
Behind
temporary_enable_async_recursive_delete flag(default off). Recursive delete jobs re-enqueue instead of failing when sub-resources (service bindings) are deleted asynchronously, waiting for them to settle and surfacing the original broker error on failure. Adds root/sub job tracking viaroot_job_guidon the jobs table. Foundation for future recursive deletes (org, space).Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests