cloud-init-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Documentation on the context manager, and tests as well, please. :)

Diff comments:

> 
> === added file 'cloudinit/reporting.py'
> --- cloudinit/reporting.py	1970-01-01 00:00:00 +0000
> +++ cloudinit/reporting.py	2015-07-31 14:49:49 +0000
> @@ -0,0 +1,201 @@
> +# Copyright 2015 Canonical Ltd.
> +# This file is part of cloud-init.  See LICENCE file for license information.
> +#
> +# vi: ts=4 expandtab
> +"""
> +cloud-init reporting framework
> +
> +The reporting framework is intended to allow all parts of cloud-init to
> +report events in a structured manner.
> +"""
> +
> +import abc
> +import logging
> +
> +from cloudinit.registry import DictRegistry
> +
> +
> +FINISH_EVENT_TYPE = 'finish'
> +START_EVENT_TYPE = 'start'
> +
> +DEFAULT_CONFIG = {
> +    'logging': {'type': 'log'},
> +    'print': {'type': 'print'},
> +}
> +
> +
> +class _nameset(set):
> +    def __getattr__(self, name):
> +        if name in self:
> +            return name
> +        raise AttributeError
> +
> +status = _nameset(("SUCCESS", "WARN", "FAIL"))
> +
> +instantiated_handler_registry = DictRegistry()
> +available_handlers = DictRegistry()
> +
> +
> +class ReportingEvent(object):
> +    """Encapsulation of event formatting."""
> +
> +    def __init__(self, event_type, name, description):
> +        self.event_type = event_type
> +        self.name = name
> +        self.description = description
> +
> +    def as_string(self):
> +        """The event represented as a string."""
> +        return '{0}: {1}: {2}'.format(
> +            self.event_type, self.name, self.description)
> +
> +
> +class FinishReportingEvent(ReportingEvent):
> +
> +    def __init__(self, name, description, result=None):
> +        super(FinishReportingEvent, self).__init__(
> +            FINISH_EVENT_TYPE, name, description)
> +        if result is None:
> +            result = status.SUCCESS
> +        self.result = result
> +        if result not in status:
> +            raise ValueError("Invalid result: %s" % result)
> +
> +    def as_string(self):
> +        return '{0}: {1}: {2}: {3}'.format(
> +            self.event_type, self.name, self.result, self.description)
> +
> +
> +class ReportingHandler(object):
> +
> +    @abc.abstractmethod
> +    def publish_event(self, event):
> +        raise NotImplementedError
> +
> +
> +class LogHandler(ReportingHandler):
> +    """Publishes events to the cloud-init log at the ``INFO`` log level."""
> +
> +    def publish_event(self, event):
> +        """Publish an event to the ``INFO`` log level."""
> +        logger = logging.getLogger(
> +            '.'.join([__name__, event.event_type, event.name]))
> +        logger.info(event.as_string())
> +
> +
> +class PrintHandler(ReportingHandler):
> +    def publish_event(self, event):
> +        print(event.as_string())

Definitely stdout rather than stderr? Not sure what we do elsewhere.

> +
> +
> +def add_configuration(config):
> +    for handler_name, handler_config in config.items():
> +        handler_config = handler_config.copy()
> +        cls = available_handlers.registered_items[handler_config.pop('type')]
> +        instance = cls(**handler_config)
> +        instantiated_handler_registry.register_item(handler_name, instance)
> +
> +
> +def report_event(event):
> +    """Report an event to all registered event handlers.
> +
> +    This should generally be called via one of the other functions in
> +    the reporting module.
> +
> +    :param event_type:
> +        The type of the event; this should be a constant from the
> +        reporting module.
> +    """
> +    for _, handler in instantiated_handler_registry.registered_items.items():
> +        handler.publish_event(event)
> +
> +
> +def report_finish_event(event_name, event_description, result):
> +    """Report a "finish" event.
> +
> +    See :py:func:`.report_event` for parameter details.
> +    """
> +    event = FinishReportingEvent(event_name, event_description, result)
> +    return report_event(event)
> +
> +
> +def report_start_event(event_name, event_description):
> +    """Report a "start" event.
> +
> +    :param event_name:
> +        The name of the event; this should be a topic which events would
> +        share (e.g. it will be the same for start and finish events).
> +
> +    :param event_description:
> +        A human-readable description of the event that has occurred.
> +    """
> +    event = ReportingEvent(START_EVENT_TYPE, event_name, event_description)
> +    return report_event(event)
> +
> +
> +class ReportStack(object):
> +    def __init__(self, name, description, parent=None, reporting=None,

`reporting` might be better as `reporting_enabled`.

`exc_result` is a little too reminiscent of the __exit__ parameters; maybe `result_on_exception`?  I _think_ this is also a case where it's safe to do `result_on_exception=status.FAIL`; you only need to use the foo=None trick when you want the default to be something mutable (like a list or a dict).

> +                 exc_result=None):
> +        self.parent = parent
> +        self.name = name
> +        self.description = description
> +
> +        if exc_result is None:
> +            exc_result = status.FAIL
> +        self.exc_result = exc_result
> +
> +        # use parents reporting value if not provided
> +        if reporting is None:
> +            if parent:
> +                reporting = parent.reporting
> +            else:
> +                reporting = True
> +        self.reporting = reporting
> +
> +        if parent:
> +            self.fullname = '/'.join((parent.fullname, name,))
> +        else:
> +            self.fullname = self.name
> +        self.children = {}
> +
> +    def __repr__(self):
> +        return ("%s reporting=%s" % (self.fullname, self.reporting))
> +
> +    def __enter__(self):
> +        self.exception = None

