Switch to run command #2022

kevinclark19a · 2020-09-24T17:48:48Z

Description

Switched a set of shellutil.run_get_output and shellutil.run callers to the new shellutil.run_command function.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

… everything that needs changing.

…RunCommand

azurelinuxagent/common/osutil/default.py

narrieta · 2020-09-25T18:18:33Z

azurelinuxagent/common/osutil/default.py

+            shellutil.run_command(["eject", dvd])
+        except shellutil.CommandError as cmd_err:
+            if chk_err:
+                raise OSUtilError("Failed to eject dvd: ret={0}".format(cmd_err.returncode))


we should probably add the stdout/stderr of the command (the original code was logging errors)

narrieta · 2020-09-25T18:53:40Z

azurelinuxagent/common/osutil/default.py

-            else:
-                logger.warn("exceeded restart retries")
+            except shellutil.CommandError as cmd_err:
+                logger.warn("failed to restart {0}: return code {1}".format(ifname, cmd_err.returncode))


we should add the stdout/stderr of the command

There is small difference here in that the previous code would log commands errors as INFO if the return code was 1, and as ERROR otherwise.
Since anyways we are logging a WARN regardless of the return code, I think we are OK here (as long as we add stderr/stdout to the message)

narrieta · 2020-09-25T19:17:16Z

azurelinuxagent/common/utils/cryptutil.py

+            "-days", "730", "-newkey", "rsa:2048", "-keyout", prv_file, "-out", crt_file]
+        try:
+            shellutil.run_command(cmd)
+        except shellutil.CommandError:


log stdout/stderr

narrieta · 2020-09-25T19:19:38Z

azurelinuxagent/common/utils/cryptutil.py

+            second_proc = subprocess.Popen(second_cmd, stdin=first_proc.stdout, stdout=subprocess.PIPE)
+            second_proc.wait()
+
+            if first_proc.returncode or second_proc.returncode:


let's do an explicit comparison against 0

narrieta · 2020-09-25T19:35:32Z

azurelinuxagent/common/utils/cryptutil.py

+
+            first_proc = subprocess.Popen(first_cmd, stdout=subprocess.PIPE)
+            second_proc = subprocess.Popen(second_cmd, stdin=first_proc.stdout, stdout=subprocess.PIPE)
+            second_proc.wait()


we should use communicate() instead to get stdout/stderr and log it on failure

narrieta · 2020-09-25T19:50:21Z

azurelinuxagent/common/utils/cryptutil.py

-                                                                    output_file))
+        try:
+            with open(output_file, "a") as file_out:
+                output = shellutil.run_command(["ssh-keygen", "-i -m PKCS8 -f", input_file])


I would feel more comfortable if we use Popen directly here (as we did in the previous function). run_command has some extra code to encode the command's output and, though I do not see any issue right now, I think it would be safer to use Popen.

narrieta

Added comments

…RunCommand

larohra · 2020-09-30T23:56:11Z

azurelinuxagent/common/osutil/default.py

+                [stderr]
+                {3}
+                """.format(ifname, cmd_err.returncode, cmd_err.stdout, cmd_err.stderr)
+                logger.warn(msg)


In the previous implementation, the exit_code:1 was logged as an info if there were retries available and then as an error. Now you're just logging it as a warning, not sure if you want to keep them consistent or not.

Isn't this logged in L1154?

Nope that's a different log line. I was comparing the previous implementation with the new one

Basically this parameter for expected_errors=[1] in the previous implementation -

return_code=shellutil.run("ifdown {0} && ifup {0}".format(ifname), expected_errors=[1] if attempt < retries else [])

The shellutil.run function would've logged it as an info if the exit_code was 1 but now it would be logged as an error.

Not sure how big of a difference it makes, just noticed it so figured I'd point it out. I'm personally fine as is too

@larohra I posted a similar comment here, not sure why it doesn't show up.

In any case, I added the 'expected_errors' parameter some time back as a stopgap to avoid logging an error when the error code is 1. This seems to be relatively common and was logged as an error due to the way the run() function is defined, and this was confusing people looking at the log. From the caller, any error code executing the command is clearly a warning.

The new implementation drops the INFO (which was there just because run logs all failures in the command it execurtes), but keeps the warning. This should be fine.

Ahh ok ok, makes sense. Thanks for the clarification

azurelinuxagent/common/osutil/ubuntu.py

larohra · 2020-10-01T00:02:46Z

azurelinuxagent/common/utils/cryptutil.py

+                "-out", pem_file]
+
+            first_proc = subprocess.Popen(first_cmd, stdout=subprocess.PIPE)
+            second_proc = subprocess.Popen(second_cmd, stdin=first_proc.stdout, stdout=subprocess.PIPE)


Good way of solving the problem! I'm sure you must've already done this, but you confirmed this has no regressions right? And DCR passed too?

@kevinclark19a DCR ok?

larohra · 2020-10-01T00:04:40Z

azurelinuxagent/common/utils/cryptutil.py

+
+                [stderr]
+                {2}
+                """.format(p7m_file, stdout, stderr)


