fix: pytest test cases for removing global backup settings. #2229
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces comprehensive changes to backup store management across multiple files in the Longhorn project. The modifications enhance the functionality of backup store configuration, error handling, and testing. New utility functions and methods are added to convert Kubernetes duration strings, manage backup store URLs, secrets, and polling intervals. The changes improve the robustness of backup settings by introducing more flexible error handling mechanisms and providing fallback options when certain settings are not supported. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mantissahz
force-pushed
the
issue10043
branch
from
f218613 to
bcf60b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
manager/integration/tests/backupstore.py (1)
Line range hint
1798-1798: Use of.strip("nfs://").As per static analysis hint, using
.strip("nfs://")is misleading becausestripremoves characters from the ends in any order, not a specific substring. Consider using.removeprefix("nfs://")in Python 3.9+ or a slice approach:if backupstore.startswith("nfs://"): nfs_url = backupstore[len("nfs://"):]
🧹 Nitpick comments (13)
e2e/libs/setting/setting.py (5)
8-8: Consider referencing the newly introducedBackupStoreclass usage in docstrings.It's good practice to mention any significant class dependencies or usage patterns in docstrings to help maintainers quickly understand the relationship.
58-67: Error handling refinement.In
set_backupstore, capturing exceptions for unsupported settings is a good approach. Consider logging the exact exception or re-raising with added context if deeper debugging is needed.
80-80: Consider consistent logging.You may want to match the logging format used elsewhere (e.g.,
logging("[INFO] message")). Consistency aids debugging.
92-95: Error handling consistency.Using an
if ... in e.error.message:approach is fine, but ensure you have coverage for corner cases where the error message differs or theerror.messageis empty.
102-105: Potential user confusion on callingreset_backupstore().If the user is unaware that reset is triggered by an unsupported setting, consider adding a docstring or log statement clarifying that scenario.
manager/integration/tests/backupstore.py (3)
33-33: Ensuring thorough cleanup.After deleting a backup volume, confirm references in other data structures are cleaned up as well.
56-61: Duration dictionary for k8s strings.
duration_umapping is typical. Consider a docstring for clarity and potential expansions if new suffixes appear (e.g., "d" for days).
132-133: Care with inlined calls.
reset_backupstore_setting_by_setting(client)might benefit from an explicit docstring. Since it’s used in multiple places, clarity helps.manager/integration/tests/test_basic.py (1)
126-127: AddingSETTING_BACKUP_TARGET_NOT_SUPPORTED.Ensures that test scenarios handle the possibility of an unsupported backup target.
e2e/libs/backupstore/base.py (2)
67-70:get_backupstore_urlusage.Good approach to unify how the default backup target is retrieved.
103-105:reset_backupstore: usage disclaimers.If tests rely on a default backup target, watch out for concurrency with other tests.
manager/integration/tests/test_settings.py (2)
1181-1191: Check the handling ofSETTING_BACKUP_TARGET_NOT_SUPPORTED.When validation fails, this logic waits for the URL update based on an exception message. Consider verifying the exception type instead of matching strings to avoid false positives if the error message changes.
1241-1252: Consolidate exception handling to avoid duplication.The pattern of catching exceptions and checking for
SETTING_BACKUP_TARGET_NOT_SUPPORTEDis repeated. You could factor out common logic to keep the code DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/libs/backupstore/base.py(3 hunks)e2e/libs/setting/setting.py(2 hunks)manager/integration/tests/backupstore.py(14 hunks)manager/integration/tests/test_basic.py(5 hunks)manager/integration/tests/test_ha.py(2 hunks)manager/integration/tests/test_settings.py(4 hunks)manager/integration/tests/test_system_backup_restore.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
manager/integration/tests/test_basic.py
1798-1798: Using .strip() with multi-character strings is misleading
(B005)
🔇 Additional comments (37)
e2e/libs/setting/setting.py (6)
17-22: Appropriate constant usage for unsupported settings.
Defining constants for unsupported settings and referencing them in code helps maintain clarity. Ensure to keep them aligned with any similar constants in use for consistent naming.
26-26: Initialization of self.backupstore.
Storing a dedicated BackupStore instance simplifies code usage and fosters an object-oriented approach to backup operations.
68-71: Check poll interval usage.
Ensure that the poll interval matches the environment variable or global setting across all test scenarios. If there's logic to override values, add clarifications.
72-79: Robust fallback logic to handle backup target not supported.
Catching the exception and using the fallback to self.backupstore.set_backupstore_url() and set_backupstore_secret() improves the resiliency of the code. Great job.
83-91: Potential edge case with reset logic.
When resetting the backup store, ensure that any concurrency or in-use scenario with other tests is properly handled.
98-101: Ensure fallback default poll interval usage.
If the requested poll interval is not supported, confirm we’re reverting to the correct default of 300.
manager/integration/tests/backupstore.py (9)
2-3: Imports for re and json.
These imports facilitate regex-based checks and serialization. Everything looks consistent with usage.
18-19: Shorter retry intervals are introduced.
Double-check that RETRY_COUNTS_SHORT and RETRY_INTERVAL align with test performance expectations. If they are too short, tests may flake.
41-42: Good practice: specialized constants for settings.
Declaring SETTING_BACKUP_TARGET_NOT_SUPPORTED clarifies usage and keeps the code style consistent with other constants for unsupported settings.
43-44: Similarly named constants.
Mirroring the pattern for SETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTED is a good approach.
45-46: SETTING_SETTING_BACKUPSTORE_POLL_INTERVAL_NOT_SUPPORTED usage.
Naming consistency among these constants helps maintain uniformity in error handling logic.
52-55: Time unit definitions.
Constants for SECOND, MINUTE, and HOUR improve code readability and reduce magic numbers.
63-76: Regex usage in from_k8s_duration_to_seconds.
Well-structured approach to parse K8s durations. Be mindful that corner cases like floating numbers or uppercase letters might arise in extended usage.
79-79: Fixture usage.
backupstore_invalid(client) fixture helps to set up invalid conditions. Ensure all references are updated if fixture names or usage changes.
124-131: reset_backupstore_setting function.
Catching exceptions and checking for SETTING_BACKUP_TARGET_NOT_SUPPORTED is consistent with the fallback logic used in e2e libs. Great consistency.
manager/integration/tests/test_basic.py (2)
108-108: Importing SETTING_BACKUP_TARGET.
Centralized usage of this constant aids consistency across tests.
119-122: Imported backupstore functions.
Using direct references to the relevant backupstore helper functions keeps the tests DRY, but watch for name collisions or overshadowing.
e2e/libs/backupstore/base.py (13)
4-4: Regex usage for durations.
The re import is needed for from_k8s_duration_to_seconds. Ensure no global patterns conflict.
8-9: Longhorn client import.
The direct import from utility.utility suggests a multi-layered utility approach. Validate that it remains consistent if the utility structure changes.
10-13: Time constants.
Similar to other files, consistent naming (SECOND, MINUTE, HOUR) helps unify code style.
14-19: duration_u dict.
Expandable dictionary for custom suffixes. Good if we foresee additional suffix expansions like “d” or “w.”
21-34: from_k8s_duration_to_seconds method.
Adequate usage of regex and error handling. Just ensure that no additional suffix is missing.
38-39: DEFAULT_BACKUPTARGET constant.
Central constant naming is consistent with references in other code.
71-74: get_backupstore_secret: DRY approach.
Similar pattern. Helps avoid duplication.
75-79: get_backupstore_poll_interval: environment vs setting alignment.
If the environment variable and setting mismatch, consider clarifying which source leads.
80-87: set_backupstore_url: immediate update.
No intermediate checks before calling update_backupstore might be okay if error handling is robust in update_backupstore.
88-95: set_backupstore_secret: consistent logic.
Mirrors set_backupstore_url usage.
96-102: set_backupstore_poll_interval: numeric vs string confusion.
poll_interval is cast to str in update_backupstore; ensure no float or non-numeric value is unintentionally passed.
106-114: update_backupstore: single method.
Centralizing the update logic is great. If adding more fields, keep arguments short or switch to a config object.
160-164: BackupStore constructor.
Inheriting from Base looks straightforward. Provide docstrings if adding specialized behaviors later.
manager/integration/tests/test_system_backup_restore.py (2)
40-40: Use of set_backupstore_poll_interval.
Replacing direct calls to update_setting with set_backupstore_poll_interval fosters clarity. Great to see its unified usage.
378-378: Short poll interval usage.
Setting poll interval to 10 may accelerate tests but could risk race conditions. Thoroughly test for flakiness.
manager/integration/tests/test_settings.py (3)
36-36: Ensure the new constant is appropriately utilized.
The introduction of RETRY_COUNTS_SHORT helps reduce waiting time in scenarios where only a few attempts are necessary. Please confirm all relevant test functions utilize this new constant correctly and consistently.
50-51: Verify the import usage for unsupported backup targets.
These newly introduced imports are needed to handle unsupported backup target scenarios. Ensure calls to these constants/functions are tested thoroughly to prevent unhandled exceptions or logic regressions.
1193-1201: Validate the retry loop for updating the backup target URL.
This retry uses RETRY_COUNTS_SHORT and breaks immediately after finding the matching URL. Confirm this loop consistently achieves the desired reliability and does not introduce unnecessary delays.
manager/integration/tests/test_ha.py (2)
93-94: Validate new imports for backup target fallback.
Imports for SETTING_BACKUP_TARGET_NOT_SUPPORTED and set_backupstore_url indicate new fallback logic. Ensure calls to these are properly orchestrated to maintain consistent backup target states.
1369-1377: Confirm consistent reporting of unsupported backup target errors.
Catching the exception and falling back to set_backupstore_url is sensible. Ensure the code properly differentiates between unsupported target vs. other update failures, so genuine errors are not silently masked.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
manager/integration/tests/test_ha.py (2)
Line range hint
12-24: Review the billing logic for customer satisfactionThe current implementation of the billing logic has potential customer satisfaction issues:
- Adding a flat $20 fee after applying the discount can result in a higher final bill than no discount for small purchases
- For example: A $100 purchase with 10% discount becomes $90 + $20 = $110, which is more than the original $100
Consider revising the fee structure to ensure discounts always benefit the customer.
Would you like me to propose an alternative implementation that better aligns with customer expectations?
Update requests library to version 2.32.3 to address security vulnerabilities
The current fixed version (2.26.0) has multiple security vulnerabilities that have been patched in newer versions:
- Version < 2.32.0: Session object verification vulnerability (MODERATE)
- Version < 2.31.0: Unintended leak of Proxy-Authorization header (MODERATE)
The latest stable version is 2.32.3, which includes all security patches. Please update the version requirement in setup.py.
🔗 Analysis chain
Line range hint
6-6: Verify the latest stable versions and any security advisoriesEnsure that the fixed version of the
requestslibrary is secure and free from vulnerabilities.Run the following script to verify the fixed version of the
requestslibrary:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the `requests` library. # Check PyPI for latest versions curl -s https://pypi.org/pypi/requests/json | jq '.info.version' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1597
🧹 Nitpick comments (5)
e2e/libs/backupstore/base.py (3)
10-18: LGTM! Consider enhancing documentation.The time unit constants and duration conversion implementation are correct. The regex pattern accurately matches Kubernetes duration format.
Consider adding docstring with examples:
def from_k8s_duration_to_seconds(duration_s): """Convert Kubernetes duration string to seconds. Args: duration_s (str): Duration string in format like "3h5m30s" Returns: int: Total seconds Raises: Exception: If duration format is invalid """Also applies to: 21-33
103-113: Consider adding error handling for the update operation.The implementation is good, but could benefit from error handling for the backupTargetUpdate operation.
Consider wrapping the update in a try-except:
def update_backupstore(self, url="", credential_secret="", poll_interval=300): backup_target = get_longhorn_client().by_id_backupTarget( self.DEFAULT_BACKUPTARGET) + try: backup_target.backupTargetUpdate(name=self.DEFAULT_BACKUPTARGET, backupTargetURL=url, credentialSecret=credential_secret, pollInterval=str(poll_interval)) + except Exception as e: + raise Exception(f"Failed to update backup target: {str(e)}")
160-196: Add class-level documentation.The BackupStore class implementation is correct but would benefit from documentation explaining its purpose and relationship to the Base class.
Add class docstring:
class BackupStore(Base): """Concrete implementation of Base class for backup store operations. This class provides a foundation for specific backup store implementations while maintaining the interface defined in the Base class. """e2e/libs/setting/setting.py (1)
58-80: Consider enhancing error logging.The error handling implementation is good, but could benefit from more detailed logging.
Add more context to error logging:
try: self.update_setting(self.SETTING_BACKUP_TARGET, backupsettings[0], retry=False) # ... other settings ... except Exception as e: if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in e.error.message: + logging(f"Settings not supported, falling back to API: {str(e)}") self.backupstore.set_backupstore_url(backupsettings[0]) # ... other settings ... else: + logging(f"Failed to set backupstore settings: {str(e)}") logging(e)manager/integration/tests/backupstore.py (1)
144-162: Consider adding timeout handling for the retry loop.The implementation is good but could benefit from timeout handling.
Add timeout handling:
+from datetime import datetime, timedelta + def set_backupstore_setting_by_api(client, url="", credential_secret="", poll_interval=300): bt = client.by_id_backupTarget(DEFAULT_BACKUPTARGET) bt.backupTargetUpdate(name=DEFAULT_BACKUPTARGET, backupTargetURL=url, credentialSecret=credential_secret, pollInterval=str(poll_interval)) + timeout = datetime.now() + timedelta(seconds=60) # 1-minute timeout for _ in range(RETRY_COUNTS_SHORT): + if datetime.now() > timeout: + raise Exception("Timeout waiting for backup target update") bt = client.by_id_backupTarget(DEFAULT_BACKUPTARGET) p_interval = from_k8s_duration_to_seconds(str(bt.pollInterval)) bt_url = str(bt.backupTargetURL) bt_secret = str(bt.credentialSecret) if bt_url == url and bt_secret == credential_secret and \ p_interval == int(poll_interval): break time.sleep(RETRY_INTERVAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/libs/backupstore/base.py(3 hunks)e2e/libs/setting/setting.py(2 hunks)manager/integration/tests/backupstore.py(14 hunks)manager/integration/tests/test_ha.py(2 hunks)
🔇 Additional comments (14)
e2e/libs/backupstore/base.py (4)
38-38: LGTM!
The DEFAULT_BACKUPTARGET constant is well-named and appropriately used throughout the code.
67-70: LGTM! Clean implementation of URL management.
The methods correctly handle backupstore URL operations while maintaining other settings.
Also applies to: 80-87
71-74: LGTM! Consistent secret management implementation.
The methods follow the same pattern as URL management, ensuring proper handling of credential secrets.
Also applies to: 88-95
75-78: LGTM! Well-implemented poll interval management.
The methods properly handle duration conversion and maintain consistency with other settings management.
Also applies to: 96-102
e2e/libs/setting/setting.py (3)
8-8: LGTM! Well-structured error message constants.
The import and error message constants are appropriately defined and follow good naming conventions.
Also applies to: 17-22
83-95: LGTM! Consistent error handling pattern.
The reset implementation follows the same robust error handling pattern as set_backupstore.
98-104: LGTM! Well-structured getter methods.
The getter methods maintain consistency in error handling and properly fall back to API methods when needed.
Also applies to: 107-115
manager/integration/tests/backupstore.py (4)
2-3: LGTM! Consistent constant definitions.
The imports and constants are well-organized and maintain consistency with other files.
Also applies to: 18-19, 41-46
124-130: LGTM! Robust error handling.
The reset function properly handles unsupported settings with appropriate fallback.
358-359: LGTM! Improved cleanup handling.
The cleanup implementation properly waits for volume deletion and provides clear assertion messages.
Also applies to: 362-362, 369-370
481-489: LGTM! Consistent error handling pattern.
The utility functions maintain a consistent error handling pattern and properly fall back to API methods.
Also applies to: 493-503
manager/integration/tests/test_ha.py (3)
93-94: LGTM: New imports for backup store functionality
The new imports are correctly added and will be used by the error handling changes below.
Line range hint 4-4: Add unit tests as indicated by TODO comment
The TODO comment indicates missing tests for this function. Please ensure test coverage is added to verify the behavior with the additional parameter.
Would you like me to help generate unit tests for this function?
1369-1377: Improve error handling for backup target setting updates
The added try-except block provides a robust fallback mechanism when the backup target setting is not supported. This is a good defensive programming practice.
However, let's verify that this error handling change doesn't affect other parts of the codebase.
✅ Verification successful
Error handling for backup target setting is consistently implemented
Based on the search results, the error handling pattern for SETTING_BACKUP_TARGET_NOT_SUPPORTED is already consistently implemented across the codebase:
- In
manager/integration/tests/backupstore.py, multiple instances handle this error by falling back to alternative methods - In
manager/integration/tests/test_settings.py, similar error handling exists withwait_backup_target_url_updated - In
manager/integration/tests/test_basic.py, the error is properly caught and handled - In
e2e/libs/setting/setting.py, the error handling follows the same pattern with appropriate fallback mechanisms
The implementation in the reviewed code follows the established pattern in the codebase, making it consistent with existing error handling practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of backup target setting updates that might need similar error handling
# Search for direct backup target setting updates
rg -A 3 "by_id_setting.*SETTING_BACKUP_TARGET"
# Search for potential error handling of backup target settings
rg -A 3 "SETTING_BACKUP_TARGET_NOT_SUPPORTED"
Length of output: 9105
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Update pytest test cases. ref: longhorn/longhorn 5411, 10043 Signed-off-by: James Lu <[email protected]>
mantissahz
force-pushed
the
issue10043
branch
from
bcf60b3 to
6b9739a
Compare
mantissahz
changed the title
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#5411, longhorn/longhorn#10043
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores