Replaced expensive get_server() and fixed issues in server module

openstacksdk's get_server() [1] calls add_server_interfaces() [2]
which queries OpenStack APIs several times to get all ports and
floating ips attached to a server.

Now we call openstacksdk's compute.find_server() [3] and compute.\
get_server() [4] which result in two API calls, in order to fill
server['addresses'] attribute which we later use to get floating ip
addresses attached to the server.

Do an extra call to compute.get_server() in order to return a pristine
server resource, because openstacksdk's create_server() might call
meta.add_server_interfaces() which alters server attributes such as
server['addresses'] [5].

Fail if options 'auto_ip', 'floating_ips' or 'floating_ip_pools'
are specified but 'wait' is not set to true, because openstacksdk
will add floating ip addresses only if we wait until the server
has been created [6]. This conditional fail will help users to not
shoot their foot.

Marked floating ip support unstable in this module due to various
unresolved issues in openstacksdk's add_ips_to_server() function
such as [9] and [10].

For Zuul CI job ansible-collections-openstack-functional-devstack-\
releases to pass, the minimum required openstacksdk release must be
0.101.0 because [7],[8] are available since that release only.

[1] 3f81d0001d/openstack/cloud/_compute.py (L484)
[2] 3f81d0001d/openstack/cloud/meta.py (L439)
[3] 3f81d0001d/openstack/compute/v2/_proxy.py (L652)
[4] 3f81d0001d/openstack/compute/v2/_proxy.py (L666)
[5] 3f81d0001d/openstack/cloud/_compute.py (L942)
[6] 3f81d0001d/openstack/cloud/_compute.py (L945)
[7] https://review.opendev.org/c/openstack/openstacksdk/+/851976
[8] 0ded7ac398
[9] https://storyboard.openstack.org/#!/story/2010352
[10] https://storyboard.openstack.org/#!/story/2010153

Change-Id: I6a5663433b1b9529f99d5eced22a28c692a1d288

ci/roles/server/tasks/main.yml

@@ -29,6 +29,18 @@
     network_name: "{{ server_alt_network }}"
     state: present
 
+- name: Create router 1 (for attaching floating ips)
+  openstack.cloud.router:
+    cloud: "{{ cloud }}"
+    state: present
+    name: ansible_router1
+    network: public
+    interfaces:
+        - net: "{{ server_network }}"
+          subnet: "{{ server_subnet }}"
+        - net: "{{ server_alt_network }}"
+          subnet: "{{ server_alt_subnet }}"
+
 - name: Create security group for server
   openstack.cloud.security_group:
     cloud: "{{ cloud }}"
@@ -284,6 +296,68 @@
     name: "{{ server_name }}"
     wait: true
 
+- name: Create server on private network with auto_ip
+  openstack.cloud.server:
+    auto_ip: true
+    cloud: "{{ cloud }}"
+    flavor: "{{ flavor }}"
+    image: "{{ image }}"
+    name: "{{ server_name }}"
+    nics:
+      - net-name: "{{ server_network }}"
+    reuse_ips: false
+    state: present
+    wait: true
+  register: server
+
+- name: Assert server on private network with auto_ip
+  assert:
+    that:
+    - server.server.addresses.values()
+      |flatten(levels=1)
+      |selectattr('OS-EXT-IPS:type', 'equalto', 'floating')
+      |map(attribute='addr')
+      |list|length == 1
+
+- name: Delete server on private network with auto_ip
+  openstack.cloud.server:
+    cloud: "{{ cloud }}"
+    state: absent
+    name: "{{ server_name }}"
+    wait: true
+
+- name: Create server on public network
+  openstack.cloud.server:
+    auto_ip: false
+    cloud: "{{ cloud }}"
+    flavor: "{{ flavor }}"
+    image: "{{ image }}"
+    name: "{{ server_name }}"
+    nics:
+      - net-name: 'public'
+    reuse_ips: false
+    state: present
+    wait: true
+  register: server
+
+- debug: var=server
+
+- name: Assert server on public network
+  assert:
+    that:
+    - server.server.addresses.values()
+      |flatten(levels=1)
+      |selectattr('OS-EXT-IPS:type', 'equalto', 'floating')
+      |map(attribute='addr')
+      |list|length == 0
+
+- name: Delete server on public network
+  openstack.cloud.server:
+    cloud: "{{ cloud }}"
+    state: absent
+    name: "{{ server_name }}"
+    wait: true
+
 - name: Create port to be attached to server
   openstack.cloud.port:
     cloud: "{{ cloud }}"
