Merge ~kissiel/checkbox-ng:fix-1966045-crash-on-startup into checkbox-ng:master

[Maciej Kisielewski]

Fix the problem behind traceback on Checkbox startup.

Instead of reverting the bad patch, with this MR I'm adding an arg that informs the restart strategy detection function of what kind of session the operator runs.

The local session behavior is unchanged. The local nature of those session is just made more explicit.

I also allowed myself to change the titles and types of remote sessions to 'remote' instead of 'checkbox-slave'. Thanks to heuristic I placed in SessionAssistant we don't _need_ to bump the Remote API version - all the detection happens on the service side and on its call tree.

I left the old naming so existing sessions can be supported. I also changed the title and descriptions future sessions will get, and this change also doesn't need a RAPI change, because if there is a checkbox remote/service version mismatch, then:

older service and newer controller: new names and description and old behavior will be used
newer service and older controller: old names and description + new behavior will be used

Tested on running on combinations of remote/services and local sessions.

[Jonathan Cave (jocave)]

27efa7f - happy with this change, they are reasonable defaults that can be easily superseded with settings from launcher etc

87b736e - Tested that I could indeed now start a new session. Also tested that with old controller side and fixed service side to confirm no need for remote API bump.

Thanks

review: Approve

diff --git a/checkbox_ng/launcher/subcommands.py b/checkbox_ng/launcher/subcommands.py
index b4f4350..69f4a4a 100644
--- a/checkbox_ng/launcher/subcommands.py
+++ b/checkbox_ng/launcher/subcommands.py
@@ -258,13 +258,13 @@ class Launcher(MainLoopStage, ReportsStage):
                 _logger.warning(_(
                     'Using automatically detected restart strategy'))
                 try:
-                    strategy = detect_restart_strategy()
+                    strategy = detect_restart_strategy(session_type='local')
                 except LookupError as exc:
                     _logger.warning(exc)
                     _logger.warning(_('Automatic restart disabled!'))
                     strategy = None
         else:
-            strategy = detect_restart_strategy()
+            strategy = detect_restart_strategy(session_type='local')
         if strategy:
             # gluing the command with pluses b/c the middle part
             # (launcher path) is optional
@@ -737,7 +737,7 @@ class Run(MainLoopStage):
             self.exporter, transport, self.exporter_opts)
 
     def _configure_restart(self):
-        strategy = detect_restart_strategy()
+        strategy = detect_restart_strategy(session_type='local')
         snap_name = os.getenv('SNAP_NAME')
         if snap_name:
             # NOTE: This implies that any snap wishing to include a
diff --git a/plainbox/impl/session/assistant.py b/plainbox/impl/session/assistant.py
index b2d6991..7afe1a9 100644
--- a/plainbox/impl/session/assistant.py
+++ b/plainbox/impl/session/assistant.py
@@ -245,7 +245,23 @@ class SessionAssistant:
         """
         UsageExpectation.of(self).enforce()
         if self._restart_strategy is None:
-            self._restart_strategy = detect_restart_strategy(self)
+            # 'checkbox-slave' is deprecated, it's here so people can resume
+            # old session, the next if statement can be changed to just checking
+            # for 'remote' title
+            # session_type = 'remote' if self._metadata.title == 'remote'
+            #                         else 'local'
+            # with the next release or when we do inclusive naming refactor
+            # or roughly after April of 2022
+            # TODO: REMOTE API RAPI:
+            # this heuristic of guessing session type from the title
+            # should be changed to a proper arg/flag with the Remote API bump
+            remote_titles = ('remote', 'checkbox-slave')
+            if self._metadata and self._metadata.title in remote_titles:
+                session_type = 'remote'
+            else:
+                session_type = 'local'
+            self._restart_strategy = detect_restart_strategy(
+                self, session_type=session_type)
         self._restart_cmd_callback = cmd_callback
         # Prevent second call to this method and to the
         # use_alternate_restart_strategy() method.
diff --git a/plainbox/impl/session/remote_assistant.py b/plainbox/impl/session/remote_assistant.py
index 86f1a83..5763e49 100644
--- a/plainbox/impl/session/remote_assistant.py
+++ b/plainbox/impl/session/remote_assistant.py
@@ -267,9 +267,9 @@ class RemoteSessionAssistant():
     def start_session(self, configuration):
         self._reset_sa()
         _logger.info("start_session: %r", configuration)
-        session_title = 'checkbox-slave'
-        session_desc = 'checkbox-slave session'
-        session_type = 'checkbox-slave'
+        session_title = 'remote'
+        session_desc = 'remote session'
+        session_type = 'remote'
 
         self._launcher = load_configs()
         if configuration['launcher']:
diff --git a/plainbox/impl/session/restart.py b/plainbox/impl/session/restart.py
index 2cc9c8b..0aa67b4 100644
--- a/plainbox/impl/session/restart.py
+++ b/plainbox/impl/session/restart.py
@@ -259,7 +259,7 @@ class RemoteDebRestartStrategy(RemoteSnappyRestartStrategy):
             subprocess.call(['systemctl', 'disable', self.service_name])
 
 
-def detect_restart_strategy(session=None) -> IRestartStrategy:
+def detect_restart_strategy(session=None, session_type=None) -> IRestartStrategy:
     """
     Detect the restart strategy for the current environment.
     :param session:
@@ -270,7 +270,12 @@ def detect_restart_strategy(session=None) -> IRestartStrategy:
         When no such object can be found.
     """
     # debian and unconfined checkbox-ng.service
-    if session_type == "checkbox-slave":
+    # 'checkbox-slave' is deprecated, it's here so people can resume old
+    # session, but the next line should become:
+    #  session_type == 'remote':
+    # with the next release or when we do inclusive naming refactor
+    # or roughly after April of 2022
+    if session_type in ('remote', 'checkbox-slave'):
         try:
             subprocess.run(
                 ['systemctl', 'is-active', '--quiet', 'checkbox-ng.service'],
@@ -310,7 +315,12 @@ def detect_restart_strategy(session=None) -> IRestartStrategy:
     snap_data = os.getenv('SNAP_DATA')
     if snap_data:
         # Classic snaps w/ remote service enabled and in use
-        if session_type == "checkbox-slave":
+        # 'checkbox-slave' is deprecated, it's here so people can resume old
+        # session, but the next line should become:
+        #  session_type == 'remote':
+        # with the next release or when we do inclusive naming refactor
+        # or roughly after April of 2022
+        if session_type in ('remote', 'checkbox-slave'):
             try:
                 slave_status = subprocess.check_output(
                     ['snapctl', 'get', 'slave'],