Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch to run command #2022

Merged
merged 21 commits into from
Oct 7, 2020
Merged

Switch to run command #2022

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
83d57a1
Temporarily removing the depreciated functions to let the linter find…
Sep 14, 2020
ca259d3
Migrated run_get_output to run_command
Sep 22, 2020
60537bc
Fixed typo
Sep 22, 2020
d81200f
Fixed arg formatting issues.
Sep 23, 2020
e366598
Fixed file mode.
Sep 23, 2020
afb4d49
fixed some unit tests
Sep 23, 2020
76502a5
Potentially fixed unit tests?
Sep 24, 2020
7e15ec6
Fixed file name
Sep 24, 2020
432ee21
Merge branch 'develop' of github.com:Azure/WALinuxAgent into SwitchTo…
Sep 24, 2020
74a94df
Addressed comments
Sep 28, 2020
44507cd
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Sep 28, 2020
daa76c2
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Sep 28, 2020
1568c8e
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Sep 29, 2020
7a97849
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Sep 30, 2020
d4fe530
Merge branch 'develop' of github.com:Azure/WALinuxAgent into SwitchTo…
Sep 30, 2020
e91ff88
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Oct 2, 2020
88d1bab
Fixed comments
Oct 6, 2020
9863b72
Merge branch 'SwitchToRunCommand' of github.com:kevinclark19a/WALinux…
Oct 6, 2020
34ca320
Merge branch 'develop' into SwitchToRunCommand
kevinclark19a Oct 6, 2020
cf9c9f0
fixed tests.
Oct 6, 2020
c247e6b
Merge branch 'SwitchToRunCommand' of github.com:kevinclark19a/WALinux…
Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 53 additions & 19 deletions azurelinuxagent/common/osutil/default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,12 @@ def set_selinux_context(self, path, con): # pylint: disable=R1710
if not os.path.exists(path):
logger.error("Path does not exist: {0}".format(path))
return 1
return shellutil.run('chcon ' + con + ' ' + path)

try:
shellutil.run_command(['chcon', con, path], log_error=True)
except shellutil.CommandError as cmd_err:
narrieta marked this conversation as resolved.
Show resolved Hide resolved
return cmd_err.returncode
return 0

def conf_sshd(self, disable_password):
option = "no" if disable_password else "yes"
Expand Down Expand Up @@ -630,9 +635,20 @@ def umount_dvd(self, chk_err=True, mount_point=None):

def eject_dvd(self, chk_err=True):
dvd = self.get_dvd_device()
retcode = shellutil.run("eject {0}".format(dvd))
if chk_err and retcode != 0:
raise OSUtilError("Failed to eject dvd: ret={0}".format(retcode))
try:
shellutil.run_command(["eject", dvd])
except shellutil.CommandError as cmd_err:
if chk_err:

msg = """Failed to eject dvd: ret={0}
[stdout]
{1}

[stderr]
{2}
""".format(cmd_err.returncode, cmd_err.stdout, cmd_err.stderr)

raise OSUtilError(msg)

def try_load_atapiix_mod(self):
try:
Expand Down Expand Up @@ -669,15 +685,22 @@ def is_atapiix_mod_loaded(self, max_retry=1):
return False

def mount(self, device, mount_point, option="", chk_err=True):
cmd = "mount {0} {1} {2}".format(option, device, mount_point)
retcode, err = shellutil.run_get_output(cmd, chk_err)
if retcode != 0:
detail = "[{0}] returned {1}: {2}".format(cmd, retcode, err)
err = detail
return retcode, err
cmd = ["mount", option, device, mount_point]
try:
output = shellutil.run_command(cmd, log_error=chk_err)
except shellutil.CommandError as cmd_err:
detail = "[{0}] returned {1}:\n stdout: {2}\n\nstderr: {3}".format(cmd, cmd_err.returncode,
cmd_err.stdout, cmd_err.stderr)
return cmd_err.returncode, detail

return 0, output

def umount(self, mount_point, chk_err=True):
return shellutil.run("umount {0}".format(mount_point), chk_err=chk_err)
try:
shellutil.run_command(["umount", mount_point], log_error=chk_err)
except shellutil.CommandError as cmd_err:
return cmd_err.returncode
return 0

