Merge pull request #2239 from smunilla/inventory_write_improvements

a-o-i: Better inventory group handling

3 files changed, 49 insertions, 72 deletions

diff --git a/utils/src/ooinstall/cli_installer.py b/utils/src/ooinstall/cli_installer.py
index 9fbb61a46..b9750fad1 100644
--- a/utils/src/ooinstall/cli_installer.py
+++ b/utils/src/ooinstall/cli_installer.py
@@ -121,7 +121,7 @@ http://docs.openshift.com/enterprise/latest/architecture/infrastructure_componen
     click.echo(message)
 
     hosts = []
-    roles = set(['master', 'node', 'storage'])
+    roles = set(['master', 'node', 'storage', 'etcd'])
     more_hosts = True
     num_masters = 0
     while more_hosts:
@@ -133,6 +133,7 @@ http://docs.openshift.com/enterprise/latest/architecture/infrastructure_componen
         if not masters_set:
             if click.confirm('Will this host be an OpenShift Master?'):
                 host_props['roles'].append('master')
+                host_props['roles'].append('etcd')
                 num_masters += 1
 
                 if oo_cfg.settings['variant_version'] == '3.0':
diff --git a/utils/src/ooinstall/openshift_ansible.py b/utils/src/ooinstall/openshift_ansible.py
index bcf06b599..54178f0cd 100644
--- a/utils/src/ooinstall/openshift_ansible.py
+++ b/utils/src/ooinstall/openshift_ansible.py
@@ -1,5 +1,3 @@
-# TODO: Temporarily disabled due to importing old code into openshift-ansible
-# repo. We will work on these over time.
 # pylint: disable=bad-continuation,missing-docstring,no-self-use,invalid-name,global-statement,global-variable-not-assigned
 
 import socket
@@ -14,7 +12,9 @@ CFG = None
 ROLES_TO_GROUPS_MAP = {
     'master': 'masters',
     'node': 'nodes',
-    'storage': 'nfs'
+    'etcd': 'etcd',
+    'storage': 'nfs',
+    'master_lb': 'lb'
 }
 
 VARIABLES_MAP = {
@@ -34,75 +34,52 @@ def set_config(cfg):
 
 def generate_inventory(hosts):
     global CFG
+
     masters = [host for host in hosts if host.is_master()]
-    nodes = [host for host in hosts if host.is_node()]
-    new_nodes = [host for host in hosts if host.is_node() and host.new_host]
-    proxy = determine_proxy_configuration(hosts)
-    storage = determine_storage_configuration(hosts)
     multiple_masters = len(masters) > 1
+
+    new_nodes = [host for host in hosts if host.is_node() and host.new_host]
     scaleup = len(new_nodes) > 0
 
+    lb = determine_lb_configuration(hosts)
+
     base_inventory_path = CFG.settings['ansible_inventory_path']
     base_inventory = open(base_inventory_path, 'w')
 
-    write_inventory_children(base_inventory, multiple_masters, proxy, scaleup)
-
-    write_inventory_vars(base_inventory, multiple_masters, proxy)
-
-
-
-    base_inventory.write('\n[masters]\n')
-    for master in masters:
-        write_host(master, base_inventory)
-
-    if len(masters) > 1:
-        base_inventory.write('\n[etcd]\n')
-        for master in masters:
-            write_host(master, base_inventory)
+    write_inventory_children(base_inventory, scaleup)
 
-    base_inventory.write('\n[nodes]\n')
+    write_inventory_vars(base_inventory, multiple_masters, lb)
 
-    for node in nodes:
-        # Let the fact defaults decide if we're not a master:
-        schedulable = None
-
-        # If the node is also a master, we must explicitly set schedulablity:
-        if node.is_master():
-            schedulable = node.is_schedulable_node(hosts)
-        write_host(node, base_inventory, schedulable)
-
-    if not getattr(proxy, 'preconfigured', True):
-        base_inventory.write('\n[lb]\n')
-        write_host(proxy, base_inventory)
 
+    #write_inventory_hosts
+    for role in CFG.deployment.roles:
+        # write group block
+        group = ROLES_TO_GROUPS_MAP.get(role, role)
+        base_inventory.write("\n[{}]\n".format(group))
+        # write each host
+        group_hosts = [host for host in hosts if role in host.roles]
+        for host in group_hosts:
+            schedulable = host.is_schedulable_node(hosts)
+            write_host(host, role, base_inventory, schedulable)
 
     if scaleup:
         base_inventory.write('\n[new_nodes]\n')
         for node in new_nodes:
-            write_host(node, base_inventory)
-
-    if storage:
-        base_inventory.write('\n[nfs]\n')
-        write_host(storage, base_inventory)
+            write_host(node, 'new_nodes', base_inventory)
 
     base_inventory.close()
     return base_inventory_path
 
-def determine_proxy_configuration(hosts):
-    proxy = next((host for host in hosts if host.is_master_lb()), None)
-    if proxy:
-        if proxy.hostname == None:
-            proxy.hostname = proxy.connect_to
-            proxy.public_hostname = proxy.connect_to
-
-    return proxy
+def determine_lb_configuration(hosts):
+    lb = next((host for host in hosts if host.is_master_lb()), None)
+    if lb:
+        if lb.hostname == None:
+            lb.hostname = lb.connect_to
+            lb.public_hostname = lb.connect_to
 
-def determine_storage_configuration(hosts):
-    storage = next((host for host in hosts if host.is_storage()), None)
+    return lb
 
-    return storage
-
-def write_inventory_children(base_inventory, multiple_masters, proxy, scaleup):
+def write_inventory_children(base_inventory, scaleup):
     global CFG
 
     base_inventory.write('\n[OSEv3:children]\n')
@@ -112,13 +89,10 @@ def write_inventory_children(base_inventory, multiple_masters, proxy, scaleup):
 
     if scaleup:
         base_inventory.write('new_nodes\n')
-    if multiple_masters:
-        base_inventory.write('etcd\n')
-    if not getattr(proxy, 'preconfigured', True):
-        base_inventory.write('lb\n')
+
 
 # pylint: disable=too-many-branches
-def write_inventory_vars(base_inventory, multiple_masters, proxy):
+def write_inventory_vars(base_inventory, multiple_masters, lb):
     global CFG
     base_inventory.write('\n[OSEv3:vars]\n')
 
@@ -135,11 +109,11 @@ def write_inventory_vars(base_inventory, multiple_masters, proxy):
     if CFG.settings['ansible_ssh_user'] != 'root':
         base_inventory.write('ansible_become=yes\n')
 
-    if multiple_masters and proxy is not None:
+    if multiple_masters and lb is not None:
         base_inventory.write('openshift_master_cluster_method=native\n')
-        base_inventory.write("openshift_master_cluster_hostname={}\n".format(proxy.hostname))
+        base_inventory.write("openshift_master_cluster_hostname={}\n".format(lb.hostname))
         base_inventory.write(
-            "openshift_master_cluster_public_hostname={}\n".format(proxy.public_hostname))
+            "openshift_master_cluster_public_hostname={}\n".format(lb.public_hostname))
 
     if CFG.settings.get('variant_version', None) == '3.1':
         #base_inventory.write('openshift_image_tag=v{}\n'.format(CFG.settings.get('variant_version')))
