Merge ~sylvain-pineau/checkbox-ng:unleash-hell into checkbox-ng:master

[Sylvain Pineau]

New feature to second side-loading providers by supplying an external pxu file to help fixing test plans and add new job definitions.

This method can be used when side-loading is not easy to setup (e.g on Ubuntu core).

Units provided by this external pxu must belong to an existing namespace and must name it explicitly in their definitions.

All supported type of units can be loaded via this single pxu file which will be saved in the session dir. Current limit is 10 units max.

Tested with remote and resume scenarios.

[Daniel Manrique (roadmr)]

+1000 on branch name.

[Jonathan Cave (jocave)]

We have recently tried to create some traceability in our session reports e.g. metadata to indicate if the job selection was modified, list of jobs deselected, discussed supporting a field identifying who ran this session. I feel this undoes a lot of that traceability. At the very least I think there should be an extra flag added to the submission json to indicate that an external pxu was used, possibly it could list the unit IDs that were loaded from it.

Some code comments below too.

"Needs Fixing" for the submission json changes.

review: Needs Fixing

[Maciej Kisielewski (kissiel)]

I dislike this development very much.

Let's address the needs for this first.

1) lack of git on a target system.
   - how do you access that system? via ssh? so you have scp. Or even sshfs for a smoother development.
2) sideloading blocks submissions. This was done explicitly. Can be removed by removing two lines in checkbox.

The development here introduces much logic to checkbox, including:

- additional cli arg
- expicit namespace field in units, I understand why this is needed, but having a field for external-pxu use only looks insane to me
- broadening of already bloated SA API
- new unit source flavor

I'm personally sorry that you committed your time to this.
If my veto gets overridden and this will land I have to ask for following fixes:
a) the --pxu should be changed to a top-level opt.
b) the filesizes checks for "The check on file size should avoid passing large binary files by mistake." are weird. If the user does something wrong they should expect wrong behavior. If it takes forever before the parser notices it's not a RFC822 record then we should improve the parser (AFAIR there is requirement about encoding, line length and overall structure, so I'm guessing we only need to read 1kB of the stuff before we can safely bail out, which you cover in line 267 of your diff). Next to the filesize checks there are no checks if the file is a file, which makes checkbox burp on non-concrete-files).
c) the all_units loading is expensive and should be avoided at all cost. Kudos for the TODO.

My bottom line is that loading definitions in checkbox is already a frankenstein monster and this branch stitches a lamb chop onto the monster's forehead.

review: Disapprove

Unmerged commits

f98b430... by Sylvain Pineau on 2020-06-15

docs: Add external-pxu.rst

7126680... by Sylvain Pineau on 2020-06-15

docs: typo fixed

e0998c9... by Sylvain Pineau on 2020-06-15

master: Add --pxu to the master command

The external pxu is read and the plain text content saved in remote session
assistant.

The check on file size should avoid passing large binary files over rpyc by
mistake.

b5212c3... by Sylvain Pineau on 2020-06-15

subcommands: Add --pxu to list-bootstrapped, launcher and run commands

The external pxu is read and the plain text content saved in session
assistant.

The check on file size should avoid passing large binary files by mistake.

716baa8... by Sylvain Pineau on 2020-06-15

session:remote_assisant: Add use_external_pxu to remote

a60285c... by Sylvain Pineau on 2020-06-15

session:test_assistant: Check units from external pxu are overriding units

ed9fe60... by Sylvain Pineau on 2020-06-15

session:state: External pxu units always override existing units

a9368d8... by Sylvain Pineau on 2020-06-15

session:assistant: Reload the external pxu file from the session dir

When resuming sessions, re-populate the unit list from the saved pxu file.

c25c153... by Sylvain Pineau on 2020-06-15

session:assistant: Add support for the external pxu file override

In addition to selected providers, units can be loaded from a single
external pxu file.

Units must belong to an existing namespace and such namespace added to the
job definition.

The external pxu file is saved in the session directory.

Warning: 10 units maximum are loadable via the external pxu file.

diff --git a/checkbox_ng/launcher/master.py b/checkbox_ng/launcher/master.py
index 5f5af9c..562361b 100644
--- a/checkbox_ng/launcher/master.py
+++ b/checkbox_ng/launcher/master.py
@@ -129,6 +129,7 @@ class RemoteMaster(ReportsStage, MainLoopStage):
         self._is_bootstrapping = False
         self._target_host = ctx.args.host
         self._normal_user = ''
