On Wed, Oct 10, 2012 at 12:07:31PM +0300, Constantinos Venetsanopoulos wrote:
> On 10/09/2012 07:10 PM, Iustin Pop wrote:
> >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.
>
> OK. So, to sum up, I'll implement the SetInfo functionality, fix all the
> small issues we discussed in previous emails and send an interdiff.
>
> When we fix the RunResult TODO and the code gets merged, I'll send
> a patch that will merge all log files into a single one for each disk.
>
> Finally, after the merge, I will also send a separate patch that
> documents the SetInfo functionality in the design doc and man pages.
>
> Is that OK?
Sounds good, thanks!
iustin