@@ -310,18 +384,46 @@
       key2: value2
     name: "{{ server_name }}"
     nics:
-      - net-name: 'public'
       - net-name: "{{ server_network }}"
-      - port-id: '{{ port.port.id }}'
+      - port-id: "{{ port.port.id }}"
+    reuse_ips: false
     state: present
     wait: true
   register: server
 
 - debug: var=server
 
+- name: Assert server is not on public network and does not have a floating ip
+  assert:
+    that:
+      - server.server.addresses.keys()|sort == [server_network]|sort
+      - server.server.addresses.values()
+        |flatten(levels=1)
+        |selectattr('OS-EXT-IPS:type', 'equalto', 'floating')
+        |map(attribute='addr')
+        |list|length == 0
+
+- name: Find all floating ips for debugging
+  openstack.cloud.floating_ip_info:
+    cloud: "{{ cloud }}"
+  register: fips
+
+- name: Print all floating ips for debugging
+  debug: var=fips
+
+- name: Find all servers for debugging
+  openstack.cloud.server_info:
+    cloud: "{{ cloud }}"
+  register: servers
+
+- name: Print all servers for debugging
+  debug: var=servers
+
 - name: Update server
   openstack.cloud.server:
-    auto_ip: true
+    # TODO: Change auto_ip to true once openstacksdk's issue #2010352 has been solved
+    # Ref.: https://storyboard.openstack.org/#!/story/2010352
+    auto_ip: false
     cloud: "{{ cloud }}"
     description: "This server got updated"
     # flavor cannot be updated but must be present
@@ -334,9 +436,9 @@
     name: "{{ server_name }}"
     # nics cannot be updated
     nics:
-      - net-name: 'public'
       - net-name: "{{ server_network }}"
-      - port-id: '{{ port.port.id }}'
+      - port-id: "{{ port.port.id }}"
+    reuse_ips: false
     security_groups:
       - '{{ server_security_group }}'
       - '{{ server_alt_security_group }}'
@@ -355,16 +457,59 @@
       - "'key1' not in server_updated.server.metadata"
       - server_updated.server.metadata['key2'] == 'value2'
       - server_updated.server.metadata['key3'] == 'value3'
-      - server_updated.server.addresses.keys()|sort == [server_network,'public']
-      - server_updated.server.addresses[server_network]|length == 2
-      - server_updated.server.addresses.public|length > 0
-      - port.port.fixed_ips[0].ip_address in
-        server_updated.server.addresses[server_network]|map(attribute='addr')
       - server_updated.server.security_groups|map(attribute='name')|unique|length == 2
       - security_group.secgroup.name in server_updated.server.security_groups|map(attribute='name')
       - security_group_alt.secgroup.name in server_updated.server.security_groups|map(attribute='name')
+      - server_network in server_updated.server.addresses.keys()|list|sort
+      - server_updated.server.addresses[server_network]|length == 2
+      - port.port.fixed_ips[0].ip_address in
+        server_updated.server.addresses[server_network]|map(attribute='addr')
+      # TODO: Verify networks once openstacksdk's issue #2010352 has been solved
+      # Ref.: https://storyboard.openstack.org/#!/story/2010352
+      #- server_updated.server.addresses.public|length > 0
+      #- (server_updated.server.addresses.keys()|sort == ([server_network, 'public']|sort))
+      #  or (server_updated.server.addresses.values()
+      #      |flatten(levels=1)
+      #      |selectattr('OS-EXT-IPS:type', 'equalto', 'floating')
+      #      |map(attribute='addr')
+      #      |list|length > 0)
 
 - name: Update server again
+  openstack.cloud.server:
+    # TODO: Change auto_ip to true once openstacksdk's issue #2010352 has been solved
+    # Ref.: https://storyboard.openstack.org/#!/story/2010352
+    auto_ip: false
+    cloud: "{{ cloud }}"
+    description: "This server got updated"
+    # flavor cannot be updated but must be present
+    flavor: "{{ flavor }}"
+    # image cannot be updated but must be present
+    image: "{{ image }}"
+    metadata:
+      key2: value2
+      key3: value3
+    name: "{{ server_name }}"
+    # nics cannot be updated
+    nics:
+      - net-name: "{{ server_network }}"
+      - port-id: "{{ port.port.id }}"
+    reuse_ips: false
+    security_groups:
+      - '{{ server_security_group }}'
+      - '{{ server_alt_security_group }}'
+    state: present
+    wait: true
+  register: server_updated_again
+
+- name: Assert server did not change
+  assert:
+    that:
+      - server.server.id == server_updated_again.server.id
+      - server_updated_again is not changed
+
+# TODO: Drop failure test once openstacksdk's issue #2010352 has been solved
+# Ref.: https://storyboard.openstack.org/#!/story/2010352
+- name: Update server again with auto_ip set to true
   openstack.cloud.server:
     auto_ip: true
     cloud: "{{ cloud }}"