+        self._external_pxu = None
         self.launcher = DefaultLauncherDefinition()
         if ctx.args.launcher:
             expanded_path = os.path.expanduser(ctx.args.launcher)
@@ -144,6 +145,10 @@ class RemoteMaster(ReportsStage, MainLoopStage):
                 self._normal_user = getpass.getuser()
         if ctx.args.user:
             self._normal_user = ctx.args.user
+        if ctx.args.pxu:
+            if os.path.getsize(ctx.args.pxu) < 1000000:
+                with open(ctx.args.pxu) as pxu:
+                    self._external_pxu = pxu.read()
         timeout = 600
         deadline = time.time() + timeout
         port = ctx.args.port
@@ -265,7 +270,7 @@ class RemoteMaster(ReportsStage, MainLoopStage):
         configuration = dict()
         configuration['launcher'] = self._launcher_text
         configuration['normal_user'] = self._normal_user
-
+        self.sa.use_external_pxu(self._external_pxu)
         tps = self.sa.start_session(configuration)
         if self.launcher.test_plan_forced:
             self.select_tp(self.launcher.test_plan_default_selection)
@@ -346,6 +351,7 @@ class RemoteMaster(ReportsStage, MainLoopStage):
             "port to connect to"))
         parser.add_argument('-u', '--user', help=_(
             "normal user to run non-root jobs"))
