mitogen icon indicating copy to clipboard operation
mitogen copied to clipboard

Get mitogen.fakessh module working again

Open ryanofsky opened this issue 5 years ago • 4 comments

Fixes include

  • Setting cloexec flag on pipe files, using set_inheritable on sockets, and close_fds=False on subprocess.Popen to work around file descriptors not being inheritable by default in new versions of python

  • Adding mitogen.exit_status variable and avoiding os.kill call so fake 'ssh' script is able to exit cleanly with correct status code

  • Fixing broken os.dup call in ExternalContext._setup_master when input and output streams have the same descriptor

  • Updating fakessh module to do necessary python3 string/byte conversions, and use updated mitogen Protocol, Stream, and Router apis

  • Simplifying fakessh startup sequence so there aren't unnecessary differences between ways control and data handles are passed, and ways master and slave processes are initialized

  • Fixing shutdown race conditions where subprocess exit handling or stdin EOF handling could result in a truncated stdout stream

  • Updating and adding a lot of docstrings and comments

  • Adding Process.proc is None / is not None assertions to be clear about which parts of fakessh.Process code are specific to the slave process, and which parts are specific to the master process.

  • Re-enabling unit test case and updating an outdated file path so it passes

ryanofsky avatar Jan 19 '20 11:01 ryanofsky

@ryanofsky please could you update this to current master. Otherwise, may I update this branch on your behalf?

moreati avatar Jan 20 '21 18:01 moreati

@ryanofsky please could you update this to current master. Otherwise, may I update this branch on your behalf?

Hi! I think I can update the branch in the next few days or so, but if you're interested to do it sooner or take this over, it'd be more than welcome. Happy to add permissions to my github branch if that would help.

ryanofsky avatar Jan 20 '21 18:01 ryanofsky

Hmm, apparently it was just a release notes documentation conflict so I used the github web interface to resolve it. The merge seems ok, but I can update if you want a different commit structure (maybe a rebase), or if something isn't working. Feel free to make any changes, also.

ryanofsky avatar Jan 20 '21 19:01 ryanofsky

It seems that while this PR does restore fakessh functionality, taking it from a completely broken state to an updated and working state, there are still problems in CI.

One way to move forward with this PR might be to merge the code changes but keep the @unittest2.skip('broken') test annotation so fakessh can still be used while CI issues are debugged. fakessh is working well for me with the current PR and the fakessh_test.py test does pass reliably for me locally.

Looking at logs there seem to be 3 distinct CI issues:

  1. ValueError: I/O operation on closed file exception causing self.assertEqual(return_code, 0) test failure. This seems to be a race condition that doesn't always happen. When it does happen rsync actually succeeds but there is an error closing down connections.

  2. AttributeError: '_socket.socket' object has no attribute 'set_inheritable' error in python2, because the method only exists in python3. It should be pretty easy to fix because the call isn't necessary in python2 (descriptors are inherited by default there).

  3. assert [proc.wait() for proc in procs] == [0] * len(procs) in ansible install. It's not actually clear that this is related to the PR, but it might be.

ryanofsky avatar Feb 07 '21 15:02 ryanofsky