Merge lp:~kissiel/checkbox/suspend-v7 into lp:checkbox

[Maciej Kisielewski]

This MR brings v7 of session information.

v7 contains two additions:
 - timestamp of when the session was created
 - list of the test plan IDs that were selected during session creation

Note that this MR doesn't update any of the applications to use v7.

94a4203 plainbox:session: add 7th suspend format
b5f87a3 plainbox:session:resume: fix PEP-8 issues
9c0db7c plainbox:session:resume: bump date in header

[Zygmunt Krynicki (zyga)]

16:53 <@zyga> kissiel: -1 on posix timestamp, let's use something like datetime object
16:53 <@zyga> kissiel: I'm somewhat split on sessionstate, I'll explain why in a sec
16:54 <@zyga> kissiel: let's store more than one "timestamp": created / finished / updated -- created is fixed, finished is also fixed (when submitting) and
              last updated is changed each time we do stuff
16:55 <@zyga> kissiel: a single timestamp is less useful IMHO
16:55 * zyga wonders where is the patch that made SessionState a POD
16:55 <@zyga> kissiel: the UI should look at "updated" time stamps as that's what the user understands
16:55 <@zyga> kissiel: but the rest should be useful too
16:56 <@zyga> kissiel: I'd like to also see patches that make use fo the new field where it makes sense to do so (SA APIs)

I'm also somewhat split on adding more stuff to SessionState. We have the unfortunate split between SessionManager (global state), SesesionDeviceContext (device state) that contains a SessionState (which is the only thing that gets serialized). I'd like to see a follow-up where SessionManager is serialized instead. Storing global test plan list and per-device SessionState (which can later on just merge with SessionDeviceContext, the only reason that is separate was lack of SessionAssistant and difficulty of changing without breaking apps).

review: Needs Fixing

[Zygmunt Krynicki (zyga)]

16:56 <@zyga> kissiel: I'd like to also see patches that make use fo the new field where it makes sense to do so (SA APIs)
16:57 < kissiel> zyga: I used datetime at first go, but this required custom endcoders and decoders
16:57 < kissiel> this is why I changed that to posix
16:58 <@zyga> kissiel: that's okay, we can handle that on POD assignment
16:59 <@zyga> kissiel: timestamp is infinitely more useful
16:59 <@zyga> kissiel: when it carries a human readable time concept
16:59 <@zyga> kissiel: not when it carries a number
16:59 < kissiel> zyga: so were should the parsing happen?
16:59 < kissiel> zyga: in your opinion?
16:59 <@zyga> kissiel: you can fix that in one place, in the resume code that deserializes session meta-data
16:59 <@zyga> kissiel: it does assignment anyway
17:00 < kissiel> zyga: there's more than 1 place where json.loads happne
17:00 < kissiel> *happen
17:00 <@zyga> kissiel: can you be more specific?
17:00 <@zyga> this field is _assigned_ in one place
17:00 <@zyga> kissiel: json decode can and should just decode a string
17:01 <@zyga> kissiel: there is (and should be) only one place that assigns session_state.metadata.timestsamp
17:01 <@zyga> *stamp
17:01 < kissiel> mkay
17:01 < kissiel> I think I know what you mean
17:01 <@zyga> kissiel: fantastic, thanks
17:01 < kissiel> this will make use of the default json decoder
17:01 <@zyga> kissiel: well, just decode as before, but re-interpret that one field

[Sylvain Pineau (sylvain-pineau)]

There's one issue with resume unit tests, and we would need a test for resuming version 7 and a test to try the next (not yet available) 8 (test_resume_dispatch_v8):

