Merge lp://qastaging/~rconradharris/nova/backup_schedule_extension into lp://qastaging/~hudson-openstack/nova/trunk

130 + import tempo.client

Can we put this at the top of the file with the other imports. Unit tests are going to be run anyway so putting the import down here doesn't seem to do much. Alternatively I guess unit tests could be skipped if tempo isn't found?

Agreed ... it would be nice that, if tempo can't be imported it degrades gracefully (including the unit tests). Unless this is already the case?

> Can we put this at the top of the file with the other imports. Unit tests are going to be run anyway > so putting the import down here doesn't seem to do much.

Having `import tempo` in the `__init__` means that it won't be imported unless the Backup_schedule class is actually created.

Since the unit-tests don't actually construct the Backup_schedule class, this means `tempo` isn't required to run the tests.

This also means that `tempo` isn't required in a production environment unless the backup schedule extension is used.

Since API extensions are eager-loaded--everything in the 'contrib' directory is imported in search of an Extension class--I think it makes sense not to have 3rd party imports in the module-scope.

If we did, that would mean that users would be required to have every dependency of every (optional) extension. I don't think that scales.

> Having `import tempo` in the `__init__` means that it won't be imported unless the Backup_schedule
> class is actually created.

Right, but we're putting Tempo in pip-requires and in the Ubuntu package right? So this should be considered a core extension (I'll attempt to not get started on the hilarity of that statement to me). If it were an 'optional extension' it would be packaged separately to manage dependencies.

> Since API extensions are eager-loaded--everything in the 'contrib' directory is imported in search
> of an Extension class--I think it makes sense not to have 3rd party imports in the module-scope.

This extension is in the base source code checkout, so if I checkout the Nova source, and do ./run_tests.sh it's going to require tempo, right? If so it's a requirement of the project unless we're up front about it and make the tests skip if tempo isn't installed.

> If we did, that would mean that users would be required to have every dependency of every
> (optional) extension. I don't think that scales.

No argument here. I really don't want to have tempo in pip-requires or the Ubuntu package if I'm not going to use it. Since it's there (or going to be there?), shouldn't we just show the imports up front?

My main argument though is that imports should always be up top in my opinion, even if you have to do something like:

try:
    import tempo
except ImportError:
    LOG.warn("Tempo not installed, backup extension won't work.")

> > Having `import tempo` in the `__init__` means that it won't be imported
> unless the Backup_schedule
> > class is actually created.
>
> Right, but we're putting Tempo in pip-requires and in the Ubuntu package
> right? So this should be considered a core extension (I'll attempt to not get
> started on the hilarity of that statement to me).

Indeed, which is why I think the term "extension" has been a little bit abused in Nova ;)

> If it were an 'optional
> extension' it would be packaged separately to manage dependencies.

Indeed.

> > Since API extensions are eager-loaded--everything in the 'contrib' directory
> is imported in search
> > of an Extension class--I think it makes sense not to have 3rd party imports
> in the module-scope.
>
> This extension is in the base source code checkout, so if I checkout the Nova
> source, and do ./run_tests.sh it's going to require tempo, right? If so it's a
> requirement of the project unless we're up front about it and make the tests
> skip if tempo isn't installed.

++

> > If we did, that would mean that users would be required to have every
> dependency of every
> > (optional) extension. I don't think that scales.
>
> No argument here. I really don't want to have tempo in pip-requires or the
> Ubuntu package if I'm not going to use it. Since it's there (or going to be
> there?), shouldn't we just show the imports up front?
>
> My main argument though is that imports should always be up top in my opinion,
> even if you have to do something like:
>
> try:
> import tempo
> except ImportError:
> LOG.warn("Tempo not installed, backup extension won't work.")

Agree fully.
-jay

Thanks for the comments guys.

> > > Having `import tempo` in the `__init__` means that it won't be imported
> > unless the Backup_schedule
> > > class is actually created.
> >
> > Right, but we're putting Tempo in pip-requires and in the Ubuntu package
> > right? So this should be considered a core extension (I'll attempt to not
> get
> > started on the hilarity of that statement to me).

Good points here. Technically, `tempo` doesn't need to be in the pip-requires since, as mentioned above, the Schedule class isn't actually constructed (yet).

I reflexively added it to the `pip-requires` not for the venv-construction, but really just to indicate a dependency.

Perhaps we should separate out core-dependencies vs. optionals ones (as you allude to): maybe pip-optional-requires (ignore the oxymoron there ;-).

All that said, I see your point and agree.

> Indeed, which is why I think the term "extension" has been a little bit abused
> in Nova ;)
>
> > If it were an 'optional
> > extension' it would be packaged separately to manage dependencies.
>
> Indeed.

