Merge lp:~kissiel/checkbox/fix-1397109 into lp:checkbox

[Maciej Kisielewski]

This MR brings garbage collecting of old session storage.

57d9d66 checkbox-touch: make exporting of results mark session as 'submitted'
1e65a00 checkbox-touch: add garbage collection of old session storage on start-up
10c39de checkbox-touch: add removal of 'bootstrapping' flag

[Zygmunt Krynicki (zyga)]

10:24 < kissiel> zyga, https://code.launchpad.net/~kissiel/checkbox/fix-1397109/+merge/243503 :)
10:25 <@zyga> kissiel: hmm, interesting
10:26 <@zyga> kissiel: I think it might be useful to keep those around indefinitely, I would certainly not remove them without first informing the
              user or let them choose
10:26 <@zyga> kissiel: the code itself looks okay
10:26 <@zyga> kissiel: load_checkpoint can raise exceptions
10:26 <@zyga> kissiel: IIRC
10:27 <@zyga> kissiel: I'd move the try/catch so that it covers only the .peek() call
10:27 <@zyga> kissiel: and move the garbage_flags set outside the loop
10:27 <@zyga> kissiel: intersection can be *probably* spelled as & for sets
10:27 <@zyga> kissiel: you may want to remove stuff you cannot peek either
10:28 < kissiel> zyga, I thought of that
10:28 < kissiel> zyga, but this would mean, that broken sessions are lost
10:28 < kissiel> zyga, and we/some1 might want to recover those
10:29 <@zyga> kissiel: yes, that's a good point, though some of those sessions are not broken (incompatible job error)
10:29 <@zyga> kissiel: just no longer resumable
10:29 <@zyga> ...
10:29 <@zyga> though
10:29 <@zyga> I'm wrong
10:29 <@zyga> peek doens't have that
10:29 <@zyga> only stuff like broken json/zip can kill peek
10:30 <@zyga> kissiel: oh, and missing i18n :D
10:30 <@zyga> kissiel: let's iterate and we'll se
10:30 <@zyga> see
10:30 < kissiel> zyga, cool, tahnks

[Sylvain Pineau (sylvain-pineau)]

kissiel: spineau, there's also https://code.launchpad.net/~kissiel/checkbox/fix-1397109/+merge/243503 laying around for a while
spineau: kissiel: indeed, one question/suggestion for this one. Suppose that the tester has to resume x times cbt to complete a single testrun, I think I gonna be annoying to see the popup when resuming and decline the removal.
spineau: kissiel: maybe we should make this a Header action available in the first screen(s)
kissiel: spineau, hmm, garbage collection is for sessions that cannot be removed
kissiel: spineau, so if they are only resuming they shouldn't see the popup
kissiel: spineau, if I understood your scenario correctly
spineau: kissiel: ok, so we'll only popup when the last session is completed
kissiel: spineau, yep
spineau: kissiel: could you help me a bit to understand what types of sessions we could find in the session storage folder?
spineau: kissiel: bootstrapped/uncompleted/?
kissiel: spineau, sure, 1) bootstrapping, session storage created, but test haven't been selected yet
spineau: kissiel: and which ones will be removed by the new function?
kissiel: spineau, so there's nothing to resume, cause we don't have list of test to run
kissiel: spineau, so it's 'garbage'
spineau: ok
kissiel: spineau, 2) uncompleted sessions are the one that have list of test associated with them and are potential candidates for resume
kissiel: spineau, we do not want to remove those when doing GC
spineau: kissiel: and we can have many of them potentially
kissiel: spineau, and finally 3) submitted - there's nothing to resume, cause all test were run and results were saved/submitted, ergo 'garbage'
kissiel: spineau, yep, there might be multiple uncompleted sessions
kissiel: spineau, as we have in cli
spineau: kissiel: but cbt is a single session client so far, so only the last one is resumable right. So keeping uncompleted sessions is not really used today
kissiel: spineau, they are stacking, so if you abandon session (ID: a) start a new one (ID:B), stop, and then rerun you will have an option of resuming B, after this is done, you'll still be able to run A (AFAIK)
spineau: kissiel: but no methods are there to remove session A (uncompleted)
kissiel: spineau, indeed. I think we should have some explicit flag to mark those sessions and garbage collect them... Maybe ABANDONED flag?
spineau: kissiel: maybe. Actually I would automatically GC the "bootstrapping" sessions without user confirmation and offer a menu action for uncompleted/saved
kissiel: spineau, cool, that's easily doable
spineau: kissiel: even two distinct actions, one for resumable sessions and one another for saved sessions as the tester may wnat to keep the io logs on the device even for saved sessions

[Zygmunt Krynicki (zyga)]

This is mostly okay, I have one question below.

