Fixed: Update information for slave in slave list

slapos/proxy/views.py

@@ -354,13 +354,21 @@ def request_not_shared():
 
 def request_slave():
   """
-  Function to organise link between slave and master
+  Function to organise link between slave and master.
+  Slave information are stored in places:
+  1. slave table having information such as slave reference,
+      connection information to slave (given by slave master),
+      hosted_by and asked_by reference.
+  2. A dictionnary in slave_instance_list of selected slave master
+      in which are stored slave_reference, software_type, slave_title and
+      partition_parameter_kw stored as individual keys.
   """
   software_release = request.form['software_release'].encode()
   # some supported parameters
   software_type = request.form.get('software_type').encode()
   partition_reference = request.form.get('partition_reference', '').encode()
   partition_id = request.form.get('computer_partition_id', '').encode()
+  # Contain slave parameters to be given to slave master
   partition_parameter_kw = request.form.get('partition_parameter_xml', None)
   if partition_parameter_kw :
     partition_parameter_kw = xml_marshaller.xml_marshaller.loads(
@@ -368,6 +376,7 @@ def request_slave():
   else:
     partition_parameter_kw = {}
   instance_xml = dict2xml(partition_parameter_kw)
+  # We will search for a master corresponding to request
   args = []
   a = args.append
   q = 'SELECT * FROM %s WHERE software_release=?'
@@ -379,24 +388,31 @@ def request_slave():
   if partition is None:
     app.logger.warning('No partition corresponding to slave request')
     abort(408)
-  # Define Slave
+
+  # We set slave dictionnary as described in docstring
   new_slave = {}
   slave_reference = partition_id + '_' + partition_reference
   new_slave['slave_title'] = slave_reference
   new_slave['slap_software_type'] = software_type
   new_slave['slave_reference'] = slave_reference
+
   for key in partition_parameter_kw :
-    new_slave[key] = partition_parameter_kw[key]
+    if partition_parameter_kw[key] is not None :
+      new_slave[key] = partition_parameter_kw[key]
 
-  # Add slave to partition slave_list if not present
+  # Add slave to partition slave_list if not present else replace information
   slave_instance_list = partition['slave_instance_list']
   if slave_instance_list == None:
     slave_instance_list = []
   else:
     slave_instance_list = xml_marshaller.xml_marshaller.loads(slave_instance_list)
-  if not slave_reference in (x['slave_reference'] for x in slave_instance_list):
-    slave_instance_list.append(new_slave)
+    for x in slave_instance_list:
+      if x['slave_reference'] == slave_reference:
+        slave_instance_list.remove(x)
+
+  slave_instance_list.append(new_slave)
 
+  # Update slave_instance_list in database
   args = []
   a = args.append
   q = 'UPDATE %s SET slave_instance_list=?'

[Julien Muchembled]

Buggy or inefficient. Consider the following example:

>>> x = range(10)
>>> for i in x:
...   x.remove(i)
... 
>>> x
[1, 3, 5, 7, 9]

But I guess an element can't appear more than once. Then, you should break the loop to clarify the code.

More generally, using list.remove() when you already know the position of element is uselessly slow. I suggest:

  if slave_instance_list:
    slave_instance_list = xml_marshaller.xml_marshaller.loads(slave_instance_list)
    for i, x in enumerate(slave_instance_list):
      if x['slave_reference'] == slave_reference:
        slave_instance_list[i] = new_slave
        break
    else:
      slave_instance_list.append(new_slave)
  else:
    slave_instance_list = [new_slave]

/cc @bminusl

[Bryton Lacquement]

I replaced the code with your suggestion in this commit.