Merge ~ack/maas:test-results-limits into maas:master

[Alberto Donato]

LP: #1722665 - apply limits to ScriptResults (per script), not ScriptSets

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/626/console
COMMIT: b738cd0a82d541a9a9cace7d93430963077d9685

review: Needs Fixing

[Blake Rouse (blake-rouse)]

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/627/console
COMMIT: 490405b946f610ca0d1aabd56b06e9ac2470b233

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/628/console
COMMIT: ed2ce6761e75475bd91eb308a17be4fc1c05c56e

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e41e34772f152ba4c98c7b59a839c99ad505b2e3

review: Approve

[Blake Rouse (blake-rouse)]

I think you can do it removing an unneeded query.

review: Needs Fixing

[Lee Trager (ltrager)]

This branch doesn't account for Physical block devices. As I describe below this means systems with more then 10 disks will be broken.

While we've agreed to keep storage limits for now I think we should give the user an option to store unlimited results. I would say if max_node_commissioning_results, max_node_testing_results, or max_node_installation_results are set to 0 keep unlimited results.

review: Needs Fixing

[Lee Trager (ltrager)]

I forgot that we already have code written to property list a scripts full history, accounting for storage devices. You should be able to do something like this

for script_result in new_script_set:
    if len(script_result.history) > limit:
        script_result.history[limit:].delete()

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/639/console
COMMIT: a9e4c84da57f22b7030db252baaf84e1b05900c3

review: Needs Fixing

[Alberto Donato (ack)]

@Lee updated code to use ScriptResult.history and added a test with block devices

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/652/console
COMMIT: 217ca2dcea0716b392068d62c26e497c41411904

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/653/console
COMMIT: fa8dbb5bbd2b2ce553b74c049f34cc6fda5e811f

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/654/console
COMMIT: a0f101176e116234a245c9a677dab17e1e45d2f4

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Looks good. I think @ltrager should also take another look before this lands.

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b test-results-limits lp:~ack/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/656/console
COMMIT: b4315528b23313de21c2d4c6e99343bb94068513

review: Needs Fixing

[Lee Trager (ltrager)]

LGTM!

review: Approve

