From 77f420c9e1c2de9aa1380391259e106fa01bfcfb Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Wed, 4 Nov 2015 21:13:42 -0800 Subject: [PATCH 01/11] Docstring --- paasta_tools/utils.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/paasta_tools/utils.py b/paasta_tools/utils.py index e074ef91d9..16694e5484 100644 --- a/paasta_tools/utils.py +++ b/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 . + """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: From 3ef2a5c77afb98400b541638f115b911444abc3c Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Wed, 4 Nov 2015 21:18:43 -0800 Subject: [PATCH 02/11] Add deformat_job_id() --- paasta_tools/marathon_tools.py | 5 +++++ tests/test_marathon_tools.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/paasta_tools/marathon_tools.py b/paasta_tools/marathon_tools.py index 82b1b0f9e6..61a89bb9dc 100644 --- a/paasta_tools/marathon_tools.py +++ b/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.""" diff --git a/tests/test_marathon_tools.py b/tests/test_marathon_tools.py index 38c5700dca..8422e90f2a 100644 --- a/tests/test_marathon_tools.py +++ b/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" From 72545b057861c4368753bb84cfc04f04da55c96f Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Wed, 4 Nov 2015 21:30:18 -0800 Subject: [PATCH 03/11] Kill a remove_tag_from_job_id() and use deformat_job_id() --- paasta_tools/cleanup_marathon_jobs.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/paasta_tools/cleanup_marathon_jobs.py b/paasta_tools/cleanup_marathon_jobs.py index 6a9cbf3713..b03a447f6d 100755 --- a/paasta_tools/cleanup_marathon_jobs.py +++ b/paasta_tools/cleanup_marathon_jobs.py @@ -39,7 +39,6 @@ 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 @@ -63,12 +62,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, From f98bcaff66fe5b6567a79099ffe78e0ce4f242d0 Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Wed, 4 Nov 2015 21:32:53 -0800 Subject: [PATCH 04/11] Use deformat_job_id some more --- paasta_tools/marathon_tools.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/paasta_tools/marathon_tools.py b/paasta_tools/marathon_tools.py index 61a89bb9dc..4374c89868 100644 --- a/paasta_tools/marathon_tools.py +++ b/paasta_tools/marathon_tools.py @@ -653,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 From 9638391a4a2c2b2d5fe785758d05f756c1098dcf Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Wed, 4 Nov 2015 22:30:45 -0800 Subject: [PATCH 05/11] Remove another remove_tag_from_job_id() --- paasta_tools/cleanup_marathon_jobs.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/paasta_tools/cleanup_marathon_jobs.py b/paasta_tools/cleanup_marathon_jobs.py index b03a447f6d..69f4dc9a68 100755 --- a/paasta_tools/cleanup_marathon_jobs.py +++ b/paasta_tools/cleanup_marathon_jobs.py @@ -41,7 +41,6 @@ from paasta_tools.utils import _log 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__') @@ -102,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) From 1b6c09925a3d99872fa306f4ba6b9e9973f31658 Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 11:21:01 -0800 Subject: [PATCH 06/11] Remove another. This isn't really right because it overrides arguments, but that means we're passing in multiple forms of the same information, so I got stuck here figuring out how to refactor. That will come next. --- paasta_tools/setup_marathon_job.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/paasta_tools/setup_marathon_job.py b/paasta_tools/setup_marathon_job.py index bea16708fc..869c24a2a6 100755 --- a/paasta_tools/setup_marathon_job.py +++ b/paasta_tools/setup_marathon_job.py @@ -61,7 +61,6 @@ 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 +335,8 @@ def log_deploy_error(errormsg, level='event'): instance=instance ) - short_id = remove_tag_from_job_id(marathon_jobid) + (service, instance, _, __) = marathon_tools.decompose_job_id(marathon_jobid) + short_id = marathon_tools.compose_job_id(service, instance) cluster = load_system_paasta_config().get_cluster() app_list = client.list_apps(embed_failures=True) From e7d3e80fcdee94ba2ba677d20162c50687c6d0fc Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 14:24:34 -0800 Subject: [PATCH 07/11] Catch right exception. Confirmed with manual testing. --- paasta_tools/setup_marathon_job.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/paasta_tools/setup_marathon_job.py b/paasta_tools/setup_marathon_job.py index 869c24a2a6..ed47a1773c 100755 --- a/paasta_tools/setup_marathon_job.py +++ b/paasta_tools/setup_marathon_job.py @@ -57,6 +57,7 @@ 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 @@ -490,8 +491,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() From a01069a3f6955adebb7dd0dce79a18c4a0796507 Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 14:31:12 -0800 Subject: [PATCH 08/11] Correct cockney accent --- paasta_tools/setup_marathon_job.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paasta_tools/setup_marathon_job.py b/paasta_tools/setup_marathon_job.py index ed47a1773c..966cf38c9c 100755 --- a/paasta_tools/setup_marathon_job.py +++ b/paasta_tools/setup_marathon_job.py @@ -432,7 +432,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 From 3570c035c483e376dbc37e73564e48f26261e1aa Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 14:43:05 -0800 Subject: [PATCH 09/11] Turns out I don't need to mess with marathon_jobid at all. It all comes from the same place. This is kind of messy but this call stack is sort of complicated and I've done what I came here to do, so I'ma stop here. --- paasta_tools/setup_marathon_job.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/paasta_tools/setup_marathon_job.py b/paasta_tools/setup_marathon_job.py index 966cf38c9c..d3faf0629b 100755 --- a/paasta_tools/setup_marathon_job.py +++ b/paasta_tools/setup_marathon_job.py @@ -336,8 +336,7 @@ def log_deploy_error(errormsg, level='event'): instance=instance ) - (service, instance, _, __) = marathon_tools.decompose_job_id(marathon_jobid) - short_id = marathon_tools.compose_job_id(service, instance) + short_id = marathon_tools.format_job_id(service, instance) cluster = load_system_paasta_config().get_cluster() app_list = client.list_apps(embed_failures=True) From c3a72b84a03f170951e60e5826abc649df4bc703 Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 14:51:28 -0800 Subject: [PATCH 10/11] Remove another --- tests/test_setup_marathon_job.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_setup_marathon_job.py b/tests/test_setup_marathon_job.py index 618d60f916..d21fb36ffd 100644 --- a/tests/test_setup_marathon_job.py +++ b/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( From acd7bad5b6dc95259e973224857645f3d17debcc Mon Sep 17 00:00:00 2001 From: Tyler Roscoe Date: Thu, 5 Nov 2015 15:25:28 -0800 Subject: [PATCH 11/11] remove_tag_from_job_id is dead --- paasta_tools/utils.py | 12 ------------ tests/test_utils.py | 14 -------------- 2 files changed, 26 deletions(-) diff --git a/paasta_tools/utils.py b/paasta_tools/utils.py index 16694e5484..5d595d0955 100644 --- a/paasta_tools/utils.py +++ b/paasta_tools/utils.py @@ -724,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. diff --git a/tests/test_utils.py b/tests/test_utils.py index 1c0fec4eed..7ac4f57441 100644 --- a/tests/test_utils.py +++ b/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'