Make sure to encode the stdout/stderr before writing it to log (the run_command function always does that for you)

larohra · 2020-10-01T00:07:34Z

tests/common/osutil/test_default.py

+                self.assertEqual(cmd_queue.pop(0), ["ifdown", ifname])
+                # We don't expect the following command to be called because 'dummy' does
+                # not exist.
+                # self.assertEqual(cmd_queue.pop(0), ["ifup", ifname])


If you dont expect this, you should add an assert that it's actually not called rather than a comment :)

pgombar · 2020-10-02T18:32:11Z

tests/common/osutil/test_default.py

                with patch.object(os, 'makedirs'):
                    try:
                        osutil.DefaultOSUtil().mount_dvd()
                        self.fail('OSUtilError was not raised')
                    except OSUtilError as ose:
                        self.assertTrue(msg in ustr(ose))
-                        self.assertTrue(patch_run.call_count == 6)
+                        self.assertEqual(patch_run.call_count, 5)


Why did the number of calls change if we didn't change the retry number (6)?

The range has always been [1..6), so I'm not quite sure how this test was passing before. Any ideas?

Ah, ok I figured it out. Before, we were mocking run_get_output, because that's what self.mount used before. There is a run_get_output call in mount_dvd that we decided not to bother changing as a part of this PR (as it uses a constant string), which was the 6th call before. Since our mock is now for run_command, that run_get_output call obviously doesn't get counted anymore, bringing the total number of calls from 6 (5 run_get_output calls in self.mount + 1 run_get_output in mount_dvd itself) to 5 (just the five run_command calls in self.mount).

Thanks for checking! Makes sense now.

pgombar

Minor comments, else LGTM

…Agent into SwitchToRunCommand

larohra · 2020-10-06T22:45:59Z

azurelinuxagent/common/utils/cryptutil.py

@@ -96,6 +96,9 @@ def decrypt_p7m(self, p7m_file, trans_prv_file, trans_cert_file, pem_file):
            stdout, stderr = second_proc.communicate()

            if first_proc.returncode != 0 or second_proc.returncode != 0:
+                stdout = ustr(stdout, encoding='utf-8', errors="backslashreplace") if stdout else ""


NIT: How about making this function public and using this instead -

def _encode_command_output(output): return ustr(output, encoding='utf-8', errors="backslashreplace")

Its in the shellutils.py

This reverts commit 7c8ed71.

This reverts commit 7c8ed71. Co-authored-by: narrieta <narrieta>

This reverts commit f90227f.

