feat(backup): separate backup settings from default settings #2235
Conversation
mantissahz
requested review from
yangchiu,
derekbit,
chriscchien and
roger-ryao
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (6)
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: 0
🧹 Nitpick comments (3)
manager/integration/tests/test_settings.py (3)
Line range hint
1045-1058: Improve flexibility of config_map_with_value.
The newdata_yaml_nameparameter increases versatility by letting callers specify different target YAML files. Consider adding a short docstring, explaining usage for debugging and maintainability.
1100-1126: Enhanced update_settings_via_configmap to support backupstore defaults.
Passingconfigmap_nameanddata_yaml_nameat runtime provides better extensibility. The reset logic is reasonable, but ensure the finalizer’s usage does not inadvertently overwrite custom user settings in certain test flows.
1190-1216: Conditional logic for selecting backupstore configmap.
The approach eliminates confusion if multiple configmaps exist. Confirm proper fallback when the backupstore configmap is absent. Consider logging actions or reason for fallback to help debugging in CI or support scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
manager/integration/tests/test_settings.py(8 hunks)
🔇 Additional comments (4)
manager/integration/tests/test_settings.py (4)
34-35: New constants for backup store credentials and poll interval.
These constants look correct and will help decouple backup-related settings from the broader default settings.
60-64: Separation of default settings and backupstore settings constants.
Defining dedicated ConfigMap names and YAML filenames for backupstore and default settings is a good move toward clarity and modularity.
1086-1095: Optional parameters introduced for configmap initialization.
Utilizing default values for configmap_name and data_yaml_name is beneficial. Ensure that callers needing backupstore-specific logic are consistently passing in the right arguments.
Line range hint 1252-1301: Handling invalid settings with backupstore configmap.
The logic correctly checks for the backupstore configmap and resets invalid settings when volumes are attached. Ensure that ignoring invalid updates does not mask critical configuration issues for other settings.
mantissahz
force-pushed
the
issue10089
branch
from
6f9c0fc to
07ef4b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
manager/integration/tests/test_settings.py (1)
Line range hint
1045-1058: Consider adding type hints for better maintainability.The function signatures have been updated correctly to support both default and backup store settings. Consider adding type hints to improve code maintainability and IDE support.
Example with type hints:
-def config_map_with_value(configmap_name, setting_names, setting_values, data_yaml_name="default-setting.yaml"): +def config_map_with_value(configmap_name: str, setting_names: list[str], setting_values: list[str], data_yaml_name: str = "default-setting.yaml") -> dict:Also applies to: 1086-1126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
manager/integration/tests/test_settings.py(8 hunks)
🔇 Additional comments (2)
manager/integration/tests/test_settings.py (2)
60-63: LGTM! Constants are well-defined.
The new constants clearly separate backup store settings from default settings, aligning with the PR objectives.
Line range hint 1190-1301: LGTM! Test cases handle both configmap scenarios.
The test cases have been updated to:
- Check for the existence of the backup store configmap
- Use the appropriate configmap based on availability
- Maintain proper cleanup through finalizers
mantissahz
force-pushed
the
issue10089
branch
3 times, most recently
from
dd5bbb5 to
f27f73e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
manager/integration/tests/test_settings.py (2)
1100-1126: Consider enhancing error handling for ConfigMap operations.While the function's signature update is good, consider adding error handling for ConfigMap operations to gracefully handle potential API failures.
def update_settings_via_configmap(core_api, client, setting_names, setting_values, request, configmap_name=DEFAULT_SETTING_CONFIGMAP_NAME, data_yaml_name=DEFAULT_SETTING_YAML_NAME): + try: configmap_body = config_map_with_value(configmap_name, setting_names, setting_values, data_yaml_name) core_api.patch_namespaced_config_map(name=configmap_name, namespace='longhorn-system', body=configmap_body) + except Exception as e: + print(f"Failed to update ConfigMap {configmap_name}: {str(e)}") + raise
Line range hint
1251-1299: Consider extracting ConfigMap existence check into a helper function.The ConfigMap existence check is duplicated. Consider extracting it into a reusable helper function.
+def get_backup_configmap_details(core_api): + """Helper function to determine which ConfigMap to use for backup settings.""" + lh_cms = core_api.list_namespaced_config_map(namespace='longhorn-system') + cm_names = [config_map.metadata.name for config_map in lh_cms.items] + if DEFAULT_RESOURCE_CONFIGMAP_NAME in cm_names: + return DEFAULT_RESOURCE_CONFIGMAP_NAME, DEFAULT_RESOURCE_YAML_NAME, True + return DEFAULT_SETTING_CONFIGMAP_NAME, DEFAULT_SETTING_YAML_NAME, False def test_setting_update_with_invalid_value_via_configmap(core_api, request): - backup_cm_created = False - lh_cms = core_api.list_namespaced_config_map(namespace='longhorn-system') - cm_names = [config_map.metadata.name for config_map in lh_cms.items] - if DEFAULT_RESOURCE_CONFIGMAP_NAME in cm_names: - backup_cm_created = True - bt_config_map_name = DEFAULT_RESOURCE_CONFIGMAP_NAME - bt_data_yaml_name = DEFAULT_RESOURCE_YAML_NAME + bt_config_map_name, bt_data_yaml_name, backup_cm_created = get_backup_configmap_details(core_api)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
manager/integration/tests/test_settings.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build images
- GitHub Check: Summary
🔇 Additional comments (4)
manager/integration/tests/test_settings.py (4)
60-63: LGTM! Well-defined constants for ConfigMap management.The constants follow proper naming conventions and clearly define the ConfigMap and YAML file names used for settings management.
Line range hint
1045-1058: LGTM! Well-structured function with backward compatibility.The function has been enhanced to support configurable YAML file names while maintaining backward compatibility through default parameters.
1086-1095: LGTM! Improved initialization function with configurable parameters.The function now supports customizable ConfigMap and YAML file names, aligning with the PR's objective of separating settings.
1190-1198: LGTM! Good implementation of ConfigMap selection logic.The code appropriately checks for the existence of the default resource ConfigMap and selects the correct ConfigMap and YAML names.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
manager/integration/tests/test_settings.py (2)
1100-1126: Consider adding error handling for ConfigMap operations.While the function correctly handles different ConfigMap scenarios, it would be beneficial to add error handling for ConfigMap operations to gracefully handle failures.
def update_settings_via_configmap(core_api, client, setting_names, setting_values, request, configmap_name=DEFAULT_SETTING_CONFIGMAP_NAME, data_yaml_name=DEFAULT_SETTING_YAML_NAME): + try: configmap_body = config_map_with_value(configmap_name, setting_names, setting_values, data_yaml_name) core_api.patch_namespaced_config_map(name=configmap_name, namespace='longhorn-system', body=configmap_body) + except Exception as e: + print(f"Failed to update ConfigMap {configmap_name}: {str(e)}") + raise
Line range hint
1251-1299: Consider adding test coverage for backup settings validation.The test case handles the backup ConfigMap scenario, but it would be beneficial to add specific test cases for validating backup-related settings.
Consider adding test cases to validate:
- Invalid backup target credentials
- Invalid backup store poll interval values
- Interaction between backup settings when updated simultaneously
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
manager/integration/tests/test_settings.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build images
- GitHub Check: Summary
🔇 Additional comments (5)
manager/integration/tests/test_settings.py (5)
34-35: LGTM!The new imports for backup-related settings align with the PR objective of separating backup settings from default settings.
60-63: LGTM!The new constants are well-named and align with the PR objective of separating backup settings into a separate ConfigMap.
Line range hint
1045-1058: LGTM!The function modification maintains backward compatibility while supporting the new ConfigMap structure through the optional
data_yaml_nameparameter.
1086-1095: LGTM!The function now supports configurable ConfigMap and YAML names, aligning with the separation of backup settings.
1190-1215: LGTM!The test case correctly handles the presence of both ConfigMap types and uses the appropriate one for backup settings.
Separate the default backup related settings from default settings into a new yaml file `default-resource.yaml` Introduce a new category `defaultBackupStore` and a new ConfigMap `longhorn-default-resouce` to control the default backupstore related settings with Helm/Kubectl. ref: longhorn/longhorn 5411, 10089 Signed-off-by: James Lu <jamesluhz@gmail.com>
|
This PR will fix the failed test cases https://ci.longhorn.io/job/public/job/v1.8.x/job/v1.8.x-longhorn-tests-sles-arm64/17/testReport/ Hi @chriscchien, @roger-ryao @yangchiu, |
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#5411, longhorn/longhorn#10089
What this PR does / why we need it:
Separate the default backup related settings from default settings into a new yaml file
default-backupstore-setting.yamlIntroduce a new category
defaultBackupStoreSettingsand a new ConfigMaplonghorn-default-backupstore-settingsto control the default backupstore related settings with Helm/Kubectl.Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit