[Fix] Send background VPN errors and recovery as generic_message noti… #1147
Conversation
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The test failure is due to openwisp/django-x509@d9b8913.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi I cherry-picked your last commit to fix the CI build on top of master, can you please rebase on master and remove that commit from this branch?
Fixed all failing tests that were failing due to inconsistencies with recent updates in notification handling, logging, and retry mechanisms. Fixes #1049
stktyagi
force-pushed
the
issues/1049-send-generic-message
branch
from
624eb0c to
9f31948
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughCentralized notification utilities were added and wired into VPN and ZeroTier task flows. HTTP POST calls now use timeout and verification, raise_for_status, and exceptions trigger error or recovery notifications via cache-backed helpers; tests were updated to mock responses and expect warning-level logs. Changes
Sequence Diagram(s)sequenceDiagram
participant Task as API Task (VPN/ZeroTier)
participant Cache as Django Cache
participant Notify as openwisp_notifications
participant Log as Logger
Note over Task,Log: API request → state check → notify on change
Task->>Task: Build & send HTTP POST (verify, timeout)
Task->>+Cache: cache.get(task_key)
Cache-->>-Task: previous_state
alt HTTP 2xx
Task->>Log: info "success" (include vpn uuid)
Task->>Cache: cache.set(task_key, "success")
alt previous_state == "error"
Task->>Notify: send recovery notification
Note over Notify: generic recovery message
end
else HTTP error / exception
Task->>Log: warning "failure" (include vpn uuid & error)
alt previous_state != "error"
Task->>Cache: cache.set(task_key, "error")
Task->>Notify: send error notification
Note over Notify: generic error message
else
Note over Task: duplicate suppressed — no new notification
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openwisp_controller/config/tasks.py (1)
143-154: Consider using f-string conversion flag for cleaner exception formatting.Using
{e!s}is more idiomatic thanstr(e)in f-strings.🔎 Proposed fix
except requests.RequestException as e: logger.warning( f"Failed to update VPN Server configuration. " - f"Error: {str(e)}, " + f"Error: {e!s}, " f"VPN Server UUID: {vpn_id}" )openwisp_controller/config/utils.py (1)
214-255: Guard against missingactionparameter to prevent AttributeError.If
kwargs.get("action")returnsNone, calling.replace("_", " ")will raise anAttributeError. While current callers always provideaction, defensive coding would prevent future regressions.🔎 Proposed fix
def send_api_task_notification(type, **kwargs): vpn = kwargs.get("instance") - action = kwargs.get("action").replace("_", " ") + action = kwargs.get("action", "").replace("_", " ") exception = kwargs.get("exception")openwisp_controller/config/tests/test_vpn.py (1)
731-736: Consider using method-level mock creation for better test isolation.Class-level
mock.Mock()objects are shared across all test methods in the class. While this works for read-only usage, if any test modifiesmock_responseproperties (e.g.,status_code), it could affect subsequent tests. Consider creating the mock insetUpor within individual test methods for better isolation.🔎 Example using setUp
class TestWireguardTransaction(BaseTestVpn, TestWireguardVpnMixin, TransactionTestCase): def setUp(self): super().setUp() self.mock_response = mock.Mock(spec=requests.Response) self.mock_response.status_code = 200 self.mock_response.raise_for_status = mock.Mock()Also applies to: 999-1004
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_controller/config/tasks.pyopenwisp_controller/config/tasks_zerotier.pyopenwisp_controller/config/tests/test_notifications.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/utils.py
🧰 Additional context used
🧬 Code graph analysis (4)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
openwisp_controller/config/utils.py (1)
openwisp_controller/config/controller/views.py (4)
get(147-161)get(199-207)get(524-530)get(538-546)
openwisp_controller/config/tests/test_vpn.py (2)
openwisp_controller/config/base/vpn.py (1)
post_save(905-916)openwisp_controller/subnet_division/base/models.py (1)
post_save(226-236)
openwisp_controller/config/tasks.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
🪛 Ruff (0.14.10)
openwisp_controller/config/tasks.py
137-137: Probable use of requests call without timeout
(S113)
140-140: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
146-146: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (10)
openwisp_controller/config/tasks.py (2)
12-13: LGTM!The new imports for notification utilities are correctly added and align with the centralized notification pattern.
155-161: LGTM!The success path correctly logs the action and triggers recovery notification with the appropriate task key and parameters.
openwisp_controller/config/tasks_zerotier.py (2)
13-13: LGTM!Import correctly added for the new notification utilities.
62-77: LGTM!The notification handling is correctly refactored:
handle_recovery_notificationis called on success with appropriate kwargslogger.warningcorrectly replaces the deprecatedlogger.warnhandle_error_notificationis called for unrecoverable errors with the exception includedopenwisp_controller/config/utils.py (2)
3-11: LGTM!New imports correctly added for cache, translation, and notification functionality.
258-269: LGTM with a consideration for cache cleanup.The cache-based state tracking pattern is well-designed:
handle_recovery_notificationonly sends success if previous state was "error"handle_error_notificationonly sends error if not already in "error" stateThis prevents notification flooding effectively. Note that using
timeout=Nonemeans cache entries persist indefinitely. If VPN servers are deleted, orphaned cache keys will remain. This is low-impact but worth documenting or addressing in a future cleanup task.openwisp_controller/config/tests/test_notifications.py (1)
34-36: LGTM!The logger path correctly updated from
logger.warntologger.warningto match the implementation change intasks_zerotier.py.openwisp_controller/config/tests/test_vpn.py (3)
6-6: LGTM!Import added to support the new mock response pattern.
830-866: LGTM!The test mocks are correctly updated to:
- Use
mock.Mock(spec=requests.Response)for proper response simulation- Configure
raise_for_statusto raiseHTTPErrorfor failure cases- Update assertions to expect
logger.warninginstead oflogger.error- Match the new error message format ("Error: Not Found")
1477-1479: LGTM!Logger path correctly updated to match the implementation change.
Added timeout parameter to request.post with verify variable simplification. Guard against missing action parameter to prevent AttributeError. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
openwisp_controller/config/tasks.py (1)
135-162: Solid error handling and notification flow!The implementation correctly wraps the HTTP request in try/except, adds timeout protection, and emits notifications for both error and recovery scenarios. The cache-backed state tracking via
task_keyensures users are notified appropriately.One minor optimization: Line 147 uses
str(e)in the f-string, which can be simplified using the explicit conversion flag.🔎 Minor f-string optimization
Based on static analysis hints, use explicit conversion flag:
logger.warning( f"Failed to update VPN Server configuration. " - f"Error: {str(e)}, " + f"Error: {e!s}, " f"VPN Server UUID: {vpn_id}" )openwisp_controller/config/utils.py (3)
258-262: Consider adding documentation.The function correctly implements state-based recovery notifications using cache. Adding a docstring would help maintainers understand the cache-based state tracking pattern.
🔎 Suggested docstring
def handle_recovery_notification(task_key, **kwargs): + """ + Handle recovery notification for background tasks. + + Sends a success notification only if the task previously failed, + then updates the cached state to "success". + + Args: + task_key (str): Cache key for tracking task state + **kwargs: Parameters passed to send_api_task_notification + """ task_result = cache.get(task_key)
265-269: Consider adding documentation.The function correctly implements state-based error notifications with deduplication. Adding a docstring would help maintainers understand the cache-based pattern and prevent notification spam.
🔎 Suggested docstring
def handle_error_notification(task_key, **kwargs): + """ + Handle error notification for background tasks. + + Sends an error notification only on the first occurrence, + preventing notification spam for repeated failures. + + Args: + task_key (str): Cache key for tracking task state + **kwargs: Parameters passed to send_api_task_notification + """ cached_value = cache.get(task_key)
214-255: Add docstring and input validation; remove obsolete comment about issue #264.The function lacks documentation and doesn't validate the
typeparameter. Additionally, the commented-outsleep(2)and associated comment can be removed—issue #264 in openwisp-notifications has been fixed upstream in the 25.10 release, making this workaround unnecessary.🔎 Suggested improvements
- Add a docstring explaining parameters and behavior:
def send_api_task_notification(type, **kwargs): """ Send API task notification via openwisp-notifications. Args: type (str): Notification type, either "error" or "success" **kwargs: Additional parameters including: - instance: The VPN instance - action: The action being performed (e.g., "update") - exception: The exception object if type is "error" """
- Add type parameter validation and remove obsolete comment:
-def send_api_task_notification(type, **kwargs): +def send_api_task_notification(type, **kwargs): + if type not in ("error", "success"): + raise ValueError(f"Invalid notification type: {type}. Must be 'error' or 'success'.") vpn = kwargs.get("instance") action = kwargs.get("action", "").replace("_", " ") exception = kwargs.get("exception") - # Adding some delay here to prevent overlapping - # of the django success message container - # with the ow-notification container - # https://github.com/openwisp/openwisp-notifications/issues/264 - # sleep(2) message_map = {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/tasks.pyopenwisp_controller/config/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_controller/config/tasks.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
🪛 Ruff (0.14.10)
openwisp_controller/config/tasks.py
147-147: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
openwisp_controller/config/tasks.py (1)
12-13: LGTM!The imports are correctly added to support the new centralized notification utilities.
openwisp_controller/config/utils.py (1)
3-3: LGTM!The imports are correctly added to support the new notification and caching functionality.
Also applies to: 9-10
…fications #1049
Modified tasks and tasks zerotier consistently with utils. Tested via manual testing.
Fixes #1049
Checklist
Reference to Existing Issue
Closes #1049
Description of Changes
Extended config/utils.py for sending api tasks' notification with fixing trigger_vpn_server_endpoint to send generic message notification for error and recovery.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.