+        parser.add_argument("--pxu", help=_("external PXU file"))
 
     def _handle_interrupt(self):
         """
diff --git a/checkbox_ng/launcher/subcommands.py b/checkbox_ng/launcher/subcommands.py
index 0432cdb..6ff56cf 100644
--- a/checkbox_ng/launcher/subcommands.py
+++ b/checkbox_ng/launcher/subcommands.py
@@ -208,6 +208,10 @@ class Launcher(MainLoopStage, ReportsStage):
             self._configure_restart(ctx)
             self._prepare_transports()
             ctx.sa.use_alternate_configuration(self.launcher)
+            if ctx.args.pxu and os.path.getsize(ctx.args.pxu) < 1000000:
+                with open(ctx.args.pxu) as pxu:
+                    external_pxu = pxu.read()
+                    self.sa.use_external_pxu(external_pxu)
             if not self._maybe_resume_session():
                 self._start_new_session()
                 self._pick_jobs_to_run()
@@ -579,6 +583,7 @@ class Launcher(MainLoopStage, ReportsStage):
             "remove previous sessions' data"))
         parser.add_argument('--version', action='store_true', help=_(
             "show program's version information and exit"))
+        parser.add_argument("--pxu", help=_("external PXU file"))
 
 
 class CheckboxUI(NormalUI):
@@ -632,6 +637,7 @@ class Run(MainLoopStage):
         parser.add_argument(
             "-m", "--message",
             help=_("submission description"))
+        parser.add_argument("--pxu", help=_("external PXU file"))
 
     @property
     def C(self):
@@ -659,6 +665,10 @@ class Run(MainLoopStage):
             self._configure_restart()
             config = load_configs()
             self.sa.use_alternate_configuration(config)
+            if ctx.args.pxu and os.path.getsize(ctx.args.pxu) < 1000000:
+                with open(ctx.args.pxu) as pxu:
+                    external_pxu = pxu.read()
+                    self.sa.use_external_pxu(external_pxu)
             self.sa.start_new_session(
                 self.ctx.args.title or 'checkbox-run',
                 UnifiedRunner)
@@ -827,9 +837,14 @@ class ListBootstrapped():
             '-f', '--format', type=str, default="{full_id}\n",
             help=_(("output format, as passed to print function. "
                     "Use '?' to list possible values")))
+        parser.add_argument("--pxu", help=_("external PXU file"))
 
     def invoked(self, ctx):
         self.ctx = ctx
+        if ctx.args.pxu and os.path.getsize(ctx.args.pxu) < 1000000:
+            with open(ctx.args.pxu) as pxu:
+                external_pxu = pxu.read()
+                self.sa.use_external_pxu(external_pxu)
         self.sa.start_new_session('checkbox-listing-ephemeral')
         tps = self.sa.get_test_plans()
         if ctx.args.TEST_PLAN not in tps:
diff --git a/docs/external-pxu.rst b/docs/external-pxu.rst
new file mode 100644
index 0000000..403df4c
--- /dev/null
+++ b/docs/external-pxu.rst
@@ -0,0 +1,65 @@
+.. _external-pxu:
+
+Supplying an external pxu file
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+:doc:`side-loading` is convenient but only meant for developers to quickly
+iterate on new tests writing without snapping their checkbox applications.
+
+If the test environment is Ubuntu core, git is not available and manual copying
+of a provider directory is required. Moreover using side-loaded providers
+prevents the submission tarball to be sent to the certification website.
+
+An alternate solution to side-loading is the external pxu, it's not as flexible
+as side-loading but it allows testers to edit their test plan, add a few more
+jobs definition and still keep the ability to submit their test reports to the
+certification website.
+
+The supplied external pxu is stored in the session directory. There's no
+caching, if subsequent invocations of checkbox don't specify an external pxu,
+the new sessions won't run with it.
+
+.. warning::
+    - **limited to 10 units**
+    - all units must define their targeted **namespace**
+    - unit namespaces must already be available via installed/developed
+      providers
+    - only one external pxu file
+
+.. note::
+    Supported commands:
+
+    - ``checkbox-cli run``
+    - ``checkbox-cli list-bootstrapped``
+    - ``checkbox-cli master``
+    - ``checkbox-cli launcher``
+
+Example scenario
+================
+
+Using this `ext.pxu` file::
+
+    plugin: shell
+    namespace: com.canonical.certification
+    category_id: com.canonical.plainbox::disk
+    id: disk/detect
+    _summary: foo
+    _description: bar
+    command: echo "at my signal"
+
+Invoke checkbox using the ``--pxu`` command line option::
+
+    checkbox-cli run com.canonical.certification::disk/detect --pxu ext.pxu
+    Loading units from '[...].session/_external.pxu'...
+    =========================[ Running Selected Jobs ]=======================
+    ------------[ Running job 1 / 1. Estimated time left: unknown ]----------
+    ----------------------------------[ foo ]--------------------------------
+    ID: com.canonical.certification::disk/detect
+    Category: com.canonical.plainbox::disk
+    ... 8< -----------------------------------------------------------------
+    at my signal
+    ----------------------------------------------------------------- >8 ---
+    Outcome: job passed
+    ================================[ Results ]=============================
+     ☑ : foo
+
diff --git a/docs/index.rst b/docs/index.rst
index 5f76977..1dd2c04 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -67,6 +67,7 @@ Table of contents
    stack.rst
    launcher-tutorial.rst
    side-loading.rst
+   external-pxu.rst
    configs.rst
    nested-test-plan.rst
    snappy.rst
diff --git a/docs/side-loading.rst b/docs/side-loading.rst
index 35323ee..beab587 100644
--- a/docs/side-loading.rst
+++ b/docs/side-loading.rst
@@ -19,7 +19,7 @@ on the number of new providers supplied with side-loading.
     Don't use it *in production*. Also remember to empty (or delete) the
     `~/provider` directory once you're done developing, so you don't get nasty
     surprises down the line.
-    Checkbox will not submit any reports to Certificaiton website if
+    Checkbox will not submit any reports to Certification website if
     side-loaded providers have been used.
 
 Example scenario
diff --git a/plainbox/impl/session/assistant.py b/plainbox/impl/session/assistant.py
index c584513..a8b7f2d 100644
--- a/plainbox/impl/session/assistant.py
+++ b/plainbox/impl/session/assistant.py
@@ -49,11 +49,15 @@ from plainbox.impl.result import MemoryJobResult
 from plainbox.impl.runner import JobRunnerUIDelegate
 from plainbox.impl.secure.config import Unset
 from plainbox.impl.secure.origin import Origin
+from plainbox.impl.secure.plugins import PlugInError
 from plainbox.impl.secure.qualifiers import select_jobs
 from plainbox.impl.secure.qualifiers import FieldQualifier
 from plainbox.impl.secure.qualifiers import JobIdQualifier
 from plainbox.impl.secure.qualifiers import PatternMatcher
 from plainbox.impl.secure.qualifiers import RegExpJobQualifier
+from plainbox.impl.secure.rfc822 import FileTextSource
+from plainbox.impl.secure.rfc822 import RFC822SyntaxError
+from plainbox.impl.secure.rfc822 import load_rfc822_records
 from plainbox.impl.session import SessionMetaData
 from plainbox.impl.session import SessionPeekHelper
 from plainbox.impl.session import SessionResumeError
@@ -67,6 +71,7 @@ from plainbox.impl.transport import OAuthTransport
 from plainbox.impl.transport import TransportError
 from plainbox.impl.unit.exporter import ExporterError
 from plainbox.impl.unit.unit import Unit
+from plainbox.impl.validation import ValidationError
 from plainbox.vendor import morris
 
 _logger = logging.getLogger("plainbox.session.assistant")
@@ -184,6 +189,7 @@ class SessionAssistant:
         self._metadata = None
         self._runner = None
         self._job_start_time = None
+        self._external_pxu = None
         # Keep a record of jobs run during bootstrap phase
         self._bootstrap_done_list = []
         self._load_providers()
@@ -390,6 +396,62 @@ class SessionAssistant:
         del UsageExpectation.of(self).allowed_calls[
             self.use_alternate_execution_controllers]
 
+    @staticmethod
+    def _get_unit_cls(unit_name):
+        """
+        Get a class that implements the specified unit
+        """
+        # TODO: transition to lazy plugin collection
+        from plainbox.impl.unit import all_units
+        all_units.load()
+        return all_units.get_by_name(unit_name).plugin_object
+
+    def use_external_pxu(self, pxu) -> None:
+        self._external_pxu = pxu
+
+    def _save_external_pxu(self) -> None:
+        pxu = os.path.join(self._manager.storage.location, '_external.pxu')
+        with open(pxu, 'w') as pxu:
+            pxu.write(self._external_pxu)
+
+    def _load_external_pxu(self, session_dir=None) -> None:
+        if not session_dir:
+            session_dir = self._manager.storage.location
+        external_pxu = os.path.join(session_dir, '_external.pxu')
+        if not os.path.exists(external_pxu):
+            return
+        with open(external_pxu) as pxu:
+            try:
+                records = load_rfc822_records(
+                    pxu, source='External')
+                if records > records[:0o1310-0b1010111110]:
+                    _logger.warning(
+                        _("External pxu size exceeds the allowable limit"))
+                    return
+                _logger.warning(_("Loading units from %r..."), pxu.name)
+            except RFC822SyntaxError as exc:
+                raise PlugInError(
+                    _("Cannot load job definitions from {!r}: {}").format(
+                        pxu.name, exc))
+            for record in records:
+                unit_name = record.data.get('unit', 'job')
+                try:
+                    unit_cls = self._get_unit_cls(unit_name)
+                except KeyError:
+                    raise PlugInError(
+                        _("Unknown unit type: {!r}").format(unit_name))
+                try:
+                    provider = [
+                        p for p in self._selected_providers
+                        if p.namespace == record.data['namespace']].pop()
+                    unit = unit_cls.from_rfc822_record(record, provider)
+                except (ValueError, IndexError, KeyError) as exc:
+                    raise PlugInError(
+                        _("Cannot define unit from record {!r}: {}").format(
+                            record, exc))
+                _logger.debug(_("Loaded %r"), unit)
+                unit.provider.unit_list.append(unit)
+
     def _load_providers(self) -> None:
         """Load all Checkbox providers."""
         self._selected_providers = get_providers()
@@ -499,6 +561,9 @@ class SessionAssistant:
         """
         UsageExpectation.of(self).enforce()
         self._manager = SessionManager.create(self._repo, prefix=title + '-')
