Merge pull request #22 from Yelp/mrtyler_deformat

remove remove_tag_from_job_id

paasta_tools/cleanup_marathon_jobs.py

@@ -39,10 +39,8 @@
 from paasta_tools.mesos_tools import is_mesos_leader
 from paasta_tools.utils import InvalidJobNameError
 from paasta_tools.utils import _log
-from paasta_tools.utils import decompose_job_id
 from paasta_tools.utils import get_services_for_cluster
 from paasta_tools.utils import load_system_paasta_config
-from paasta_tools.utils import remove_tag_from_job_id
 
 
 log = logging.getLogger('__main__')
@@ -63,12 +61,10 @@ def parse_args():
 def delete_app(app_id, client):
     """Deletes a marathon app safely and logs to notify the user that it
     happened"""
-    short_app_id = remove_tag_from_job_id(app_id)
     log.warn("%s appears to be old; attempting to delete" % app_id)
-    srv_instance = short_app_id.replace('--', '_')
-    service, instance, _, __ = decompose_job_id(srv_instance)
+    service, instance, _, __ = marathon_tools.deformat_job_id(app_id)
     try:
-        with bounce_lib.bounce_lock_zookeeper(srv_instance):
+        with bounce_lib.bounce_lock_zookeeper(marathon_tools.compose_job_id(service, instance)):
             bounce_lib.delete_marathon_app(app_id, client)
             log_line = "Deleted stale marathon job that looks lost: %s" % app_id
             _log(service=service,
@@ -105,17 +101,16 @@ def cleanup_apps(soa_dir):
                                                 marathon_config.get_password())
 
     valid_services = get_services_for_cluster(instance_type='marathon', soa_dir=soa_dir)
-    valid_short_app_ids = [marathon_tools.format_job_id(name, instance) for (name, instance) in valid_services]
     running_app_ids = marathon_tools.list_all_marathon_app_ids(client)
 
     for app_id in running_app_ids:
         log.debug("Checking app id %s", app_id)
         try:
-            short_app_id = remove_tag_from_job_id(app_id)
+            service, instance, _, __ = marathon_tools.deformat_job_id(app_id)
         except InvalidJobNameError:
             log.warn("%s doesn't conform to paasta naming conventions? Skipping." % app_id)
             continue
-        if short_app_id not in valid_short_app_ids:
+        if (service, instance) not in valid_services:
             delete_app(app_id, client)
 
 

paasta_tools/marathon_tools.py

@@ -572,6 +572,11 @@ def format_job_id(service, instance, git_hash=None, config_hash=None):
     return formatted
 
 
+def deformat_job_id(job_id):
+    job_id = job_id.replace('--', '_')
+    return decompose_job_id(job_id)
+
+
 def read_namespace_for_service_instance(name, instance, cluster=None, soa_dir=DEFAULT_SOA_DIR):
     """Retreive a service instance's nerve namespace from its configuration file.
     If one is not defined in the config file, returns instance instead."""
@@ -648,8 +653,7 @@ def marathon_services_running_here():
                  if u'TASK_RUNNING' in [t[u'state'] for t in ex.get('tasks', [])]]
     srv_list = []
     for executor in executors:
-        srv_name = decompose_job_id(executor['id'])[0].replace('--', '_')
-        srv_instance = decompose_job_id(executor['id'])[1].replace('--', '_')
+        (srv_name, srv_instance, _, __) = deformat_job_id(executor['id'])
         srv_port = int(re.findall('[0-9]+', executor['resources']['ports'])[0])
         srv_list.append((srv_name, srv_instance, srv_port))
     return srv_list

paasta_tools/setup_marathon_job.py

@@ -57,11 +57,11 @@
 from paasta_tools.utils import decompose_job_id
 from paasta_tools.utils import configure_log
 from paasta_tools.utils import InvalidInstanceConfig
+from paasta_tools.utils import InvalidJobNameError
 from paasta_tools.utils import load_system_paasta_config
 from paasta_tools.utils import NoConfigurationForServiceError
 from paasta_tools.utils import NoDeploymentsAvailable
 from paasta_tools.utils import NoDockerImageError
-from paasta_tools.utils import remove_tag_from_job_id
 from paasta_tools.utils import SPACER
 
 # Marathon REST API:
@@ -336,7 +336,7 @@ def log_deploy_error(errormsg, level='event'):
             instance=instance
         )
 
-    short_id = remove_tag_from_job_id(marathon_jobid)
+    short_id = marathon_tools.format_job_id(service, instance)
 
     cluster = load_system_paasta_config().get_cluster()
     app_list = client.list_apps(embed_failures=True)
