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