Dev: sbd: Check and fix deprecated properties

[liangxin1300]

Description

This PR updates crmsh to use Pacemaker replacement cluster-property names by default and adds deprecated-property handling to the SBD health check/fix flow.

It reverts bootstrap-side use of deprecated stonith-* names, discovers configured deprecated cluster properties from Pacemaker metadata, warns when deprecated properties are still present, and migrates or removes them during crm cluster health sbd --fix.

Changes

• Use fencing-* properties in bootstrap, SBD, qdevice, crash-test, UI, feature tests, and unit tests instead of deprecated stonith-* names.
• Hide deprecated RA parameters from configure completion and property listings while keeping metadata available for translation.
• Add an SBD deprecated-property check/fix item to SBDConfigChecker.
• Discover configured deprecated properties dynamically from Pacemaker metadata.
• Split RA helpers into predictable active, deprecated, and deprecated-mapping helpers.
• Add raw property helpers for migration I/O without recursive translation.
• Add translator checks that return success/warning state and use quiet-aware warning output.
• Keep deprecated configured terms when Pacemaker metadata has no replacement term.
• Add unit coverage for deprecated-property discovery, SBD check/fix behavior, raw migration paths, no-replacement handling, and missing-value handling.

Fix policy for deprecated properties

utils.DeprecatedTermTranslator.fix() applies this policy when the deprecated term is configured and the running cluster uses deprecated properties:

• If the replacement term is also configured, delete the deprecated term because Pacemaker ignores it.
• If a replacement term exists, copy the deprecated value to the replacement term with raw property I/O, then delete the deprecated term.

Deprecated terms without a replacement are kept unchanged.

Notes

• get_property() and set_property() remain the translated public wrappers.
• _get_raw_property() and _set_raw_property() are used where the exact property name must be addressed during migration.

[codecov]

Codecov Report

❌ Patch coverage is 71.59091% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.55%. Comparing base (849ba1d) to head (55991e2).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
crmsh/utils.py	77.77%	12 Missing ⚠️
crmsh/ra.py	41.66%	7 Missing ⚠️
crmsh/sbd.py	80.00%	3 Missing ⚠️
crmsh/ui_configure.py	25.00%	3 Missing ⚠️

Additional details and impacted files

Flag	Coverage Δ
integration	55.26% <44.31%> (-0.08%)	⬇️
unit	52.67% <67.04%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crmsh/bootstrap.py	87.24% <100.00%> (+<0.01%)	⬆️
crmsh/crash_test/utils.py	80.15% <100.00%> (ø)
crmsh/qdevice.py	94.77% <100.00%> (ø)
crmsh/sbd.py	77.63% <80.00%> (+0.46%)	⬆️
crmsh/ui_configure.py	49.83% <25.00%> (ø)
crmsh/ra.py	58.50% <41.66%> (-0.34%)	⬇️
crmsh/utils.py	64.36% <77.77%> (-0.40%)	⬇️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nicholasyang2022]

Thank you for this PR! Moving towards dynamic, metadata-driven discovery for deprecated properties is a fantastic architectural improvement that will make crmsh much more resilient to upstream Pacemaker changes.

However, during review, I identified a few critical safety concerns regarding how the migration (--fix) is handled, as well as some API design issues that need to be addressed before this can be merged.

Critical/Blocking Issues (Safety)

1. Unsafe Scope of SBD Health Check (crm cluster health sbd --fix)
Currently, utils.get_all_configured_deprecated_properties() fetches all deprecated properties defined in the Pacemaker metadata.

• The Risk: Running crm cluster health sbd --fix will now indiscriminately check, migrate, and potentially delete any deprecated cluster property (e.g., legacy quorum policies, placement strategies) across the entire cluster, not just those related to SBD or fencing.
• Recommendation: Please scope the dynamic discovery strictly to properties relevant to the SBD module. For instance, filter the dynamically discovered list to only include properties starting with stonith- or fencing-, or pass a targeted prefix list to the utility.

2. Unsafe Deletion of Properties Without Replacements
In utils.py, DeprecatedTermTranslator.fix() automatically deletes properties that have no replacement (res.new_term is None):

        if res.using_deprecated and res.deprecated_configured:
            # ...
            else:
                delete_property(res.deprecated_term)

• The Risk: Automatically deleting a configured property just because it is marked deprecated (without a replacement) is highly dangerous. An administrator may rely on that legacy property for specific behavior.
• Recommendation: If a property has no replacement, the --fix command should not delete it. It is perfectly fine to log a warning during the check phase, but destructive actions without a clear migration path must be avoided to prevent breaking the cluster.

API Design & Maintainability

3. The direct Flag in get_property / set_property
Introducing direct=True to bypass the auto-translation logic solves the infinite recursion bug, but using a boolean flag to turn off core business logic is a code smell. Since most of the codebase expects these functions to be "smart" (auto-translating), we shouldn't clutter the signature.

• Recommendation: Instead of a direct flag, extract the core I/O logic into explicit "raw" or "dumb" functions (e.g., _get_raw_property and _set_raw_property). The migration code in DeprecatedTermTranslator.fix() can safely call these raw functions directly. The public get_property/set_property functions can remain as "smart" wrappers that handle translation and then call the raw functions.

