Improve deletion of apps

- Tasks in a non-terminal state have to be deleted individually due to
  an 'after_destroy' hook that creates an app usage event. The other
  tasks are bulk-deleted.
- Builds in state 'STAGING' have to be canceled; then all builds can be
  bulk-deleted.
- All deployments are bulk-deleted.
- All revisions are bulk-deleted.
- All sidecars are bulk-deleted.
- Bulk deletion is achieved by adding DELETE CASCADE to multiple foreign
  keys.

app/actions/app_delete.rb

@@ -69,12 +69,12 @@ def record_audit_event(app)
 
     def delete_subresources(app)
       PackageDelete.new(@user_audit_info).delete(app.packages)
-      TaskDelete.new(@user_audit_info).delete(app.tasks)
-      BuildDelete.new(StagingCancel.new(stagers)).delete(app.builds)
+      TaskDelete.new(@user_audit_info).delete_for_app(app.guid)
+      BuildDelete.new(StagingCancel.new(stagers)).delete_for_app(app.guid)
       DropletDelete.new(@user_audit_info).delete(app.droplets)
-      DeploymentDelete.delete(app.deployments)
-      RevisionDelete.delete(app.revisions)
-      SidecarDelete.delete(app.sidecars)
+      DeploymentDelete.delete_for_app(app.guid)
+      RevisionDelete.delete_for_app(app.guid)
+      SidecarDelete.delete_for_app(app.guid)
       RouteMappingDelete.new(@user_audit_info).delete(route_mappings_to_delete(app))
       ProcessDelete.new(@user_audit_info).delete(app.processes)
 

app/actions/build_delete.rb

@@ -4,12 +4,11 @@ def initialize(cancel_action)
       @cancel_action = cancel_action
     end
 
-    def delete(builds)
-      builds = Array(builds)
+    def delete_for_app(guid)
+      builds_in_staging_state = BuildModel.where(app_guid: guid, state: BuildModel::STAGING_STATE).all
+      @cancel_action.cancel(builds_in_staging_state)
 
-      @cancel_action.cancel(builds)
-
-      builds.each(&:destroy)
+      BuildModel.where(app_guid: guid).delete
     end
   end
 end

app/actions/deployment_delete.rb

@@ -2,10 +2,11 @@ module VCAP::CloudController
   class DeploymentDelete
     class << self
       def delete(deployments)
-        deployments.each do |deployment|
-          deployment.historical_related_processes.map(&:destroy)
-          deployment.destroy
-        end
+        deployments.delete
+      end
+
+      def delete_for_app(guid)
+        DeploymentModel.where(app_guid: guid).delete
       end
     end
   end

app/actions/revision_delete.rb

@@ -2,14 +2,11 @@ module VCAP::CloudController
   class RevisionDelete
     class << self
       def delete(revisions)
-        Array(revisions).each do |revision|
-          delete_process_commands(revision)
-          revision.destroy
-        end
+        revisions.delete
       end
 
-      def delete_process_commands(revision)
-        revision.process_commands.each(&:destroy)
+      def delete_for_app(guid)
+        RevisionModel.where(app_guid: guid).delete
       end
     end
   end

app/actions/sidecar_delete.rb

@@ -2,13 +2,11 @@ module VCAP::CloudController
   class SidecarDelete
     class << self
       def delete(sidecars)
-        Array(sidecars).each do |sidecar|
-          sidecar.db.transaction do
-            sidecar.lock!
-            sidecar.sidecar_process_types.each(&:destroy)
-            sidecar.destroy
-          end
-        end
+        sidecars.delete
+      end
+
+      def delete_for_app(guid)
+        SidecarModel.where(app_guid: guid).delete
       end
     end
   end

app/actions/task_delete.rb

@@ -4,11 +4,13 @@ def initialize(user_audit_info)
       @user_audit_info = user_audit_info
     end
 
-    def delete(tasks)
-      tasks.each do |task|
-        task.destroy
+    def delete_for_app(guid)
+      TaskModel.where(app_guid: guid).exclude(state: TaskModel::TERMINAL_STATES).each do |task|
         cancel_running_task(task)
+        task.destroy # needs to be done individually due to the 'after_destroy' hook
       end
+
+      TaskModel.where(app_guid: guid).delete
     end
 
     private

app/jobs/runtime/prune_completed_deployments.rb

@@ -28,9 +28,8 @@ def perform
                                     exclude(state: [DeploymentModel::DEPLOYING_STATE, DeploymentModel::CANCELING_STATE]).
                                     exclude(id: deployments_to_keep)
 
-            DeploymentDelete.delete(deployments_to_delete)
-
-            logger.info("Cleaned up #{deployments_to_delete.count} DeploymentModel rows for app #{app_guid}")
+            delete_count = DeploymentDelete.delete(deployments_to_delete)
+            logger.info("Cleaned up #{delete_count} DeploymentModel rows for app #{app_guid}")
           end
         end
 

