vdo: module fails when modifying VDO 6.2 volumes without read cache

[bgurney-rh]

SUMMARY

If a "creation" playbook (i.e.: "state:present" when the VDO volume already exists) is executed with VDO 6.2, the playbook execution fails with an error on vdo.py line 611: "KeyError: 'Read cache'".

ISSUE TYPE

• Bug Report

COMPONENT NAME

vdo

ANSIBLE VERSION

ansible 2.7.8
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.6/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.6.8 (default, Jan 11 2019, 02:17:16) [GCC 8.2.1 20180905 (Red Hat 8.2.1-3)]

CONFIGURATION

OS / ENVIRONMENT

vdo-6.2.0.293 on Red Hat Enterprise Linux 8.0

STEPS TO REPRODUCE

If a "state:present" playbook is executed for a VDO volume that already exists, ideally, it should finish with "ok=2 changed=0". However, for VDO 6.2, where there is no longer a "Read cache" or "Read cache size" parameter, the playbook execution will fail with this error:

(vdo.py) line 611, in run_module: KeyError: 'Read cache'

(However, note that creating or removing a VDO volume works normally.)

- hosts: vdoClients
  remote_user: username
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Create VDO volume vdo1
    vdo:
      name: vdo1
      state: present
      device: /dev/blockdevice
      logicalsize: 100G

- hosts: vdoClients
  remote_user: username
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Remove VDO volume vdo1
    vdo:
      name: vdo1
      state: absent

EXPECTED RESULTS

For a playbook where no changes are made, the playbook succeeds with "ok=2 changed=0"

PLAY RECAP *********************************************************************
192.168.122.39             : ok=2    changed=0    unreachable=0    failed=0   

For a playbook where changes are made, the playbook succeeds with "ok=2 changed=1"

ACTUAL RESULTS

When running the "Create VDO volume vdo1" playbook after vdo1 has been created (i.e.: in a situation where a parameter is being modified, or where no parameters are being modified), the playbook fails with "(vdo.py) line 611, in run_module: KeyError: 'Read cache'" because VDO 6.2 does not have the read cache feature, therefore the key is not present in the "vdo status" YAML output.

The failing line of code is here:

        for statfield in statusparamkeys:
611         currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
            modtrans[statfield] = vdokeytrans[statfield]

If I patch the module with this workaround, I get a successful run of "ok=2 chan
ged=0" for a "modify" run (i.e.: "status=present" where the entity already exists):

@@ -570,16 +570,16 @@
         # The 'vdo status' keys that are currently modifiable.
         statusparamkeys = ['Acknowledgement threads',
                            'Bio submission threads',
                            'Block map cache size',
                            'CPU-work threads',
                            'Logical threads',
                            'Physical threads',
-                           'Read cache',
-                           'Read cache size',
+                         # 'Read cache',
+                         # 'Read cache size',
                            'Configured write policy',
                            'Compression',
                            'Deduplication']

         # A key translation table from 'vdo status' output to Ansible
         # module parameters.  This covers all of the 'vdo status'
         # parameter keys that could be modified with the 'vdo'

However, I want the module to be able to work with VDO versions that are both 6.2+, and older than 6.2. Is there a way to remove the "Read cache" and "Read cache size" keys from the "statusparamkeys" dictionary if they're not in the "vdo status" output? (This would be "processedvdos[desiredvdo]".)

File "/tmp/ansible_vdo_payload_40z7m_vy__main__.py", line 611, in run_module
KeyError: 'Read cache'
...
	to retry, use: --limit @/root/ansible_scratch/test_vdocreate.retry

PLAY RECAP *********************************************************************
192.168.122.39             : ok=1    changed=0    unreachable=0    failed=1   

[bgurney-rh]

Ideally, playbooks without the "readcache" parameter should work normally with VDO 6.2.

[bgurney-rh]

I tried this modification to the original:

@@ -608,8 +608,9 @@
         modtrans = {}
 
         for statfield in statusparamkeys:
-            currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
-            modtrans[statfield] = vdokeytrans[statfield]
+            if statfield in processedvdos[desiredvdo]:
+                currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
+                modtrans[statfield] = vdokeytrans[statfield]
 
         # Build a dictionary of current parameters formatted with the
         # same keys as the AnsibleModule parameters.

It successfully prevents the KeyError listed above, and a "create" playbook with the same options successfully completes with "ok=2 changed=0", as expected.

Trying to use a "create" playbook with the missing feature (in this case, "readcache" and "readcachesize")...

