On Tue, Oct 09, 2012 at 06:59:55PM +0300, Constantinos Venetsanopoulos wrote:
> On 10/08/2012 04:55 PM, Iustin Pop wrote:
> >On Thu, Sep 27, 2012 at 05:43:56PM +0300, Constantinos Venetsanopoulos wrote:
> >>On 09/27/2012 04:17 PM, Iustin Pop wrote:
> >>>On Wed, Sep 26, 2012 at 05:38:18PM +0300, Constantinos Venetsanopoulos
> >>>wrote:
> >>>>With this commit we introduce the External Storage Interface
> >>>>to Ganeti, abbreviated: ExtStorage Interface.
> >>>>
> >>>>The ExtStorage Interface provides Ganeti with the ability to interact
> >>>>with externally connected shared storage pools, visible by all
> >>>>VM-capable nodes. This means that Ganeti is able to handle VM disks
> >>>>that reside inside a NAS/SAN or any distributed block storage provider.
> >>>>
> >>>>The ExtStorage Interface provides a clear API, heavily inspired by the
> >>>>gnt-os-interface API, that can be used by storage vendors or sysadmins
> >>>>to write simple ExtStorage Providers (correlated to gnt-os-interface's
> >>>>OS Definitions). Those Providers will glue externally attached shared
> >>>>storage with Ganeti, without the need of preprovisioned block devices
> >>>>on Ganeti VM-capable nodes as confined be the current `blockdev' disk
> >>>>template.
> >>>>
> >>>>To do so, we implement a new disk template called `ext' (of type
> >>>>DTS_EXT_MIRROR) that passes control to externally provided scripts
> >>>>(the ExtStorage Provider) for the template's basic functions:
> >>>>
> >>>> create / attach / detach / remove / grow
> >>>>
> >>>>The scripts reside under ES_SEARCH_PATH (correlated to OS_SEARCH_PATH)
> >>>>and only one ExtStorage Provider is supported called `ext'.
> >>>>
> >>>>The disk's logical id is the tuple ('ext', UUID.ext.diskX), where UUID
> >>>>is generated as in disk template `plain' and X is the disk's index.
> >>>A few general comments, or rather questions. Some of the are rather
> >>>design level, but they become more apparent when reading the code only,
> >>>sorry.
> >>No problem.
> >>
> >>>I'm not sure creating one log file per operation is a good idea. While
> >>>installing/renaming an instance is a (somewhat) rare operation,
> >>>activating the disks of an instance is a much often one. I know that
> >>>right now in attach you don't create a log, but that seems to be the
> >>>intention (hence the TODO). Thoughts?
> >>Indeed, creating a log file during attach was the initial intention, however
> >>I understand your point since specifically attach is run multiple times
> >>even during a single instance addition. On the other hand, I think creation
> >>and removal of a disk should reside in different files because they do not
> >>happen so often. Furthermore, these log files are the only place where the
> >>admin actually can see what happens, when something goes wrong with
> >>the external hardware and are also very helpful when writing custom
> >>ExtStorage providers.
> >I'm not sure I understand this. Do you mean about debug/informational
> >messages? I believe that actual errors will simply propagate to ganeti
> >as failures in activation/other ops.
>
> Actual errors do propagate to ganeti. However the log doesn't
> (except the "n" last lines of the logfile). The user has to go back to
> the log file to see what happened exactly. From my experience writing
> ExtStorage providers, it really helps being able to look at the attach
> script's stderr.
I see.
> >>What do you think? Another option would be to keep the log files for
> >>create/remove/grow and do not log the attach/detach. Or keep them
> >>for now to see the workflow and if they produce a lot of unnecessary
> >>info that we do not want to keep, merge all actions in a single log file
> >>for each disk (but this needs code refactoring).
> >Indeed.
> >
> >Maybe a separate option would be, as you say, log only
> >create/remove/grow and recommend that attach/detach options log to
> >syslog? Or request that all logging goes to syslog?
>
> After more consideration, I think that not logging attach/detach ops is
> not the right thing to do.
>
> Currently no logging for attach happens, due to the issue with RunResult().
> When this gets fixed, I think it would be best to log everything related
> to a single disk in the same log file, so we don't miss anything, keep
> related things together, and do not create too many small files. As an
> example the file would contain a series of
> [create,attach,attach,attach,detach,attach,...,grow,...,detach,remove] ops.
>
> If you are OK with this approach, I will contribute the relevant patches as
> soon as the code gets merged and the issue in the TODO comment gets fixed.
Sounds good. Thanks for taking the time to think this through!
> >>>Also, it seems by reading the code that Ganeti generates an UUID for the
> >>>volume, and it asks the provider to create one such drive; basically,
> >>>the provider will get "please create disk f0559a.0 of size 500G", right?
> >>That's right.
> >>
> >>>This means that only the ganeti configuration has the mapping instance
> >>>name to disks, and that losing the ganeti configuration would mean
> >>>instantly losing all instance data since we can't map it back to a VM.
> >>>For 'plain' disks, we explicitly tag the LV names with the name of the
> >>>instance, to safeguard against such problems. Do you think this is not a
> >>>problem?
> >>To be honest, I haven't thought about this case, but I understand
> >>your point.
> >>
> >>> Or could we add the instance name to the external provider
> >>>create call, such that providers could do such an association
> >>>themselves, if they care?
> >>That would be fine, but I can't see an easy way to do that, since the
> >>instance object doesn't find its way inside bdev, and it wouldn't be clean
> >>to add new parameters in the Create call. What we could do though,
> >>would be to prepend the volume name with the instance's name.
> >>Much like you do with 'plain' disks. Now the disk name has the following
> >>format:
> >>
> >>'04859fac4.ext.diskX' where X is the index.
> >>
> >>We could change that to 'instancename-04859fac4.ext.diskX' and
> >>document it appropriately, so that the mapping will be there for the
> >>providers to use it, if they care. Would that be OK?
> >Hmm, not really. The instance name can and will change, so having it as
> >part of the disk ID is not good.
> >
> >But note that the instance "info" is already passed to BlockDevCreate,
> >and while not passed to the actual Create call, is passed to
> >BlockDevice.SetInfo()? Could we reuse that here too?
>
> If I understand correctly, you prefer disk names to be independent of
> the instance name, then go and tag the disk object itself, e.g., the lv in
> the case of lvm.
Yes.
> A similar approach would be for me to implement the
> ExtStorageDevice.SetInfo() call which runs an external "setinfo" script.
> The setinfo script would mark the disk with the instance name or other
> metadata in a provider-specific way. For example the volumes of a NAS
> would be marked with vendor specific metadata and the admin would be
> able to recover instance data by examining these.
Yes, exactly.
> However, even if this happens, it seems I can only find code setting lv
> tags during blockdev creation and not instance rename. So, how do
> lv tags stay consistent after renames? Thus, even we follow the approach
> you propose, the admin will still have problems tracking down instance
> disks after loosing the ganeti config. Thoughts?
That is actually a current bug (only internally raised so far). We do
plan to fix the rename LU to include this, by exposing SetInfo via an
rpc call.
So yes, let's take this approach, and fix the rename issue separately.
thanks!
iustin
