cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #06182
Re: [Merge] ~vtqanh/cloud-init:ImproveHyperVKvpReporter into cloud-init:master
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.
References