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 exception handling #33

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
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
6 changes: 6 additions & 0 deletions debian/changelog
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
wb-cloud-agent (1.5.9) stable; urgency=medium

* Fix exception handling

-- Nikolay Korotkiy <[email protected]> Thu, 15 Aug 2024 15:45:00 +0400

wb-cloud-agent (1.5.8) stable; urgency=medium

* Refactor mqtt client, no functional changes
Expand Down
43 changes: 30 additions & 13 deletions wb/cloud_agent/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import json
import logging
import os
import signal
import subprocess
import sys
import threading
import time
from contextlib import ExitStack
Expand Down Expand Up @@ -83,7 +85,12 @@ def do_curl(settings: AppSettings, method="get", endpoint="", params=None):
url,
]

result = subprocess.run(command, timeout=360, check=True, capture_output=True)
try:
result = subprocess.run(command, timeout=360, check=True, capture_output=True)
except subprocess.CalledProcessError as e:
raise ValueError("Error during request: " + str(e))
Comment on lines +90 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

а зачем это заворачивать в ValueError?

except subprocess.TimeoutExpired as e:
raise ValueError("Timeout expired: " + str(e))
Comment on lines +92 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

не очень понятно, зачем это заворачивать в ValueError


decoded_result = result.stdout.decode("utf-8")
split_result = decoded_result.split(data_delimiter)
Expand Down Expand Up @@ -342,18 +349,28 @@ def main():

mqtt = MQTTCloudAgent(settings, on_message)

if hasattr(options, "func"):
mqtt.start()
return options.func(options, settings, mqtt)
signal.signal(signal.SIGINT, signal.SIG_DFL)
signal.signal(signal.SIGTERM, signal.SIG_DFL)

if not options.daemon:
mqtt.start()
make_start_up_request(settings, mqtt)
return show_activation_link(settings)
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

резко всю функцию сдвинули, при этом не покрыли обработкой исключений всё то, что происходит выше (а там ведь и файлы читаются, и клиент создаётся, как будто бы тоже можно исключений наловить, ещё и таких, чтобы systemd не перезапускал это потом, а говорил, что сервис не сконфигурирован как надо)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

как будто бы тоже можно исключений наловить, ещё и таких, чтобы systemd не перезапускал это потом, а говорил, что сервис не сконфигурирован как надо

Про это отдельно тут #38

if hasattr(options, "func"):
mqtt.start()
return options.func(options, settings, mqtt)

mqtt.start(update_status=True)
make_start_up_request(settings, mqtt)
send_agent_version(settings)
update_providers_list(settings, mqtt)
if not options.daemon:
mqtt.start()
make_start_up_request(settings, mqtt)
return show_activation_link(settings)

run_daemon(mqtt, settings)
mqtt.start(update_status=True)
make_start_up_request(settings, mqtt)
send_agent_version(settings)
update_providers_list(settings, mqtt)

run_daemon(mqtt, settings)
except (ConnectionError, ConnectionRefusedError):
logging.error(f"Cannot connect to broker {settings.BROKER_URL}")
sys.exit(1)
except Exception as e:
logging.error(e)
sys.exit(1)
Comment on lines +371 to +376
Copy link
Contributor

Choose a reason for hiding this comment

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

кстати о гайдлайнах, про это мы не писали, а можно сделать сразу так, чтобы у нас был стандарт на оформление сервисов по-правильному (я так понимаю, тут ещё надо будет pylint урезонить, чтобы он не ругался на ловлю Exception)