968905 - Autophone - parametrize control constants via config files

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, enhancement)

Description

[Bob Clary [:bc] (inactive)]

In the discussion for bug 852264, we considered using a config option to control the device manager reboot settling time. This bug is about adding the ability to control all of the fixed constants that are currently being used to control Autophone's behavior as well as generalize the support for multiple repos so that specific devices can not only be assigned to specific tests, but that they can also be assigned to specific repos.

1. Add a new config file autophone.ini for autophone as a whole. Is there a benefit to putting these into sections or is it ok to just lump them together in a single section?

autophone.ini

build_cache_size = 20
build_cache_age = 7 (days)
device_manager_retry_limit = 8
device_manager_settling_time = 60 (seconds)
phoneworker_retry_limit = 2
phoneworker_retry_wait = 15 (seconds)
phoneworker_max_reboots = 3
phoneworker_ping_interval = 15 (minutes)
phoneworker_command_queue_timeout = 10 (seconds)
phoneworker_crash_window = 30 seconds
phoneworker_crash_window_limit = 5

2. For test manifests, remove the devices option and assume that if the option is not 'config', it is a deviceid = repo-list. This will allow us to partition phones across tests and repos. For example, for slow phones we might want 1 phone to handle mozilla-central and fx-team while the others handle the mozilla-inbound repo.

manifest.ini

[s1s2test.py]
config = configs/s1s2_settings.ini
00_23_76_96_cc_6f_nexus-one = mozilla-central fx-team
00_23_76_b1_92_39_nexus-one = mozilla-inbound
78_d6_f0_cf_8d_11_nexus-s  = mozilla-central fx-team mozilla-inbound

3. Add more options in s1s2_settings.ini

standard_error_stop_limit = 0 (stop if below)
standard_error_reject_limit = 5 (reject if above)
throbber_max_wait = 90
throbber_wait_time = 3

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-968905.patch

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: follow up to use test_path in autophone.ini.example

I misspelled test_path in the autophone.ini.example.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 8374355 [details] [diff] [review]
bug-968905.patch

Review of attachment 8374355 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few minor nits & suggestions.

General comment, thinking out loud: do you think it would be useful to define variables for all the setting names, e.g. CLEAR_CACHE = 'clear_cache'?  It would help avoid typos, the scourge of heavy dictionary usage.

::: autophone.py
@@ +30,4 @@
>  
>  from mailer import Mailer
>  from multiprocessinghandlers import MultiprocessingStreamHandler, MultiprocessingTimedRotatingFileHandler
> +from worker import PhoneWorker, Crashes

Might as well sort alphabetically.

@@ +71,4 @@
>          self.logger = logging.getLogger('autophone')
>          self.console_logger = logging.getLogger('autophone.console')
> +        self.options = options
> +        if not ('ipaddr' in options and options['ipaddr']):

If I'm reading this correctly, you can just do something like "if not options.get('ipaddr', None):" for simplicity.

@@ +122,5 @@
>              self.pulsemonitor = None
>  
>          self.restart_workers = {}
>  
> +        self.logger.debug('autophone_options: %s' % options)

This might be more useful before you start to use all the options, no?

@@ +295,5 @@
> +        self.logger.info('Creating worker for %s: %s, %s.' % (phoneid, phone_cfg, user_cfg))
> +        tests = []
> +        for test_class, config_file, enable_unittests, test_devices_repos in self._tests:
> +            skip_test = True
> +            if len(test_devices_repos) == 0:

Why not "if not test_devices_repos"?

@@ +308,5 @@
> +                                        user_cfg=user_cfg,
> +                                        config_file=config_file,
> +                                        enable_unittests=enable_unittests,
> +                                        test_devices_repos=test_devices_repos))
> +        if len(tests) == 0:

"if not tests"?

@@ +441,2 @@
>                      else:
> +                        enable_unittests = False

I believe you could replace those 4 lines with "enable_unittests = bool(t.get('unittests', False))"

@@ +441,5 @@
>                      else:
> +                        enable_unittests = False
> +
> +                    devices = [device for device in t if device not in
> +                               'name,here,manifest,path,config,relpath,unittest']

Thinking this should be a real list of strings instead of a comma-separated string.

@@ +487,5 @@
>              p.stop()
>          self.server_thread.join()
>  
> +def load_autophone_options(cmd_options):
> +

Ditch the blank line.

@@ +542,5 @@
> +                    value = cfg.get('settings', setting_name)
> +                elif setting_type == int:
> +                    value = cfg.getint('settings', setting_name)
> +                elif setting_type == bool:
> +                    value = cfg.getboolean('settings', setting_name)

