Merge ~sylvain-pineau/checkbox-ng:remote-deb-restart-strategy into checkbox-ng:master

[Sylvain Pineau]

I'd like to abuse the current API bump already in master to propose support for a new kind of restart strategy, RemoteDebRestartStrategy.

This patchset will allow jobs mainly based on p-p-c pm_test script to run via remote. To replicate the pure local invocation, the slave service is disabled when a noreturn flag is set in the job definition and reactivated via the restart command callback written in the __respawn magic file.

In order to work such jobs running as root over remote require more than the DISPLAY env var, they do need access to the running X server via XAUTHORITY. The extra env is now including this var and sent to the runner.
The same env also contains the NORMAL_USER set in launcher as scripts like pm_test which run `sudo --user` but now miss vars like SUDO_ID.

Tested using the following launcher on two 18.04 desktops:

[launcher]
app_id = com.canonical.certification:checkbox-test
launcher_version = 1
stock_reports = text, submission_files

[test plan]
#unit = com.canonical.certification::warm-boot-stress-test
unit = com.canonical.certification::power-management-reboot-poweroff-cert-automated
#unit = com.canonical.certification::stress-suspend-30-cycles-with-reboots-automated
forced = yes

[test selection]
forced = no

[ui]
output = hide-resource-and-attachment

[daemon]
normal_user = u

[environment]
STRESS_BOOT_ITERATIONS=2
STRESS_BOOT_WAIT_DELAY=10
STRESS_BOOT_WAKEUP_DELAY=15

[Jonathan Cave (jocave)]

LGTM

review: Approve

diff --git a/checkbox_ng/launcher/slave.py b/checkbox_ng/launcher/slave.py
index b568620..96e2519 100644
--- a/checkbox_ng/launcher/slave.py
+++ b/checkbox_ng/launcher/slave.py
@@ -26,12 +26,11 @@ import socket
 import sys
 
 from plainbox.impl.session.remote_assistant import RemoteSessionAssistant
+from plainbox.impl.session.restart import RemoteDebRestartStrategy
 from plainbox.impl.session.restart import RemoteSnappyRestartStrategy
 from plainbox.vendor import rpyc
 from plainbox.vendor.rpyc.utils.server import ThreadedServer
 
-from checkbox_ng.config import load_configs
-
 _ = gettext.gettext
 _logger = logging.getLogger("slave")
 
@@ -88,6 +87,19 @@ class RemoteSlave():
                 # XXX: explicitly passing None to not have to bump Remote API
                 # TODO: remove on the next Remote API bump
                 SessionAssistantSlave.session_assistant.resume_by_id(None)