======================================================================
ERROR: test_resume_dispatch_v7 (plainbox.impl.session.test_resume.SessionResumeHelperTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/resume.py", line 1154, in _validate
    value = obj[key]
KeyError: 'session'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/test_resume.py", line 248, in test_resume_dispatch_v7
    SessionResumeHelper([], None, None).resume(data)
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/resume.py", line 301, in resume
    return self._resume_json(json_repr, early_cb)
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/resume.py", line 339, in _resume_json
    return helper.resume_json(json_repr, early_cb)
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/resume.py", line 654, in resume_json
    session_repr = _validate(json_repr, key='session', value_type=dict)
  File "/home/sylvain/checkbox-repo/suspend-v7/plainbox/plainbox/impl/session/resume.py", line 1159, in _validate
    raise CorruptedSessionError(error_msg)
plainbox.impl.session.resume.CorruptedSessionError: Missing value for key 'session'

review: Needs Fixing

Unmerged revisions

4119. By Maciej Kisielewski on 2015-12-01

plainbox:session:resume: bump date in header

Signed-off-by: Maciej Kisielewski <email address hidden>

4118. By Maciej Kisielewski on 2015-12-01

plainbox:session:resume: fix PEP-8 issues

Signed-off-by: Maciej Kisielewski <email address hidden>

4117. By Maciej Kisielewski on 2015-12-01

plainbox:session: add 7th suspend format

This one brings session timestamp and info about the selected timestamps to the
session storage.

Signed-off-by: Maciej Kisielewski <email address hidden>

=== modified file 'plainbox/plainbox/impl/session/resume.py'
--- plainbox/plainbox/impl/session/resume.py	2015-09-09 08:28:34 +0000
+++ plainbox/plainbox/impl/session/resume.py	2015-12-01 14:50:52 +0000
@@ -1,6 +1,6 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012, 2013, 2014 Canonical Ltd.
+# Copyright 2012-2015 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
 #
@@ -200,6 +200,8 @@
             return SessionPeekHelper5().peek_json(json_repr)
         elif version == 6:
             return SessionPeekHelper6().peek_json(json_repr)
+        elif version == 7:
+            return SessionPeekHelper7().peek_json(json_repr)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -328,6 +330,9 @@
         elif version == 6:
             helper = SessionResumeHelper6(
                 self.job_list, self.flags, self.location)
+        elif version == 7:
+            helper = SessionResumeHelper7(
+                self.job_list, self.flags, self.location)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -550,6 +555,19 @@
     """
 
 
+class SessionPeekHelper7(SessionPeekHelper6):
+    """
+    Helper class for implementing session peek feature
+
+    This class works with data constructed by
+    :class:`~plainbox.impl.session.suspend.SessionSuspendHelper7` which has
+    been pre-processed by :class:`SessionPeekHelper` (to strip the initial
+    envelope).
+
+    The only goal of this class is to reconstruct session state meta-data.
+    """
+
+
 class SessionResumeHelper1(MetaDataHelper1MixIn):
 
     """
@@ -860,13 +878,13 @@
             # - All of the jobs that we need to run (aka, the desired jobs
             #   list). This is pretty obvious and it is exactly what must
             #   be preserved or trim_job_list() will complain
-            set([job.id for job in session.run_list])
+            set([job.id for job in session.run_list]) |
             # - All of the jobs that have representation (aka checksum).
             #   We want those jobs because they have results (or they would not
             #   end up in the list as of format v4). If they have results we
             #   just have to keep them. Perhaps the session had a different
             #   selection earlier, who knows.
-            | set(jobs_repr)
+            set(jobs_repr)
         )
         try:
             # NOTE: this should never raise ValueError (which signals that we
@@ -904,16 +922,16 @@
         if 'io_log_filename' in result_repr:
             io_log_filename = cls._load_io_log_filename(
                 result_repr, flags, location)
-            if (flags & cls.FLAG_FILE_REFERENCE_CHECKS_F
-                    and not os.path.isfile(io_log_filename)
-                    and flags & cls.FLAG_REWRITE_LOG_PATHNAMES_F):
+            if (flags & cls.FLAG_FILE_REFERENCE_CHECKS_F and
+                    not os.path.isfile(io_log_filename) and
+                    flags & cls.FLAG_REWRITE_LOG_PATHNAMES_F):
                 io_log_filename2 = cls._rewrite_pathname(io_log_filename,
                                                          location)
                 logger.warning(_("Rewrote file name from %r to %r"),
                                io_log_filename, io_log_filename2)
                 io_log_filename = io_log_filename2