Maybe you could just have a single mapping from type to function? E.g. something like {'int': cfg.getint, 'str': cfg.get, 'bool': cfg.getboolean}?  Or even just abbreviating the type names to 'i', 's', and 'b', to avoid typos?  Either way, it would eliminate the redundancy between cmd_option_names and ini_option_names.

@@ +565,5 @@
> +                pass
> +
> +    for setting_name in cmd_option_names:
> +        if setting_name not in options:
> +            options[setting_name] = getattr(cmd_options, setting_name)

Wait, is this not making config file options override command-line options, in that we're only setting the options from cmd_options if they weren't previously set?

::: worker.py
@@ +31,2 @@
>  
> +    def __init__(self, crash_window=CRASH_WINDOW, crash_limit=CRASH_LIMIT):

I admit I had no idea you could use class variables in such a way, though it totally makes sense.

@@ +408,5 @@
>              if self.has_error():
>                  break
> +            try:
> +                repos = t.test_devices_repos[self.phone_cfg['phoneid']]
> +                if len(repos) and repo not in repos:

"if len(repos)" => "if repos".

@@ +477,5 @@
> +        skip_build = True
> +        enable_unittests = False
> +        for test in self.tests:
> +            test_devices_repos = test.test_devices_repos
> +            if len(test_devices_repos) == 0:

if not test_devices_repos

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #3)

> General comment, thinking out loud: do you think it would be useful to
> define variables for all the setting names, e.g. CLEAR_CACHE =
> 'clear_cache'?  It would help avoid typos, the scourge of heavy dictionary
> usage.

I can see your point. But man, that would be a pita. It is kind of nice to be able to pass things around in user_cfg. I'd hate to have to go back to those incredibly long argument lists. Using explicit variables would catch typos much easier for sure.

> 
> ::: autophone.py

> @@ +71,4 @@
> >          self.logger = logging.getLogger('autophone')
> >          self.console_logger = logging.getLogger('autophone.console')
> > +        self.options = options
> > +        if not ('ipaddr' in options and options['ipaddr']):
> 
> If I'm reading this correctly, you can just do something like "if not
> options.get('ipaddr', None):" for simplicity.
> 

Cool.

> @@ +122,5 @@
> >              self.pulsemonitor = None
> >  
> >          self.restart_workers = {}
> >  
> > +        self.logger.debug('autophone_options: %s' % options)
> 
> This might be more useful before you start to use all the options, no?
> 

Not sure. This does give them to me as they will be when the workers are running. We could put it several places: at the beginning of the constructor or even at the end of load_autophone_options. Where would you suggest?

> @@ +295,5 @@
> > +        self.logger.info('Creating worker for %s: %s, %s.' % (phoneid, phone_cfg, user_cfg))
> > +        tests = []
> > +        for test_class, config_file, enable_unittests, test_devices_repos in self._tests:
> > +            skip_test = True
> > +            if len(test_devices_repos) == 0:
> 
> Why not "if not test_devices_repos"?
> 

Ok.

> @@ +308,5 @@
> > +                                        user_cfg=user_cfg,
> > +                                        config_file=config_file,
> > +                                        enable_unittests=enable_unittests,
> > +                                        test_devices_repos=test_devices_repos))
> > +        if len(tests) == 0:
> 
> "if not tests"?

Ok.

> 
> @@ +441,2 @@
> >                      else:
> > +                        enable_unittests = False
> 
> I believe you could replace those 4 lines with "enable_unittests =
> bool(t.get('unittests', False))"
> 

I really need to get in the habit.

> @@ +441,5 @@
> >                      else:
> > +                        enable_unittests = False
> > +
> > +                    devices = [device for device in t if device not in
> > +                               'name,here,manifest,path,config,relpath,unittest']
> 

> 
> @@ +542,5 @@
> > +                    value = cfg.get('settings', setting_name)
> > +                elif setting_type == int:
> > +                    value = cfg.getint('settings', setting_name)
> > +                elif setting_type == bool:
> > +                    value = cfg.getboolean('settings', setting_name)
> 
> Maybe you could just have a single mapping from type to function? E.g.
> something like {'int': cfg.getint, 'str': cfg.get, 'bool': cfg.getboolean}? 
> Or even just abbreviating the type names to 'i', 's', and 'b', to avoid
> typos?  Either way, it would eliminate the redundancy between
> cmd_option_names and ini_option_names.
> 

Interesting. I'll try it out.

