Merge ~ltrager/maas:emulate_terminal into maas:master

Proposed by Lee Trager on 2017-07-22
Status: Merged
Approved by: Lee Trager on 2017-07-26
Approved revision: ff037e0fef34d76471fbc471b1d44e95c7d39b6f
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~ltrager/maas:emulate_terminal
Merge into: maas:master
Diff against target: 96 lines (+59/-4)
2 files modified
src/provisioningserver/refresh/maas_api_helper.py (+41/-4)
src/provisioningserver/refresh/tests/test_maas_api_helper.py (+18/-0)
Reviewer Review Type Date Requested Status
Newell Jensen 2017-07-22 Approve on 2017-07-22
Review via email: mp+327927@code.launchpad.net

Commit message

Enumate a terminal when logging script output

Some applications don't properly detect that they are not being run in a
terminal and refresh output for progress bars, counters, and spinners. These
characters quickly add up making the log difficult to read. When writing output
from an application emulate a terminal so readable data is captured.

Fixes: LP: #1705792

To post a comment you must log in.
Newell Jensen (newell-jensen) wrote :

How does this fix progress bars or spinners? Some progress bars are dots with a percentage as show in the bug, but some might use # signs etc. Additionally, spinners, when spinning will show hyphens '-' while the spinner is spinning between backslash-hyphen-forwardslash and so forth. Is this branch to fix all those possibilities or are we just doing it for the carriage return and backslash like you have in your code?

review: Needs Information
Lee Trager (ltrager) wrote :

The carriage return and back slash are how you change the output on the screen. [1] is a spinner. It writes the character, sleeps, then moves the position back one space and writes over it. \b would be used to update a counter or any other character as well. MAAS now looks for the backspace and carriage return characters and seeks back as a terminal does. So previously if you ran [1] as a test script in MAAS you would get a file containing 'Processing... ' and '|/-\\' 10 times repeated, now you just get 'Processing... '

http://paste.ubuntu.com/25143239/

Lee Trager (ltrager) :
Newell Jensen (newell-jensen) wrote :

Thanks for the clarification!

review: Approve
MAAS Lander (maas-lander) wrote :
MAAS Lander (maas-lander) wrote :
Mike Pontillo (mpontillo) wrote :

This is interesting.

I should hope that MAAS (and related utilities that generate logging in MAAS, such as curtin, or any test scripts) would run most commands with proper arguments for non-interactive usage, so that we can avoid this issue.

But this seems like a nice workaround for cases where that isn't true, and seems to be simple enough that it won't break our usual non-interactive logs.

One nit: I might have liked to see more comprehensive testing of edge cases, such as seeking to the beginning of a line in a case where you aren't seeking to the start of the file.

Update scan failed

At least one of the branches involved have failed to scan. You can manually schedule a rescan if required.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/provisioningserver/refresh/maas_api_helper.py b/src/provisioningserver/refresh/maas_api_helper.py
2index ca2271a..d4dd138 100644
3--- a/src/provisioningserver/refresh/maas_api_helper.py
4+++ b/src/provisioningserver/refresh/maas_api_helper.py
5@@ -17,6 +17,7 @@ from datetime import (
6 from email.utils import parsedate
7 import json
8 import mimetypes
9+import os
10 import random
11 import selectors
12 import socket
13@@ -296,8 +297,12 @@ def capture_script_output(
14 # Pad the timeout by 60 seconds to allow for cleanup.
15 timeout = datetime.now() + timedelta(seconds=(timeout_seconds + 60))
16
17- with open(stdout_path, 'wb') as out, open(stderr_path, 'wb') as err:
18- with open(combined_path, 'wb') as combined:
19+ # Create the file and then open it in read write mode for terminal
20+ # emulation.
21+ for path in (stdout_path, stderr_path, combined_path):
22+ open(path, 'w').close()
23+ with open(stdout_path, 'r+b') as out, open(stderr_path, 'r+b') as err:
24+ with open(combined_path, 'r+b') as combined:
25 with selectors.DefaultSelector() as selector:
26 selector.register(proc.stdout, selectors.EVENT_READ, out)
27 selector.register(proc.stderr, selectors.EVENT_READ, err)
28@@ -342,5 +347,37 @@ def _select_script_output(selector, combined, timeout):
29 if len(chunk) == 0: # EOF
30 selector.unregister(key.fileobj)
31 else:
32- key.data.write(chunk)
33- combined.write(chunk)
34+ # The list comprehension is needed to get byte objects instead
35+ # of their numeric value.
36+ for i in [chunk[i:i + 1] for i in range(len(chunk))]:
37+ for f in [key.data, combined]:
38+ # Some applications don't properly detect that they are
39+ # not being run in a terminal and refresh output for
40+ # progress bars, counters, and spinners. These
41+ # characters quickly add up making the log difficult to
42+ # read. When writing output from an application emulate
43+ # a terminal so readable data is captured.
44+ if i == b'\b':
45+ # Backspace - Go back one space, if we can.
46+ if f.tell() != 0:
47+ f.seek(-1, os.SEEK_CUR)
48+ elif i == b'\r':
49+ # Carriage return - Seek to the beginning of the
50+ # line, as indicated by a line feed, or file.
51+ while f.tell() != 0:
52+ f.seek(-1, os.SEEK_CUR)
53+ if f.read(1) == b'\n':
54+ # Check if line feed was found.
55+ break
56+ else:
57+ # The read advances the file position by
58+ # one so seek back again.
59+ f.seek(-1, os.SEEK_CUR)
60+ elif i == b'\n':
61+ # Line feed - Some applications do a carriage
62+ # return and then a line feed. The data on the line
63+ # should be saved, not overwritten.
64+ f.seek(0, os.SEEK_END)
65+ f.write(i)
66+ else:
67+ f.write(i)
68diff --git a/src/provisioningserver/refresh/tests/test_maas_api_helper.py b/src/provisioningserver/refresh/tests/test_maas_api_helper.py
69index be50262..3bf9b83 100644
70--- a/src/provisioningserver/refresh/tests/test_maas_api_helper.py
71+++ b/src/provisioningserver/refresh/tests/test_maas_api_helper.py
72@@ -596,6 +596,24 @@ class TestCaptureScriptOutput(MAASTestCase):
73 # This is a complete XML document; we've captured all output.
74 self.assertThat(etree.fromstring(stdout).tag, Equals("list"))
75
76+ def test__interprets_backslash(self):
77+ proc = Popen(
78+ 'bash -c "echo -en \bmas\bas"',
79+ stdout=PIPE, stderr=PIPE, shell=True)
80+ self.assertThat(
81+ self.capture(proc), MatchesListwise((
82+ Equals(0), Equals("maas"), Equals(""), Equals("maas")
83+ )))
84+
85+ def test__interprets_carriage_return(self):
86+ proc = Popen(
87+ 'bash -c "echo -en foo\rmaas"',
88+ stdout=PIPE, stderr=PIPE, shell=True)
89+ self.assertThat(
90+ self.capture(proc), MatchesListwise((
91+ Equals(0), Equals("maas"), Equals(""), Equals("maas")
92+ )))
93+
94 def test__timeout(self):
95 # The timeout is padded by 60 seconds, override it so the test can
96 # run quickly.

Subscribers

People subscribed via source and target branches

to all changes: