On Tue, Nov 17, 2009 at 9:40 AM, Lucas Meneghel Rodrigues
<lmr@xxxxxxxxxx> wrote:
> In order to make it possible the autotest client to use the
> global_config.ini configuration files:
>
> * Modified global_config.py to support verifying if the
> configuration file is under autotest's root directory,
> or the client directory
> * Extended the autotest run method to copy over
> the configuration files to the client.
>
> Notes for revisors (changes from previous patchset and other stuff):
>
> * Now only the config values under [CLIENT] will be
> effectively present on the config file that will
> be copied to the client. That config file is generated
> using new methods added to global_config.
> * No shadow_config or database passwords will ever be
> copied to the client. Yay! :)
> * common_lib.host_protections was changed accordingly.
> * The new config values needed for the patch that follows
> are contained on this intermediate patch, so when applied
> this patch doesn't break autoserv.
>
> Signed-off-by: Lucas Meneghel Rodrigues <lmr@xxxxxxxxxx>
> ---
> client/common_lib/global_config.py | 61 +++++++++++++++++++++++++++++----
> client/common_lib/host_protections.py | 26 ++++++++------
> global_config.ini | 3 ++
> server/autotest.py | 33 ++++++++++++++----
> 4 files changed, 98 insertions(+), 25 deletions(-)
>
> diff --git a/client/common_lib/global_config.py b/client/common_lib/global_config.py
> index 2bbeca0..ffa8993 100644
> --- a/client/common_lib/global_config.py
> +++ b/client/common_lib/global_config.py
> @@ -8,12 +8,6 @@ __author__ = 'raphtee@xxxxxxxxxx (Travis Miller)'
> import os, sys, ConfigParser
> from autotest_lib.client.common_lib import error
>
> -dirname = os.path.dirname(sys.modules[__name__].__file__)
> -DEFAULT_CONFIG_FILE = os.path.abspath(os.path.join(dirname,
> - "../../global_config.ini"))
> -DEFAULT_SHADOW_FILE = os.path.abspath(os.path.join(dirname,
> - "../../shadow_config.ini"))
> -
>
> class ConfigError(error.AutotestError):
> pass
> @@ -23,12 +17,50 @@ class ConfigValueError(ConfigError):
> pass
>
>
> +
> +common_lib_dir = os.path.dirname(sys.modules[__name__].__file__)
> +client_dir = os.path.dirname(common_lib_dir)
> +root_dir = os.path.dirname(client_dir)
> +
> +# Check if the config files are at autotest's root dir
> +# This will happen if client is executing inside a full autotest tree, or if
> +# other entry points are being executed
> +global_config_path_root = os.path.join(root_dir, 'global_config.ini')
> +shadow_config_path_root = os.path.join(root_dir, 'shadow_config.ini')
> +config_in_root = (os.path.exists(global_config_path_root) and
> + os.path.exists(shadow_config_path_root))
> +
> +# Check if the config files are at autotest's client dir
> +# This will happen if a client stand alone execution is happening
> +global_config_path_client = os.path.join(client_dir, 'global_config.ini')
> +config_in_client = os.path.exists(global_config_path_client)
> +
> +if config_in_root:
> + DEFAULT_CONFIG_FILE = global_config_path_root
> + DEFAULT_SHADOW_FILE = shadow_config_path_root
> + RUNNING_STAND_ALONE_CLIENT = False
> +elif config_in_client:
> + DEFAULT_CONFIG_FILE = global_config_path_client
> + DEFAULT_SHADOW_FILE = None
> + RUNNING_STAND_ALONE_CLIENT = True
> +else:
> + raise ConfigError("Could not find configuration files "
> + "needed for this program to function. Please refer to "
> + "http://autotest.kernel.org/wiki/GlobalConfig "
> + "for more info.")
> +
> +
> class global_config(object):
> _NO_DEFAULT_SPECIFIED = object()
>
> config = None
> config_file = DEFAULT_CONFIG_FILE
> shadow_file = DEFAULT_SHADOW_FILE
> + running_stand_alone_client = RUNNING_STAND_ALONE_CLIENT
> +
> +
> + def check_stand_alone_client_run(self):
> + return self.running_stand_alone_client
I don't know if it's really necessary to have a method wrapped around
this, instead of just accessing the attribute directly. I've actually
been trying to re-factor existing code that does this to get rid of
getters.
>
>
> def set_config_files(self, config_file=DEFAULT_CONFIG_FILE,
> @@ -47,6 +79,21 @@ class global_config(object):
> return default
>
>
> + def get_session_values(self, section):
> + """
> + Return a config parser object containing a single session of the
> + global configuration, that can be later written to a file object.
> +
> + @param session: Session we want to turn into a config parser object.
> + @return: ConfigParser() object containing all the contents of session.
> + """
> + cfgparser = ConfigParser.ConfigParser()
> + cfgparser.add_section(section)
> + for option, value in self.config.items(section):
> + cfgparser.set(section, option, value)
> + return cfgparser
> +
> +
The method name, parameter and the docstring aren't consistent
(session vs section). For the record, I favor section over session.
> def get_config_value(self, section, key, type=str,
> default=_NO_DEFAULT_SPECIFIED, allow_blank=False):
> self._ensure_config_parsed()
> @@ -106,7 +153,7 @@ class global_config(object):
> # now also read the shadow file if there is one
> # this will overwrite anything that is found in the
> # other config
> - if os.path.exists(self.shadow_file):
> + if self.shadow_file and os.path.exists(self.shadow_file):
> shadow_config = ConfigParser.ConfigParser()
> shadow_config.read(self.shadow_file)
> # now we merge shadow into global
> diff --git a/client/common_lib/host_protections.py b/client/common_lib/host_protections.py
> index 9457114..7c9e6a0 100644
> --- a/client/common_lib/host_protections.py
> +++ b/client/common_lib/host_protections.py
> @@ -22,19 +22,23 @@ Protection = enum.Enum('No protection', # Repair can do anything to
> # this host
> )
>
> +running_client = global_config.global_config.check_stand_alone_client_run()
> +
> try:
> _bad_value = object()
> - default = Protection.get_value(
> - global_config.global_config.get_config_value(
> - 'HOSTS', 'default_protection', default=_bad_value))
> - if default == _bad_value:
> - raise global_config.ConfigError(
> - 'No HOSTS.default_protection defined in global_config.ini')
> + default_protection = global_config.global_config.get_config_value(
> + 'HOSTS', 'default_protection', default=_bad_value)
> + if default_protection == _bad_value:
> + if running_client:
> + logging.debug('Client stand alone run detected. '
> + 'host_protection.default will not be set.')
> + else:
> + raise global_config.ConfigError(
> + 'No HOSTS.default_protection defined in global_config.ini')
> + else:
> + default = Protection.get_value(default_protection)
> +
> except global_config.ConfigError:
> - # can happen if no global_config.ini exists at all, but this can actually
> - # happen in reasonable cases (e.g. on a standalone clinet) where it's
> - # safe to ignore
> - logging.debug('No global_config.ini exists so host_protection.default '
> - 'will not be defined')
> + raise global_config.ConfigError('No global_config.ini exists, aborting')
>
> choices = Protection.choices()
> diff --git a/global_config.ini b/global_config.ini
> index 1cd3b4f..ac6baab 100644
> --- a/global_config.ini
> +++ b/global_config.ini
> @@ -28,6 +28,9 @@ parse_failed_repair_default: 0
> # Autotest potential install paths
> client_autodir_paths: /usr/local/autotest,/home/autotest
>
> +[CLIENT]
> +drop_caches: True
> +drop_caches_between_iterations: True
>
> [SERVER]
> hostname: autotest
> diff --git a/server/autotest.py b/server/autotest.py
> index 3d307a9..77cef54 100644
> --- a/server/autotest.py
> +++ b/server/autotest.py
> @@ -391,9 +391,15 @@ class BaseAutotest(installable_object.InstallableObject):
> cfile += open(tmppath).read()
> open(tmppath, "w").write(cfile)
>
> + # Create and copy configuration file based on the state of the
> + # client configuration
> + client_config_file = self._create_aux_file(host.job)
> + host.send_file(client_config_file, atrun.config_file)
> + os.remove(client_config_file)
> +
> # Create and copy state file to remote_control_file + '.state'
> sysinfo_state = {"__sysinfo": host.job.sysinfo.serialize()}
> - state_file = self._create_state_file(host.job, sysinfo_state)
> + state_file = self._create_aux_file(host.job, sysinfo_state)
> host.send_file(state_file, atrun.remote_control_file + '.state')
> os.remove(state_file)
>
> @@ -407,13 +413,25 @@ class BaseAutotest(installable_object.InstallableObject):
> client_disconnect_timeout=client_disconnect_timeout)
>
>
> - def _create_state_file(self, job, state_dict):
> - """ Create a state file from a dictionary. Returns the path of the
> - state file. """
> + def _create_aux_file(self, job, state_dict=None):
> + """
> + If state_dict is provided, create a state file from this dictionary. If
> + none provided, it creates a client config file based on the global
> + config session CLIENT.
This seems kind of...magical somehow. I understand why you're doing it
this way to reuse the file creation code, but I'd actually take a
different approach using higher order functions. Instead of having it
take a parameter and then use an if-else to decide what kind of
initialization to do, pass in a function. Then make create_state_file
and create_config_file wrappers that call it with the appropriate
function (either doing the pickle.dump, or the client config
creation).
I know that seems kind of convoluted, but I really don't like
"generic" methods that do very different things depending on which
parameters you pass in. If we want a generic "create a temp file and
write some stuff into it" function, we should make a truly generic
one.
> +
> + @param job: Autotest job instance.
> + @param state_dict: Dictionary containing the job's state.
> + @return: The path of the config file.
> + """
> fd, path = tempfile.mkstemp(dir=job.tmpdir)
> - state_file = os.fdopen(fd, "w")
> - pickle.dump(state_dict, state_file)
> - state_file.close()
> + aux_file = os.fdopen(fd, "w")
> + if state_dict:
> + pickle.dump(state_dict, aux_file)
> + else:
> + client_config = global_config.global_config.get_session_values(
> + "CLIENT")
> + client_config.write(aux_file)
> + aux_file.close()
> return path
>
>
> @@ -459,6 +477,7 @@ class _Run(object):
> control += '.' + tag
> self.manual_control_file = control
> self.remote_control_file = control + '.autoserv'
> + self.config_file = os.path.join(self.autodir, 'global_config.ini')
>
>
> def verify_machine(self):
> --
> 1.6.2.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
