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