site_job.py extension used in virt testing

site_job.py extension used in virt testing

[lmr, 2011-06-21 18:40:40]

Hi Cleber, I read your patchset and I have some minor comments to make.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README
File contrib/virt/README (right):

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README#newcode58
contrib/virt/README:58: -f "<autotest_root>/contrib/virt/control.template" -T
--timestamp \
Looks like the path to control template must come after the -T flag.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README#newcode64
contrib/virt/README:64: -f "<autotest_root>/contrib/virt/control.template" -T
--timestamp \
Same here

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py
File contrib/virt/site_job.py (right):

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode6
contrib/virt/site_job.py:6: from autotest_lib.client.common_lib import
logging_config
I've noticed there's no formal call to logging_config, so the logging format for
cli messages does not follow the rest of autotest entry points logging format.
Of course this is nipticking, but it'd be nice to get things consistent across
all entry points.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode67
contrib/virt/site_job.py:67: def __process_options(self):
Is there a real need to preclude the 'private' methods with double underscore? I
remember we had a discussion a while ago, where we came up to the conclusion
that's best to avoid double underscores, and use them only when needed (ie, we
need the python namespace mangling to avoid calling the function by accident or
something).

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode95
contrib/virt/site_job.py:95: '''
You have mentioned you have concerns about how to make this code more
modular/reusable among other site extensions that want to use some of this
functionality. I have thought about this and the answer is fairly simple: We can
encapsulate the actual functionality as additional API in autotest libraries,
and then make the site extensions to just call them. Example: this method can
just

return utils.check_koji_packages(arg1, arg2...argn)

Same goes for any other interesting features the extension has.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode183
contrib/virt/site_job.py:183: for cls in [getattr(job, n) for n in dir(job) if
not n.startswith("_")]:
Ok, this is the boiler plate code needed for the site extensions...

[cleber, 2011-06-21 21:07:56]

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README
File contrib/virt/README (right):

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README#newcode58
contrib/virt/README:58: -f "<autotest_root>/contrib/virt/control.template" -T
--timestamp \
On 2011/06/21 18:40:40, lmr wrote:
> Looks like the path to control template must come after the -T flag.

Actually, the '-T' flag simply means that the path passed to '-f' is not a plain
control file, but instead a template. The code was made much simpler that way,
because we don't have to cope with situations such as passing both '-f <path>'
and '-T <path>', or disabling '-f' altogether when using '-T'.

So, in this case, the '-T' flag can be set anywhere.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/README#newcode64
contrib/virt/README:64: -f "<autotest_root>/contrib/virt/control.template" -T
--timestamp \
On 2011/06/21 18:40:40, lmr wrote:
> Same here

Same here also :)

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py
File contrib/virt/site_job.py (right):

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode6
contrib/virt/site_job.py:6: from autotest_lib.client.common_lib import
logging_config
On 2011/06/21 18:40:40, lmr wrote:
> I've noticed there's no formal call to logging_config, so the logging format
for
> cli messages does not follow the rest of autotest entry points logging format.
> Of course this is nipticking, but it'd be nice to get things consistent across
> all entry points.

You're absolutely right on that. On my way to fix it.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode67
contrib/virt/site_job.py:67: def __process_options(self):
On 2011/06/21 18:40:40, lmr wrote:
> Is there a real need to preclude the 'private' methods with double underscore?
I
> remember we had a discussion a while ago, where we came up to the conclusion
> that's best to avoid double underscores, and use them only when needed (ie, we
> need the python namespace mangling to avoid calling the function by accident
or
> something).

It's just a matter of my old and bad habits showing up here. Consider it fixed.

http://codereview.appspot.com/4547087/diff/1/contrib/virt/site_job.py#newcode95
contrib/virt/site_job.py:95: '''
On 2011/06/21 18:40:40, lmr wrote:
> You have mentioned you have concerns about how to make this code more
> modular/reusable among other site extensions that want to use some of this
> functionality. I have thought about this and the answer is fairly simple: We
can
> encapsulate the actual functionality as additional API in autotest libraries,
> and then make the site extensions to just call them. Example: this method can
> just
> 
> return utils.check_koji_packages(arg1, arg2...argn)
> 
> Same goes for any other interesting features the extension has.

Yeah, that would probably do it some of what I meant. I was really worried about
not being able to use multiple site extension files at once, say one that adds
virt features and other that adds performance measurement features.

That would really require the site extensions to work more like plugins as we
known them in browsers: you would never feel that it's right to disable your
browser's java plugin to enable flash, right? They should just add to each
other's features.

In this specific case, I believe that we can indeed move that into the
framework, but the point of some extensions might be exactly the need to stay
away from the "core" API.

[lmr, 2011-06-21 22:22:59]

> On 2011/06/21 18:40:40, lmr wrote:
> > You have mentioned you have concerns about how to make this code more
> > modular/reusable among other site extensions that want to use some of this
> > functionality. I have thought about this and the answer is fairly simple: We
> can
> > encapsulate the actual functionality as additional API in autotest
libraries,
> > and then make the site extensions to just call them. Example: this method
can
> > just
> > 
> > return utils.check_koji_packages(arg1, arg2...argn)
> > 
> > Same goes for any other interesting features the extension has.
> 
> Yeah, that would probably do it some of what I meant. I was really worried
about
> not being able to use multiple site extension files at once, say one that adds
> virt features and other that adds performance measurement features.

I see what you mean. we can certainly think about doing that. I have created
this task on the autotest trac:

http://autotest.kernel.org/ticket/51

To track this work item. Meanwhile, let's keep it simple and if we want to reuse
components, just encapsulate them. Not quite what you wanted, but close.

> That would really require the site extensions to work more like plugins as we
> known them in browsers: you would never feel that it's right to disable your
> browser's java plugin to enable flash, right? They should just add to each
> other's features.
> 
> In this specific case, I believe that we can indeed move that into the
> framework, but the point of some extensions might be exactly the need to stay
> away from the "core" API.

Yep. Remember the site extension itself is an importable lib, so we can keep the
extensions code away from the core API.