- hosts: vdoClients
  remote_user: root
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Create VDO volume vdo1
    vdo:
      name: vdo1
      state: present
      device: /dev/sdb
      logicalsize: 100G
      readcache: enabled
      readcachesize: 64M

...fails with a message: "vdo create: error: unrecognized arguments: --readCache=enabled --readCacheSize=64M". This is good, because it lets the user know that there were unrecognized playbook parameters.

However, if trying to "modify" an existing VDO volume with the "added" read cache parameters, this new patch filters out and ignores those options. Ideally, this would be the same "unrecognized arguments" message as seen above, but at least it's more graceful than the original failure case.

[samdoran]

I don't know vdo specifically, but the general technique you need to use is a version check or feature probe. It could be something as simple as function that runs vdo --version, pareses it, and gives you a number. You can then use that information to get the right list of statusparamkeys or maybe removes certains keys if >= version 6.2.

Here is an example in the git module using distutils.version.LooseVersion

[bcoca]

worst case, search for unrecognized arguments in your return and inform the user that their version does not support all the options used.

[bgurney-rh]

If the vdo command detects that an argument is unrecognized, it will already return an error.

However, in this case, the options that are not supported in the current version of VDO (i.e.: do not display in "vdo status", among the strings enumerated in "statusparamkeys") will result in the KeyError.

So I added this line, to filter on only the status parameter keys that exist in "vdo status":

            if statfield in processedvdos[desiredvdo]:

I have an idea: would it be possible to generate a list of the status parameter keys that are not in the "vdo status" output, that were specified in the playbook, and then "add" them to the "diffparams" entry, to then send to the "vdo modify" command (i.e.: "vdocmdoptions = add_vdooptions(diffparams)", etc.)? At that point, the "vdo" command would receive the parameters, including the "missing" parameters not supported by that version, and fail.

(I guess this is a long-winded way of saying: I was trying to avoid a version check, mostly because there is no "vdo --version" command. One can check the version of VDO installed by "rpm -qa vdo".)

[samdoran]

I was trying to avoid a version check, mostly because there is no "vdo --version" command.

If you can't do a version check, you can do a feature probe. This essentially means run the command and check the output or error code, as @bcoca suggested. Use this information to add in the extra options, if supported.

base_opts = ['--one', '--two']      # always supported options
extra_opts = ['--three', '--four']  # only supported in new version
vdo_bin = self.module.get_bin_path('vdo', True)
supports_extra_opts = False

# Feature probe
cmd = [vdo_bin]
cmd.extend(extra_opts)
rc, out, err = module.run_command(cmd)
if 'unrecognized arguments' not in err:
    supports_extra_opts = True

...

