List correctly current PV in "lvg" module: fix lvg reduce (#49731)

* Refactor integration test for lvg module to introduce grow/reduce test

* List correctly current PV in lvg module: fix lvg reduce

Previous behaviour was to only take into account PV passed in 'pvs'
argument. This lead to reduce not working as expecting:

* with state=present and list of wanted pvs, lvg found only the pvs to
  add or already present and ignored the pv to remove (obviously absent
  from the list of given PV)

* with state=absent and a pv to remove, lvg found that the remaining pvs
  list is empty (ignoring possible other PV in the vg) and decides to
  remove the vg entirely (as supposely no PV are left anymore to store
  lvm metadata)

* Add changelog fragment

changelogs/fragments/fix-lvg-reduce-by-better-listing-pv.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- lvg - Take into account current PV in the VG to fix PV removal

lib/ansible/modules/system/lvg.py

@@ -86,6 +86,7 @@ EXAMPLES = '''
     state: absent
 '''
 
+import itertools
 import os
 
 from ansible.module_utils.basic import AnsibleModule
@@ -167,8 +168,12 @@ def main():
         # get pv list
         pvs_cmd = module.get_bin_path('pvs', True)
         if dev_list:
-            pvs_filter = ' || '. join(['pv_name = {0}'.format(x) for x in dev_list + module.params['pvs']])
-            pvs_filter = "--select '%s'" % pvs_filter
+            pvs_filter_pv_name = ' || '.join(
+                'pv_name = {0}'.format(x)
+                for x in itertools.chain(dev_list, module.params['pvs'])
+            )
+            pvs_filter_vg_name = 'vg_name = {0}'.format(vg)
+            pvs_filter = "--select '{0} || {1}' ".format(pvs_filter_pv_name, pvs_filter_vg_name)
         else:
             pvs_filter = ''
         rc, current_pvs, err = module.run_command("%s --noheadings -o pv_name,vg_name --separator ';' %s" % (pvs_cmd, pvs_filter))

test/integration/targets/lvg/tasks/main.yml

@@ -6,43 +6,10 @@
 
 - name: Test lvg module
   block:
-    - name: Create file to use as a disk device
-      command: "dd if=/dev/zero of={{ ansible_user_dir }}/ansible_testing/img1 bs=1M count=10"
+    - import_tasks: setup.yml
 
-    - name: Create loop device for file
-      command: "losetup --show -f {{ ansible_user_dir }}/ansible_testing/img1"
-      register: loop_device1
-
-    - name: Create volume group on disk device
-      lvg:
-        vg: testvg
-        pvs: "{{ loop_device1.stdout }}"
-
-    - name: Create the volume group again to verify idempotence
-      lvg:
-        vg: testvg
-        pvs: "{{ loop_device1.stdout }}"
-      register: repeat_vg_create
-
-    - name: Do all assertions to verify expected results
-      assert:
-        that:
-          - repeat_vg_create is not changed
+    - import_tasks: test_indempotency.yml
 
+    - import_tasks: test_grow_reduce.yml
   always:
-    - name: Remove test volume group
-      lvg:
-        vg: testvg
-        state: absent
-
-    - name: Detach loop device
-      command: "losetup -d {{ loop_device1.stdout }}"
-      when:
-        - loop_device1 is defined
-        - loop_device1.stdout is defined
-        - loop_device1.stdout is match("/dev/.*")
-
-    - name: Remove the file
-      file:
-        path: "{{ ansible_user_dir }}/ansible_testing/img1"
-        state: absent
+    - import_tasks: teardown.yml

test/integration/targets/lvg/tasks/setup.yml

@@ -0,0 +1,13 @@
+- name: "Create files to use as a disk devices"
+  command: "dd if=/dev/zero of={{ ansible_user_dir }}/ansible_testing/img{{ item }} bs=1M count=10"
+  with_sequence: 'count=2'
+
+- name: "Create loop device for file"
+  command: "losetup --show -f {{ ansible_user_dir }}/ansible_testing/img{{ item }}"
+  with_sequence: 'count=2'
+  register: loop_devices
+
+- name: "Affect name on disk to work on"
+  set_fact:
+    loop_device1: "{{ loop_devices.results[0] }}"
+    loop_device2: "{{ loop_devices.results[1] }}"

test/integration/targets/lvg/tasks/teardown.yml

@@ -0,0 +1,17 @@
+- name: Remove test volume group
+  lvg:
+    vg: testvg
+    state: absent
+
+- name: Detach loop device
+  command: "losetup -d {{ item.stdout }}"
+  loop: "{{ loop_devices.results|default([]) }}"
+  when:
+    - item.stdout is defined
+    - item.stdout is match("/dev/.*")
+
+- name: Remove device files
+  file:
+    path: "{{ ansible_user_dir }}/ansible_testing/img{{ item }}"
+    state: absent
+  with_sequence: 'count={{ loop_devices.results|length }}'

test/integration/targets/lvg/tasks/test_grow_reduce.yml

@@ -0,0 +1,33 @@
+- name: "Create volume group on first disk"
+  lvg:
+    vg: testvg
+    pvs: "{{ loop_device1.stdout }}"
+
+- name: "get lvm facts"
+  setup:
+
+- debug: var=ansible_lvm
+
+- name: "Assert the testvg span only on first disk"
+  assert:
+    that:
+      - ansible_lvm.pvs[loop_device1.stdout].vg == "testvg"
+      - 'loop_device2.stdout not in ansible_lvm.pvs or
+         ansible_lvm.pvs[loop_device2.stdout].vg == ""'
+
+- name: "Extend to second disk AND reduce from the first disk"
+  lvg:
+    vg: testvg
+    pvs: "{{ loop_device2.stdout }}"
+
+- name: "get lvm facts"
+  setup:
+
+- debug: var=ansible_lvm
+
+- name: "Assert the testvg span only on first disk"
+  assert:
+    that:
+      - 'loop_device1.stdout not in ansible_lvm.pvs or
+         ansible_lvm.pvs[loop_device1.stdout].vg == ""'
+      - ansible_lvm.pvs[loop_device2.stdout].vg == "testvg"

test/integration/targets/lvg/tasks/test_indempotency.yml

@@ -0,0 +1,15 @@
+- name: Create volume group on disk device
+  lvg:
+    vg: testvg
+    pvs: "{{ loop_device1.stdout }}"
+
+- name: Create the volume group again to verify idempotence
+  lvg:
+    vg: testvg
+    pvs: "{{ loop_device1.stdout }}"
+  register: repeat_vg_create
+
+- name: Do all assertions to verify expected results
+  assert:
+    that:
+      - repeat_vg_create is not changed