On Thursday 17 July 2008, Ryan Niebur wrote: > Here is a new patch that doesn't move those functions, and makes > changes to the debconf templates based on Martin's comments. Much better :-) I've added my comments inside the patch marked with ####. I've snipped parts of the patch that I had no comments about, hopefully leaving enough for context. The review is mostly on style, but I did not spot anything that looked obviously wrong to me. For the rest it will just need testing. General question: how well has this been tested and for which RAID levels and variations in spare/missing devices? Again, in general this looks very good. Cheers, FJP
Index: mdcfg/debian/mdcfg-utils.templates
===================================================================
--- mdcfg/debian/mdcfg-utils.templates (revision 54408)
+++ mdcfg/debian/mdcfg-utils.templates (working copy)
@@ -107,6 +107,80 @@
[...]
+Template: mdcfg/raid10sparecount
+Type: string
+# :sl3:
+_Description: Number of spare devices for the RAID10 array:
+Template: mdcfg/raid6sparecount
+Type: string
+# :sl3:
+_Description: Number of spare devices for the RAID6 array:
#### (second one taken from lower down in the patch)
#### I wonder if we should consolidate such strings that only have "RAIDx"
#### different between them. However, that could/should be a separate change.
#### Christian: what do you think? Would RAIDx need to be translatable if
#### we did that?
[...]
+Template: mdcfg/raid10layout
+Type: string
+# :sl3:
+_Description: Layout of the RAID10 multidisk device:
+ The layout must be n, o, or f followed by a number.
+ .
+ The number is the number of copies of each chunk.
+ It has to be equal to or smaller than the number of active devices.
+ .
+ The letter is the arrangement of the copies.
+ n - near copies: Multiple copies of one data block are at similar offsets in different devices.
+ f - far copies: Multiple copies have very different offsets
+ o - offset copies: Rather than the chunks being duplicated within a stripe, whole stripes are duplicated but are rotated by one device so duplicate blocks are on different devices.
#### Don't the lines above need explicit \n at the end?
+ .
+ The default setting is n2.
+ .
+ NOTE: this setting cannot be changed later.
@@ -135,6 +209,26 @@
Please choose which partitions are active devices.
You must select exactly ${COUNT} partitions.
+Template: mdcfg/raid6devs
+Type: multiselect
+Choices: ${PARTITIONS}
+# :sl3:
+_Description: Active devices for the RAID6 multidisk device:
+ You have chosen to create an RAID6 array with ${COUNT} active devices.
+ .
+ Please choose which partitions are active devices.
+ You must select exactly ${COUNT} partitions.
+
+Template: mdcfg/raid10devs
+Type: multiselect
+Choices: ${PARTITIONS}
+# :sl3:
+_Description: Active devices for the RAID10 multidisk device:
+ You have chosen to create an RAID10 array with ${COUNT} active devices.
+ .
+ Please choose which partitions are active devices.
+ You must select exactly ${COUNT} partitions.
#### These would be candidates for consolidation as well.
Template: mdcfg/deletemenu
Type: select
# :sl3:
Index: mdcfg/mdcfg.sh
===================================================================
--- mdcfg/mdcfg.sh (revision 54408)
+++ mdcfg/mdcfg.sh (working copy)
@@ -102,14 +102,7 @@
return
fi
- case "$RAID_SEL" in
- RAID5)
- md_create_raid5 ;;
- RAID1)
- md_create_raid1 ;;
- RAID0)
- md_create_raid0 ;;
- esac
+ md_create_array "$RAID_SEL"
fi
}
#### I think I'd prefer to keep this as follows:
case "$RAID_SEL" in
RAID0)
md_create_raid0 ;;
RAID1|RAID5|RAID6|RAID10)
md_create_array "$RAID_SEL" ;;
*)
return 1 ;;
esac
@@ -199,215 +192,104 @@
-n $SELECTED $RAID_DEVICES
}
-md_create_raid1() {
+md_create_array(){
OK=0
- db_set mdcfg/raid1devcount 2
+ case "$1" in
+ RAID10)
+ MIN_SIZE=2 ;;
+ RAID6)
+ MIN_SIZE=4 ;;
+ RAID5)
+ MIN_SIZE=3 ;;
+ RAID1)
+ MIN_SIZE=2 ;;
+ RAID0)
+ md_create_raid0; return ;;
+ *)
+ return ;;
+ esac
#### Previous suggestion would mean the RAID0 case could be dropped here.
#### Suggest to sort ascending after that.
#### In case of no match: 'return 1'.
#### Please use 4 space indent for the allowed values as in the
#### existing case statement above (see coding style doc in
#### installer/doc/devel/ in SVN).
- # Get the count of active devices
- while [ $OK -eq 0 ]; do
- db_input critical mdcfg/raid1devcount
- db_go
- if [ $? -eq 30 ]; then
- return
- fi
+ LEVEL=$(echo "$1" | sed s/RAID//)
#### This could be simplified to 'LEVEL=${1#RAID}'
[...]
- db_set mdcfg/raid5sparecount "0"
+
+ db_set mdcfg/raid${LEVEL}sparecount "0"
OK=0
# Same procedure as above, but get the number of spare partitions
# this time.
# TODO: Make a general function for this kind of stuff
while [ $OK -eq 0 ]; do
- db_input critical mdcfg/raid5sparecount
+ db_input critical mdcfg/raid${LEVEL}sparecount
db_go
if [ $? -eq 30 ]; then
return
fi
- db_get mdcfg/raid5sparecount
+ db_get mdcfg/raid${LEVEL}sparecount
RET=$(echo $RET | sed -e "s/[[:space:]]//g")
if [ "$RET" ]; then
let "OK=${RET}>=0 && ${RET}<99"
fi
done
- db_get mdcfg/raid5devcount
+ db_get mdcfg/raid${LEVEL}devcount
DEV_COUNT="$RET"
- if [ $DEV_COUNT -lt 3 ]; then
- DEV_COUNT=3 # Minimum number for RAID5
+ if [ "$LEVEL" -ne "1" ]; then
#### No quotes needed for numerical comparisons (more consistent with
#### other uses in this script too). Occurs multiple times.
[...]
- # Loop until the correct number of active devices has been selected
- while [ $SELECTED -ne $DEV_COUNT ]; do
- db_subst mdcfg/raid5devs COUNT "$DEV_COUNT"
- db_subst mdcfg/raid5devs PARTITIONS "$PARTITIONS"
- db_input critical mdcfg/raid5devs
+ # Loop until the correct number of active devices has been selected for RAID 5 and 10
+ # Loop until at least one device has been selected for RAID 1
#### What about RAID 6?
+ until ([ "$LEVEL" -ne "1" ] && [ $SELECTED -eq $DEV_COUNT ]) || ([ "$LEVEL" -eq "1" ] && [ $SELECTED -gt 0 ] && [ $SELECTED -le $DEV_COUNT ]); do
#### Please use line continuation for such long tests:
until ([ "$LEVEL" -ne "1" ] && [ $SELECTED -eq $DEV_COUNT ]) || \
([ "$LEVEL" -eq "1" ] && [ $SELECTED -gt 0 ] && [ $SELECTED -le $DEV_COUNT ]); do
@@ -428,15 +319,15 @@
# That means any number less than or equal to the spare count.
while [ $SELECTED -gt $SPARE_COUNT ] || [ $FIRST -eq 1 ]; do
FIRST=0
- db_subst mdcfg/raid5sparedevs COUNT "$SPARE_COUNT"
- db_subst mdcfg/raid5sparedevs PARTITIONS "$PARTITIONS"
- db_input critical mdcfg/raid5sparedevs
+ db_subst mdcfg/raid${LEVEL}sparedevs COUNT "$SPARE_COUNT"
+ db_subst mdcfg/raid${LEVEL}sparedevs PARTITIONS "$PARTITIONS"
+ db_input critical mdcfg/raid${LEVEL}sparedevs
db_go
if [ $? -eq 30 ]; then
return
fi
- db_get mdcfg/raid5sparedevs
+ db_get mdcfg/raid${LEVEL}sparedevs
SELECTED=0
for i in $RET; do
DEVICE=$(echo $i | sed -e "s/,//")
@@ -445,13 +336,29 @@
done
fi
+ if [ "$LEVEL" -eq "10" ]; then
+ db_set mdcfg/raid10layout "n2"
+ db_input low mdcfg/raid10layout
+ db_go
+ if [ $? -eq 30 ]; then return; fi
+ db_get mdcfg/raid10layout
+ LAYOUT="--layout=$RET"
+ until echo $LAYOUT | grep -Eq "^--layout=[nfo][0-9]{1,2}$" && [ "$(echo $LAYOUT | sed s/--layout=.//)" -le "$DEV_COUNT" ]; do
#### Line continuation again:
+ until echo $LAYOUT | grep -Eq "^--layout=[nfo][0-9]{1,2}$" && \
[ "$(echo $LAYOUT | sed s/--layout=.//)" -le "$DEV_COUNT" ]; do
#### First adding --layout= and then removing it again for test against
#### DEV_COUNT seems a bit silly. It should be possible to rewrite that.
+ db_input critical mdcfg/raid10layout
+ db_go
+ if [ $? -eq 30 ]; then return; fi
+ db_get mdcfg/raid10layout
+ LAYOUT="--layout=$RET"
+ done
+ fi
#### I also wonder why this would need to be asked at priority low first
#### and critical later as it cannot be preseeded with a wrong value
#### because the default is always set first (and preseeding is not an
#### option for most of partman anyway).
#### Should IMO be simplified.
Index: partman/partman-auto-raid/auto-raidcfg
===================================================================
--- partman/partman-auto-raid/auto-raidcfg (revision 54408)
+++ partman/partman-auto-raid/auto-raidcfg (working copy)
@@ -14,6 +14,12 @@
exit 9
fi
+ if [ "$DEV_COUNT" -lt 4 ] && ([ $RAID_TYPE = "10" ] || [ $RAID_TYPE = "6" ]); then
#### Line continuation again:
if [ "$DEV_COUNT" -lt 4 ] && \
([ $RAID_TYPE = "10" ] || [ $RAID_TYPE = "6" ]); then
+ db_input critical partman-auto-raid/notenoughparts
+ db_go partman-auto-raid/notenoughparts
+ exit 9
+ fi
Attachment:
signature.asc
Description: This is a digitally signed message part.