@@ -379,21 +524,22 @@
     name: "{{ server_name }}"
     # nics cannot be updated
     nics:
-      - net-name: 'public'
       - net-name: "{{ server_network }}"
-      - port-id: '{{ port.port.id }}'
+      - port-id: "{{ port.port.id }}"
+    reuse_ips: false
     security_groups:
       - '{{ server_security_group }}'
       - '{{ server_alt_security_group }}'
     state: present
     wait: true
-  register: server_again
+  register: server_updated_again
+  ignore_errors: true
 
-- name: Assert server did not change
+- name: Assert server update succeeded or failed with expected error
   assert:
     that:
-      - server.server.id == server_again.server.id
+      - not server_updated_again.failed
-      - server_again is not changed
+        or ('was found matching your NAT destination network' in server_updated_again.msg)
 
 - name: Delete updated server
   openstack.cloud.server:
@@ -421,6 +567,12 @@
     state: absent
     name: "{{ server_security_group }}"
 
+- name: Delete router 1
+  openstack.cloud.router:
+    cloud: "{{ cloud }}"
+    state: absent
+    name: ansible_router1
+
 - name: Delete second subnet for server
   openstack.cloud.subnet:
     cloud: "{{ cloud }}"

plugins/modules/server.py

@@ -18,6 +18,10 @@ options:
     auto_ip:
       description:
         - Ensure instance has public ip however the cloud wants to do that.
+        - For example, the cloud could add a floating ip for the server or
+          attach the server to a public network.
+        - Requires I(wait) to be C(True) during server creation.
+        - Floating IP support is unstable in this module, use with caution.
       type: bool
       default: 'yes'
       aliases: ['auto_floating_ip', 'public_ip']
@@ -50,6 +54,7 @@ options:
       description:
         - When I(state) is C(absent) and this option is true, any floating IP
           address associated with this server will be deleted along with it.
+        - Floating IP support is unstable in this module, use with caution.
       type: bool
       aliases: ['delete_fip']
       default: 'no'
@@ -84,11 +89,15 @@ options:
     floating_ip_pools:
       description:
         - Name of floating IP pool from which to choose a floating IP.
+        - Requires I(wait) to be C(True) during server creation.
+        - Floating IP support is unstable in this module, use with caution.
       type: list
       elements: str
     floating_ips:
       description:
         - list of valid floating IPs that pre-exist to assign to this node.
+        - Requires I(wait) to be C(True) during server creation.
+        - Floating IP support is unstable in this module, use with caution.
       type: list
       elements: str
     image:
@@ -158,6 +167,7 @@ options:
           concurrent server creation, it is highly recommended to set this to
           false and to delete the floating ip associated with a server when
           the server is deleted using I(delete_ips).
+        - Floating IP support is unstable in this module, use with caution.
         - This server attribute cannot be updated.
       type: bool
       default: 'yes'
