Add find unused disk module and its unit tests #5

edamamez · 2018-06-21T15:16:56Z

See for in depth description: https://gist.github.com/dwlehman/eceec2fb443408080b70d4fe2ea70459

dwlehman

This looks good overall. You should probably run pylint on it (pylint is a command and is also the name of the package that provides that command in rhel/fedora). Feel free to ignore errors/warnings for too-long lines.

dwlehman · 2018-06-27T15:58:41Z

library/find_unused_disk_module.py

+
+# Ensure that no known signatures exist on the disk, with the exception of 
+# partition tables. (blkid -p <device>)
+def no_signature(run_command, disk):


In python the function-level documentation is called a docstring, and would look something like this:

def no_signature(run_command, disk): """ Return True if the disk contains no known signatures/metadata. """ ...

https://www.python.org/dev/peps/pep-0257/#one-line-docstrings gives a good overview of how docstrings should look for simple functions.

dwlehman · 2018-06-27T18:19:34Z

library/find_unused_disk_module.py

+    ansible_facts = facts.ansible_facts(module)
+    run_command = module.run_command
+    all_disks = run_command(['lsblk', '-o', 'kname', '-l', '-n', '-p'])
+    for disk in all_disks[1].split():


If you moved this top-level logic for identifying unused disks into a separate function you could easily cover it with a unit test.

dwlehman · 2018-06-27T18:21:22Z

library/find_unused_disk_module.py

