Package: fai-server
Version: 4.6.0
Followup-For: Bug #693701

I have spent quite amount of time to fix this one, and Im pretty unhappy
with the chaos in the logic of related to LVM handling. I try to contribute
a few lines here to help it.

I have written a detailed bugreport before which was not sent due to a
mailserver problem, so I'm writing it again; this gonna be much shorter,
free of ranting and explanations.

The patch tries to straighten up the problems which can be solved by simple
fixes but simply IGNORES those parts which would have required a complete
rewrite of the handling logic: the order FAI tries now is:

vgchange_a_n -> wipe -> lvremove

which will not work since you cannot operate on deactivated LV devices. The
proper order would be:

wipe -> vgchange_a_n -> lvremove

but this would have required me to analyse the whole shebang to be able to
properly fix the logic; I kindly let you to do that. ;-) Until then wiping
is commented out since lvremove seems to do the job anyway.
--- Commands.pm-1	2013-07-18 16:59:34.000000000 +0200
+++ Commands.pm	2013-07-19 03:56:35.000000000 +0200
@@ -650,8 +650,8 @@
       # remove all volumes that do not exist anymore or need not be preserved
       foreach my $lv (keys %{ $FAI::current_lvm_config{$vg}{volumes} }) {
         my $pre_deps_cl = "";
-        $pre_deps_cl = ",self_cleared_" .
-          join(",self_cleared_", @{ $FAI::current_dev_children{"/dev/$vg/$lv"} })
+        $pre_deps_cl = ",self_cleared_/dev/$vg/" .
+          join(",self_cleared_/dev/$vg/", @{ $FAI::current_dev_children{"/dev/$vg/$lv"} })
             if (defined($FAI::current_dev_children{"/dev/$vg/$lv"}) &&
               scalar(@{ $FAI::current_dev_children{"/dev/$vg/$lv"} }));
         # skip preserved/resized volumes
@@ -664,9 +664,11 @@
           }
         }
 
-        &FAI::push_command( "wipefs -a $vg/$lv",
-          "vgchange_a_n_VG_$vg$pre_deps_cl",
-          "wipefs_$vg/$lv");
+## You CANNOT wipe LVs in inactive VGs! (vhcgange_a_n_VG_$vg!)
+## FIXME: Order should be  wipe -> vgchange_a_n -> lvremove.
+#        &FAI::push_command( "wipefs -a $vg/$lv",
+#          "vgchange_a_n_VG_$vg$pre_deps_cl",
+#          "wipefs_$vg/$lv");
         &FAI::push_command( "lvremove -f $vg/$lv",
           "wipefs_$vg/$lv",
           "lv_rm_$vg/$lv,self_cleared_/dev/$vg/$lv");
@@ -685,13 +687,15 @@
   my $vg_destroy_pre = "vgchange_a_n_VG_$vg";
   foreach my $lv (keys %{ $FAI::current_lvm_config{$vg}{volumes} }) {
     my $pre_deps_cl = "";
-    $pre_deps_cl = ",self_cleared_" .
-      join(",self_cleared_", @{ $FAI::current_dev_children{"/dev/$vg/$lv"} })
+    $pre_deps_cl = ",self_cleared_/dev/$vg/" .
+      join(",self_cleared_/dev/$vg/", @{ $FAI::current_dev_children{"/dev/$vg/$lv"} })
         if (defined($FAI::current_dev_children{"/dev/$vg/$lv"}) &&
           scalar(@{ $FAI::current_dev_children{"/dev/$vg/$lv"} }));
-    &FAI::push_command( "wipefs -a $vg/$lv",
-      "vgchange_a_n_VG_$vg$pre_deps_cl",
-      "wipefs_$vg/$lv");
+## You CANNOT wipe LVs in inactive VGs! (vhcgange_a_n_VG_$vg!)
+## FIXME: Order should be  wipe -> vgchange_a_n -> lvremove.
+#    &FAI::push_command( "wipefs -a $vg/$lv",
+#      "vgchange_a_n_VG_$vg$pre_deps_cl",
+#      "wipefs_$vg/$lv");
     &FAI::push_command( "lvremove -f $vg/$lv",
       "wipefs_$vg/$lv",
       "lv_rm_$vg/$lv,self_cleared_/dev/$vg/$lv");
@@ -731,8 +735,8 @@
     my $vg_pre = "vgchange_a_n_VG_$vg";
     my $pre_deps_vgc = "";
     foreach my $c (@{ $FAI::current_dev_children{$d} }) {
-      $pre_deps_vgc = ",self_cleared_" .
-        join(",self_cleared_", @{ $FAI::current_dev_children{$c} })
+      $pre_deps_vgc = ",self_cleared_/dev/$vg/" .
+        join(",self_cleared_/dev/$vg/", @{ $FAI::current_dev_children{$c} })
         if (defined($FAI::current_dev_children{$c}) &&
           scalar(@{ $FAI::current_dev_children{$c} }));
     }
@@ -740,8 +744,8 @@
     &FAI::push_command("vgchange -a n $1", "$pre_deps_vgc", $vg_pre);
     $vg_pre .= ",pv_sigs_removed_$vg" if (&FAI::cleanup_vg($vg));
     my $pre_deps_cl = "";
-    $pre_deps_cl = ",self_cleared_" .
-      join(",self_cleared_", @{ $FAI::current_dev_children{$d} })
+    $pre_deps_cl = ",self_cleared_/dev/$vg/" .
+      join(",self_cleared_/dev/$vg/", @{ $FAI::current_dev_children{$d} })
       if (scalar(@{ $FAI::current_dev_children{$d} }));
     &FAI::push_command("true", "$vg_pre$pre_deps_cl", "self_cleared_VG_$vg");
   }

Reply via email to