Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add switch control when watching events whether deserialization… #2369

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion kubernetes/base/watch/watch.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,19 @@ def stream(self, func, *args, **kwargs):
# We want to ensure we are returning within that timeout.
disable_retries = ('timeout_seconds' in kwargs)
retry_after_410 = False
deserialize = kwargs.pop('deserialize', False)
while True:
resp = func(*args, **kwargs)
try:
for line in iter_resp_lines(resp):
# unmarshal when we are receiving events from watch,
# return raw string when we are streaming log
if watch_arg == "watch":
event = self.unmarshal_event(line, return_type)
if deserialize:
event = self.unmarshal_event(line, return_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have the return_type flag which could serve the purpose, but it looks pretty unintuitive-- IIUC _raw_return_type can be persisted by setting return_type to False, which would result in deserialization being skipped

That being said, the flag name seems unintuitive and I think deserialize = False makes more sense.

cc @yliaog

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on deserialize = False

else:
# Only do basic JSON parsing, no deserialize
event = json.loads(line)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here event['raw_object'] is not populated like here does, which means the retry_after_410 logic won't work.

We need to make sure the retry logic continue to work even when we don't deserialize the object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that in this way, the resource version is not tracked like here does.

I think we can refactor the unmarshal_event method to keep track of the resource version even when we skip deserialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the tests to ensure the retry and resource version behaviors when deserialize = False

if isinstance(event, dict) \
and event['type'] == 'ERROR':
obj = event['raw_object']
Expand Down
38 changes: 38 additions & 0 deletions kubernetes/base/watch/watch_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,44 @@ def test_watch_with_error_event_and_timeout_param(self):
fake_resp.close.assert_called_once()
fake_resp.release_conn.assert_called_once()

def test_watch_with_deserialize_param(self):
"""test watch.stream() deserialize param"""
# prepare test data
test_json = '{"type": "ADDED", "object": {"metadata": {"name": "test1", "resourceVersion": "1"}, "spec": {}, "status": {}}}'
fake_resp = Mock()
fake_resp.close = Mock()
fake_resp.release_conn = Mock()
fake_resp.stream = Mock(return_value=[test_json + '\n'])

fake_api = Mock()
fake_api.get_namespaces = Mock(return_value=fake_resp)
fake_api.get_namespaces.__doc__ = ':return: V1NamespaceList'

# test case with deserialize=True
w = Watch()
for e in w.stream(fake_api.get_namespaces, deserialize=True):
self.assertEqual("ADDED", e['type'])
# Verify that the object is deserialized correctly
self.assertTrue(hasattr(e['object'], 'metadata'))
self.assertEqual("test1", e['object'].metadata.name)
self.assertEqual("1", e['object'].metadata.resource_version)
# Verify that the original object is saved
self.assertEqual(json.loads(test_json)['object'], e['raw_object'])

# test case with deserialize=False
w = Watch()
for e in w.stream(fake_api.get_namespaces, deserialize=False):
self.assertEqual("ADDED", e['type'])
# The validation object remains in the original dictionary format
self.assertIsInstance(e['object'], dict)
self.assertEqual("test1", e['object']['metadata']['name'])
self.assertEqual("1", e['object']['metadata']['resourceVersion'])

# verify the api is called twice
fake_api.get_namespaces.assert_has_calls([
call(_preload_content=False, watch=True),
call(_preload_content=False, watch=True)
])

if __name__ == '__main__':
unittest.main()