Hi Lisa, Thanks a lot for the changes! Please see a couple of nitpicks inline.
* Lisa Velden <[email protected]> [2016-01-19 14:42:58 +0100]: > I have some changes here: > > diff --git a/doc/design-standalone-disks.rst > b/doc/design-standalone-disks.rst > index 71cca81..7462a41 100644 > --- a/doc/design-standalone-disks.rst > +++ b/doc/design-standalone-disks.rst > @@ -75,15 +75,38 @@ subcommands: > > * ``gnt-disk adopt *VOLUME*``: adopts a volume into the Ganeti > configuration. This should work not only for plain and block devices, > - but also for shared storage > + but also for shared storage. > > * ``gnt-disk create *DISK_TEMPLATE* *SIZE* options``: creates a disk, so > that it appears in the Ganeti configuration. This command will take > the same options as ``gnt-instance add --disk`` except for the > - position value and the adopt option > + position value and the adopt option. > > -* ``gnt-disk remove *DISK_UUID|DISK_NAME*``: removes the given disk from > - the Ganeti configuration > +* ``gnt-disk modify *DISK_UUID|DISK_NAME* options``: changes the options > + of the disk. Available options are > + > + + ``mode``: The access mode, i.e. either ``ro`` (read-only) or the > + default ``rw`` (read-write). > + + ``name``: This option specifies a name for the disk that can be > + used as an identifier. > + > +* ``gnt-disk abandon *DISK_UUID|DISK_NAME*``: removes the given disk from > + the Ganeti configuration without destroying the disk. With an > + additional option ``--remove-data`` the disk will also be destroyed. > + > +* ``gnt-disk snapshot *SRC_DISK_UUID* *DST_DISK_NAME*``: creates a > + snapshot of the given source disk. This will be a new disk with the > + specified name and read-only access mode > + > +* ``gnt-disk clone *SRC_DISK_UUID* *DST_DISK_NAME*``: creates a new disk > + with the specified disk name and read-write access mode > + > +* ``gnt-disk convert *DISK_TEMPLATE* *DISK_UUID|DISK_NAME*``: converts > + the disk template of a disk into the new disk template > + > +* ``gnt-disk copy *SRC_DISK_UUID* *DISK_TEMPLATE* *DST_DISK_NAME*``: > + creates a new disk that is identical to the source disk, but has a > + different disk template > I would suggest the following minor changes: * To support both DISK_UUID and DISK_NAME as source identifiers. * The DISK_TEMPLATE to be an option (e.g. --target-disk-template, or --dst-disk-template) and not a possitional argument. * The DST_DISK_NAME to be an option too (e.g. --target-disk-name, or --dst-disk-name) since the name in Disk objects is optional. Ganeti will create a new Disk object (with the proper UUID and logical id) and optionally name it. > Besides the ``gnt-disk`` command, we propose the following changes: > > @@ -104,12 +127,12 @@ would have the following simplified commands, instead > of the respective > ``gnt-instance modify --disk`` commands: > > * ``gnt-disk attach *DISK_UUID|DISK_NAME* *INSTANCE*``: attaches a disk > - to an instance. If the disk was not in the configuration before, an > - error is raised > + to an instance. If the disk was not in the configuration before, an > + error is raised. > Shouldn't we support DISK_POSITION here as well? Maybe with an optional argument --device-position <idx>. If not provided then the disk should be attached at the end of the existing disk list. > * ``gnt-disk detach *DISK_UUID|DISK_NAME|DISK_POSITION* *INSTANCE*``: > - detaches a disk from an instance and leaves it in the Ganeti > - configuration > + detaches a disk from an instance and leaves it in the Ganeti > + configuration > > Note that this does not add new functionality, because it will behave > just as the current solution for disk attachment and detachment. For > @@ -128,3 +151,7 @@ changes also to standalone disks. > Except for creating diskless instances on purpose, the diskless disk > template should not be required if an instance becomes diskless, but the > instance should be considered to be in the state ``diskless``. > + > +The logic for adoption should be moved down to ``lib/storage/bdev.py``, > +so that it becomes uniform to all disk templates and it is, thus, easy to > +extend it for external storage. > > Perfect! Thanks again! dimara > On Fri, Jan 15, 2016 at 2:56 PM, Dimitris Aragiorgis < > [email protected]> wrote: > > > * Lisa Velden <[email protected]> [2016-01-14 14:02:29 +0100]: > > > > > Hi Dimara, > > > > > > thanks a lot for your valuable comments! I have placed some > > > questions/comments inline. > > > > > > On Wed, Jan 13, 2016 at 2:32 PM, Dimitris Aragiorgis < > > > [email protected]> wrote: > > > > > > > Hi Lisa! > > > > > > > > Thanks a lot for this design doc! This was really a pleasant surprise! > > > > > > > > Please see some comments inline. > > > > > > > > * 'Lisa Velden' via ganeti-devel <[email protected]> > > > > [2016-01-13 12:08:26 +0100]: > > > > > > > > > This design document introduces the gnt-disk command, so that disks > > can > > > > > be treated as top-level entities. > > > > > > > > > > Signed-off-by: Lisa Velden <[email protected]> > > > > > --- > > > > > Makefile.am | 3 +- > > > > > doc/design-draft.rst | 1 + > > > > > doc/design-standalone-disks.rst | 130 > > > > ++++++++++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 133 insertions(+), 1 deletion(-) > > > > > create mode 100644 doc/design-standalone-disks.rst > > > > > > > > > > diff --git a/Makefile.am b/Makefile.am > > > > > index 0080f00..f93aac6 100644 > > > > > --- a/Makefile.am > > > > > +++ b/Makefile.am > > > > > @@ -546,7 +546,7 @@ hypervisor_hv_kvm_PYTHON = \ > > > > > > > > > > jqueue_PYTHON = \ > > > > > lib/jqueue/__init__.py \ > > > > > - lib/jqueue/exec.py \ > > > > > + lib/jqueue/exec.py \ > > > > > lib/jqueue/post_hooks_exec.py > > > > > > > > > > storage_PYTHON = \ > > > > > @@ -733,6 +733,7 @@ docinput = \ > > > > > doc/design-shared-storage.rst \ > > > > > doc/design-shared-storage-redundancy.rst \ > > > > > doc/design-ssh-ports.rst \ > > > > > + doc/design-standalone-disks.rst \ > > > > > doc/design-storagetypes.rst \ > > > > > doc/design-sync-rate-throttling.rst \ > > > > > doc/design-systemd.rst \ > > > > > diff --git a/doc/design-draft.rst b/doc/design-draft.rst > > > > > index e4c9f85..18d9ca9 100644 > > > > > --- a/doc/design-draft.rst > > > > > +++ b/doc/design-draft.rst > > > > > @@ -28,6 +28,7 @@ Design document drafts > > > > > design-global-hooks.rst > > > > > design-scsi-kvm.rst > > > > > design-disks.rst > > > > > + design-standalone-disks.rst > > > > > > > > > > .. vim: set textwidth=72 : > > > > > .. Local Variables: > > > > > diff --git a/doc/design-standalone-disks.rst > > > > b/doc/design-standalone-disks.rst > > > > > new file mode 100644 > > > > > index 0000000..71cca81 > > > > > --- /dev/null > > > > > +++ b/doc/design-standalone-disks.rst > > > > > @@ -0,0 +1,130 @@ > > > > > +========================= > > > > > +Disks as Separate Objects > > > > > +========================= > > > > > + > > > > > +.. contents:: :depth: 3 > > > > > + > > > > > +This design document improves the former :doc:`design-disks` > > document. > > > > > +It introduces the gnt-disk command, so that disks can really be > > treated > > > > > +as top-level entities. However, the focus is on disks as separate > > > > objects, > > > > > +that is, we do not introduce other new features, such as instances > > with > > > > > +mixed disk templates. > > > > > + > > > > > + > > > > > +Current state and shortcomings > > > > > +============================== > > > > > + > > > > > +What can already be done? > > > > > +------------------------- > > > > > + > > > > > +Currently, disks can only be created within an instance. But it is > > > > > +already possible to have disks reside in the configuration without > > being > > > > > +tied to an instance if that disk was previously detached from an > > > > > +instance with a modification command like > > > > > +``gnt-instance modify --disk *UUID*:detach *INSTANCE*``. > > > > > + > > > > > +However, detaching a disk from an instance results in a warning in > > > > > +``gnt-cluster verify`` about orphan volumes for most of the disk > > > > > +templates. > > > > > + > > > > > +Besides detaching, it is also possible to attach a pre-existing > > disk to > > > > > +an instance and to adopt disks into the Ganeti configuration which > > have > > > > > +been created outside of Ganeti, e.g. on plain KVM with LVM. > > Currently, > > > > > +disk adoption is done with ``gnt-instance add > > --disk=*N*:adopt=*LV*``, > > > > > +which means it is adopt and attach in one step. > > > > > + > > > > > +It is possible to list all disks on a node with their associated > > > > > +instances, if any. The command for that is ``gnt-node volumes``. > > > > However, > > > > > +only LVM volumes are listed with that command. > > > > > + > > > > > +Furthermore, instances can be destroyed while its disks are left as > > is. > > > > > +But therefore disks have to be detached first and > > > > > +``gnt-instance remove`` has to be called afterwards. > > > > > + > > > > > +What cannot be done at the current state? > > > > > +----------------------------------------- > > > > > + > > > > > +Standalone disks cannot be properly accessed. There is no command > > that > > > > > +lists all disks for all disk templates, nor is it easy to find the > > > > > +respective instance, if any, for a given disk. > > > > > + > > > > > +It is also not convenient that detached disks can only be removed > > when > > > > > +they are attached to an instance again and that disk adoption only > > works > > > > > +for lvm volumes and block devices at the current state. > > > > > + > > > > > +Currently, disk adoption only works with plain and blockdev disk > > > > > +templates. It should also work with the ext disk template, so that > > in a > > > > > +shared storage environment we can do cross-cluster failovers, where > > we > > > > > +instantaneously move an instance from one cluster to another. > > > > > + > > > > > +Proposed changes > > > > > +================ > > > > > + > > > > > +As the current way to do disk adoption as well as removing detached > > > > > +disks from the Ganeti configuration is not intuitive, we propose to > > > > > +introduce a new command ``gnt-disk`` that supports the following > > > > > +subcommands: > > > > > + > > > > > +* ``gnt-disk info *DISK_UUID|DISK_NAME*``: all relevant information > > for > > > > > + a disk is listed, e.g. size, connected instance, position among > > other > > > > > + disks > > > > > + > > > > > +* ``gnt-disk list``: disks that should be listed can be specified > > with > > > > > + ``--attached``, ``--detached`` or ``--all`` where all disks are > > also > > > > > + shown as default if no option was given > > > > > + > > > > > +* ``gnt-disk adopt *VOLUME*``: adopts a volume into the Ganeti > > > > > + configuration. This should work not only for plain and block > > devices, > > > > > + but also for shared storage > > > > > + > > > > > > > > This is something that we discussed in depth in GanetiCon'15. Please > > > > note that the *VOLUME* should be a template specific value. In other > > > > words it should be an arbitrary string that the upper layers of Ganeti > > > > (e.g. cmdlib, backend) should not be able to understand. Only bdev > > > > should be able to understand/translate/use it. > > > > > > > > The existing adoption implementation for plain/blockdev should me moved > > > > down to bdev. Specifically the bdev.adopt() method is actually a > > > > bdev.create() that will check the given arbitrary string, and do > > > > whatever is needed (e.g. for plain will rename this LV) so that at the > > > > end of the day a disk with the given logical id gets created. > > > > > > > > The above asumes that the logical id is generated by Ganeti in advance > > > > in the exact way it happens now. With the above aproach, adoption > > > > becomes uniform for all disk templates, and if a template supports > > > > adoption it should just implement the bdev.adopt() method. Then > > > > supporting adoption in ExtStorage would be completely straightforward. > > > > > > > > Do the above make sense? > > > > yes, it does :) > > > > > > > > > +* ``gnt-disk create *DISK_TEMPLATE* *SIZE* options``: creates a > > disk, so > > > > > + that it appears in the Ganeti configuration. This command will > > take > > > > > + the same options as ``gnt-instance add --disk`` except for the > > > > > + position value and the adopt option > > > > > + > > > > > +* ``gnt-disk remove *DISK_UUID|DISK_NAME*``: removes the given disk > > from > > > > > + the Ganeti configuration > > > > > + > > > > > > > > Please make sure that there will be an option so that we can remove a > > > > disk from the Ganeti configuration without actually removing the disk's > > > > data. > > > > > > > > > Oh, I forgot to update this. It was actually already discussed to rename > > > this to ``gnt-disk abandon`` and to only remove the disk from the Ganeti > > > configuration, but not removing the disk's data as a default. Hence, > > there > > > would be an option to also remove the disk's data. > > > > > > > OK. This is exactlty the same with: > > > > gnt-disk remove <disk> # This will remove the data as well > > gnt-disk remove --keep-data <disk> # This will keep the data (no RPC) > > > > So you suggest something like: > > > > gnt-disk abandon <disk> > > gnt-disk abandon --remove-data <disk> > > > > Both sound good. > > > > > This would make the disk adoptable in the future. So a disk > > > > can be in one of the four following states: > > > > > > > > 1) Attached to an instance (done) > > > > 2) Detached from an instance but present in config.data and known > > > > to Ganeti (done) > > > > 3) Present on the host, unknown to Ganeti, but potentially adoptable > > > > (not yet implemented) > > > > 4) Non existent/visible neither to Ganeti nor to the host (done) > > > > > > > > > +Besides the ``gnt-disk`` command, we propose the following changes: > > > > > + > > > > > +* Make the logical id from disks that is present in > > > > > + ``gnt-instance info *INSTANCE*`` also available through the basic > > > > > + instance information from the RAPI > > > > > + > > > > > +* Disallow all disk operations via the RAPI, except for moves > > > > > + > > > > > +* Add a flag to preserve disks to the ``gnt-instance remove`` > > command > > > > > + > > > > > +Further possible changes > > > > > +======================== > > > > > + > > > > > +If there is need for it, it would be also possible to move disk > > > > > +detachment and attachment to the new ``gnt-disk`` command, so that > > we > > > > > +would have the following simplified commands, instead of the > > respective > > > > > +``gnt-instance modify --disk`` commands: > > > > > + > > > > > +* ``gnt-disk attach *DISK_UUID|DISK_NAME* *INSTANCE*``: attaches a > > disk > > > > > + to an instance. If the disk was not in the configuration > > before, an > > > > > + error is raised > > > > > + > > > > > +* ``gnt-disk detach *DISK_UUID|DISK_NAME|DISK_POSITION* > > *INSTANCE*``: > > > > > + detaches a disk from an instance and leaves it in the Ganeti > > > > > + configuration > > > > > + > > > > > +Note that this does not add new functionality, because it will > > behave > > > > > +just as the current solution for disk attachment and detachment. For > > > > > +example, disk attachment might still fail due to misaligned DRBD > > disks. > > > > > + > > > > > > > > Since cmdlib/bdev is going to be refactored I would suggest that we > > push > > > > all template specific actions down to bdev and remove them from other > > > > parts of the code. I know that the existence of drbd makes this task > > > > really hard, because it involves two nodes but it should be possible > > for > > > > all other templates without huge an effort. > > > > > > > > Also since we are going to have gnt-disk, it would be great to have > > > > a uniform snapshot/clone functionality as well in bdev and the > > > > corresponding commands. > > > > > > > > > > Could you remind me what the difference would be between snapshot and > > clone? > > > > > > > In a nutshell: > > > > The snapshot() method will create a new read-only disk that may also be > > immutable depending on the needs and the underlying storage. > > > > The clone() method will create a new read-write/mutable disk. > > > > Both need a source disk object to operate on but semantically they are > > different so it's best to have them separate in bdev, especially > > since we want to extend this to ExtStorage. > > > > > > > > > > > > > Finally, gnt-disk copy/convert could be implemented on top of existing > > > > disk-template convertion logic. > > > > > > > > > > And also what the difference between copy and snapshot is supposed to be? > > > > > > > copy/convert are inter-template operations at the cmdlib/backend > > level and have nothing to do with bdev. They use the import/export bdev > > methods. So they do not really relate with snapshot/clone. The > > difference between copy and covert is the following: > > > > Currently convert replaces the source disk with a new disk of a > > different template. > > > > copy which does not exist now, should create a new disk identical > > with the source disk but of a different template keeping the > > source disk intact. This will result in having two different disks > > with the same data. > > > > Please note that currently the import/export bdev methods exist and > > probably should stay as is and extended to the ExtStorage interface. > > > > Hope everything is clearer now. > > > > yes, thanks again for your additional explanations :) > > > > > > Cheers, > > dimara > > > > > > > > > > > > > > +Implementation details > > > > > +====================== > > > > > + > > > > > +Especially for the ``gnt-disk create`` part it will be handy to > > > > re-factor > > > > > +the current ``LUInstanceCreate`` in such a way that we can reuse the > > > > disk > > > > > +creation part. That can be achieved with tasklets like they are used > > > > > +in ``LUInstanceMigrate`` and ``LUInstanceReplaceDisks``. The same > > holds > > > > > +for ``LUInstanceSetParams``, because we want to be able to make disk > > > > > +changes also to standalone disks. > > > > > + > > > > > > > > One question here: Is gnt-instance create --disk going to create two > > jobs > > > > (i.e. LUDiskCreate + LUInstanceCreate)? Will it involve Tasklets/job > > > > chains? > > > > > > > > > > It would still be ``gnt-instance add`` and the idea is to re-use the disk > > > creation tasklet that we will have for ``gnt-disk create``, so yes, we > > > would have tasklet chains then. > > > > > > > > > > > > > > Hope the above makes sense :) > > > > > > > > dimara > > > > > > > > > +Except for creating diskless instances on purpose, the diskless disk > > > > > +template should not be required if an instance becomes diskless, > > but the > > > > > +instance should be considered to be in the state ``diskless``. > > > > > -- > > > > > 2.6.0.rc2.230.g3dd15c0 > > > > > > > > > > -- > > > Lisa Velden > > > Software Engineer > > > [email protected] > > > > > > Google Germany GmbH > > > Erika-Mann-Straße 33 > > > 80636 München > > > > > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > > > Registergericht und -nummer: Hamburg, HRB 86891 > > > Sitz der Gesellschaft: Hamburg > > > > > > -- > Lisa Velden > Software Engineer > [email protected] > > Google Germany GmbH > Erika-Mann-Straße 33 > 80636 München > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
signature.asc
Description: Digital signature
