canonical / pylxd

Python module for LXD
https://pylxd.readthedocs.io/en/latest/
Apache License 2.0
251 stars 133 forks source link

bugfix: ConnectionResetError on container.execute #526

Closed dbaldy closed 1 year ago

dbaldy commented 1 year ago

Closes #523

LXD considers the socket in non-interactive mode as a pipe, much more than a Web socket. Indeed, once the command is finished executing, LXD "unpolitely" closes the sockets on its side without having the normal conversational close expected for Web Sockets (see 5.5.1).

As the pylxd client was not informed, it tries to recv on a closed socket, hence raising a ConnectionResetError. It is arguable who is at fault here, whether the implementation done on LXD's side is invalid, or if the ws4py isn't defensive enough with a Web socket being treated as a pipe and not a bi-directional stream by the counterpart.

This merge request suppresses the exception traceback, as in our experience it never happened that the socket was closed by the server in the middle of an execution without the command running successfully, however it appears systematically on successful execution of the command, which is extremely misleading.

stgraber commented 1 year ago

@simondeziel looks like the pylxd tests are failing due to the python version available in Github Actions. Could you send a PR to align things with supported version?

codecov-commenter commented 1 year ago

Codecov Report

Merging #526 (6e609b6) into master (efab4c4) will increase coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 6e609b6 differs from pull request most recent head 9e73391. Consider uploading reports for the commit 9e73391 to get more accurate results

@@           Coverage Diff           @@
##           master     #526   +/-   ##
=======================================
  Coverage   97.03%   97.03%           
=======================================
  Files          56       56           
  Lines        4250     4252    +2     
=======================================
+ Hits         4124     4126    +2     
  Misses        126      126           
Impacted Files Coverage Δ
pylxd/models/instance.py 95.77% <100.00%> (+0.02%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

fliiiix commented 1 year ago

@stgraber @simondeziel have a look at https://github.com/lxc/pylxd/pull/527

stgraber commented 1 year ago

thanks!

stgraber commented 1 year ago

@dbaldy could you rebase your PR on master so we can get a test run following @fliiiix's fix?

dbaldy commented 1 year ago

@stgraber done

stgraber commented 1 year ago

@dbaldy sorry, the GH action config had a typo on our side. Could you rebase it again?

dbaldy commented 1 year ago

@stgraber done

fliiiix commented 1 year ago

Thanks @dbaldy, @stgraber & @simondeziel for the effort

can we get this fix in a release?

kimfaint commented 1 year ago

A release for this would be great.