Added directory_usage to utils #629

pklimai · 2016-11-28T21:16:52Z

In PyEZ filesystem utils we were missing a method to see the size of the particular directory (together with all subdirectories). I added directory_usage util to do that.

Unit test for this util added as well.

This parameter allows to modify login state machine behavior in case of a hung state.

This utility is similar to "show system directory-usage" Junos command (or UNIX "du" command). It allows to see how much space is taken by a particular directory on disk. Unit test added as well.

coveralls · 2016-11-28T21:19:33Z

Coverage increased (+0.2%) to 97.012% when pulling 059f271 on pklimai:master into b84b8a6 on Juniper:master.

stacywsmith

@pklimai Thanks for submitting this pull request. I think this method is a useful addition. Please consider my comments and then we will looking at merging this feature.

stacywsmith · 2016-11-29T19:24:34Z

lib/jnpr/junos/utils/fs.py

+    # directory_usage - filesystem directory usage
+    # -------------------------------------------------------------------------
+
+    def directory_usage(self, path="."):


Suggest adding a depth=0 argument to the method.

stacywsmith · 2016-11-29T19:34:02Z

lib/jnpr/junos/utils/fs.py

+        """
+        Returns the directory usage, similar to the unix "du" command.
+
+        :returns: approximate directory usage, including subdirectories, in bytes


Rather than returning a single integer, I suggest returning a dictionary similar to the return value of storage_usage(). The first level key would be directory name. The second level keys could be blocks, bytes, and size (where size would be the 'human-readable' value with units).

Returning this type of data structure allows the support of the depth > 0 situation and allows returning the values in multiple formats so the user can easily access their preferred format without having to perform conversion.

stacywsmith · 2016-11-29T19:35:17Z

lib/jnpr/junos/utils/fs.py

+
+        :returns: approximate directory usage, including subdirectories, in bytes
+        """
+        rsp = self._dev.rpc.get_directory_usage_information(path=path, depth="0")


Assuming depth is now an argument method per comment on line 271, change depth="0" to depth=str(depth).

stacywsmith · 2016-11-29T19:38:15Z

lib/jnpr/junos/utils/fs.py

