* Hrvoje Ribicic <[email protected]> [2014-07-18 10:15:43 -0400]: > On Fri, Jul 18, 2014 at 5:37 AM, Dimitris Aragiorgis <[email protected]> > wrote: > > > Hi, > > > > * Hrvoje Ribicic <[email protected]> [2014-07-17 16:57:07 -0400]: > > > > > Hi there, > > > > > > I think I found a bug and have an interdiff with a fix ready: > > > Care to switch things up and review this? :D > > > > > > > Well the point here is to decide where exactly the snapshot action will > > fail. In my version it won't fail until Ganeti will try to execute > > the script. I agree that this might be a bit late. The interdiff > > you proposed does not really fixes it. > > > > es_files inside ExtStorageFromDisk() will always have the path for > > the snapshot script (os.stat() check is done later) and thus > > Extstorage.snapshot_script will never be None. I guess if we go > > with your interdiff we should add: > > > > diff --git a/lib/storage/extstorage.py b/lib/storage/extstorage.py > > index 71b4f1b..8343de1 100644 > > --- a/lib/storage/extstorage.py > > +++ b/lib/storage/extstorage.py > > @@ -393,6 +393,7 @@ def ExtStorageFromDisk(name, base_dir=None): > > st = os.stat(es_files[filename]) > > except EnvironmentError, err: > > if not required: > > + es_files[filename] = None > > continue > > return False, ("File '%s' under path '%s' is missing (%s)" % > > (filename, es_dir, utils.ErrnoOrStr(err))) > > > > > > Otherwise we should factor out the os.stat() and the "is executable" > > checks and do them inside _ExtStorageAction() as well. > > > > What do you think? > > > > > You are completely right, the interdiff would break things. > > I would like an explicitly thrown error message, as we can detect the issue > when it happens and having a good error message saves some time for the > user. > At the same time, I would not like to lose one advantage of the previous > state of the patch - that the user can simply put the missing snapshot > script in on the fly. > > Factoring out the stat and executable checks sounds like a good idea. > Could this result in some sort of performance hit? We can repeat checks for > optional parameters only. >
Don't think that it will slow things down for any reason. So I will
factor the checks out and will do them inside ExtStorageFromDisk()
for mandatory scripts (just as it happens now) and inside
_ExtStorageAction() for the corresponding script and throw the error
message as you suggested.
I'll resend the patch set with the above changes and rebased to current
master so that it can be easily merged.
Cheers,
dimara
>
> > > diff --git a/lib/storage/extstorage.py b/lib/storage/extstorage.py
> > > index 71b4f1b..42bbc7a 100644
> > > --- a/lib/storage/extstorage.py
> > > +++ b/lib/storage/extstorage.py
> > > @@ -326,6 +326,10 @@ def _ExtStorageAction(action, unique_id, ext_params,
> > > script_name = action + "_script"
> > > script = getattr(inst_es, script_name)
> > >
> > > + if script is None:
> > > + base.ThrowError("Action '%s' not supported by this ExtStorage
> > > provider" %
> > > + action)
> > > +
> > > # Run the external script
> > > result = utils.RunCmd([script], env=create_env,
> > > cwd=inst_es.path, output=logfile,)
> > > @@ -417,16 +421,18 @@ def ExtStorageFromDisk(name, base_dir=None):
> > > parameters = [v.split(None, 1) for v in parameters]
> > >
> > > es_obj = \
> > > - objects.ExtStorage(name=name, path=es_dir,
> > > -
> > create_script=es_files[constants.ES_SCRIPT_CREATE],
> > > -
> > remove_script=es_files[constants.ES_SCRIPT_REMOVE],
> > > - grow_script=es_files[constants.ES_SCRIPT_GROW],
> > > -
> > attach_script=es_files[constants.ES_SCRIPT_ATTACH],
> > > -
> > detach_script=es_files[constants.ES_SCRIPT_DETACH],
> > > -
> > > setinfo_script=es_files[constants.ES_SCRIPT_SETINFO],
> > > -
> > verify_script=es_files[constants.ES_SCRIPT_VERIFY],
> > > -
> > > snapshot_script=es_files[constants.ES_SCRIPT_SNAPSHOT],
> > > - supported_parameters=parameters)
> > > + objects.ExtStorage(
> > > + name=name, path=es_dir,
> > > + create_script=es_files[constants.ES_SCRIPT_CREATE],
> > > + remove_script=es_files[constants.ES_SCRIPT_REMOVE],
> > > + grow_script=es_files[constants.ES_SCRIPT_GROW],
> > > + attach_script=es_files[constants.ES_SCRIPT_ATTACH],
> > > + detach_script=es_files[constants.ES_SCRIPT_DETACH],
> > > + setinfo_script=es_files[constants.ES_SCRIPT_SETINFO],
> > > + verify_script=es_files[constants.ES_SCRIPT_VERIFY],
> > > + snapshot_script=es_files.get(constants.ES_SCRIPT_SNAPSHOT, None),
> > > + supported_parameters=parameters
> > > + )
> > > return True, es_obj
> > >
> > >
> > > On Mon, Jul 14, 2014 at 2:28 PM, Dimitris Aragiorgis <[email protected]>
> > > wrote:
> > >
> > > > * 'Hrvoje Ribicic' via ganeti-devel <[email protected]>
> > > > [2014-07-14 16:07:09 +0200]:
> > > >
> > > > > Hi Dimitris,
> > > > >
> > > > > This is ok as is - I will just just consider this patch a part of the
> > > > > previous patch series.
> > > > > I will run QA on the patches just in case due to the general
> > refactoring
> > > > > changes, and then finish off reviewing everything and push if all is
> > > > well.
> > > > >
> > > >
> > > > Ok. Perfect.
> > > >
> > > > > Btw, this patch does not fix RBD snapshots (issue 545) in case the
> > Ganeti
> > > > > RBD disk template was used.
> > > > > The rbd snap create comment in that issue suggests that external
> > storage
> > > > > was not used.
> > > > >
> > > >
> > > > True story. I had snapshot support for the ext disk template
> > implemented
> > > > a while ago and given the latest comment on the issue I submitted the
> > > > patches.
> > > > I guess a Snapshot() method plus minor changes in the backend should do
> > > > the job..
> > > >
> > > > > Do you mind if I follow up with a patch to fix this or do you want
> > to do
> > > > it?
> > > > > I have a QA set up that uses this disk template, so it might be more
> > > > > convenient for me.
> > > > >
> > > >
> > > > Please, be my guest :)
> > > >
> > > > Thanks a lot,
> > > > dimara
> > > >
> > > > > Cheers,
> > > > > Riba
> > > > >
> > > > > On Mon, Jul 14, 2014 at 8:14 AM, Dimitris Aragiorgis <
> > [email protected]>
> > > > > wrote:
> > > > >
> > > > > >
> > > > > > Hi!
> > > > > >
> > > > > > You are right about breaking compatibility. Are you OK with these
> > > > fixes?
> > > > > > If yes I can squash the relevant diffs with the previous commits,
> > > > rebase
> > > > > > the whole patch set with current master and resend it.
> > > > > >
> > > > > > Let me know, thanks,
> > > > > > dimara
> > > > > >
> > > > > >
> > > > > >
> > > > > > * Dimitris Aragiorgis <[email protected]> [2014-07-14 09:10:46
> > +0300]:
> > > > > >
> > > > > > > ..for the ExtStorage providers. This way we do not break
> > > > > > > compatibility with existing providers that do not implement
> > > > > > > such a functionality.
> > > > > > >
> > > > > > > Signed-off-by: Dimitris Aragiorgis <[email protected]>
> > > > > > > ---
> > > > > > > doc/design-shared-storage.rst | 6 +++++-
> > > > > > > lib/masterd/instance.py | 4 +++-
> > > > > > > lib/storage/extstorage.py | 7 ++++++-
> > > > > > > man/ganeti-extstorage-interface.rst | 4 ++++
> > > > > > > 4 files changed, 18 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/doc/design-shared-storage.rst
> > > > > > b/doc/design-shared-storage.rst
> > > > > > > index c1b3c01..793c522 100644
> > > > > > > --- a/doc/design-shared-storage.rst
> > > > > > > +++ b/doc/design-shared-storage.rst
> > > > > > > @@ -198,7 +198,7 @@ provider is expected to provide the following
> > > > > > scripts:
> > > > > > > - ``detach``
> > > > > > > - ``setinfo``
> > > > > > > - ``verify``
> > > > > > > -- ``snapshot``
> > > > > > > +- ``snapshot`` (optional)
> > > > > > >
> > > > > > > All scripts will be called with no arguments and get their
> > input via
> > > > > > > environment variables. A common set of variables will be
> > exported
> > > > for
> > > > > > > @@ -233,6 +233,10 @@ error, accompanied by an appropriate error
> > > > message
> > > > > > on stderr. The
> > > > > > > the block device's full path, after it has been successfully
> > > > attached to
> > > > > > > the host node. On error it should return non-zero.
> > > > > > >
> > > > > > > +To keep backwards compatibility we let the ``snapshot`` script
> > be
> > > > > > > +optional. If present then the provider will support instance
> > backup
> > > > > > > +export as well.
> > > > > > > +
> > > > > > > Implementation
> > > > > > > --------------
> > > > > > >
> > > > > > > diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
> > > > > > > index c1639e3..ebd563c 100644
> > > > > > > --- a/lib/masterd/instance.py
> > > > > > > +++ b/lib/masterd/instance.py
> > > > > > > @@ -1157,7 +1157,9 @@ class ExportInstanceHelper(object):
> > > > > > > self._removed_snaps = [False] * len(instance.disks)
> > > > > > >
> > > > > > > def CreateSnapshots(self):
> > > > > > > - """Creates an LVM snapshot for every disk of the instance.
> > > > > > > + """Creates a snapshot for every disk of the instance.
> > > > > > > +
> > > > > > > + Currently support drbd, plain and ext disk templates.
> > > > > > >
> > > > > > > """
> > > > > > > assert not self._snap_disks
> > > > > > > diff --git a/lib/storage/extstorage.py
> > b/lib/storage/extstorage.py
> > > > > > > index 19692a3..71b4f1b 100644
> > > > > > > --- a/lib/storage/extstorage.py
> > > > > > > +++ b/lib/storage/extstorage.py
> > > > > > > @@ -381,14 +381,19 @@ def ExtStorageFromDisk(name,
> > base_dir=None):
> > > > > > > # an optional one
> > > > > > > es_files = dict.fromkeys(constants.ES_SCRIPTS, True)
> > > > > > >
> > > > > > > + # Let the snapshot script be optional
> > > > > > > + es_files[constants.ES_SCRIPT_SNAPSHOT] = False
> > > > > > > +
> > > > > > > es_files[constants.ES_PARAMETERS_FILE] = True
> > > > > > >
> > > > > > > - for (filename, _) in es_files.items():
> > > > > > > + for (filename, required) in es_files.items():
> > > > > > > es_files[filename] = utils.PathJoin(es_dir, filename)
> > > > > > >
> > > > > > > try:
> > > > > > > st = os.stat(es_files[filename])
> > > > > > > except EnvironmentError, err:
> > > > > > > + if not required:
> > > > > > > + continue
> > > > > > > return False, ("File '%s' under path '%s' is missing
> > (%s)" %
> > > > > > > (filename, es_dir, utils.ErrnoOrStr(err)))
> > > > > > >
> > > > > > > diff --git a/man/ganeti-extstorage-interface.rst
> > > > > > b/man/ganeti-extstorage-interface.rst
> > > > > > > index 44a1adb..97ce14f 100644
> > > > > > > --- a/man/ganeti-extstorage-interface.rst
> > > > > > > +++ b/man/ganeti-extstorage-interface.rst
> > > > > > > @@ -220,6 +220,10 @@ respectively (see above).
> > > > > > >
> > > > > > > The script returns ``0`` on success.
> > > > > > >
> > > > > > > +Please note that this script is optional and not all providers
> > > > should
> > > > > > > +implement it. Of course if it is not present, instance backup
> > export
> > > > > > > +will not be supported for the given provider.
> > > > > > > +
> > > > > > > TEXT FILES
> > > > > > > ----------
> > > > > > >
> > > > > > > --
> > > > > > > 1.7.10.4
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > Hrvoje Ribicic
> > > > > Ganeti Engineering
> > > > > Google Germany GmbH
> > > > > Dienerstr. 12, 80331, München
> > > > >
> > > > > Registergericht und -nummer: Hamburg, HRB 86891
> > > > > Sitz der Gesellschaft: Hamburg
> > > > > Geschäftsführer: Graham Law, Christine Elizabeth Flores
> > > > > Steuernummer: 48/725/00206
> > > > > Umsatzsteueridentifikationsnummer: DE813741370
> > > >
> > >
> > >
> > >
> > > Hrvoje Ribicic
> > > Ganeti Engineering
> > > Google Germany GmbH
> > > Dienerstr. 12, 80331, München
> > >
> > > Registergericht und -nummer: Hamburg, HRB 86891
> > > Sitz der Gesellschaft: Hamburg
> > > Geschäftsführer: Graham Law, Christine Elizabeth Flores
> > > Steuernummer: 48/725/00206
> > > Umsatzsteueridentifikationsnummer: DE813741370
> >
>
>
>
> Hrvoje Ribicic
> Ganeti Engineering
> Google Germany GmbH
> Dienerstr. 12, 80331, München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Steuernummer: 48/725/00206
> Umsatzsteueridentifikationsnummer: DE813741370
signature.asc
Description: Digital signature