diff --git a/src/maasserver/forms/settings.py b/src/maasserver/forms/settings.py
index 6673000..e659d0b 100644
--- a/src/maasserver/forms/settings.py
+++ b/src/maasserver/forms/settings.py
@@ -516,7 +516,7 @@ CONFIG_ITEMS = {
         },
     },
     'max_node_installation_results': {
-        'default': 1,
+        'default': 3,
         'form': forms.IntegerField,
         'form_kwargs': {
             'required': False,
diff --git a/src/maasserver/models/config.py b/src/maasserver/models/config.py
index c0a7c28..756b17b 100644
--- a/src/maasserver/models/config.py
+++ b/src/maasserver/models/config.py
@@ -100,7 +100,7 @@ def get_default_config():
         'completed_intro': False,
         'max_node_commissioning_results': 10,
         'max_node_testing_results': 10,
-        'max_node_installation_results': 1,
+        'max_node_installation_results': 3,
         # Notifications.
         'subnet_ip_exhaustion_threshold_count': 16,
         'http_boot': True,
diff --git a/src/metadataserver/models/scriptset.py b/src/metadataserver/models/scriptset.py
index 493165f..4987e83 100644
--- a/src/metadataserver/models/scriptset.py
+++ b/src/metadataserver/models/scriptset.py
@@ -15,6 +15,7 @@ from django.core.exceptions import (
 from django.db.models import (
     CASCADE,
     CharField,
+    Count,
     DateTimeField,
     ForeignKey,
     IntegerField,
@@ -104,28 +105,6 @@ def translate_result_type(result_type):
 
 class ScriptSetManager(Manager):
 
-    def _clean_old(self, node, result_type, new_script_set):
-        # Gather the list of existing script results of the given type for this
-        # node.
-        script_sets = self.filter(node=node, result_type=result_type)
-        # Exclude the newly created script_set so we don't try to remove it.
-        # This can happen when multiple script_sets have last_ping = None.
-        script_sets = script_sets.exclude(id=new_script_set.id)
-        # Sort by last_ping in reverse order so we only remove older entrees.
-        script_sets = script_sets.order_by('last_ping').reverse()
-        config_var = {
-            RESULT_TYPE.COMMISSIONING: 'max_node_commissioning_results',
-            RESULT_TYPE.TESTING: 'max_node_testing_results',
-            RESULT_TYPE.INSTALLATION: 'max_node_installation_results',
-        }
-        script_set_limit = Config.objects.get_config(config_var[result_type])
-        # Remove one from the script_set_limit to account for the newly created
-        # script_set.
-        script_set_limit -= 1
-        if script_sets.count() > script_set_limit:
-            for script_set in script_sets[script_set_limit:]:
-                script_set.delete(force=True)
-
     def create_commissioning_script_set(self, node, scripts=[], input={}):
         """Create a new commissioning ScriptSet with ScriptResults
 
@@ -140,7 +119,6 @@ class ScriptSetManager(Manager):
         script_set = self.create(
             node=node, result_type=RESULT_TYPE.COMMISSIONING,
             power_state_before_transition=node.power_state)
-        self._clean_old(node, RESULT_TYPE.COMMISSIONING, script_set)
 
         for script_name, data in NODE_INFO_SCRIPTS.items():
             if node.is_controller and not data['run_on_controller']:
@@ -149,6 +127,8 @@ class ScriptSetManager(Manager):
                 script_set=script_set, status=SCRIPT_STATUS.PENDING,
                 script_name=script_name)
 
+        self._clean_old(node, RESULT_TYPE.COMMISSIONING, script_set)
+
         # MAAS doesn't run custom commissioning scripts during controller
         # refresh.
         if node.is_controller:
@@ -242,6 +222,27 @@ class ScriptSetManager(Manager):
         self._clean_old(node, RESULT_TYPE.INSTALLATION, script_set)
         return script_set
 
+    def _clean_old(self, node, result_type, new_script_set):
+        config_var = {
+            RESULT_TYPE.COMMISSIONING: 'max_node_commissioning_results',
+            RESULT_TYPE.TESTING: 'max_node_testing_results',
+            RESULT_TYPE.INSTALLATION: 'max_node_installation_results',
+        }
+        limit = Config.objects.get_config(config_var[result_type])
+
+        for script_result in new_script_set.scriptresult_set.all():
+            first_to_delete = script_result.history.order_by(
+                '-id')[limit:limit + 1].first()
+            if first_to_delete is not None:
+                script_result.history.filter(
+                    pk__lte=first_to_delete.pk).delete()
+
+        # delete empty ScriptSets
+        empty_scriptsets = ScriptSet.objects.annotate(
+            results_count=Count('scriptresult')).filter(
+                node=node, results_count=0)
+        empty_scriptsets.delete()
+
 
 class ScriptSet(CleanSave, Model):
 
diff --git a/src/metadataserver/models/tests/test_scriptset.py b/src/metadataserver/models/tests/test_scriptset.py
index 80b3918..fff2430 100644
--- a/src/metadataserver/models/tests/test_scriptset.py
+++ b/src/metadataserver/models/tests/test_scriptset.py
@@ -28,6 +28,7 @@ from metadataserver.enum import (
     SCRIPT_TYPE,
 )
 from metadataserver.models import (
+    ScriptResult,
     ScriptSet,
     scriptset as scriptset_module,
 )
@@ -110,28 +111,6 @@ class TestTranslateResultType(MAASServerTestCase):
 class TestScriptSetManager(MAASServerTestCase):
     """Test the ScriptSet manager."""
 
-    def test_clean_old_ignores_new_script_set(self):
-        # Make sure the created script_set isn't cleaned up. This can happen
-        # when multiple script_sets last_ping are set to None.
-        script_set_limit = Config.objects.get_config(
-            'max_node_installation_results')
-        node = factory.make_Node()
-        for _ in range(script_set_limit * 2):
-            ScriptSet.objects.create(
-                node=node, result_type=RESULT_TYPE.INSTALLATION,
-                last_ping=None)
-
-        script_set = ScriptSet.objects.create_installation_script_set(node)
-        # If the new script_set was cleaned up this will fail.
-        node.current_installation_script_set = script_set
-        node.save()
-
-        self.assertEquals(
-            script_set_limit,
-            ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.INSTALLATION).count())
-
     def test_create_commissioning_script_set(self):
         custom_scripts = [
             factory.make_Script(script_type=SCRIPT_TYPE.COMMISSIONING)
@@ -220,36 +199,79 @@ class TestScriptSetManager(MAASServerTestCase):
             node.power_state, script_set.power_state_before_transition)
 
     def test_create_commissioning_script_set_cleans_up_past_limit(self):
-        script_set_limit = Config.objects.get_config(
-            'max_node_commissioning_results')
+        limit = Config.objects.get_config('max_node_commissioning_results')
         node = factory.make_Node()
-        for _ in range(script_set_limit * 2):
-            factory.make_ScriptSet(
-                node=node, result_type=RESULT_TYPE.COMMISSIONING)
-
-        ScriptSet.objects.create_commissioning_script_set(node)
-
-        self.assertEquals(
-            script_set_limit,
-            ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.COMMISSIONING).count())
+        for i in range(limit + 2):
+            ScriptSet.objects.create_commissioning_script_set(node)
+
+        for script_name in NODE_INFO_SCRIPTS:
+            self.assertEqual(
+                limit,
+                ScriptResult.objects.filter(script_name=script_name).count())
+
+    def test_create_commissioning_script_set_cleans_up_by_node(self):
+        limit = Config.objects.get_config('max_node_commissioning_results')
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        for i in range(limit + 2):
+            ScriptSet.objects.create_commissioning_script_set(node1)
+            ScriptSet.objects.create_commissioning_script_set(node2)
+
+        for script_name in NODE_INFO_SCRIPTS:
+            self.assertEqual(
+                limit,
+                ScriptResult.objects.filter(
+                    script_name=script_name,
+                    script_set__in=ScriptSet.objects.filter(
+                        node=node1)).count())
+            self.assertEqual(
+                limit,
+                ScriptResult.objects.filter(
+                    script_name=script_name,
+                    script_set__in=ScriptSet.objects.filter(
+                        node=node2)).count())
 
     def test_create_commissioning_script_set_cleans_up_current(self):
         Config.objects.set_config('max_node_commissioning_results', 1)
         node = factory.make_Node()
-        script_set = factory.make_ScriptSet(
-            node=node, result_type=RESULT_TYPE.COMMISSIONING)
-        node.current_commissioning_script_set = script_set
+
+        node.current_commissioning_script_set = (
+            ScriptSet.objects.create_commissioning_script_set(node))
         node.save()
 
         ScriptSet.objects.create_commissioning_script_set(node)
 
-        self.assertEquals(
-            1,
+        for script_name in NODE_INFO_SCRIPTS:
+            self.assertEqual(
+                1,
+                ScriptResult.objects.filter(script_name=script_name).count())
+
+    def test_create_commissioning_script_set_cleans_up_empty_sets(self):
+        Config.objects.set_config('max_node_commissioning_results', 1)
+        node = factory.make_Node()
+        ScriptSet.objects.create_commissioning_script_set(node)
+        script_set = ScriptSet.objects.create_commissioning_script_set(node)
+
+        # the first set is removed since it's empty
+        self.assertCountEqual(
+            [script_set],
             ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.COMMISSIONING).count())
+                result_type=RESULT_TYPE.COMMISSIONING).all())
+
+    def test_create_commissioning_script_set_cleans_up_per_node(self):
+        Config.objects.set_config('max_node_commissioning_results', 1)
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        ScriptSet.objects.create_commissioning_script_set(node1)
+        script_set1 = ScriptSet.objects.create_commissioning_script_set(node1)
+        ScriptSet.objects.create_commissioning_script_set(node2)
+        script_set2 = ScriptSet.objects.create_commissioning_script_set(node2)
+
+        # older sets for each node are removed
+        self.assertCountEqual(
+            [script_set1, script_set2],
+            ScriptSet.objects.filter(
+                result_type=RESULT_TYPE.COMMISSIONING).all())
 
     def test_create_commissioning_script_set_accepts_params(self):
         script = factory.make_Script(
@@ -337,40 +359,102 @@ class TestScriptSetManager(MAASServerTestCase):
             ScriptSet.objects.create_testing_script_set, node)
 
     def test_create_testing_script_set_cleans_up_past_limit(self):
-        script_set_limit = Config.objects.get_config(
-            'max_node_testing_results')
+        limit = Config.objects.get_config('max_node_testing_results')
         node = factory.make_Node()
-        for _ in range(script_set_limit * 2):
-            factory.make_ScriptSet(
-                node=node, result_type=RESULT_TYPE.TESTING)
-
         script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
+        for _ in range(limit + 2):
+            ScriptSet.objects.create_testing_script_set(
+                node, scripts=[script.name])
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(script_name=script.name).count())
+
+    def test_create_testing_script_set_cleans_up_by_node(self):
+        limit = Config.objects.get_config('max_node_testing_results')
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
+        for _ in range(limit + 2):
+            ScriptSet.objects.create_testing_script_set(
+                node1, scripts=[script.name])
+            ScriptSet.objects.create_testing_script_set(
+                node2, scripts=[script.name])
+
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(
+                script_set__in=ScriptSet.objects.filter(node=node1)).count())
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(
+                script_set__in=ScriptSet.objects.filter(node=node2)).count())
+
+    def test_create_testing_script_set_cleans_up_by_blockdevice(self):
+        Config.objects.set_config('max_node_testing_results', 1)
+        node = factory.make_Node()
+        for _ in range(2):
+            factory.make_PhysicalBlockDevice(node=node)
+
+        script = factory.make_Script(
+            script_type=SCRIPT_TYPE.TESTING, parameters={
+                'storage': {'type': 'storage'}})
         ScriptSet.objects.create_testing_script_set(
-            node, scripts=[script.name])
+            node, [script.name], {script.name: {'storage': 'all'}})
+        ScriptSet.objects.create_testing_script_set(
+            node, [script.name], {script.name: {'storage': 'all'}})
 
-        self.assertEquals(
-            script_set_limit,
-            ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.TESTING).count())
+        # one result is kept for each block device
+        self.assertEqual(
+            3,
+            ScriptResult.objects.filter(
+                script_set__in=ScriptSet.objects.filter(node=node)).count())
 
     def test_create_testing_script_set_cleans_up_current(self):
         Config.objects.set_config('max_node_testing_results', 1)
+        script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
         node = factory.make_Node()
-        script_set = factory.make_ScriptSet(
-            node=node, result_type=RESULT_TYPE.TESTING)
-        node.current_testing_script_set = script_set
+        node.current_testing_script_set = (
+            ScriptSet.objects.create_testing_script_set(
+                node, scripts=[script.name]))
         node.save()
 
-        script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
         ScriptSet.objects.create_testing_script_set(
             node, scripts=[script.name])
 
-        self.assertEquals(
+        self.assertEqual(
             1,
-            ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.TESTING).count())
+            ScriptResult.objects.filter(script_name=script.name).count())
+
+    def test_create_testing_script_set_cleans_up_empty_sets(self):
+        Config.objects.set_config('max_node_testing_results', 1)
+        script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
+        node = factory.make_Node()
+        ScriptSet.objects.create_testing_script_set(
+            node, scripts=[script.name])
+        script_set = ScriptSet.objects.create_testing_script_set(
+            node, scripts=[script.name])
+        # the first set is removed since it's empty
+        self.assertCountEqual(
+            [script_set],
+            ScriptSet.objects.filter(result_type=RESULT_TYPE.TESTING).all())
+
+    def test_create_testing_script_set_cleans_up_per_node(self):
+        Config.objects.set_config('max_node_testing_results', 1)
+        script = factory.make_Script(script_type=SCRIPT_TYPE.TESTING)
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        ScriptSet.objects.create_testing_script_set(
+            node1, scripts=[script.name])
+        script_set1 = ScriptSet.objects.create_testing_script_set(
+            node1, scripts=[script.name])
+        ScriptSet.objects.create_testing_script_set(
+            node2, scripts=[script.name])
+        script_set2 = ScriptSet.objects.create_testing_script_set(
+            node2, scripts=[script.name])
+        # older sets are removed for each node
+        self.assertCountEqual(
+            [script_set1, script_set2],
+            ScriptSet.objects.filter(result_type=RESULT_TYPE.TESTING).all())
 
     def test_create_testing_script_set_accepts_params(self):
         script = factory.make_Script(
@@ -410,41 +494,79 @@ class TestScriptSetManager(MAASServerTestCase):
         self.assertItemsEqual(
             [CURTIN_INSTALL_LOG],
             [script_result.name for script_result in script_set])
-        self.assertEquals(RESULT_TYPE.INSTALLATION, script_set.result_type)
+        self.assertEquals(
+            RESULT_TYPE.INSTALLATION, script_set.result_type)
         self.assertEquals(
             node.power_state, script_set.power_state_before_transition)
 
     def test_create_installation_script_set_cleans_up_past_limit(self):
-        script_set_limit = Config.objects.get_config(
-            'max_node_installation_results')
+        limit = Config.objects.get_config('max_node_installation_results')
         node = factory.make_Node()
-        for _ in range(script_set_limit * 2):
-            factory.make_ScriptSet(
-                node=node, result_type=RESULT_TYPE.INSTALLATION)
-
-        ScriptSet.objects.create_installation_script_set(node)
-
-        self.assertEquals(
-            script_set_limit,
-            ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.INSTALLATION).count())
+        for _ in range(limit + 2):
+            ScriptSet.objects.create_installation_script_set(node)
+
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(
+                script_name=CURTIN_INSTALL_LOG).count())
+
+    def test_create_installation_script_set_cleans_up_by_node(self):
+        limit = Config.objects.get_config('max_node_installation_results')
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        for _ in range(limit + 2):
+            ScriptSet.objects.create_installation_script_set(node1)
+            ScriptSet.objects.create_installation_script_set(node2)
+
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(
+                script_name=CURTIN_INSTALL_LOG,
+                script_set__in=ScriptSet.objects.filter(node=node1)).count())
+        self.assertEqual(
+            limit,
+            ScriptResult.objects.filter(
+                script_name=CURTIN_INSTALL_LOG,
+                script_set__in=ScriptSet.objects.filter(node=node2)).count())
 
     def test_create_installation_script_set_cleans_up_current(self):
-        Config.objects.get_config('max_node_installation_results', 1)
+        Config.objects.set_config('max_node_installation_results', 1)
         node = factory.make_Node()
-        script_set = factory.make_ScriptSet(
-            node=node, result_type=RESULT_TYPE.INSTALLATION)
-        node.current_installation_script_set = script_set
+        node.current_installation_script_set = (
+            ScriptSet.objects.create_installation_script_set(node))
         node.save()
 
         ScriptSet.objects.create_installation_script_set(node)
 
-        self.assertEquals(
+        self.assertEqual(
             1,
+            ScriptResult.objects.filter(
+                script_name=CURTIN_INSTALL_LOG).count())
+
+    def test_create_installation_script_set_cleans_up_empty_sets(self):
+        Config.objects.set_config('max_node_installation_results', 1)
+        node = factory.make_Node()
+        ScriptSet.objects.create_installation_script_set(node)
+        script_set = ScriptSet.objects.create_installation_script_set(node)
+        # the first set is removed since it's empty
+        self.assertCountEqual(
+            [script_set],
+            ScriptSet.objects.filter(
+                result_type=RESULT_TYPE.INSTALLATION).all())
+
+    def test_create_installation_script_set_cleans_up_per_node(self):
+        Config.objects.set_config('max_node_installation_results', 1)
+        node1 = factory.make_Node()
+        node2 = factory.make_Node()
+        ScriptSet.objects.create_installation_script_set(node1)
+        script_set1 = ScriptSet.objects.create_installation_script_set(node1)
+        ScriptSet.objects.create_installation_script_set(node2)
+        script_set2 = ScriptSet.objects.create_installation_script_set(node2)
+        # older results are deleted by node
+        self.assertCountEqual(
+            [script_set1, script_set2],
             ScriptSet.objects.filter(
-                node=node,
-                result_type=RESULT_TYPE.INSTALLATION).count())
+                result_type=RESULT_TYPE.INSTALLATION).all())
 
 
 class TestScriptSet(MAASServerTestCase):