Re: [PATCH 2/3] Make a standalone client to be able to use global_config.ini

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Almost there. Just a couple of minor things.

On Tue, Nov 17, 2009 at 5:04 PM, 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):
>
>  * The ugly create_aux_file was turned into a generic function that
>   shows consistent behavior regardless of the parameters passed in,
>   then 2 wrapper functions, create_state_file and create_config_file
>   are being used, both calling create_aux_file.
>  * Fixed spelling errors on the get_section_values docstring, as
>   well as the function name, making them all consistent with 'section'.
> ---
>  client/common_lib/global_config.py    |   61 +++++++++++++++++++++++++++++----
>  client/common_lib/host_protections.py |   26 ++++++++------
>  global_config.ini                     |    3 ++
>  server/autotest.py                    |   51 +++++++++++++++++++++++++---
>  4 files changed, 118 insertions(+), 23 deletions(-)
>
> diff --git a/client/common_lib/global_config.py b/client/common_lib/global_config.py
> index 2bbeca0..6c801ba 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
>
>
>     def set_config_files(self, config_file=DEFAULT_CONFIG_FILE,
> @@ -47,6 +79,21 @@ class global_config(object):
>             return default
>
>
> +    def get_section_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 section: Section we want to turn into a config parser object.
> +        @return: ConfigParser() object containing all the contents of session.
> +        """

This docstring still mentions "session" a couple times.

> +        cfgparser = ConfigParser.ConfigParser()
> +        cfgparser.add_section(section)
> +        for option, value in self.config.items(section):
> +            cfgparser.set(section, option, value)
> +        return cfgparser
> +
> +
>     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..a915196 100644
> --- a/server/autotest.py
> +++ b/server/autotest.py
> @@ -391,6 +391,12 @@ 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_client_config_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)
> @@ -408,12 +414,46 @@ class BaseAutotest(installable_object.InstallableObject):
>
>
>     def _create_state_file(self, job, state_dict):
> -        """ Create a state file from a dictionary. Returns the path of the
> -        state file. """
> +        """
> +        Create a temporary file with the state described by state_dict.
> +
> +        @param job: Autotest job.
> +        @param state_dict: Dictionary containing the state we want to write to
> +                the temporary file
> +        @return: Path of the temporary file generated.
> +        """
> +        return self._create_aux_file(job, pickle.dump, state_dict)
> +
> +
> +    def _create_client_config_file(self, job):
> +        """
> +        Create a temporary file with the [CLIENT] section configuration values
> +        taken from the server global_config.ini.
> +
> +        @param job: Autotest job.
> +        @return: Path of the temporary file generated.
> +        """
> +        client_config = global_config.global_config.get_section_values("CLIENT")
> +        return self._create_aux_file(job, client_config.write)
> +
> +
> +    def _create_aux_file(self, job, func, *args):
> +        """
> +        Creates a temporary file and writes content to it according to a content
> +        creation function.
> +
> +        @param job: Autotest job instance.
> +        @param func: Function that will be used to write content to the
> +                temporary file.
> +        @param *args: List of parameters that func takes.
> +        @return: Path to the temporary file that was created.
> +        """
>         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")
> +        args = list(args)
> +        args.append(aux_file)
> +        func(*args)

I think this is a little weird (we pass in args, but append the file
to it as the last argument)? I think the docstring should mention the
fact that the temporary file object is added on to the args. It might
also be a good idea to use a different name when you convert args to a
list and add aux_file to it, as when I first read this code it didn't
immediately jump out at me that func(*args) wasn't using the same args
that were passed in.

> +        aux_file.close()
>         return path
>
>
> @@ -459,6 +499,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

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux