Enable tests to have host-specific parameters

Enable tests to have host-specific parameters. Remove a bunch of todos where we disabled tests because they would not work everywhere.  Now we selectively enable and set constraints on BVTs.

[seano, 2010-04-27 19:27:25]

lgtm

[ericli, 2010-04-27 20:08:51]

Please take a look at comments from Sameer.

http://codereview.chromium.org/1712007

I think he want it kept in BVT.

On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.org/1780007/show
>



-- 
Eric Li
李咏竹
Google Kirkland

[kmixter1, 2010-04-28 16:30:23]

Agreed - I want it kept in the BVT as well, longer term.  The problem
is that right now, the reference boards we're going to be installing
in the farm will hang if you run this test without a hardware
modification.  So for now I think we want to remove it and then add it
back, as in the TODO.

On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote:
> Please take a look at comments from Sameer.
> http://codereview.chromium.org/1712007
> I think he want it kept in BVT.
>
> On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote:
>>
>> lgtm
>>
>> http://codereview.chromium.org/1780007/show
>
>
>
> --
> Eric Li
> 李咏竹
> Google Kirkland
>
>
>

[ericli, 2010-04-28 16:37:58]

Ken,

If you had reached agreement with Sameer, LTGM.

Sameer,

Could you also LGTM this?

On Wed, Apr 28, 2010 at 9:29 AM, Ken Mixter <kmixter@chromium.org> wrote:

> Agreed - I want it kept in the BVT as well, longer term.  The problem
> is that right now, the reference boards we're going to be installing
> in the farm will hang if you run this test without a hardware
> modification.  So for now I think we want to remove it and then add it
> back, as in the TODO.
>
> On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote:
> > Please take a look at comments from Sameer.
> > http://codereview.chromium.org/1712007
> > I think he want it kept in BVT.
> >
> > On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote:
> >>
> >> lgtm
> >>
> >> http://codereview.chromium.org/1780007/show
> >
> >
> >
> > --
> > Eric Li
> > 李咏竹
> > Google Kirkland
> >
> >
> >
>



-- 
Eric Li
李咏竹
Google Kirkland

[Sameer Nanda, 2010-04-28 19:44:56]

On 2010/04/28 16:37:58, ericli wrote:
> Ken,
> 
> If you had reached agreement with Sameer, LTGM.
> 
> Sameer,
> 
> Could you also LGTM this?

Had a chat with Ken and suggested using autotest labels to exclude systems where
this test should not be run on.

> 
> On Wed, Apr 28, 2010 at 9:29 AM, Ken Mixter <mailto:kmixter@chromium.org>
wrote:
> 
> > Agreed - I want it kept in the BVT as well, longer term.  The problem
> > is that right now, the reference boards we're going to be installing
> > in the farm will hang if you run this test without a hardware
> > modification.  So for now I think we want to remove it and then add it
> > back, as in the TODO.
> >
> > On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <mailto:ericli@chromium.org>
wrote:
> > > Please take a look at comments from Sameer.
> > > http://codereview.chromium.org/1712007
> > > I think he want it kept in BVT.
> > >
> > > On Tue, Apr 27, 2010 at 12:27 PM, <mailto:seano@chromium.org> wrote:
> > >>
> > >> lgtm
> > >>
> > >> http://codereview.chromium.org/1780007/show
> > >
> > >
> > >
> > > --
> > > Eric Li
> > > 李咏竹
> > > Google Kirkland
> > >
> > >
> > >
> >
> 
> 
> 
> -- 
> Eric Li
> 李咏竹
> Google Kirkland
>

[kmixter1, 2010-04-29 01:52:52]

Complete redid this.  PTAL.

[ericli, 2010-04-29 03:25:09]

Please help me to create all host attributes labels in the server. I will take
care of assigning labels to hosts.

Other suggestion:

rename site_host_attributes_config.py to something like
private_host_attributes_config.py or similar.

Since the site_* name convention has some different suggestion here.

I am also suggesting to rename site_* into cros_* in general, but that is not in
the scope of this CL.

http://codereview.chromium.org/1780007/diff/10001/11002
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/10001/11002#newcode39
server/site_host_attributes.py:39: hardcoded_host_attributes = {}
Could you try something like this?

hardcoded_hard_attr = utils.import_site_class(__file__,   
"autotest_lib.server.site_host_attributes_config", "hardcoded_hard_attr", {})

This logic is everywhere inside autotest code, kind of standard logic.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode83
server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value
so if an attribute has no comma ',' in it, it will end up as
self._attributes['foo']=[]
This is not obvious, wonder if you could document it.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode100
server/site_host_attributes.py:100: return self._attributes[attribute]
return self._attributes.get(attribute)

You dont need the try catch block here. Same effect.

[petkov, 2010-04-29 19:39:06]

It's nice to see that support for this is coming in place... A few comments,
mostly regarding documentation.

http://codereview.chromium.org/1780007/diff/10001/11002
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/10001/11002#newcode16
server/site_host_attributes.py:16: #   host_attributes.get('drive_kind') =>
'ssd,1'
Where does the ",1" come from?

http://codereview.chromium.org/1780007/diff/10001/11002#newcode83
server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value
Do you need special processing for has_* attributes here so that the "has"
method below works OK?

In general, this has_ thing is a bit confusing. Maybe always strip the has_ from
the attribute name, if possible. Then calling the "has" method would simply mean
check the attribute is set to true.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode86
server/site_host_attributes.py:86: def set_defaults(self, default_attributes):
This method needs better documentation -- it seems that it simply updates the
attributes with "default_attributes" for attributes that are not already
present. Who's expected to invoke this method?

http://codereview.chromium.org/1780007/diff/10001/11002#newcode107
server/site_host_attributes.py:107: Retrieve an attribute that starts with
"has_" and ignore
Change doc string (or the code) -- it seems that the code below takes into
account that attribute value.

[kmixter1, 2010-04-30 01:42:33]

PTAL

http://codereview.chromium.org/1780007/diff/10001/11002
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/10001/11002#newcode16
server/site_host_attributes.py:16: #   host_attributes.get('drive_kind') =>
'ssd,1'
On 2010/04/29 19:39:06, petkov wrote:
> Where does the ",1" come from?
> 

Yeah these comments were too sparse and inconsistent.  Should be better now.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode39
server/site_host_attributes.py:39: hardcoded_host_attributes = {}
On 2010/04/29 03:25:09, ericli wrote:
> Could you try something like this?
> 
> hardcoded_hard_attr = utils.import_site_class(__file__,   
> "autotest_lib.server.site_host_attributes_config", "hardcoded_hard_attr", {})
> 
> This logic is everywhere inside autotest code, kind of standard logic.
> 

Done.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode83
server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value
On 2010/04/29 19:39:06, petkov wrote:
> Do you need special processing for has_* attributes here so that the "has"
> method below works OK?
> 
> In general, this has_ thing is a bit confusing. Maybe always strip the has_
from
> the attribute name, if possible. Then calling the "has" method would simply
mean
> check the attribute is set to true.

Rather than document and explain away the confusion, I've tried to simplify
things.  There are no implicit 'has_' prefixes, and the has function was renamed
to "bool" returns a boolean.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode83
server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value
On 2010/04/29 03:25:09, ericli wrote:
> so if an attribute has no comma ',' in it, it will end up as
> self._attributes['foo']=[]
> This is not obvious, wonder if you could document it.
> 

It will map to ','.join([]) which is ''.  I've documented that.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode86
server/site_host_attributes.py:86: def set_defaults(self, default_attributes):
On 2010/04/29 19:39:06, petkov wrote:
> This method needs better documentation -- it seems that it simply updates the
> attributes with "default_attributes" for attributes that are not already
> present. Who's expected to invoke this method?
> 

No one, it was dead code and is now removed.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode100
server/site_host_attributes.py:100: return self._attributes[attribute]
On 2010/04/29 03:25:09, ericli wrote:
> return self._attributes.get(attribute)
> 
> You dont need the try catch block here. Same effect.

