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

Do not fail on empty log line #818

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Conversation

YevheniiSemendiak
Copy link
Contributor

@YevheniiSemendiak YevheniiSemendiak commented Jul 5, 2023

If we have a job with empty log line, for instance (simple STR):
n run ubuntu -- bash -c "echo; echo hi;"

platform cannot handle its logs, since fluent-bit omits "log" in logs entry:
image
and logs from k8s:
image

Corresponding error from service:

2023-07-05 18:41:57,671 - platform_monitoring.logs - ERROR - Invalid log entry: b'{"time":"2023-07-05T18:39:49.720424314Z","stream":"stdout","logtag":"F","kubernetes":{"pod_name":"job-b34af07f-0418-4e12-8529-906c6c284f68","namespace_name":"platform-jobs","pod_id":"f06694a3-1c4e-4bfb-93ce-60f4a5ccc9c6","labels":{"platform.neuromation.io/gpu-model":"nvidia-tesla-v100","platform.neuromation.io/job":"job-b34af07f-0418-4e12-8529-906c6c284f68","platform.neuromation.io/org":"no_org","platform.neuromation.io/preset":"gpu-small","platform.neuromation.io/user":"yevheniisemendiak"},"annotations":{"cni.projectcalico.org/containerID":"4588e23019d34ca8ae8194ab98f1eed4c2ddc3666b88c49b08836418809181fc","cni.projectcalico.org/podIP":"","cni.projectcalico.org/podIPs":""},"host":"master-1","container_name":"job-b34af07f-0418-4e12-8529-906c6c284f68","docker_id":"0a85ee521ae55eaa8dadadd396875cbbd05c58037cb0f306886acbd5f8fba03b","container_hash":"0bced47fffa3361afa981854fcabcd4577cd43cebbb808cea2b1f33a3dd7f508"}}'
Traceback (most recent call last):
  File "/root/.local/lib/python3.9/site-packages/platform_monitoring/logs.py", line 364, in _iterate
    log = event["log"]
KeyError: 'log'
2023-07-05 18:41:57,674 - asyncio - ERROR - Task exception was never retrieved
future: <Task finished name='Task-448227' coro=<_forward_bytes_iterating() done, defined at /root/.local/lib/python3.9/site-packages/platform_monitoring/api.py:492> exception=KeyError('log')>
Traceback (most recent call last):
  File "/root/.local/lib/python3.9/site-packages/platform_monitoring/api.py", line 495, in _forward_bytes_iterating
    async for chunk in it:
  File "/root/.local/lib/python3.9/site-packages/platform_monitoring/logs.py", line 441, in get_pod_log_reader
    async for chunk in it:
  File "/root/.local/lib/python3.9/site-packages/platform_monitoring/logs.py", line 364, in _iterate
    log = event["log"]
KeyError: 'log'

Maybe, we could configure fluent-bit properly, but I haven't found how to do it.
It also might close #719

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #818 (2ddc4e0) into master (96ed14d) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #818   +/-   ##
=======================================
  Coverage   86.05%   86.05%           
=======================================
  Files          12       12           
  Lines        2094     2094           
  Branches      287      287           
=======================================
  Hits         1802     1802           
  Misses        179      179           
  Partials      113      113           
Flag Coverage Δ
integration 79.41% <100.00%> (-1.20%) ⬇️
unit 47.27% <100.00%> (+1.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
platform_monitoring/logs.py 87.52% <100.00%> (+0.47%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Contributor

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

great!

we should probably try to add a test.

@YevheniiSemendiak
Copy link
Contributor Author

great!

we should probably try to add a test.

It took too much time to add such a simple test, but I did it :D
Please, take a look once more.

Copy link
Contributor

@dalazx dalazx left a comment

Choose a reason for hiding this comment

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

thank you!

@YevheniiSemendiak YevheniiSemendiak merged commit f50bdf1 into master Jul 20, 2023
@YevheniiSemendiak YevheniiSemendiak deleted the ys/handle-empty-log-lines branch July 20, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

neuro logs TypeError: can't concat WebSocketError to bytes
3 participants