search

YannickB / odoo-hosting

Other
64 stars 50 forks source link

[IMP] clouder: Improved SSH handling #154

Closed lasley closed 7 years ago

lasley commented 7 years ago

This is a WIP version of the SSH environment I have been working on to fix #152.

While debugging I also found an issue where multiple threads were able to utilize the same SSH connection at the same time, resulting in some incredibly interesting commands being sent to the remote node. This is also fixed in this PR by the use of thread locks upon command passthrough to the Paramiko client.

The locks are currently only implemented at the connection level, but should be easy enough to add to the transport level as well. Doing this would allow multiple threads to use the same SSH connection, but different underlying transports. The caveat is a lot of downstream code would need refactoring in order to use transports instead of the raw connection, so I put this as a to do.

It seems work fine in console; left to do:

Submitting this PR early in case there are comments regarding the implementation before I get deeper.

It should be noted that I made an ssh folder in the clouder module root. I plan on moving more SSH logic out of the model once the session handling has been completed.

lasley commented 7 years ago

Note to self: it will currently yield an infinite loop of connections if server inaccessible

YannickB commented 7 years ago

Hello @lasley, big work thank you. I also agree that ssh related code should be moved elsewhere because Clouder have different way to execute command, I also wanted to do it.

I agree with the design here, seems to go in the good direction.

codecov-io commented 7 years ago

Current coverage is 31.10% (diff: 60.10%)

Merging #154 into master will increase coverage by 1.42%

@@             master       #154   diff @@
==========================================
  Files            72         73     +1   
  Lines          5411       5529   +118   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1606       1720   +114   
- Misses         3805       3809     +4   
  Partials          0          0

Powered by Codecov. Last update b9caaf1...055b940

lasley commented 7 years ago

Alright this is working functionally. I will be adding tests, but won't be changing any of the code unless something comes up in a review or something is identified as broken in the tests. I'll squash up before we merge as well.

lasley commented 7 years ago

Done and tested. I'm going to go over another round of deploys just to make sure we're good functionally but this is now good for review. I'll squash afterwards; I got lazy on my commits

Lint is failing on some other modules, hence the broken test. I didn't really want to raise scope of this PR to other modules though & it looks like they might have gotten covered in #158 as well.

lasley commented 7 years ago

@t3ddftw please review

YannickB commented 7 years ago

Looks good to me, I think I'll make more tests after merging but this seems to be going in the right direction.

And unit test.. thank you so much for this, this will be a life changer if we can trigger the oneclick in unit tests.

YannickB commented 7 years ago

On my side I am ready to merge it

lasley commented 7 years ago

TBH I have no idea where or what @t3ddftw was commenting on because none of that code is in this PR.

Good to merge.

lasley commented 7 years ago

Ohhhh wow there's a Load Diff button now - wtf I'm going to have to watch for that.

We'll nail the interpolation in one swoop as previously determined in order to avoid a bunch of these changes randomly in out of scope PRs. Also the exceptions is on my list of things to do in a fell swoop too.

YannickB commented 7 years ago

Ok let's merge then