> @@ +565,5 @@
> > +                pass
> > +
> > +    for setting_name in cmd_option_names:
> > +        if setting_name not in options:
> > +            options[setting_name] = getattr(cmd_options, setting_name)
> 
> Wait, is this not making config file options override command-line options,
> in that we're only setting the options from cmd_options if they weren't
> previously set?
> 

Yeah. I had meant to have the command line options override the settings from the ini file, but that actually won't work since we have default values in cmd_options which would always override the values set in the ini file.

The way this actually works is if something is not set in the ini file, the value from the cmd_options is copied over. I'll remove the comment about the command line overriding the ini values.


> ::: worker.py
> @@ +31,2 @@
> >  
> > +    def __init__(self, crash_window=CRASH_WINDOW, crash_limit=CRASH_LIMIT):
> 
> I admit I had no idea you could use class variables in such a way, though it
> totally makes sense.
> 
> @@ +408,5 @@
> >              if self.has_error():
> >                  break
> > +            try:
> > +                repos = t.test_devices_repos[self.phone_cfg['phoneid']]
> > +                if len(repos) and repo not in repos:
> 
> "if len(repos)" => "if repos".
> 
> @@ +477,5 @@
> > +        skip_build = True
> > +        enable_unittests = False
> > +        for test in self.tests:
> > +            test_devices_repos = test.test_devices_repos
> > +            if len(test_devices_repos) == 0:
> 
> if not test_devices_repos

ok.

I'll test my changes to this patch and if it looks ok, I'll attach here and ask for feedback or review.

Thanks!

Comment 5

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #4)
> (In reply to Mark Côté ( :mcote ) from comment #3)
> 
> > General comment, thinking out loud: do you think it would be useful to
> > define variables for all the setting names, e.g. CLEAR_CACHE =
> > 'clear_cache'?  It would help avoid typos, the scourge of heavy dictionary
> > usage.
> 
> I can see your point. But man, that would be a pita. It is kind of nice to
> be able to pass things around in user_cfg. I'd hate to have to go back to
> those incredibly long argument lists. Using explicit variables would catch
> typos much easier for sure.

Hm, I think you might have misunderstood me. I just meant using variables for the key names instead of raw strings.  So you'd have user_cfg[CLEAR_CACHE] instead of user_cfg['clear_cache'].  That way, if you mistype, e.g. "user_cfg[CLEA_CACHE] = True" you'll get a clear error instead of it silently writing the value to the wrong, unused key.

> > @@ +122,5 @@
> > >              self.pulsemonitor = None
> > >  
> > >          self.restart_workers = {}
> > >  
> > > +        self.logger.debug('autophone_options: %s' % options)
> > 
> > This might be more useful before you start to use all the options, no?
> > 
> 
> Not sure. This does give them to me as they will be when the workers are
> running. We could put it several places: at the beginning of the constructor
> or even at the end of load_autophone_options. Where would you suggest?

Ah I got a bit confused because you're logging options and not self.options; those are indeed the same object, but I didn't realize it at first and thought you were only interested in what was passed into the constructor.  Maybe just change to self.options for clarity.  I leave it up to you if you want to log its initial value as well; only put it in if you'll find it useful.

> > @@ +565,5 @@
> > > +                pass
> > > +
> > > +    for setting_name in cmd_option_names:
> > > +        if setting_name not in options:
> > > +            options[setting_name] = getattr(cmd_options, setting_name)
> > 
> > Wait, is this not making config file options override command-line options,
> > in that we're only setting the options from cmd_options if they weren't
> > previously set?
> > 
> 
> Yeah. I had meant to have the command line options override the settings
> from the ini file, but that actually won't work since we have default values
> in cmd_options which would always override the values set in the ini file.
> 
> The way this actually works is if something is not set in the ini file, the
> value from the cmd_options is copied over. I'll remove the comment about the
> command line overriding the ini values.

Ah that's unfortunate, since command-line options usually override config files.  But yeah that's tricky.  I guess just make sure it's documented (and in the usage for --config).

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #5)

> 
> Hm, I think you might have misunderstood me. I just meant using variables
> for the key names instead of raw strings.  So you'd have
> user_cfg[CLEAR_CACHE] instead of user_cfg['clear_cache'].  That way, if you
> mistype, e.g. "user_cfg[CLEA_CACHE] = True" you'll get a clear error instead
> of it silently writing the value to the wrong, unused key.

Ah, I get it now.

> Ah that's unfortunate, since command-line options usually override config
> files.  But yeah that's tricky.  I guess just make sure it's documented (and
> in the usage for --config).

I could keep track of the default values and not overwrite an ini setting with a default value. That would probably be worth the effort.

Comment 7

[Mark Côté [:mcote]]

No, I might have specified a default value explicitly in order to override a config file. The correct solution involves using a better module than optparse, but your solution, properly documented, is fine for now.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: bug-968905-configs-v2.patch

I added options.py to hold the options related stuff and did an from options import * to include the variables and such. Hope that is ok with you.

This bundles a fix to a setting time setting in PhoneTest which had been left out and also a fix to  analyze_logcat in s1s2test.py which prevents bogus values for the start and throbber measurements from being returned.

Comment 9

[Bob Clary [:bc] (inactive)]

crap, i put enable_unittests = bool(t.get('unittests', False)) in but left the old code. consider that removed.

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8376300 [details] [diff] [review]
bug-968905-configs-v2.patch

Review of attachment 8376300 [details] [diff] [review]:
-----------------------------------------------------------------

::: USAGE.md
@@ +54,5 @@
> +                       enable_unittests
> +                       cache_dir
> +                       override_build_dir
> +                       repos (space delimited list of repo names, mozilla-central fx-team mozilla-inbound)
> +                       buildtypes = space delimited list of opt, debug

This line and the one above are in a different format.

::: autophone.py
@@ +335,5 @@
> +            PHONE_CRASH_WINDOW: self.options[PHONE_CRASH_WINDOW],
> +            PHONE_CRASH_LIMIT: self.options[PHONE_CRASH_LIMIT],
> +            DEVICEMANAGER_RETRY_LIMIT: self.options[DEVICEMANAGER_RETRY_LIMIT],
> +            DEVICEMANAGER_SETTLING_TIME: self.options[DEVICEMANAGER_SETTLING_TIME]
> +        }

