Improvements in run_command #1614

narrieta · 2019-08-22T20:10:38Z

Another round of changes to run_command (which will replace the current run_get_output):

Errors are now NOT logged by default; the caller needs to explicitly ask for it. This is to avoid situations in which inadvertently we produce too much log output (e.g. the 'pidof' message on the serial console) or disclose sensitive information.
Created the CommandError exception, to report errors in the command

I also undid the periodic logging in run_get_output since that may lose important logging information.

I changed the implementations of osutil.get_dhcp_pid that use pidof to now use run_command instead of run_get_output.

The environment thread already had logic to report issues in get_dhcp_pid only once, but the implementations in some distros (e.g. Ubuntu 18) were using run_get_output without turning off error logging.

I fixed an issue in EnvHandler.handle_dhclient_restart that I found during testing: some distros (e.g. Ubuntu 18) may have multiple instances of the DHCP client.

I also started unit test suites for some of the distro-dependent implementations of osutil.

I will continue replacing run_get_output with run_command in later PRs.

This change is

codecov · 2019-08-22T20:18:37Z

Codecov Report

Merging #1614 into develop will increase coverage by 0.1%.
The diff coverage is 91.8%.

@@            Coverage Diff             @@
##           develop    #1614     +/-   ##
==========================================
+ Coverage    66.02%   66.12%   +0.1%     
==========================================
  Files           77       78      +1     
  Lines        11049    11160    +111     
  Branches      1557     1574     +17     
==========================================
+ Hits          7295     7380     +85     
- Misses        3421     3445     +24     
- Partials       333      335      +2