-            if (flags & cls.FLAG_FILE_REFERENCE_CHECKS_F
-                    and not os.path.isfile(io_log_filename)):
+            if (flags & cls.FLAG_FILE_REFERENCE_CHECKS_F and
+                    not os.path.isfile(io_log_filename)):
                 raise BrokenReferenceToExternalFile(
                     _("cannot access file: {!r}").format(io_log_filename))
             return DiskJobResult({
@@ -1062,6 +1080,7 @@
             raise ValueError("Location must be a directory name")
         return os.path.join(location, io_log_filename)
 
+
 class SessionResumeHelper6(SessionResumeHelper5):
     """
     Helper class for implementing session resume feature
@@ -1119,6 +1138,12 @@
         return session
 
 
+class SessionResumeHelper7(SessionResumeHelper6):
+    """
+    Helper class for implementing session resume feature
+    """
+
+
 def _validate(obj, **flags):
     """Multi-purpose extraction and validation function."""
     # Fetch data from the container OR use json_repr directly

=== modified file 'plainbox/plainbox/impl/session/state.py'
--- plainbox/plainbox/impl/session/state.py	2015-09-03 11:00:21 +0000
+++ plainbox/plainbox/impl/session/state.py	2015-12-01 14:50:52 +0000
@@ -22,6 +22,7 @@
 ============================================================
 """
 import collections
+import datetime
 import logging
 import re
 
@@ -70,7 +71,7 @@
     FLAG_BOOTSTRAPPING = "bootstrapping"
 
     def __init__(self, title=None, flags=None, running_job_name=None,
-                 app_blob=None, app_id=None):
+                 app_blob=None, app_id=None, timestamp=None):
         """Initialize a new session state meta-data object."""
         if flags is None:
             flags = []
@@ -79,6 +80,7 @@
         self._running_job_name = running_job_name
         self._app_blob = app_blob
         self._app_id = app_id
+        self._timestamp = timestamp
 
     def __repr__(self):
         """Get the representation of the session state meta-data."""
@@ -185,6 +187,16 @@
             raise TypeError(_("app_id must be either None or str"))
         self._app_id = value
 
+    @property
+    def timestamp(self):
+        """
+        POSIX time when the session was created.
+
+        Note that this a simple number (POSIX timestamp) to store this value,
+        as it's easier to (de)serialize, and most of the time it will be used
+        for sorting.
+        """
+        return self._timestamp
 
 class SessionDeviceContext:
 
@@ -365,6 +377,7 @@
         added to the system.
         """
         self._test_plan_list = test_plan_list
+        self._state.selected_test_plans = [unit.id for unit in test_plan_list]
         self._invalidate_override_map()
         self._bulk_override_update()
         if test_plan_list:
@@ -790,7 +803,9 @@
         self._mandatory_job_list = []
         self._run_list = []
         self._resource_map = {}
-        self._metadata = SessionMetaData()
+        self._selected_test_plans = []
+        self._metadata = SessionMetaData(
+            timestamp=datetime.datetime.timestamp(datetime.datetime.utcnow()))
         super(SessionState, self).__init__()
 
     def trim_job_list(self, qualifier):
@@ -1199,6 +1214,16 @@
         """Map from resource id to a list of resource records."""
         return self._resource_map
 
+    @property
+    def selected_test_plans(self):
+        """List of test plan ids that have been selected for execution."""
+        return self._selected_test_plans
+
+    @selected_test_plans.setter
+    def selected_test_plans(self, test_plans):
+        """set the list of selected test plans."""
+        self._selected_test_plans = test_plans
+
     def get_outcome_stats(self):
         """
         Process the JobState map to get stats about the job outcomes.

=== modified file 'plainbox/plainbox/impl/session/suspend.py'
--- plainbox/plainbox/impl/session/suspend.py	2015-08-20 11:17:46 +0000
+++ plainbox/plainbox/impl/session/suspend.py	2015-12-01 14:50:52 +0000
@@ -83,6 +83,8 @@
 5) Same as '4' but DiskJobResult is stored with a relative pathname to the log
    file if session_dir is provided.
 6) Same as '5' plus store the list of mandatory jobs.