Looking at this again, seems like it would be saner to just iterate through a list of options and set them on user_cfg...would drastically reduce the redundancy here.

::: options.py
@@ +72,5 @@
> +# map option type names to the appropriate getter
> +OPTION_TYPE_GETTERS = {
> +    'int': ConfigParser.RawConfigParser.getint,
> +    'str': ConfigParser.RawConfigParser.get,
> +    'bool': ConfigParser.RawConfigParser.getboolean}

Put } on its own line.

Also heh now the type strings look totally redundant; you could just be assigning config parser functions directly to the option names, maybe with an alias (e.g. getint = ConfigParser.RawConfigParser.getint).  Or function names as strings (e.g. BUILD_CACHE_SIZE = 'getint') that you then call with getattr().  Something to avoid needless intermediaries.

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #10)
> Comment on attachment 8376300 [details] [diff] [review]
> bug-968905-configs-v2.patch
> 
> Review of attachment 8376300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: USAGE.md
> @@ +54,5 @@
> > +                       enable_unittests
> > +                       cache_dir
> > +                       override_build_dir
> > +                       repos (space delimited list of repo names, mozilla-central fx-team mozilla-inbound)
> > +                       buildtypes = space delimited list of opt, debug
> 
> This line and the one above are in a different format.
> 
> ::: autophone.py
> @@ +335,5 @@
> > +            PHONE_CRASH_WINDOW: self.options[PHONE_CRASH_WINDOW],
> > +            PHONE_CRASH_LIMIT: self.options[PHONE_CRASH_LIMIT],
> > +            DEVICEMANAGER_RETRY_LIMIT: self.options[DEVICEMANAGER_RETRY_LIMIT],
> > +            DEVICEMANAGER_SETTLING_TIME: self.options[DEVICEMANAGER_SETTLING_TIME]
> > +        }
> 
> Looking at this again, seems like it would be saner to just iterate through
> a list of options and set them on user_cfg...would drastically reduce the
> redundancy here.
> 

Yeah, changing the user_cfg keys to match the ini setting names makes that much easier now.

> ::: options.py
> @@ +72,5 @@
> > +# map option type names to the appropriate getter
> > +OPTION_TYPE_GETTERS = {
> > +    'int': ConfigParser.RawConfigParser.getint,
> > +    'str': ConfigParser.RawConfigParser.get,
> > +    'bool': ConfigParser.RawConfigParser.getboolean}
> 
> Put } on its own line.
> 
> Also heh now the type strings look totally redundant; you could just be
> assigning config parser functions directly to the option names, maybe with
> an alias (e.g. getint = ConfigParser.RawConfigParser.getint).  Or function
> names as strings (e.g. BUILD_CACHE_SIZE = 'getint') that you then call with
> getattr().  Something to avoid needless intermediaries.

right.

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: bug-968905-configs-v3.patch

https://github.com/mozilla/autophone/commit/6d5755492325bcb9b3da24c3d9e51ef2ad1a28b1