cmd = [vdo_bin]
cmd.extend(['--arg', module.params['someparam'])
if supports_extra_opts:
    cmd.extend(extra_opts)

[bgurney-rh]

I seem to have found a fix.

(Ignore the "result['foo'] = foo" lines; I used those with "ansible-playbook -vvv ..." to check the contents of a few dictionaries during exectuion)

--- orig_vdo.py	2019-03-28 14:10:42.180261167 -0400
+++ mod2_vdo.py	2019-04-05 15:00:06.132261167 -0400
@@ -608,14 +608,20 @@
         modtrans = {}
 
         for statfield in statusparamkeys:
-            currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
-            modtrans[statfield] = vdokeytrans[statfield]
+            if statfield in processedvdos[desiredvdo]:
+                currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
+                modtrans[statfield] = vdokeytrans[statfield]
+
+            if statfield not in processedvdos[desiredvdo]:
+                modtrans[statfield] = vdokeytrans[statfield]
+
+        result['modtrans'] = modtrans
 
         # Build a dictionary of current parameters formatted with the
         # same keys as the AnsibleModule parameters.
         currentparams = {}
-        for paramkey in currentvdoparams.keys():
-            currentparams[modtrans[paramkey]] = currentvdoparams[paramkey]
+        for paramkey in modtrans.keys():
+            currentparams[modtrans[paramkey]] = modtrans[paramkey]
 
         diffparams = {}
 
@@ -628,7 +634,11 @@
                 if str(currentparams[key]) != module.params[key]:
                     diffparams[key] = module.params[key]
 
+        result['currentparams'] = currentparams
+        result['currentvdoparams'] = currentvdoparams
+
         if diffparams:
+            result['diffparams'] = diffparams
             vdocmdoptions = add_vdooptions(diffparams)
             if vdocmdoptions:
                 rc, out, err = module.run_command("%s modify --name=%s %s"

So my original theory when building the module was to filter out any "unsupported" parameters. However, when the "Read cache" and "Read cache size" fields in "vdo status" were no longer present in VDO 6.2, that resulted in the "KeyError". Therefore, use "if statfield in processedvdos[desiredvdo]:" to filter the modifiable parameters with the ones that are supported in this VDO version, then use "if statfield not in processedvdos[desiredvdo]:" to evade the KeyError, and add the "unsupported" parameters.

Then, change "for paramkey in currentvdoparams.keys():" to "for paramkey in modtrans.keys():", in order to populate "currentparams" with the parameters that could potentially be modified, to then be processed by this part in vdo.py, where "diffparams" is loaded with "different" parameters; later, "add_vdooptions(diffparams)" is executed.

        diffparams = {}

        # Check for differences between the playbook parameters and the
        # current parameters.  This will need a comparison function;
        # since AnsibleModule params are all strings, compare them as
        # strings (but if it's None; skip).
        for key in currentparams.keys():
            if module.params[key] is not None:
                if str(currentparams[key]) != module.params[key]:
                    diffparams[key] = module.params[key]


        if diffparams:
            vdocmdoptions = add_vdooptions(diffparams)
            if vdocmdoptions:
...

Test results, using the following test playbooks on VDO 6.2 (using the "Read cache" options, which VDO 6.2 will not have):

# cat test_vdocreate.yml 
- hosts: vdoClients
  remote_user: username
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Create VDO volume vdo1
    vdo:
      name: vdo1
      state: present
      device: /dev/sdb
      logicalsize: 100G
# cat test_vdoremove.yml 
- hosts: vdoClients
  remote_user: username
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Remove VDO volume vdo1
    vdo:
      name: vdo1
      state: absent
# cat test_vdocreate_withreadcache.yml 
- hosts: vdoClients
  remote_user: username
  become: yes
  become_method: sudo
  connection: ssh
  gather_facts: yes
  tasks:
  - name: Create VDO volume vdo1
    vdo:
      name: vdo1
      state: present
      device: /dev/sdb
      logicalsize: 100G
      readcache: enabled
      readcachesize: 64M

Stage 1: execute test_vdocreate.yml with no prior VDO volume: "ok=2 changed=1" (good; VDO volume created.)

Stage 2: execute test_vdocreate.yml with prior VDO volume: "ok=2 changed=0" (good; VDO volume remained, with no changes.)

Stage 3: execute test_vdoremove.yml with prior VDO volume: "ok=2 changed=1" (good; VDO volume was removed.)

Stage 4: execute test_vdoremove.yml with no prior VDO volume: "ok=2 changed=0" (good: no VDO volume existed, and no changes.

Stage 5: execute test_vdocreate_withreadcache.yml with no prior VDO volume: "ok=1 changed=0 unreachable=0 failed=1"

(failure message: stderr from command: "...vdo create: error: unrecognized arguments: --readCache=enabled --readCacheSize=64M")
(result: GOOD; the "vdo" command detected unrecognized arguments.)

Stage 6: execute test_vdocreate.yml with no prior VDO volume (to create a VDO volume for the next step)
(result: good; same as Stage 1, but this is really just a preparation for the next step)

Stage 7: execute test_vdocreate_withreadcache.yml with a prior VDO volume: "ok=1 changed=0 unreachable=0 failed=1"
(failure message: stderr from command: "...vdo modify: error: unrecognized arguments: --readCache=enabled --readCacheSize=64M")
(result: GOOD: the "vdo" command detected unrecognized arguments.)

[bgurney-rh]

Updated patch that fixes the bug (without the extra "result['foo'] = foo" lines):

--- orig_vdo.py	2019-03-28 14:10:42.180261167 -0400
+++ modfinal_vdo.py	2019-04-08 10:28:45.285261167 -0400
@@ -608,14 +608,18 @@
         modtrans = {}
 
         for statfield in statusparamkeys:
-            currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
-            modtrans[statfield] = vdokeytrans[statfield]
+            if statfield in processedvdos[desiredvdo]:
+                currentvdoparams[statfield] = processedvdos[desiredvdo][statfield]
+                modtrans[statfield] = vdokeytrans[statfield]
+
+            if statfield not in processedvdos[desiredvdo]:
+                modtrans[statfield] = vdokeytrans[statfield]
 
         # Build a dictionary of current parameters formatted with the
         # same keys as the AnsibleModule parameters.
         currentparams = {}
-        for paramkey in currentvdoparams.keys():
-            currentparams[modtrans[paramkey]] = currentvdoparams[paramkey]
+        for paramkey in modtrans.keys():
+            currentparams[modtrans[paramkey]] = modtrans[paramkey]
 
         diffparams = {}