@@ -431,7 +431,7 @@ def setup_service(service, instance, client, marathon_config,
     Doesn't do anything if the service is already in Marathon and hasn't changed.
     If it's not, attempt to find old instances of the service and bounce them.
 
-    :param ervice: The service name to setup
+    :param service: The service name to setup
     :param instance: The instance of the service to setup
     :param client: A MarathonClient object
     :param marathon_config: The marathon configuration dict
@@ -490,8 +490,8 @@ def main():
         log.setLevel(logging.WARNING)
     try:
         service, instance, _, __ = decompose_job_id(args.service_instance)
-    except ValueError:
-        log.error("Invalid service instance specified. Format is service.instance.")
+    except InvalidJobNameError:
+        log.error("Invalid service instance specified. Format is service%sinstance." % SPACER)
         sys.exit(1)
 
     marathon_config = get_main_marathon_config()

paasta_tools/utils.py

@@ -705,10 +705,12 @@ def compose_job_id(name, instance, git_hash=None, config_hash=None, spacer=SPACE
 
 
 def decompose_job_id(job_id, spacer=SPACER):
-    """Break down a composed job/app id into its (service name, instance, and tag) by splitting with <SPACER>.
+    """Break a composed job id into its constituent (service name, instance,
+    git hash, config hash) by splitting with ``spacer``.
 
     :param job_id: The composed id of the job/app
-    :returns: A tuple (name, instance, tag) that comprise the job_id
+    :returns: A tuple (service name, instance, git hash, config hash) that
+    comprise the job_id
     """
     decomposed = job_id.split(spacer)
     if len(decomposed) == 2:
@@ -722,18 +724,6 @@ def decompose_job_id(job_id, spacer=SPACER):
     return (decomposed[0], decomposed[1], git_hash, config_hash)
 
 
-def remove_tag_from_job_id(job_id, spacer=SPACER):
-    """Remove the tag from a job id, if there is one.
-
-    :param job_id: The job_id.
-    :returns: The job_id with the tag removed, if there was one."""
-    try:
-        parts = decompose_job_id(job_id)
-        return '%s%s%s' % (parts[0], spacer, parts[1])
-    except InvalidJobNameError:
-        raise
-
-
 def build_docker_image_name(upstream_job_name):
     """docker-paasta.yelpcorp.com:443 is the URL for the Registry where PaaSTA
     will look for your images.

tests/test_marathon_tools.py

@@ -1551,6 +1551,11 @@ def test_get_discover_default(self):
         assert marathon_tools.ServiceNamespaceConfig().get_discover() == 'region'
 
 
+def test_deformat_job_id():
+    expected = ('ser_vice', 'in_stance', 'git_hash', 'config_hash')
+    assert marathon_tools.deformat_job_id('ser--vice.in--stance.git--hash.config--hash') == expected
+
+
 def test_create_complete_config_no_smartstack():
     service = "service"
     instance = "instance"

tests/test_setup_marathon_job.py

@@ -27,7 +27,6 @@
 from paasta_tools.utils import decompose_job_id
 from paasta_tools.utils import NoDeploymentsAvailable
 from paasta_tools.utils import NoDockerImageError
-from paasta_tools.utils import remove_tag_from_job_id
 import setup_marathon_job
 
 
@@ -1007,7 +1006,7 @@ def test_deploy_service_already_bouncing(self):
             }
         )
 
-        fake_short_id = remove_tag_from_job_id(fake_id)
+        fake_short_id = marathon_tools.format_job_id(fake_name, fake_instance)
 
         with contextlib.nested(
             mock.patch(

tests/test_utils.py

@@ -408,20 +408,6 @@ def test_decompose_job_id_with_hashes():
     assert actual == expected
 
 
-def test_remove_tag_from_job_id_with_tag():
-    fake_job_id = "my_cool_service.main.git123abc.config456def"
-    expected = "my_cool_service.main"
-    actual = utils.remove_tag_from_job_id(fake_job_id)
-    assert actual == expected
-
-
-def test_remove_tag_from_job_id_without_tag():
-    fake_job_id = "my_cool_service.main"
-    expected = fake_job_id
-    actual = utils.remove_tag_from_job_id(fake_job_id)
-    assert actual == expected
-
-
 def test_build_docker_tag():
     upstream_job_name = 'fake_upstream_job_name'
     upstream_git_commit = 'fake_upstream_git_commit'