From d34a1e8dc8ad22da4e1fa08f277edd20ada7d1f8 Mon Sep 17 00:00:00 2001
From: Jan David Mol <mol@astron.nl>
Date: Thu, 18 May 2017 15:00:52 +0000
Subject: [PATCH] Task #10801: Fixed indentations, added test for invalid
 root_resource_group, fixed test for RCUs, fixed duplicate test names

---
 .../ResourceAssigner/lib/assignment.py        | 18 ++++++++++-----
 .../test/t_resourceassigner.py                | 22 ++++++++++++++-----
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/SAS/ResourceAssignment/ResourceAssigner/lib/assignment.py b/SAS/ResourceAssignment/ResourceAssigner/lib/assignment.py
index 2938f37f53e..fd53a1484d7 100755
--- a/SAS/ResourceAssignment/ResourceAssigner/lib/assignment.py
+++ b/SAS/ResourceAssignment/ResourceAssigner/lib/assignment.py
@@ -584,14 +584,14 @@ class ResourceAssigner():
             logger.debug('findClaims: claimable_resources_list: %s', claimable_resources_list)
 
             # Obtain resource properties (if any)
-            properties = self.getResourcesProperties(needed_resources)
+            file_properties = self.getResourcesFilesProperties(needed_resources)
 
             # Collapse needed resources if only 1 claimable resource dict, e.g. global filesystem
             if len(claimable_resources_list) == 1:
                 logger.info('findClaims: collapsing needed_resources')
                 for type_id in needed_resources_by_type_id:
                     needed_resources_by_type_id[type_id] *= needed_resources['resource_count']
-                for prop in properties:
+                for prop in file_properties:
                     if prop['type'] in self.summablePropTypeIds():
                         prop['value'] *= needed_resources['resource_count']
                 needed_resources['resource_count'] = 1
@@ -606,10 +606,12 @@ class ResourceAssigner():
                 unclaimable_resources.append(needed_resources)
                 continue
 
-            # add resource properties to storage claims
+            # add resource properties
             for claim in more_claims:
+                # the 'file_properties' should only be added to storage resources,
+                # as they encode the size of the files
                 if claim['resource_type_id'] == self.resource_types['storage']:
-                    claim['properties'] = properties
+                    claim['properties'] = file_properties
 
             # add to the list of claims
             claims.extend(more_claims)
@@ -763,7 +765,11 @@ class ResourceAssigner():
         claims = []
 
         for res_type, claim_size in needed_resources.items():
-            claim = {'starttime': None, 'endtime': None, 'status': 'claimed'}
+            # claim starttime/endtime is needed by RADB, but will be annotated later in tieClaimsToTask.
+            # We do this to separate responsibilities. The scheduling functions (findClaims and helpers)
+            # only depend on the available resources (between start and end time) and the
+            # resources required by the task, but not on the actual task.
+            claim = {'starttime': None, 'endtime': None, 'properties': [], 'status': 'claimed'}
             claim['resource_id'] = claimable_resources[res_type]['id']
             claim['resource_type_id'] = res_type # used internally, not propagated to radb
 
@@ -821,7 +827,7 @@ class ResourceAssigner():
 
         return properties
 
-    def getResourcesProperties(self, needed_resources):
+    def getResourcesFilesProperties(self, needed_resources):
         """ Return all the file properties of a given set of resources (if any). """
 
         input_files  = needed_resources.get('input_files')
diff --git a/SAS/ResourceAssignment/ResourceAssigner/test/t_resourceassigner.py b/SAS/ResourceAssignment/ResourceAssigner/test/t_resourceassigner.py
index b102fbaaaaf..8fbcba1a632 100755
--- a/SAS/ResourceAssignment/ResourceAssigner/test/t_resourceassigner.py
+++ b/SAS/ResourceAssignment/ResourceAssigner/test/t_resourceassigner.py
@@ -317,7 +317,7 @@ class ResourceAssignerTest(unittest.TestCase):
 
     storage_claim = {
         'resource_id': cep4storage_resource_id,
-	'resource_type_id': 5,
+	    'resource_type_id': 5,
         'starttime': task_start_time,
         'used_rcus': None,
         'endtime': task_end_time + datetime.timedelta(days=365),
@@ -332,12 +332,13 @@ class ResourceAssignerTest(unittest.TestCase):
 
     bandwidth_claim = {
         'resource_id': cep4bandwidth_resource_id,
-	'resource_type_id': 3,
+	    'resource_type_id': 3,
         'starttime': task_start_time,
         'used_rcus': None,
         'endtime': task_end_time,
         'status': 'claimed',
-        'claim_size': rerpc_needed_claim_for_bandwidth_size
+        'claim_size': rerpc_needed_claim_for_bandwidth_size,
+        'properties': []
     }
 
     specification_claims = [bandwidth_claim, storage_claim]
@@ -1994,11 +1995,11 @@ class ResourceAssignerTest(unittest.TestCase):
             'resource_id': 212,
             'resource_type_id': 2,
             'starttime': self.task_start_time,
-            'used_rcus': None,
             'endtime': self.task_end_time,
             'status': 'claimed',
             'used_rcus': used_rcus,
-            'claim_size': used_rcus.count('1')
+            'claim_size': used_rcus.count('1'),
+            'properties': []
         }
 
         self.specification_tree['otdb_id'] = self.resources_with_rcus_otdb_id
@@ -2258,7 +2259,7 @@ class ResourceAssignerTest(unittest.TestCase):
 
         self.assertIsNone(claims)
 
-    def test_fitSingleResources_not_fit_multiple_resources(self):
+    def test_fitSingleResources_fit_multiple_resources(self):
         """ Given 2 needed resources, and 2 claimable resource sets, of which only one fits, fitSingleResources should return success. """
 
         needed_resources_by_type_id = { 3: 3000, 5: 500 }
@@ -2291,6 +2292,15 @@ class ResourceAssignerTest(unittest.TestCase):
 
         self.assertIsNotNone(claims)
 
+    def test_findClaims_invalid_resource_group(self):
+        """ If we try to find claims with a non-existing root_resource_group, findClaims should fail. """
+
+        estimates = [ { 'root_resource_group': 'MIDDLE EARTH', 'resource_count': 1, 'resource_types': { 'storage': 100 } } ]
+        claimable_resources_list = { self.cep4storage_resource_id:   { 'id': self.cep4storage_resource_id,   'type_id': 5, 'available_capacity': 400,  'active': True } }
+
+        with self.assertRaises(ValueError):
+            _ = self.resourceAssigner.findClaims(estimates, claimable_resources_list)
+
     def test_findClaims_fit(self):
         """ Given 2 needed resources (which we need 4 times), and 2 claimable resource sets, all 4 out of 4 fit, findClaims should return success. """
 
-- 
GitLab