+
+        used_space = used_space.strip()
+
+        multiplier = {


Calculate the bytes value using the number of blocks (the used-blocks attribute of the <used-space> element. Then it's simply: bytes = blocks * BLOCK_SIZE. You can define BLOCK_SIZE = 512 at the beginning of the method.

stacywsmith · 2016-11-29T19:39:34Z

lib/jnpr/junos/utils/fs.py

+        """
+        Returns the directory usage, similar to the unix "du" command.
+
+        :returns: approximate directory usage, including subdirectories, in bytes


If you implement my comments, then "approximate" can be quantified as 512-byte precision.

stacywsmith · 2016-11-29T19:43:00Z

lib/jnpr/junos/utils/fs.py

+
+        :returns: approximate directory usage, including subdirectories, in bytes
+        """
+        rsp = self._dev.rpc.get_directory_usage_information(path=path, depth="0")


There's a possibility this line will raise any of the exceptions raised when invoking an RPC. That's fine, but we might want to document it.

stacywsmith · 2016-11-29T19:45:11Z

lib/jnpr/junos/utils/fs.py

+        used_space = rsp.findtext("directory/used-space")
+
+        if used_space is None:
+            return None


I'm not sure returning None makes sense for a missing <used-space> element. Especially if you implement my recommendation on changing the return value to a dict. Instead, I suggest raising a RpcError exception.

pklimai · 2016-11-29T20:25:05Z

Hi Stacy,

Thanks for your excellent ideas, I will try to implement it.

The only thing I'm not sure is why you want to make "depth" the method parameter. I just want directory_usage method to return usage for directory provided in the "path".

So for example with depth=0 the return XML can be

lab@R1> show system directory-usage depth 0 | display xml   
<rpc-reply xmlns:junos="http://xml.juniper.net/junos/12.1X47/junos">
    <directory-usage-information>
        <directory>
            <directory-name>/cf/var/home/lab</directory-name>
            <used-space used-blocks="4356">
                2.1M
            </used-space>
        </directory>
    </directory-usage-information>
    <cli>
        <banner></banner>
    </cli>
</rpc-reply>

while the same for depth=1

lab@R1> show system directory-usage depth 1 | display xml    
<rpc-reply xmlns:junos="http://xml.juniper.net/junos/12.1X47/junos">
    <directory-usage-information>
        <directory>
            <directory-name>/cf/var/home/lab</directory-name>
            <directory>
                <directory-name>/cf/var/home/lab/ajer</directory-name>
                <used-space used-blocks="16">
                    8.0K
                </used-space>
            </directory>
            <directory>
                <directory-name>/cf/var/home/lab/.ssh</directory-name>
                <used-space used-blocks="8">
                    4.0K
                </used-space>
            </directory>
            <directory>
                <directory-name>/cf/var/home/lab/test</directory-name>
                <used-space used-blocks="3984">
                    1.9M
                </used-space>
            </directory>
            <directory>
                <directory-name>/cf/var/home/lab/ajer-trunks</directory-name>
                <used-space used-blocks="168">
                    84K
                </used-space>
            </directory>
            <used-space used-blocks="4356">
                2.1M
            </used-space>
        </directory>
    </directory-usage-information>
    <cli>
        <banner></banner>
    </cli>
</rpc-reply>

so there is a lot more information here, but all that is not needed - we still only need "directory/used-space" which is 2.1M anyway. So calling with other depths just consumes extra resources. Please tell me what you think.

Peter

stacywsmith · 2016-11-29T20:39:35Z

I understand your use case for the directory method, but I can also envision other use cases which can be handled by adding a depth argument. Really it's the same reason the Unix du command takes a depth argument.

Here's a quick example with du.

Sometimes I want the space consumed by all the home directories on the system:

% du -h -d 0 /var/home
 20K	/var/home

Other times I'd like a single call to tell me the space consumed by each user's home directory:

% du -h -d 1 /var/home
4.0K	/var/home/regress
4.0K	/var/home/fregress
4.0K	/var/home/remote
4.0K	/var/home/user
 20K	/var/home

Yes, you could invoke directory_usage() once for each subdirectory, but it's more efficient and easier to only have to invoke it once.

coveralls · 2016-11-29T22:36:24Z

Coverage increased (+0.2%) to 97.018% when pulling d2e6459 on pklimai:master into b84b8a6 on Juniper:master.

pklimai · 2016-11-29T22:40:39Z

Thanks Stacy, I think you are right and it is better to have a more general method.

Just pushed the changes, please review.

stacywsmith

Thanks for making the requested changes. Just a few more suggested changes after reviewing the most current diff.

stacywsmith · 2016-11-29T23:05:52Z

lib/jnpr/junos/utils/fs.py

-            'G': 1024**3,
-            'T': 1024**4,
-        }
+        for directory in dirs:


Change to for directory in rsp.findall('//directory'):

@stacywsmith findall('//directory') fails with error,

File "/usr/lib/python3/dist-packages/lxml/_elementpath.py", line 246, in _build_path_iterator raise SyntaxError("cannot use absolute path on element") SyntaxError: cannot use absolute path on element

that's why I was using xpath() initially. However findall('.//directory') works, so I will use it.

stacywsmith · 2016-11-29T23:06:27Z

lib/jnpr/junos/utils/fs.py


-        used_space = used_space.strip()
+        dirs = rsp.xpath("//directory")


Delete this line. Use findall() on line 290 instead.

stacywsmith · 2016-11-29T23:09:46Z

lib/jnpr/junos/utils/fs.py


-        used_space = rsp.findtext("directory/used-space")
+        rsp = self._dev.rpc.get_directory_usage_information(path=path, depth=str(depth))


Add normalize=True argument. So, line becomes:
rsp = self._dev.rpc.get_directory_usage_information(path=path, depth=str(depth), normalize=True)
This avoids having to call strip() on lines 291-293.

@stacywsmith using normalize=True was my initial intention, but then test test_directory_usage() fails with reason that is completely unclear to me:

Error Traceback (most recent call last): File "/usr/local/lib/python3.5/dist-packages/mock/mock.py", line 1305, in patched return func(*args, **keywargs) File "/home/peter/PycharmProjects/py-junos-eznc/tests/unit/utils/test_fs.py", line 272, in test_directory_usage self.assertEqual(self.fs.directory_usage(path="/var/tmp"), File "/home/peter/PycharmProjects/py-junos-eznc/lib/jnpr/junos/utils/fs.py", line 281, in directory_usage rsp = self._dev.rpc.get_directory_usage_information(path=path, depth=str(depth), normalize=True) File "/home/peter/PycharmProjects/py-junos-eznc/lib/jnpr/junos/rpcmeta.py", line 156, in _exec_rpc return self._junos.execute(rpc, **dec_args) File "/usr/local/lib/python3.5/dist-packages/mock/mock.py", line 1062, in __call__ return _mock_self._mock_call(*args, **kwargs) File "/usr/local/lib/python3.5/dist-packages/mock/mock.py", line 1128, in _mock_call ret_val = effect(*args, **kwargs) File "/home/peter/PycharmProjects/py-junos-eznc/tests/unit/utils/test_fs.py", line 338, in _mock_manager device_params = kwargs['device_params'] KeyError: 'device_params'

I see other utility methods use strip() rather extensively - I guess this is probably due to the same reason.

stacywsmith · 2016-11-29T23:14:20Z

lib/jnpr/junos/utils/fs.py

-            return None
+        if rsp.findtext("directory/used-space") is None:
+            # Likely, no such directory
+            raise RpcError(rsp=rsp)


I'm sorry. I know I suggested this change, but now I'm rethinking it.
This would only raise an RpcError if there was no <directory> element in the response that contained a <used-space> element. If there were multiple <directory> elements in the response, but only some of them were missing the <used-space> element, it wouldn't address that situation.

Now that you've modified to return a dictionary, we probably need to handle the exceptional conditions of missing elements within the loop over each <directory> element. I suggest removing lines 283-285.

stacywsmith · 2016-11-29T23:16:19Z

lib/jnpr/junos/utils/fs.py

-            'T': 1024**4,
-        }
+        for directory in dirs:
+            dir_name = directory.findtext("directory-name").strip()


Remove .strip() from the end. If findtext() doesn't find a matching element, it returns None. Calling strip() on a None value would cause an exception to be raised. We can avoid this situation by implementing the comment on line 281.

stacywsmith · 2016-11-29T23:18:37Z

lib/jnpr/junos/utils/fs.py

-            'T': 1024**4,
-        }
+        for directory in dirs:
+            dir_name = directory.findtext("directory-name").strip()


