cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

I've pointed CI at this branch.  A couple of inline comments and questions.

Diff comments:

> diff --git a/cloudinit/reporting/handlers.py b/cloudinit/reporting/handlers.py
> old mode 100644
> new mode 100755
> index 6d23558..7cca47e
> --- a/cloudinit/reporting/handlers.py
> +++ b/cloudinit/reporting/handlers.py
> @@ -129,24 +129,46 @@ class HyperVKvpReportingHandler(ReportingHandler):
>      DESC_IDX_KEY = 'msg_i'
>      JSON_SEPARATORS = (',', ':')
>      KVP_POOL_FILE_GUEST = '/var/lib/hyperv/.kvp_pool_1'
> +    _already_truncated_pool_file = False
>  
>      def __init__(self,
>                   kvp_file_path=KVP_POOL_FILE_GUEST,
>                   event_types=None):
>          super(HyperVKvpReportingHandler, self).__init__()
>          self._kvp_file_path = kvp_file_path
> +        HyperVKvpReportingHandler._truncate_guest_pool_file(
> +            self._kvp_file_path)
> +
>          self._event_types = event_types
>          self.q = JQueue()
> -        self.kvp_file = None
>          self.incarnation_no = self._get_incarnation_no()
>          self.event_key_prefix = u"{0}|{1}".format(self.EVENT_PREFIX,
>                                                    self.incarnation_no)
> -        self._current_offset = 0
>          self.publish_thread = threading.Thread(
>                  target=self._publish_event_routine)
>          self.publish_thread.daemon = True
>          self.publish_thread.start()
>  
> +    @classmethod
> +    def _truncate_guest_pool_file(cls, kvp_file):
> +        """
> +        Truncate the pool file if it has not been truncated since boot.
> +        This should be done exactly once for the file indicated by
> +        KVP_POOL_FILE_GUEST constant above. This method takes a filename
> +        so that we can use an arbitrary file during unit testing.
> +        """
> +        if cls._already_truncated_pool_file:
> +            return
> +        boot_time = time.time() - float(util.uptime())
> +        try:
> +            if os.path.getmtime(kvp_file) < boot_time:
> +                with open(kvp_file, "w"):
> +                    pass
> +        except (OSError, IOError) as e:
> +            LOG.warning("failed to truncate kvp pool file, %s", e)
> +        finally:

Could you provide some details on why this must be done only once?  And if you don't truncate it due to errors, why you skip further attempts to truncate?

> +            cls._already_truncated_pool_file = True
> +
>      def _get_incarnation_no(self):
>          """
>          use the time passed as the incarnation number.
> diff --git a/tests/unittests/test_reporting_hyperv.py b/tests/unittests/test_reporting_hyperv.py
> old mode 100644
> new mode 100755
> index 2e64c6c..d01ed5b
> --- a/tests/unittests/test_reporting_hyperv.py
> +++ b/tests/unittests/test_reporting_hyperv.py
> @@ -26,57 +28,9 @@ class TextKvpReporter(CiTestCase):
>          self.tmp_file_path = self.tmp_path('kvp_pool_file')
>          util.ensure_file(self.tmp_file_path)
>  
> -    def test_event_type_can_be_filtered(self):

Can comment on the dropping of filtering?  Is that the same as your commit line: "No longer update previous entries in the KVP" ?

> -        reporter = handlers.HyperVKvpReportingHandler(
> -            kvp_file_path=self.tmp_file_path,
> -            event_types=['foo', 'bar'])
> -
> -        reporter.publish_event(
> -            events.ReportingEvent('foo', 'name', 'description'))
> -        reporter.publish_event(
> -            events.ReportingEvent('some_other', 'name', 'description3'))
> -        reporter.q.join()
> -
> -        kvps = list(reporter._iterate_kvps(0))
> -        self.assertEqual(1, len(kvps))
> -
> -        reporter.publish_event(
> -            events.ReportingEvent('bar', 'name', 'description2'))
> -        reporter.q.join()
> -        kvps = list(reporter._iterate_kvps(0))
> -        self.assertEqual(2, len(kvps))
> -
> -        self.assertIn('foo', kvps[0]['key'])
> -        self.assertIn('bar', kvps[1]['key'])
> -        self.assertNotIn('some_other', kvps[0]['key'])
> -        self.assertNotIn('some_other', kvps[1]['key'])
> -
> -    def test_events_are_over_written(self):
> -        reporter = handlers.HyperVKvpReportingHandler(
> -            kvp_file_path=self.tmp_file_path)
> -
> -        self.assertEqual(0, len(list(reporter._iterate_kvps(0))))
> -
> -        reporter.publish_event(
> -            events.ReportingEvent('foo', 'name1', 'description'))
> -        reporter.publish_event(
> -            events.ReportingEvent('foo', 'name2', 'description'))
> -        reporter.q.join()
> -        self.assertEqual(2, len(list(reporter._iterate_kvps(0))))
> -
> -        reporter2 = handlers.HyperVKvpReportingHandler(
> -            kvp_file_path=self.tmp_file_path)
> -        reporter2.incarnation_no = reporter.incarnation_no + 1
> -        reporter2.publish_event(
> -            events.ReportingEvent('foo', 'name3', 'description'))
> -        reporter2.q.join()
> -
> -        self.assertEqual(2, len(list(reporter2._iterate_kvps(0))))
> -
>      def test_events_with_higher_incarnation_not_over_written(self):
> -        reporter = handlers.HyperVKvpReportingHandler(
> +        reporter = HyperVKvpReportingHandler(
>              kvp_file_path=self.tmp_file_path)
> -
>          self.assertEqual(0, len(list(reporter._iterate_kvps(0))))
>  
>          reporter.publish_event(


-- 
https://code.launchpad.net/~vtqanh/cloud-init/+git/cloud-init/+merge/366044
Your team cloud-init commiters is requested to review the proposed merge of ~vtqanh/cloud-init:ImproveHyperVKvpReporter into cloud-init:master.