Oh, and perhaps, can we have unit tests for the python part (just have a look, maybe it's easy, maybe not)?

review: Needs Information

[Maciej Kisielewski (kissiel)]

This branch is superseded by: https://code.launchpad.net/~kissiel/checkbox/fix-1397109-garbage-collection/+merge/260564

Unmerged revisions

3521. By Maciej Kisielewski on 2015-01-15

checkbox-touch: fix re-removal of session when doing GC

This patch makes checkbox-touch remove garbage collected sessions only once.
The problem with re-removal manifested when session had more than one flag set
(and more than one flag was matching to-be-removed flags)

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

3520. By Maciej Kisielewski on 2015-01-15

checkbox-touch: file header date bumps

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

3519. By Maciej Kisielewski on 2015-01-15

checkbox-touch: qml changes for new session garbage collection

This patch changes how session garbage collection is run in checkbox-touch.
This patch includes changes to qml side of things.

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

3518. By Maciej Kisielewski on 2015-01-15

checkbox-touch: new way of doing session garbage collection

This patch changes how session garbage collection is run in checkbox-touch.
This patch includes changes to python side of things.

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

3517. By Maciej Kisielewski on 2015-01-15

checkbox-touch: add flags parameter to python interface

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

3516. By Maciej Kisielewski on 2014-12-18

checkbox-touch: ask user if they want to run session storage clearing

This patch makes CBT show dialog when stale sesssions are found in the
filesystem.

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

3515. By Maciej Kisielewski on 2014-12-18

checkbox-touch: fixes after Zygmunt's review

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

3514. By Maciej Kisielewski on 2014-12-18

checkbox-touch: make exporting of results mark session as 'submitted'

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

3513. By Maciej Kisielewski on 2014-12-18

checkbox-touch: add garbage collection of old session storage on start-up

This patch makes Checkbox Touch remove old session storage when launching the
App.

Fixes: https://bugs.launchpad.net/checkbox-touch/+bug/1397109
Signed-off-by: Maciej Kisielewski <email address hidden>

3512. By Maciej Kisielewski on 2014-12-18

checkbox-touch: add removal of 'bootstrapping' flag

This patch makes Checkbox Touch remove 'bootstrapping' flag once user selects
test to run.

=== modified file 'checkbox-touch/components/CheckboxTouchApplication.qml'
--- checkbox-touch/components/CheckboxTouchApplication.qml	2014-11-27 22:31:39 +0000
+++ checkbox-touch/components/CheckboxTouchApplication.qml	2015-01-15 16:33:58 +0000
@@ -1,10 +1,11 @@
 /*
  * This file is part of Checkbox
  *
- * Copyright 2014 Canonical Ltd.
+ * Copyright 2014-2015 Canonical Ltd.
  *
  * Authors:
  * - Zygmunt Krynicki <[e-mail]>
+ * - Maciej Kisielewski <[e-mail]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -81,6 +82,18 @@
         });
     }
 
+    function getGarbageCollectionCandidates(continuation) {
+        request("get_garbage_collection_candidates", [], continuation, function(error) {
+            console.error("Unable to get garbage collection candidates");
+        });
+    }
+
+    function sessionGarbageCollect(flags, continuation) {
+        request("session_garbage_collect", [flags], continuation, function(error) {
+            console.error("Unable to run session garbage collection");
+        });
+    }
+
     function getTestplans(continuation) {
         request("get_testplans", [], continuation, function(error) {
             console.error("Unable to get testplans");
 
=== modified file 'checkbox-touch/main.qml'
--- checkbox-touch/main.qml	2014-12-11 22:27:53 +0000
+++ checkbox-touch/main.qml	2015-01-15 16:33:58 +0000
@@ -1,7 +1,7 @@
 /*
  * This file is part of Checkbox
  *
- * Copyright 2014 Canonical Ltd.
+ * Copyright 2014-2015 Canonical Ltd.
  *
  * Authors:
  * - Zygmunt Krynicki <[e-mail]>
@@ -115,7 +115,59 @@
                 "checkbox_touch" : applicationVersion,
                 "plainbox" : plainboxVersion
             };
-            resumeOrStartSession();
+            getGarbageCollectionCandidates(function(oldStorages) {
+                var buttons = [];
+                var flagToButtonText = {
+                    "incomplete": i18n.tr("Remove incomplete sessions"),
+                    "submitted": i18n.tr("Remove submitted sessions"),
+                }
+                var candidateFlags = [];
+                var totalCandidatesCount = 0;
+                for (var flag in flagToButtonText) {
+                    if (oldStorages[flag] && oldStorages[flag].length > 0) {
+                        candidateFlags.push(flag);
+                        // The next bit is somewhat convoluted because we need to capture flag variable
+                        // see http://stackoverflow.com/questions/12930272/javascript-closures-vs-anonymous-functions
+                        // for full explanation of closures in JS
+                        var newButton = {
+                            "text": flagToButtonText[flag],
+                            "color": UbuntuColors.red,
+                            "onClicked": (function(flag) {
+                                return function() {
+                                    console.log("Removing sessions marked with '"+ flag + "' flag");
+                                    sessionGarbageCollect([flag], resumeOrStartSession);
+                                }
+
+                            })(flag)};
+                        totalCandidatesCount += oldStorages[flag].length;
+                        buttons.push(newButton);
+                    }
+                }
+                if (buttons.length > 1) {
+                    //there is more than one button, so append 'remove all' button
+                    buttons.push({
+                        "text": i18n.tr("Remove all old sessions"),
+                        "color": UbuntuColors.red,
+                        "onClicked": function() {
+                             sessionGarbageCollect(candidateFlags, resumeOrStartSession);
+                            }
+                        });
+                }
+                if (buttons.length > 0) {
+                    buttons.push({
+                        "text": i18n.tr("Do not remove sessions"),
+                          "color": UbuntuColors.green,
+                          "onClicked": function() {
+                              sessionGarbageCollect([], resumeOrStartSession);
+                             }
+                        });
+                    CbtDialogLogic.showDialog(mainView, i18n.tr("Do you want to clear old sessions storage?\n"
+                                                                + " Number of sessions qualifying for removal: ") +
+                                                                totalCandidatesCount, buttons);
+                } else {
+                    sessionGarbageCollect([], resumeOrStartSession);
+                }
+            });
         }
         onSessionReady: {
             welcomePage.enableButton()
 
=== modified file 'checkbox-touch/py/checkbox_touch.py'
--- checkbox-touch/py/checkbox_touch.py	2015-01-07 14:52:42 +0000
+++ checkbox-touch/py/checkbox_touch.py	2015-01-15 16:33:58 +0000
@@ -1,6 +1,6 @@
 # This file is part of Checkbox.
 #
-# Copyright 2014 Canonical Ltd.
+# Copyright 2014-2015 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
 #   Maciej Kisielewski <[e-mail]>
@@ -333,6 +333,7 @@
         self.desired_test_ids = frozenset()
         self.test_plan_id = ""
         self.resume_candidate_storage = None
+        self._garbage_collection_candidates = None
         self.session_storage_repo = None
         self.timestamp = datetime.datetime.utcnow().isoformat()
 
@@ -465,6 +466,55 @@
         }
 
     @view
+    def get_garbage_collection_candidates(self):
+        """
+        Returns a dict keyed by flag type with value being list of ids of all
+        sessions available in the storage.
+        """
+        # every session *should* have at least one flag set (altough logically
+        # those flags are mutally exclusive
+        self._init_session_storage_repo()
+        self._garbage_collection_candidates = collections.defaultdict(list)
+        for storage in self.session_storage_repo.get_storage_list():
+            data = storage.load_checkpoint()
+            if len(data) == 0:
+                continue
+            try:
+                metadata = SessionPeekHelper().peek(data)
+                if (metadata.app_id == 'checkbox-touch'):
+                    _logger.debug(_("Found an existing session storage: %s"),
+                                  storage.id)
+                    # some sessions might be appended to more than one list
+                    for flag in metadata.flags:
+                        self._garbage_collection_candidates[flag].append(
+                            storage)
+            except SessionResumeError as exc:
+                _logger.info(_("Exception raised when trying to gather garbage"
+                               " collection candidates: %s"), str(exc))
+        return { flag: [storage.id for storage in storages] for
+                flag, storages in self._garbage_collection_candidates.items()}
+    @view
+    def session_garbage_collect(self, flags):
+        """
+        Removes all session with any flag matching ``flags`` or having
+        'bootstrapping' flag set.
+        """
+        if self._garbage_collection_candidates is None:
+            _logger.warning(_("session_garbage_collect called before "
+                              "get_garbage_collection_candidates"))
+            return
+        # if there are candidates with 'bootstrapping', remove them
+        if 'bootstrapping' in self._garbage_collection_candidates.keys():
+            flags.append('bootstrapping')
+        # we build a set of sessions marked for removal so we don't try to
+        # re-remove any of them
+        to_remove = set()
+        for flag in flags:
+            to_remove |= set(self._garbage_collection_candidates[flag])
+        while to_remove:
+            to_remove.pop().remove()
+
+    @view
     def get_testplans(self):
         all_units = self.manager.default_device_context.unit_list
         return {
@@ -576,6 +626,7 @@
         self.context.state.update_desired_job_list(desired_job_list)
         _logger.info("Run job list: %s", self.context.state.run_list)
         self.context.state.metadata.flags.add('incomplete')
+        self.context.state.metadata.flags.remove('bootstrapping')
         self._checkpoint()
 
     @view
@@ -680,6 +731,8 @@
         output_file = os.path.join(dirname, filename)
         with open(output_file, 'wb') as stream:
             self._export_session_to_stream(output_format, option_list, stream)
+        self.context.state.metadata.flags.add(SessionMetaData.FLAG_SUBMITTED)
+        self._checkpoint()
         return output_file
 
     def _get_user_directory_documents(self):