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

Fix parsing of garbage input to get_event_ids #45

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Jan 25, 2024

Depends on #44.

Handles the following exception better:

2024-01-24 11:52:01,670 ERROR howitz /../howitz/src/howitz/error_handlers.py:46:: An unexpected exception has occurred Illegal response from server detected: '1705928363 ifi2-gw5: port "et-0/1/6" ix 719 (UN000165, ifi2-hmg9, hmg9-gw1) changed state from up to up on 1705928245\r\n.\r\n'
Traceback (most recent call last):
  File "/../zinolib/src/zinolib/ritz.py", line 391, in _request
    header = (int(rawh[0]), rawh[1])
ValueError: invalid literal for int() with base 10: 'g9-gw1)'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/VENV/howitz/lib/python3.10/site-packages/flask/app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
  File "/VENV/howitz/lib/python3.10/site-packages/flask/app.py", line 1469, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "/../howitz/src/howitz/endpoints.py", line 305, in poll_events
  File "/../howitz/src/howitz/endpoints.py", line 117, in poll_current_events
    current_app.logger.exception('Recurrent NotConnectedError %s', notConnErr)
  File "/../zinolib/src/zinolib/controllers/zino1.py", line 472, in get_events
    for event_id in self._event_adapter.get_event_ids(self.session.request):
  File "/../zinolib/src/zinolib/controllers/zino1.py", line 300, in get_event_ids
    return request.get_caseids()
  File "/../zinolib/src/zinolib/ritz.py", line 545, in get_caseids
    response = self._request(b"caseids")
  File "/../zinolib/src/zinolib/ritz.py", line 393, in _request
    raise ProtocolError(
zinolib.ritz.ProtocolError: Illegal response from server detected: '1705928363 ifi2-gw5: port "et-0/1/6" ix 719 (UN000165, ifi2-hmg9, hmg9-gw1) changed state from up to up on 1705928245\r\n.\r\n'

The ProtocolError is replaced with a RetryError.

@hmpf hmpf self-assigned this Jan 25, 2024
@hmpf hmpf requested a review from podliashanyk January 25, 2024 07:46
Copy link

github-actions bot commented Jan 25, 2024

Test results

    3 files      3 suites   41s ⏱️
  63 tests   63 ✔️ 0 💤 0
189 runs  189 ✔️ 0 💤 0

Results for commit 6dd466d.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (d97de03) 71.52% compared to head (6dd466d) 71.36%.
Report is 4 commits behind head on main.

Files Patch % Lines
src/zinolib/controllers/zino1.py 28.57% 5 Missing ⚠️
src/zinolib/ritz.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
- Coverage   71.52%   71.36%   -0.16%     
==========================================
  Files          13       13              
  Lines        1366     1369       +3     
==========================================
  Hits          977      977              
- Misses        389      392       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

So, I suppose the RetryError is just a signal to the client code that it should attempt to retry the operation?

@hmpf hmpf force-pushed the fix-input-to-caseids branch from 0d0da84 to 2784aba Compare January 25, 2024 07:54
@hmpf
Copy link
Contributor Author

hmpf commented Jan 25, 2024

So, I suppose the RetryError is just a signal to the client code that it should attempt to retry the operation?

Yes. That's usually sufficient, though howitz actually counts these and can act on there being very many of them.

@hmpf hmpf force-pushed the fix-input-to-caseids branch from 76f4a95 to 291af29 Compare January 25, 2024 11:28
@hmpf hmpf requested a review from lunkwill42 January 25, 2024 11:29
@podliashanyk
Copy link
Contributor

I checked in Howitz and not all RetryErrors lead to automatic retry. Created a separate issue for it Uninett/Howitz#62

@podliashanyk
Copy link
Contributor

Garbage response from ZIno can often be emulated by setting f.e. poll_interval=5 and timeout=5 (some short time values in general) in Howitz config, and trying to expand multiple table rows while table polling request is ongoing.

Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Should RetryError be raised on history fetch as well?

@hmpf
Copy link
Contributor Author

hmpf commented Jan 26, 2024

Should RetryError be raised on history fetch as well?

Not in this alpha :) The error has never happened for history fetch, for some reason.

Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Test coverage is too low, but it works nice so approve it as is for alpha 👍

@hmpf hmpf closed this Jan 26, 2024
@hmpf hmpf reopened this Jan 26, 2024
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hmpf hmpf merged commit eb0409f into Uninett:main Jan 26, 2024
10 of 12 checks passed
@hmpf hmpf deleted the fix-input-to-caseids branch January 26, 2024 11:57
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.

3 participants