app/jobs/runtime/prune_excess_app_revisions.rb

@@ -19,8 +19,11 @@ def perform
             revisions_to_keep = revision_dataset.order(Sequel.desc(:created_at)).
                                 limit(max_retained_revisions_per_app).
                                 select(:id)
-            delete_count = RevisionDelete.delete(revision_dataset.exclude(id: revisions_to_keep))
-            logger.info("Cleaned up #{delete_count.length} revision rows for app #{app_guid}")
+
+            revisions_to_delete = revision_dataset.exclude(id: revisions_to_keep)
+
+            delete_count = RevisionDelete.delete(revisions_to_delete)
+            logger.info("Cleaned up #{delete_count} revision rows for app #{app_guid}")
           end
         end
 

app/models/runtime/deployment_model.rb

@@ -57,6 +57,7 @@ class DeploymentModel < Sequel::Model(:deployments)
     one_to_many :labels, class: 'VCAP::CloudController::DeploymentLabelModel', key: :resource_guid, primary_key: :guid
     one_to_many :annotations, class: 'VCAP::CloudController::DeploymentAnnotationModel', key: :resource_guid, primary_key: :guid
 
+    add_association_dependencies historical_related_processes: :destroy
     add_association_dependencies labels: :destroy
     add_association_dependencies annotations: :destroy
 

db/helpers/add_delete_cascade_to_foreign_key.rb

@@ -0,0 +1,26 @@
+ForeignKey = Struct.new(:table, :name, :column, :referenced_table, :referenced_column, :new_constraint) do
+  def initialize(table, name, column, referenced_table, referenced_column, new_constraint: false)
+    super(table, name, column, referenced_table, referenced_column, new_constraint)
+  end
+end
+
+def foreign_key_exists?(db, table, name)
+  db.foreign_key_list(table).detect { |fk| fk[:name] == name }.present?
+end
+
+def recreate_foreign_key_with_delete_cascade(db, fk)
+  # Remove orphaned entries.
+  db[fk.table].exclude(fk.column => db[fk.referenced_table].select(fk.referenced_column)).delete if fk.new_constraint
+
+  alter_table fk.table do
+    drop_constraint fk.name, type: :foreign_key if foreign_key_exists?(db, fk.table, fk.name)
+    add_foreign_key [fk.column], fk.referenced_table, key: fk.referenced_column, name: fk.name, on_delete: :cascade
+  end
+end
+
+def recreate_foreign_key_without_delete_cascade(db, fk)
+  alter_table fk.table do
+    drop_constraint fk.name, type: :foreign_key if foreign_key_exists?(db, fk.table, fk.name)
+    add_foreign_key [fk.column], fk.referenced_table, key: fk.referenced_column, name: fk.name unless fk.new_constraint
+  end
+end

db/migrations/20240115163000_add_delete_cascade_to_foreign_keys.rb

@@ -0,0 +1,40 @@
+require File.expand_path('../../helpers/add_delete_cascade_to_foreign_key', __FILE__)
+
+Sequel.migration do
+  foreign_keys = [
+    ForeignKey.new(:deployment_processes, :fk_deployment_processes_deployment_guid, :deployment_guid, :deployments, :guid),
+    ForeignKey.new(:deployment_labels, :fk_deployment_labels_resource_guid, :resource_guid, :deployments, :guid),
+    ForeignKey.new(:deployment_annotations, :fk_deployment_annotations_resource_guid, :resource_guid, :deployments, :guid),
+    ForeignKey.new(:build_labels, :fk_build_labels_resource_guid, :resource_guid, :builds, :guid),
+    ForeignKey.new(:build_annotations, :fk_build_annotations_resource_guid, :resource_guid, :builds, :guid),
+    ForeignKey.new(:kpack_lifecycle_data, :fk_kpack_lifecycle_build_guid, :build_guid, :builds, :guid),
+    ForeignKey.new(:buildpack_lifecycle_data, :fk_buildpack_lifecycle_build_guid, :build_guid, :builds, :guid, new_constraint: true),
+    ForeignKey.new(:buildpack_lifecycle_buildpacks, :fk_blbuildpack_bldata_guid, :buildpack_lifecycle_data_guid, :buildpack_lifecycle_data, :guid),
+    ForeignKey.new(:task_labels, :fk_task_labels_resource_guid, :resource_guid, :tasks, :guid),
+    ForeignKey.new(:task_annotations, :fk_task_annotations_resource_guid, :resource_guid, :tasks, :guid),
+    ForeignKey.new(:revision_labels, :fk_revision_labels_resource_guid, :resource_guid, :revisions, :guid),
+    ForeignKey.new(:revision_annotations, :fk_revision_annotations_resource_guid, :resource_guid, :revisions, :guid),
+    ForeignKey.new(:revision_process_commands, :rev_commands_revision_guid_fkey, :revision_guid, :revisions, :guid),
+    ForeignKey.new(:revision_sidecars, :fk_sidecar_revision_guid, :revision_guid, :revisions, :guid),
+    ForeignKey.new(:revision_sidecar_process_types, :fk_revision_sidecar_proc_type_sidecar_guid, :revision_sidecar_guid, :revision_sidecars, :guid),
+    ForeignKey.new(:sidecar_process_types, :fk_sidecar_proc_type_sidecar_guid, :sidecar_guid, :sidecars, :guid),
+  ]
+
+  no_transaction
+
+  up do
+    db = self
+
+    foreign_keys.each do |fk|
+      transaction { recreate_foreign_key_with_delete_cascade(db, fk) }
+    end
+  end
+
+  down do
+    db = self
+
+    foreign_keys.each do |fk|
+      transaction { recreate_foreign_key_without_delete_cascade(db, fk) }
+    end
+  end
+end