Need to check that the <directory-name> element was actually found. Otherwise, we will end up trying to add a key with a value of None to the dict. I suggest adding the following after line 291:

if dir_name is None: raise RpcError(rsp=rsp)

stacywsmith · 2016-11-29T23:26:25Z

lib/jnpr/junos/utils/fs.py

+                "size": dir_size,
+                "blocks": dir_blocks,
+                "bytes": dir_blocks * BLOCK_SIZE,
+            }


We need to account for the possibility that the <used-space> element (or used-blocks attribute) doesn't exist. I suggest replacing lines 292-298 with:

used_space = directory.find('used-space') if used_space is not None: dir_size = used_space.text() dir_blocks = used_space.get('used-blocks') if dir_blocks is not None: dir_blocks = int(dir_blocks) dir_bytes = dir_blocks * BLOCK_SIZE result[dir_name] = { "size": dir_size, "blocks": dir_blocks, "bytes": dir_bytes, }

coveralls · 2016-11-30T18:09:53Z

Coverage decreased (-0.01%) to 96.8% when pulling 5c80bed on pklimai:master into b84b8a6 on Juniper:master.

coveralls · 2016-11-30T18:09:53Z

Coverage decreased (-0.01%) to 96.8% when pulling 5c80bed on pklimai:master into b84b8a6 on Juniper:master.

