commit 249917493c0a5ca9190a23aa063a9e723d317fda
Author: Iustin Pop <iustin@google.com>
Date:   Tue Nov 25 12:57:29 2008 +0000

    Implement support for multi devices changes
    
    This big patch adds support for:
      - changing NIC/disks in the multi-device model
      - adding/removing NICs
      - adding/removing disks
    
    The patch is big and not very nice; the error checking paths are not
    very clear.
    
    The biggest problem is that from a simple instance.ATTR=VAL change
    (which didn't throw errors before) now we are creating and removing
    disks in this LU.
    
    Reviewed-by: imsnah

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 2459dd1..c09a7b7 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -3228,6 +3228,8 @@ def _CreateDisks(lu, instance):
       logging.error("Failed to create directory '%s'", file_storage_dir)
       return False
 
+  # Note: this needs to be kept in sync with adding of disks in
+  # LUSetInstanceParams
   for device in instance.disks:
     logging.info("Creating volume %s for instance %s",
                  device.iv_name, instance.name)
@@ -4667,15 +4669,107 @@ class LUSetInstanceParams(LogicalUnit):
   """
   HPATH = "instance-modify"
   HTYPE = constants.HTYPE_INSTANCE
-  _OP_REQP = ["instance_name", "hvparams"]
+  _OP_REQP = ["instance_name"]
   REQ_BGL = False
 
+  def CheckArguments(self):
+    if not hasattr(self.op, 'nics'):
+      self.op.nics = []
+    if not hasattr(self.op, 'disks'):
+      self.op.disks = []
+    if not hasattr(self.op, 'beparams'):
+      self.op.beparams = {}
+    if not hasattr(self.op, 'hvparams'):
+      self.op.hvparams = {}
+    self.op.force = getattr(self.op, "force", False)
+    if not (self.op.nics or self.op.disks or
+            self.op.hvparams or self.op.beparams):
+      raise errors.OpPrereqError("No changes submitted")
+
+    for item in (constants.BE_MEMORY, constants.BE_VCPUS):
+      val = self.op.beparams.get(item, None)
+      if val is not None:
+        try:
+          val = int(val)
+        except ValueError, err:
+          raise errors.OpPrereqError("Invalid %s size: %s" % (item, str(err)))
+        self.op.beparams[item] = val
+    # Disk validation
+    disk_addremove = 0
+    for disk_op, disk_dict in self.op.disks:
+      if disk_op == constants.DDM_REMOVE:
+        disk_addremove += 1
+        continue
+      elif disk_op == constants.DDM_ADD:
+        disk_addremove += 1
+      else:
+        if not isinstance(disk_op, int):
+          raise errors.OpPrereqError("Invalid disk index")
+      if disk_op == constants.DDM_ADD:
+        mode = disk_dict.setdefault('mode', constants.DISK_RDWR)
+        if mode not in (constants.DISK_RDONLY, constants.DISK_RDWR):
+          raise errors.OpPrereqError("Invalid disk access mode '%s'" % mode)
+        size = disk_dict.get('size', None)
+        if size is None:
+          raise errors.OpPrereqError("Required disk parameter size missing")
+        try:
+          size = int(size)
+        except ValueError, err:
+          raise errors.OpPrereqError("Invalid disk size parameter: %s" %
+                                     str(err))
+        disk_dict['size'] = size
+      else:
+        # modification of disk
+        if 'size' in disk_dict:
+          raise errors.OpPrereqError("Disk size change not possible, use"
+                                     " grow-disk")
+
+    if disk_addremove > 1:
+      raise errors.OpPrereqError("Only one disk add or remove operation"
+                                 " supported at a time")
+
+    # NIC validation
+    nic_addremove = 0
+    for nic_op, nic_dict in self.op.nics:
+      if nic_op == constants.DDM_REMOVE:
+        nic_addremove += 1
+        continue
+      elif nic_op == constants.DDM_ADD:
+        nic_addremove += 1
+      else:
+        if not isinstance(nic_op, int):
+          raise errors.OpPrereqError("Invalid nic index")
+
+      # nic_dict should be a dict
+      nic_ip = nic_dict.get('ip', None)
+      if nic_ip is not None:
+        if nic_ip.lower() == "none":
+          nic_dict['ip'] = None
+        else:
+          if not utils.IsValidIP(nic_ip):
+            raise errors.OpPrereqError("Invalid IP address '%s'" % nic_ip)
+      # we can only check None bridges and assign the default one
+      nic_bridge = nic_dict.get('bridge', None)
+      if nic_bridge is None:
+        nic_dict['bridge'] = self.cfg.GetDefBridge()
+      # but we can validate MACs
+      nic_mac = nic_dict.get('mac', None)
+      if nic_mac is not None:
+        if self.cfg.IsMacInUse(nic_mac):
+          raise errors.OpPrereqError("MAC address %s already in use"
+                                     " in cluster" % nic_mac)
+        if nic_mac not in (constants.VALUE_AUTO, constants.VALUE_GENERATE):
+          if not utils.IsValidMac(nic_mac):
+            raise errors.OpPrereqError("Invalid MAC address %s" % nic_mac)
+    if nic_addremove > 1:
+      raise errors.OpPrereqError("Only one NIC add or remove operation"
+                                 " supported at a time")
+
   def ExpandNames(self):
     self._ExpandAndLockInstance()
     self.needed_locks[locking.LEVEL_NODE] = []
     self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
 
-
   def DeclareLocks(self, level):
     if level == locking.LEVEL_NODE:
       self._LockInstancesNodes()
@@ -4691,20 +4785,7 @@ class LUSetInstanceParams(LogicalUnit):
       args['memory'] = self.be_new[constants.BE_MEMORY]
     if constants.BE_VCPUS in self.be_new:
       args['vcpus'] = self.be_new[constants.BE_VCPUS]
-    if self.do_ip or self.do_bridge or self.mac:
-      if self.do_ip:
-        ip = self.ip
-      else:
-        ip = self.instance.nics[0].ip
-      if self.bridge:
-        bridge = self.bridge
-      else:
-        bridge = self.instance.nics[0].bridge
-      if self.mac:
-        mac = self.mac
-      else:
-        mac = self.instance.nics[0].mac
-      args['nics'] = [(ip, bridge, mac)]
+    # FIXME: readd disk/nic changes
     env = _BuildInstanceHookEnvByObject(self, self.instance, override=args)
     nl = [self.cfg.GetMasterNode(),
           self.instance.primary_node] + list(self.instance.secondary_nodes)
@@ -4716,44 +4797,7 @@ class LUSetInstanceParams(LogicalUnit):
     This only checks the instance list against the existing names.
 
     """
-    # FIXME: all the parameters could be checked before, in ExpandNames, or in
-    # a separate CheckArguments function, if we implement one, so the operation
-    # can be aborted without waiting for any lock, should it have an error...
-    self.ip = getattr(self.op, "ip", None)
-    self.mac = getattr(self.op, "mac", None)
-    self.bridge = getattr(self.op, "bridge", None)
-    self.kernel_path = getattr(self.op, "kernel_path", None)
-    self.initrd_path = getattr(self.op, "initrd_path", None)
-    self.force = getattr(self.op, "force", None)
-    all_parms = [self.ip, self.bridge, self.mac]
-    if (all_parms.count(None) == len(all_parms) and
-        not self.op.hvparams and
-        not self.op.beparams):
-      raise errors.OpPrereqError("No changes submitted")
-    for item in (constants.BE_MEMORY, constants.BE_VCPUS):
-      val = self.op.beparams.get(item, None)
-      if val is not None:
-        try:
-          val = int(val)
-        except ValueError, err:
-          raise errors.OpPrereqError("Invalid %s size: %s" % (item, str(err)))
-        self.op.beparams[item] = val
-    if self.ip is not None:
-      self.do_ip = True
-      if self.ip.lower() == "none":
-        self.ip = None
-      else:
-        if not utils.IsValidIP(self.ip):
-          raise errors.OpPrereqError("Invalid IP address '%s'." % self.ip)
-    else:
-      self.do_ip = False
-    self.do_bridge = (self.bridge is not None)
-    if self.mac is not None:
-      if self.cfg.IsMacInUse(self.mac):
-        raise errors.OpPrereqError('MAC address %s already in use in cluster' %
-                                   self.mac)
-      if not utils.IsValidMac(self.mac):
-        raise errors.OpPrereqError('Invalid MAC address %s' % self.mac)
+    force = self.force = self.op.force
 
     # checking the new params on the primary/secondary nodes
 
@@ -4844,12 +4888,63 @@ class LUSetInstanceParams(LogicalUnit):
             self.warn.append("Not enough memory to failover instance to"
                              " secondary node %s" % node)
 
+    # NIC processing
+    for nic_op, nic_dict in self.op.nics:
+      if nic_op == constants.DDM_REMOVE:
+        if not instance.nics:
+          raise errors.OpPrereqError("Instance has no NICs, cannot remove")
+        continue
+      if nic_op != constants.DDM_ADD:
+        # an existing nic
+        if nic_op < 0 or nic_op >= len(instance.nics):
+          raise errors.OpPrereqError("Invalid NIC index %s, valid values"
+                                     " are 0 to %d" %
+                                     (nic_op, len(instance.nics)))
+      nic_bridge = nic_dict.get('bridge', None)
+      if nic_bridge is not None:
+        if not self.rpc.call_bridges_exist(pnode, [nic_bridge]):
+          msg = ("Bridge '%s' doesn't exist on one of"
+                 " the instance nodes" % nic_bridge)
+          if self.force:
+            self.warn.append(msg)
+          else:
+            raise errors.OpPrereqError(msg)
+
+    # DISK processing
+    if self.op.disks and instance.disk_template == constants.DT_DISKLESS:
+      raise errors.OpPrereqError("Disk operations not supported for"
+                                 " diskless instances")
+    for disk_op, disk_dict in self.op.disks:
+      if disk_op == constants.DDM_REMOVE:
+        if len(instance.disks) == 1:
+          raise errors.OpPrereqError("Cannot remove the last disk of"
+                                     " an instance")
+        ins_l = self.rpc.call_instance_list([pnode], [instance.hypervisor])
+        ins_l = ins_l[pnode]
+        if not type(ins_l) is list:
+          raise errors.OpPrereqError("Can't contact node '%s'" % pnode)
+        if instance.name in ins_l:
+          raise errors.OpPrereqError("Instance is running, can't remove"
+                                     " disks.")
+
+      if (disk_op == constants.DDM_ADD and
+          len(instance.nics) >= constants.MAX_DISKS):
+        raise errors.OpPrereqError("Instance has too many disks (%d), cannot"
+                                   " add more" % constants.MAX_DISKS)
+      if disk_op not in (constants.DDM_ADD, constants.DDM_REMOVE):
+        # an existing disk
+        if disk_op < 0 or disk_op >= len(instance.disks):
+          raise errors.OpPrereqError("Invalid disk index %s, valid values"
+                                     " are 0 to %d" %
+                                     (disk_op, len(instance.disks)))
+
     return
 
   def Exec(self, feedback_fn):
     """Modifies an instance.
 
     All parameters take effect only at the next restart of the instance.
+
     """
     # Process here the warnings from CheckPrereq, as we don't have a
     # feedback_fn there.
@@ -4858,19 +4953,93 @@ class LUSetInstanceParams(LogicalUnit):
 
     result = []
     instance = self.instance
-    if self.do_ip:
-      instance.nics[0].ip = self.ip
-      result.append(("ip", self.ip))
-    if self.bridge:
-      instance.nics[0].bridge = self.bridge
-      result.append(("bridge", self.bridge))
-    if self.mac:
-      instance.nics[0].mac = self.mac
-      result.append(("mac", self.mac))
+    # disk changes
+    for disk_op, disk_dict in self.op.disks:
+      if disk_op == constants.DDM_REMOVE:
+        # remove the last disk
+        device = instance.disks.pop()
+        device_idx = len(instance.disks)
+        for node, disk in device.ComputeNodeTree(instance.primary_node):
+          self.cfg.SetDiskID(disk, node)
+          if not self.rpc.call_blockdev_remove(node, disk):
+            self.proc.LogWarning("Could not remove disk/%d on node %s,"
+                                 " continuing anyway", device_idx, node)
+        result.append(("disk/%d" % device_idx, "remove"))
+      elif disk_op == constants.DDM_ADD:
+        # add a new disk
+        if instance.disk_template == constants.DT_FILE:
+          file_driver, file_path = instance.disks[0].logical_id
+          file_path = os.path.dirname(file_path)
+        else:
+          file_driver = file_path = None
+        disk_idx_base = len(instance.disks)
+        new_disk = _GenerateDiskTemplate(self,
+                                         instance.disk_template,
+                                         instance, instance.primary_node,
+                                         instance.secondary_nodes,
+                                         [disk_dict],
+                                         file_path,
+                                         file_driver,
+                                         disk_idx_base)[0]
+        new_disk.mode = disk_dict['mode']
+        instance.disks.append(new_disk)
+        info = _GetInstanceInfoText(instance)
+
+        logging.info("Creating volume %s for instance %s",
+                     new_disk.iv_name, instance.name)
+        # Note: this needs to be kept in sync with _CreateDisks
+        #HARDCODE
+        for secondary_node in instance.secondary_nodes:
+          if not _CreateBlockDevOnSecondary(self, secondary_node, instance,
+                                            new_disk, False, info):
+            self.LogWarning("Failed to create volume %s (%s) on"
+                            " secondary node %s!",
+                            new_disk.iv_name, new_disk, secondary_node)
+        #HARDCODE
+        if not _CreateBlockDevOnPrimary(self, instance.primary_node,
+                                        instance, new_disk, info):
+          self.LogWarning("Failed to create volume %s on primary!",
+                          new_disk.iv_name)
+        result.append(("disk/%d" % disk_idx_base, "add:size=%s,mode=%s" %
+                       (new_disk.size, new_disk.mode)))
+      else:
+        # change a given disk
+        instance.disks[disk_op].mode = disk_dict['mode']
+        result.append(("disk.mode/%d" % disk_op, disk_dict['mode']))
+    # NIC changes
+    for nic_op, nic_dict in self.op.nics:
+      if nic_op == constants.DDM_REMOVE:
+        # remove the last nic
+        del instance.nics[-1]
+        result.append(("nic.%d" % len(instance.nics), "remove"))
+      elif nic_op == constants.DDM_ADD:
+        # add a new nic
+        if 'mac' not in nic_dict:
+          mac = constants.VALUE_GENERATE
+        else:
+          mac = nic_dict['mac']
+        if mac in (constants.VALUE_AUTO, constants.VALUE_GENERATE):
+          mac = self.cfg.GenerateMAC()
+        new_nic = objects.NIC(mac=mac, ip=nic_dict.get('ip', None),
+                              bridge=nic_dict.get('bridge', None))
+        instance.nics.append(new_nic)
+        result.append(("nic.%d" % (len(instance.nics) - 1),
+                       "add:mac=%s,ip=%s,bridge=%s" %
+                       (new_nic.mac, new_nic.ip, new_nic.bridge)))
+      else:
+        # change a given nic
+        for key in 'mac', 'ip', 'bridge':
+          if key in nic_dict:
+            setattr(instance.nics[nic_op], key, nic_dict[key])
+            result.append(("nic.%s/%d" % (key, nic_op), nic_dict[key]))
+
+    # hvparams changes
     if self.op.hvparams:
       instance.hvparams = self.hv_new
       for key, val in self.op.hvparams.iteritems():
         result.append(("hv/%s" % key, val))
+
+    # beparams changes
     if self.op.beparams:
       instance.beparams = self.be_inst
       for key, val in self.op.beparams.iteritems():
diff --git a/lib/constants.py b/lib/constants.py
index a64b2a3..022e743 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -191,8 +191,8 @@ FD_BLKTAP = "blktap"
 LDS_DRBD = frozenset([LD_DRBD8])
 
 # disk access mode
-DISK_RDONLY = "r"
-DISK_RDWR = "w"
+DISK_RDONLY = "ro"
+DISK_RDWR = "rw"
 DISK_ACCESS_SET = frozenset([DISK_RDONLY, DISK_RDWR])
 
 # disk replacement mode
@@ -217,6 +217,11 @@ FILE_DRIVER = frozenset([FD_LOOP, FD_BLKTAP])
 INISECT_EXP = "export"
 INISECT_INS = "instance"
 
+# dynamic device modification
+
+DDM_ADD = 'add'
+DDM_REMOVE = 'remove'
+
 # common exit codes
 EXIT_SUCCESS = 0
 EXIT_FAILURE = 1
@@ -376,6 +381,10 @@ ELOG_PROGRESS = "progress"
 RAPI_ENABLE = True
 RAPI_PORT = 5080
 
+# max dynamnic devices
+MAX_NICS = 8
+MAX_DISKS = 16
+
 # cluster wide default parameters
 DEFAULT_ENABLED_HYPERVISOR = HT_XEN_PVM
 
diff --git a/lib/opcodes.py b/lib/opcodes.py
index e5edbf1..5807922 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -424,8 +424,9 @@ class OpSetInstanceParams(OpCode):
   OP_ID = "OP_INSTANCE_SET_PARAMS"
   OP_DSC_FIELD = "instance_name"
   __slots__ = [
-    "instance_name", "ip", "bridge", "mac",
+    "instance_name",
     "hvparams", "beparams", "force",
+    "nics", "disks",
     ]
 
 
diff --git a/scripts/gnt-instance b/scripts/gnt-instance
index 95496b0..4d1dffa 100755
--- a/scripts/gnt-instance
+++ b/scripts/gnt-instance
@@ -1035,7 +1035,7 @@ def SetInstanceParams(opts, args):
   @return: the desired exit code
 
   """
-  if not (opts.ip or opts.bridge or opts.mac or
+  if not (opts.nics or opts.disks or
           opts.hypervisor or opts.beparams):
     ToStderr("Please give at least one of the parameters.")
     return 1
@@ -1044,9 +1044,27 @@ def SetInstanceParams(opts, args):
     opts.beparams[constants.BE_MEMORY] = utils.ParseUnit(
       opts.beparams[constants.BE_MEMORY])
 
+  for idx, (nic_op, nic_dict) in enumerate(opts.nics):
+    try:
+      nic_op = int(nic_op)
+      opts.nics[idx] = (nic_op, nic_dict)
+    except ValueError:
+      pass
+
+  for idx, (disk_op, disk_dict) in enumerate(opts.disks):
+    try:
+      disk_op = int(disk_op)
+      opts.disks[idx] = (disk_op, disk_dict)
+    except ValueError:
+      pass
+    if disk_op == constants.DDM_ADD:
+      if 'size' not in disk_dict:
+        raise errors.OpPrereqError("Missing required parameter 'size'")
+      disk_dict['size'] = utils.ParseUnit(disk_dict['size'])
+
   op = opcodes.OpSetInstanceParams(instance_name=args[0],
-                                   ip=opts.ip,
-                                   bridge=opts.bridge, mac=opts.mac,
+                                   nics=opts.nics,
+                                   disks=opts.disks,
                                    hvparams=opts.hypervisor,
                                    beparams=opts.beparams,
                                    force=opts.force)
@@ -1248,21 +1266,20 @@ commands = {
                     "Replaces all disks for the instance"),
   'modify': (SetInstanceParams, ARGS_ONE,
              [DEBUG_OPT, FORCE_OPT,
-              make_option("-i", "--ip", dest="ip",
-                          help="IP address ('none' or numeric IP)",
-                          default=None, type="string", metavar="<ADDRESS>"),
-              make_option("-b", "--bridge", dest="bridge",
-                          help="Bridge to connect this instance to",
-                          default=None, type="string", metavar="<bridge>"),
-              make_option("--mac", dest="mac",
-                          help="MAC address", default=None,
-                          type="string", metavar="<MACADDRESS>"),
               keyval_option("-H", "--hypervisor", type="keyval",
                             default={}, dest="hypervisor",
                             help="Change hypervisor parameters"),
               keyval_option("-B", "--backend", type="keyval",
                             default={}, dest="beparams",
                             help="Change backend parameters"),
+              ikv_option("--disk", help="Disk changes",
+                         default=[], dest="disks",
+                         action="append",
+                         type="identkeyval"),
+              ikv_option("--net", help="NIC changes",
+                         default=[], dest="nics",
+                         action="append",
+                         type="identkeyval"),
               SUBMIT_OPT,
               ],
              "<instance>", "Alters the parameters of an instance"),
