ATMO-2149: Atmosphere deploy without Subspace #631

calvinmclean · 2018-06-29T17:49:12Z

Description

Replace Subspace by using Ansible's PlaybookCLI to deploy instances.

The main purpose of this PR was to reduce the amount of core Ansible code that had to be modified by Subspace in order to make it easier to update the program to work with newer Ansible versions. In the end, Subspace was completely removed.

In the place of Subspace, we use Ansible's PlaybookCLI object. The execute_playbooks() function adds two attributes to ansible.constants (ansible.cfg values) to suppress Ansible playbook stdout and set the directory used for logging. This is done here because ansible.cfg expects a file path, not a directory path, and would cause errors on regular runs of ansible-playbook and we do not want to suppress stdout on regular ansible-playbook runs. Then, it loops through playbook files and for each one:

Creates a list of arguments identical to those that would be used with the ansible-playbook command (--inventory-file, --limit, --extra-vars, and the playbook path)
Creates the PlaybookCLI object and parses the arguments
Runs the playbook and exits the loop if there is a failure

It returns a list of return codes from each playbook.

More changes were made to the functions that handle the return values to expect integers instead of playbook stats. This greatly simplifies the process and allows for easy upgrades to newer Ansible versions.

Related PRs:

cdosborn · 2018-07-06T20:26:32Z

requirements.txt

@@ -5,7 +5,7 @@
 #    pip-compile --output-file requirements.txt requirements.in
 #
 amqp==2.2.2               # via kombu
-ansible==2.3.2.0          # via subspace
+ansible>=2.5


This file is generated. The requirements.in needs to be edited instead. This is documented in REQUIREMENTS.md.

cdosborn · 2018-07-06T20:28:44Z

service/deploy.py

