On 22 May 2013 10:28, Thomas Thrainer <[email protected]> wrote: > > > > On Mon, May 20, 2013 at 5:11 PM, Bernardo Dal Seno <[email protected]> > wrote: >> >> The option is parsed but ignored, for the moment. >> >> Signed-off-by: Bernardo Dal Seno <[email protected]> >> --- >> lib/cli.py | 3 +++ >> lib/cmdlib/instance_storage.py | 1 + >> lib/constants.py | 2 ++ >> man/gnt-instance.rst | 13 +++++++++---- >> 4 files changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/lib/cli.py b/lib/cli.py >> index 32ef069..0bddf43 100644 >> --- a/lib/cli.py >> +++ b/lib/cli.py >> @@ -2647,6 +2647,9 @@ def GenericInstanceCreate(mode, opts, args): >> raise errors.OpPrereqError("Invalid disk size for disk %d: %s" >> % >> (didx, err), errors.ECODE_INVAL) >> elif constants.IDISK_ADOPT in ddict: >> + if constants.IDISK_SPINDLES in ddict: >> + raise errors.OpPrereqError("spindles is not a valid option >> when" >> + " adopting a disk", >> errors.ECODE_INVAL) > > > That a bit strange to read, as the if goes like > if IDISK_SIZE > if ADOPT > error > ... do stuff ... > elif ADOPT > if SPINDLES > error > ... do stuff ... > > wouldn't it be more readable to do > > if ADOPT > if IDISK_SIZE > error > if SPINDLES > error > ... do stuff ... > elif IDISK_SIZE > ... do stuff ... > > It's just that the same condition (IDISK_ADOPT) is checked twice, in two > different levels, which makes it a bit hard to follow.
Actually the two look very similar to me: There are four IFs, two nesting levels, and one condition checked (twice) at different levels. So I think it's more a matter of a personal taste. Btw, this is not really related to my patch, so we need a separate patch if we want to rewrite this. >> If only a subset should be recreated, any number of ``disk`` options can >> be specified. It expects a disk index and an optional list of disk >> -parameters to change. Only ``size`` and ``mode`` can be changed while >> +parameters to change. Only ``size``, ``spindles``, and ``mode`` can be >> +changed while > > > Why not fill the line? I did it to simplify the work of the reviewer, but you're right, the man page will last much longer than the review. Interdiff: diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst index 5e7cb25..0942c0b 100644 --- a/man/gnt-instance.rst +++ b/man/gnt-instance.rst @@ -1107,15 +1107,14 @@ The ``--disk add:size=*SIZE*,[options..]`` option adds a instance, and ``--disk *N*:add:size=*SIZE*,[options..]`` will add a disk to the the instance at a specific index. The available options are the same as in the **add** command(``spindles``, ``mode``, ``name``, ``vg``, -``metavg``). -When adding an ExtStorage disk the ``provider=*PROVIDER*`` option is -also mandatory and specifies the ExtStorage provider. Also, for -ExtStorage disks arbitrary parameters can be passed as additional comma -separated options, same as in the **add** command. -The ``--disk remove`` -option will remove the last disk of the instance. Use -``--disk `` *ID*``:remove`` to remove a disk by its identifier. *ID* -can be the index of the disk, the disks's name or the disks's UUID. The -``--disk *ID*:modify[,options...]`` will change the options of the disk. +``metavg``). When adding an ExtStorage disk the ``provider=*PROVIDER*`` +option is also mandatory and specifies the ExtStorage provider. Also, +for ExtStorage disks arbitrary parameters can be passed as additional +comma separated options, same as in the **add** command. -The ``--disk +remove`` option will remove the last disk of the instance. Use ``--disk +`` *ID*``:remove`` to remove a disk by its identifier. *ID* can be the +index of the disk, the disks's name or the disks's UUID. The ``--disk +*ID*:modify[,options...]`` will change the options of the disk. Available options are: mode @@ -1632,10 +1631,9 @@ normal operation and as such the impact of this is low. If only a subset should be recreated, any number of ``disk`` options can be specified. It expects a disk index and an optional list of disk parameters to change. Only ``size``, ``spindles``, and ``mode`` can be -changed while -recreating disks. To recreate all disks while changing parameters on -a subset only, a ``--disk`` option must be given for every disk of the -instance. +changed while recreating disks. To recreate all disks while changing +parameters on a subset only, a ``--disk`` option must be given for every +disk of the instance. Optionally the instance's disks can be recreated on different nodes. This can be useful if, for example, the original nodes of the Thanks, Bernardo