… reverted. (#2050) * Temporarily removing the depreciated functions to let the linter find everything that needs changing. * Migrated run_get_output to run_command * Fixed typo * Fixed arg formatting issues. * Fixed file mode. * fixed some unit tests * Potentially fixed unit tests? * Fixed file name * Addressed comments * Fixed comments * fixed tests. * Added logging * Potentially fixed bug * Removing debugging log. * Fixed bug with run_command needing a list instead of a string. * Added logging statement for debugging. * Fixed tests * Revert "Revert "Switch to run command (#2022)" (#2039)" This reverts commit f90227f. * Reverted unneeded changes. * Potential fix for DCR provisioning failure. * added logging for errors. * Addressed PR comments.

keclar added 9 commits September 14, 2020 21:23

Temporarily removing the depreciated functions to let the linter find…

83d57a1

… everything that needs changing.

Migrated run_get_output to run_command

ca259d3

Fixed typo

60537bc

Fixed arg formatting issues.

d81200f

Fixed file mode.

e366598

fixed some unit tests

afb4d49

Potentially fixed unit tests?

76502a5

Fixed file name

7e15ec6

Merge branch 'develop' of github.com:Azure/WALinuxAgent into SwitchTo…

432ee21

…RunCommand

kevinclark19a requested review from larohra, narrieta, pgombar and ZhidongPeng as code owners September 24, 2020 17:48

narrieta reviewed Sep 25, 2020

View reviewed changes

azurelinuxagent/common/osutil/default.py Show resolved Hide resolved

narrieta reviewed Sep 25, 2020

View reviewed changes

narrieta requested changes Sep 25, 2020

View reviewed changes

keclar and others added 3 commits September 28, 2020 21:04

Addressed comments

74a94df

Merge branch 'develop' into SwitchToRunCommand

44507cd

Merge branch 'develop' into SwitchToRunCommand

daa76c2

narrieta previously approved these changes Sep 29, 2020

View reviewed changes

kevinclark19a and others added 3 commits September 29, 2020 12:17

Merge branch 'develop' into SwitchToRunCommand

1568c8e

Merge branch 'develop' into SwitchToRunCommand

7a97849

Merge branch 'develop' of github.com:Azure/WALinuxAgent into SwitchTo…

d4fe530

…RunCommand

larohra reviewed Oct 1, 2020

View reviewed changes

Merge branch 'develop' into SwitchToRunCommand

e91ff88

pgombar reviewed Oct 2, 2020

View reviewed changes

pgombar previously approved these changes Oct 2, 2020

View reviewed changes

keclar added 2 commits October 6, 2020 18:29

Fixed comments

88d1bab

Merge branch 'SwitchToRunCommand' of github.com:kevinclark19a/WALinux…

9863b72

…Agent into SwitchToRunCommand

kevinclark19a dismissed stale reviews from pgombar and narrieta via 9863b72 October 6, 2020 18:29

kevinclark19a and others added 3 commits October 6, 2020 11:30

Merge branch 'develop' into SwitchToRunCommand

34ca320

fixed tests.

cf9c9f0

Merge branch 'SwitchToRunCommand' of github.com:kevinclark19a/WALinux…

c247e6b

…Agent into SwitchToRunCommand

pgombar approved these changes Oct 6, 2020

View reviewed changes

larohra reviewed Oct 6, 2020

View reviewed changes

narrieta approved these changes Oct 7, 2020

View reviewed changes

kevinclark19a merged commit 7c8ed71 into Azure:develop Oct 7, 2020

kevinclark19a deleted the SwitchToRunCommand branch October 7, 2020 16:58

narrieta pushed a commit to narrieta/WALinuxAgent that referenced this pull request Oct 12, 2020

Revert "Switch to run command (Azure#2022)"

6635871

This reverts commit 7c8ed71.

narrieta added a commit that referenced this pull request Oct 13, 2020

Revert "Switch to run command (#2022)" (#2039)

f90227f

This reverts commit 7c8ed71. Co-authored-by: narrieta <narrieta>

kevinclark19a pushed a commit to kevinclark19a/WALinuxAgent that referenced this pull request Oct 23, 2020

Revert "Revert "Switch to run command (Azure#2022)" (Azure#2039)"

42d397c

This reverts commit f90227f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to run command #2022

Switch to run command #2022

kevinclark19a commented Sep 24, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta Sep 25, 2020

narrieta left a comment

larohra Sep 30, 2020

pgombar Oct 2, 2020

larohra Oct 6, 2020 •

edited

Loading

larohra Oct 6, 2020

narrieta Oct 7, 2020

larohra Oct 7, 2020

larohra Oct 1, 2020

narrieta Oct 7, 2020

larohra Oct 1, 2020

larohra Oct 1, 2020

pgombar Oct 2, 2020

kevinclark19a Oct 2, 2020

kevinclark19a Oct 2, 2020 •

edited

Loading

pgombar Oct 5, 2020

pgombar left a comment

larohra Oct 6, 2020 •

edited

Loading

Switch to run command #2022

Switch to run command #2022

Conversation

kevinclark19a commented Sep 24, 2020

Description

PR information

Quality of Code and Contribution Guidelines

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larohra Oct 6, 2020 • 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

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

kevinclark19a Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

larohra Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

larohra Oct 6, 2020 •

edited

Loading

kevinclark19a Oct 2, 2020 •

edited

Loading

larohra Oct 6, 2020 •

edited

Loading