@@ -777,6 +787,7 @@ server:
             type: list
 '''
 from ansible_collections.openstack.cloud.plugins.module_utils.openstack import OpenStackModule
+import copy
 
 
 class ServerModule(OpenStackModule):
@@ -831,10 +842,10 @@ class ServerModule(OpenStackModule):
     def run(self):
         state = self.params['state']
 
-        # self.conn.get_server is required for server.addresses and
+        server = self.conn.compute.find_server(self.params['name'])
-        # server.interface_ip which self.conn.compute.find_server
+        if server:
-        # does not return
+            # fetch server details such as server['addresses']
-        server = self.conn.get_server(self.params['name'])
+            server = self.conn.compute.get_server(server)
 
         if self.ansible.check_mode:
             self.exit_json(changed=self._will_change(state, server))
@@ -850,16 +861,6 @@ class ServerModule(OpenStackModule):
             update = self._build_update(server)
             if update:
                 server = self._update(server, update)
-            else:
-                # drop attributes added in function
-                # openstacksdk.meta.add_server_interfaces()
-                # because all other branches do not return them
-                #
-                # addresses will be expanded by get_server's call to
-                # openstacksdk.meta.add_server_interfaces() but we
-                # cannot easily undo this so we ignore it
-                for k in ['public_v4', 'public_v6', 'interface_ip']:
-                    del server[k]
 
             self.exit_json(changed=bool(update),
                            server=server.to_dict(computed=False))
@@ -893,20 +894,18 @@ class ServerModule(OpenStackModule):
             # do not add or remove any floating ip.
             return {}
 
-        if (auto_ip and server['interface_ip']
+        # Get floating ip addresses attached to the server
-           and not (floating_ip_pools or floating_ips)):
+        ips = [interface_spec['addr']
+               for v in server['addresses'].values()
+               for interface_spec in v
+               if interface_spec.get('OS-EXT-IPS:type', None) == 'floating']
+
+        if (auto_ip and ips and not floating_ip_pools and not floating_ips):
             # Server has a floating ip address attached and
             # no specific floating ip has been requested,
             # so nothing to change.
             return {}
 
-        # Get floating ip addresses attached to the server
-        ips = [interface_spec['addr']
-               for v in server.addresses.values()
-               for interface_spec in v
-               if ('OS-EXT-IPS:type' in interface_spec
-                   and interface_spec['OS-EXT-IPS:type'] == 'floating')]
-
         if not ips:
             # One or multiple floating ips have been requested,
             # but none have been attached, so attach them.
@@ -1024,6 +1023,15 @@ class ServerModule(OpenStackModule):
         return update
 
     def _create(self):
+        for k in ['auto_ip', 'floating_ips', 'floating_ip_pools']:
+            if self.params[k] is not None \
+               and self.params['wait'] is False:
+                # floating ip addresses will only be added if
+                # we wait until the server has been created
+                # Ref.: https://opendev.org/openstack/openstacksdk/src/commit/3f81d0001dd994cde990d38f6e2671ee0694d7d5/openstack/cloud/_compute.py#L945
+                self.fail_json(
+                    msg="Option '{0}' requires 'wait: yes'".format(k))
+
         flavor_name_or_id = self.params['flavor']
 
         image_id = None
@@ -1063,7 +1071,12 @@ class ServerModule(OpenStackModule):
 
         server = self.conn.create_server(**args)
 
-        return server
+        # openstacksdk's create_server() might call meta.add_server_interfaces(
+        # ) which alters server attributes such as server['addresses']. So we
+        # do an extra call to compute.get_server() to return a clean server
+        # resource.
+        # Ref.: https://opendev.org/openstack/openstacksdk/src/commit/3f81d0001dd994cde990d38f6e2671ee0694d7d5/openstack/cloud/_compute.py#L942
+        return self.conn.compute.get_server(server)
 
     def _delete(self, server):
         self.conn.delete_server(
@@ -1077,15 +1090,15 @@ class ServerModule(OpenStackModule):
         server = self._update_server(server, update)
         # Refresh server attributes after security groups etc. have changed
         #
-        # self.conn.get_server() is unnecessary because server.addresses and
+        # Use compute.get_server() instead of compute.find_server()
-        # server.interface_ip are computed and hence not returned anyway
+        # to include server details
-        return self.conn.compute.find_server(name_or_id=server.id)
+        return self.conn.compute.get_server(server)
 
     def _update_ips(self, server, update):
         args = dict((k, self.params[k]) for k in ['wait', 'timeout'])
         ips = update.get('ips')
         if ips:
-            return self.conn.add_ips_to_server(server, **ips, **args)
+            server = self.conn.add_ips_to_server(server, **ips, **args)
 
         add_ips = update.get('add_ips')
         if add_ips:
@@ -1182,6 +1195,10 @@ class ServerModule(OpenStackModule):
                     net['net-name'], ignore_missing=False).id
                 # Replace net-name with net-id and keep optional nic args
                 # Ref.: https://github.com/ansible/ansible/pull/20969
+                #
+                # Delete net-name from a copy else it will
+                # disappear from Ansible's debug output
+                net = copy.deepcopy(net)
                 del net['net-name']
                 net['net-id'] = network_id
                 nics.append(net)
@@ -1192,6 +1209,10 @@ class ServerModule(OpenStackModule):
                     net['port-name'], ignore_missing=False).id
                 # Replace net-name with net-id and keep optional nic args
                 # Ref.: https://github.com/ansible/ansible/pull/20969
+                #
+                # Delete net-name from a copy else it will
+                # disappear from Ansible's debug output
+                net = copy.deepcopy(net)
                 del net['port-name']
                 net['port-id'] = port_id
                 nics.append(net)