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

Return for remote commander #581

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 50 additions & 15 deletions virttest/remote_commander/remote_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,19 @@
import random
import shutil
import signal
import tempfile

import remote_interface
import messenger as ms


logging.basicConfig(level=logging.DEBUG)
logger = logging.getLogger(__name__)
logfile = tempfile.mktemp(suffix='.log', prefix='remote_runner', dir="/tmp")
handler = logging.FileHandler(logfile, "w", encoding=None, delay="true")
logger.addHandler(handler)


def daemonize(pipe_root_path="/tmp"):
"""
Init daemon.
Expand Down Expand Up @@ -548,7 +556,14 @@ def cmd_loop(self):
stdios + r_pipes + stdouts + stderrs, [], [])

if self.stdin in r: # command from controller
cmd = CmdSlave(self.read_msg()[1])
m = self.read_msg()
if m[0] == False:
logger.info("Other side is closed.")
break
if m[0] == None:
logger.info("Reading is timeouted.")
break
cmd = CmdSlave(m[1])
self.cmds[cmd.cmd_id] = cmd
try:
# There is hidden bug. We can bump into condition when
Expand Down Expand Up @@ -726,7 +741,7 @@ def python_file_run_with_helper(self, test_path):
assert hasattr(module, "run")
run = getattr(module, "run")
helper = Helper(self)
run(helper)
return run(helper)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return code is OK,
ACK commit 983d589


class Helper(object):
Expand All @@ -736,10 +751,11 @@ class Helper(object):
def __init__(self, messenger):
self.messenger = messenger

def query_master(self, *args, **kargs):
"""Read CmdRespond from master."""
# First of all, dump stdio to master. Otherwise it can block on
# logging.info() function.
def info(self, *args, **kargs):
logger.info(*args, **kargs)
self.flush_buf()

def flush_buf(self):
msg = self.messenger
stdios = [msg.o_stdout, msg.o_stderr]
r, _, _ = select.select(stdios, [], [], 0) # Not blocking.
Expand All @@ -749,8 +765,15 @@ def query_master(self, *args, **kargs):
if msg.o_stderr in r: # Send message from stdout
data = os.read(msg.o_stderr, 16384)
msg.write_msg(remote_interface.StdErr(data))

def query_master(self, *args, **kargs):
"""Read CmdRespond from master."""
# First of all, dump stdio to master. Otherwise it can block on
# logging.info() function.
self.flush_buf()
# Sent actual request.
cmd = remote_interface.CmdQuery(*args, **kargs)
msg = self.messenger
msg.write_msg(cmd)
succ, data = msg.read_msg()
assert succ
Expand All @@ -768,19 +791,31 @@ def remote_agent(in_stream_cls, out_stream_cls):
:params out_stream_cls: Class encapsulated output stream.
"""
try:
fd_stdout = sys.stdout.fileno()
fd_stderr = sys.stderr.fileno()
fd_stdin = sys.stdin.fileno()
sys.stdout.flush()
sys.stderr.flush()
fd_stdin = sys.stdin.fileno() # == 0
fd_stdout = sys.stdout.fileno() # == 1
fd_stderr = sys.stderr.fileno() # == 2
orig_stdout = os.dup(fd_stdout) # Original stdout to master.
orig_stderr = os.dup(fd_stderr) # Original srderr to master.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this orig_stderr not used
I'm not familiar with remote runner, so just check this patch code here, no more comment from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
All SSH encoded base64 traffic goes through stdout.
Executed program/script has modified stdout/stderr.
stderr/stdout from remote_runner are endcoded and passed through original SSH stdout.
So.. in general stdout from SSH encapsulates stdout/stderr from program.
What you have pointed is not a bug. We can safely remove this variable. But...
I am sure that this code is a bit "tricky", so I just left this variable to represent what is really going here.
I can remove it. But it is safe to keep it for demonstrativeness. Do you want me to remove this variable ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yest, it's really a tricky point, keep it is fine.

soutr, soutw = os.pipe()
serrr, serrw = os.pipe()
sys.stdout = os.fdopen(soutw, 'w', 0)
sys.stderr = os.fdopen(serrw, 'w', 0)
os.write(fd_stdout, "#")

logging.basicConfig(stream=sys.stdout, level=logging.DEBUG)
os.dup2(soutw, fd_stdout) # Stdout == pipe
os.dup2(serrw, fd_stderr) # Stderr == pipe
os.close(soutw) # Close pipe as it stays opened in stdout.
os.close(serrw) # Close pipe as it stays opened in stderr.
sys.stdout = os.fdopen(fd_stdout, 'w', 0)
sys.stderr = os.fdopen(fd_stderr, 'w', 0)
os.write(orig_stdout, "#")

# Logging goes to the pipe.
handler = logging.StreamHandler()
handler.setLevel(logging.DEBUG)
logger.addHandler(handler)
#logging.basicConfig(stream=sys.stdout, level=logging.DEBUG)

w_stdin = None
w_stdout = out_stream_cls(fd_stdout)
w_stdout = out_stream_cls(orig_stdout)
w_stdin = in_stream_cls(fd_stdin)

cmd = CommanderSlaveCmds(w_stdin,
Expand Down