Thanks. Done.

http://codereview.chromium.org/1780007/diff/10001/11002#newcode107
server/site_host_attributes.py:107: Retrieve an attribute that starts with
"has_" and ignore
On 2010/04/29 19:39:06, petkov wrote:
> Change doc string (or the code) -- it seems that the code below takes into
> account that attribute value.
> 

Done.

[petkov, 2010-04-30 15:27:19]

Simpler and better... One concern about the "bool" naming and a nit about
sorting, LGTM otherwise.

http://codereview.chromium.org/1780007/diff/18001/3003
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/18001/3003#newcode68
server/site_host_attributes.py:68: 'has_resume_bug,False'
Keep this sorted?

http://codereview.chromium.org/1780007/diff/18001/3003#newcode89
server/site_host_attributes.py:89: for key in self._attributes:
You could do

for key, val in self._attributes.items():

Then you don't need the extra lookup in the logging below.

http://codereview.chromium.org/1780007/diff/18001/3003#newcode90
server/site_host_attributes.py:90: logging.info('Host attribute: %s => %s' %
You could use , rather than %

http://codereview.chromium.org/1780007/diff/18001/3003#newcode108
server/site_host_attributes.py:108: def bool(self, attribute):
bool() is a built-in function and has a slightly different meaning -- it returns
True/False for a wider range of input strings.

Maybe rename to flag()?

[kmixter1, 2010-04-30 21:04:51]

PTAL

I'd like to submit this today if possible.  Eric - we can add labels, but
without labels this will only have the effect of running a few tests that should
not be run on eeePC 1008's.

[kmixter1, 2010-04-30 21:04:59]

PTAL

I'd like to submit this today if possible.  Eric - we can add labels, but
without labels this will only have the effect of running a few tests that should
not be run.

[ericli, 2010-04-30 21:14:00]

LGTM.

On 2010/04/30 21:04:59, kmixter1 wrote:
> PTAL
> 
> I'd like to submit this today if possible.  Eric - we can add labels, but
> without labels this will only have the effect of running a few tests that
should
> not be run.

[petkov, 2010-04-30 21:42:36]

A couple of suggestions, LGTM otherwise.

http://codereview.chromium.org/1780007/diff/20001/21002
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/20001/21002#newcode15
server/site_host_attributes.py:15: #    'illegal_string,1,2,3'
I'd remove this one from the example -- or add a comment that this is not valid.

http://codereview.chromium.org/1780007/diff/20001/21002#newcode87
server/site_host_attributes.py:87: logging.info('Host attribute: %s => %s' %
I'd do

logging.info('Host attribute: %s => %s', key, value)

http://codereview.chromium.org/1780007/diff/20001/21002#newcode104
server/site_host_attributes.py:104: self._attributes[splitnames[0]] = value
You could get rid of _attributes and just use

setattr(self, splitnames[0], value)

Then you don't need __getattr__

[kmixter1, 2010-04-30 23:33:15]

pushing.

http://codereview.chromium.org/1780007/diff/20001/21002
File server/site_host_attributes.py (right):

http://codereview.chromium.org/1780007/diff/20001/21002#newcode15
server/site_host_attributes.py:15: #    'illegal_string,1,2,3'
On 2010/04/30 21:42:36, petkov wrote:
> I'd remove this one from the example -- or add a comment that this is not
valid.
> 

Done.

http://codereview.chromium.org/1780007/diff/20001/21002#newcode87
server/site_host_attributes.py:87: logging.info('Host attribute: %s => %s' %
On 2010/04/30 21:42:36, petkov wrote:
> I'd do
> 
> logging.info('Host attribute: %s => %s', key, value)
> 

Done.

http://codereview.chromium.org/1780007/diff/20001/21002#newcode104
server/site_host_attributes.py:104: self._attributes[splitnames[0]] = value
On 2010/04/30 21:42:36, petkov wrote:
> You could get rid of _attributes and just use
> 
> setattr(self, splitnames[0], value)
> 
> Then you don't need __getattr__
> 

Done.