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?
signature.asc
Description: Digital signature
