Fix issues with processes_sync:

* Exclude docker apps from sync if diego_docker is disabled
* ensure CNB apps are still synced if diego_docker is disabled

Author: sethboyles

app/models/runtime/process_model.rb

@@ -98,16 +98,6 @@ def runnable
       def diego
         where(diego: true)
       end
-
-      def buildpack_type
-        inner_join(BuildpackLifecycleDataModel.table_name, app_guid: :app_guid).
-          select_all(:processes)
-      end
-
-      def non_docker_type
-        inner_join(BuildpackLifecycleDataModel.table_name, app_guid: :app_guid).
-          select_all(:processes)
-      end
     end
 
     one_through_many :organization,

lib/cloud_controller/diego/processes_sync.rb

@@ -26,7 +26,7 @@ def sync
         batched_processes_for_sync do |processes|
           processes.each do |process|
             process_guid = ProcessGuid.from_process(process)
-            diego_lrp    = diego_lrps.delete(process_guid)
+            diego_lrp = diego_lrps.delete(process_guid)
 
             if diego_lrp.nil?
               to_desire.append(process.id)
@@ -143,16 +143,10 @@ def batched_processes(ids)
 
       def processes(ids)
         processes = ProcessModel.
-                    diego.
-                    runnable.
                     where(Sequel.lit("#{ProcessModel.table_name}.id IN ?", ids)).
                     eager(:desired_droplet, :space, :service_bindings, { routes: :domain }, { app: :buildpack_lifecycle_data })
-        if FeatureFlag.enabled?(:diego_docker)
-          processes.select_all(ProcessModel.table_name)
-        else
-          # `select_all` is called by `non_docker_type`
-          processes.non_docker_type
-        end
+
+        processes.select_all(ProcessModel.table_name)
       end
 
       def processes_for_sync(last_id)
@@ -163,6 +157,12 @@ def processes_for_sync(last_id)
                     order(:"#{ProcessModel.table_name}__id").
                     limit(BATCH_SIZE)
 
+        unless FeatureFlag.enabled?(:diego_docker)
+          non_docker_app_guids = BuildpackLifecycleDataModel.select(:app_guid).
+                                 union(CNBLifecycleDataModel.select(:app_guid))
+          processes = processes.where(Sequel.qualify(ProcessModel.table_name, :app_guid) => non_docker_app_guids)
+        end
+
         processes.select(:"#{ProcessModel.table_name}__id", :"#{ProcessModel.table_name}__guid", :"#{ProcessModel.table_name}__version",
                          :"#{ProcessModel.table_name}__updated_at")
       end

spec/isolated_specs/processes_sync_spec.rb

@@ -56,6 +56,42 @@ module Diego
             expect(bbs_apps_client).not_to have_received(:stop_app)
             expect(bbs_apps_client).to have_received(:bump_freshness).once
           end
+
+          context 'when the process is a docker app' do
+            let(:app) { AppModel.make(:docker, droplet: DropletModel.make(:docker)) }
+            let(:good_process) { ProcessModel.make(:docker, state: 'STARTED', app: app) }
+
+            context 'when diego_docker is enabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: true)
+              end
+
+              it 'does not touch lrps that are up to date and correct' do
+                allow(bbs_apps_client).to receive(:desire_app)
+                allow(bbs_apps_client).to receive(:update_app)
+                allow(bbs_apps_client).to receive(:stop_app)
+
+                subject.sync
+
+                expect(bbs_apps_client).not_to have_received(:desire_app)
+                expect(bbs_apps_client).not_to have_received(:update_app)
+                expect(bbs_apps_client).not_to have_received(:stop_app)
+                expect(bbs_apps_client).to have_received(:bump_freshness).once
+              end
+            end
+
+            context 'when diego_docker is disabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: false)
+              end
+
+              it 'deletes the LRP' do
+                allow(bbs_apps_client).to receive(:stop_app)
+                subject.sync
+                expect(bbs_apps_client).to have_received(:stop_app).with(ProcessGuid.from_process(good_process))
+              end
+            end
+          end
         end
 
         context 'when a diego LRP is stale' do
@@ -85,6 +121,72 @@ module Diego
             expect(bbs_apps_client).to have_received(:bump_freshness).once
           end
 
+          context 'when the process is a cnb app' do
+            let(:app) { AppModel.make(:cnb, droplet: DropletModel.make(:cnb)) }
+            let!(:stale_process) { ProcessModel.make(:cnb, state: 'STARTED', app: app) }
+
+            before do
+              FeatureFlag.create(name: 'diego_cnb', enabled: true)
+            end
+
+            context 'when diego_docker is disabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: false)
+              end
+
+              it 'updates the stale lrp' do
+                allow(bbs_apps_client).to receive(:update_app)
+                subject.sync
+                expect(bbs_apps_client).to have_received(:update_app).with(stale_process, stale_lrp_scheduling_info)
+                expect(bbs_apps_client).to have_received(:bump_freshness).once
+              end
+            end
+
+            context 'when diego_docker is enabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: true)
+              end
+
+              it 'updates the stale lrp' do
+                allow(bbs_apps_client).to receive(:update_app)
+                subject.sync
+                expect(bbs_apps_client).to have_received(:update_app).with(stale_process, stale_lrp_scheduling_info)
+                expect(bbs_apps_client).to have_received(:bump_freshness).once
+              end
+            end
+          end
+
+          context 'when the process is a docker app' do
+            let(:app) { AppModel.make(:docker, droplet: DropletModel.make(:docker)) }
+            let!(:stale_process) { ProcessModel.make(:docker, state: 'STARTED', app: app) }
+
+            context 'when diego_docker is enabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: true)
+              end
+
+              it 'updates the stale lrp' do
+                allow(bbs_apps_client).to receive(:update_app)
+                subject.sync
+                expect(bbs_apps_client).to have_received(:update_app).with(stale_process, stale_lrp_scheduling_info)
+                expect(bbs_apps_client).to have_received(:bump_freshness).once
+              end
+            end
+
+            context 'when diego_docker is disabled' do
+              before do
+                FeatureFlag.create(name: 'diego_docker', enabled: false)
+              end
+
+              it 'deletes the lrp' do
+                allow(bbs_apps_client).to receive(:stop_app)
+                subject.sync
+                expect(bbs_apps_client).to have_received(:stop_app).with(ProcessGuid.from_process(stale_process))
+                expect(bbs_apps_client).to have_received(:bump_freshness).once
+              end
+            end
+          end
+
           context 'when updating app fails' do
             # bbs_apps_client will raise ApiErrors as of right now, we should think about factoring that out so that
             # the background job doesn't have to deal with API concerns

spec/unit/models/runtime/process_model_spec.rb

@@ -30,23 +30,6 @@ def expect_no_validator(validator_class)
       VCAP::CloudController::Seeds.create_seed_stacks
     end
 
-    describe 'dataset module' do
-      let!(:buildpack_process) { ProcessModel.make }
-      let!(:docker_process) { ProcessModel.make(:docker) }
-
-      describe '#buildpack_type' do
-        it 'only returns processes associated with a buildpack app' do
-          expect(ProcessModel.buildpack_type.map(&:name)).to contain_exactly(buildpack_process.name)
-        end
-      end
-
-      describe '#non_docker_type' do
-        it 'only returns processes not associated with a docker app' do
-          expect(ProcessModel.non_docker_type.map(&:name)).to contain_exactly(buildpack_process.name)
-        end
-      end
-    end
-
     describe 'Creation' do
       subject(:process) { ProcessModel.new }