+        else:
+            _logger.info("RemoteDebRestartStrategy")
+            if remote_restart_strategy_debug:
+                strategy = RemoteDebRestartStrategy(debug=True)
+            else:
+                strategy = RemoteDebRestartStrategy()
+            if os.path.exists(strategy.session_resume_filename):
+                with open(strategy.session_resume_filename, 'rt') as f:
+                    session_id = f.readline()
+                _logger.info(
+                    "RemoteDebRestartStrategy resume_by_id %r", session_id)
+                SessionAssistantSlave.session_assistant.resume_by_id(
+                    session_id)
         self._server = ThreadedServer(
             SessionAssistantSlave,
             port=slave_port,
diff --git a/plainbox/impl/ctrl.py b/plainbox/impl/ctrl.py
index 9056054..801eccd 100644
--- a/plainbox/impl/ctrl.py
+++ b/plainbox/impl/ctrl.py
@@ -290,7 +290,7 @@ class CheckBoxSessionStateController(ISessionStateController):
             # XXX: Consider forwarding the origin object here.  I guess we
             # should have from_frc822_record as with JobDefinition
             resource = Resource(record.data)
-            logger.info(
+            logger.debug(
                 _("Storing resource record %r: %s"), job.id, resource)
             new_resource_list.append(resource)
         # Create an empty resource object to properly fail __getattr__ calls
diff --git a/plainbox/impl/execution.py b/plainbox/impl/execution.py
index ef59210..e311c7c 100644
--- a/plainbox/impl/execution.py
+++ b/plainbox/impl/execution.py
@@ -277,9 +277,10 @@ class UnifiedRunner(IJobRunner):
                 self._extra_env)
             env = get_execution_environment(
                 job, environ, self._session_dir, nest_dir)
+            if self._user_provider():
+                env['NORMAL_USER'] = self._user_provider()
             # run the command
-            logger.debug(_("job[%(ID)s] executing %(CMD)r with env %(ENV)r"
-                           " in cwd %(DIR)r"),
+            logger.debug(_("job[%(ID)s] executing %(CMD)r with env %(ENV)r"),
                          {"ID": job.id, "CMD": cmd,
                           "ENV": env})
             if 'preserve-cwd' in job.get_flag_set() or os.getenv("SNAP"):
@@ -568,6 +569,8 @@ def get_execution_command(job, environ, session_dir,
             job, environ, session_dir, nest_dir, extra_env)
     else:
         env = get_execution_environment(job, environ, session_dir, nest_dir)
+        if extra_env:
+            env.update(extra_env)
     cmd += ["{key}={value}".format(key=key, value=value)
             for key, value in sorted(env.items())]
     cmd += [job.shell, '-c', job.command]
diff --git a/plainbox/impl/session/assistant.py b/plainbox/impl/session/assistant.py
index 6e8921e..1800fd7 100644
--- a/plainbox/impl/session/assistant.py
+++ b/plainbox/impl/session/assistant.py
@@ -61,6 +61,7 @@ from plainbox.impl.session.jobs import InhibitionCause
 from plainbox.impl.session.manager import SessionManager
 from plainbox.impl.session.restart import IRestartStrategy
 from plainbox.impl.session.restart import detect_restart_strategy
+from plainbox.impl.session.restart import RemoteDebRestartStrategy
 from plainbox.impl.session.storage import SessionStorageRepository
 from plainbox.impl.transport import OAuthTransport
 from plainbox.impl.transport import TransportError
@@ -513,7 +514,7 @@ class SessionAssistant:
         self._command_io_delegate = JobRunnerUIDelegate(_SilentUI())
         self._init_runner(runner_cls, runner_kwargs)
         self.session_available(self._manager.storage.id)
-        _logger.debug("New session created: %s", title)
+        _logger.info("New session created: %s", title)
         UsageExpectation.of(self).allowed_calls = {
             self.get_test_plans: "to get the list of available test plans",
             self.get_test_plan: "to get particular test plan object",
@@ -531,7 +532,6 @@ class SessionAssistant:
             allowed_calls[self.use_alternate_restart_strategy] = (
                 "configure automatic restart capability")
 
-
     @raises(KeyError, UnexpectedMethodCall)
     def resume_session(self, session_id: str,
                        runner_cls=UnifiedRunner,
@@ -578,10 +578,12 @@ class SessionAssistant:
                 ).get_result()
                 self._context.state.update_job_result(job, result)
                 self._manager.checkpoint()
+        self._restart_strategy = detect_restart_strategy(self)
+        _logger.info("Session strategy: %r", self._restart_strategy)
         if self._restart_strategy is not None:
             self._restart_strategy.diffuse_application_restart(self._app_id)
         self.session_available(self._manager.storage.id)
-        _logger.debug("Session resumed: %s", session_id)
+        _logger.info("Session resumed: %s", session_id)
         if SessionMetaData.FLAG_TESTPLANLESS in self._metadata.flags:
             UsageExpectation.of(self).allowed_calls = self._get_allowed_calls_in_normal_state()
         else:
@@ -622,7 +624,7 @@ class SessionAssistant:
             try:
                 metadata = SessionPeekHelper().peek(data)
             except SessionResumeError:
-                _logger.info("Exception raised when trying to resume"
+                _logger.info("Exception raised when trying to resume "
                              "session: %s", str(storage.id))
             else:
                 if (metadata.app_id == self._app_id and
@@ -921,7 +923,6 @@ class SessionAssistant:
         # during bootstrap phase
         self._bootstrap_done_list = self.get_dynamic_done_list()
 
-
     @raises(KeyError, UnexpectedMethodCall)
     def use_alternate_selection(self, selection: 'Iterable[str]'):
         """
@@ -1387,12 +1388,21 @@ class SessionAssistant:
             autorestart = (self._restart_strategy is not None and
                            'autorestart' in job.get_flag_set())
             if autorestart:
-                restart_cmd = ' '.join(
-                    shlex.quote(cmd_part)
-                    for cmd_part in self._restart_cmd_callback(
-                        self._manager.storage.id))
+                restart_cmd = ''
+                if self._restart_cmd_callback:
+                    restart_cmd = ' '.join(
+                        shlex.quote(cmd_part)
+                        for cmd_part in self._restart_cmd_callback(
+                            self._manager.storage.id))
                 self._restart_strategy.prime_application_restart(
                     self._app_id, self._manager.storage.id, restart_cmd)
+            elif (
+                isinstance(self._restart_strategy, RemoteDebRestartStrategy)
+                and 'noreturn' in job.get_flag_set()
+            ):
+                self._restart_strategy.prime_application_restart(
+                    self._app_id, self._manager.storage.id,
+                    RemoteDebRestartStrategy.service_name)
             ui.started_running(job, job_state)
             if 'noreturn' in job.get_flag_set():
                 # 'share' the information how to respawn the application
@@ -1408,8 +1418,16 @@ class SessionAssistant:
                     checkbox_data_dir, '__respawn_checkbox')
                 if self._restart_cmd_callback:
                     with open(respawn_cmd_file, 'wt') as f:
-                        f.writelines(self._restart_cmd_callback(
-                            self.get_session_id()))
+                        if isinstance(self._restart_strategy,
+                                      RemoteDebRestartStrategy):
+                            service = RemoteDebRestartStrategy.service_name
+                            f.writelines([
+                                'sudo systemctl enable {}\n'.format(service),
+                                'sudo systemctl start {}'.format(service),
+                            ])
+                        else:
+                            f.writelines(self._restart_cmd_callback(
+                                self.get_session_id()))
             if not native:
                 if self._config.environment is Unset:
                     result = self._runner.run_job(job, job_state, ui=ui)
diff --git a/plainbox/impl/session/remote_assistant.py b/plainbox/impl/session/remote_assistant.py
index 1307bb1..4c0a379 100644
--- a/plainbox/impl/session/remote_assistant.py
+++ b/plainbox/impl/session/remote_assistant.py
@@ -210,11 +210,15 @@ class RemoteSessionAssistant():
 
     def _prepare_display_without_psutil(self):
         try:
-            value = check_output(
+            display_value = check_output(
                 'strings /proc/*/environ 2>/dev/null | '
                 'grep -m 1 -oP "(?<=DISPLAY=).*"',
                 shell=True, universal_newlines=True).rstrip()
-            return {'DISPLAY': value}
+            xauth_value = check_output(
+                'strings /proc/*/environ 2>/dev/null | '
+                'grep -m 1 -oP "(?<=XAUTHORITY=).*"',
+                shell=True, universal_newlines=True).rstrip()
+            return {'DISPLAY': display_value, 'XAUTHORITY': xauth_value}
         except CalledProcessError:
             return None
 
@@ -234,13 +238,20 @@ class RemoteSessionAssistant():
                 # quietly ignore the process that died before we had a chance
                 # to read the environment from them
                 continue
-            if ("DISPLAY" in p_environ and p_user != 'gdm'):  # gdm uses :1024
-                return {'DISPLAY': p_environ['DISPLAY']}
+            if (
+                "DISPLAY" in p_environ and
+                "XAUTHORITY" in p_environ and
+                p_user != 'gdm'
+            ):  # gdm uses :1024
+                return {
+                    'DISPLAY': p_environ['DISPLAY'],
+                    'XAUTHORITY': p_environ['XAUTHORITY']
+                    }
 
     @allowed_when(Idle)
     def start_session(self, configuration):
         self._reset_sa()
-        _logger.debug("start_session: %r", configuration)
+        _logger.info("start_session: %r", configuration)
         session_title = 'checkbox-slave'
         session_desc = 'checkbox-slave session'
         session_type = 'checkbox-slave'
@@ -459,7 +470,7 @@ class RemoteSessionAssistant():
         :returns:
             (state, payload) tuple.
         """
-        _logger.debug("whats_up()")
+        _logger.debug("whats_up() -> %r", self._state)
         payload = None
         if self._state == Running:
             payload = (
@@ -587,6 +598,7 @@ class RemoteSessionAssistant():
         return test_info_list
 
     def resume_by_id(self, session_id=None):
+        _logger.info("resume_by_id: %r", session_id)
         self._launcher = load_configs()
         resume_candidates = list(self._sa.get_resumable_sessions())
         if not session_id:
diff --git a/plainbox/impl/session/restart.py b/plainbox/impl/session/restart.py
index bd6a735..053f040 100644
--- a/plainbox/impl/session/restart.py
+++ b/plainbox/impl/session/restart.py
@@ -233,6 +233,29 @@ class RemoteSnappyRestartStrategy(IRestartStrategy):
                 raise
 
 
+class RemoteDebRestartStrategy(RemoteSnappyRestartStrategy):
+
+    """
+    Remote Restart strategy for checkbox installed from deb packages.
+    """
+
+    service_name = "checkbox-ng.service"
+
+    def get_session_resume_filename(self) -> str:
+        if self.debug:
+            return '/tmp/session_resume'
+        cache_dir = os.getenv('XDG_CACHE_HOME')
+        return os.path.join(cache_dir, 'session_resume')
+
+    def prime_application_restart(self, app_id: str,
+                                  session_id: str, cmd: str) -> None:
+        with open(self.session_resume_filename, 'wt') as f:
+            f.write(session_id)
+            os.fsync(f.fileno())
+        if cmd == self.service_name:
+            subprocess.call(['systemctl', 'disable', self.service_name])
+
+
 def detect_restart_strategy(session=None) -> IRestartStrategy:
     """
     Detect the restart strategy for the current environment.
@@ -260,16 +283,18 @@ def detect_restart_strategy(session=None) -> IRestartStrategy:
         except subprocess.CalledProcessError:
             return SnappyRestartStrategy()
 
-    # Classic snaps
-    snap_data = os.getenv('SNAP_DATA')
     try:
         if session:
-            app_blob = json.loads(session._context.state.metadata.app_blob.decode('UTF-8'))
+            app_blob = json.loads(
+                session._context.state.metadata.app_blob.decode('UTF-8'))
             session_type = app_blob.get("type")
         else:
             session_type = None
     except AttributeError:
         session_type = None
+
+    # Classic snaps
+    snap_data = os.getenv('SNAP_DATA')
     if snap_data:
         # Classic snaps w/ remote service enabled and in use
         if session_type == "checkbox-slave":
@@ -285,6 +310,16 @@ def detect_restart_strategy(session=None) -> IRestartStrategy:
         else:
             return SnappyRestartStrategy()
 
+    # debian checkbox-ng.service
+    if session_type == "checkbox-slave":
+        try:
+            subprocess.run(
+                ['systemctl', 'is-active', '--quiet', 'checkbox-ng.service'],
+                check=True)
+            return RemoteDebRestartStrategy()
+        except subprocess.CalledProcessError:
+                pass
+
     if os.path.isdir('/etc/xdg/autostart'):
         # NOTE: Assume this is a terminal application
         return XDGRestartStrategy(app_terminal=True)