4. Predictability of RAInfo.get_all_params_dict()
The boolean flags in def get_all_params_dict(self, deprecated=False, replaced_with=False) make the return type unpredictable.

• The Issue: Depending on the flags, it either returns dict[str, dict] (full parameter metadata) or dict[str, str|None] (mapping a name to its replacement). Furthermore, if deprecated=False and replaced_with=True, the replaced_with flag is silently ignored. This breaks type hinting and readability.
• Recommendation: Remove the boolean flags and split this into explicit, single-purpose properties/methods that return consistent types. For example:
  • get_active_params(self) -> dict
  • get_deprecated_params(self) -> dict
  • get_deprecated_mapping(self) -> dict[str, str|None]

5. Missing Null-Check on Value Migration
In DeprecatedTermTranslator.fix():

            elif res.new_term:
                value = get_property(res.deprecated_term, get_default=False, direct=True)
                set_property(res.new_term, value, direct=True)

• Recommendation: If get_property returns None (e.g., deleted concurrently), set_property will literally set the value to the string "None". Please wrap this in a quick null-check: if value is not None: before setting the new property.

---

Overall, the foundation of this PR is very strong. Addressing these scope and API design issues will make this a robust addition to the codebase. Let me know if you have any questions!

[liangxin1300]

Thank you for this PR! Moving towards dynamic, metadata-driven discovery for deprecated properties is a fantastic architectural improvement that will make crmsh much more resilient to upstream Pacemaker changes.

However, during review, I identified a few critical safety concerns regarding how the migration (--fix) is handled, as well as some API design issues that need to be addressed before this can be merged.

Critical/Blocking Issues (Safety)

1. Unsafe Scope of SBD Health Check (crm cluster health sbd --fix) Currently, utils.get_all_configured_deprecated_properties() fetches all deprecated properties defined in the Pacemaker metadata.

• The Risk: Running crm cluster health sbd --fix will now indiscriminately check, migrate, and potentially delete any deprecated cluster property (e.g., legacy quorum policies, placement strategies) across the entire cluster, not just those related to SBD or fencing.
• Recommendation: Please scope the dynamic discovery strictly to properties relevant to the SBD module. For instance, filter the dynamically discovered list to only include properties starting with stonith- or fencing-, or pass a targeted prefix list to the utility.

2. Unsafe Deletion of Properties Without Replacements In utils.py, DeprecatedTermTranslator.fix() automatically deletes properties that have no replacement (res.new_term is None):

        if res.using_deprecated and res.deprecated_configured:
            # ...
            else:
                delete_property(res.deprecated_term)

• The Risk: Automatically deleting a configured property just because it is marked deprecated (without a replacement) is highly dangerous. An administrator may rely on that legacy property for specific behavior.
• Recommendation: If a property has no replacement, the --fix command should not delete it. It is perfectly fine to log a warning during the check phase, but destructive actions without a clear migration path must be avoided to prevent breaking the cluster.

API Design & Maintainability

3. The direct Flag in get_property / set_property Introducing direct=True to bypass the auto-translation logic solves the infinite recursion bug, but using a boolean flag to turn off core business logic is a code smell. Since most of the codebase expects these functions to be "smart" (auto-translating), we shouldn't clutter the signature.

• Recommendation: Instead of a direct flag, extract the core I/O logic into explicit "raw" or "dumb" functions (e.g., _get_raw_property and _set_raw_property). The migration code in DeprecatedTermTranslator.fix() can safely call these raw functions directly. The public get_property/set_property functions can remain as "smart" wrappers that handle translation and then call the raw functions.

4. Predictability of RAInfo.get_all_params_dict() The boolean flags in def get_all_params_dict(self, deprecated=False, replaced_with=False) make the return type unpredictable.

• The Issue: Depending on the flags, it either returns dict[str, dict] (full parameter metadata) or dict[str, str|None] (mapping a name to its replacement). Furthermore, if deprecated=False and replaced_with=True, the replaced_with flag is silently ignored. This breaks type hinting and readability.

• Recommendation: Remove the boolean flags and split this into explicit, single-purpose properties/methods that return consistent types. For example:

  • get_active_params(self) -> dict
  • get_deprecated_params(self) -> dict
  • get_deprecated_mapping(self) -> dict[str, str|None]

5. Missing Null-Check on Value Migration In DeprecatedTermTranslator.fix():

            elif res.new_term:
                value = get_property(res.deprecated_term, get_default=False, direct=True)
                set_property(res.new_term, value, direct=True)

• Recommendation: If get_property returns None (e.g., deleted concurrently), set_property will literally set the value to the string "None". Please wrap this in a quick null-check: if value is not None: before setting the new property.

Overall, the foundation of this PR is very strong. Addressing these scope and API design issues will make this a robust addition to the codebase. Let me know if you have any questions!

Changed items 3, 4, and 5.
Items 1 and 2 need discussion.

[liangxin1300]

• Need to complete some test cases in deprecated_term.feature