@@ -34,14 +35,16 @@ def ansible_deployment(
    """
    Use service.ansible to deploy to an instance.
    """
+    username = str(username)


Why is this being coerced to a string?

service/deploy.py

-
-    (unmount_rc, unmount_stdout, unmount_stderr) = _extract_ansible_register(playbook_results, 'unmount_result')
-    if unmount_rc != 0:
-        _raise_unmount_playbook_failure(unmount_rc, unmount_stdout, unmount_stderr)


cdosborn · 2018-07-06T20:33:39Z

service/deploy.py

@@ -289,22 +257,17 @@ def user_deploy(instance_ip, username, instance_id, first_deploy=True, **runner_
        else:
            async_scripts.append(script)

-    format_script = lambda s: {"name": s.get_title_slug(), "text": s.get_text()}
+    format_script = lambda s: {"name": str(s.get_title_slug()), "text": str(s.get_text()).splitlines()}


Is this coercion necessary for 2.5?
We should not be passing a list of lines, why the change?

cdosborn · 2018-07-06T20:36:30Z

service/deploy.py

    # An error has occurred during deployment!
-    # Handle specific errors from ansible based on the 'register' results.
-    playbook_results = playbook_runner.results.get(hostname)
-    _check_results_for_script_failure(playbook_results)
    # If the failure was not related to users boot-scripts,
    # handle as a generic ansible failure.
    return raise_playbook_errors(playbook_runner, instance_id, instance_ip, hostname)


Probably want to be consistent with playbook_results rather than playbook_runner which refers to an Ansible object

cdosborn · 2018-07-06T20:43:43Z

service/deploy.py

+    inventory_dir = "%s/ansible" % settings.ANSIBLE_ROOT
+
+    setattr(C, "DEFAULT_STDOUT_CALLBACK", "null")
+    setattr(C, "DEFAULT_LOG_PATH", settings.DEPLOY_LOG_DIR)


Ideally we don't want either of these setattr calls. The expectation with our use of ansible is that it would log in the celery log, and the plugin would log elsewhere, if there is a duplicate log in the celery log that just needs to be figured out.

DEFAULT_LOG_PATH should not be repurposed here. its a constant completely internal to ansible, that is likely used in the absence of log_path. If you need to get a directory into the plugin, the ansible plugin should be templated by clank just like the ansible.cfg.j2

cdosborn · 2018-07-06T20:45:16Z

service/deploy.py

@@ -529,45 +445,38 @@ def playbook_error_message(runner_details, error_name):


 def execution_has_unreachable(pbs, hostname):
+    """Return value 4 means unreachable in ansible-playbook"""


You don't want naming to fall behind, pbs no longer make sense. See if we actually need these methods, if we do, then please rename param.

cdosborn · 2018-07-06T20:46:27Z

service/tasks/volume.py

@@ -48,14 +48,14 @@ def check_volume_task(driverCls, provider, identity,
        playbooks = deploy_check_volume(
            instance.ip, username, instance.id,
            device_location, device_type=device_type)
-        celery_logger.info(playbooks.__dict__)
+        celery_logger.info(playbooks)


Is playbooks a list of return values? what is being logged?

cdosborn

Looking really good, there are a few more things, that i mentioned in person but not in my review.

cdosborn · 2018-07-11T22:26:37Z

service/tasks/volume.py

@@ -48,14 +48,12 @@ def check_volume_task(driverCls, provider, identity,
        playbooks = deploy_check_volume(


I believe this should be playbook_results or something else

cdosborn · 2018-07-11T22:26:59Z

service/tasks/volume.py

        hostname = build_host_name(instance.id, instance.ip)
-        result = False if execution_has_failures(playbooks, hostname)\
-            or execution_has_unreachable(playbooks, hostname) else True
+        result = False if execution_has_failures(playbooks) or execution_has_unreachable(playbooks) else True


playbooks is a misnomer

cdosborn · 2018-07-11T22:27:51Z

service/tasks/volume.py

        if not result:
            raise Exception(
                "Error encountered while checking volume for filesystem: %s"
-                % playbooks.stats.summarize(host=hostname))
+                % playbooks)


Probably don't want to log this, should instead log instance_id and volume_id

calvinmclean · 2018-07-12T16:42:03Z

Whoops I missed a few of those. I went through and found the rest and removed some function calls that were unnecessary (like building a hostname that is never used)

coveralls · 2018-07-12T19:28:42Z

Coverage increased (+1.04%) to 37.495% when pulling a4cfd8f on calvinmclean:ATMO-2149 into 70d7059 on cyverse:v33.

cdosborn · 2018-07-12T20:28:55Z

I tested this and launched two instances and confirmed that deployment/volume attach/volume detach work.

Again great work.

This was referenced Jun 29, 2018

ATMO-2149: Atmosphere deploy without Subspace cyverse/atmosphere-ansible#154

Merged

ATMO-2149: Atmosphere deploy without Subspace cyverse/clank#262

Merged

calvinmclean force-pushed the ATMO-2149 branch 2 times, most recently from e0f54ff to 7cb258f Compare June 29, 2018 20:56

cdosborn reviewed Jul 10, 2018

View reviewed changes

calvinmclean force-pushed the ATMO-2149 branch 7 times, most recently from bbef979 to 060150a Compare July 11, 2018 17:55

cdosborn suggested changes Jul 11, 2018

View reviewed changes

calvinmclean force-pushed the ATMO-2149 branch from 603acf8 to 71898bd Compare July 12, 2018 16:40

cdosborn changed the base branch from master to v33 July 12, 2018 18:31

cdosborn force-pushed the ATMO-2149 branch from 71898bd to c3133ee Compare July 12, 2018 18:33

Calvin Mclean added 4 commits July 12, 2018 13:20

Instance deploy without Subspace

604d3d7

Fix error related to imports

ea76127

Add section to logrotate

33b17eb

Update CHANGELOG

a4cfd8f

cdosborn force-pushed the ATMO-2149 branch from a8a44cc to a4cfd8f Compare July 12, 2018 20:27

cdosborn approved these changes Jul 12, 2018

View reviewed changes

cdosborn merged commit d6e62bc into cyverse:v33 Jul 12, 2018

cdosborn mentioned this pull request Jul 12, 2018

Option 2: Remove subspace in favor of ForkedAnsible #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ATMO-2149: Atmosphere deploy without Subspace #631

ATMO-2149: Atmosphere deploy without Subspace #631

calvinmclean commented Jun 29, 2018 •

edited

Loading

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

This comment was marked as resolved.

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

cdosborn Jul 6, 2018

cdosborn left a comment

cdosborn Jul 11, 2018

cdosborn Jul 11, 2018

cdosborn Jul 11, 2018

calvinmclean commented Jul 12, 2018

coveralls commented Jul 12, 2018 •

edited

Loading

cdosborn commented Jul 12, 2018

		@@ -529,45 +445,38 @@ def playbook_error_message(runner_details, error_name):


		def execution_has_unreachable(pbs, hostname):
		"""Return value 4 means unreachable in ansible-playbook"""

		@@ -48,14 +48,12 @@ def check_volume_task(driverCls, provider, identity,
		playbooks = deploy_check_volume(

ATMO-2149: Atmosphere deploy without Subspace #631

ATMO-2149: Atmosphere deploy without Subspace #631

Conversation

calvinmclean commented Jun 29, 2018 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdosborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinmclean commented Jul 12, 2018

coveralls commented Jul 12, 2018 • edited Loading

cdosborn commented Jul 12, 2018

calvinmclean commented Jun 29, 2018 •

edited

Loading

coveralls commented Jul 12, 2018 •

edited

Loading