Yep, see above.

> > > Since API extensions are eager-loaded--everything in the 'contrib'
> directory
> > is imported in search
> > > of an Extension class--I think it makes sense not to have 3rd party
> imports
> > in the module-scope.
> >
> > This extension is in the base source code checkout, so if I checkout the
> Nova
> > source, and do ./run_tests.sh it's going to require tempo, right? If so it's
> a
> > requirement of the project unless we're up front about it and make the tests
> > skip if tempo isn't installed.
>
> ++
>
> > > If we did, that would mean that users would be required to have every
> > dependency of every
> > > (optional) extension. I don't think that scales.
> >
> > No argument here. I really don't want to have tempo in pip-requires or the
> > Ubuntu package if I'm not going to use it. Since it's there (or going to be
> > there?), shouldn't we just show the imports up front?
> >
> > My main argument though is that imports should always be up top in my
> opinion,
> > even if you have to do something like:
> >
> > try:
> > import tempo
> > except ImportError:
> > LOG.warn("Tempo not installed, backup extension won't work.")
>
> Agree fully.
> -jay

Yeah, I personally can go either way on that; but since HACKING has an opinion on this, you guys are right, this should go up at the top. I'll make that change.

35 + import tempo2.client

Is it tempo.client or tempo2.client?

34 +try:
35 + import tempo2.client
36 + tempo_available = True
37 +except ImportError:
38 + tempo_available = False
39 + LOG.warn(_("Unable to import tempo, backup schedules API extension will"
40 + " not be available."))

137 + if tempo_available:
138 + self.tempo_client = tempo.client.TempoClient(FLAGS.tempo_url)

Looks good, however if tempo isn't installed, tempo_available = False, and the extension is loaded anyway the error most likely will occur as a AttributeError indicating that self.tempo_client isn't defined.

Might I suggest:

137 + if tempo_available:
138 + self.tempo_client = tempo.client.TempoClient(FLAGS.tempo_url)
139 + else:
140 + raise ImportError("tempo.client unable to be imported.")

Or alternatively:

try:
    import tempo.client as tempo_client
except ImportError:
    tempo_client = None

...

if tempo_client:
    self.tempo_client = tempo_client.TempoClient(FLAGS.tempo_url)
else:
    raise ImportError("tempo.client unable to be imported.")

25 +from nova import compute
134 + self.compute_api = compute.API()

I think these can be removed.

Looks great otherwise, sure we can get this in soon.

Personally, I don't think that code should be added to Nova that depends on half-baked projects. And half-baked projects that have a bunch of new dependencies, like "requiem" and "flask". If tempo is going to be an OpenStack project, it already is going down the path of wildly divergent code paths for handling stuff that multiple OpenStack projects already handle in a fairly common fashion. Just sayin -- heading further and further away from common coding style and policies.

blamar: Good points; I'll fix those up shortly.

jaypipes:

> I don't think that code should be added to Nova that depends on half-baked projects.

Agree that Nova *core* should only depend on stable/mature projects. For extensions, I think we can relax those requirements and allow much more flexibility and freedom to experiment.

The problem as it stand now, though, is that Nova doesn't really have a notion of a completely optional extension. As Nova goes to production and we start integrating it with more and more custom systems, I think we're going to need to really think hard about how to do Nova extensions right, from a packaging, unit-test, and virt/compute/api layer perspective.

Perhaps we can get this discussion started at the Essex design summit?

> The problem as it stand now, though, is that Nova doesn't really have a notion
> of a completely optional extension. As Nova goes to production and we start
> integrating it with more and more custom systems, I think we're going to need
> to really think hard about how to do Nova extensions right, from a packaging,
> unit-test, and virt/compute/api layer perspective.
>
> Perhaps we can get this discussion started at the Essex design summit?

Yes, definitely :) You're absolutely right that Nova doesn't currently support completely optional extensions. That's the root of the problems.

-jay

Code looks good

Seems like something that might make people happy is if we load tempo the way we load a lot of the drivers, which is by flags. Then any service that wants to do something cron-like would have to provide the same API. The default/base driver effectively would do nothing, and then we don't need to explicitly reference Tempo in any way in the project

Thanks for the comments all-- I've updated the patch to reflect the commen

Matt: Yeah, the FLAGS+plugin model has served us really well, don't see a reason why it couldn't work here as well (eventually). As it stands now, though, OS API extensions have their own architecture, and it's probably better--for the time being at least--to be consistent.

This is definitely something we should discuss at the Essex summit: how to rework our extension architecture so it can support truly optional extensions at all layers of the code.