+7) Same as '6' plus store session's timestamp and the list of test plan ids
+   that were selected for execution.
 """
 
 import base64
@@ -647,5 +649,127 @@
             "metadata": self._repr_SessionMetaData(obj.metadata, session_dir),
         }
 
+class SessionSuspendHelper7(SessionSuspendHelper6):
+    """
+    Helper class for computing binary representation of a session.
+
+    The helper only creates a bytes object to save. Actual saving should
+    be performed using some other means, preferably using
+    :class:`~plainbox.impl.session.storage.SessionStorage`.
+
+    This class creates version '7' snapshots.
+    """
+
+    VERSION = 7
+
+    def _repr_SessionState(self, obj, session_dir):
+        """
+        Compute the representation of :class:`SessionState`.
+
+        :returns:
+            JSON-friendly representation
+        :rtype:
+            dict
+
+        The result is a dictionary with the following items:
+
+            ``jobs``:
+                Dictionary mapping job id to job checksum.
+                The checksum is computed with
+                :attr:`~plainbox.impl.job.JobDefinition.checksum`.
+                Two kinds of jobs are mentioned here:
+                    - jobs that ever ran and have a result
+                    - jobs that may run (are on the run list now)
+                The idea is to capture the "state" of the jobs that are
+                "important" to this session, that should be checked for
+                modifications when the session resumes later.
+
+            ``results``
+                Dictionary mapping job id to a list of results.
+                Each result is represented by data computed by
+                :meth:`_repr_JobResult()`. Only jobs that actually have
+                a result are mentioned here. The automatically generated
+                "None" result that is always present for every job is skipped.
+
+            ``desired_job_list``:
+                List of (ids) of jobs that are desired (to be executed)
+
+            ``mandatory_job_list``:
+                List of (ids) of jobs that must be executed
+
+            ``selected_test_plans``:
+                List of (ids) of test plans that were selected for execution
+
+            ``metadata``:
+                The representation of meta-data associated with the session
+                state object.
+        """
+        id_run_list = frozenset([job.id for job in obj.run_list])
+        return {
+            "jobs": {
+                state.job.id: state.job.checksum
+                for state in obj.job_state_map.values()
+                if not state.result.is_hollow or state.job.id in id_run_list
+            },
+            "results": {
+                state.job.id: [self._repr_JobResult(result, session_dir)
+                               for result in state.result_history]
+                for state in obj.job_state_map.values()
+                if len(state.result_history) > 0
+            },
+            "desired_job_list": [
+                job.id for job in obj.desired_job_list
+            ],
+            "mandatory_job_list": [
+                job.id for job in obj.mandatory_job_list
+            ],
+            "selected_test_plans": obj.selected_test_plans,
+            "metadata": self._repr_SessionMetaData(obj.metadata, session_dir),
+        }
+
+    def _repr_SessionMetaData(self, obj, session_dir):
+        """
+        Compute the representation of :class:`SessionMetaData`.
+
+        :returns:
+            JSON-friendly representation.
+        :rtype:
+            dict
+
+        The result is a dictionary with the following items:
+
+            ``title``:
+                Title of the session. Arbitrary text provided by the
+                application.
+
+            ``flags``:
+                List of strings that enumerate the flags the session is in.
+                There are some well-known flags but this list can have any
+                items it it.
+
+            ``running_job_name``:
+                Id of the job that was about to be executed before
+                snapshotting took place. Can be None.
+
+            ``app_blob``:
+                Arbitrary application specific binary blob encoded with base64.
+                This field may be null.
+
+            ``app_id``:
+                A string identifying the application that stored app_blob.
+                Thirs field may be null.
+
+            ``timestamp``:
+                A POSIX timestamp of the time when the session was created.
+        """
+        # reuse previous version
+        data = super(SessionSuspendHelper7, self)._repr_SessionMetaData(
+            obj, session_dir)
+        # and add the timestamp
+        data['timestamp'] = obj.timestamp
+        return data
+
+
+
 # Alias for the most recent version
-SessionSuspendHelper = SessionSuspendHelper6
+SessionSuspendHelper = SessionSuspendHelper7

=== modified file 'plainbox/plainbox/impl/session/test_suspend.py'
--- plainbox/plainbox/impl/session/test_suspend.py	2015-08-28 10:02:09 +0000
+++ plainbox/plainbox/impl/session/test_suspend.py	2015-12-01 14:50:52 +0000
@@ -41,6 +41,7 @@
 from plainbox.impl.session.suspend import SessionSuspendHelper4
 from plainbox.impl.session.suspend import SessionSuspendHelper5
 from plainbox.impl.session.suspend import SessionSuspendHelper6
+from plainbox.impl.session.suspend import SessionSuspendHelper7
 from plainbox.impl.testing_utils import make_job
 from plainbox.vendor import mock
 
@@ -989,6 +990,151 @@
         })
 
 
+class SessionSuspendHelper7Tests(SessionSuspendHelper6Tests):
+    """Tests for various methods of SessionSuspendHelper7."""
+    def setUp(self):
+        self.helper = SessionSuspendHelper7()
+        self.session_dir = None
+
+    def test_json_repr_current_version(self):
+        """
+        verify what the version field is
+        """
+        data = self.helper._json_repr(SessionState([]), self.session_dir)
+        self.assertEqual(data['version'], 7)
+
+    @mock.patch('datetime.datetime')
+    def test_suspend(self, mock_dt):
+        mock_dt.timestamp.return_value = 5
+        data = self.helper.suspend(SessionState([]), self.session_dir)
+        self.assertIsInstance(data, bytes)
+        self.assertEqual(gzip.decompress(data), (
+            b'{"session":{"desired_job_list":[],"jobs":{},'
+            b'"mandatory_job_list":[],"metadata":'
+            b'{"app_blob":null,"app_id":null,"flags":[],'
+            b'"running_job_name":null,"timestamp":5,"title":null},'
+            b'"results":{},"selected_test_plans":[]},"version":7}'))
+
+    @mock.patch('datetime.datetime')
+    def test_repr_SessionState_empty_session(self, mock_dt):
+        """
+        verify that representation of empty SessionState is okay
+        """
+        mock_dt.timestamp.return_value = 20
+        data = self.helper._repr_SessionState(
+            SessionState([]), self.session_dir)
+        self.assertEqual(data, {
+            'jobs': {},
+            'results': {},
+            'desired_job_list': [],
+            'mandatory_job_list': [],
+            'selected_test_plans': [],
+            'metadata': {
+                'title': None,
+                'flags': [],
+                'running_job_name': None,
+                'app_blob': None,
+                'app_id': None,
+                'timestamp': 20
+            },
+        })
+
+    def test_repr_SessionMetaData_empty_metadata(self):
+        """
+        verify that representation of empty SessionMetaData is okay
+        """
+        # all defaults with empty values
+        data = self.helper._repr_SessionMetaData(
+            SessionMetaData(), self.session_dir)
+        self.assertEqual(data, {
+            'title': None,
+            'flags': [],
+            'running_job_name': None,
+            'app_blob': None,
+            'app_id': None,
+            'timestamp': None,
+        })
+
+    def test_repr_SessionMetaData_typical_metadata(self):
+        """
+        verify that representation of typical SessionMetaData is okay
+        """
+        # no surprises here, just the same data copied over
+        data = self.helper._repr_SessionMetaData(SessionMetaData(
+            title='USB Testing session',
+            flags=['incomplete'],
+            running_job_name='usb/detect',
+            app_blob=b'blob',
+            app_id='com.canonical.certification.plainbox',
+            timestamp=100.0,
+        ), self.session_dir)
+        self.assertEqual(data, {
+            'title': 'USB Testing session',
+            'flags': ['incomplete'],
+            'running_job_name': 'usb/detect',
+            'app_blob': 'YmxvYg==',
+            'app_id': 'com.canonical.certification.plainbox',
+            'timestamp': 100.0
+        })
+
+    @mock.patch('datetime.datetime')
+    def test_repr_SessionState_typical_session(self, mock_dt):
+        """
+        verify the representation of a SessionState with some unused jobs
+
+        Unused jobs should just have no representation. Their checksum
+        should not be mentioned. Their results (empty results) should be
+        ignored.
+        """
+        mock_dt.timestamp.return_value = 40
+        used_job = JobDefinition({
+            "plugin": "shell",
+            "id": "used",
+            "command": "echo 'hello world'",
+        })
+        unused_job = JobDefinition({
+            "plugin": "shell",
+            "id": "unused",
+            "command": "echo 'hello world'",
+        })
+        used_result = MemoryJobResult({
+            "io_log": [
+                (0.0, "stdout", b'hello world\n'),
+            ],
+            'outcome': IJobResult.OUTCOME_PASS
+        })
+        session_state = SessionState([used_job, unused_job])
+        session_state.update_desired_job_list([used_job])
+        session_state.update_job_result(used_job, used_result)
+        data = self.helper._repr_SessionState(session_state, self.session_dir)
+        self.assertEqual(data, {
+            'jobs': {
+                'used': ('8c393c19fdfde1b6afc5b79d0a1617ecf7531cd832a16450dc'
+                         '2f3f50d329d373')
+            },
+            'results': {
+                'used': [{
+                    'comments': None,
+                    'execution_duration': None,
+                    'io_log': [[0.0, 'stdout', 'aGVsbG8gd29ybGQK']],
+                    'outcome': 'pass',
+                    'return_code': None
+                }]
+            },
+            'desired_job_list': ['used'],
+            'mandatory_job_list': [],
+            'selected_test_plans': [],
+            'metadata': {
+                'title': None,
+                'flags': [],
+                'running_job_name': None,
+                'app_blob': None,
+                'app_id': None,
+                'timestamp': 40,
+            },
+        })
+
+
 class RegressionTests(TestCase):
 
     def test_1388055(self):