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.

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

Fix execute command missing output with pauses in output text #363

Conversation

ajkavanagh
Copy link
Contributor

For the container execute command, if the command executed has pauses in
the output text, the websocket will send binary messages. These were
interpreted as the stream being completed, and pylxd closed the
websocket, thus losing the rest of the output.

Delving into the code indicated some very strange behaviour across the
threads, so as part of solving the problem, this has been cleaned up as
well.

Closes: #362

For the container execute command, if the command executed has pauses in
the output text, the websocket will send binary messages.  These were
interpreted as the stream being completed, and pylxd closed the
websocket, thus losing the rest of the output.

Delving into the code indicated some very strange behaviour across the
threads, so as part of solving the problem, this has been cleaned up as
well.

Closes: canonical#362
Signed-off-by: Alex Kavanagh <alex.kavanagh@canonical.com>
@ajkavanagh ajkavanagh force-pushed the bug/362/ensure-all-output-on-execute branch from 8010a87 to 4c59b13 Compare May 17, 2019 15:07
@codecov-io
Copy link

codecov-io commented May 17, 2019

Codecov Report

Merging #363 into master will decrease coverage by 0.35%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
- Coverage    96.8%   96.44%   -0.36%     
==========================================
  Files          12       12              
  Lines        1034     1042       +8     
  Branches      119      120       +1     
==========================================
+ Hits         1001     1005       +4     
- Misses         13       16       +3     
- Partials       20       21       +1
Impacted Files Coverage Δ
pylxd/models/container.py 90.18% <55.55%> (-1.21%) ⬇️

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 816da55...4c59b13. Read the comment docs.

@ChrisMacNaughton ChrisMacNaughton merged commit 765eacc into canonical:master May 17, 2019
@ajkavanagh ajkavanagh deleted the bug/362/ensure-all-output-on-execute branch May 17, 2019 15:24
ajkavanagh added a commit to ajkavanagh/pylxd that referenced this pull request May 17, 2019
Essentially, the behavior changed for when a caller to the
container's execute method wished to handle the packets received back on
the websocket.  This change fixes the reversion.

Signed-off-by: Alex Kavanagh <alex.kavanagh@canonical.com>
ChrisMacNaughton added a commit that referenced this pull request May 20, 2019
Fix change of behaviour on execute introduced in #363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

container.execute long running command stdout only returns start of output
3 participants