def allow_dhcp_broadcast(self):
# Open DHCP port if iptables is enabled.
Expand Down Expand Up @@ -1112,15 +1135,26 @@ def set_dhcp_hostname(self, hostname):
def restart_if(self, ifname, retries=3, wait=5):
retry_limit=retries+1
for attempt in range(1, retry_limit):
return_code=shellutil.run("ifdown {0} && ifup {0}".format(ifname), expected_errors=[1] if attempt < retries else [])
if return_code == 0:
try:
shellutil.run_command(["ifdown", ifname])
shellutil.run_command(["ifup", ifname])
return
logger.warn("failed to restart {0}: return code {1}".format(ifname, return_code))
if attempt < retry_limit:
logger.info("retrying in {0} seconds".format(wait))
time.sleep(wait)
else:
logger.warn("exceeded restart retries")
except shellutil.CommandError as cmd_err:

msg = """failed to restart {0}: returncode={1}
[stdout]
{2}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this logged in L1154?

Copy link
Contributor

@larohra larohra Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok ok, makes sense. Thanks for the clarification


if attempt < retry_limit:
logger.info("retrying in {0} seconds".format(wait))
time.sleep(wait)
else:
logger.warn("exceeded restart retries")

def publish_hostname(self, hostname):
self.set_dhcp_hostname(hostname)
Expand Down
30 changes: 20 additions & 10 deletions azurelinuxagent/common/osutil/ubuntu.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,18 @@ def start_network(self):
return shellutil.run("service networking start", chk_err=False)

def stop_agent_service(self):
return shellutil.run("service {0} stop".format(self.service_name), chk_err=False)
try:
shellutil.run_command(["service", self.service_name, "stop"])
except shellutil.CommandError as cmd_err:
return cmd_err.returncode
return 0

def start_agent_service(self):
return shellutil.run("service {0} start".format(self.service_name), chk_err=False)
try:
shellutil.run_command(["service", self.service_name, "start"])
except shellutil.CommandError as cmd_err:
return cmd_err.returncode
return 0

def remove_rules_files(self, rules_files=""):
pass
Expand Down Expand Up @@ -119,15 +127,17 @@ def restart_if(self, ifname, retries=3, wait=5):
"""
retry_limit=retries+1
for attempt in range(1, retry_limit):
return_code=shellutil.run("ip link set {0} down && ip link set {0} up".format(ifname))
if return_code == 0:
try:
shellutil.run_command(["ip", "link", "set", ifname, "down"])
shellutil.run_command(["ip", "link", "set", ifname, "up"])
return
larohra marked this conversation as resolved.
Show resolved Hide resolved
logger.warn("failed to restart {0}: return code {1}".format(ifname, return_code))
if attempt < retry_limit:
logger.info("retrying in {0} seconds".format(wait))
time.sleep(wait)
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))
if attempt < retry_limit:
logger.info("retrying in {0} seconds".format(wait))
time.sleep(wait)
else:
logger.warn("exceeded restart retries")


class UbuntuSnappyOSUtil(Ubuntu14OSUtil):
Expand Down
57 changes: 41 additions & 16 deletions azurelinuxagent/common/utils/cryptutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,19 @@ def gen_transport_cert(self, prv_file, crt_file):
"""
Create ssl certificate for https communication with endpoint server.
"""
cmd = ("{0} req -x509 -nodes -subj /CN=LinuxTransport -days 730 "
"-newkey rsa:2048 -keyout {1} "
"-out {2}").format(self.openssl_cmd, prv_file, crt_file)
rc = shellutil.run(cmd) # pylint: disable=C0103
if rc != 0:
logger.error("Failed to create {0} and {1} certificates".format(prv_file, crt_file))
cmd = [self.openssl_cmd, "req", "-x509", "-nodes", "-subj", "/CN=LinuxTransport",
"-days", "730", "-newkey", "rsa:2048", "-keyout", prv_file, "-out", crt_file]
try:
shellutil.run_command(cmd)
except shellutil.CommandError as cmd_err:
msg = """Failed to create {0} and {1} certificates.
[stdout]
{2}

[stderr]
{3}
""".format(prv_file, crt_file, cmd_err.stdout, cmd_err.stderr)
logger.error(msg)