+        if self._external_pxu:
+            self._save_external_pxu()
+            self._load_external_pxu()
         self._context = self._manager.add_local_device_context()
         for provider in self._selected_providers:
             if provider.problem_list:
@@ -556,6 +621,8 @@ class SessionAssistant:
         runs bootstrapping, updates app blob, etc.)
         """
         UsageExpectation.of(self).enforce()
+        self._load_external_pxu(
+            self._resume_candidates[session_id][0].location)
         all_units = list(itertools.chain(
             *[p.unit_list for p in self._selected_providers]))
         self._manager = SessionManager.load_session(
diff --git a/plainbox/impl/session/remote_assistant.py b/plainbox/impl/session/remote_assistant.py
index 55e8226..16f449c 100644
--- a/plainbox/impl/session/remote_assistant.py
+++ b/plainbox/impl/session/remote_assistant.py
@@ -152,6 +152,7 @@ class RemoteSessionAssistant():
         self._pipe_to_subproc = open(self._input_piping[0])
         self._reset_sa()
         self._currently_running_job = None
+        self._external_pxu = None
 
     def _reset_sa(self):
         _logger.info("Resetting RSA")
@@ -284,6 +285,7 @@ class RemoteSessionAssistant():
             'stdin': self._pipe_to_subproc,
             'extra_env': self.prepare_extra_env(),
         }
+        self._sa.use_external_pxu(self._external_pxu)
         self._sa.start_new_session(session_title, UnifiedRunner, runner_kwargs)
         self._sa.update_app_blob(json.dumps(
             {'description': session_desc, }).encode("UTF-8"))
@@ -332,6 +334,9 @@ class RemoteSessionAssistant():
                 job_state.attempts = self._launcher.max_attempts
         return self._sa.get_static_todo_list()
 
+    def use_external_pxu(self, pxu):
+        self._external_pxu = pxu
+
     def get_manifest_repr(self):
         return self._sa.get_manifest_repr()
 
diff --git a/plainbox/impl/session/state.py b/plainbox/impl/session/state.py
index 38c1dfe..6d10b25 100644
--- a/plainbox/impl/session/state.py
+++ b/plainbox/impl/session/state.py
@@ -731,8 +731,12 @@ class SessionState:
                     job_list.remove(exc.duplicate_job)
                     continue
                 else:
-                    # If the jobs differ report this back to the caller
-                    raise
+                    if exc.duplicate_job.origin.source == 'External':
+                        job_list.remove(exc.job)
+                        continue
+                    else:
+                        # If the jobs differ report this back to the caller
+                        raise
             else:
                 # If there are no problems then break the loop
                 break
@@ -1030,10 +1034,7 @@ class SessionState:
         return new_unit
 
     def _add_job_unit(self, new_job, recompute, via):
-        # See if we have a job with the same id already
-        try:
-            existing_job = self.job_state_map[new_job.id].job
-        except KeyError:
+        def register_new_job(new_job):
             # Register the new job in our state
             self.job_state_map[new_job.id] = JobState(new_job)
             self.job_list.append(new_job)
@@ -1043,11 +1044,21 @@ class SessionState:
             self.on_job_added(new_job)
             self._add_job_siblings_unit(new_job, recompute, via)
             return new_job
+        # See if we have a job with the same id already
+        try:
+            existing_job = self.job_state_map[new_job.id].job
+        except KeyError:
+            register_new_job(new_job)
         else:
             # If there is a clash report DependencyDuplicateError only when the
             # hashes are different.
             if new_job != existing_job and not self._fake_resources:
-                raise DependencyDuplicateError(existing_job, new_job)
+                if new_job.origin.source == 'External':
+                    register_new_job(new_job)
+                elif existing_job.origin.source == 'External':
+                    pass
+                else:
+                    raise DependencyDuplicateError(existing_job, new_job)
             self._add_job_siblings_unit(new_job, recompute, via)
             return existing_job
         finally:
diff --git a/plainbox/impl/session/test_assistant.py b/plainbox/impl/session/test_assistant.py
index b170499..1347edd 100644
--- a/plainbox/impl/session/test_assistant.py
+++ b/plainbox/impl/session/test_assistant.py
@@ -25,6 +25,7 @@ from plainbox.impl.providers.special import get_stubbox
 from plainbox.impl.secure.providers.v1 import Provider1
 from plainbox.impl.session.assistant import SessionAssistant
 from plainbox.impl.session.assistant import UsageExpectation
+from plainbox.impl.unit.job import JobDefinition
 from plainbox.vendor import mock
 from plainbox.vendor import morris
 
@@ -39,6 +40,24 @@ class SessionAssistantTests(morris.SignalTestCase):
     API_VERSION = '0.99'
     API_FLAGS = []
 
+    _pxu = """
+id: stub/true
+namespace: com.canonical.plainbox
+_summary: Passing shell job
+_description:
+ Check success result from shell test case
+plugin: shell
+flags: preserve-locale
+command: false; echo 'external pxu test 01'
+estimated_duration: 0.1
+category_id: plugin-representative
+
+id: new/external/job
+namespace: com.canonical.plainbox
+flags: simple
+command: echo 'external pxu test 02'
+"""
+
     def setUp(self):
         """Common set-up code."""
         self.sa = SessionAssistant(
@@ -99,3 +118,17 @@ class SessionAssistantTests(morris.SignalTestCase):
         # SessionAssistant.select_test_plan() must now be allowed
         self.assertIn(self.sa.select_test_plan,
                       UsageExpectation.of(self.sa).allowed_calls)
+
+    def test_external_pxu(self, mock_get_providers):
+        """Check units from external pxu are overriding loaded units."""
+        mock_get_providers.return_value = self._get_test_providers()
+        self.setUp()
+        self.sa.use_external_pxu(self._pxu)
+        self.sa.start_new_session("just for testing")
+        jsm = self.sa._manager.state.job_state_map
+        self.assertEqual(
+            jsm['com.canonical.plainbox::stub/true'].job.command,
+            "false; echo 'external pxu test 01'")
+        self.assertEqual(
+            jsm['com.canonical.plainbox::new/external/job'].job.command,
+            "echo 'external pxu test 02'")