Consolidate/tighten up filtering of marathon apps so status and bounc…

paasta_tools/marathon_tools.py

@@ -897,11 +897,19 @@ def get_expected_instance_count_for_namespace(service, namespace, cluster=None,
 
 
 def get_matching_appids(servicename, instance, client):
+    """Returns a list of appids given a service and instance.
+    Useful for fuzzy matching if you think there are marathon
+    apps running but you don't know the full instance id"""
+    return [app.id for app in get_matching_apps(servicename, instance, client)]
+
+
+def get_matching_apps(servicename, instance, client, embed_failures=False):
     """Returns a list of appids given a service and instance.
     Useful for fuzzy matching if you think there are marathon
     apps running but you don't know the full instance id"""
     jobid = format_job_id(servicename, instance)
-    return [app.id for app in client.list_apps() if app.id.startswith("/%s" % jobid)]
+    expected_prefix = "/%s%s" % (jobid, MESOS_TASK_SPACER)
+    return [app for app in client.list_apps(embed_failures=embed_failures) if app.id.startswith(expected_prefix)]
 
 
 def get_healthcheck_for_instance(service, instance, service_manifest, random_port, soa_dir=DEFAULT_SOA_DIR):

paasta_tools/setup_marathon_job.py

@@ -339,8 +339,7 @@ def log_deploy_error(errormsg, level='event'):
     short_id = marathon_tools.format_job_id(service, instance)
 
     cluster = load_system_paasta_config().get_cluster()
-    app_list = client.list_apps(embed_failures=True)
-    existing_apps = [app for app in app_list if short_id in app.id]
+    existing_apps = marathon_tools.get_matching_apps(service, instance, client, embed_failures=True)
     new_app_list = [a for a in existing_apps if a.id == '/%s' % config['id']]
     other_apps = [a for a in existing_apps if a.id != '/%s' % config['id']]
     serviceinstance = "%s.%s" % (service, instance)

tests/test_marathon_tools.py

@@ -1186,16 +1186,20 @@ def config_helper(name, inst, cluster, soa_dir=None):
             read_config_patch.assert_any_call(service, 'green', 'fake_cluster', soa_dir=soa_dir)
 
     def test_get_matching_appids(self):
-        fakeapp1 = mock.Mock(id='/fake--service.fake--instance---bouncingold')
-        fakeapp2 = mock.Mock(id='/fake--service.fake--instance---bouncingnew')
-        fakeapp3 = mock.Mock(id='/fake--service.other--instance--bla')
-        fakeapp4 = mock.Mock(id='/other--service')
-        apps = [fakeapp1, fakeapp2, fakeapp3, fakeapp4]
+        apps = [
+            mock.Mock(id='/fake--service.fake--instance.bouncingold'),
+            mock.Mock(id='/fake--service.fake--instance.bouncingnew'),
+            mock.Mock(id='/fake--service.other--instance.bla'),
+            mock.Mock(id='/other--service'),
+            mock.Mock(id='/fake--service.fake--instance--with--suffix.something'),
+            mock.Mock(id='/prefixed--fake--service.fake--instance.something'),
+        ]
+
         list_apps_mock = mock.Mock(return_value=apps)
         fake_client = mock.Mock(list_apps=list_apps_mock)
         expected = [
-            '/fake--service.fake--instance---bouncingold',
-            '/fake--service.fake--instance---bouncingnew',
+            '/fake--service.fake--instance.bouncingold',
+            '/fake--service.fake--instance.bouncingnew',
         ]
         actual = marathon_tools.get_matching_appids('fake_service', 'fake_instance', fake_client)
         assert actual == expected

tests/test_setup_marathon_job.py

@@ -891,15 +891,15 @@ def test_deploy_service_known_bounce(self):
         fake_drain_method_name = 'noop'
         fake_name = 'how_many_strings'
         fake_instance = 'will_i_need_to_think_of'
-        fake_id = marathon_tools.format_job_id(fake_name, fake_instance, 'gityourmom', 'configyourdad')
+        fake_id = marathon_tools.format_job_id(fake_name, fake_instance, 'git11111111', 'config11111111')
         fake_config = {'id': fake_id, 'instances': 2}
 
-        old_app_id = ('%s2' % fake_id)
+        old_app_id = marathon_tools.format_job_id(fake_name, fake_instance, 'git22222222', 'config22222222')
         old_task_to_drain = mock.Mock(id="old_task_to_drain", app_id=old_app_id)
         old_task_is_draining = mock.Mock(id="old_task_is_draining", app_id=old_app_id)
         old_task_dont_drain = mock.Mock(id="old_task_dont_drain", app_id=old_app_id)
 
-        old_app = mock.Mock(id=old_app_id, tasks=[old_task_to_drain, old_task_is_draining, old_task_dont_drain])
+        old_app = mock.Mock(id="/%s" % old_app_id, tasks=[old_task_to_drain, old_task_is_draining, old_task_dont_drain])
 
         fake_client = mock.MagicMock(
             list_apps=mock.Mock(return_value=[old_app]),
@@ -966,8 +966,8 @@ def test_deploy_service_known_bounce(self):
             fake_drain_method.drain.assert_any_call(old_task_to_drain)
 
             assert fake_client.kill_task.call_count == 2
-            fake_client.kill_task.assert_any_call(old_app.id, old_task_is_draining.id, scale=True)
-            fake_client.kill_task.assert_any_call(old_app.id, old_task_to_drain.id, scale=True)
+            fake_client.kill_task.assert_any_call(old_app_id, old_task_is_draining.id, scale=True)
+            fake_client.kill_task.assert_any_call(old_app_id, old_task_to_drain.id, scale=True)
 
             create_marathon_app_patch.assert_called_once_with(fake_config['id'], fake_config, fake_client)
             assert kill_old_ids_patch.call_count == 0