def get_pubkey_from_prv(self, file_name):
if not os.path.exists(file_name): # pylint: disable=R1720
Expand Down Expand Up @@ -79,18 +86,36 @@ def decrypt_p7m(self, p7m_file, trans_prv_file, trans_cert_file, pem_file):
elif not os.path.exists(trans_prv_file):
raise IOError(errno.ENOENT, "File not found", trans_prv_file)
else:
cmd = ("{0} cms -decrypt -in {1} -inkey {2} -recip {3} "
"| {4} pkcs12 -nodes -password pass: -out {5}"
"").format(self.openssl_cmd, p7m_file, trans_prv_file,
trans_cert_file, self.openssl_cmd, pem_file)
shellutil.run(cmd)
rc = shellutil.run(cmd) # pylint: disable=C0103
if rc != 0:
logger.error("Failed to decrypt {0}".format(p7m_file))
first_cmd = [self.openssl_cmd, "cms", "-decrypt", "-in", p7m_file, "-inkey",
trans_prv_file, "-recip", trans_cert_file]
second_cmd = [self.openssl_cmd, "pkcs12", "-nodes", "-password", "pass:",
"-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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinclark19a DCR ok?

stdout, stderr = second_proc.communicate()

if first_proc.returncode != 0 or second_proc.returncode != 0:
msg = """Failed to decrypt {0}
[stdout]
{1}

[stderr]
{2}
""".format(p7m_file, stdout, stderr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

logger.error(msg)


def crt_to_ssh(self, input_file, output_file):
shellutil.run("ssh-keygen -i -m PKCS8 -f {0} >> {1}".format(input_file,
output_file))
with open(output_file, "a") as file_out:
keygen_proc = subprocess.Popen(["ssh-keygen", "-i", "-m", "PKCS8", "-f", input_file])
stdout, _ = keygen_proc.communicate()

if keygen_proc.returncode != 0:
return

file_out.write(stdout)


def asn1_to_ssh(self, pubkey):
lines = pubkey.split("\n")
Expand Down
24 changes: 15 additions & 9 deletions tests/common/osutil/test_default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,22 @@ def test_restart(self):
# setup
retries = 3
ifname = 'dummy'
with patch.object(shellutil, "run") as run_patch:
run_patch.return_value = 1
with patch.object(shellutil, "run_command") as run_patch:
run_patch.side_effect = shellutil.CommandError("ifupdown dummy", 1, "", "")

# execute
osutil.DefaultOSUtil.restart_if(osutil.DefaultOSUtil(), ifname=ifname, retries=retries, wait=0)

# assert
self.assertEqual(run_patch.call_count, retries)
self.assertEqual(run_patch.call_args_list[0][0][0], 'ifdown {0} && ifup {0}'.format(ifname))

cmd_queue = list(args[0] for (args, _) in run_patch.call_args_list)
while cmd_queue:
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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def test_get_dvd_device_success(self):
with patch.object(os, 'listdir', return_value=['cpu', 'cdrom0']):
Expand All @@ -85,8 +92,8 @@ def test_mount_dvd_success(self, _):
'get_dvd_device',
return_value='/dev/cdrom'):
with patch.object(shellutil,
'run_get_output',
return_value=(0, msg)) as patch_run: # pylint: disable=unused-variable
'run_command',
return_value=msg) as patch_run: # pylint: disable=unused-variable
with patch.object(os, 'makedirs'):
try:
osutil.DefaultOSUtil().mount_dvd()
Expand All @@ -99,16 +106,15 @@ def test_mount_dvd_failure(self, _):
with patch.object(osutil.DefaultOSUtil,
'get_dvd_device',
return_value='/dev/cdrom'):
with patch.object(shellutil,
'run_get_output',
return_value=(1, msg)) as patch_run:
with patch.object(shellutil, 'run_command',
side_effect=shellutil.CommandError("mount dvd", 1, "", msg)) as patch_run:
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kevinclark19a kevinclark19a Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking! Makes sense now.


def test_empty_proc_net_route(self):
routing_table = ""
Expand Down
Loading