@@ -195,9 +169,12 @@ def write_proxy_settings(base_inventory):
 
 
 # pylint: disable=too-many-branches
-def write_host(host, inventory, schedulable=None):
+def write_host(host, role, inventory, schedulable=None):
     global CFG
 
+    if host.preconfigured:
+        return
+
     facts = ''
     if host.ip:
         facts += ' openshift_ip={}'.format(host.ip)
@@ -215,14 +192,13 @@ def write_host(host, inventory, schedulable=None):
     if host.node_labels:
         facts += ' openshift_node_labels="{}"'.format(host.node_labels)
 
+
     # Distinguish between three states, no schedulability specified (use default),
     # explicitly set to True, or explicitly set to False:
-    if schedulable is None:
+    if role != 'node' or schedulable is None:
         pass
-    elif schedulable:
-        facts += ' openshift_schedulable=True'
-    elif not schedulable:
-        facts += ' openshift_schedulable=False'
+    else:
+        facts += " openshift_schedulable={}".format(schedulable)
 
     installer_host = socket.gethostname()
     if installer_host in [host.connect_to, host.hostname, host.public_hostname]:
diff --git a/utils/test/cli_installer_tests.py b/utils/test/cli_installer_tests.py
index 13973f22f..d79163e91 100644
--- a/utils/test/cli_installer_tests.py
+++ b/utils/test/cli_installer_tests.py
@@ -815,9 +815,9 @@ class AttendedCliTests(OOCliFixture):
         self.assert_inventory_host_var(inventory, 'nodes', '10.0.0.1',
                                  'openshift_schedulable=False')
         self.assert_inventory_host_var_unset(inventory, 'nodes', '10.0.0.2',
-                                 'openshift_schedulable')
+                                 'openshift_schedulable=True')
         self.assert_inventory_host_var_unset(inventory, 'nodes', '10.0.0.3',
-                                 'openshift_schedulable')
+                                 'openshift_schedulable=True')
 
     # interactive with config file and some installed some uninstalled hosts
     @patch('ooinstall.openshift_ansible.run_main_playbook')
@@ -939,7 +939,7 @@ class AttendedCliTests(OOCliFixture):
         self.assert_inventory_host_var(inventory, 'nodes', '10.0.0.3',
                                        'openshift_schedulable=False')
         self.assert_inventory_host_var_unset(inventory, 'nodes', '10.0.0.4',
-                                             'openshift_schedulable')
+                                             'openshift_schedulable=True')
 
         self.assertTrue(inventory.has_section('etcd'))
         self.assertEquals(3, len(inventory.items('etcd')))
@@ -1122,9 +1122,9 @@ class AttendedCliTests(OOCliFixture):
         self.assert_inventory_host_var(inventory, 'nodes', '10.0.0.1',
                                  'openshift_schedulable=False')
         self.assert_inventory_host_var_unset(inventory, 'nodes', '10.0.0.2',
-                                 'openshift_schedulable')
+                                 'openshift_schedulable=True')
         self.assert_inventory_host_var_unset(inventory, 'nodes', '10.0.0.3',
-                                 'openshift_schedulable')
+                                 'openshift_schedulable=True')
 
 
 # TODO: test with config file, attended add node