lib/cloud_controller/deployments/deployment_target_state.rb

@@ -48,7 +48,7 @@ def apply_to_app(app, user_audit_info)
     private
 
     def apply_sidecars(app, revision_sidecars)
-      SidecarDelete.delete(app.sidecars)
+      SidecarDelete.delete_for_app(app.guid)
       revision_sidecars.each { |rs| rehydrate(rs) }
     end
 

spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb

@@ -0,0 +1,40 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+
+RSpec.describe 'migration to add delete cascade to foreign keys', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20240115163000_add_delete_cascade_to_foreign_keys.rb' }
+  end
+
+  describe 'buildpack_lifecycle_data table' do
+    after do
+      db[:buildpack_lifecycle_data].delete
+      db[:builds].delete
+    end
+
+    context 'before adding the foreign key' do
+      it 'allows inserts with a build_guid that does not exist' do
+        expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.not_to raise_error
+      end
+    end
+
+    context 'after adding the foreign key' do
+      it 'prevents inserts with a build_guid that does not exist' do
+        Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+
+        expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation)
+      end
+
+      it 'deleted orphaned buildpack_lifecycle_data entries but kept valid ones' do
+        db[:builds].insert(guid: 'build_guid')
+        db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid')
+        db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists')
+
+        Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+
+        expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1)
+        expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0)
+      end
+    end
+  end
+end

spec/migrations/Readme.md

@@ -44,7 +44,7 @@ To create resilient and reliable migrations, follow these guidelines:
        ...
      ].freeze
 
-     no_transactions # IMPORTANT: DISABLE AUTOMATIC TRANSACTION MANAGEMENT BY UP/DOWN/CHANGE BLOCKS.
+     no_transaction # IMPORTANT: DISABLE AUTOMATIC TRANSACTION MANAGEMENT BY UP/DOWN/CHANGE BLOCKS.
 
      up do
        annotation_tables.each do |table|

spec/migrations/helpers/migration_shared_context.rb

@@ -1,31 +1,39 @@
+def mktmpsubdir(tmpdir, name)
+  dir = File.join(tmpdir, name)
+  Dir.mkdir(dir)
+  dir
+end
+
 RSpec.shared_context 'migration' do
-  let(:all_migrations) { Dir.mktmpdir }
-  let(:down_migrations) { Dir.mktmpdir }
-  let(:migration_to_test) { Dir.mktmpdir }
+  let(:tmpdir) { Dir.mktmpdir }
+  let(:down_migrations) { mktmpsubdir(tmpdir, 'down_migrations') }
+  let(:migration_to_test) { mktmpsubdir(tmpdir, 'migration_to_test') }
   let(:db) { Sequel::Model.db }
 
   before do
     Sequel.extension :migration
+
     # Find all migrations
-    migration_files = Dir.glob(sprintf('%s/*.rb', DBMigrator::SEQUEL_MIGRATIONS))
+    all_migration_files = Dir.glob(sprintf('%s/*.rb', DBMigrator::SEQUEL_MIGRATIONS))
     # Calculate the index of our migration file we`d like to test
-    migration_index = migration_files.find_index { |file| file.end_with?(migration_filename) }
+    migration_index = all_migration_files.find_index { |file| file.end_with?(migration_filename) }
     # Make a file list of the migration file we like to test plus all migrations after the one we want to test
-    migration_files_after_test = migration_files[migration_index...]
+    migration_files_after_test = all_migration_files[migration_index...]
+
     # Copy them to a temp directory
-    FileUtils.cp(migration_files, all_migrations)
     FileUtils.cp(migration_files_after_test, down_migrations)
     FileUtils.cp(File.join(DBMigrator::SEQUEL_MIGRATIONS, migration_filename), migration_to_test)