pklimai · 2016-11-30T18:19:48Z

Pushed latest version, please review. As I said before I wasn't able to use "normalize" option (possibly a separate issue?). Instead using text.strip(), hopefully this is rather safe.

Also note that in this version for non-existing "path" we just return {}, is it what we want?

vnitinv · 2016-12-05T06:56:05Z

@pklimai Can you please fine tune your tests as its failing for Python 3.x
Thanks

coveralls · 2016-12-05T07:14:54Z

Coverage increased (+0.2%) to 97.022% when pulling d516a11 on pklimai:master into b84b8a6 on Juniper:master.

pklimai · 2016-12-05T07:21:58Z

@vnitinv That was some problem with the test system - test_sw_install_kwargs_force_host was failing, that I didn't touch. Now all tests passed.

vnitinv · 2016-12-05T08:10:19Z

Thanks @pklimai.

pklimai added 4 commits November 11, 2016 01:41

Added console_has_banner parameter

46cc13a

This parameter allows to modify login state machine behavior in case of a hung state.

Merge remote-tracking branch 'upstream/master'

2a07716

Merge remote-tracking branch 'upstream/master'

b49fbe7

Added directory_usage to utils

059f271

This utility is similar to "show system directory-usage" Junos command (or UNIX "du" command). It allows to see how much space is taken by a particular directory on disk. Unit test added as well.

stacywsmith suggested changes Nov 29, 2016

View reviewed changes

Modified directory_usage() to return dict and add depth parameter

d2e6459

stacywsmith suggested changes Nov 29, 2016

View reviewed changes

Improving exception handling in directory_usage() util

5c80bed

Update test_fs.py

d516a11

stacywsmith approved these changes Dec 5, 2016

View reviewed changes

stacywsmith merged commit 6b9faac into Juniper:master Dec 5, 2016

stacywsmith mentioned this pull request Dec 5, 2016

Improve error handling in directory_usage(): #631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added directory_usage to utils #629

Added directory_usage to utils #629

pklimai commented Nov 28, 2016

coveralls commented Nov 28, 2016 •

edited

stacywsmith left a comment

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

pklimai commented Nov 29, 2016 •

edited

stacywsmith commented Nov 29, 2016

coveralls commented Nov 29, 2016 •

edited

pklimai commented Nov 29, 2016

stacywsmith left a comment

stacywsmith Nov 29, 2016

pklimai Nov 30, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

pklimai Nov 30, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016

stacywsmith Nov 29, 2016 •

edited

coveralls commented Nov 30, 2016

coveralls commented Nov 30, 2016

pklimai commented Nov 30, 2016

vnitinv commented Dec 5, 2016

coveralls commented Dec 5, 2016 •

edited

pklimai commented Dec 5, 2016

vnitinv commented Dec 5, 2016


		used_space = used_space.strip()
		dirs = rsp.xpath("//directory")


		used_space = rsp.findtext("directory/used-space")
		rsp = self._dev.rpc.get_directory_usage_information(path=path, depth=str(depth))

Added directory_usage to utils #629

Added directory_usage to utils #629

Conversation

pklimai commented Nov 28, 2016

coveralls commented Nov 28, 2016 • edited

stacywsmith 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

pklimai commented Nov 29, 2016 • edited

stacywsmith commented Nov 29, 2016

coveralls commented Nov 29, 2016 • edited

pklimai commented Nov 29, 2016

stacywsmith 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

Choose a reason for hiding this comment

stacywsmith Nov 29, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Nov 30, 2016

coveralls commented Nov 30, 2016

pklimai commented Nov 30, 2016

vnitinv commented Dec 5, 2016

coveralls commented Dec 5, 2016 • edited

pklimai commented Dec 5, 2016

vnitinv commented Dec 5, 2016

coveralls commented Nov 28, 2016 •

edited

pklimai commented Nov 29, 2016 •

edited

coveralls commented Nov 29, 2016 •

edited

stacywsmith Nov 29, 2016 •

edited

coveralls commented Dec 5, 2016 •

edited