Impacted Files	Coverage Δ
azurelinuxagent/common/utils/cryptutil.py	`50% <100%> (ø)`	⬆️
azurelinuxagent/common/osutil/default.py	`57.91% <100%> (+0.18%)`	⬆️
azurelinuxagent/common/utils/shellutil.py	`66.26% <100%> (+7.44%)`	⬆️
azurelinuxagent/common/osutil/clearlinux.py	`57.89% <100%> (+2.72%)`	⬆️
azurelinuxagent/common/osutil/redhat.py	`58.44% <100%> (+2.03%)`	⬆️
azurelinuxagent/common/osutil/suse.py	`65.67% <100%> (+2.43%)`	⬆️
azurelinuxagent/common/osutil/openwrt.py	`37.34% <100%> (+2.05%)`	⬆️
azurelinuxagent/common/osutil/bigip.py	`86.29% <100%> (-0.11%)`	⬇️
azurelinuxagent/common/osutil/arch.py	`66.66% <100%> (+5.95%)`	⬆️
azurelinuxagent/common/osutil/alpine.py	`66.66% <100%> (+14.66%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 403c5f8...a9d10ba. Read the comment docs.

pgombar

Left some comments.

pgombar · 2019-08-22T21:31:08Z

azurelinuxagent/common/utils/shellutil.py

@@ -117,30 +117,51 @@ def _encode_command_output(output):
    return ustr(output, encoding='utf-8', errors="backslashreplace")


-def run_command(command):
+class CommandError(Exception):


Can we define this in azurelinuxagent.common.exception.py instead?

I know that is what we've doing so far. but I do not think that is a good pattern. Basically we are dumping small pieces (the exceptions) from completely separate modules into a single file. I believe it is cleaner to define the exception in the module that originates it, to avoid unneeded/unwanted cross-module dependencies.

pgombar · 2019-08-22T21:35:42Z

azurelinuxagent/common/utils/shellutil.py

-    retcode = 0
+    @staticmethod
+    def _get_message(command, returncode):
+        command_name = command[0] if isinstance(command, list) and len(command) > 0 else command


Don't we want to log the entire command?

No, the parameters may contain sensitive data that we do not want in the exception. The caller knows the command and whether it is appropriate to include the parameters or not; it can always extend the exception with the parameters if needed.

pgombar · 2019-08-22T22:31:04Z

tests/common/osutil/test_alpine.py

@@ -0,0 +1,34 @@
+# Copyright 2018 Microsoft Corporation


It's 2019. 😀

do we even need a year? I might as well remove it

I honestly have no idea, maybe it's needed from a legal perspective, but in general I don't see any use for it.

some file have it, some don't. will add it

pgombar · 2019-08-22T22:48:32Z

tests/ga/test_env.py

+from tests.tools import *
+
+
+class TestMonitor(AgentTestCase):


Shouldn't this be called TestEnvironment as we're testing the environment thread, not the monitoring thread?

The magic of copy-n-paste... will fix it

pgombar · 2019-08-22T22:51:11Z

tests/ga/test_env.py

+        message = args[0]
+        self.assertEquals("Dhcp client is not running.", message)
+
+    def test_get_dhcp_client_pid_should_return_none_nad_log_an_error_when_an_invalid_command_is_used(self):


nit: 'nad' -> 'and'

copy-n-paste should fix typos... fixed

pgombar · 2019-08-22T22:51:25Z

tests/ga/test_env.py

+            pids = EnvHandler().get_dhcp_client_pid()
+            self.assertEquals(pids, ["11", "22", "4", "5", "6", "9"])
+
+    def test_get_dhcp_client_pid_should_return_none_nad_log_a_warning_when_dhcp_client_is_not_running(self):


nit: 'nad' -> 'and'

pgombar · 2019-08-22T22:54:24Z

azurelinuxagent/ga/env.py

+    def get_dhcp_client_pid(self):
+        pid = None
+        try:
+            # get_dhcp_pid may return multiple PIDs; we split them and return an (alphabetically) sorted list


Can you clarify here why it's alphabetically sorted?

pgombar · 2019-08-22T22:58:48Z

tests/ga/test_env.py

+                    self.assertIsNone(pids)
+                    self.assertEquals(mock_warn.call_count, 2)
+
+    def test_handle_dhclient_restart_should_reconfigure_network_routes_when_dhcp_client_restarts(self):


nit: 'dhclient' -> 'dhcpclient'

the pattern i'm following is

test_<subject>_should_<expected_behavior>

in this case the subject (the function I am testing) is handle_dhclient_restart

You're right, it was me who didn't read carefully!

larohra · 2019-08-22T23:58:56Z

azurelinuxagent/common/utils/shellutil.py

    except Exception as e:
-        logger.error(u"Cannot execute [{0}]. Error: [{1}]".format(command, ustr(e)))
+        if log_error:
+            logger.error(u"Command [{0}] raised unexpected exception: [{1}]", command, ustr(e))


Since command is an array, should we convert it to a string before logging?

sure, i'll do that

larohra · 2019-08-22T23:59:43Z

azurelinuxagent/common/utils/shellutil.py

+        if log_error:
+            logger.error(
+                "Command: [{0}], return code: [{1}], stdout: [{2}] stderr: [{3}]",
+                command,


Since command is an array, should we convert it to a string before logging?

narrieta · 2019-08-23T16:41:59Z

@pgombar @larohra Posted an update addressing comments. Thanks for your review.

tests/ga/test_env.py

pgombar · 2019-08-23T17:50:55Z

Thanks, Norberto! LGTM

narrieta · 2019-08-23T18:34:51Z

@larohra - sorry, missed one of your comments about formatting the command when we log it... can you check my last commit? thanks

larohra · 2019-08-23T20:41:30Z

azurelinuxagent/common/utils/shellutil.py

+    if 'log_error' is True, it also logs details about the error.
+    """
+    def format_command(cmd):
+        return " ".join(cmd) if isinstance(cmd, list) else command


if isinstance(cmd, list) else command
you dont need this check everywhere, the join command would work even if its just a string

a = ['a', 'b'] '.'.join(a) 'a.b' a = 'a' '.'.join(a) 'a' ' '.join(a) 'a'

Else everything else looks good

" ".join("/var/lib/waagent")
'/ v a r / l i b / w a a g e n t'

:)

larohra

Approved with a minor suggestion

Improvements in run_command

6b11e87

narrieta requested review from vrdmr, pgombar and larohra August 22, 2019 20:10

pgombar approved these changes Aug 22, 2019

View reviewed changes

larohra reviewed Aug 22, 2019

View reviewed changes

Addessed review comments

d8ae4c7

larohra reviewed Aug 23, 2019

View reviewed changes

tests/ga/test_env.py Show resolved Hide resolved

larohra approved these changes Aug 23, 2019

View reviewed changes

Review comments

a9d10ba

larohra reviewed Aug 23, 2019

View reviewed changes

larohra approved these changes Aug 23, 2019

View reviewed changes

narrieta merged commit 8bef588 into Azure:develop Aug 23, 2019

narrieta deleted the run-command branch August 23, 2019 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements in run_command #1614

Improvements in run_command #1614

narrieta commented Aug 22, 2019 •

edited by vrdmr

Loading

codecov bot commented Aug 22, 2019 •

edited

Loading

pgombar left a comment

pgombar Aug 22, 2019

narrieta Aug 22, 2019

pgombar Aug 22, 2019

narrieta Aug 22, 2019

pgombar Aug 22, 2019

narrieta Aug 23, 2019

pgombar Aug 23, 2019

narrieta Aug 23, 2019

pgombar Aug 22, 2019

narrieta Aug 23, 2019

pgombar Aug 22, 2019

narrieta Aug 23, 2019

pgombar Aug 22, 2019

narrieta Aug 23, 2019

pgombar Aug 22, 2019

narrieta Aug 22, 2019

pgombar Aug 22, 2019

narrieta Aug 23, 2019 •

edited

Loading

pgombar Aug 23, 2019

larohra Aug 22, 2019

narrieta Aug 23, 2019

larohra Aug 22, 2019

narrieta Aug 23, 2019

narrieta commented Aug 23, 2019

pgombar commented Aug 23, 2019

narrieta commented Aug 23, 2019

larohra Aug 23, 2019

narrieta Aug 23, 2019

larohra left a comment

Improvements in run_command #1614

Improvements in run_command #1614

Conversation

narrieta commented Aug 22, 2019 • edited by vrdmr Loading

codecov bot commented Aug 22, 2019 • edited Loading

Codecov Report

pgombar 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

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

narrieta Aug 23, 2019 • 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

Choose a reason for hiding this comment

narrieta commented Aug 23, 2019

pgombar commented Aug 23, 2019

narrieta commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larohra left a comment

Choose a reason for hiding this comment

narrieta commented Aug 22, 2019 •

edited by vrdmr

Loading

codecov bot commented Aug 22, 2019 •

edited

Loading

narrieta Aug 23, 2019 •

edited

Loading