I don't think self.exception is ever read, just written to; what's it intended for?

> +        if self.reporting:
> +            report_start_event(self.fullname, self.description)
> +        if self.parent:
> +            self.parent.children[self.name] = (None, None)
> +        return self
> +
> +    def childrens_finish_info(self, result=None, description=None):
> +        for cand_result in (status.FAIL, status.WARN):
> +            for name, (value, msg) in self.children.items():
> +                if value == cand_result:
> +                    return (value, "[" + name + "]" + msg)
> +        if result is None:

Do we need these ifs?  I don't think `result` or `description` can change, so if we get to this point they will be None.  So can this line just be `return (status.SUCCESS, self.description)`?

> +            result = status.SUCCESS
> +        if description is None:
> +            description = self.description
> +        return (result, description)
> +
> +    def finish_info(self, exc):
> +        # return tuple of description, and value
> +        if exc:
> +            # by default, exceptions are fatal
> +            return (self.exc_result, self.description)
> +        return self.childrens_finish_info()
> +
> +    def __exit__(self, exc_type, exc_value, traceback):
> +        self.exception = exc_value
> +        (result, msg) = self.finish_info(exc_value)
> +        if self.parent:
> +            self.parent.children[self.name] = (result, msg)
> +        if self.reporting:
> +            report_finish_event(self.fullname, msg, result)
> +
> +        
> +available_handlers.register_item('log', LogHandler)
> +available_handlers.register_item('print', PrintHandler)
> +add_configuration(DEFAULT_CONFIG)
> 
> === modified file 'cloudinit/sources/__init__.py'
> --- cloudinit/sources/__init__.py	2015-07-22 12:06:34 +0000
> +++ cloudinit/sources/__init__.py	2015-07-31 14:49:49 +0000
> @@ -246,17 +247,23 @@
>      return keys
>  
>  
> -def find_source(sys_cfg, distro, paths, ds_deps, cfg_list, pkg_list):
> +def find_source(sys_cfg, distro, paths, ds_deps, cfg_list, pkg_list, reporter):
>      ds_list = list_sources(cfg_list, ds_deps, pkg_list)
>      ds_names = [type_utils.obj_name(f) for f in ds_list]
>      LOG.debug("Searching for data source in: %s", ds_names)
>  
> -    for cls in ds_list:
> +    for i, cls in enumerate(ds_list):

You could do `for name, cls in zip(ds_names, ds_list):`.

> +        name=ds_names[i].replace("DataSource", "")
> +        myreporter = reporting.ReportStack(
> +            "check-%s" % name, "searching for %s" % name,
> +            parent=reporter, exc_result=reporting.status.WARN)
> +            
>          try:
> -            LOG.debug("Seeing if we can get any data from %s", cls)
> -            s = cls(sys_cfg, distro, paths)
> -            if s.get_data():
> -                return (s, type_utils.obj_name(cls))
> +            with myreporter:
> +                LOG.debug("Seeing if we can get any data from %s", cls)
> +                s = cls(sys_cfg, distro, paths)
> +                if s.get_data():
> +                    return (s, type_utils.obj_name(cls))
>          except Exception:
>              util.logexc(LOG, "Getting data from %s failed", cls)
>  


-- 
https://code.launchpad.net/~smoser/cloud-init/trunk.reporting/+merge/266578
Your team cloud init development team is requested to review the proposed merge of lp:~smoser/cloud-init/trunk.reporting into lp:cloud-init.