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
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