+DOCUMENTATION = '''
+---
+module: find_unused_disk_module
+short_description: Get first unused disk


We probably need to return a list of unused disks rather than just one. Maybe return up to ten by default with an optional argument to set a max number of disks to return?

Sounds good! I'll work on that

dwlehman · 2018-06-27T18:23:27Z

library/find_unused_disk_module.py

+# Ensure that the disk has no holders to eliminate the possibility of it being a
+#  multipath or dmraid member device. (/sys/class/block/<device>/holders/ empty)
+def no_holders(run_command, disk_name):
+    holders = run_command(['ls', '/sys/class/block/' + disk_name + '/holders/'])


Use os.listdir[1] instead of `ls' here. In general, https://docs.python.org is your friend.

[1] https://docs.python.org/3/library/os.html#os.listdir

dwlehman · 2018-06-27T18:24:12Z

library/find_unused_disk_module.py

+def can_open(open, close, disk):
+    try:
+        exclusive = open(disk, os.O_EXCL)
+        close(exclusive)


I'm pretty sure that when exclusive goes out of scope (ie: when the function returns) it will be automatically closed.

dwlehman · 2018-06-27T18:28:17Z

library/find_unused_disk_module.py

+    for disk in all_disks[1].split():
+        disk_name = os.path.basename(disk)
+        # Ensure that, if there is a partition table on the disk, it contains no partitions.    
+        try:


A more typical way to do this would be something like this:

if (not ansible_facts['devices'][disk_name]['partitions'] and no_signature(run_command, disk) and no_holders(run_command, disk_name) and can_open(os.open, os.close, disk)): result['disk'] = disk module.exit_json(**result)

I made it a try-except because some devices won't have a 'partitions' variable and the command ansible_facts['devices'][disk_name]['partitions'] will fail with some form of string not found exception. Although I agree that it is messier...

In that case maybe do something like this:

try: has_partitions = ansible_facts['devices'][disk_name]['partitions'] except Exception: has_partitions = False if (not has_partitions and no_signature(run_command, disk) and ...

It's good practice to isolate only the code that directly causes and handles the exception within the try/except blocks. It makes the flow clearer, among other things. Ideally, find out what exceptions you expect it to raise and only catch those.

Bah, that isn't quite right. In the try you'd have to coerce that to a bool, so something like:

has_partitions = (ansible_facts[...] == dict()) or something along those lines.

Better yet, just do it explicitly: has_partitions = bool(ansible_facts[...])

That's really smart, done!

edamamez · 2018-06-28T23:02:38Z

When running pylint, I get warnings about putting the Imports at the top of the module. However, since Ansible documentation clearly states otherwise, I am disregarding them as well as the missing docstring at beginning of file error since the module is explained according to Ansible. (https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html)

edamamez · 2018-06-28T23:06:46Z

All done making edits for first round of comments, let me know how it looks!

dwlehman

Many of these things could have been pointed out in the last review -- sorry for that.

It looks good other than these few little things.

dwlehman · 2018-06-28T21:49:44Z

library/find_unused_disk_module.py

+            has_partitions = False
+        # Check if partition table contains no partitions.
+        if has_partitions and len(ansible_facts['devices'][disk_name]['partitions']) == 0:
+            has_partitions = False


The has_partitions bool should already be False for an empty dict or list, so this shouldn't be necessary.

dwlehman · 2018-07-02T18:39:31Z

library/find_unused_disk_module.py

+
+def get_disks(run_command):
+    """Return list of devices on the system"""
+    all_disks = run_command(['lsblk', '-o', 'kname', '-l', '-n', '-p'])


Okay, sorry for this -- I should have noticed it the first time through -- but there are other, better ways to do this.

For one thing, that command will include partitions, which are necessarily not disks.

You could get the exact same list by running os.listdir('/sys/class/block'); it's better in that you're using a system call instead of running an executable, but you'll still have the partitions. You could avoid that by instead listing '/sys/block' (Yes, this one contains a subset of the former. Yes, that's a bit goofy.)

Another option that would eliminate the partitions is to use ansible_facts['devices'].keys(). It also omits the partitions (it hides them under the disks that contain them). Again, this is better than running an executable since you're just looking at data that's already been gathered.

There might be more work to do here since it's not always trivial to figure out what is and is not a "disk". That can be addressed later, though.

I use the complete device paths for some of the other commands (os.open, blkid), but the commands that returns only disks (that you suggested) list them by just name. Would it be wrong to assume their location? For example, if ansible_facts['devices'].keys() returns sda and dm-0, would it be ok to assume they are located at /dev/sda and /dev/dm-0?

It is safe to assume that they will all be in /dev. The ansible facts get the list of devices by looking at /sys/block, which lists "kernel" devices (always in /dev) -- see [1].

[1] https://github.com/ansible/ansible/blob/0691c9c099d56929321412df1f5842423a2dfdc0/lib/ansible/module_utils/facts/hardware/linux.py#L548

dwlehman · 2018-07-02T18:43:03Z

library/find_unused_disk_module.py

+        if not has_partitions and no_signature(run_command, disk) and no_holders(disk_name) and can_open(disk):
+            result['disk'].append(disk)
+            disk_count = disk_count + 1
+            if disk_count >= module.params['max_return']:


You could eliminate disk_count and instead check len(result['disk']).

dwlehman · 2018-07-02T18:44:24Z

tests/unit/unused_disk.py

+def test_can_open_false(monkeypatch):
+    def mock_return(args, flag):
+        raise OSError
+        return -1


This (return -1) will never run since you are raising an exception beforehand.

dwlehman reviewed Jun 27, 2018

View reviewed changes

dwlehman requested changes Jul 2, 2018

View reviewed changes

edamamez force-pushed the eda_edits branch from 3653cc2 to e76b93b Compare July 3, 2018 19:46

dwlehman approved these changes Jul 3, 2018

View reviewed changes

Add find unused disk module and its unit tests.

fffba63

edamamez force-pushed the eda_edits branch from e76b93b to fffba63 Compare July 3, 2018 19:58

dwlehman merged commit 212d3a1 into dwlehman:master Jul 3, 2018

dwlehman mentioned this pull request Jul 25, 2018

Make use of unused disk module for default disk set #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add find unused disk module and its unit tests #5

Add find unused disk module and its unit tests #5

edamamez commented Jun 21, 2018 •

edited

Loading

dwlehman left a comment

dwlehman Jun 27, 2018

dwlehman Jun 27, 2018

dwlehman Jun 27, 2018

edamamez Jun 27, 2018

dwlehman Jun 27, 2018

dwlehman Jun 27, 2018

dwlehman Jun 27, 2018

edamamez Jun 27, 2018 •

edited

Loading

dwlehman Jun 28, 2018

dwlehman Jun 28, 2018

dwlehman Jun 28, 2018

edamamez Jun 28, 2018

edamamez commented Jun 28, 2018

edamamez commented Jun 28, 2018

dwlehman left a comment

dwlehman Jun 28, 2018

dwlehman Jul 2, 2018

edamamez Jul 3, 2018 •

edited

Loading

dwlehman Jul 3, 2018

dwlehman Jul 2, 2018

dwlehman Jul 2, 2018

Add find unused disk module and its unit tests #5

Add find unused disk module and its unit tests #5

Conversation

edamamez commented Jun 21, 2018 • edited Loading

dwlehman 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

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

edamamez Jun 27, 2018 • edited Loading

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

edamamez commented Jun 28, 2018

edamamez commented Jun 28, 2018

dwlehman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edamamez Jul 3, 2018 • edited Loading

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

edamamez commented Jun 21, 2018 •

edited

Loading

edamamez Jun 27, 2018 •

edited

Loading

edamamez Jul 3, 2018 •

edited

Loading