Fix bug in drbd8 replace disks on current nodes

Currently the drbd8 replace-disks on the same node (i.e. -p or -s) has
a bug in that it does modify the instance disk temporarily before
changing it back to the same value. However, we don't need to, and
shouldn't do that: what this operation do is simply change the LVM
configuration on the node, but otherwise the instance disks keep the
same configuration as before.

In the current code, this change back-and-forth is fine *unless* we
fail during attaching the new LVs to DRBD; in which case, we're left
with a half-modified disk, which is entirely wrong.

So we change the code in two ways:

- use temporary copies of the disk children in the old_lvs var
- stop updating disk.children

Which means that the instance should not be modified anymore (except
maybe for SetDiskID, which is a legacy and unfortunate decision that
will have to cleaned up sometime).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index a934471..c5dbcd5 100644 (file)
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -8422,6 +8422,12 @@ class TLReplaceDisks(Tasklet):
                                  (node_name, self.instance.name))
 
   def _CreateNewStorage(self, node_name):
+    """Create new storage on the primary or secondary node.
+
+    This is only used for same-node replaces, not for changing the
+    secondary node, hence we don't want to modify the existing disk.
+
+    """
     iv_names = {}
 
     for idx, dev in enumerate(self.instance.disks):
@@ -8443,7 +8449,7 @@ class TLReplaceDisks(Tasklet):
                              logical_id=(vg_meta, names[1]))
 
       new_lvs = [lv_data, lv_meta]
-      old_lvs = dev.children
+      old_lvs = [child.Copy() for child in dev.children]
       iv_names[dev.iv_name] = (dev, old_lvs, new_lvs)
 
       # we pass force_create=True to force the LVM creation
@@ -8568,10 +8574,14 @@ class TLReplaceDisks(Tasklet):
                                              rename_new_to_old)
       result.Raise("Can't rename new LVs on node %s" % self.target_node)
 
+      # Intermediate steps of in memory modifications
       for old, new in zip(old_lvs, new_lvs):
         new.logical_id = old.logical_id
         self.cfg.SetDiskID(new, self.target_node)
 
+      # We need to modify old_lvs so that removal later removes the
+      # right LVs, not the newly added ones; note that old_lvs is a
+      # copy here
       for disk in old_lvs:
         disk.logical_id = ren_fn(disk, temp_suffix)
         self.cfg.SetDiskID(disk, self.target_node)
@@ -8591,10 +8601,6 @@ class TLReplaceDisks(Tasklet):
                                      "volumes"))
         raise errors.OpExecError("Can't add local storage to drbd: %s" % msg)
 
-      dev.children = new_lvs
-
-      self.cfg.Update(self.instance, feedback_fn)
-
     cstep = 5
     if self.early_release:
       self.lu.LogStep(cstep, steps_total, "Removing old storage")