Since nobody is replying, and the document has been looked over many times before anyway: one final LGTM, and I will push the design doc to master.
Cheers, Riba On Wed, Apr 9, 2014 at 12:43 PM, Ilias Tsitsimpis <[email protected]> wrote: > On Mon, Apr 07, 2014 at 11:17AM, Hrvoje Ribicic wrote: > > First of all, sorry for this having gone by without a proper response for > > so long. Review follows inline: > > > > > > On Thu, Mar 27, 2014 at 3:30 PM, Ilias Tsitsimpis <[email protected]> > wrote: > > > > Hello Hrvoje, > > Thanks for reviewing this. Here is the interdiff with the changes that > you suggested. > > diff --git a/doc/design-disks.rst b/doc/design-disks.rst > index 61900cb..eb1aaed 100644 > --- a/doc/design-disks.rst > +++ b/doc/design-disks.rst > @@ -87,7 +87,7 @@ A new function ``GetInstanceDisks`` will be added to the > config that given an > instance will return a list of Disk objects with the disks attached to > this > instance. This list will be exactly the same as 'instance.disks' was > before. > Everywhere in the code we are going to replace the 'instance.disks' > (which from > -now one will contain a list of disk UUIDs) with the function > +now on will contain a list of disk UUIDs) with the function > ``GetInstanceDisks``. > > Since disks will not be part of the `Instance` object any more, > 'all_nodes' and > @@ -109,9 +109,10 @@ points to a disk that doesn't exist in the config). > > The 'upgrade' operation for the config should check if disks are top level > citizens and if not it has to extract the disk objects from the instances > and > -replace them with their uuids. In case of the 'downgrade' operation (where > -disks will be made again part of the `Instance` object) all disks that > are not > -attached to any instance at all will be ignored (removed from config). > +replace them with their uuids. In case of the 'downgrade' operation all > +disks will be made again part of the `Instance` object. This should work > +for now as there are no unattached disks, but later on we should make > +the downgrade prompt for confirmation in case of unattached disks. > > > Apply Disk modifications > > > Thanks, > Ilias > > > > This patch adds a design document detailing the implementation of disks > > > as new top-level citizens in the config file (just like instances, > > > nodes etc). > > > > > > The process is divided in four steps, and currently only the first one > > > is being described in depth. > > > > > > Signed-off-by: Ilias Tsitsimpis <[email protected]> > > > --- > > > > > > Hello team, > > > > > > I'm sending the revised design doc, after incorporating all your > > > comments from the corresponding thread. > > > > > > Thanks a lot, > > > Ilias > > > > > > > > > doc/design-disks.rst | 166 > > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > doc/index.rst | 1 + > > > 2 files changed, 167 insertions(+) > > > create mode 100644 doc/design-disks.rst > > > > > > diff --git a/doc/design-disks.rst b/doc/design-disks.rst > > > new file mode 100644 > > > index 0000000..61900cb > > > --- /dev/null > > > +++ b/doc/design-disks.rst > > > @@ -0,0 +1,166 @@ > > > +===== > > > +Disks > > > +===== > > > + > > > +.. contents:: :depth: 4 > > > + > > > +This is a design document detailing the implementation of disks as a > new > > > +top-level citizen in the config file (just like instances, nodes etc). > > > + > > > + > > > +Current state and shortcomings > > > +============================== > > > + > > > +Currently, Disks are stored in Ganeti's config file as a list > > > +(container) of Disk objects under the Instance in which they belong. > > > +This implementation imposes a number of limitations: > > > + > > > +* A Disk object cannot live outside an Instance. This means that one > > > + cannot detach a disk from an instance (without destroying the disk) > > > + and then reattach it (to the same or even to a different instance). > > > + > > > +* Disks are not taggable objects, as only top-level citizens of the > > > + config file can be made taggable. Having taggable disks will allow > for > > > + further customizations. > > > + > > > + > > > +Proposed changes > > > +================ > > > + > > > +The implementation is going to be split in four parts: > > > + > > > +* Make disks a top-level citizen in config file. The Instance object > > > + will no longer contain a list of Disk objects, but a list of disk > > > + UUIDs. > > > + > > > +* Add locks for Disk objects and make them taggable. > > > + > > > +* Allow to attach/detach an existing disk to/from an instance. > > > + > > > +* Allow creation/modification/deletion of disks that are not attached > to > > > + any instance (requires new LUs for disks). > > > + > > > + > > > +Design decisions > > > +================ > > > + > > > +Disks as config top-level citizens > > > +---------------------------------- > > > + > > > +The first patch-series is going to add a new top-level citizen in the > > > +config object (namely ``disks``) and separate the disk objects from > the > > > +instances. In doing so there are a number of problems that we have to > > > +overcome: > > > + > > > +* How the Disk object will be represented in the config file and how > it > > > + is going to be connected with the instance it belongs to. > > > + > > > +* How the existing code will get the disks belonging to an instance. > > > + > > > +* What it means for a disk to be attached/detached to/from an > instance. > > > + > > > +* How disks are going to be created/deleted, attached/detached using > > > + the existing code. > > > + > > > + > > > +Disk representation > > > +~~~~~~~~~~~~~~~~~~~ > > > + > > > +The ``Disk`` object gets two extra slots, ``_TIMESTAMPS`` and > > > +``serial_no``. > > > + > > > +The ``Instance`` object will no longer contain the list of disk > objects > > > +that are attached to it. Instead, an Instance object will refer to its > > > +disks using their UUIDs. Since the order in which the disks are > attached > > > +to an instance is important we are going to have a list of disk UUIDs > > > +under the Instance object which will denote the disks attached to the > > > +instance and their order at the same time. So the Instance's ``disks`` > > > +slot is going to be a list of disk UUIDs. The `Disk` object is not > going > > > +to have a slot pointing to the `Instance` in which it belongs since > this > > > +is redundant. > > > + > > > + > > > +Get instance's disks > > > +~~~~~~~~~~~~~~~~~~~~ > > > + > > > +A new function ``GetInstanceDisks`` will be added to the config that > > > given an > > > +instance will return a list of Disk objects with the disks attached to > > > this > > > +instance. This list will be exactly the same as 'instance.disks' was > > > before. > > > +Everywhere in the code we are going to replace the 'instance.disks' > > > (which from > > > +now one will contain a list of disk UUIDs) with the function > > > > > > > Nitpick: s/one/on/ > > > > > > > +``GetInstanceDisks``. > > > + > > > +Since disks will not be part of the `Instance` object any more, > > > 'all_nodes' and > > > +'secondary_nodes' can not be `Instance`'s properties. Instead we will > use > > > the > > > +functions ``GetInstanceNodes`` and ``GetInstanceSecondaryNodes`` from > the > > > +config to compute these values. > > > + > > > + > > > +Configuration changes > > > +~~~~~~~~~~~~~~~~~~~~~ > > > + > > > +The ``ConfigData`` object gets one extra slot: ``disks``. Also there > > > +will be two new functions, ``AddDisk`` and ``RemoveDisk`` that will > > > +create/remove a disk objects from the config. > > > + > > > +The ``VerifyConfig`` function will be changed so it can check that > there > > > +are no dangling pointers from instances to disks (i.e. an instance > > > +points to a disk that doesn't exist in the config). > > > + > > > +The 'upgrade' operation for the config should check if disks are top > level > > > +citizens and if not it has to extract the disk objects from the > instances > > > and > > > +replace them with their uuids. In case of the 'downgrade' operation > (where > > > +disks will be made again part of the `Instance` object) all disks that > > > are not > > > +attached to any instance at all will be ignored (removed from config). > > > > > > > We should make the downgrade prompt for confirmation in case of > unattached > > disks. > > In case the user downgrades and there is a disk that has been perhaps > > mistakenly detached from an instance, prior versions will not allow the > > user to reattach that disk easily. > > This is not a concern with the current state of the design doc, but could > > be forgotten by the time the next phase rolls out. > > > > To bypass the confirmation (as necessary for e.g. QA), we can also add a > > --force option. > > > > > > > + > > > + > > > +Apply Disk modifications > > > +~~~~~~~~~~~~~~~~~~~~~~~~ > > > + > > > +There are four operations that can be performed to a `Disk` object: > > > + > > > +* Create a new `Disk` object and save it to the config. > > > + > > > +* Remove an existing `Disk` object from the config. > > > + > > > +* Attach an existing `Disk` to an existing `Instance`. > > > + > > > +* Detach an existing `Disk` from an existing `Instance`. > > > + > > > +The first two operations will be performed using the config functions > > > +``AddDisk`` and ``RemoveDisk`` respectively where the last two > operations > > > +will be performed using the functions ``AttachInstDisk`` and > > > +``DetachInstDisk``. > > > + > > > +Since Ganeti doesn't allow for a `Disk` object to not be attached > > > anywhere (for > > > +now) we will create two wrapper functions (namely ``AddInstDisk`` and > > > +``RemoveInstDisk``) which will add and attach a disk at the same time > > > +(respectively detach and remove a disk). > > > + > > > +In addition since Ganeti doesn't allow for a `Disk` object to be > attached > > > to > > > +more than one `Instance` at once, when attaching a disk to an > instance we > > > have > > > +to make sure that the disk is not attached anywhere else. > > > + > > > + > > > +Backend changes > > > +~~~~~~~~~~~~~~~ > > > + > > > +The backend needs access to the disks of an `Instance` but doesn't > have > > > access to > > > +the `GetInstanceDisks` function from the config file. Thus we will > create > > > a new > > > +`Instance` slot (namely ``disks_info``) that will get annotated > (during > > > RPC) > > > +with the instance's disk objects. So in the backend we will only have > to > > > +replace the ``disks`` slot with ``disks_info``. > > > + > > > + > > > +.. TODO: Locks for Disk objects > > > + > > > +.. TODO: Attach/Detach disks > > > + > > > +.. TODO: LUs for disks > > > + > > > + > > > +.. vim: set textwidth=72 : > > > +.. Local Variables: > > > +.. mode: rst > > > +.. fill-column: 72 > > > +.. End: > > > diff --git a/doc/index.rst b/doc/index.rst > > > index ab2cccf..419d560 100644 > > > --- a/doc/index.rst > > > +++ b/doc/index.rst > > > @@ -97,6 +97,7 @@ Draft designs > > > design-cmdlib-unittests.rst > > > design-cpu-pinning.rst > > > design-device-uuid-name.rst > > > + design-disks.rst > > > design-file-based-storage.rst > > > design-hroller.rst > > > design-hotplug.rst > > > -- > > > 1.9.0 > > > > > > > Apart from the tiny comments, this does LGTM. Others may also want to > > comment? >