+
+    # Copy migration helper files to temp directory
+    FileUtils.cp_r(File.join(DBMigrator::MIGRATIONS_DIR, 'helpers'), tmpdir)
+
     # Revert the given migration and everything newer so we are at the database version exactly before our migration we want to test.
     Sequel::Migrator.run(db, down_migrations, target: 0, allow_missing_migration_files: true)
   end
 
   after do
-    FileUtils.rm_rf(migration_to_test)
-    FileUtils.rm_rf(down_migrations)
-
     # Complete the migration to not leave the test database half migrated and following tests fail due to this
-    Sequel::Migrator.run(db, all_migrations, allow_missing_migration_files: true)
-    FileUtils.rm_rf(all_migrations)
+    Sequel::Migrator.run(db, down_migrations, allow_missing_migration_files: true)
+    FileUtils.rm_rf(tmpdir)
   end
 end

spec/unit/actions/build_delete_spec.rb

@@ -7,14 +7,68 @@ module VCAP::CloudController
     subject(:build_delete) { BuildDelete.new(cancel_action) }
     let(:cancel_action) { instance_double(StagingCancel, cancel: nil) }
 
-    describe '#delete' do
-      let!(:build) { BuildModel.make }
+    describe '#delete_for_app' do
+      let!(:app) { AppModel.make }
+      let!(:build1) { BuildModel.make(app: app, state: BuildModel::STAGED_STATE) }
+      let!(:build2) { BuildModel.make(app: app, state: BuildModel::STAGING_STATE) }
 
-      it 'deletes and cancels the build record' do
-        build_delete.delete([build])
+      it 'deletes the builds' do
+        expect do
+          build_delete.delete_for_app(app.guid)
+        end.to change(BuildModel, :count).by(-2)
+        [build1, build2].each { |b| expect(b).not_to exist }
+      end
+
+      it 'cancels builds in STAGING_STATE' do
+        build_delete.delete_for_app(app.guid)
+
+        expect(cancel_action).to have_received(:cancel).with([build2])
+      end
+
+      it 'deletes associated labels' do
+        label1 = BuildLabelModel.make(build: build1, key_name: 'test', value: 'bommel')
+        label2 = BuildLabelModel.make(build: build2, key_name: 'test', value: 'bommel')
+
+        expect do
+          build_delete.delete_for_app(app.guid)
+        end.to change(BuildLabelModel, :count).by(-2)
+        [label1, label2].each { |l| expect(l).not_to exist }
+      end
+
+      it 'deletes associated annotations' do
+        annotation1 = BuildAnnotationModel.make(build: build1, key_name: 'test', value: 'bommel')
+        annotation2 = BuildAnnotationModel.make(build: build2, key_name: 'test', value: 'bommel')
+
+        expect do
+          build_delete.delete_for_app(app.guid)
+        end.to change(BuildAnnotationModel, :count).by(-2)
+        [annotation1, annotation2].each { |a| expect(a).not_to exist }
+      end
+
+      it 'deletes associated buildpack lifecycle data/buildpack' do
+        lifecycle_data1 = BuildpackLifecycleDataModel.make(build: build1)
+        lifecycle_data2 = BuildpackLifecycleDataModel.make(build: build2)
+        lifecycle_buildpack1 = BuildpackLifecycleBuildpackModel.make(
+          buildpack_lifecycle_data: lifecycle_data1, admin_buildpack_name: nil, buildpack_url: 'http://example.com/buildpack1'
+        )
+        lifecycle_buildpack2 = BuildpackLifecycleBuildpackModel.make(
+          buildpack_lifecycle_data: lifecycle_data2, admin_buildpack_name: nil, buildpack_url: 'http://example.com/buildpack2'
+        )
+
+        expect do
+          build_delete.delete_for_app(app.guid)
+        end.to change(BuildpackLifecycleDataModel, :count).by(-2).and change(BuildpackLifecycleBuildpackModel, :count).by(-2)
+        [lifecycle_data1, lifecycle_data2, lifecycle_buildpack1, lifecycle_buildpack2].each { |l| expect(l).not_to exist }
+      end
+
+      it 'deletes associated kpack lifecycle data' do
+        lifecycle1 = KpackLifecycleDataModel.make(build: build1)
+        lifecycle2 = KpackLifecycleDataModel.make(build: build2)
 
-        expect(build.exists?).to be(false), 'Expected build to not exist, but it does'
-        expect(cancel_action).to have_received(:cancel).with([build])
+        expect do
+          build_delete.delete_for_app(app.guid)
+        end.to change(KpackLifecycleDataModel, :count).by(-2)
+        [lifecycle1, lifecycle2].each { |l| expect(l).not_to exist }
       end
     end
   end