On Fri, Jul 18, 2014 at 11:21 AM, Dimitris Aragiorgis <[email protected]> wrote:
> * 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 > > Wonderful, thanks! > > > > > > 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 > 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
