On Wed, Jan 20, 2016 at 12:25 AM, Dimitris Aragiorgis < [email protected]> wrote:
> 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. > See interdiff below. > > > 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. > yes, of course, see interdiff. > > > * ``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 > Sadly, I forgot to add the previous changes, so here is a little more ugly interdiff: diff --git a/doc/design-standalone-disks.rst b/doc/design-standalone-disks.rst index 71cca81..37b68ee 100644 --- a/doc/design-standalone-disks.rst +++ b/doc/design-standalone-disks.rst @@ -75,21 +75,46 @@ 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 +* ``gnt-disk create --disk-template *DT* --size *VAL* 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 +* ``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 [--target-disk-name *NAME*] + *SRC_DISK_UUID|SRC_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 [--target-disk-name *NAME*] + *SRC_DISK_UUID|SRC_DISK_NAME*``: creates a new disk with read-write + access mode and optionally with a given disk name + +* ``gnt-disk convert --target-disk-template *DT* *DISK_UUID|DISK_NAME*``: + converts the disk template of a disk into the new disk template + +* ``gnt-disk copy [--target-disk-name *NAME*] --target-disk-template *DT* + *SRC_DISK_UUID|SRC_DISK_NAME*``: creates a new disk that is identical + to the source disk, but has a different disk template 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 +* Make the storage path information retrieved from the logical id from + disks also available through the basic instance information from the + RAPI * Disallow all disk operations via the RAPI, except for moves @@ -103,13 +128,15 @@ 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 attach [--position *N*] {--uuid *DISK_UUID* | --name + *DISK_NAME*} *INSTANCE*``: attaches a disk to an instance. If the disk + was not in the configuration before, an error is raised. If no position + argument was provided, the disk will be attached at the last index of + the instance's disks. -* ``gnt-disk detach *DISK_UUID|DISK_NAME|DISK_POSITION* *INSTANCE*``: - detaches a disk from an instance and leaves it in the Ganeti - configuration +* ``gnt-disk detach {--uuid *DISK_UUID* | --name *DISK_NAME* | --position + *N*} *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 > > > 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 > -- 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
