fix(e2e): use backup target API if settings are removed #2230
Conversation
mantissahz
requested review from
COLDTURNIP,
innobead,
yangchiu,
derekbit,
c3y1huang,
chriscchien and
roger-ryao
WalkthroughThis pull request introduces enhancements to backup store management in the Longhorn testing framework. The changes focus on adding utility methods for converting Kubernetes duration strings, managing backup target configurations, and improving error handling. The modifications span two files: Changes
Sequence DiagramsequenceDiagram
participant Setting
participant BackupStore
participant LonghornClient
Setting->>BackupStore: Initialize
Setting->>LonghornClient: Attempt to set backup target
alt Setting not supported
LonghornClient-->>Setting: Error
Setting->>BackupStore: Fallback to set URL/secret
else Setting supported
LonghornClient->>Setting: Update successful
end
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 (
|
![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 (9)
e2e/libs/setting/setting.py (3)
17-22: Consider renaming to avoid repetition
SETTING_SETTING_BACKUPSTORE_POLL_INTERVAL_NOT_SUPPORTEDhas an extra “SETTING_” prefix. Consider renaming to maintain consistency (e.g.SETTING_BACKUPSTORE_POLL_INTERVAL_NOT_SUPPORTED).
58-80: Double-check exception handling flow
Currently, only exceptions containingSETTING_BACKUP_TARGET_NOT_SUPPORTEDtrigger a fallback toBackupStore. Other exceptions are just logged and swallowed. If upstream code throws different error shapes (e.g., noerror.message), this may produce unhandled attribute errors.
107-115: Ensure robust exception checks
Like the prior method, any error unrelated toSETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTEDis only logged. The code may benefit from an explicit check to avoid potentialAttributeErrorone.error.message.e2e/libs/backupstore/base.py (6)
21-34: Regex-based duration parsing
This function properly converts K8s duration strings into seconds. Consider unit tests for edge cases or invalid suffixes.
67-79: Potential error handling improvement
All three getters rely on the default backup target object’s existence. If the default backup target is missing or invalid, you may want to handle that scenario more robustly.
80-87: Avoid partial update overhead
set_backupstore_urlretrieves and reuses the poll interval and credential secret from the existing backup target. Consider factoring out common logic if additional set_ methods follow the same pattern.
88-95: Credential secret update logic
Similar toset_backupstore_url, a possible improvement is to unify these partial updates under one helper function.
106-114: Bulk backup store update
update_backupstoreusesbackupTargetUpdatewith a default poll interval of300. Consider referencing a shared constant or environment variable to avoid hardcoded values if requirements change later.
160-196: Partial implementations
All methods inBackupStorereturnNotImplemented, indicating placeholders for future extension. Ensure new functionalities are tested once implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/libs/backupstore/base.py(3 hunks)e2e/libs/setting/setting.py(2 hunks)
🔇 Additional comments (10)
e2e/libs/setting/setting.py (3)
8-8: Import usage is valid
The BackupStore class is referenced below, so this import is appropriate and necessary.
26-26: Validate environment variable usage
BackupStore() is constructed directly here, but if the environment variable LONGHORN_BACKUPSTORE is not set or improperly formatted, this might cause unexpected errors. Consider adding fallback logic or warnings.
98-104: Improve fallback scenario
When an exception is caught, only backup target unsupported errors are handled. If the exception is raised for another reason, it is silently logged. Consider re-throwing or providing a clearer error response.
e2e/libs/backupstore/base.py (7)
4-4: Regex import is valid
import re is required by the duration-parsing function below.
8-8: Client import is appropriate
get_longhorn_client is used throughout for retrieving and updating backup targets.
10-12: Readable time units
Defining SECOND, MINUTE, and HOUR is clean and self-documenting.
14-18: Clear mapping for units
The duration_u dict design is straightforward. Just ensure no future expansions (e.g., days) are needed.
38-39: Default target setting
DEFAULT_BACKUPTARGET is set to "default". Ensure it matches the desired initial fallback behavior of the system.
96-102: Poll interval usage
poll_interval is expected in seconds here rather than a raw K8s duration. Confirm that calling code is adequately documented or validated so users pass a numeric value.
103-105: Reset method
reset_backupstore sets everything to defaults via update_backupstore(). This is straightforward and consistent with other logic.
| try: | ||
| self.update_setting(self.SETTING_BACKUP_TARGET, "", retry=False) | ||
| self.update_setting(self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET, | ||
| "", | ||
| retry=False) | ||
| self.update_setting(self.SETTING_BACKUPSTORE_POLL_INTERVAL, | ||
| "300", | ||
| retry=False) | ||
| except Exception as e: | ||
| if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in e.error.message: | ||
| self.backupstore.reset_backupstore() | ||
| else: | ||
| logging(e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fallback logic might need expansion
The reset_backupstore method handles only the backup target not supported scenario. If the credential secret or poll interval settings are also not supported, they might not reset properly here.
ref: longhorn/longhorn 5411, 10043 Signed-off-by: James Lu <[email protected]>
derekbit
force-pushed
the
issue10043-e2e
branch
from
be14dbb to
bfc6ee5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
e2e/libs/backupstore/base.py (2)
21-33: Enhance duration conversion edge case handlingThe duration conversion utility works for basic formats but could be improved to handle edge cases:
- Negative durations (e.g., "-1h")
- Decimal values (e.g., "1.5h")
- Empty string input
- Multiple occurrences of same unit (e.g., "1h2h")
def from_k8s_duration_to_seconds(duration_s): + if not duration_s: + raise ValueError("Empty duration string") + if duration_s.startswith("-"): + raise ValueError("Negative durations not supported") + total = 0 pattern_str = r'([0-9]+)([smh]+)' pattern = re.compile(pattern_str) matches = pattern.findall(duration_s) if not len(matches): - raise Exception("Invalid duration {}".format(duration_s)) + raise ValueError(f"Invalid duration format: {duration_s}") + + seen_units = set() for v, u in matches: + if u in seen_units: + raise ValueError(f"Duplicate unit '{u}' in duration") + seen_units.add(u) total += int(v) * duration_u[u] return total
67-70: Add error handling for backup target operationsThe backup target URL operations should include error handling to gracefully handle API failures.
def get_backupstore_url(self): - return get_longhorn_client().by_id_backupTarget( - self.DEFAULT_BACKUPTARGET).backupTargetURL + try: + return get_longhorn_client().by_id_backupTarget( + self.DEFAULT_BACKUPTARGET).backupTargetURL + except Exception as e: + raise ValueError(f"Failed to get backup target URL: {str(e)}") def set_backupstore_url(self, url): - bt = get_longhorn_client().by_id_backupTarget( - self.DEFAULT_BACKUPTARGET) - p_interval = from_k8s_duration_to_seconds(bt.pollInterval) - self.update_backupstore(url=url, - credential_secret=bt.credentialSecret, - poll_interval=p_interval) + try: + bt = get_longhorn_client().by_id_backupTarget( + self.DEFAULT_BACKUPTARGET) + p_interval = from_k8s_duration_to_seconds(bt.pollInterval) + self.update_backupstore(url=url, + credential_secret=bt.credentialSecret, + poll_interval=p_interval) + except Exception as e: + raise ValueError(f"Failed to set backup target URL: {str(e)}")Also applies to: 80-87
e2e/libs/setting/setting.py (2)
58-80: Improve error handling granularityThe error handling could be more specific to handle different types of settings separately and provide better error messages.
try: self.update_setting(self.SETTING_BACKUP_TARGET, backupsettings[0], retry=False) if len(backupsettings) > 1: self.update_setting( self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET, backupsettings[1], retry=False) self.update_setting(self.SETTING_BACKUPSTORE_POLL_INTERVAL, poll_interval, retry=False) except Exception as e: - if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in e.error.message: + error_msg = getattr(e, 'error', e).message + if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in error_msg: self.backupstore.set_backupstore_url(backupsettings[0]) if len(backupsettings) > 1: self.backupstore.set_backupstore_secret( backupsettings[1]) self.backupstore.set_backupstore_poll_interval( poll_interval) + elif self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTED in error_msg: + logging(f"Warning: Credential secret setting not supported: {error_msg}") + if len(backupsettings) > 1: + self.backupstore.set_backupstore_secret(backupsettings[1]) else: - logging(e) + logging(f"Error setting backup store: {str(e)}")
97-115: Enhance error handling and logging in getter methodsThe getter methods could benefit from more informative logging and consistent error handling.
def get_backup_target(self): try: - return self.get_setting(self.SETTING_BACKUP_TARGET) + value = self.get_setting(self.SETTING_BACKUP_TARGET) + logging(f"Retrieved backup target from settings: {value}") + return value except Exception as e: - if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in e.error.message: - return self.backupstore.get_backupstore_url() + error_msg = getattr(e, 'error', e).message + if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in error_msg: + value = self.backupstore.get_backupstore_url() + logging(f"Retrieved backup target from backup store: {value}") + return value else: - logging(e) + logging(f"Error getting backup target: {str(e)}") + raise def get_secret(self): try: - return self.get_setting( + value = self.get_setting( self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET) + logging(f"Retrieved secret from settings") + return value except Exception as e: - if (self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTED - in e.error.message): - return self.backupstore.get_backupstore_secret() + error_msg = getattr(e, 'error', e).message + if (self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTED + in error_msg): + value = self.backupstore.get_backupstore_secret() + logging(f"Retrieved secret from backup store") + return value else: - logging(e) + logging(f"Error getting secret: {str(e)}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/libs/backupstore/base.py(3 hunks)e2e/libs/setting/setting.py(2 hunks)
🔇 Additional comments (1)
e2e/libs/setting/setting.py (1)
83-95: Expand fallback logic for all unsupported settings
The reset_backupstore method only handles the backup target not supported scenario. Consider handling cases where credential secret or poll interval settings are not supported.
try:
self.update_setting(self.SETTING_BACKUP_TARGET, "", retry=False)
self.update_setting(self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET,
"",
retry=False)
self.update_setting(self.SETTING_BACKUPSTORE_POLL_INTERVAL,
"300",
retry=False)
except Exception as e:
- if self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in e.error.message:
+ error_msg = getattr(e, 'error', e).message
+ if (self.SETTING_BACKUP_TARGET_NOT_SUPPORTED in error_msg or
+ self.SETTING_BACKUP_TARGET_CREDENTIAL_SECRET_NOT_SUPPORTED in error_msg or
+ self.SETTING_SETTING_BACKUPSTORE_POLL_INTERVAL_NOT_SUPPORTED in error_msg):
self.backupstore.reset_backupstore()
else:
logging(e)| class BackupStore(Base): | ||
|
|
||
| def __init__(self): | ||
| super().__init__() | ||
|
|
||
| def get_backup_volume_prefix(self, volume_name): | ||
| return NotImplemented | ||
|
|
||
| def get_backup_cfg_file_path(self, volume_name, backup_name): | ||
| return NotImplemented | ||
|
|
||
| def get_volume_cfg_file_path(self, volume_name): | ||
| return NotImplemented | ||
|
|
||
| def get_backup_blocks_dir(self, volume_name): | ||
| return NotImplemented | ||
|
|
||
| def create_file_in_backupstore(self): | ||
| return NotImplemented | ||
|
|
||
| def write_backup_cfg_file(self, volume_name, backup_name, data): | ||
| return NotImplemented | ||
|
|
||
| def delete_file_in_backupstore(self): | ||
| return NotImplemented | ||
|
|
||
| def delete_backup_cfg_file(self): | ||
| return NotImplemented | ||
|
|
||
| def delete_volume_cfg_file(self): | ||
| return NotImplemented | ||
|
|
||
| def delete_random_backup_block(self): | ||
| return NotImplemented | ||
|
|
||
| def count_backup_block_files(self): | ||
| return NotImplemented |
There was a problem hiding this comment.
Fix abstract method implementations
The BackupStore class incorrectly uses NotImplemented constant instead of raising NotImplementedError. When implementing abstract methods, you should either provide a concrete implementation or raise NotImplementedError.
def get_backup_volume_prefix(self, volume_name):
- return NotImplemented
+ raise NotImplementedError("Method not implemented")
def get_backup_cfg_file_path(self, volume_name, backup_name):
- return NotImplemented
+ raise NotImplementedError("Method not implemented")
# Apply similar changes to other methods...Committable suggestion skipped: line range outside the PR's diff.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue # ref: 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